Skip to content

cpu/sam0_common: avoid bitfield usage#20747

Merged
dylad merged 19 commits intoRIOT-OS:masterfrom
dylad:pr/cpu/sam0/avoid_bitfield_usage
Jun 26, 2024
Merged

cpu/sam0_common: avoid bitfield usage#20747
dylad merged 19 commits intoRIOT-OS:masterfrom
dylad:pr/cpu/sam0/avoid_bitfield_usage

Conversation

@dylad
Copy link
Copy Markdown
Member

@dylad dylad commented Jun 11, 2024 •
edited
Loading

Contribution description

This PR removes all bitfields usage from cpu/sam0_common/ folder.
I decided to go with a commit per file to allow an easy revert in case something got terribly wrong. Nevertheless, CI should catch most of the issues early. However, I might get sleepy here and there so please, take a careful eyes at this.

/!\ Breakages are highly probable here /!\

Testing procedure

Triple careful review and CI should save us hopefully.

Issues/PRs references

#20457

@dylad dylad requested review from benpicco and keestux as code owners June 11, 2024 20:19
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Jun 11, 2024
@dylad dylad added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 11, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Jun 11, 2024 •
edited
Loading

Murdock results

✔️ PASSED

ccc155e cpu/sam0: remove bitfield usage in sdhc driver

Success Failures Total Runtime
10178 0 10178 18m:06s

Artifacts

@dylad dylad removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Jun 12, 2024
dylad
dylad commented Jun 12, 2024
View reviewed changes
kfessel
kfessel reviewed Jun 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

there are a lot of 1 rmw to 2 rmw cases.

some might need a temporary, can probably be some thing like

REG = (REG & ~Msk) | ((val << Pos) &Msk);

there might be a macro for this

kfessel
kfessel previously requested changes Jun 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

Please do not merge until the doubling of the rmws is fixed

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 12, 2024

@kfessel I've pushed my fixup commits.

kfessel
kfessel reviewed Jun 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

👍

mostly good

cpu/sam0_common/periph/timer.c
Comment on lines +353 to +356
do {
cmd = ((dev(tim)->CTRLBSET.reg & TC_CTRLBSET_CMD_Msk) >> TC_CTRLBSET_CMD_Pos);
} while(cmd == TC_CTRLBSET_CMD_READSYNC_Val);

Copy link
Copy Markdown
Contributor

@kfessel kfessel Jun 13, 2024

Choose a reason for hiding this comment

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

might be faster but please ignore unless someone objdumped

Suggested change
do {
cmd = ((dev(tim)->CTRLBSET.reg & TC_CTRLBSET_CMD_Msk) >> TC_CTRLBSET_CMD_Pos);
} while(cmd == TC_CTRLBSET_CMD_READSYNC_Val);
do {
cmd = ((dev(tim)->CTRLBSET.reg & TC_CTRLBSET_CMD_Msk));
} while(cmd == TC_CTRLBSET_CMD_READSYNC_Val << TC_CTRLBSET_CMD_Pos);

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 14, 2024

@kfessel A new round of fixups has been pushed.

kfessel
kfessel reviewed Jun 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

it getting fewer

btw i'd like to ask: if you are doing tests?

cpu/sam0_common/periph/adc.c
Comment on lines +193 to +198
uint32_t ctrlc = dev->CTRLC.reg;
dev->CTRLC.reg = ((ctrlc & ~ADC_CTRLC_RESSEL_Msk) | ADC_CTRLC_RESSEL(res));
#else
/* Reset resolution bits in CTRLB */
dev->CTRLB.bit.RESSEL = res & 0x3;
uint32_t ctrlb = dev->CTRLB.reg;
dev->CTRLB.reg = ((ctrlb & ~ADC_CTRLB_RESSEL_Msk) | ADC_CTRLB_RESSEL(res));
Copy link
Copy Markdown
Contributor

@kfessel kfessel Jun 14, 2024

Choose a reason for hiding this comment

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

the function is called configure but does not configure all the bits (and nothing else in this file does) this seem to me like bad code translated well.

you might ignore this comment your translation mimics the old behavior

benpicco
benpicco reviewed Jun 14, 2024
View reviewed changes
@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 14, 2024

@benpicco @kfessel Third round is done.

kfessel
kfessel reviewed Jun 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

looks fine testing?

@kfessel kfessel dismissed their stale review June 14, 2024 10:47

obvious issues are resolved -- looks fine

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 14, 2024

looks fine testing?

I don't have hardware with me right now. I'll re-run tests on hardware this weekend if I manage to get some time.

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 17, 2024

I've run basics tests (timers, RTC, I2C, SPI, UART, USB, PWM) on saml11-xpro, saml21-xpro, samr21-xpro. I didn't spot any regression so far.

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 20, 2024

@benpicco @kfessel May I squash this one ?

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Jun 20, 2024 •
edited
Loading

thanks for testing; please squash

dylad added 7 commits June 21, 2024 09:29
@dylad
cpu/sam0/periph: remove bitfield usage in ADC driver
3c64901 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in DAC driver
292111a 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in DMAC driver
9d29813 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in GMAC driver
1140241 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in flashpage driver
ae7ed46 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in gpio driver
1e7167d 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in gpio_ll_irq driver
b2bbc4c 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad dylad force-pushed the pr/cpu/sam0/avoid_bitfield_usage branch from fa40dc1 to f32f08a Compare June 21, 2024 07:46
@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 21, 2024

Squashed !

@dylad dylad force-pushed the pr/cpu/sam0/avoid_bitfield_usage branch from f32f08a to f1116f2 Compare June 24, 2024 11:57
@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 24, 2024

Fixed a typo in the UART driver spotted by Murdock.
Let's see if it's happy now.

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 24, 2024

All green !

kfessel
kfessel approved these changes Jun 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

translation looks correct,

For the test i trust @dylad.

@dylad dylad added this pull request to the merge queue Jun 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 25, 2024
dylad added 11 commits June 25, 2024 16:13
@dylad
cpu/sam0/periph: remove bitfield usage in PWM driver
38c65b9 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in trng driver
37b03d8 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in I2C driver
a66eebb 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in RTC/RTT driver
7c1e66b 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in SPI driver
925e98b 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in timer driver
6ccaca1 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in UART driver
f31ac8a 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in usbdev driver
707fecb 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0/periph: remove bitfield usage in watchdog driver
25a04e6 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0: remove bitfield usage in headers
04e4770 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad
cpu/sam0: remove bitfield usage in sdhc driver
ccc155e 
Signed-off-by: Dylan Laduranty <[email protected]>
@dylad dylad force-pushed the pr/cpu/sam0/avoid_bitfield_usage branch from f1116f2 to ccc155e Compare June 25, 2024 14:13
@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 25, 2024

Murdock spotted an inconsistency in PWM driver for SAMD20. That should be good now.

@dylad dylad enabled auto-merge June 25, 2024 14:17
@dylad dylad added this pull request to the merge queue Jun 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 25, 2024
@dylad dylad added this pull request to the merge queue Jun 25, 2024
Merged via the queue into RIOT-OS:master with commit 8f645a9 Jun 26, 2024
@dylad dylad deleted the pr/cpu/sam0/avoid_bitfield_usage branch June 26, 2024 05:24
@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 26, 2024

Thanks !

@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco left review comments

@kfessel kfessel kfessel approved these changes

@keestux keestux Awaiting requested review from keestux keestux is a code owner

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU 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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dylad @riot-ci @kfessel @benpicco @mguetschow