Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/stm32: improvement of USB device driver selection #18787

Conversation

gschorcht
Copy link
Contributor

Contribution description

This PR provides an improved selection of USB device driver for periph/usbdev and pkg/tinyusb.

There are STM32 families where all models use only the Synopsys DWC2 USB OTG core while others completely use only the USB Device FS core. For these families either the driver drivers/usbdev_synopsys_dwc2 or the driver cpu/stm32/periph/usbdev is used depending on the respective family. However, the STM32 families F1 and L4 use both cores. The correct driver must therefore be selected depending on the CPU line or the CPU model.

Testing procedure

Since the definitions used by the modules drivers/usbdev_synopsys_dwc2, periph/usbdev, tinyusb_portable_synopsys_dwc and tinyusb_portable_stm32_fsdev are provided by the header files in CMSIS and are only present if the corresponding USB core is used by the STM32 MCU, a successful CI compilation is sufficient proof for the correct driver selection.

Issues/PRs references

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 23, 2022
@gschorcht gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 23, 2022
@gschorcht gschorcht requested a review from maribu October 23, 2022 16:20
@riot-ci
Copy link

riot-ci commented Oct 23, 2022

Murdock results

✔️ PASSED

112f0c6 pkg/tinyusb: improvement of USB driver selection for STM32

Success Failures Total Runtime
2000 0 2000 06m:45s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@gschorcht
Copy link
Contributor Author

To be honest, I'm not really happy with the large selection list in cpu/stm32/periph/Kconfig and pkg/tinyusb/Kconfig. It might be better to define additional features, e.g. HAS_PERIPH_USBDEV_OTG and HAS_PERIPH_USBDEV_FSDEV, which are selected in cpu/stm32/kconfig/*/Kconfig for a whole family or in cpu/stm32/kconfig/*/Kconfig.lines for a certain line. However, the later file is generated automatically and such changes would be overwritten when cpu/stm32/dist/kconfig/gen_kconfig.py is used next time. I don't know whether cpu/stm32/dist/kconfig/gen_kconfig.py could be extended to extract the used USB core from the ProductsList.xlsx files.

@gschorcht gschorcht force-pushed the cpu/stm32/usbdev_synopsys_dwc_dependency branch 3 times, most recently from 885be9c to 22c69f5 Compare October 23, 2022 20:29
@MrKevinWeiss
Copy link
Contributor

I agree with that. This is also some why we have the HAVE_*** design pattern, as HAS_* is a "keyword" for make (ie FEATURE_PROVIDED. HAVE_*` is our convention that better describes the capabilities of a board (or cpu, of family, or line) that doesn't need to worry about all the dependencies. That way there would be fewer places that the dependencies need to be checked. This still means that for each stm family it would need to be added, but personally I like that better as a verbose description of a board is often preferred that a complicated module selection.

I will take a closer look when I am done with the stm clock tree rework...

@maribu
Copy link
Member

maribu commented Oct 24, 2022

It does look like the xlsx file containing the relevant data on USB support. So I guess extending the script should be very much possible.

Part Number General Description Marketing Status Package Core Operating Frequency (MHz) FPU Flash Size (kB) (Prog) Dual-bank Flash RAM Size (kB) Timers (16-bit) typ Timers (32-bit) typ Other timer functions A/D Converters 12-bit Number of Channels typ D/A Converters (12-bit) typ I/Os (High Current) Display controller Graphic accelerator CAN I2C typ SPI typ I2S typ USART typ UART typ Additional Interfaces External Memory Interfaces USB Type Cryptography Security Functions Ethernet Ethernet ports typ Supply Voltage (V) min Supply Voltage (V) max Supply Current (µA) (@ Lowest Power) typ Supply Current (µA) (Run Mode (per MHz)) typ Operating Temperature (°C) min Operating Temperature (°C) max Buy On Line
STM32F722VE High-performance and DSP with FPU, Arm Cortex-M7 MCU with 512 Kbytes of Flash memory, 216 MHz CPU, Art Accelerator, L1 cache, SDRAM Active LQFP 100 14x14x1.4 mm Arm Cortex-M7 216 true 512 - 256 12 2 Dual Watchdog,RTC,SysTick 3 16 2 82 - - 1 4 4 3 4 4 SAI,SD/MMC Dual Quad SPI,FMC USB OTG FS + USB OTG FS/HS TRNG Abnormal Situation Handling,Application Lifecycle,Secure Boot,Secure Install/Update,Silicon Device Lifecycle,Software IP Protection - - 1.7 3.6 - - -40 105,85 true
STM32F722IC High-performance and DSP with FPU, Arm Cortex-M7 MCU with 256 Kbytes of Flash memory, 216 MHz CPU, Art Accelerator, L1 cache, SDRAM Active LQFP 176 24x24x1.4 mm,UFBGA 176+25 10x10x0.6 P 0.65 mm Arm Cortex-M7 216 true 256 - 256 12 2 Dual Watchdog,RTC,SysTick 3 24 2 140 - - 1 4 5 3 4 4 SAI,SD/MMC Dual Quad SPI,FMC USB OTG FS + USB OTG FS/HS TRNG Abnormal Situation Handling,Application Lifecycle,Secure Boot,Secure Install/Update,Silicon Device Lifecycle,Software IP Protection - - 1.7 3.6 - - -40 85 false

Maybe we can try to summon @aabadie. The possible values in the USB Type column seem to be:

  • -
  • USB OTG FS + USB OTG FS/HS (e.g. STM32F722xx)
  • USB OTG FS,USB OTG FS + USB OTG FS/HS (e.g. STM32F720xx)
  • USB Device (e.g. STM32F103xx)
  • USB OTG FS (e.g. STM32F105xx)

@gschorcht
Copy link
Contributor Author

It does look like the xlsx file containing the relevant data on USB support. So I guess extending the script should be very much possible.

Yes. My concern is that it would be the only feature extracted from the ProductsList.xlsx file and defined as HAVE_... in cpu/stm32/kconfigs/*/Kconfig.lines or cpu/stm32/kconfigs/*/Kconfig.models.

@MrKevinWeiss
Copy link
Contributor

Note: that this will be fixing the issue in nightlies for the nucleo-f3*

@MrKevinWeiss
Copy link
Contributor

As it has been 7 days with broken nightlies I would make a 1 liner fix PR if there are no objections. Already some merge conflicts need to be sorted out...

@maribu
Copy link
Member

maribu commented Nov 3, 2022

+1 for fixing now with a large selection list and cleaning up later :)

There are STM32 families where all models use only the Synopsys DWC2 USB OTG core while others completely use only the USB Device FS core. For these families then either the driver `drivers/usbdev_synopsys_dwc2` or the driver `cpu/stm32/periph/usbdev` is used depending on the respective family. However, the STM32 families F1 and L4 use both cores. The correct driver must therefore be selected depending on the CPU line or CPU model.
There are STM32 families where all models use only the Synopsys DWC2 USB OTG core while others completely use only the USB Device FS core. For these families then either the driver `drivers/usbdev_synopsys_dwc2` or the driver `cpu/stm32/periph/usbdev` is used depending on the respective family. However, the STM32 families F1 and L4 use both cores. The correct driver must therefore be selected depending on the CPU line or CPU model.
@gschorcht gschorcht force-pushed the cpu/stm32/usbdev_synopsys_dwc_dependency branch from 22c69f5 to 112f0c6 Compare November 3, 2022 12:57
@gschorcht
Copy link
Contributor Author

I have rebased the PR to resolve the conflicts.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Tested all stm32 based CPU boards with NIGHTLY=1 and #18797 patched on tests/pkg_tinyusb_cdc_msc/ and tests/shell. Everything passed without mismatches.

Cleanup can be done later... This may make the nightlies pass again, so ACK.

@MrKevinWeiss MrKevinWeiss merged commit 19943d2 into RIOT-OS:master Nov 4, 2022
@gschorcht gschorcht deleted the cpu/stm32/usbdev_synopsys_dwc_dependency branch November 6, 2022 17:27
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants