dts: bindings: usb: Add maximum-speed property

[ydamigos]

Add maximum-speed property to usb node.
It configures USB controllers to work up to a specific speed.
Valid arguments are 0 for super-speed, 1 for high-speed, 2
for full-speed and 3 for low-speed.

[ydamigos]

@jli157 and @aurel32 could you also review it and test is on your boards?

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@0d12031). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #9062   +/-   ##
=========================================
  Coverage          ?   53.14%           
=========================================
  Files             ?      210           
  Lines             ?    25747           
  Branches          ?     5673           
=========================================
  Hits              ?    13682           
  Misses            ?     9755           
  Partials          ?     2310

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d12031...91eceb9. Read the comment docs.

[jli157]

@ydamigos it is weird that I still have to add the line

@@ -256,6 +256,7 @@ static int usb_dc_stm32_clock_enable(void)
 #else
        /* Disable ULPI interface (for external high-speed PHY) clock */
        LL_AHB1_GRP1_DisableClock(LL_AHB1_GRP1_PERIPH_OTGHSULPI);
+       LL_AHB1_GRP1_DisableClockLowPower(RCC_AHB1LPENR_OTGHSULPILPEN);
 #endif /* USB_HS_PHYC */

here to make my STM32F4 board work on its USB-HS full-speed mode. I found a link which also talked abut the similar issue:

I thought I had found the issue, that the

OTGHSULPILPEN: "USB OTG HS ULPI clock enable during Sleep mode" by
defailt should be disabled if it wasn't in use.

The STM32F429i-disco use the OTG_HS but as FS PHY, not a external
ULPI based PHY.

[ydamigos]

@jli157 Thanks for testing. The reset value for RCC_AHB1LPENR register is 0x7EEF 97FF which enables USB OTG HS ULPI clock during Sleep mode. I will fix it.

[ydamigos]

@jli157 I updated it. Could you test it again?

[jli157]

@ydamigos Yes, the rebased PR works on my custom board pretty well now. Thank you!

[aurel32]

Thanks @ydamigos for this patch. I have tested it on my STM32F723 board and it works. I have a few comments though:

• Setting speed to low doesn't work for both USB ports on the board, FS is used instead. After looking quickly at the code, it seems to be a limitation from the STM32 HAL.
• When an unsupported speed is requested (e.g. super speed on the STM32F723) board, the selected speed is FS instead of HS on the port with the HS PHY. On the other hand, I think it might be better to change the code to fail at compile time instead of printing a message to the console that can be missed.
• At some point we'll need to add a function to get the actual USB speed used, as some descriptors might be slightly different. For example interrupt and isochronous endpoints give their polling rate in term of frames, ie every 125µs in HS or 1ms in FS. I therefore think the name usb_dc_stm32_get_speed can be confusing, you can maybe use something like usb_dc_stm32_get_max_speed.

[ydamigos]

@aurel32 Thanks for testing. Could you test it again?

Setting speed to low doesn't work for both USB ports on the board, FS is used instead. After looking quickly at the code, it seems to be a limitation from the STM32 HAL.

The reference manual also mentions for the OTG_HS/FS_DCFG register:

DSPD[1:0]:  Device speed
Indicates the speed at which the application requires the core to enumerate,
or the maximum speed the application can support. However, the actual bus
speed is determined only after the chirp sequence is completed, and is based
on the speed of the USB host to which the core is connected.

When an unsupported speed is requested (e.g. super speed on the STM32F723) board, the selected speed is FS instead of HS on the port with the HS PHY. On the other hand, I think it might be better to change the code to fail at compile time instead of printing a message to the console that can be missed.

Done

At some point we'll need to add a function to get the actual USB speed used, as some descriptors might be slightly different. For example interrupt and isochronous endpoints give their polling rate in term of frames, ie every 125µs in HS or 1ms in FS. I therefore think the name usb_dc_stm32_get_speed can be confusing, you can maybe use something like usb_dc_stm32_get_max_speed.

Done

[aurel32]

I have justed tested it, it looks all good to me. Thanks for your work on that.

[aurel32]

Small nitpick, it might be a good idea to move that code at the beginning of the function when USB_OTG_SPEED_FULL is assigned to speed.

[ydamigos]

Done

[jli157]

I think this PR is ready to be merged. Can any reviewer give approval to it? I like to have this one in my project. :)

[erwango]

In Zephyr current state of the art, this code is fine.
In the evolution foreseen for device tree information usage, a clearer Kconfig/dts split is expected, along with fixup files removal.
Here this is clearly a case where we're using CONFIG_ #define which does not belong to Kconfig space but is purely device HW description, hence dts 'space'.

So, as a heads up, it is likely this piece will have to be reworked in some time, to make a clearer split between HW boot time configuration and device instantiation (device tree) and SW configuration (Kconfig).

[ydamigos]

Thanks for the heads up. I will do the rework when the time comes.

[galak]

I know its a bit of a pain, but can we match the linux binding use of a string for this.

• maximum-speed: tells USB controllers we want to work up to a certain
  speed. Valid arguments are "super-speed", "high-speed",
  "full-speed" and "low-speed". In case this isn't passed
  via DT, USB controllers should default to their maximum
  HW capability.

[ydamigos]

@galak I'll do the changes to use string in max-speed property. Right now the driver fails to compile if unsupported speed is defined in device tree:

#if (CONFIG_USB_MAXIMUM_SPEED != USB_DEVICE_MAX_SPEED_HIGH) && \
    (CONFIG_USB_MAXIMUM_SPEED != USB_DEVICE_MAX_SPEED_FULL) && \
    (CONFIG_USB_MAXIMUM_SPEED != USB_DEVICE_MAX_SPEED_LOW)
#error "Unsupported maximum speed defined in device tree"
#endif

I would like to inform user if an unsupported speed is defined in device tree. I can't use #if directive with strings. Should I use an error message using the SYS_LOG or LOG subsys at runtime or is it possible to restrict the values supported on device tree level?

[galak]

I would like to inform user if an unsupported speed is defined in device tree. I can't use #if directive with strings. Should I use an error message using the SYS_LOG or LOG subsys at runtime or is it possible to restrict the values supported on device tree level?

We should probably put this check/parsing in the device tree generation script. Having some means in the yaml to describe a set of know string values that get checked against.

[aurel32]

We should probably put this check/parsing in the device tree generation script. Having some means in the yaml to describe a set of know string values that get checked against.

I am not sure it's something possible as the allowed values depends on the controller version, which is not known at the time the device-tree is parsed. For example some STM32F7 have an HS internal PHY, while on others the internal PHY is limited to FS. This is known by checking the USB_HS_PHYC defined in the HAL code.

[ydamigos]

I am not sure it's something possible as the allowed values depends on the controller version, which is not known at the time the device-tree is parsed. For example some STM32F7 have an HS internal PHY, while on others the internal PHY is limited to FS. This is known by checking the USB_HS_PHYC defined in the HAL code.

We are responsible the device tree to have the correct values for each SoC. The check/parsing is just to avoid typos or invalid values (e.g. "super-fast-speed").

[galak]

I am not sure it's something possible as the allowed values depends on the controller version, which is not known at the time the device-tree is parsed. For example some STM32F7 have an HS internal PHY, while on others the internal PHY is limited to FS. This is known by checking the USB_HS_PHYC defined in the HAL code.

Its annoying, but we should have different compatibles for this, and have different yaml if we want to error check the values.

[ydamigos]

Its annoying, but we should have different compatibles for this, and have different yaml if we want to error check the values.

Could we use phy bindings like it is done in linux, instead of different compatibles and different yaml?

For example in stm32429i-eval.dts in linux::

usbotg_hs_phy: usbphy {
	#phy-cells = <0>;
	compatible = "usb-nop-xceiv";
	clocks = <&rcc 0 STM32F4_AHB1_CLOCK(OTGHSULPI)>;
	clock-names = "main_clk";
};

&usbotg_hs {
	dr_mode = "host";
	phys = <&usbotg_hs_phy>;
	phy-names = "usb2-phy";
	pinctrl-0 = <&usbotg_hs_pins_a>;
	pinctrl-names = "default";
	status = "okay";
};

[galak]

Could we use phy bindings like it is done in linux, instead of different compatibles and different yaml?

that works for me.

[ydamigos]

I'll study a bit more the phy bindings in linux and I'll create a different PR to add it in Zephyr asap.

[jli157]

@ydamigos did you create a new PR to address the same issue on this PR? I want to follow it until it is merged.

[ydamigos]

@jli157 Not yet, it is in my todo list.

[ydamigos]

Rebased to resolve conflicts.

[ydamigos]

@galak any more comments? Could we merge it?

[ydamigos]

@galak I opened PR #10359 to add support for USB PHYs.

[ydamigos]

Rebased to resolve conflicts.