Skip to content

Conversation

kbidani
Copy link
Contributor

@kbidani kbidani commented Feb 20, 2025

This pull request introduces several enhancements and refactorings to the USB code, device tree bindings, and board configurations for STM32 devices. The changes aim to improve the maintainability of the USB driver code.

@kbidani kbidani force-pushed the usb_clean_up branch 8 times, most recently from 3495aa9 to 17bc665 Compare February 25, 2025 10:58
@kbidani kbidani closed this Feb 25, 2025
@kbidani kbidani reopened this Feb 25, 2025
@kbidani kbidani force-pushed the usb_clean_up branch 7 times, most recently from 93fb344 to 8c781c7 Compare February 25, 2025 16:53
@kbidani kbidani marked this pull request as ready for review February 26, 2025 08:52
@zephyrbot zephyrbot added area: USB Universal Serial Bus platform: STM32 ST Micro STM32 labels Feb 26, 2025
@kbidani kbidani force-pushed the usb_clean_up branch 2 times, most recently from 093c9b8 to 49b364e Compare May 7, 2025 20:28
uint8_t physel;

#if DT_HAS_COMPAT_STATUS_OKAY(usb_ulpi_phy)
physel = USB_OTG_HS_PHY_ULPI;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a comment here stating that if "usb-ulpi-phy" is enabled on board.dts, we suppose it is selected (otherwise it doesn't make no sense to define it).

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, otherwise LGTM

@kbidani kbidani requested a review from erwango May 9, 2025 15:34
@kbidani kbidani marked this pull request as ready for review May 12, 2025 07:01
@kbidani
Copy link
Contributor Author

kbidani commented May 12, 2025

@jfischer-no PTAL

@kbidani kbidani dismissed stale reviews from JarmouniA and etienne-lms via 645120b May 13, 2025 14:34
Comment on lines 65 to 68
/* Definitions aligned with HAL macros for PHY type selection */
#define USB_OTG_HS_PHY_ULPI 1U
#define USB_FS_EMBEDDED_PHY 2U
#define USB_OTG_HS_EMBEDDED_PHY 3U
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I missed the explanations for these. But these macros are not properly prefixed and USB_OTG_HS_EMBEDDED_PHY is already defined in the HAL, would it not clash at some point? Also, I see two other values defined there, why cannot they be used here in the UDC driver?

Copy link
Contributor Author

@kbidani kbidani May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Macros are now prefixed with UDC_STM32 to avoid any naming conflicts.
Another value is defined there but is not used here:
#if !defined (USB_HS_PHYC_TUNE_VALUE)
#define USB_HS_PHYC_TUNE_VALUE 0x00000F13U /*!< Value of USB HS PHY Tune /
#endif /
USB_HS_PHYC_TUNE_VALUE */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USB_OTG_HS_EMBEDDED_PHY is already defined in the HAL, would it not clash at some point? Also, I see two other values defined there, why cannot they be used here in the UDC driver?

There is one question that remains unanswered. Why cannot the macros from the HAL be used directly in the UDC driver?


/* If the ULPI interface for an external high-speed PHY is enabled, it is assumed to be chosen. */
#if DT_HAS_COMPAT_STATUS_OKAY(usb_ulpi_phy)
physel = USB_OTG_HS_PHY_ULPI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

physel = USB_OTG_ULPI_PHY; ?

(DT_PHANDLE(DT_INST(0, st_stm32_usb), phys) == otgfs_phy) || \
(DT_PHANDLE(DT_INST(0, st_stm32_usb), phys) == usb_fs_phy)) && \
DT_HAS_COMPAT_STATUS_OKAY(usb_nop_xceiv))
physel = USB_FS_EMBEDDED_PHY;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

physel = USB_OTG_EMBEDDED_PHY; ?

kbidani added 5 commits May 25, 2025 23:08
Replace the conditional compilation blocks with a single assignment.
Rely on the DT reg property for the USB controller instance
IOMEM base address.

Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>
Replace the conditional compilation blocks with a single assignment.
Rely on the DT reg property for the USB controller instance
IOMEM base address.

Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>
Replace the conditional compilation blocks with a dedicated
function to determine the USB PHY interface selection for STM32
using device tree configurations.
Define generic macros for USB physel that supported by STM32 boards,
while the HALs use different macro names for identical values.

Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>
Replace the conditional compilation blocks with a dedicated
function to determine the USB PHY interface selection for STM32
using device tree configurations.
Define generic macros for USB physel that supported by STM32 boards,
while the HALs use different macro names for identical values.

Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>
Factorize 'USB_NUM_BIDIR_ENDPOINTS' macro use in usb_dc_stm32_init().
Modifie the condition to verify ep_idx from > to >=.

Signed-off-by: Khaoula Bidani <khaoula.bidani-ext@st.com>
Copy link

@erwango erwango dismissed their stale review May 28, 2025 13:34

Dismissing in order not to block next steps.

Copy link

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.

@mathieuchopstm
Copy link
Contributor

Mostly superseded by #96028.

Feel free to re-open with only the clean-up commits; otherwise, they will be integrated as part of another PR with more cleanups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree area: USB Universal Serial Bus platform: ADI Analog Devices, Inc. platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants