Updates and fixes for stm32g4xx: FDCAN, FLASH, TIM #668

timblakely · 2021-11-27T04:49:32Z

Add missing fields and enumerations to FDCAN via fdcan_g4.yaml
Add registers for FLASH handling. Note that there are some commented out since different devices have different support for secure flash based on their device ID and flash capacity.
Fixes for some timer values. In particular, lower non-32-bit timers back down to 16 bit, since 20 bit is only enabled via dithering.

Let me know if you prefer these to be in a different location; kind of hard for me to grok what goes where in terms of peripheral and register updates/modifications 😅

timblakely · 2021-11-27T06:28:22Z

Gah. I'm seeing some failures in the presubmits, but I can't seem to replicate them on my end...? The pip version of svdtools doesn't seem to fail D:

burrbull · 2021-11-27T12:54:28Z

devices/common_patches/fdcan/fdcan_g4.yaml

+  HPMS:
+    _modify:
+      FIDX:
+        msb: 12


STM32 usually use bitOffset/bitWidth instead of msb/lsb

adamgreig · 2021-11-27T13:05:34Z

Thanks for the PR and your other recent G4 work!

The failures are coming from the linter which checks the generated SVD complies with the CMSIS-SVD schema: if you run make lint locally it should hopefully reproduce the errors locally (you need xmllint installed).

I think the main problem is that where you've used lsb and msb, the old bitOffset and bitWidth tags still exist for those elements, creating an invalid SVD that specifies both sets instead of only one or the other. So far in stm32-rs we've exclusively used bitOffset and bitWidth (although the svdtools patcher does know about lsb and msb), so maybe the best thing to do is keep using bitOffset and bitWidth here for consistency. To fix the failure otherwise you'd have to explicitly delete the existing bitOffset and bitWidth tags too.

Besides that, on lines like:

    accept_rx_fifo0:  [0b00, accept_rx_fifo0]

the last item should be a string description of the field, not just its name repeated: something like "Accept in Rx FIFO 0" or even "Accept non-matching extended frames into Rx FIFO 0". It's what's used to generate the documentation for this enum variant.

Let me know if you prefer these to be in a different location; kind of hard for me to grok what goes where in terms of peripheral and register updates/modifications sweat_smile

Yep, this is pretty tough to track as the project evolved! The gist is that anything "structural" i.e. changing registers or fields should go into common_patches, and anything that adds enumerated values (or permitted write ranges) to a field goes into peripherals/. So, in this case, the various enums you've added should be split out from the patch file and put into a new file inside peripherals/.

timblakely · 2021-11-28T02:46:41Z

Thanks for the PR and your other recent G4 work!

My pleasure! Seeing as the G4 is likely the only chip I have ~~forever~~ for the near future thanks to the global semiconductor apocalypse I figured I might as well contribute to it 🥴

I've updated the PR and run the linter. PTAL! :)

1) Add missing fields and enumerations to FDCAN via fdcan_g4.yaml 2) Add registers for FLASH handling. Note that there are some commented out since different devices have different support for secure flash based on their device ID and flash capacity. 3) Fixes for some timer values. In particular, lower non-32-bit timers back down to 16 bit, since 20 bit is only enabled via dithering.

github-actions · 2021-11-28T02:56:17Z

Memory map comparison

timblakely · 2022-01-10T22:57:44Z

Let me know if there's something I need to do to trigger the review notification again (happened last time I submitted a PR too, so I'm probably just missing a step in the flow somewhere, apologies!)

richardeoin

Don't worry, this PR hasn't been forgotten! I had a look through, and I didn't spot any errors compared to RM0440 🎉 Nice work!

I've noted where declarations be moved to match the rules Adam mentioned earlier, and we need to decide what to do with those flash CR / OPTR bits that only appear based on yet another configuration item

richardeoin · 2022-01-14T21:38:04Z

devices/common_patches/fdcan/fdcan_g4.yaml

+    ANFS:
+      accept_rx_fifo0:  [0b00, "Accept non-matching standard frames in Rx FIFO 0"]
+      accept_rx_fifo1:  [0b01, "Accept non-matching standard frames in Rx FIFO 1"]
+      reject:           [0b10, "Reject non-matching standard frames"]
+      reject_:          [0b11, "Reject non-matching standard frames"]
+    ANFE:
+      accept_rx_fifo0:  [0b00, "Accept non-matching extended frames in Rx FIFO 0"]
+      accept_rx_fifo1:  [0b01, "Accept non-matching extended frames in Rx FIFO 1"]
+      reject:           [0b10, "Reject non-matching extended frames"]
+      reject_:          [0b11, "Reject non-matching extended frames"]
+    RRFS:
+      filter: [0b0, "Filter remote frames with 11-bit standard IDs"]
+      reject: [0b1, "Reject all remote frames with 11-bit standard IDs"]
+    RRFE:
+      filter: [0b0, "Filter remote frames with 29-bit standard IDs"]
+      reject: [0b1, "Reject remote frames with 29-bit standard IDs"]


This is one of those blocks that belongs in peripherals/ because it adds enumerated values (or permitted write ranges) to a field

richardeoin · 2022-01-14T21:41:42Z

devices/common_patches/fdcan/fdcan_g4.yaml

        bitWidth: 2
      TFGI:
+        bitOffset: 8


Looks like these two new bitOffset values don't change anything since these fields were already at this offset already. Don't think it's a problem, just harder to review

richardeoin · 2022-01-14T21:48:45Z

devices/common_patches/tim/v2/g4.yaml

+  CR1:
+    CKD:
+      Div1: [0, "t_DTS = t_CK_INT"]
+      Div2: [1, "t_DTS = 2 × t_CK_INT"]
+      Div4: [2, "t_DTS = 4 × t_CK_INT"]
+    CMS:
+      EdgeAligned: [0, "The counter counts up or down depending on the direction bit"]
+      CenterAligned1: [1, "The counter counts up and down alternatively. Output compare interrupt flags are set only when the counter is counting down."]
+      CenterAligned2: [2, "The counter counts up and down alternatively. Output compare interrupt flags are set only when the counter is counting up."]
+      CenterAligned3: [3, "The counter counts up and down alternatively. Output compare interrupt flags are set both when the counter is counting up or down."]
+    DIR:
+      Up: [0, "Counter used as upcounter"]
+      Down: [1, "Counter used as downcounter"]
+    CEN:
+      Disabled: [0, "Counter disabled"]
+      Enabled: [1, "Counter enabled"]
+
+  CCMR1_Output:
+    OC2PE:
+      Disabled: [0, "Preload register on CCR2 disabled. New values written to CCR2 are taken into account immediately"]
+      Enabled: [1, "Preload register on CCR2 enabled. Preload value is loaded into active register on each update event"]
+    CC2S:
+      Output: [0, "CC2 channel is configured as output"]
+    OC1PE:
+      Disabled: [0, "Preload register on CCR1 disabled. New values written to CCR1 are taken into account immediately"]
+      Enabled: [1, "Preload register on CCR1 enabled. Preload value is loaded into active register on each update event"]
+    CC1S:
+      Output: [0, "CC1 channel is configured as output"]
+
+  CCMR2_Output:
+    OC4PE:
+      Disabled: [0, "Preload register on CCR4 disabled. New values written to CCR4 are taken into account immediately"]
+      Enabled: [1, "Preload register on CCR4 enabled. Preload value is loaded into active register on each update event"]
+    CC4S:
+      Output: [0, "CC4 channel is configured as output"]
+    OC3PE:
+      Disabled: [0, "Preload register on CCR3 disabled. New values written to CCR3 are taken into account immediately"]
+      Enabled: [1, "Preload register on CCR3 enabled. Preload value is loaded into active register on each update event"]
+    CC3S:
+      Output: [0, "CC3 channel is configured as output"]
+
+  DIER:
+    TDE:
+      Disabled: [0, "Trigger DMA request disabled"]
+      Enabled: [1, "Trigger DMA request enabled"]
+    "CC?DE":
+      Disabled: [0, "CCx DMA request disabled"]
+      Enabled: [1, "CCx DMA request enabled"]
+    UDE:
+      Disabled: [0, "Update DMA request disabled"]
+      Enabled: [1, "Update DMA request enabled"]
+    TIE:
+      Disabled: [0, "Trigger interrupt disabled"]
+      Enabled: [1, "Trigger interrupt enabled"]
+    "CC?IE":
+      Disabled: [0, "CCx interrupt disabled"]
+      Enabled: [1, "CCx interrupt enabled"]
+    UIE:
+      Disabled: [0, "Update interrupt disabled"]
+      Enabled: [1, "Update interrupt enabled"]


This is one of those blocks that belongs in peripherals/ because it adds enumerated values (or permitted write ranges) to a field

richardeoin · 2022-01-14T21:50:31Z

devices/common_patches/fdcan/fdcan_g4.yaml

+    TFQM:
+      fifo:   [0b0, "Tx FIFO operation"]
+      queue:  [0b1, "Tx queue operation"]


A little block that belongs in peripherals/ because it adds enumerated values (or permitted write ranges) to a field

richardeoin · 2022-01-14T21:54:40Z

peripherals/flash/flash_g4.yaml

+  CR:
+    _add:
+      SEC_PROT2:
+        description: Securable memory area protection bank 2
+        bitOffset: 29
+        bitWidth: 1
+      MER2:
+        description: Bank 2 mass erase
+        bitOffset: 15
+        bitWidth: 1
+      BKER:
+        description: Bank erase
+        bitOffset: 11
+        bitWidth: 1
+  OPTR:
+    _modify:
+      SRAM2_RST:
+        name: CCMSRAM_RST
+        description: CCM SRAM Erase when system reset
+      SRAM2_PE:
+        name: SRAM_PE
+    _add:
+      DBANK:
+        description: Single or dual bank mode
+        bitOffset: 22
+        bitWidth: 1
+      BFB2:
+        description: Dual-bank boot
+        bitOffset: 20
+        bitWidth: 1


This section is "structural" i.e. changing registers or fields, so belongs in common_patches

These bits are also quite annoying because their presence depends on the category of the device (Section 3, 4 or 5 of RM0440). OPTR bit 20 even gets re-used for two different functions 🤦

I would at least put them in a separate file under common_patches and include "category_3" in the name so we can decide what to do with them

richardeoin · 2022-01-14T22:12:33Z

peripherals/can/fdcan.yaml

+      normal:       [0b0, "Normal operation, register TEST holds reset values"]
+      test:         [0b1, "Test Mode, write access to register TEST enabled"]
+    DAR:
+      retransmit:   [0b0, "Automatric retransmission of messages not transmitted successfully enabled"]


burrbull · 2024-07-07T12:40:47Z

@timblakely do you plan to finish the PR?

newAM added the stm32g4 label Nov 27, 2021

burrbull reviewed Nov 27, 2021

View reviewed changes

timblakely force-pushed the g4_updates branch from 7258952 to d26d42f Compare November 28, 2021 02:44

timblakely force-pushed the g4_updates branch from d26d42f to 4241df6 Compare November 28, 2021 02:48

timblakely force-pushed the g4_updates branch from 4241df6 to c27c618 Compare November 28, 2021 02:54

richardeoin reviewed Jan 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates and fixes for stm32g4xx: FDCAN, FLASH, TIM #668

Updates and fixes for stm32g4xx: FDCAN, FLASH, TIM #668

timblakely commented Nov 27, 2021

timblakely commented Nov 27, 2021

burrbull Nov 27, 2021

adamgreig commented Nov 27, 2021

timblakely commented Nov 28, 2021 •

edited

Loading

github-actions bot commented Nov 28, 2021

timblakely commented Jan 10, 2022

richardeoin left a comment

richardeoin Jan 14, 2022

richardeoin Jan 14, 2022

richardeoin Jan 14, 2022

richardeoin Jan 14, 2022

richardeoin Jan 14, 2022

richardeoin Jan 14, 2022

richardeoin Jan 14, 2022

burrbull commented Jul 7, 2024

Updates and fixes for stm32g4xx: FDCAN, FLASH, TIM #668

Are you sure you want to change the base?

Updates and fixes for stm32g4xx: FDCAN, FLASH, TIM #668

Conversation

timblakely commented Nov 27, 2021

timblakely commented Nov 27, 2021

Choose a reason for hiding this comment

adamgreig commented Nov 27, 2021

timblakely commented Nov 28, 2021 • edited Loading

github-actions bot commented Nov 28, 2021

timblakely commented Jan 10, 2022

richardeoin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burrbull commented Jul 7, 2024

timblakely commented Nov 28, 2021 •

edited

Loading