Add support for stm32u595/5a5 OTG_HS in client mode #3613

mubes · 2024-12-05T16:38:49Z

Simple changes to support OTG-HS on the stm32u5 family. New to this so yell up if there's something to be done differently.

embassy-stm32/src/usb/otg.rs

Dirbaio · 2024-12-05T16:50:27Z

embassy-stm32/src/usb/otg.rs

+
+            // Only the 32MHz clock is suitable here, which the magic number represents
+            crate::pac::SYSCFG.otghsphycr().modify(|w| {
+                w.set_clksel(11);


why 32mhz only? could you do like the PR that added this for H7RS? It looks at the freq and sets the right multiplier in the PLL. https://github.com/embassy-rs/embassy/pull/3337/files#diff-74f374f4cb5810def4ca10a02262115a42a22e943591b534a71ffd519f488ea4R569-R581

The U5 has a new core clock for the USB subsystem with different clock requirements but I agree that could be done more dynamically.

See comments below.

which comment?

ah the "For OTG-HS on STM32U5 only the 32MHz clock is fast enough (Table 762, Sect 73.14.4)" thing.

It's not quite right, the OTG_HS has two different clock inputs:

The APB clock, which must be 30mhz or higher according to Table 762.

The kernel clock. It's fed from a PLL which takes a clock from the OTGHSSEL mux. The PLL can take as input any of 16 MHz, 19.2 MHz, 20 MHz, 24 MHz, 26 MHz, 32 MHz.

so all these frequencies are OK to use.

I'm not seeing the reference to Usbrefcksel when I compare with the H7RS code, so I'm leaving this one open for more experienced hands.

There is no enum yet in the stm32-data crate for it, I'll add one similarly to how it's done for the H7 series.

Added the missing enum for the syscfg reg in this PR: embassy-rs/stm32-data#545

MDr164

Looks good so far, I'll get around testing it tomorrow on hardware and let you know how it went.

embassy-stm32/src/usb/otg.rs

MDr164 · 2024-12-05T16:50:54Z

embassy-stm32/src/usb/mod.rs

    if freq.0.abs_diff(48_000_000) > 120_000 {
        panic!(
            "USB clock should be 48Mhz but is {} Hz. Please double-check your RCC settings.",
            freq.0
        )
    }

+    // For OTG-HS on STM32U5 only the 32MHz clock is fast enough (Table 762, Sect 73.14.4)


I recall that in the general datasheet of the U5 it mentioned the HSE clock explicitly needed as well but that might be an implementation detail not relevant here but rather in RCC. Just wanted to write it down so I don't forget about it during testing.

That's the reason I put the explicit reference. I thought this tied together with the otgphycr issue above, and the table only gives a value for 30MHz+ entry. I may have wrapped this around my neck but the PLL1_P was also setting the AHB frequency?

USB requires a crystal oscillator (so, HSE) because RC oscillators (like MSI, HSI) are not precise enough to meet the USB spec. This is the case for all chips, and both FS and HS.

(there are exceptions, many stm32 chips have some mechanism to calibrate HSI/MSI against either USB SOF or LSE which does make them precise enough)

Currently Embassy doesn't enforce this, on any chips. Not sure if we should, using USB from HSI does work in practice and if the user really wants to violate the spec we should let them (so it should have an opt-out at least). Also it's not trivial to implement. So IMO it's out of scope for this PR, and if we do it we should do it for all chips.

OK, I think it's time for me to throw this one over the wall now for others with more experience to look at. For me it works. Outstanding issue just really seems to be to bring the PLL mapping over from the H7RS code but I quickly got myself in a mess with that.

There are some enums and defines still missing in the pac for the U5 series that I can add later. Otherwise this looks pretty good imo.

examples/stm32u5/src/bin/usb_hs_serial.rs

embassy-stm32/src/usb/otg.rs

MDr164

I tested it on my hardware here and it works just fine. I'll update some of the code according to feedback regarding the clocks if you don't mind.

MDr164 · 2024-12-06T12:17:32Z

embassy-stm32/src/usb/otg.rs

+
+            // Only the 32MHz clock is suitable here, which the magic number represents
+            crate::pac::SYSCFG.otghsphycr().modify(|w| {
+                w.set_clksel(11);


There is no enum yet in the stm32-data crate for it, I'll add one similarly to how it's done for the H7 series.

MDr164 · 2024-12-06T12:19:01Z

embassy-stm32/src/usb/mod.rs

    if freq.0.abs_diff(48_000_000) > 120_000 {
        panic!(
            "USB clock should be 48Mhz but is {} Hz. Please double-check your RCC settings.",
            freq.0
        )
    }

+    // For OTG-HS on STM32U5 only the 32MHz clock is fast enough (Table 762, Sect 73.14.4)


There are some enums and defines still missing in the pac for the U5 series that I can add later. Otherwise this looks pretty good imo.

mubes · 2024-12-06T12:30:14Z

Please go ahead and make whatever changes are appropriate!

…

On Fri, 6 Dec 2024, 12:20 Marvin Drees, ***@***.***> wrote: ***@***.**** commented on this pull request. I tested it on my hardware here and it works just fine. I'll update some of the code according to feedback regarding the clocks if you don't mind. ------------------------------ In embassy-stm32/src/usb/otg.rs <#3613 (comment)>: > @@ -311,6 +314,26 @@ impl<'d, T: Instance> Bus<'d, T> { }); }); + #[cfg(all(stm32u5, peri_usb_otg_hs))] + { + crate::pac::RCC.ccipr2().modify(|w| { + w.set_otghssel(Otghssel::PLL1_P); + }); + + // Only the 32MHz clock is suitable here, which the magic number represents + crate::pac::SYSCFG.otghsphycr().modify(|w| { + w.set_clksel(11); There is no enum yet in the stm32-data crate for it, I'll add one similarly to how it's done for the H7 series. ------------------------------ In embassy-stm32/src/usb/mod.rs <#3613 (comment)>: > if freq.0.abs_diff(48_000_000) > 120_000 { panic!( "USB clock should be 48Mhz but is {} Hz. Please double-check your RCC settings.", freq.0 ) } + // For OTG-HS on STM32U5 only the 32MHz clock is fast enough (Table 762, Sect 73.14.4) There are some enums and defines still missing in the pac for the U5 series that I can add later. Otherwise this looks pretty good imo. — Reply to this email directly, view it on GitHub <#3613 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJTBD66SE6T7I5ADZR7W232EGJBFAVCNFSM6AAAAABTC3XRZCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBUGY4TMMJXGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

MDr164 · 2024-12-06T15:29:14Z

I can't push to your branch as I got no write access for it but this patch is how I intend to fixup some of the clock stuff. It's a bit awkward to call SYSCFG inside the RCC code as the Reg to set the reference clock is part of SYSCFG and not RCC for the U5.

0001-Update-STM32U5-OTG-HS-clock-handling.txt

MDr164 · 2024-12-06T18:02:39Z

I updated the PR with an update to the clock selection, not 100% if that is the best way to do it on the U5 but still works on my hardware here. I think the CI failure is due to #3621 ?

Dirbaio · 2024-12-10T11:19:27Z

lgtm! can you fix CI?

MDr164 · 2024-12-10T11:48:29Z

lgtm! can you fix CI?

I was already looking at it but I'm confused why it fails. Am I misunderstanding the conditional compilation flags here? It should either match the first check for STM32H7RS and any STM32U5 with OTG_HS or any other platform so freq should be used in any case. I could just rename it to _freq but that shouldn't be needed as it wasn't needed before?

Dirbaio · 2024-12-10T11:56:03Z

embassy-stm32/src/usb/mod.rs

@@ -16,6 +16,7 @@ fn common_init<T: Instance>() {

    // On the H7RS, the USBPHYC embeds a PLL accepting one of the input frequencies listed below and providing 48MHz to OTG_FS and 60MHz to OTG_HS internally
    #[cfg(stm32h7rs)]
+    #[cfg(all(stm32u5, peri_usb_otg_hs))]


ah it's failing because when you apply two cfg to the same statement, the conditiosn are ANDed, not ORed. So, this code is actually never compiled (no chip is H7RS and U5 at the same time!).

this should work: #[cfg(any(stm32h7rs, all(stm32u5, peri_usb_otg_hs)))]

Oh I always thought multiple cfg statements are ORed. Let me quickly fix the cfg statement in this PR and then it should be good to go. @mubes please also test on your hardware once to see if everything still works to your liking as well as it's your PR after all.

Dirbaio · 2024-12-10T11:57:26Z

embassy-stm32/src/usb/mod.rs

@@ -26,6 +27,7 @@ fn common_init<T: Instance>() {
    // Clock might not be exact 48Mhz due to rounding errors in PLL calculation, or if the user
    // has tight clock restrictions due to something else (like audio).
    #[cfg(not(stm32h7rs))]
+    #[cfg(not(all(stm32u5, peri_usb_otg_hs)))]


could you change this to #[cfg(not(any(....)))] ? for consistency with the other cfg above, and also because it's easier to read. having twocfgs can get confusing.

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>

MDr164 · 2024-12-10T12:17:31Z

lgtm! can you fix CI?

Done, fixed all the cfg statements and CI is happy

mubes · 2024-12-10T13:29:23Z

This is all working fine in my builds. From my side I think it can be committed. Thanks for the work folks!

MDr164 · 2024-12-11T11:09:54Z

This closes embassy-rs/stm32-data#519 btw as all registers are there and confirmed working

Dirbaio reviewed Dec 5, 2024

View reviewed changes

embassy-stm32/src/usb/otg.rs Outdated Show resolved Hide resolved

Dirbaio reviewed Dec 5, 2024

View reviewed changes

MDr164 reviewed Dec 5, 2024

View reviewed changes

mubes force-pushed the stm32u5_otg_hs_support branch from 9bd11a2 to 6b1f3b1 Compare December 5, 2024 17:16

MDr164 reviewed Dec 5, 2024

View reviewed changes

examples/stm32u5/src/bin/usb_hs_serial.rs Outdated Show resolved Hide resolved

MDr164 reviewed Dec 5, 2024

View reviewed changes

embassy-stm32/src/usb/otg.rs Outdated Show resolved Hide resolved

mubes force-pushed the stm32u5_otg_hs_support branch 4 times, most recently from 94d2ae3 to a6f4e8e Compare December 6, 2024 00:06

MDr164 reviewed Dec 6, 2024

View reviewed changes

MDr164 mentioned this pull request Dec 6, 2024

Add USBREFCLK enum for OTG HS PHY embassy-rs/stm32-data#545

Merged

MDr164 force-pushed the stm32u5_otg_hs_support branch from 6198fd5 to 9daa7fc Compare December 6, 2024 17:59

Add support for stm32u595/5a5 OTG_HS in client mode

501d394

MDr164 force-pushed the stm32u5_otg_hs_support branch from 9daa7fc to 57b5267 Compare December 10, 2024 09:52

Dirbaio reviewed Dec 10, 2024

View reviewed changes

MDr164 force-pushed the stm32u5_otg_hs_support branch 2 times, most recently from bbd6585 to 7af066e Compare December 10, 2024 12:06

Update STM32U5 OTG HS clock handling

a0e056a

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>

MDr164 force-pushed the stm32u5_otg_hs_support branch from 7af066e to a0e056a Compare December 10, 2024 12:10

Dirbaio added this pull request to the merge queue Dec 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 11, 2024

Dirbaio added this pull request to the merge queue Dec 13, 2024

Merged via the queue into embassy-rs:main with commit e44ee26 Dec 13, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for stm32u595/5a5 OTG_HS in client mode #3613

Add support for stm32u595/5a5 OTG_HS in client mode #3613

mubes commented Dec 5, 2024

Dirbaio Dec 5, 2024

MDr164 Dec 5, 2024

mubes Dec 5, 2024

Dirbaio Dec 5, 2024

Dirbaio Dec 5, 2024

mubes Dec 5, 2024

MDr164 Dec 6, 2024

MDr164 Dec 6, 2024

MDr164 left a comment

MDr164 Dec 5, 2024

mubes Dec 5, 2024

Dirbaio Dec 5, 2024

mubes Dec 5, 2024

MDr164 Dec 6, 2024

MDr164 left a comment

MDr164 Dec 6, 2024

MDr164 Dec 6, 2024

mubes commented Dec 6, 2024 via email

MDr164 commented Dec 6, 2024

MDr164 commented Dec 6, 2024

Dirbaio commented Dec 10, 2024

MDr164 commented Dec 10, 2024

Dirbaio Dec 10, 2024 •

edited

Loading

MDr164 Dec 10, 2024

Dirbaio Dec 10, 2024

MDr164 Dec 10, 2024

MDr164 commented Dec 10, 2024

mubes commented Dec 10, 2024

MDr164 commented Dec 11, 2024

Add support for stm32u595/5a5 OTG_HS in client mode #3613

Add support for stm32u595/5a5 OTG_HS in client mode #3613

Conversation

mubes commented Dec 5, 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

MDr164 left a 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

MDr164 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mubes commented Dec 6, 2024 via email

MDr164 commented Dec 6, 2024

MDr164 commented Dec 6, 2024

Dirbaio commented Dec 10, 2024

MDr164 commented Dec 10, 2024

Dirbaio Dec 10, 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

MDr164 commented Dec 10, 2024

mubes commented Dec 10, 2024

MDr164 commented Dec 11, 2024

Dirbaio Dec 10, 2024 •

edited

Loading