USB STM32: Improve usb clock code #87061

kbidani · 2025-03-13T16:02:17Z

This PR includes several improvements to the USB code for STM32 devices. The changes aim to enhance code maintainability and add necessary configurations for PHY clocks.

dts/arm/st/h7/stm32h723.dtsi

drivers/usb/udc/udc_stm32.c

etienne-lms · 2025-04-01T09:23:26Z

drivers/usb/device/usb_dc_stm32.c


+#if USB_OTG_HS_ULPI_PHY || \
+DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy) || \
+USB_OTG_HS_EMB_PHYC


Actually here also #if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC should be enough.

etienne-lms · 2025-04-01T09:23:59Z

drivers/usb/device/usb_dc_stm32.c

-#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy)

-static const struct stm32_pclken phy_pclken[] = STM32_DT_CLOCKS(DT_INST_PHANDLE(0, phys));
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy)


nitpicking: remove the extra empty line above.

etienne-lms · 2025-04-01T09:25:57Z

drivers/usb/device/usb_dc_stm32.c

-	LL_AHB2_GRP1_EnableClock(LL_AHB2_GRP1_PERIPH_USBPHY);
-	/* Both OTG HS and USBPHY sleep clock MUST be disabled here at the same time */
+	clock_control_on(clk, (clock_control_subsys_t)&phy_pclken[0]);
+/* Both OTG HS and USBPHY sleep clock MUST be disabled here at the same time */


indentation

etienne-lms · 2025-04-01T09:30:07Z

boards/arduino/portenta_h7/arduino_portenta_h7-common.dtsi

+		clocks = <&rcc STM32_CLOCK(AHB1, 26U)>;
 		reset-gpios = < &gpioj 4 GPIO_ACTIVE_LOW >;
 		#phy-cells = <0>;
-		clocks = <&rcc STM32_CLOCK(AHB1, 26U)>;


Can discard this change (no change)

boards/adi/sdp_k1/adi_sdp_k1.dts

boards/arduino/nicla_vision/arduino_nicla_vision_stm32h747xx_m7.dts

boards/arduino/portenta_h7/arduino_portenta_h7-common.dtsi

boards/st/stm32f769i_disco/stm32f769i_disco.dts

boards/st/stm32h757i_eval/stm32h757i_eval_stm32h757xx_m7.dts

drivers/usb/device/usb_dc_stm32.c

drivers/usb/udc/udc_stm32.c

dts/arm/st/f7/stm32f7.dtsi

dts/arm/st/h7/stm32h723.dtsi

dts/arm/st/h7/stm32h745.dtsi

mathieuchopstm · 2025-04-03T11:38:09Z

drivers/usb/device/usb_dc_stm32.c

-#else
-	LL_AHB1_GRP1_EnableClock(LL_AHB1_GRP1_PERIPH_OTGHSULPI);
-#endif
+	clock_control_on(clk, (clock_control_subsys_t)&phy_pclken[0]);


I find the duplication of this call all over the place confusing to read.
Is there anything preventing this clock_control_on call from being placed in a single spot, gated behind the same #if USB_OTG_HS_ULPI_PHY || DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy) || \ USB_OTG_HS_EMB_PHYC as the phy_pclken definition?

(If this is possible, having a top-level #define HAS_PHY_CLK 1 gated behind the existing #if test with all series/IP, and testing for HAS_PHY_CLK everywhere else, could be cleaner)

etienne-lms · 2025-04-03T12:00:49Z

drivers/usb/udc/udc_stm32.c

 #if USB_OTG_HS_ULPI_PHY
-#if defined(CONFIG_SOC_SERIES_STM32H7X)
+	clock_control_on(clk, (clock_control_subsys_t)&phy_pclken[0]);
 	LL_AHB1_GRP1_EnableClock(LL_AHB1_GRP1_PERIPH_USB1OTGHSULPI);


Remove line 1151 (duplicates the clock_control_on() call above)

etienne-lms · 2025-04-03T12:00:55Z

drivers/usb/udc/udc_stm32.c

+	clock_control_on(clk, (clock_control_subsys_t)&phy_pclken[0]);
 	/* Both OTG HS and USBPHY sleep clock MUST be disabled here at the same time */
-	LL_AHB2_GRP1_DisableClockStopSleep(LL_AHB2_GRP1_PERIPH_OTG_HS ||
+	LL_AHB2_GRP1_DisableClockStopSleep(LL_AHB2_GRP1_PERIPH_OTG_HS |


This change is a fix. It deserves a dedicated commit IMHO.

etienne-lms · 2025-04-03T12:08:20Z

drivers/usb/device/usb_dc_stm32.c

+	clock_control_on(clk, (clock_control_subsys_t)&phy_pclken[0]);
 	/* Both OTG HS and USBPHY sleep clock MUST be disabled here at the same time */
-	LL_AHB2_GRP1_DisableClockStopSleep(LL_AHB2_GRP1_PERIPH_OTG_HS ||
+	LL_AHB2_GRP1_DisableClockStopSleep(LL_AHB2_GRP1_PERIPH_OTG_HS |


Fixup change, deserves a dedicated commit.

etienne-lms

The 3 last commits changing DTS/DTSI files should be placed before the 2 commits modifying the drivers implementation.

etienne-lms · 2025-04-09T08:31:21Z

drivers/usb/device/usb_dc_stm32.c

+static const struct stm32_pclken phy_pclken[] = STM32_DT_CLOCKS(DT_INST_PHANDLE(0, phys));
+#endif
+
+#if USB_OTG_HS_ULPI_PHY || DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs) || USB_OTG_HS_EMB_PHYC


s/st_stm32_otghs/st_stm32u5_otghs_phy/?
If so, can define HAS_PHY_CLK in the above #ifdef/#endif close.

etienne-lms · 2025-04-10T07:44:23Z

drivers/usb/device/usb_dc_stm32.c

-	LL_AHB1_GRP1_EnableClock(LL_AHB1_GRP1_PERIPH_USB1OTGHSULPI);
-#else
-	LL_AHB1_GRP1_EnableClock(LL_AHB1_GRP1_PERIPH_OTGHSULPI);
+#if HAS_PHY_CLK


Looking again, I'm not sure HAS_PHY_CLK can be used here.

In previous implementaion, line 416 is embedded when both compats st_stm32u5_otghs_phy AND st_stm32_otghs are enabled, that is when USB_OTG_HS_EMB_PHY is true.
So I think here it should be:

#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC || USB_OTG_HS_EMB_PHY

At line 105 (new implementation), you still need to define phy_pclken upon

#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC || DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy)

since it's required by usb_dc_stm32u5_phy_clock_select() and usb_dc_stm32u5_phy_clock_enable().

Could you cross-check that?

yes it's required for u5.

etienne-lms · 2025-04-10T07:45:50Z

drivers/usb/udc/udc_stm32.c

 #define USB_OTG_HS_ULPI_PHY (DT_HAS_COMPAT_STATUS_OKAY(usb_ulpi_phy) && \
 			    DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs))

+#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC


|| USB_OTG_HS_EMB_PHY
(to cover case at line 1135 in previous implementation)

And USB_OTG_HS_EMB_PHY should be defined:

#define USB_OTG_HS_EMB_PHY (DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy) && \ DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs))

(edited: fixed)

Sorry my previous comment was inaccurate and confusing.

For consistency with device/usb_dc_stm32.c implementation, I think we should add commit to rename macro USB_OTG_HS_EMB_PHY to USB_OTG_HS_EMB_PHYC in this udc/udc_stm32.c and then introduce USB_OTG_HS_EMB_PHY related to STM32U5 OTGHS PHY, as done in usb_dc_stm32.c:

#define USB_OTG_HS_EMB_PHYC (DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usbphyc) && \ DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)) +#define USB_OTG_HS_EMB_PHY (DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy) && \ + DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)) + #define USB_OTG_HS_ULPI_PHY (DT_HAS_COMPAT_STATUS_OKAY(usb_ulpi_phy) && \ DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs))

Based on that, your can factorize PHY clock management upon USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC || USB_OTG_HS_EMB_PHY.

Renaming USB_OTG_HS_EMB_PHY to USB_OTG_HS_EMB_PHYC could be controversial. @erwango, what do you think.

TBH, I'd minimize the changes to the minimum considered the pain experienced in previous PR.
But the long term direction sounds fine though, but we should not forget that device/usb_dc_stm32.c should disappear in 1 or 2 releases.

Ok, I understand renaming is not the preferred way.
However I think current proposal won't match: USB_OTG_HS_EMB_PHYC is not defined (..._PHY not ..._PHYC) and U5 case is not considered (at line 1135 in previous implementation).

That would be something like:

#define USB_OTG_HS_EMB_PHY (DT_HAS_COMPAT_STATUS_OKAY(st_stm32_usbphyc) && \ DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)) #define USB_OTG_HS_ULPI_PHY (DT_HAS_COMPAT_STATUS_OKAY(usb_ulpi_phy) && \ DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)) +#define USB_OTG_HS_EMB_U5_PHY (DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy) && \ + DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)) + -#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC +#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHY || USB_OTG_HS_EMB_U5_PHY #define HAS_PHY_CLK

That said, you proposal implemented in the next commit of these series could apply here, no?

-#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC +#if DT_NODE_HAS_PROP(DT_PHANDLE(DT_INST(0, DT_DRV_COMPAT), phys), clocks) #define HAS_PHY_CLK

replaced with: #if DT_NODE_HAS_PROP(DT_PHANDLE(DT_INST(0, DT_DRV_COMPAT), phys), clocks)

etienne-lms · 2025-04-10T07:46:15Z

drivers/usb/udc/udc_stm32.c

-	LL_AHB1_GRP1_EnableClock(LL_AHB1_GRP1_PERIPH_USB1OTGHSULPI);
-#else
-	LL_AHB1_GRP1_EnableClock(LL_AHB1_GRP1_PERIPH_OTGHSULPI);
+#if HAS_PHY_CLK


etienne-lms · 2025-04-10T07:58:45Z

drivers/usb/udc/udc_stm32.c

+	}
 #endif
-#elif DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs) /* USB_OTG_HS_ULPI_PHY */
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs) /* USB_OTG_HS_ULPI_PHY */


By the way, I think an empty line above would be welcome.

erwango · 2025-04-10T12:59:56Z

boards/adi/sdp_k1/adi_sdp_k1.dts

 	otghs_ulpi_phy: otghs_ulpis_phy {
 		compatible = "usb-ulpi-phy";
 		reset-gpios = <&gpiod 7 (GPIO_ACTIVE_LOW)>;
+		clocks = <&rcc STM32_CLOCK(AHB1, 30)>;
 		#phy-cells = <0>;
 	};


I'd propose to move node declaration with status = <disabled>; along with not configurable properties (clock, compatible, phy-cells) to .dtsi files.
Then board description should enable node and populate board specific properties like reset-gpios.
In any case, since it has impact on out of tree boards, this should be documented in migration guide (doc/releases/migration-guide-4.2.rst)

erwango · 2025-04-10T13:06:16Z

drivers/usb/device/usb_dc_stm32.c

 #define USB_DC_STM32_FULL_SPEED             USB_OTG_SPEED_FULL
 #endif
+
+#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC || DT_HAS_COMPAT_STATUS_OKAY(st_stm32u5_otghs_phy)


Is there any way to use DT_NODE_HAS_PROP ?

Something like:
#if DT_NODE_HAS_PROP(DT_PHANDLE(DT_INST(0, DT_DRV_COMPAT), phys), clocks)

erwango · 2025-04-10T13:10:21Z

drivers/usb/udc/udc_stm32.c

 #define USB_OTG_HS_ULPI_PHY (DT_HAS_COMPAT_STATUS_OKAY(usb_ulpi_phy) && \
 			    DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs))

+#if USB_OTG_HS_ULPI_PHY || USB_OTG_HS_EMB_PHYC


TBH, I'd minimize the changes to the minimum considered the pain experienced in previous PR.
But the long term direction sounds fine though, but we should not forget that device/usb_dc_stm32.c should disappear in 1 or 2 releases.

etienne-lms · 2025-04-11T09:11:18Z

drivers/usb/device/usb_dc_stm32.c

+	}
 #endif
-#elif DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs) /* USB_OTG_HS_ULPI_PHY */
+#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs)


Here also I think an empty line above would be welcome.

erwango · 2025-04-11T07:02:29Z

dts/arm/st/f7/stm32f7.dtsi

 		compatible = "usb-nop-xceiv";
 		#phy-cells = <0>;
+		clocks = <&rcc STM32_CLOCK(AHB1, 30)>;
 	};


To be verified, but probably all these nodes should get a status = "disabled";
(When no status is provided, default is "okay").

for stm32f723 and stm32f769 status = "okay"

erwango · 2025-04-11T07:04:27Z

dts/arm/st/h7/stm32h747Xi_m7.dtsi

+
+	otghs_ulpi_phy: otghs_ulpis_phy {
+		compatible = "usb-ulpi-phy";
+		clocks = <&rcc STM32_CLOCK(AHB1, 26)>;
+	};


This should be possible to use this on M4 side, so to be put in stm32h747.dtsi file.

erwango · 2025-04-11T07:04:38Z

dts/arm/st/h7/stm32h757Xi_m7.dtsi

+
+	otghs_ulpi_phy: otghs_ulpis_phy {
+		clocks = <&rcc STM32_CLOCK(AHB1, 26)>;
+	};


To be removed as this will come by stm32h747 file inclusion

erwango · 2025-04-14T06:21:28Z

dts/arm/st/h7/stm32h747Xi_m7.dtsi

+
+	otghs_ulpi_phy: otghs_ulpis_phy {
+		compatible = "usb-ulpi-phy";
+		#phy-cells = <0>;
+		clocks = <&rcc STM32_CLOCK(AHB1, 26)>;
+	};


To be moved to stm32h747.dtsi, or to be checked if it wouldn't be obtained by the already available inlcusion of stm32h745.dtsi.

moved to stm32h747 dtsi file.

erwango · 2025-04-14T06:23:28Z

boards/arduino/portenta_h7/arduino_portenta_h7-common.dtsi

 		compatible = "usb-ulpi-phy";
 		reset-gpios = < &gpioj 4 GPIO_ACTIVE_LOW >;
 		#phy-cells = <0>;
+		clocks = <&rcc STM32_CLOCK(AHB1, 26)>;


Part of phy descriptions that are not board specifics should be moved to soc files

erwango · 2025-04-14T06:23:50Z

boards/arduino/nicla_vision/arduino_nicla_vision_stm32h747xx_m7.dts


 	otghs_ulpi_phy: otghs_ulpis_phy {
 		compatible = "usb-ulpi-phy";
+		clocks = <&rcc STM32_CLOCK(AHB1, 26)>;


add phy clocks to dtsi files. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

Replace the direct call to LL_AHBx_GRP1_EnableClock with the clock control API. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

corrected the logical OR (||) to bitwise OR (|) in the LL_AHB2_GRP1_DisableClockStopSleep function call. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

github-actions · 2025-06-15T00:43:55Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

kbidani force-pushed the usb_clean_clock branch 2 times, most recently from 7556439 to 54f099e Compare March 14, 2025 11:51

kbidani changed the title ~~Improve usb clock code~~ USB STM32: Improve usb clock code Mar 14, 2025

kbidani force-pushed the usb_clean_clock branch 11 times, most recently from 25be4dc to 70cf204 Compare March 29, 2025 10:01

etienne-lms reviewed Apr 1, 2025

View reviewed changes

mathieuchopstm reviewed Apr 1, 2025

View reviewed changes

kbidani force-pushed the usb_clean_clock branch 2 times, most recently from 8216b18 to e8bd04d Compare April 1, 2025 14:44

kbidani requested review from etienne-lms and mathieuchopstm April 1, 2025 14:46

mathieuchopstm reviewed Apr 3, 2025

View reviewed changes

etienne-lms reviewed Apr 3, 2025

View reviewed changes

kbidani force-pushed the usb_clean_clock branch from e8bd04d to f22a8a2 Compare April 4, 2025 21:04

kbidani requested review from etienne-lms and mathieuchopstm April 4, 2025 21:06

kbidani force-pushed the usb_clean_clock branch from f22a8a2 to e9f3b90 Compare April 9, 2025 08:20

etienne-lms reviewed Apr 9, 2025

View reviewed changes

etienne-lms reviewed Apr 10, 2025

View reviewed changes

kbidani requested a review from etienne-lms April 10, 2025 08:14

erwango requested changes Apr 10, 2025

View reviewed changes

kbidani force-pushed the usb_clean_clock branch 3 times, most recently from 6b538ac to 4e1d494 Compare April 10, 2025 16:40

etienne-lms reviewed Apr 11, 2025

View reviewed changes

kbidani force-pushed the usb_clean_clock branch 2 times, most recently from bd5f229 to 81fe5a0 Compare April 13, 2025 17:50

erwango requested changes Apr 14, 2025

View reviewed changes

kbidani force-pushed the usb_clean_clock branch from 81fe5a0 to 67e120e Compare April 14, 2025 09:54

dts: arm: st: add phy clocks to dtsi

593ee1b

add phy clocks to dtsi files. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

kbidani force-pushed the usb_clean_clock branch 2 times, most recently from 07f922d to 884526e Compare April 14, 2025 15:15

kbidani requested review from erwango and etienne-lms April 14, 2025 15:41

kbidani added 5 commits April 15, 2025 09:34

boards: adi: sdp_k1: add phy clocks to dtsi

0feb7a4

add phy clocks to dtsi files. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

drivers: usb: udc_stm32: imporve usb code maintainability

f3e61f4

Replace the direct call to LL_AHBx_GRP1_EnableClock with the clock control API. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

drivers: usb: device_stm32: imporve usb code maintainability

5f46860

Replace the direct call to LL_AHBx_GRP1_EnableClock with the clock control API. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

drivers: usb: device_stm32: corrected the logical OR || to |

ff7bcef

corrected the logical OR (||) to bitwise OR (|) in the LL_AHB2_GRP1_DisableClockStopSleep function call. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

drivers: usb: udc_stm32: corrected the logical OR || to |

b5ccd7c

corrected the logical OR (||) to bitwise OR (|) in the LL_AHB2_GRP1_DisableClockStopSleep function call. Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>

kbidani force-pushed the usb_clean_clock branch from 884526e to b5ccd7c Compare April 15, 2025 07:34

etienne-lms approved these changes Apr 15, 2025

View reviewed changes

vai-rikar mentioned this pull request Jun 3, 2025

drivers: usb: stm32: fix stm32u5x clock disable in sleep mode #88082

Merged

github-actions bot added the Stale label Jun 15, 2025

github-actions bot closed this Jun 29, 2025

USB STM32: Improve usb clock code #87061

USB STM32: Improve usb clock code #87061

Uh oh!

Conversation

kbidani commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etienne-lms Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etienne-lms Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbidani commented Mar 13, 2025 •

edited

Loading

etienne-lms Apr 10, 2025 •

edited

Loading

etienne-lms Apr 10, 2025 •

edited

Loading