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

watchdog: Add Broadcom BCM2711 watchdog timer driver #5664

Draft
wants to merge 3 commits into
base: rpi-6.1.y
Choose a base branch
from

Conversation

popcornmix
Copy link
Collaborator

This adds a driver for watchdog timer hardware present on Broadcom BCM2711
and BCM2712 SoC, used in Raspberry Pi 4 and 5 devices.

While the existing BCM2835 driver does work on these parts,
the new peripheral increases the maximum timeout from ~15s
to ~159s which can be useful.

This adds a driver for watchdog timer hardware present on Broadcom BCM2711
and BCM2712 SoC, used in Raspberry Pi 4 and 5 devices.

While the existing BCM2835 driver does work on these parts,
the new peripheral increases the maximum timeout from ~15s
to ~159s which can be useful.

Signed-off-by: Dom Cobley <[email protected]>
@popcornmix
Copy link
Collaborator Author

This is for comments. It's a little ugly how this goes through the existing pm driver.

Rather than a separate watchdog driver, I wonder if it may be better to merge them,
and just have the register write (which is small part of driver) conditional based on DT.

@pelwell
Copy link
Contributor

pelwell commented Oct 19, 2023

It's a little ugly how this goes through the existing pm driver.

What do you think is the purpose of the interlinking? Is there anything stopping us instantiating the WDT directly from DT?

@popcornmix
Copy link
Collaborator Author

What do you think is the purpose of the interlinking? Is there anything stopping us instantiating the WDT directly from DT?

It looks like the main reason is power (bcm2835-power) and watchdog (bcm2835-wdt) share a peripheral block.
A parent (brcm,bcm2835-pm-wdt) mfd driver actually registers the peripheral block (Power Manager) and passes the registers to the two sub drivers (power and wdt).

But I don't see any locking wrapper, that would mean this design was necessary. But perhaps it's considered more correct.

But main reason is upstream chose this implementation, and following their pattern seems wise.

@lategoodbye
Copy link
Contributor

In case maximum timeout is the only reason, there is a fix for the bcm2835_wdt which let the kernel take care of keeping alive for longer timeouts:
https://lore.kernel.org/linux-watchdog/[email protected]/T/

@popcornmix
Copy link
Collaborator Author

@lategoodbye thank - that is interesting. The limited watchdog duration is the main reason for considering this driver switch.

I guess you patch may get an earlier (15s) than expected watchdog reboot if the core watchdog code dies, but that's probably a pretty fatal situation anyway, so a reset isn't unreasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants