Skip to content

gen-device-stm32: fix USB_OTG_FS groupName inconsistency - #5562

Open
knieriem wants to merge 6 commits into
tinygo-org:devfrom
knieriem:fix-usbotg-inconsistency
Open

knieriem wants to merge 6 commits into
tinygo-org:devfrom
knieriem:fix-usbotg-inconsistency

Conversation

@knieriem

Copy link
Copy Markdown
Contributor

This PR attempts to address #5154.

[When working on the stm32 svd update in February, it also had a solution for that issue in mind, since I already had been working on gen-device-svd at that time. But I did not implement it before trying to re-add my stm32h723 support based on work on h753 merged into dev recently. Esp. when trying to adjust machine_stm32h7_usb.go to the h723 I was reminded of #5154 again.]

This change adjusts gen-device-svd so that it not just generates one USB_OTG_{FS|HS}_Type (with a register set matching one of OTG_{FS|HS}_{GLOBAL|DEVICE|HOST|PWRCLK} while shadowing the others).
Instead, as the original issue comment suggests, now it generates distinct types USB_OTG_{FS|HS}_{GLOBAL|DEVICE|HOST|PWRCLK}_Type, and also sub types USB_OTG_HS_DEVICE_{DIEP|DOEP}_Type.

This results also in modified names of flag, pos and mask definitions, specifically definitions for the GLOBAL interfaces -- these now have _GLOBAL in their names. I think this is the only breaking changemachine_stm32_otgfs_usb.go and related files have been adjusted so that they compile again. More changes could be applied to take advantage of the new DEVICE related flags and mask definitions.

STM32 MCUs affected by this change are those of families f[1247], h7, and l4.
I checked that no unintended changes are introduced to other SVD based platforms.

Also, processCluster now tries to detect the case where one cluster with a specialized index-0 item is followed by a dim array cluster for items 1..N. This is used in the following case: Since the previously ignored .._DEVICE_Type is now visible, the ..DEVICE_{DIEP|DOEP}_Types are visible now too. The change to cluster handling ensures that there is not a ..DEVICE_DIEP0_Type for index 0 and a ..DEVICE_DIEP_Type array for the following endpoints 1..N, by detecting and merging these adjacent clusters into one (this also ensures that endpoint register flag definitions are not split into DIEP0 and DIEP versions).

[A question will be if we actually want to use these kind of type and const definitions. What I realized when extending the stm32g0 CAN driver to h7, it could also be an advantage not to use the definitions in device/xyz if something like CAN IP (Bosch) or Synopsis OTG IP is involved, which could lead to generic drivers that work on any MCU with such peripherals, with vendor-specific adaptions.
But it won't hurt if the device/stm32/* definitions are structured better.]


One of the changes fixes size calculation for non-array clusters (the size of the last element was omitted from the cluster size). This removes warnings like:

peripheral: CTRLAP_NS cluster: CTRLAPPERI_ERASEPROTECT size: 4 struct size: 8

when generating device/nrf files, it also fixes a few struct layout of the (unused) nrf9160 and nrf5340,
which appear to use sub-clusters. I found this bug when implementing the cluster merge.

knieriem added 6 commits July 28, 2026 12:27
Now there will be distinct types like
USB_OTG_FS_HOST_Type, USB_OTG_FS_DEVICE_Type,
USB_OTG_FS_GLOBAL_Type, not just one
USB_OTG_FS_Type that shadows the other types.
See tinygo-org#5154.
…/stm32

Now there is stm32.OTG_FS_DEVICE and stm32.OTG_FS_PWRCLK,
and GLOBAL related flags now have "_GLOBAL" in their names.
For non-array clusters, extend lastAddress to the position behind
the last element — so that the last element, which is part of the
cluster, is actually included in the size calcution.
This removes a warning at generation stage, if the calculated size does not
match the struct's size.
This is a preparatory step towards merging
clusters separated into e.g. DIEP0 and DIEP%s.
…ent clusters

This improves OTG_{HS|FS}_DEVICE_Type structure,
and makes DIEP and DOEP types more useful.
Also, it adjusts corresponding register flag names
(using ".._DIEP_.." rather than ".._DIEP0_..").
@deadprogram

Copy link
Copy Markdown
Member

Thanks @knieriem for this work. The following notes are edited from an automated review.

I built the generator at this branch and at its merge base, then regenerated all SVD sets and compared the output. The scope agrees with your description: 63 generated files change, all in stm32 f1/f2/f4/f7/h7/l4 plus nrf9160 and nrf5340. No other vendor output changes. The cluster size fix also corrects the SPU_Type layout in nrf9160, where 7 registers were at the wrong offset before.

  1. The DIEP0/DIEP%s merge keeps only the index-0 registers, so the EP0 bit layout applies to endpoints 1..N.

In processCluster, clusterRegisters still comes from cluster.Registers, and clusters[i+1] is skipped. The struct layout is correct, but the field constants and the accessors are not. Examples from the generated output:

constant this branch value for EP1..N
USB_OTG_FS_DEVICE_DIEP_CTL_MPSIZ_Msk (f405) 0x3 0x7ff
USB_OTG_HS_DEVICE_DOEP_CTL_MPSIZ_Msk (h743) 0x3 0x7ff
USB_OTG_HS_DEVICE_DOEP_TSIZ_XFRSIZ_Msk (h743) 0x7f 19 bits

SetCTL_MPSIZ on USB_OTG_HS_DEVICE_DOEP_Type masks 0x3 for all 6 array elements, and its doc comment still says "endpoint 0 control register".

I scanned all SVD files in lib/stm32-svd, lib/nrfx/mdk and lib/cmsis-svd. 57 files use this path, and in each one the two merged clusters differ by 13 to 17 field definitions. On h7, DOEP0 also has no DMA register, but EP1..N have one.

Is the intention to take the register definitions from the array cluster instead, or to skip the merge when the two field sets are different?

  1. The adjacency test can underflow.

uint32(nextClusterOffset-clusterOffset) subtracts two uint64 values. If the next cluster has a lower offset, the result wraps instead of failing the test. A nextClusterOffset > clusterOffset guard prevents this.

  1. tweakPeriphGroup applies to all packages.

readSVD calls it for every device, but tweakDevice below it returns early unless pkgName is stm32. The regeneration shows no change to other vendors today, so this is latent. Is the group name tweak intended for stm32 only?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants