cpu/samd5x: avoid the use of bitfields by dylad · Pull Request #20714 · RIOT-OS/RIOT

dylad · 2024-05-30T14:42:20Z

Contribution description

This PR replaces all calls of foo->bar.bit.xyz = ABCD to foo->bar.reg &= ABCD for all SAMD5x MCUs
This should not have much impact on the code but it would be good to test the CAN driver implementation as I might have break something.

Testing procedure

A careful review especially on the CAN driver is needed?
CI should catch most of the failure but feel free to run some tests on hardware.

Issues/PRs references

see #20457

kfessel · 2024-05-31T09:08:46Z

Why? -
ok i saw the issue #20457 (microchips droping the bit field support)

I think bitfields are more legible, so there must be a reason.

dylad · 2024-05-31T09:14:25Z

Why? -
ok i saw the issue #20457 (microchips droping the bit field support)
I think bitfields are more legible, so there must be a reason.

I'm not happy either but that's a needed step to tackle on our side if we want to add support to newer MCUs from Microchip at some point.

cpu/samd5x/cpu.c

kfessel · 2024-05-31T09:34:08Z

cpu/samd5x/periph/can.c

        DEBUG_PUTS("Extended ID");
-        dev->msg_ram_conf.tx_fifo_queue[can_mm.put].TXBE_0.bit.ID = frame->can_id & CAN_EFF_MASK;
-        dev->msg_ram_conf.tx_fifo_queue[can_mm.put].TXBE_0.bit.XTD = 1;
+        dev->msg_ram_conf.tx_fifo_queue[can_mm.put].TXBE_0.reg |= CAN_TXBE_0_ID(frame->can_id & CAN_EFF_MASK)


if bitfield operations are removed make them one write

assembble the whole id -> then write it

kfessel · 2024-05-31T09:35:47Z

cpu/samd5x/periph/can.c

+        uint8_t tx_err_cnt = (uint8_t)(dev->conf->can->ECR.reg & CAN_ECR_TEC_Msk);
        DEBUG("tx error counter = %u\n", tx_err_cnt);
-        uint8_t rx_err_cnt = (uint8_t)dev->conf->can->ECR.bit.REC;
+        uint8_t rx_err_cnt = (uint8_t)((dev->conf->can->ECR.reg & CAN_ECR_REC_Msk)


kfessel · 2024-05-31T09:36:07Z

cpu/samd5x/periph/can.c

+        uint8_t tx_err_cnt = (uint8_t)(dev->conf->can->ECR.reg & CAN_ECR_TEC_Msk);
        DEBUG("tx error counter = %u\n", tx_err_cnt);
-        uint8_t rx_err_cnt = (uint8_t)dev->conf->can->ECR.bit.REC;
+        uint8_t rx_err_cnt = (uint8_t)((dev->conf->can->ECR.reg & CAN_ECR_REC_Msk)
+                             >> CAN_ECR_REC_Pos);


kfessel · 2024-05-31T09:46:29Z

but the registers also changed name and from being a union of bitfield and reg to just a uint

dylad · 2024-05-31T09:49:22Z

but the registers also changed name and from being a union of bitfield and reg to just a uint

Yes. But I'd like to proceed in small steps for now. I am expecting breakages and it will be easier to deal with those if we do it step by step.
See #20457 for the step details.

dylad · 2024-06-04T09:33:46Z

@kfessel I've added a variable here and there to avoid multiple read of the same register. I hope I didn't miss some.

riot-ci · 2024-06-04T09:57:59Z

Murdock results

✔️ PASSED

26b3583 cpu/samd5x: avoid the use of bitfield in periph

Success	Failures	Total	Runtime
10178	0	10178	16m:57s

Artifacts

Documentation preview

cpu/samd5x/cpu.c

kfessel

we need more of this (.reg & REG_Msk) >> REG_Pos

seems like the other way around is hidden in these MACROS (i did not check)

I for sure have missed some places where this is needed.

@ microchip: this makes the code so much easier to read and write

cpu/samd5x/periph/can.c

dylad · 2024-06-06T20:14:06Z

@kfessel I've addressed the new round of review.

cpu/samd5x/periph/can.c

kfessel · 2024-06-14T15:51:42Z

cpu/samd5x/periph/can.c

+        uint32_t reg = dev->conf->can->ECR.reg;
+        uint8_t tx_err_cnt = (uint8_t) (reg & CAN_ECR_TEC_Msk);
        DEBUG("tx error counter = %u\n", tx_err_cnt);
-        uint8_t rx_err_cnt = (uint8_t)dev->conf->can->ECR.bit.REC;
+        uint8_t rx_err_cnt = (uint8_t)((reg & CAN_ECR_REC_Msk) >> CAN_ECR_REC_Pos);
        DEBUG("rx error counter = %u\n", rx_err_cnt);
        /* Check the CAN error type */
-        uint8_t error_code = (uint8_t)dev->conf->can->PSR.bit.LEC;
+        uint8_t error_code = (uint8_t)(dev->conf->can->PSR.reg & CAN_PSR_LEC_Msk);


it is allready done here so copy

dylad · 2024-06-20T08:07:20Z

Latest round of review was addressed.

dylad · 2024-06-26T07:13:00Z

May I squash this one ?

kfessel · 2024-06-27T16:53:08Z

look good please squash
look good

Signed-off-by: Dylan Laduranty <[email protected]>

dylad · 2024-06-27T19:52:03Z

Rebased and squashed.

maribu

Thx a bunch!

This will actually result in more efficient machine code, as a few instances of subsequent accesses of bit-fields of the same word in memory (marked as volatile) were replaced by loading that word into a temporary (non volatile) variable, from which the fields were extracted via bit-masks. The volatile qualifier prevents the optimizer to combine memory accesses. But in the new code, the temporary variable could be allocated as a register resulting in fewer memory accesses. Quite cool!

I have an style suggestion inline. Please squash directly if you agree, or just hit merge if you don't.

maribu · 2024-06-28T08:48:42Z

cpu/samd5x/cpu.c

+    const uint32_t flags = (OSCCTRL_DPLLSTATUS_CLKRDY | OSCCTRL_DPLLSTATUS_LOCK);
+    while (!((OSCCTRL->Dpll[idx].DPLLSTATUS.reg & flags) == flags)) {}


This will even be more efficient. I believe normally, the optimizer would combine the two bit-field accesses on the same word to a single memory access. But since those registers are marked as volatile, the compiler is not allowed to combine memory accesses.

cpu/samd5x/periph/can.c

maribu · 2024-06-28T08:54:10Z

cpu/samd5x/periph/can.c

        uint16_t rx_get_idx = 0;
        uint16_t rx_put_idx = 0;
-        rx_get_idx = (dev->conf->can->RXF0S.reg >> CAN_RXF0S_F0GI_Pos) & 0x3F;
+        uint32_t reg = dev->conf->can->RXF0S.reg;


Again, this will produce more efficient machine code :)

maribu · 2024-06-28T08:55:19Z

cpu/samd5x/periph/can.c

        dev->conf->can->IR.reg |= CAN_IR_PEA;
        /* Extract the Tx and Rx error counters */
-        uint8_t tx_err_cnt = (uint8_t)dev->conf->can->ECR.bit.TEC;
+        uint32_t reg = dev->conf->can->ECR.reg;


And another instance of better machine code :)

dylad · 2024-06-28T16:07:53Z

Fixed extra character added by mistake after a semicolon.

Signed-off-by: Dylan Laduranty <[email protected]>

maribu · 2024-06-29T16:39:01Z

🎉

dylad · 2024-06-29T18:33:01Z

Thanks for the reviews !

With this one merged, the first step of the migration is now completed.

dylad requested a review from kfessel May 30, 2024 14:42

dylad requested review from benpicco and kaspar030 as code owners May 30, 2024 14:42

github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels May 30, 2024

dylad force-pushed the pr/cpu/samd5x/avoid_bitfields_reg branch from a6c028a to 6324c2a Compare May 30, 2024 14:55

kfessel reviewed May 31, 2024

View reviewed changes

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved

kfessel reviewed May 31, 2024

View reviewed changes

dylad force-pushed the pr/cpu/samd5x/avoid_bitfields_reg branch from 2de511f to 6324c2a Compare June 4, 2024 09:29

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 4, 2024

kfessel reviewed Jun 4, 2024

View reviewed changes

cpu/samd5x/cpu.c Outdated Show resolved Hide resolved

kfessel reviewed Jun 4, 2024

View reviewed changes

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved

cpu/samd5x/periph/can.c Show resolved Hide resolved

kfessel reviewed Jun 14, 2024

View reviewed changes

cpu/samd5x/periph/can.c Outdated Show resolved Hide resolved

kfessel reviewed Jun 14, 2024

View reviewed changes

cpu/samd5x: avoid the use of bitfield in cpu init

41c003d

Signed-off-by: Dylan Laduranty <[email protected]>

dylad force-pushed the pr/cpu/samd5x/avoid_bitfields_reg branch from 43bbcac to bfadc44 Compare June 27, 2024 19:51

maribu approved these changes Jun 28, 2024

View reviewed changes

dylad force-pushed the pr/cpu/samd5x/avoid_bitfields_reg branch from bfadc44 to f36e6eb Compare June 28, 2024 09:09

maribu added this pull request to the merge queue Jun 28, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 28, 2024

dylad force-pushed the pr/cpu/samd5x/avoid_bitfields_reg branch from f36e6eb to 8e2369f Compare June 28, 2024 16:06

dylad enabled auto-merge June 28, 2024 16:07

dylad added this pull request to the merge queue Jun 28, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 28, 2024

cpu/samd5x: avoid the use of bitfield in periph

26b3583

Signed-off-by: Dylan Laduranty <[email protected]>

dylad force-pushed the pr/cpu/samd5x/avoid_bitfields_reg branch from 8e2369f to 26b3583 Compare June 29, 2024 11:26

dylad enabled auto-merge June 29, 2024 11:28

dylad added this pull request to the merge queue Jun 29, 2024

Merged via the queue into RIOT-OS:master with commit 6b7dd90 Jun 29, 2024

dylad deleted the pr/cpu/samd5x/avoid_bitfields_reg branch June 29, 2024 18:33

mguetschow added this to the Release 2024.07 milestone Jul 3, 2024

		const uint32_t flags = (OSCCTRL_DPLLSTATUS_CLKRDY \| OSCCTRL_DPLLSTATUS_LOCK);
		while (!((OSCCTRL->Dpll[idx].DPLLSTATUS.reg & flags) == flags)) {}

Conversation

dylad commented May 30, 2024

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kfessel commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylad commented May 31, 2024

Uh oh!

Uh oh!

kfessel May 31, 2024

Choose a reason for hiding this comment

Uh oh!

kfessel May 31, 2024

Choose a reason for hiding this comment

Uh oh!

kfessel May 31, 2024

Choose a reason for hiding this comment

Uh oh!

kfessel commented May 31, 2024

Uh oh!

dylad commented May 31, 2024

Uh oh!

dylad commented Jun 4, 2024

Uh oh!

riot-ci commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

Uh oh!

kfessel left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylad commented Jun 6, 2024

Uh oh!

Uh oh!

kfessel Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

dylad commented Jun 20, 2024

Uh oh!

dylad commented Jun 26, 2024

Uh oh!

kfessel commented Jun 27, 2024

Uh oh!

dylad commented Jun 27, 2024

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

maribu Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

maribu Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dylad commented Jun 28, 2024

Uh oh!

Uh oh!

maribu commented Jun 29, 2024

Uh oh!

dylad commented Jun 29, 2024

Uh oh!

Reviewers

Assignees

Labels

kfessel commented May 31, 2024 •

edited

Loading

riot-ci commented Jun 4, 2024 •

edited

Loading

kfessel left a comment •

edited

Loading