drivers: uhc: implement UHC shim driver for NXP KHCI and EHCI controller #77973

MarkWangChinese · 2024-09-04T08:15:22Z

Enablet the NXP MCUX UHC drivers.
Add uhc support for frdm_k22f, rt1060, lpc55s69 and lpc55s28. frdm_k22f supports the KHCI controller, rt1060 supports the EHCI controller, lpc55s69 and lpc55s28 support the OHCI and IP3516HS controller.

zephyrbot · 2024-09-04T08:16:17Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@`b5f6470` (`master`)	zephyrproject-rtos/hal_nxp#496	zephyrproject-rtos/hal_nxp#496/files

⛔ DNM label due to: 1 project with PR revision

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

PetervdPerk-NXP · 2024-09-04T08:53:02Z

Any plans to enable 1170 and 1180 as well?
Since they share almost the same EHCI controller.

MarkWangChinese · 2024-09-04T13:18:59Z

Any plans to enable 1170 and 1180 as well? Since they share almost the same EHCI controller.

After this basic NXP UHC controller driver merges, I think next step is implementing the host stack. Because the host stack is not ready, so supporting more platforms does not make much sense.

tmon-nordic · 2024-09-05T18:15:45Z

include/zephyr/usb/usbh.h

+ *
+ * @return the device's address.
+ */
+uint8_t usbh_udev_get_address(void *usb_device);


Why void *usb_device? Shouldn't it simply be struct usb_device *usb_device?

It is called from context that can't see struct usb_device. For example: the uhc files that can't see struct usb_device.

Just treat it as opaque pointer then. Having void * here is bad.

In the struct uhc_transfer, it is void *udev. So I use the void *. Could you please give reference codes how to treat it as opaque pointer?

That would need a change in the struct uhc_transfer then. Opaque pointers in C are simply achieved by having incomplete struct definition. So in this case it would be struct usb_device; above struct uhc_transfer { and then the void *udev; can be changed to struct usb_device *udev;. Similar could be done with the transfer completion callback. I somehow completely missed this during the uhc review.

Thanks @tmon-nordic, I changed it, please help to review again.

tmon-nordic · 2024-09-10T11:15:17Z

include/zephyr/usb/usbh.h

@@ -89,6 +89,9 @@ struct usbh_class_data {
 	int (*resumed)(struct usbh_contex *const uhs_ctx);
 };

+/* declare usb_device */


This comment doesn't really make anything clearer, I'd remove it.

jfischer-no · 2024-09-10T13:38:27Z

drivers/usb/uhc/Kconfig.mcux

+	bool "NXP MCUX USB EHCI Host controller driver"
+	default y
+	depends on DT_HAS_NXP_UHC_EHCI_ENABLED
+	select EVENTS


Where is k_event used?

The NXP hal controller drivers use the k_event. Hal controller drivers depend on the event of modules\hal\nxp\mcux\mcux-sdk\components\osa\fsl_os_abstraction_zephyr, and the fsl_os_abstraction_zephyr depend on k_event.

The hal driver must not use anything from Zephyr RTOS.

The MCUX usb host hal controller driver is like the UHC of Zephyr. In the hal driver, one task function is provided, the upper layer can use the task function to create one task to handle the controller interrupts. Then the OS event is used to handle interrupts in this task function.

jfischer-no · 2024-09-10T13:40:24Z

drivers/usb/uhc/Kconfig.mcux

+	  NXP MCUX USB Host Controller Driver for ohci.
+
+config HEAP_MEM_POOL_ADD_SIZE_UHC_MCUX
+	int


Intentionally hidden?

jfischer-no · 2024-09-10T13:42:31Z

drivers/usb/uhc/uhc_mcux_common.c

+	}
+	return 0;


Please add an empty line above return if it is not a single line within a block, please fix it everywhere.

jfischer-no · 2024-09-10T13:48:04Z

drivers/usb/uhc/uhc_mcux_common.c

+	case kUSB_HostGetDeviceHubNumber:   /* device hub address */
+	case kUSB_HostGetDevicePortNumber:  /* device port no */
+	case kUSB_HostGetDeviceHSHubNumber: /* device high-speed hub address */
+	case kUSB_HostGetDeviceHSHubPort:   /* device high-speed hub port no */
+	case kUSB_HostGetHubThinkTime:      /* device hub think time */
+		*infoValue = 0;
+		break;
+	case kUSB_HostGetDeviceLevel: /* device level */


Please cleanup from redundant comments.

jfischer-no · 2024-09-10T13:48:10Z

drivers/usb/uhc/uhc_mcux_common.c

+		break;
+
+	default:
+		/*no action*/


Please cleanup from redundant comments.

jfischer-no · 2024-09-10T14:38:37Z

drivers/usb/uhc/uhc_mcux_ip3516hs.c

+	for (uint8_t i = 0; i < USB_HOST_CONFIG_MAX_PIPES; i++) {
+		uint8_t mcux_ep = ep;
+
+		if (mcux_ep == 0x80) {


if (mcux_ep == USB_CONTROL_EP_IN) {

jfischer-no · 2024-09-10T14:39:56Z

drivers/usb/uhc/uhc_mcux_ip3516hs.c

+	mcux_ep_handle = uhc_mcux_get_hal_ep(priv, udev, ep);
+
+	/* Initialize mcux endpoint pipe */
+	if (mcux_ep_handle == NULL) {


if (mcux_ep_handle != NULL) { return mcux_ep_handle; }

jfischer-no · 2024-09-10T14:41:16Z

drivers/usb/uhc/uhc_mcux_khci.c

+	mcux_ep_handle = uhc_mcux_get_hal_ep(priv, udev, ep);
+
+	/* Initialize mcux endpoint pipe */
+	if (mcux_ep_handle == NULL) {


if (mcux_ep_handle != NULL) { return mcux_ep_handle; }

jfischer-no · 2024-09-10T14:43:01Z

include/zephyr/usb/usbh.h

@@ -89,6 +89,9 @@ struct usbh_class_data {
 	int (*resumed)(struct usbh_contex *const uhs_ctx);
 };

+/* declare usb_device */


Please cleanup.

jfischer-no · 2024-09-10T14:46:54Z

drivers/usb/uhc/uhc_mcux_common.c

+usb_status_t USB_HostHelperGetPeripheralInformation(usb_device_handle deviceHandle,
+						    uint32_t infoCode, uint32_t *infoValue)
+{
+	const struct device *dev = usbh_udev_get_dev(deviceHandle);


Driver must not call anything from the higher layer. Each uhc_transfer has a device address, you have to use it somehow.

The purpose of I add usbh_udev_get_dev and usbh_udev_get_address is to get device's information by controller driver. I think the device's related hub information (hub address, port num, first hs hub address, first hs hub port num) and speed are also needed by the controller driver to do the transfer. Will you plan to put all the information to every uhc_transfer?

Regardless of whether this information is needed or not, you cannot include this header or use any higher-level API in the driver.

Even the linux EHCI driver include the usb.h linux/usb.h> in linux/drivers/usb/host/ehci-hcd.c. Actually I don't understand why the simple helper API can't be called in lower level. Just want to figure out the style, do you mean that only callback is right from lower level to higher level? Anyway I have changed the codes to get device information based on the xfer. When developing the usb host stack, I think the codes may need to be updated to match host stack's design. For now, it should work.

Actually I don't understand why the simple helper API can't be called in lower level.

Because it is always a level violation, and it ends in a nightmare.

I think the device's related hub information (hub address, port num, first hs hub address, first hs hub port num) and speed are also needed by the controller driver to do the transfer. Will you plan to put all the information to every uhc_transfer?

Yes, but maybe rather a pointer to a device structure similar to struct urb. struct usb_device would have to be defined in uhc.h and replace addr in struct uhc_transfer.

OK, Thanks for the explanation.

CrushedB · 2024-09-15T10:56:28Z

Any plans to enable 1170 and 1180 as well? Since they share almost the same EHCI controller.

After this basic NXP UHC controller driver merges, I think next step is implementing the host stack. Because the host stack is not ready, so supporting more platforms does not make much sense.

Hi @MarkWangChinese,
Thanks for this delivery.
Can you please elaborate on the missing host stack? What won't be supported after this delivery? (I would like to be able to host a USB thumb drive on the rx10xx board).

DerekSnell · 2024-09-16T14:38:06Z

Hi @CrushedB ,

Can you please elaborate on the missing host stack? What won't be supported after this delivery? (I would like to be able to host a USB thumb drive on the rx10xx board).

Currently, Zephyr does not include a USB Host stack, only USB Device. This USB RFC tracks the plans and progress for adding a USB Host stack. Today, the USB Host Controller (UHC) driver API has be introduced for the low-level APIs. And this PR is porting the UHC APIs to the NXP USB hardware. But as the RFC shows, the USB Host stack for the high-level APIs still needs to be introduced. And the RFC shows the plans for the Host classes that will be supported.

If you have any further questions on the USB Host plans or progress, we encourage you to engage in the USB RFC. Best regards

MarkWangChinese · 2025-01-02T07:13:47Z

Rebase the Zephyr main branch.
Hi @jfischer-no Could you please help to review again?
I have another query, I see you have one task item as "Introduction of USB host stack implementation (WIP @jfischer-no)" in #42066. When will you plan to give this introducation?

tmon-nordic · 2025-01-09T13:32:16Z

Twister failures sounds like some hal issue with missing fsl_os_abstraction.h header. Please fix it.

It is based on SDK USB Host controller driver. Support NXP EHCI, KHCI, OHCI and IP3516HS controllers. Signed-off-by: Mark Wang <yichang.wang@nxp.com>

…5s28 add uhc related items to dts. add clock initialization add BM4 if CONFIG_USB_UHC_NXP_KHCI is enabled add pin mux update board related CMakeLists.txt update west.yml to contain the hal_nxp pr Signed-off-by: Mark Wang <yichang.wang@nxp.com>

add uhc related items to dts. Signed-off-by: Mark Wang <yichang.wang@nxp.com>

MarkWangChinese · 2025-01-14T09:48:20Z

Twister failures sounds like some hal issue with missing fsl_os_abstraction.h header. Please fix it.

Fixed

MarkWangChinese requested review from mmahadevan108 and dleach02 as code owners September 4, 2024 08:15

zephyrbot added area: ARM ARM (32-bit) Architecture area: Architectures area: USB Universal Serial Bus platform: NXP NXP platform: NXP Drivers NXP Semiconductors, drivers labels Sep 4, 2024

zephyrbot requested review from bperseghetti and butok September 4, 2024 08:16

zephyrbot requested review from danieldegrasse, dbaluta, decsny, DerekSnell, EmilioCBen, ithinuel, jfischer-no, manuargue, PetervdPerk-NXP, tmon-nordic and yvanderv September 4, 2024 08:16

zephyrbot added the manifest label Sep 4, 2024

zephyrbot assigned jfischer-no Sep 4, 2024

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

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from 96b82c7 to 9d544e0 Compare September 4, 2024 08:41

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from 9d544e0 to ec3466f Compare September 4, 2024 09:02

MarkWangChinese mentioned this pull request Sep 4, 2024

Enable NXP USB Host controllers in Zephyr zephyrproject-rtos/hal_nxp#327

Closed

jfischer-no added this to the v4.0.0 milestone Sep 5, 2024

tmon-nordic requested changes Sep 5, 2024

View reviewed changes

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from ec3466f to ef61ef7 Compare September 9, 2024 03:13

tmon-nordic previously approved these changes Sep 10, 2024

View reviewed changes

jfischer-no changed the title ~~Feature/enable mcux uhc~~ drivers: uhc: implement UHC shim driver for NXP KHCI and EHCI controller Sep 10, 2024

jfischer-no requested changes Sep 10, 2024

View reviewed changes

MarkWangChinese dismissed tmon-nordic’s stale review via 8f326da September 20, 2024 11:03

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch 2 times, most recently from 8f326da to 09db202 Compare September 25, 2024 14:00

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from 09db202 to 7b45a44 Compare September 29, 2024 05:04

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from 7b45a44 to 1652c13 Compare October 31, 2024 07:42

bperseghetti requested a review from jfischer-no November 4, 2024 18:40

mmahadevan108 modified the milestones: v4.0.0, v4.1.0 Nov 8, 2024

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from 1652c13 to 9c0cd0c Compare January 2, 2025 07:07

drivers: uhc: implement nxp mcux uhc driver

56f5890

It is based on SDK USB Host controller driver. Support NXP EHCI, KHCI, OHCI and IP3516HS controllers. Signed-off-by: Mark Wang <yichang.wang@nxp.com>

MarkWangChinese mentioned this pull request Jan 14, 2025

Feature/enable usb host controller zephyrproject-rtos/hal_nxp#496

Open

MarkWangChinese added 2 commits January 14, 2025 13:17

boards: nxp: add uhc support for mimxrt1050_evk

186641a

add uhc related items to dts. Signed-off-by: Mark Wang <yichang.wang@nxp.com>

MarkWangChinese force-pushed the feature/enable_mcux_uhc branch from 9c0cd0c to 186641a Compare January 14, 2025 05:20

fabiobaltieri added DNM (manifest) This PR should not be merged (controlled by action-manifest) and removed DNM This PR should not be merged (Do Not Merge) labels Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: uhc: implement UHC shim driver for NXP KHCI and EHCI controller #77973

drivers: uhc: implement UHC shim driver for NXP KHCI and EHCI controller #77973

MarkWangChinese commented Sep 4, 2024

zephyrbot commented Sep 4, 2024 •

edited

Loading

PetervdPerk-NXP commented Sep 4, 2024

MarkWangChinese commented Sep 4, 2024

tmon-nordic Sep 5, 2024

MarkWangChinese Sep 6, 2024

tmon-nordic Sep 6, 2024

MarkWangChinese Sep 6, 2024

tmon-nordic Sep 6, 2024

MarkWangChinese Sep 9, 2024

tmon-nordic Sep 10, 2024

jfischer-no Sep 10, 2024

MarkWangChinese Sep 12, 2024

jfischer-no Sep 20, 2024

MarkWangChinese Sep 25, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

jfischer-no Sep 10, 2024

MarkWangChinese Sep 12, 2024 •

edited

Loading

jfischer-no Sep 20, 2024

MarkWangChinese Sep 25, 2024

jfischer-no Oct 9, 2024

MarkWangChinese Oct 31, 2024

CrushedB commented Sep 15, 2024

DerekSnell commented Sep 16, 2024

MarkWangChinese commented Jan 2, 2025

tmon-nordic commented Jan 9, 2025

MarkWangChinese commented Jan 14, 2025

drivers: uhc: implement UHC shim driver for NXP KHCI and EHCI controller #77973

Are you sure you want to change the base?

drivers: uhc: implement UHC shim driver for NXP KHCI and EHCI controller #77973

Conversation

MarkWangChinese commented Sep 4, 2024

zephyrbot commented Sep 4, 2024 • edited Loading

PetervdPerk-NXP commented Sep 4, 2024

MarkWangChinese commented Sep 4, 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

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

MarkWangChinese Sep 12, 2024 • edited Loading

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

CrushedB commented Sep 15, 2024

DerekSnell commented Sep 16, 2024

MarkWangChinese commented Jan 2, 2025

tmon-nordic commented Jan 9, 2025

MarkWangChinese commented Jan 14, 2025

zephyrbot commented Sep 4, 2024 •

edited

Loading

MarkWangChinese Sep 12, 2024 •

edited

Loading