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

drivers/include/periph: add FREQM peripheral driver #20020

Merged
merged 14 commits into from
Nov 27, 2023

Conversation

gompper
Copy link
Contributor

@gompper gompper commented Oct 26, 2023

Contribution description

This PR adds a peripheral driver for frequency meter (FREQM). Tested with SAM E54 XPLAINED PRO EVALUATION KIT (ATSAME54P20A microcontroller).

I think I have access to some other boards to test the driver in the near future.

Testing procedure

As the FREQM needs a reference clock to measure the frequency of another clock this test uses two internal clocks. The slower internal clock is used as reference to measure the faster internal clock.

The test can be invoked with:

make -C tests/periph/freqm flash

Expected result (via serial):

main(): This is RIOT! (Version: <INSERT VERSION HERE>)
FREQM peripheral driver test
Measured Clock Frequency: <MEASURED CLOCK FREQUENCY> Hz

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Oct 26, 2023
boards/same54-xpro/board.c Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
boards/same54-xpro/board.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
tests/periph/freqm/README.md Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Give it a quick try and it works nice !

cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Show resolved Hide resolved
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

This is looking good!
Some nits and a suggestion to simplify the API:

Comment on lines 73 to 75
#ifndef SAM0_GCLK_MAIN
#define SAM0_GCLK_MAIN 0 /**< 120 MHz main clock */
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifndef SAM0_GCLK_MAIN
#define SAM0_GCLK_MAIN 0 /**< 120 MHz main clock */
#endif
#define SAM0_GCLK_MAIN 0 /**< 120 MHz main clock */

Main Clock is fixed to GCLK0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible that SAM0_GCLK_32KHZ is also fixed? When trying to overwrite it I didn't get any text on the terminal.

Copy link
Contributor

Choose a reason for hiding this comment

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

It shoudln't be, unless it's hard-coded somewhere.
But maybe we should add some static_assert() to ensure no two GCLKs are configured to the same ID.

boards/same54-xpro/Makefile.include Show resolved Hide resolved
cpu/sam0_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Outdated Show resolved Hide resolved
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks pretty good now!

cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 20, 2023
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good!
Only some minor nitpicks.

boards/same54-xpro/include/periph_conf.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/freqm.c Outdated Show resolved Hide resolved
drivers/include/periph/freqm.h Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
tests/periph/freqm/main.c Outdated Show resolved Hide resolved
@riot-ci
Copy link

riot-ci commented Nov 20, 2023

Murdock results

✔️ PASSED

b36e8e9 kconfigs/Kconfig.features: define periph_freqm

Success Failures Total Runtime
7957 0 7957 10m:05s

Artifacts

@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Nov 23, 2023
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for your contribution!

@maribu maribu added this pull request to the merge queue Nov 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 24, 2023
@dylad
Copy link
Member

dylad commented Nov 26, 2023

Looks like something is missing with Kconfig.

@benpicco benpicco enabled auto-merge November 27, 2023 15:57
@benpicco benpicco added this pull request to the merge queue Nov 27, 2023
Merged via the queue into RIOT-OS:master with commit c93a5b8 Nov 27, 2023
25 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools 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
Development

Successfully merging this pull request may close these issues.

5 participants