Implement nxp USB device controllers (udc_mcux) #70401

MarkWangChinese · 2024-03-19T01:14:29Z

Create the mcux usb device controller driver (udc_mcux.c) based on NXP SDK USB Device controller drivers (usb_device_ehci.c, usb_device_ip3511.c etc).
Support EHCI and IP3511 controllers. Shell demo's transfer function is verified on RT1060, RT685 and LPC55S69.

Edit: addresses list found in #42066

zephyrbot · 2024-03-19T01:15:39Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

tmon-nordic · 2024-03-19T08:02:11Z

Same as with the host PR, the order of commits has to be bisectable, meaning first update hal and then use it, not the other way round.

MarkWangChinese · 2024-03-20T01:36:49Z

Same as with the host PR, the order of commits has to be bisectable, meaning first update hal and then use it, not the other way round.

Thanks @tmon-nordic , I have updated the commits order. Updating soc device tree firstly, enabling the udc/uhc driver secondly, then enabling the board level device tree and related clock. Please help to check whether it is OK.

dleach02 · 2024-03-26T23:21:24Z

@jfischer-no, need your attention to this PR.

@tmon-nordic, could you confirm the changes address your raised issue?

tmon-nordic · 2024-04-08T09:25:51Z

@tmon-nordic, could you confirm the changes address your raised issue?

Yes, the commit order seems fine now.

jfischer-no · 2024-06-06T21:04:30Z

drivers/usb/udc/udc_mcux_ehci.c

+#include "usb_device_config.h"
+#include "usb_device_mcux_drv_port.h"
+#include "usb_device_ehci.h"
+#ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED


Please remove #ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED, it should not be necessary.

As I explained, NXP has may platforms. Even the same USB controller IP is used, the PHY is used or not in different platforms. For example: (1) LPC54608 uses the IP3511 driver, the phy is not used; LPC55S69 uses the IP3511 driver, the phy is used. (2) RW610 uses the EHCI driver, the phy is not used; RTxxxx series uses the EHCI driver, the phy is used.
So, the MACRO is needed here.

I do not care how many platforms NXP has. There is no need to include this header conditionally.

why does it matter?

drivers/usb/udc/udc_mcux_ehci.c

jfischer-no · 2024-06-07T08:17:11Z

drivers/usb/udc/udc_mcux_ehci.c

+#include "usb_device_config.h"
+#include "usb_device_mcux_drv_port.h"
+#include "usb_device_ehci.h"
+#ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED


I do not care how many platforms NXP has. There is no need to include this header conditionally.

jfischer-no · 2024-06-07T08:18:13Z

drivers/usb/udc/udc_mcux_ehci.c

+#ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED
+#define UDC_MCUX_PHY_DEFINE(n)								\
+static usb_phy_config_struct_t phy_config_##n = {					\
+	.D_CAL = DT_PROP_OR(DT_INST_PHANDLE(n, phy_handle), tx_d_cal, 0),		\
+	.TXCAL45DP = DT_PROP_OR(DT_INST_PHANDLE(n, phy_handle), tx_cal_45_dp_ohms, 0),	\
+	.TXCAL45DM = DT_PROP_OR(DT_INST_PHANDLE(n, phy_handle), tx_cal_45_dm_ohms, 0),	\
+}
+
+#define UDC_MCUX_PHY_DEFINE_OR(n)							\
+	COND_CODE_1(DT_NODE_HAS_PROP(DT_DRV_INST(n), phy_handle),			\
+		    (UDC_MCUX_PHY_DEFINE(n)), ())
+
+#define UDC_MCUX_PHY_CFG_PTR_OR_NULL(n)							\
+	.phy_config = COND_CODE_1(DT_NODE_HAS_PROP(DT_DRV_INST(n), phy_handle),		\
+		    (&phy_config_##n), (NULL))
+#else
+#define UDC_MCUX_PHY_DEFINE_OR(n)
+#define UDC_MCUX_PHY_CFG_PTR_OR_NULL(n)
+#endif


Not addressed, #ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED is not required.

I don't understand. In one platform that doesn't have the PHY driver, so the usb_phy.h is not present/valid in this platform. Why should I include this file?
I use CONFIG_DT_HAS_NXP_USBPHY_ENABLED to reduce the phy_config here because it is not needed if the platform doesn't have this phy.

I can remove all the phy related codes from the driver and put it in the soc.c of different platforms. The previous version NXP usb device controller driver (usb_dc_mcux.c) works this way.
Please let me know your thought.

The phy initialization code is fine and belongs to the shim driver. There is nothing in phy/usb_phy.h that requires you to include it conditionally, it can always build. Therefore, all your #ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED can be removed, resulting in much cleaner code without unnecessary #ifdef. Here it is doubly unnecessary, whatever phy is present and needs to be initialized is covered by the macros above.

Hi Johann, In SDK USB stack, I cannot guarantee the usb_phy.h will not contain the soc/platform/IP level codes in future even it only contains the common C codes now. The reason is: NXP releases SDK package by platform, one platform has one package. usb_phy.h is only present in the package of platforms that have this PHY, and it is only valid/used in the platform, it is never built with the platforms that doesn't have the PHY. Without NXP SDK release's pre-building, I cannot guarantee usb_phy.h will build successfully with the platforms that doesn't have this phy forever.

@MarkWangChinese Got it, can you please add a comment before #include "usb_phy.h" that the header is not available for every package, thanks.

But, #ifdef CONFIG_DT_HAS_NXP_USBPHY_ENABLED ... #else is not necessary here, see jfischer-no@7652d16 (https://github.com/jfischer-no/zephyr/commit/7652d1647de7dbf1cdce4b6f706605f6ea8c9e48.patch)

Dev Review discussion:
@MarkWangChinese we discussed this PR with @jfischer-no in today's dev review meeting led by @MaureenHelm. He is would like to see the #ifdef removed and will approve the PR for merging. If we find later on subsequent platforms this is a problem we can address then.

Johann also re-kicked the CI so that is clean. So we will be able to merge this PR on Friday 6/14.

@dleach02 @MarkWangChinese Actually let get it in, we have CI green and do not know what happens tomorrow 😃, #70401 (comment) can be followup.

I have created one PR #74382, @jfischer-no please help to review.

drivers/usb/udc/udc_mcux_ehci.c

drivers/usb/udc/udc_mcux_ip3511.c

drivers/usb/udc/udc_mcux_ehci.c

dleach02 · 2024-06-11T16:12:43Z

The CI issues are unrelated. A fix has been posted by the TF-M folks [#74052] for the high priority issue #73954

define usbphy in DT and controller DT node ref to usbphy node. define the usbphy yaml and update ehci and ip3511 yaml for usbphy. Signed-off-by: Mark Wang <[email protected]>

udc_mcux_ehci is based on the MCUX USB controller driver (usb_device_ehci.c); udc_mcux_ip3511 is based on the MCUX USB controller driver (usb_device_lpcip3511.c); add related Kconfig and CMake; include the usb_phy.h path in modules/hal_nxp/usb/CMakeLists.txt because udc_mcux.c use it; add related macros to usb_device_config.h; update CMakeLists for udc_mcux_ehci and udc_mcux_ip3511. Signed-off-by: Mark Wang <[email protected]>

set DT node as Okay in board device tree; add board level's d-cal, txcal45dp and txcal45dm to usbphy node; enable usb clock; set USB_STACK_USE_DEDICATED_RAM for lpc55s69 and rt685; load usb.ld for lpc55s69 and rt685. Signed-off-by: Mark Wang <[email protected]>

enable next usb device stack samples (cdc_acm, mass and shell) on rt685 and mimxrt1060_evk Signed-off-by: Mark Wang <[email protected]>

MarkWangChinese requested review from mmahadevan108 and dleach02 as code owners March 19, 2024 01:14

zephyrbot added area: USB Universal Serial Bus platform: NXP Drivers NXP Semiconductors, drivers area: Devicetree Binding PR modifies or adds a Device Tree binding platform: NXP NXP labels Mar 19, 2024

zephyrbot requested review from bperseghetti, danieldegrasse, dbaluta, decsny, DerekSnell, EmilioCBen, galak, jfischer-no, manuargue, PetervdPerk-NXP, tmon-nordic and yvanderv March 19, 2024 01:15

zephyrbot assigned jfischer-no Mar 19, 2024

zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Mar 19, 2024

MarkWangChinese changed the title ~~Implement nxp device host controllers (udc_mcux)~~ Implement nxp device controllers (udc_mcux) Mar 19, 2024

MarkWangChinese changed the title ~~Implement nxp device controllers (udc_mcux)~~ Implement nxp USB device controllers (udc_mcux) Mar 19, 2024

dleach02 mentioned this pull request Apr 1, 2024

Planned USB device/host support rework/enhancements #42066

Open

57 tasks

zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Jun 6, 2024

tmon-nordic previously approved these changes Jun 6, 2024

View reviewed changes

MarkWangChinese requested a review from jfischer-no June 6, 2024 08:59

dleach02 previously approved these changes Jun 6, 2024

View reviewed changes

jfischer-no reviewed Jun 6, 2024

View reviewed changes

MarkWangChinese dismissed stale reviews from dleach02 and tmon-nordic via d54efb3 June 7, 2024 03:01

jfischer-no requested changes Jun 7, 2024

View reviewed changes

jfischer-no reviewed Jun 10, 2024

View reviewed changes

drivers/usb/udc/udc_mcux_ehci.c Outdated Show resolved Hide resolved

drivers/usb/udc/udc_mcux_ip3511.c Outdated Show resolved Hide resolved

drivers/usb/udc/udc_mcux_ehci.c Outdated Show resolved Hide resolved

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jun 11, 2024

MarkWangChinese added 4 commits June 12, 2024 11:05

dts: arm: nxp: enable udc DT on lpc55s69, rt1060 and rt685

82f8405

define usbphy in DT and controller DT node ref to usbphy node. define the usbphy yaml and update ehci and ip3511 yaml for usbphy. Signed-off-by: Mark Wang <[email protected]>

samples: usb: enable next usb device stack samples on NXP platforms

4dcf54c

enable next usb device stack samples (cdc_acm, mass and shell) on rt685 and mimxrt1060_evk Signed-off-by: Mark Wang <[email protected]>

zephyrbot removed manifest manifest-hal_nxp labels Jun 12, 2024

tmon-nordic approved these changes Jun 12, 2024

View reviewed changes

MarkWangChinese requested a review from jfischer-no June 12, 2024 13:09

MaureenHelm removed the dev-review To be discussed in dev-review meeting label Jun 13, 2024

jfischer-no approved these changes Jun 13, 2024

View reviewed changes

aescolar merged commit 1fabbe0 into zephyrproject-rtos:main Jun 13, 2024
31 of 34 checks passed

dleach02 deleted the impl_udc_mcux branch July 17, 2024 01:49

DerekSnell mentioned this pull request Oct 14, 2024

Add USB next UDC support for NXP MCX MCUs #79806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement nxp USB device controllers (udc_mcux) #70401

Implement nxp USB device controllers (udc_mcux) #70401

MarkWangChinese commented Mar 19, 2024 •

edited by dleach02

Loading

zephyrbot commented Mar 19, 2024 •

edited

Loading

tmon-nordic commented Mar 19, 2024

MarkWangChinese commented Mar 20, 2024

dleach02 commented Mar 26, 2024

tmon-nordic commented Apr 8, 2024

jfischer-no Jun 6, 2024

MarkWangChinese Jun 7, 2024

jfischer-no Jun 7, 2024

decsny Jun 7, 2024

jfischer-no Jun 7, 2024

jfischer-no Jun 7, 2024

MarkWangChinese Jun 7, 2024

MarkWangChinese Jun 7, 2024

jfischer-no Jun 10, 2024

MarkWangChinese Jun 11, 2024

jfischer-no Jun 13, 2024

dleach02 Jun 13, 2024 •

edited

Loading

jfischer-no Jun 13, 2024 •

edited

Loading

MarkWangChinese Jun 17, 2024

dleach02 commented Jun 11, 2024 •

edited

Loading

Implement nxp USB device controllers (udc_mcux) #70401

Implement nxp USB device controllers (udc_mcux) #70401

Conversation

MarkWangChinese commented Mar 19, 2024 • edited by dleach02 Loading

zephyrbot commented Mar 19, 2024 • edited Loading

tmon-nordic commented Mar 19, 2024

MarkWangChinese commented Mar 20, 2024

dleach02 commented Mar 26, 2024

tmon-nordic commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleach02 Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

jfischer-no Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleach02 commented Jun 11, 2024 • edited Loading

MarkWangChinese commented Mar 19, 2024 •

edited by dleach02

Loading

zephyrbot commented Mar 19, 2024 •

edited

Loading

dleach02 Jun 13, 2024 •

edited

Loading

jfischer-no Jun 13, 2024 •

edited

Loading

dleach02 commented Jun 11, 2024 •

edited

Loading