drivers: watchdog: wdt_xilinx_wwdt: cleanup, firmware-owned reset, DEVICE_MMIO, zero-window fix - #113363
drivers: watchdog: wdt_xilinx_wwdt: cleanup, firmware-owned reset, DEVICE_MMIO, zero-window fix#113363Harini-T wants to merge 4 commits into
Conversation
kedareswararao
left a comment
There was a problem hiding this comment.
Changes looks fine to me
|
Force-pushed: rebased and extended the series since the last review. Changes:
|
| default y | ||
| depends on DT_HAS_XLNX_VERSAL_WWDT_ENABLED | ||
| help | ||
| Enable Window watchdog driver for the versal_wwdt IP core. |
There was a problem hiding this comment.
After this series the umbrella symbol XILINX_WINDOW_WATCHDOG also enables the
generic watchdog engine (via the new mode choice), yet the prompt and help text describe only
the window watchdog. A reader enabling GWDT still has to turn on a symbol literally named "window
watchdog", which is confusing.
There was a problem hiding this comment.
Agreed, Shall i rename the symbol to XILINX_VERSAL_WATCHDOG or reword the help only ?
There was a problem hiding this comment.
If the code is Vearl specific rename else update the help menu
There was a problem hiding this comment.
I'll rename the umbrella symbol to XILINX_VERSAL_WATCHDOG and update the prompt/help to describe the Versal watchdog IP (both WWDT and GWDT engines). The mode choice underneath stays as-is; window mode remains the default.
| default XILINX_VERSAL_WDT_WINDOW_MODE | ||
| help | ||
| Select which watchdog engine of the IP the driver operates. | ||
|
|
There was a problem hiding this comment.
The engine is chosen at build time via a mutually-exclusive choice, so a single
image can drive only one engine even though the IP exposes both WWDT and GWDT concurrently (and a
system could legitimately want the window watchdog on one instance and the generic watchdog on
another). XWWDT_IRQ_NAME, the mode branches, and the BUILD_ASSERT all key off this global
choice, so it also applies uniformly to every instance of the compatible
Is a global build-time choice sufficient for the target products, or should the
engine be selectable per-node (e.g. a DT property / distinct compatible) so both engines can be used
in one image?
There was a problem hiding this comment.
A global build-time choice is sufficient for our target platforms. On Versal, which engine can trigger a PMC reset is fixed by the hardware design and the PLM boot-time CDO — not selectable at runtime. On most Versal boards, WWDT expiry is routed to the PMC via the CDO; GWDT is typically not. On a smaller set of designs, only GWDT is wired to the PMC for reset.
So each board/product uses one engine for platform reset, and that same choice applies to all xlnx,versal-wwdt instances in the image. A mutually exclusive Kconfig choice matches that integration model. I don't have a use case for mixed WWDT/GWDT reset routing across instances in one image; we can add per-node selection in a follow-up if a concrete product need appears.
6e41ae0 to
e066ddb
Compare
|
You have been identified as a likely reviewer for the code this pull request changes, but could not be added to its review request automatically. Please review it if you are able to. |
4223211 to
c472adf
Compare
|
|
|
||
| config XILINX_WINDOW_WATCHDOG | ||
| bool "Xilinx window watchdog driver" | ||
| config XILINX_VERSAL_WATCHDOG |
There was a problem hiding this comment.
XILINX_WINDOW_WATCHDOG was released in v4.3.0 (driver added in
e343379510f1, v4.3.0-rc1~7233). This series renames it to XILINX_VERSAL_WATCHDOG on the
4.4 development branch (current VERSION = 4.4.99) with no release-notes / migration-guide
entry and no deprecated alias. Downstream configs that set CONFIG_XILINX_WINDOW_WATCHDOG=…
will silently stop resolving after upgrade.
Add a doc/releases/release-notes-4.4.rst (and/or migration guide) entry noting the
rename XILINX_WINDOW_WATCHDOG → XILINX_VERSAL_WATCHDOG. Practical impact is low because the
symbol is default y and gated on `DT_HAS_XLNX_VERSAL_WWDT_ENABLED
There was a problem hiding this comment.
And also this commit bundles three distinct changes: (a) a pure refactor that extracts the
existing window logic into wdt_xilinx_wwdt_install_window() and wraps setup/feed/
disable/isr in IS_ENABLED(...WINDOW_MODE) branches, (b) the new GWDT feature, and
(c) the Kconfig symbol rename. Mixing a behavior-preserving refactor with a new feature and a
rename in one 362-line commit makes review and bisection harder.
Consider splitting: one commit that refactors the window path into per-mode helpers
(no behavior change), then a commit that adds GWDT + the Kconfig choice/rename.
c472adf to
af09921
Compare
The driver does not use any hwinfo API. Remove the stray include. Signed-off-by: Harini T <harini.t@amd.com>
On Versal/Versal Gen2 the WWDT expiry action (no reset, CPU/subsystem reset, or SoC reset) is routed through the Error Aggregation Module and configured by the PLM via the boot-time CDO. Zephyr on the APU/RPU can neither observe nor change that routing, so no WDT_FLAG_RESET_* value can be honoured or guaranteed by the driver - including WDT_FLAG_RESET_SOC, which the driver previously accepted as the only valid value. Stop interpreting cfg->flags and accept the timeout regardless of the WDT_FLAG_RESET_* value; the reset behaviour remains entirely defined by platform firmware. Emit a warning at install time so a caller is aware that WDT_FLAG_RESET_* is not honoured, and document the firmware-owned reset behaviour in the binding. Signed-off-by: Harini T <harini.t@amd.com>
Store the register region with the DEVICE_MMIO ROM/RAM helpers and access it via DEVICE_MMIO_GET() instead of a raw physical base address. On MMU targets (e.g. the aarch64 APU) the region is mapped dynamically via device_map(); on non-MMU targets (e.g. the R52 RPU) it reduces to direct physical access, so behaviour is unchanged there. Signed-off-by: Harini T <harini.t@amd.com>
wdt_install_timeout() accepted window.max == 0, programming a zero timeout and returning success instead of -EINVAL. Reject it. Signed-off-by: Harini T <harini.t@amd.com>
af09921 to
ad16a5e
Compare
|
Scope update: I've dropped the final two commits (WINT interrupt callback and generic watchdog / GWDT support) and force-pushed. The series is now limited to the WWDT cleanup and fixes: drop unused include, firmware-owned reset handling, DEVICE_MMIO conversion, and zero-max-window rejection. Rationale:
|



Summary
This series cleans up the Xilinx Versal Window Watchdog (WWDT) driver: drops an unused include, corrects reset-flag handling to reflect the platform's firmware-owned reset model, converts register access to the
DEVICE_MMIOAPI, and fixes a zero-max-window validation gap. The changes are backward compatible: existing device tree nodes that providereg/clockscontinue to work unchanged, and there is no new required property or Kconfig option.Commits
WDT_FLAG_RESET_*value (includingWDT_FLAG_RESET_NONE). Stop rejecting non-WDT_FLAG_RESET_SOCflags; accept the timeout regardless and warn at install time that the flag has no effect. The firmware-owned reset behavior is documented in the binding.DEVICE_MMIOROM/RAM helpers and access it viaDEVICE_MMIO_GET(). On MMU targets (aarch64 APU) the region is mapped dynamically viadevice_map(); on non-MMU targets (R52 RPU) it reduces to direct physical access, so behavior is unchanged there.wdt_install_timeout()acceptedwindow.max == 0, programming a zero timeout and returning success; reject it with-EINVAL.