Skip to content

drivers: counter: sam: Introduce sama7g5 pit64b #93806

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

drivers: counter: sam: Introduce sama7g5 pit64b #93806

wants to merge 6 commits into from
+595 −14

Conversation

@MCHP-MPU-Solutions-SHA
Copy link
Contributor

Add counter driver for sama7g5 pit64b IP, use it as a standard zephyr counter device.
pit64 has only one compare register, by default pit64 counter device only have top_value channel, and have no alarm channel.

Didn't add support in samples/drivers/counter/alarm because pit64b counter device have no alarm channel by default.
Didn't add support in tests/drivers/counter/counter_basic_api because pit64b has no get_value() api, and get_value_64() should be used.

@zephyrbot zephyrbot added platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) area: Counter platform: Microchip MEC Microchip MEC Platform labels Jul 29, 2025
nandojve
nandojve requested changes Aug 3, 2025
View reviewed changes
Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

Hi Xing,

You need to follow always the dependency order. In this case, you need to reorder the commits:

dts: arm: microchip: sam: add pit64b device to sama7g5
drivers: counter: add support for sama7g54 PIT64B

Then you need to proper extract and name those commits:

west: Add mchp sam pit64b dependency (when applicable)
dts: bindings: counter: Add mchp sam pit64b
soc: ? You need to add the early init because it is a pre-req to the driver.
drivers: counter: Introduce pit64b driver
boards: ? You need to consume the driver on a board
tests: ? You should enable the tests using that board (when applicable)

In the first commit, you create the binding and already can add the relevant nodes inside MPU dtsi.
In the drivers commit, you will put all the driver/kconfig/cmake work at once.
Finally you can consume the driver on a board.

This sequence allow each maintainer look in the topics that are relevant and do not break git bisect. If we see that something is missing in the hal, the west changes should be the first commit in the series using the reference pull/<pr number>/head.

BTW, we do not accept a driver that it is not build on a test or on a board.

@albertofloyd albertofloyd removed their request for review August 4, 2025 20:46
@zephyrbot zephyrbot added the area: Samples Samples label Aug 5, 2025
@zephyrbot zephyrbot requested review from kartben and nashif August 5, 2025 10:10
@MCHP-MPU-Solutions-SHA
Copy link
Contributor Author

MCHP-MPU-Solutions-SHA commented Aug 5, 2025 •
edited
Loading

Hi Xing,

You need to follow always the dependency order. In this case, you need to reorder the commits:

dts: arm: microchip: sam: add pit64b device to sama7g5 drivers: counter: add support for sama7g54 PIT64B

Then you need to proper extract and name those commits:

west: Add mchp sam pit64b dependency (when applicable) dts: bindings: counter: Add mchp sam pit64b soc: ? You need to add the early init because it is a pre-req to the driver. drivers: counter: Introduce pit64b driver boards: ? You need to consume the driver on a board tests: ? You should enable the tests using that board (when applicable)

In the first commit, you create the binding and already can add the relevant nodes inside MPU dtsi. In the drivers commit, you will put all the driver/kconfig/cmake work at once. Finally you can consume the driver on a board.

This sequence allow each maintainer look in the topics that are relevant and do not break git bisect. If we see that something is missing in the hal, the west changes should be the first commit in the series using the reference pull/<pr number>/head.

BTW, we do not accept a driver that it is not build on a test or on a board.

Hi Gerson,

Thank you so much for this detailed guide!

I have reorder these commits, please help to check.
Here is the updated commit list:

  1. dts: arm: microchip: sam: add pit64b device to sama7g5
    dts/arm/microchip/sam/sama7g5.dtsi

  2. drivers: counter: add support for sama7g54 PIT64B
    drivers/counter/Kconfig
    drivers/counter/Kconfig.mchp_sam_pit64b
    drivers/counter/counter_mchp_sam_pit64b.c

  3. dts: bindings: counter: add mchp sam pit64b
    dts/bindings/counter/microchip,sam-pit64b-counter.yaml

  4. soc: microchip: sam: update mmu for sama7g5 pit64b
    soc/microchip/sam/sama7g5/soc.c

  5. drivers: counter: introduce mchp sam pit64b driver
    drivers/counter/CMakeLists.txt

  6. boards: microchip: sam: add and enable pit64b1
    boards/microchip/sam/sama7g54_ek/sama7g54_ek.dts

For this issue:
BTW, we do not accept a driver that it is not build on a test or on a board.
Please check commit ff8d0ef and daff86a.
Will you accept this solution? because PIT64B can only accept "COUNTER_ALARM_CFG_ABSOLUTE" alarm flag.
With these two commits pit64b counter device works well with zephyr/samples/drivers/counter/alarm.

Many thanks.
Xing.

nandojve
nandojve requested changes Aug 5, 2025
View reviewed changes
@nandojve nandojve changed the title Sama7g5 pit64 drivers: counter: sam: Introduce sama7g5 pit64b Aug 5, 2025
@nandojve nandojve added this to the v4.3.0 milestone Aug 16, 2025
nandojve
nandojve requested changes Aug 28, 2025
View reviewed changes
@zephyrbot zephyrbot requested a review from JarmouniA August 29, 2025 06:45
nandojve
nandojve requested changes Sep 14, 2025
View reviewed changes
@nandojve nandojve mentioned this pull request Sep 16, 2025
Open
@MCHP-MPU-Solutions-SHA
Copy link
Contributor Author

Hi Xing,

I still don't see the driver in the list. Unfortunately, you need to force an entry here:

zephyr/samples/drivers/counter/alarm/sample.yaml

Line 19 in 5525941

platform_allow:

People are trying to reduce the number of board that compile the driver. This is something that we need to reevaluate in the project because, sometimes, we have different paths inside the driver.

So, only the boards in the list will build (no variations).

CC: @NhMch

Hi Gerson,
File zephyr/samples/drivers/counter/alarm/sample.yaml has been updated.
And verified with command:
west twister -p sama7g54_ek -s sample.drivers.counter.alarm

@nandojve
Copy link
Member

needs rebase

@MCHP-MPU-Solutions-SHA
Copy link
Contributor Author

needs rebase

PR has been rebased.

nandojve
nandojve previously approved these changes Sep 18, 2025
View reviewed changes
pdgendt
pdgendt reviewed Sep 22, 2025
View reviewed changes
Copy link
Contributor

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

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

No implicit boolean conversions in if-statements. https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#:~:text=85,N/A

pdgendt
pdgendt approved these changes Sep 24, 2025
View reviewed changes
drivers/counter/counter_mchp_sam_pit64b.c
Comment on lines +126 to +132
k_spinlock_key_t key;

key = k_spin_lock(&data->lock);

*ticks = (uint32_t)pit_counter_value(config->regs);

k_spin_unlock(&data->lock, key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative is to use K_SPINLOCK, which is a bit cleaner IMO (doesn't work with return statements in the block).

	K_SPINLOCK(&data->lock) {
		*ticks = (uint32_t)pit_counter_value(config->regs);
	}

@nandojve
Copy link
Member

Xing,

I think you need to rebase due to CI issues.

@MCHP-MPU-Solutions-SHA
Copy link
Contributor Author

Xing,

I think you need to rebase due to CI issues.

Gerson,

So, maybe this issue "Some checks were not successful" can be fixed by a rebase?
Actually I'm trying to fix it but can not find the root cause.

@pdgendt
Copy link
Contributor

pdgendt commented Sep 24, 2025

So, maybe this issue "Some checks were not successful" can be fixed by a rebase? Actually I'm trying to fix it but can not find the root cause.

Probably, a Python dependency was added on main

@nandojve
Copy link
Member

So, maybe this issue "Some checks were not successful" can be fixed by a rebase? Actually I'm trying to fix it but can not find the root cause.

Probably, a Python dependency was added on main

ahh, so if CI is broken then we need to wait the fix.
But we can make PR ready in the meantime : )

@pdgendt
Copy link
Contributor

pdgendt commented Sep 24, 2025

ahh, so if CI is broken then we need to wait the fix. But we can make PR ready in the meantime : )

A fix has already been merged to main, a rebase should get you going again.

@MCHP-MPU-Solutions-SHA
Copy link
Contributor Author

Many thanks for comments.
I will rebase this PR firstly.

@MCHP-MPU-Solutions-SHA
dts: arm: microchip: sam: add pit64b device to sama7g5
4cc25c5 
Add pit64b1 ~ pit64b5 counter devices to sama7g5

Signed-off-by: CHEN Xing <[email protected]>
@MCHP-MPU-Solutions-SHA
drivers: counter: add support for sama7g54 PIT64B
5c2847d 
Add driver for sama7g54 PIT64B
Use PIT64B as a zephyr counter device

Signed-off-by: CHEN Xing <[email protected]>
@MCHP-MPU-Solutions-SHA
soc: microchip: sam: update mmu for sama7g5 pit64b
1f1d407 
Update mmu region for mchp sam pit64b
Add gclk configuration for mchp sam pit64b

Signed-off-by: CHEN Xing <[email protected]>
@MCHP-MPU-Solutions-SHA
boards: microchip: sam: add and enable pit64b1
640e3f6 
Add and enable pit64b1 counter device.

Signed-off-by: CHEN Xing <[email protected]>
@MCHP-MPU-Solutions-SHA
samples: drivers: counter: alarm: add definition for alarm flag
1ec659c 
Counter devices can use this definition to specify alarm flags.

Signed-off-by: CHEN Xing <[email protected]>
@MCHP-MPU-Solutions-SHA
samples: drivers: counter: alarm: add support for pit64b1
bab918b 
Add support for pit64b1 counter device.

Signed-off-by: CHEN Xing <[email protected]>
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@MCHP-MPU-Solutions-SHA
Copy link
Contributor Author

ahh, so if CI is broken then we need to wait the fix. But we can make PR ready in the meantime : )

A fix has already been merged to main, a rebase should get you going again.

I have rebase this PR, and the CI issue has gone.
Many thanks for support.

@nandojve nandojve requested a review from NhMchp October 23, 2025 16:27
@nandojve
Copy link
Member

needs rebase

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

Reviewers

@pdgendt pdgendt pdgendt approved these changes

@jvasanth1 jvasanth1 Awaiting requested review from jvasanth1

@mnkp mnkp Awaiting requested review from mnkp

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@stephanosio stephanosio Awaiting requested review from stephanosio

@VenkatKotakonda VenkatKotakonda Awaiting requested review from VenkatKotakonda

@kartben kartben Awaiting requested review from kartben

@nashif nashif Awaiting requested review from nashif

@JarmouniA JarmouniA Awaiting requested review from JarmouniA

@nandojve nandojve Awaiting requested review from nandojve

@NhMchp NhMchp Awaiting requested review from NhMchp

At least 2 approving reviews are required to merge this pull request.

Assignees

@nandojve nandojve

Labels

area: Counter area: Samples Samples platform: Microchip MEC Microchip MEC Platform platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)

Projects

None yet

Milestone

v4.3.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MCHP-MPU-Solutions-SHA @nandojve @pdgendt @nordic-krch @zephyrbot