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

Add OpenTitan watchdog support #57501

Merged
merged 7 commits into from
May 26, 2023

Conversation

tkng-rivos
Copy link
Contributor

The OpenTitan contains a HWIP called the Always-On (AON) Timer, which has two features - a wakeup timer and a watchdog timer. This patch series is concerned solely with the watchdog feature and does the following:

  • Adds the OpenTitan watchdog dts binding
  • Adds a driver for the watchdog
  • Adds the watchdog for the OpenTitan Earlgrey "board".

More details in the commits.

The AON Timer spec can be found here:
https://opentitan.org/book/hw/ip/aon_timer/index.html

To make the watchdog interrupt work with OpenTitan Earlgrey, some changes were needed to be made to the PLIC configuration.

@zephyrbot zephyrbot added area: Watchdog Watchdog area: Devicetree Binding PR modifies or adds a Device Tree binding area: RISCV RISCV Architecture (32-bit & 64-bit) labels May 2, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello @tkng-rivos, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
@tkng-rivos tkng-rivos force-pushed the dev/tkng/opentitan_wdt branch 2 times, most recently from 9b6401d to 063c810 Compare May 3, 2023 19:42
gmarull
gmarull previously requested changes May 5, 2023
drivers/watchdog/Kconfig.opentitan Outdated Show resolved Hide resolved
drivers/watchdog/Kconfig.opentitan Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
@tkng-rivos tkng-rivos force-pushed the dev/tkng/opentitan_wdt branch 3 times, most recently from 8632968 to f81cf73 Compare May 9, 2023 05:01
@tkng-rivos
Copy link
Contributor Author

I've made a few adjustments to the driver:

  1. I added offset labels to the reg block struct. I would still prefer to have the reg block struct instead of going with an alternative.
  2. I changed the logic surrounding how a timeout is chosen when the requested timeout values approach max_uint32. Before, I had simply returned an errored state when the requested max timeout was too large to fit in the hardware register. I added a check to see if it can fit in the requested [min, max] while respecting the bite timeout >= bark timeout requirement.
  3. I originally had a lock attribute that would be set in the devicetree to indicate that the watchdog configuration would be "locked" by hardware, but removed it before the PR as I thought it wouldn't be so useful. Revisiting it, I added it back because I couldn't see a good way to give the user an option to adjust this setting.

@tkng-rivos tkng-rivos force-pushed the dev/tkng/opentitan_wdt branch 2 times, most recently from 5001a47 to 3652f38 Compare May 10, 2023 01:59
@tkng-rivos
Copy link
Contributor Author

I made a mistake when opening the PR in that I did not test it with the OpenTitan Verilator simulation; instead I used a QEMU setup I have locally. Lo and behold, it did not work with the Verilator simulation. I have investigated the cause and it seems like some additional configuration was needed to activate the watchdog reset functionality in the power management block.

Thus, I have included additional patches for adding a barebones devicetree binding for the power manager register block in the OpenTitan Earlgrey. All it has is a register configuration so that we can define the addresses of the necessary registers are; nothing else is implemented. These registers are used at SoC init to enable the watchdog reset. Note that the watchdog is not active until the watchdog timer itself is activated. Without these patches, the watchdog "bite" will never trigger a reset.

I have since tested the Verilator simulation and confirmed that both the bark and bite work with a modified version of the watchdog sample, which basically checks for the WDT_MULTISTAGE configuration option and adjusts the watchdog timeout installation with a 2-stage timeout. If interested, I can add that change to the patch series.

fkokosinski
fkokosinski previously approved these changes May 22, 2023
carlocaione
carlocaione previously approved these changes May 22, 2023
drivers/watchdog/wdt_opentitan.c Outdated Show resolved Hide resolved
@tkng-rivos
Copy link
Contributor Author

Fixed some dead links in the OpenTitan Earlgrey page as well.

carlocaione
carlocaione previously approved these changes May 23, 2023
fkokosinski
fkokosinski previously approved these changes May 24, 2023
@fkokosinski
Copy link
Member

@tkng-rivos could you rebase?

@gmarull gmarull dismissed their stale review May 25, 2023 08:18

concerns addressed

@carlescufi
Copy link
Member

@tkng-rivos please rebase and resolve conflicts

The OpenTitan AON Timer is a hardware device that has two features:
the wakeup timer and watchdog timer. This commit series implements the
watchdog feature.

The spec can be found here:
https://opentitan.org/book/hw/ip/aon_timer/index.html

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
The OpenTitan watchdog driver is a watchdog driver that can be configured
with two stages: a watchdog "bark", which generates an interrupt, and a
watchdog "bite", which resets the system. The two-stage watchdog can be
enabled by setting CONFIG_WDT_MULTISTAGE=y. Otherwise, the driver
functions as a single-stage watchdog.

A callback function may be set for the bark interrupt through the
wdt setup interface, but will only be used if the two-stage watchdog is
enabled. It must be configured for the first watchdog stage.

The driver controls only the watchdog portion of the OpenTitan AON timer.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
Adds the AON Timer device in the OpenTitan Earlgrey device tree.
Adds overlay files to enable the watchdog and set the alias to
`watchdog0`.

Adds the AON timer (watchdog part) to the supported features section
of the OpenTitan documentation.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
1. Fixes the number of interrupts in OpenTitan by default. This should
   be 32 + 185 = 217 IRQs, as there are 185 configurable registers,
   including interrupt 0.

2. Adds 2ND_LVL_INTR_00_OFFSET Kconfig, which is needed to generate a
   PLIC interrupt on IRQ 11.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
The OpenTitan power manager is responsible for changing the OpenTitan's
operation to and from low power state. This patch adds a simple binding
for the power manager's config registers.

This is part of the OpenTitan watchdog patch series. The power manager
HWIP block needs to be configured to enable the watchdog reset
functionality in the OpenTitan Verilator simulation.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
Adds the pwrmgr devicetree node. This is a simple binding that holds
only the address of the registers for now.

This patch is part of the OpenTitan watchdog (AON Timer) support patch
series. It is needed to ensure that the watchdog reset functionality
is enabled.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
Adds watchdog reset as a reset source at SoC init. This is achieved by:

1. Writing 0x2 to the RESET_EN bitfield register to indicate watchdog
reset is enabled.
2. Writing 0x1 to the CFG_CDC_SYNC register to commit the change.
3. Polling the CFG_CDC_SYNC register until reading 0 to confirm the
change has been processed.

This patch is part of the OpenTitan watchdog (AON Timer) support patch
series. It is needed to ensure that the watchdog reset functionality
is enabled. Note that the timer itself is not enabled here, only the
reset function.

Signed-off-by: Tyler Ng <tkng@rivosinc.com>
@nashif nashif merged commit 28a18ec into zephyrproject-rtos:main May 26, 2023
25 checks passed
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi @tkng-rivos!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: RISCV RISCV Architecture (32-bit & 64-bit) area: Watchdog Watchdog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants