Lpc 11u6x pinctrl #44843

danieldegrasse · 2022-04-13T17:41:31Z

Enable pin control on LPC11u6x SOCs. The following boards have been converted:

faze
lpcxpresso11u68

~~This PR is dependent on #44842, as the LPC11u6x port configuration registers are not contiguous, so calculating offsets using the port and pin is not possible.~~

danieldegrasse · 2022-04-13T17:46:04Z

@mbittan I am unsure if you are still active on Github, but could you test this PR against an LPCxpresso11u68 if possible?

The project has a mandate to move from pinmux APIs to Linux style pin control APIs to handle pin selection by the next release, hence these changes. I do not currently have hardware to test on, so I can only verify that this patch builds for the faze and lpcxpresso11u68 targets. If the serial port input/output works correctly, as well as an application using I2C, then this PR likely works fine on hardware.

mbittan · 2022-04-14T09:43:48Z

@mbittan I am unsure if you are still active on Github, but could you test this PR against an LPCxpresso11u68 if possible?

The project has a mandate to move from pinmux APIs to Linux style pin control APIs to handle pin selection by the next release, hence these changes. I do not currently have hardware to test on, so I can only verify that this patch builds for the faze and lpcxpresso11u68 targets. If the serial port input/output works correctly, as well as an application using I2C, then this PR likely works fine on hardware.

I will test this as soon as possible

mbittan

I tested this PR on a lpcxpresso11u68 board: everything is ok except UART which does not work. The changes I suggest fix the issue on both boards.

mbittan · 2022-04-15T16:45:33Z

drivers/serial/uart_lpc11u6x.c

clkid must remain to bring the UART controller out of reset

mbittan · 2022-04-15T16:46:05Z

drivers/serial/uart_lpc11u6x.c

Same here, clkid is still necessary

mbittan · 2022-04-15T17:17:02Z

drivers/serial/uart_lpc11u6x.c

We can't use the instance number for this driver because all the UART controllers of the LPC11U6X are not the same.
This should work:

Suggested change

PINCTRL_DT_INST_DEFINE(idx); \

PINCTRL_DT_DEFINE(DT_NODELABEL(uart##idx)); \

mbittan · 2022-04-15T17:20:05Z

drivers/serial/uart_lpc11u6x.c

The instance number can't be used here either, this should work:

Suggested change

.pincfg = PINCTRL_DT_INST_DEV_CONFIG_GET(idx), \

.pincfg = PINCTRL_DT_DEV_CONFIG_GET(DT_NODELABEL(uart##idx)), \

mbittan · 2022-04-15T17:21:45Z

drivers/serial/uart_lpc11u6x.h

This can be removed too

danieldegrasse · 2022-04-18T16:58:21Z

@mbittan I've implemented your changes, but when testing using the LPCXpresso11u68 EVK we are not seeing output on the UART4 pins. This issue exists in this PR, and on the main branch. Is there a step we are missing in testing the Zephyr enablement for this board? Also, what was the reason for selecting UART4 as the primary Zephyr console, instead of UART0, which is connected to the onboard debugger?

mbittan · 2022-04-19T15:56:50Z

@mbittan I've implemented your changes, but when testing using the LPCXpresso11u68 EVK we are not seeing output on the UART4 pins. This issue exists in this PR, and on the main branch. Is there a step we are missing in testing the Zephyr enablement for this board?

UART4 should work without any specific configuration. For example, the hello_world sample works correctly on my board (which is an LPCxpressoV2 RevC) with your updated patches and on the main branch. What revision of the board are you using ? How are you testing the UART4 on your board ?

Also, what was the reason for selecting UART4 as the primary Zephyr console, instead of UART0, which is connected to the onboard debugger?

We chose UART4 because it was accessible on the arduino connector, but I agree that it makes more sense to use UART0.

Regarding the LPC11U6*-pinctrl.h files, I think it would be better it they were located in the zephyr tree. That would allow the lpc11u6x port to stay independent of the NXP HAL.

simonguinot · 2022-04-20T09:51:20Z

Regarding the LPC11U6*-pinctrl.h files, I think it would be better it they were located in the zephyr tree. That would allow the lpc11u6x port to stay independent of the NXP HAL.

Hi @danieldegrasse.

I agree with Maxime that there may be rewards (don't ask me which ones, I'd be embarrassed) to keep the lpc11u6x support standalone (i.e. not dependent on the NXP HAL). It was part of the effort when we wrote this port.

I think that the pin muxing is the same for all LPC11U6x SoCs. The only difference is the availability of pins which depends on the package size (48, 64 or 100 pins). So, IMHO, we can have a single pinctrl header file in Zephyr to hold all the definitions.

Note that the LPC11U67JBD48-pinctrl.h and LPC11U68JBD100-pinctrl.h files provided by the NXP HAL are identical. And so the LPC11U67JBD48-pinctrl.h file does include definitions for pins that are not available on the 48-pin package...

danieldegrasse · 2022-04-20T14:30:27Z

@mbittan @simonguinot I'll go ahead and move the header in tree. The reason that headers are currently generated per-package is due to the way the generation script used for other LPC parts (such as LPC55s69) works, and you are correct that all the LPC11u6x parts have the same pinmux settings.

I am also going to move the default UART port to UART0 as part of this PR (on the LPC11u68 EVK), since that will make it simpler for new Zephyr users to get started with the platform.

dleach02 · 2022-05-02T14:46:43Z

@mbittan can we get your feedback on the current state of this PR? I would like to get it ready and merged before the feature freeze on 5/13

mbittan

Looks good to me

gmarull · 2022-05-03T08:30:51Z

dts/arm/nxp/nxp_lpc11u6x.dtsi

why not name this one pinctrl?

The pio0 through pio2 nodes are used by the GPIO driver to determine base addresses for each IOCON port, so the compatible string for the iocon node needs to define the properties for each pio child node. Then, the child pinctrl node has its own properties, and that node has the pincontrol selection defined as children.

I believe if this node was pinctrl then the pio child nodes could not be present, because they do not contain the same properties as pin control groups (which are the children of the pinctrl node) do. Please correct me if that is wrong, because this node being named pinctrl would be cleaner.

If a child has a compatible set, then it can do whatever it wants, see for example STM32/GD32 pinctrl/gpio layout.

Makes sense. The only other hurdle to renaming that pin control node would be that the lpc pin control driver currently relies on the node label iocon being present in the devicetree in order to get the base address of the pin controller. I used the node label directly because like the iMX pin control, there are several different register layouts present across the SOC families with the IOCON peripheral, so there is not one shared DTS compatible string that could be used for all pin control nodes.

So I could either:

rename the iocon node for all devices currently using this pin control driver (and rework the dts binding file (currently the RT600 and all LPC SOCs use this driver)

add a corner case to this driver for the LPC11u6x

personally, I would rather keep the driver and node naming as it currently is designed, but if you feel strongly about this, I can rename the iocon node across the LPC line. I would prefer not to add a corner case for the LPC11u6x, however.

I think it would make more sense to make iocon the pinctrl (following stm32/gd32 layout), but this can be improved later.

dts/bindings/pinctrl/nxp,lpc-iocon-pinctrl.yaml

boards/arm/faze/faze-pinctrl.dtsi

drivers/i2c/i2c_lpc11u6x.c

dts/bindings/serial/nxp,lpc11u6x-uart.yaml

simonguinot

Hi @danieldegrasse,
Thanks for this PR. It is looking good.

gmarull

please update includes, otherwise lgtm

gmarull · 2022-05-10T15:07:50Z

drivers/clock_control/clock_control_lpc11u6x.h

Suggested change

#include <drivers/pinctrl.h>

#include <zephyr/drivers/pinctrl.h>

gmarull · 2022-05-10T15:08:08Z

drivers/i2c/i2c_lpc11u6x.h

Suggested change

#include <drivers/pinctrl.h>

#include <zephyr/drivers/pinctrl.h>

gmarull · 2022-05-10T15:08:24Z

drivers/serial/uart_lpc11u6x.h

Suggested change

#include <drivers/pinctrl.h>

#include <zephyr/drivers/pinctrl.h>

gmarull · 2022-05-10T15:08:53Z

soc/arm/nxp_lpc/lpc11u6x/pinctrl_soc.h

Suggested change

#include <devicetree.h>

#include <zephyr/devicetree.h>

add pin control definitions for LPC11u6x to LPU11u6x pinctrl binding. Signed-off-by: Daniel DeGrasse <[email protected]>

update lpc11u6x dtsi file to include required DTS nodes to support LPC pin control Signed-off-by: Daniel DeGrasse <[email protected]>

add pin control definitions to LPC11u6x soc file, to handle the lack of a HAL for this SOC. Signed-off-by: Daniel DeGrasse <[email protected]>

switch gpio driver to use pio nodes to configure pin control settings, and stop using pinmux driver within gpio driver. Signed-off-by: Daniel DeGrasse <[email protected]>

add pin control header to enable pin control support for lpc11u6x Signed-off-by: Daniel DeGrasse <[email protected]>

Update pin control driver for lpc11u6x. This SOC does not have a HAL, so fsl_clock is not available. It also lacks a slew-rate field in the IOCON register, so this property must be optional. Signed-off-by: Daniel DeGrasse <[email protected]>

add pin control definitions and nodes to faze board, which uses an LPC11u67 SOC. Signed-off-by: Daniel DeGrasse <[email protected]>

Add pin control node definitions to lpcxpresso11u68 board. Signed-off-by: Daniel DeGrasse <[email protected]>

convert lpc11u6x syscon clock driver to pin control, and remove all pinmux usage from driver and syscon dts node. Signed-off-by: Daniel DeGrasse <[email protected]>

Enable pin control for lpc11u6x i2c driver, and remove pinmux usage from board level DTS files. Signed-off-by: Daniel DeGrasse <[email protected]>

Enable pin control api for lpc11u6x serial driver, and remove pinmux api usage. Signed-off-by: Daniel DeGrasse <[email protected]>

Enable pin control for lpc11u6x soc by selecting CONFIG_PINCTRL=y. At this time no drivers are ported. Signed-off-by: Daniel DeGrasse <[email protected]>

UART0 is routed to onboard debugger on LPC11u68 EVK. Change the default console from UART4 to UART0 to enable output on the onboard debugger console. Signed-off-by: Daniel DeGrasse <[email protected]>

danieldegrasse · 2022-05-10T19:58:44Z

@gmarull headers should be updated

danieldegrasse requested review from dleach02, gmarull, mmahadevan108 and mnkp as code owners April 13, 2022 17:41

github-actions bot added area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: I2C area: Modules platform: NXP NXP labels Apr 13, 2022

zephyrbot added manifest west manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Apr 13, 2022

danieldegrasse mentioned this pull request Apr 13, 2022

Road from pinmux to pinctrl #39740

Closed

29 tasks

simonguinot requested review from mbittan and simonguinot April 14, 2022 12:41

danieldegrasse mentioned this pull request Apr 14, 2022

Process: replacing CODEOWNERS with MAINTAINERS #38725

Closed

mbittan suggested changes Apr 15, 2022

View reviewed changes

danieldegrasse mentioned this pull request Apr 15, 2022

Full pin control enablement for LPC SOCs #44907

Merged

danieldegrasse requested review from MaureenHelm, carlescufi, galak and nashif as code owners April 20, 2022 14:45

mbittan previously approved these changes May 2, 2022

View reviewed changes

danieldegrasse dismissed mbittan’s stale review via 6bb766c May 2, 2022 21:26

gmarull reviewed May 3, 2022

View reviewed changes

simonguinot self-requested a review May 9, 2022 19:58

simonguinot previously approved these changes May 9, 2022

View reviewed changes

dleach02 previously approved these changes May 10, 2022

View reviewed changes

mmahadevan108 previously approved these changes May 10, 2022

View reviewed changes

gmarull requested changes May 10, 2022

View reviewed changes

danieldegrasse added 13 commits May 10, 2022 14:56

include: dt-bindings: add lpc pin control definitions

87723a9

add pin control definitions for LPC11u6x to LPU11u6x pinctrl binding. Signed-off-by: Daniel DeGrasse <[email protected]>

dts: lpc11u6x: update lpc dtsi file to include pin control nodes

aacd564

update lpc11u6x dtsi file to include required DTS nodes to support LPC pin control Signed-off-by: Daniel DeGrasse <[email protected]>

soc: lpc11u6x: add pin control definitions to LPC11u6x soc file

d59b085

add pin control definitions to LPC11u6x soc file, to handle the lack of a HAL for this SOC. Signed-off-by: Daniel DeGrasse <[email protected]>

drivers: gpio_lpc11u6x: use pio nodes to configure pin mux for gpio

a1f78f4

switch gpio driver to use pio nodes to configure pin control settings, and stop using pinmux driver within gpio driver. Signed-off-by: Daniel DeGrasse <[email protected]>

soc: lpc11u6x: add pin control header

475b60c

add pin control header to enable pin control support for lpc11u6x Signed-off-by: Daniel DeGrasse <[email protected]>

boards: faze: add pin control nodes to faze board

914fcd4

add pin control definitions and nodes to faze board, which uses an LPC11u67 SOC. Signed-off-by: Daniel DeGrasse <[email protected]>

boards: lpcxpresso11u68: add pin control nodes to lpcxpresso11u68

274d899

Add pin control node definitions to lpcxpresso11u68 board. Signed-off-by: Daniel DeGrasse <[email protected]>

drivers: clock_control: convert lpc11u6x syscon driver to pinctrl

0dfb41d

convert lpc11u6x syscon clock driver to pin control, and remove all pinmux usage from driver and syscon dts node. Signed-off-by: Daniel DeGrasse <[email protected]>

drivers: i2c: i2c_lpc11u6x: enable pin control

ba7960b

Enable pin control for lpc11u6x i2c driver, and remove pinmux usage from board level DTS files. Signed-off-by: Daniel DeGrasse <[email protected]>

drivers: serial: enable pin control for lpc11u6x serial driver

73e36e5

Enable pin control api for lpc11u6x serial driver, and remove pinmux api usage. Signed-off-by: Daniel DeGrasse <[email protected]>

soc: lpc11u6x: enable pin control

ec70b93

Enable pin control for lpc11u6x soc by selecting CONFIG_PINCTRL=y. At this time no drivers are ported. Signed-off-by: Daniel DeGrasse <[email protected]>

boards: lpcxpresso11u68: change default console to uart0

c1764c0

UART0 is routed to onboard debugger on LPC11u68 EVK. Change the default console from UART4 to UART0 to enable output on the onboard debugger console. Signed-off-by: Daniel DeGrasse <[email protected]>

danieldegrasse dismissed stale reviews from mmahadevan108, dleach02, and simonguinot via c1764c0 May 10, 2022 19:57

gmarull approved these changes May 10, 2022

View reviewed changes

dleach02 approved these changes May 10, 2022

View reviewed changes

dleach02 merged commit 48877f8 into zephyrproject-rtos:main May 10, 2022

dleach02 deleted the lpc-11u6x-pinctrl branch May 10, 2022 22:27

	PINCTRL_DT_INST_DEFINE(idx); \
	PINCTRL_DT_DEFINE(DT_NODELABEL(uart##idx)); \

	.pincfg = PINCTRL_DT_INST_DEV_CONFIG_GET(idx), \
	.pincfg = PINCTRL_DT_DEV_CONFIG_GET(DT_NODELABEL(uart##idx)), \

	#include <drivers/pinctrl.h>
	#include <zephyr/drivers/pinctrl.h>

Lpc 11u6x pinctrl #44843

Lpc 11u6x pinctrl #44843

Uh oh!

Conversation

danieldegrasse commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieldegrasse commented Apr 13, 2022

Uh oh!

mbittan commented Apr 14, 2022

Uh oh!

mbittan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieldegrasse commented Apr 18, 2022

Uh oh!

mbittan commented Apr 19, 2022

Uh oh!

simonguinot commented Apr 20, 2022

Uh oh!

danieldegrasse commented Apr 20, 2022

Uh oh!

dleach02 commented May 2, 2022

Uh oh!

mbittan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonguinot left a comment

Choose a reason for hiding this comment

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieldegrasse commented May 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

danieldegrasse commented Apr 13, 2022 •

edited

Loading