USB HS support fixes on STM32U5

[erikarn]

• The dev board in question has the bits populated to support USB 2.0 high speed, so add it to the documentation and device tree
• Add support to the STM32 USB 2.0 driver to support a non-default clock source

(This was for pull reuqest 80249, but that was against main and not a branch, and I was asked on discord to put it in a branch)

[erikarn]

Yay it passed all the tests!

[mathieuchopstm]

Looks OK. A few nits and proposals for simplification:

[erikarn]

anything else? is everything ok to merge? :P

[JarmouniA]

anything else? is everything ok to merge? :P

This PR will have to go into 4.1 (once 4.0 releases) as we are now in the 4.0 feature freeze.

[erikarn]

anything else? is everything ok to merge? :P

This PR will have to go into 4.1 (once 4.0 releases) as we are now in the 4.0 feature freeze.

oh! dang ok, what's the timeline for that? I'm just trying to avoid multiple merges on my side!

[JarmouniA]

anything else? is everything ok to merge? :P

This PR will have to go into 4.1 (once 4.0 releases) as we are now in the 4.0 feature freeze.

oh! dang ok, what's the timeline for that? I'm just trying to avoid multiple merges on my side!

https://github.com/zephyrproject-rtos/zephyr/milestone/75

[mniestroj]

Please add usb_device tag to nucleo_u5a5zj_q.yaml.

[erikarn]

Please add usb_device tag to nucleo_u5a5zj_q.yaml.

hi! can you explain that a bit more please?

[mniestroj]

Please add usb_device tag to nucleo_u5a5zj_q.yaml.

hi! can you explain that a bit more please?

See

zephyr/boards/st/nucleo_wb55rg/nucleo_wb55rg.yaml

Line 23 in 28bd478

- usb_device

. This advertises that USB device mode is supported, so that twister will build samples that utilize USB device mode.

[erikarn]

Please add usb_device tag to nucleo_u5a5zj_q.yaml.

hi! can you explain that a bit more please?

See

zephyr/boards/st/nucleo_wb55rg/nucleo_wb55rg.yaml

Line 23 in 28bd478

- usb_device

. This advertises that USB device mode is supported, so that twister will build samples that utilize USB device mode.

Done!

[erikarn]

ok, since this is a long review, would someone please help summarise what's left for me to address? Thanks!

[mniestroj]

See some of my comments.

We also need to address OTGEN and OTGHSPHYEN clock enabling in RCC_AHB2ENR1 register. Commit 57723cf has changed from enabling both bits to just enabling OTGHSPHYEN (bit 15) with:

clocks = <&rcc STM32_CLOCK(AHB2, 15U)>,

I think we should choose one of those options:

1. Select OTG and OTG_PHY clocks separately in each devicetree node. This means that we should add clocks property to OTG_PHY node. That property will allow to define bit that enabled OTG_PHY clock, but will also allow us to select clock configuration (e.g. whether it is HSE, PLL1, HSE/2 or PLL1/2...).
2. Use only OTG devicetree node to select all clocks (including OTG_PHY). That way we should move clock-cfg selection out of OTG_PHY node, so we are consistent with where we select clock.

[mniestroj]

I think it makes more sense to define list of possible frequencies:

      - 16000000
      - 19200000
      - 20000000
      - 24000000
      - 26000000
      - 32000000

here. That way it will still be pretty straightforward to select specific configuration in devicetree, e.g. with DT_FREQ_M(16), while being able to drop include/zephyr/dt-bindings/phy/stm32u5_otg_hs_phy.h entirely.

[mniestroj]

Though STM32_SRC_HSI48 is not used by the driver (only second cell is used to configure clock, i.e. OTGHS_SEL(0)), it is confusing to leave it like that since OTGHS_SEL(0) select HSE and not HSI48 as input clock.

Switching that to STM32_SRC_HSE seems the way to go. However, we need to disable CONFIG_USB_DC_STM32_CLOCK_CHECK so it won't complain about 48MHz frequency. Or we can update that check (it won't be easy to cover all cases, so I guess we can leave that as future enhancement).

[mniestroj]

Another thing that might be worth considering is calculating 16MHz...32MHz (which in current PR implementation is done with clock-cfg OTG_PHY property) in the driver implementation. In theory we could calculate that speed based on selected clock source (e.g. when we would have clocks = <&clk_hse>; or clocks = <&pll1>; in OTG_PHY). That would probably mean more calculations in runtime, but it would improve developer experience, since there would be much easier way to select clocks (less things to select in devicetree).

[mniestroj]

Please take a look at #82540, which implements following suggestion I had:

Another thing that might be worth considering is calculating 16MHz...32MHz (which in current PR implementation is done with clock-cfg OTG_PHY property) in the driver implementation. In theory we could calculate that speed based on selected clock source (e.g. when we would have clocks = <&clk_hse>; or clocks = <&pll1>; in OTG_PHY). That would probably mean more calculations in runtime, but it would improve developer experience, since there would be much easier way to select clocks (less things to select in devicetree).

[erikarn]

ok, this is all great feedback and ideas, but my goal right now is to get /something/ working into the tree that we can then build on. I kind of feel like I've stumbled into a rabbit hole of many "oh and this sharp corner should be fixed" tasks, which is great and I'd love to help with! But I'd also like to get something working into the tree.

I'll go and re-test today and see if it even works. If it doesn't work without the clock enable then I'd appreciate some hand-holding to get that lined up. Even just getting the /first/ diff in the stack landed to enable USB on this board would be super helpful.

Thanks!

[erikarn]

ok, so testing with the cdc_acm demo again:

• yeah, i needed to enable both clocks for USB init to work
• USB init is fine, CDC-ADM is fine, linux finds it
• DTR is never set when I connect to it
• I hit divide by zero's if I get to the DCD/DSR calls in the demo
• Commenting them out - I get through to baud rate, but now the echo part isn't working

So, there's been a bunch of other regressions in the meantime. This is why I really want to get this landed in some working form first! :)

[erikarn]

ok, PEBKAC - bad USB cable on the USB-C side. Yup, with that clock bit set, the demo still works fine.

So first step - what to do about configuring that missing clock bit (bit 14). This seems like it could be a problem for other boards/parts, no? Should we create a separate diff /just/ for figuring out what to do there?

[erwango]

So first step - what to do about configuring that missing clock bit (bit 14). This seems like it could be a problem for other boards/parts, no? Should we create a separate diff /just/ for figuring out what to do there?

Can you clarify ?

[erikarn]

So first step - what to do about configuring that missing clock bit (bit 14). This seems like it could be a problem for other boards/parts, no? Should we create a separate diff /just/ for figuring out what to do there?

Can you clarify ?

Did any other STM32 parts get caught out by this clock config refactor? is it /just/ the stm32u5 series stuff that was setting >1 clock enable in the RCC?

[erwango]

Did any other STM32 parts get caught out by this clock config refactor? is it /just/ the stm32u5 series stuff that was setting >1 clock enable in the RCC?

Not that I know, should be limited to U5 indeed.

[jfischer-no]

In the last commit, remove (Issue #79823) from the commit title.

[erwango]

Closing as superseded by #82540