Stm32 usb high speed fixes

[bearsh]

fixes #89831 and #89841

[etienne-lms]

LGTM. @kbidani, could you have a look at this change?

[tmon-nordic]

Note that the selection in devicetree is not the end-user way to limit High-Speed capable device to operate at Full-Speed. Such limiting is achieved by the CONFIG_UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED Kconfig that user can set to n on targets that select CONFIG_UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT.

[bearsh]

this seems not really intuitive to me... isn't the devicetree meant to describe and configure the hardware? so for a uart for example, the baudrate is or at least can be selected in devicetree. why is this different for usb?

so you mean the speed selection should in the end only depend on CONFIG_UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED or further down CONFIG_USBD_MAX_SPEED_HIGH/CONFIG_USBD_MAX_SPEED_FULL?

[etienne-lms]

I wonder whether the us/next framework should provide a helper macro/function to get the max speed based on DT maxixum-speed property and CONFIG_UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED value, instead of requiring all drivers implement the same tests.

[jfischer-no]

DT property "maximum-speed" is not required for any driver.

[jfischer-no]

For UDC_STM32, it should just be select UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT, since many variants support external or embedded high-speed PHY.

[erwango]

Is it really required ?
According to this comment this adds ~450 unused bytes on USB stack usage on 24KB RAM targets that could support USB FS.

[tmon-nordic]

The increased RAM usage (which is essentially wasted RAM) with default configuration resulting from select UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT on targets that support only Full-Speed depends on used USB classes. With CDC ACM it is at least 896 bytes alone for cdc_acm_ep_pool.

[jfischer-no]

Is it really required ? According to this comment this adds ~450 unused bytes on USB stack usage on 24KB RAM targets that could support USB FS.

Yes. To optimize for a specific SoC, Kconfig option UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED feature can be disabled in the application or boards Kconfig.defconfig.

[erwango]

So we'll need to duplicate this for > 100 boards Kconfig.config that are based on these SoCs and this will probably be missed by 90% of out of tree users that could potentially benefit from it.

Can you provide a practical reason to do so while it is fully IP block dependent ?
What would be the negative consequence of doing the configuration at this level, ie one single place ?

[erwango]

To conclude on that point, I won't block for this PR on that point., but I'm clearly not in line. I find this way of doing sub-optimized and unpractical for no clear reason.

[jfischer-no]

Can you provide a practical reason to do so while it is fully IP block dependent ?

Both the "IP block" and the driver support high speed. However, the particular configuration of the "IP block" on a board/SoC may or may not support high speed. There may also be two 'IP block' instances on an board/SoC: one configured to support high speed and the other to support full speed. The driver must support this kind of hardware. For DWC2 controllers, the driver can retrieve the configuration from the registers at runtime. (By the way, I have pointed out several times that the shim driver should be fixed to support multiple instances; instead, it is moving further towards an unfixable state.) Therefore, the driver must select UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT because it supports high speed, regardless of the "IP block" configuration. To avoid bloating full-speed devices and mixing DT and Kconfig, the clean approach is to disable UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED at board level. Whether you want to use UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED directly or a ST-specific Kconfig helper option is up to you.

What would be the negative consequence of doing the configuration at this level, ie one single place ?

We already do it in one single place, in drivers/usb/udc/Kconfig. The problem with selecting it based on the specific DT compatible is that is completely messed up for ST devices. Compatible we have in the tree are: "st,stm32n6-otghs", "st,stm32-otgfs", "st,stm32-otghs", "st,stm32-usb".
"st,stm32n6-otghs" and "st,stm32-otghs" are derived from st,stm32-otghs-common.yaml. The different compatibles are mostly used to configure the software in the driver, and the properties in the bindings are almost identical. I think there could be a single binding for all variants with the manufacturer compatible st,stm32-usb (first one added to the tree), and a second binding could be used to describe the variants.
In the Kconfig.stm32 file, we would then need

DT_UDC_ST_SUPPORTS_HS := $(dt_compat_any_has_prop,$(DT_COMPAT_ST_STM32_USB),maximum-speed,high-speed)
...
	select UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT if DT_UDC_ST_SUPPORTS_HS

which would work for all variants and would not need to be modified when a new variant is added.

[erwango]

ok, thanks for the input. I acknowledge that we have lot of work remaining on that driver and still the objective to fix it this year.

[jfischer-no]

Only Kconfig option UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED and the PHY type should be considered here.

[bearsh]

what if the user selects UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED=n and USBD_MAX_SPEED_HIGH=y which is currently possible?

If I understand it correctly, this is only for device-next. What about the non next variant?

It also make me questioning what the maximum_speed dt-property is for? Currently some drivers (only) use this to select the speed.

[tmon-nordic]

what if the user selects UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED=n and USBD_MAX_SPEED_HIGH=y which is currently possible?

Then the memory is simply wasted. I am fully against this being possibile but d6a8bd5 seemed to be the only way to get #76255 forward.

If I understand it correctly, this is only for device-next. What about the non next variant?

The old device stack is going to be deprecated. The High-Speed support in the old stack was non compliant (Full-Speed backwards compatibility was essentially broken).

It also make me questioning what the maximum_speed dt-property is for? Currently some drivers (only) use this to select the speed.

Devicetree in general is for describing the hardware capability. So if the hardware is capable of High-Speed operation, and the corresponding devicetree binding uses maximum-speed property then it should be set to high-speed. UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED is for user configuration, if for whatever reason the application wants to limit itself for Full-Speed only operation.

[jfischer-no]

what if the user selects UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED=n and USBD_MAX_SPEED_HIGH=y which is currently possible?

The driver should not report high-speed capability for any controller on the SoC and the stack will handle it accordingly.

If I understand it correctly, this is only for device-next.

Yes.

What about the non next variant?

No idea why are you changing two drivers in a single commit. But proper high-speed support is not possible with usb_dc_ API and legacy stack.

It also make me questioning what the maximum_speed dt-property is for? Currently some drivers (only) use this to select the speed.

That was introduced a very long time ago for the STM32 shim driver. Some device controllers only operate at full speed. There are also controllers where the supported speeds (PHY) can be obtained at runtime. Most of the others have a PHY handle/compatible that also describes the maximum supported speed. Furthermore, the DT property "maximum-speed" cannot easily be fed into the drivers' Kconfig options. The way it is used for the usb_dc_stm32 is terrible. I'm not sure we should continue to feed the DT into the Kconfig, as there are other issues that we will likely face when reworking the DT or bindings.

[bearsh]

No idea why are you changing two drivers in a single commit. But proper high-speed support is not possible with usb_dc_ API and legacy stack.

ok, so I think I'll drop that then, leaving it as is for now

before letting the ci system waste again some energy, here another try. high-speed support is based on CONFIG_UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED but compilation of the driver will fail if the speed is limited by maximum_speed DT-property or incompatible phy. it's similar to the implementation in udc_renesas_ra.c but does the checks at compile time as the stm32 driver can only be instantiated once.

@@ -51,21 +51,20 @@ LOG_MODULE_REGISTER(udc_stm32, CONFIG_UDC_DRIVER_LOG_LEVEL);
 			    DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs))
 
 /**
- * The following defines are used to map the value of the "maxiumum-speed"
- * DT property to the corresponding definition used by the STM32 HAL.
- *
- * If full speed is selected (idx 1), but the phy is high speed capable, then
- * select USB_OTG_SPEED_HIGH_IN_FULL. Otherwise, select the corresponding
- * speed.
+ * Select the desired speed based on enabled high speed support and phy.
  */
-#define UDC_STM32_HIGH_SPEED             USB_OTG_SPEED_HIGH
-#if DT_ENUM_IDX(DT_DRV_INST(0), maximum_speed) == 1 && \
-	(defined(CONFIG_SOC_SERIES_STM32H7X) || USB_OTG_HS_EMB_PHYC || USB_OTG_HS_EMB_PHY)
-#define UDC_STM32_FULL_SPEED             USB_OTG_SPEED_HIGH_IN_FULL
+#if CONFIG_UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED && \
+    DT_ENUM_IDX_OR(id, maximum_speed, UDC_BUS_SPEED_HS) != UDC_BUS_SPEED_HS && \
+    !(USB_OTG_HS_EMB_PHYC || USB_OTG_HS_EMB_PHY || USB_OTG_HS_ULPI_PHY)
+#error "High speed support enabled by USB device controller (Kconfig) but not supported by the phy (devicetree)"
+#elif CONFIG_UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED
+#define UDC_STM32_SPEED                  USB_OTG_SPEED_HIGH
+#elif defined(CONFIG_SOC_SERIES_STM32H7X) || USB_OTG_HS_EMB_PHYC || USB_OTG_HS_EMB_PHY
+#define UDC_STM32_SPEED                  USB_OTG_SPEED_HIGH_IN_FULL
 #elif DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usb)
-#define UDC_STM32_FULL_SPEED             PCD_SPEED_FULL
+#define UDC_STM32_SPEED                  PCD_SPEED_FULL
 #else
-#define UDC_STM32_FULL_SPEED             USB_OTG_SPEED_FULL
+#define UDC_STM32_SPEED                  USB_OTG_SPEED_FULL
 #endif
 
 struct udc_stm32_data  {
@@ -86,7 +85,6 @@ struct udc_stm32_config {
 	uint32_t dram_size;
 	uint16_t ep0_mps;
 	uint16_t ep_mps;
-	int speed_idx;
 };
 
 enum udc_stm32_msg_type {
@@ -987,7 +985,6 @@ static const struct udc_stm32_config udc0_cfg  = {
 	.pma_offset = USB_BTABLE_SIZE,
 	.ep0_mps = EP0_MPS,
 	.ep_mps = EP_MPS,
-	.speed_idx = DT_ENUM_IDX_OR(DT_DRV_INST(0), maximum_speed, 1),
 };
 
 static void priv_pcd_prepare(const struct device *dev)
@@ -1000,7 +997,7 @@ static void priv_pcd_prepare(const struct device *dev)
 	/* Default values */
 	priv->pcd.Init.dev_endpoints = cfg->num_endpoints;
 	priv->pcd.Init.ep0_mps = cfg->ep0_mps;
-	priv->pcd.Init.speed = UTIL_CAT(UDC_STM32_, DT_INST_STRING_UPPER_TOKEN(0, maximum_speed));
+	priv->pcd.Init.speed = UDC_STM32_SPEED;
 
 	/* Per controller/Phy values */
 #if defined(USB)
@@ -1249,9 +1246,9 @@ static int udc_stm32_driver_init0(const struct device *dev)
 	data->caps.rwup = true;
 	data->caps.out_ack = false;
 	data->caps.mps0 = UDC_MPS0_64;
-	if (cfg->speed_idx == 2) {
-		data->caps.hs = true;
-	}
+#if UDC_STM32_SPEED == USB_OTG_SPEED_HIGH
+	data->caps.hs = true;
+#endif
 
 	priv->dev = dev;
 	priv->irq = UDC_STM32_IRQ;

this also removes the speed_idx member as it is no longer needed.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.3% Duplication on New Code

See analysis details on SonarQube Cloud

[nandojve]

Hi @marwaiehm-st ,

Could you check if it is possible approve this without @etienne-lms ?
I think he is on holidays and the fixes are working fine.

[marwaiehm-st]

Hi @marwaiehm-st ,

Could you check if it is possible approve this without @etienne-lms ? I think he is on holidays and the fixes are working fine.

Hi @nandojve ,

Yes, i'll review the changes and approve if everything looks good.

[jfischer-no]

What is id? Is it a copy&paste from udc_renesas_ra.c , or am I missing something?

[bearsh]

yes, you are right, this really seems to be a leftover from copy&paste. fixed now...

[bearsh]

mmh looking again at that code, this can't work, as UDC_BUS_SPEED_HS is an enum value and not a define. So I supposed it does also not work in the renesas driver 🤔

[jfischer-no]

@marwaiehm-st, could you please check why it did not fail in CI, and which board or platform catches this configuration?

[marwaiehm-st]

With these modifications, there is no board in CI that actually catches this configuration. For boards that use the USB OTG HS peripheral without an external high-speed PHY (ULPI chip), the maximum-speed property in the device tree node should be set to "full-speed" instead of "high-speed". This would more accurately reflect the actual operating speed and ensure the correct code paths are tested.

Here are some examples of boards that use the USB OTG HS peripheral without a ULPI PHY and therefore always operate at full-speed:

To ensure proper coverage and behavior, it would be best to update the maximum-speed property to "full-speed" for these boards in their device tree files.

Example device tree modification:

[bearsh]

@marwaiehm-st your statement confuses me as according to the STM32U5Axx datasheet, it has an embedded high-speed phy:

if I plug my Nucleo-U5A5J-Q into my pc it shows:

Aug 14 13:45:52 grayhound kernel: usb 3-6.1.1.4: new high-speed USB device number 118 using xhci_hcd
Aug 14 13:45:52 grayhound kernel: usb 3-6.1.1.4: New USB device found, idVendor=2fe3, idProduct=0001, bcdDevice= 4.01
Aug 14 13:45:52 grayhound kernel: usb 3-6.1.1.4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
Aug 14 13:45:52 grayhound kernel: usb 3-6.1.1.4: Product: USBD CDC ACM sample
Aug 14 13:45:52 grayhound kernel: usb 3-6.1.1.4: Manufacturer: Zephyr Project
Aug 14 13:45:52 grayhound kernel: usb 3-6.1.1.4: SerialNumber: 001700044231500120393441
Aug 14 13:45:52 grayhound kernel: cdc_acm 3-6.1.1.4:1.0: ttyACM0: USB ACM device

[nandojve]

See here for details about Full and High speed on STM32U5A5
#94001

[marwaiehm-st]

@bearsh you are correct: the STM32U5 series (including the Nucleo-U5A5ZJ-Q) features an embedded high-speed PHY, which is different from the other STM32 families that require an external ULPI PHY for high-speed operation. This means the board is indeed capable of true high-speed USB operation without an external PHY chip.

The device tree configuration should reflect this capability, and it is appropriate to set maximum-speed = "high-speed" for the STM32U5 Nucleo board. My earlier suggestion about setting "full-speed" applies to boards that lack any high-speed PHY (embedded or external), but not to the STM32U5 series.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.4% Duplication on New Code

See analysis details on SonarQube Cloud

[SebastianFoerster]

Works for me and speeds up the zperf tcp download test to 58 MBits/s with ncm class. Before was just 1.5 Mbits/s.

[jfischer-no]

@erwango @nandojve @marwaiehm-st I can see at least four PRs, #89866, #94001, #93796, #86100, that are trying to resolve issues with the STM32 shim driver. However, I have the impression that they are unsynchronised and stuck. I think we should therefore have a meeting to discuss how to move these PRs forward. If there is a free slot, I would like to abuse Thursday's ArchWG meeting for that (@carlescufi). What are your thoughts on this?

[nandojve]

@jfischer-no ,

@erwango @nandojve @marwaiehm-st I can see at least four PRs, #89866, #94001, #93796, #86100, that are trying to resolve issues with the STM32 shim driver. However, I have the impression that they are unsynchronised and stuck. I think we should therefore have a meeting to discuss how to move these PRs forward. If there is a free slot, I would like to abuse Thursday's ArchWG meeting for that (@carlescufi). What are your thoughts on this?

Thank you for the initiative.
Yes, It will be nice to sync about this. I need to confirm the hour.

[erwango]

Sure, let's have a discussion on this.

[bearsh]

@erwango @nandojve @marwaiehm-st I can see at least four PRs, #89866, #94001, #93796, #86100, that are trying to resolve issues with the STM32 shim driver. However, I have the impression that they are unsynchronised and stuck. I think we should therefore have a meeting to discuss how to move these PRs forward. If there is a free slot, I would like to abuse Thursday's ArchWG meeting for that (@carlescufi). What are your thoughts on this?

have you been able to discuss the topic? if yes, is there anything I can/should do/change in this PR?

[nandojve]

Hi @bearsh ,

have you been able to discuss the topic? if yes, is there anything I can/should do/change in this PR?

I'm pretty sure that many actions have been made in parallel but, I think, the meeting will be next Tuesday 23-set in the architecture group. Agenda usually is available 1 day before meeting. If the topic is there and if you are interested you can join in the conversation too.

[mathieuchopstm]

Superseded by #96028 but thanks for your contributions 🙂