Hi Bernhard,
On Mon, Oct 5, 2026 at 6:21 PM Bernhard Beschow <[email protected]> wrote:
>
> Since WDT and WDE are write one once bits, their values always
> correspond with their respective locked attributes. Thus, the attributes
> are redundant. Take advantage of this observation to simplify the code.
>
> Note that removing the attributes changes the vmstate structure which
> constitutes a migration break. This is allowed since QEMU does not
> guarantee migration compatibility for i.MX devices between feature
> releases.
>
> Signed-off-by: Bernhard Beschow <[email protected]>
> ---
> include/hw/watchdog/wdt_imx2.h | 2 --
> hw/watchdog/wdt_imx2.c | 21 ++++-----------------
> 2 files changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/include/hw/watchdog/wdt_imx2.h b/include/hw/watchdog/wdt_imx2.h
> index fc01fc46e7..0bf2903959 100644
> --- a/include/hw/watchdog/wdt_imx2.h
> +++ b/include/hw/watchdog/wdt_imx2.h
> @@ -84,8 +84,6 @@ struct IMX2WdtState {
> uint16_t wmcr;
>
> bool wcr_locked; /* affects WDZST, WDBG, and WDW */
> - bool wcr_wde_locked; /* affects WDE */
> - bool wcr_wdt_locked; /* affects WDT, cleared on POR */
> };
>
> #endif /* WDT_IMX2_H */
> diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
> index a15f452d48..e9cb5819e4 100644
> --- a/hw/watchdog/wdt_imx2.c
> +++ b/hw/watchdog/wdt_imx2.c
> @@ -59,8 +59,6 @@ static void imx2_wdt_reset(DeviceState *dev)
>
> s->wicr_locked = false;
> s->wcr_locked = false;
> - s->wcr_wde_locked = false;
> - s->wcr_wdt_locked = false;
>
> s->wcr = IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS;
> s->wsr = 0;
> @@ -156,24 +154,14 @@ static void imx2_wdt_write(void *opaque, hwaddr addr,
> switch (addr) {
> case IMX2_WDT_WCR:
> if (s->wcr_locked) {
> + /* WDZST, WDBG, and WDW are write-once bits */
> value &= ~IMX2_WDT_WCR_LOCK_MASK;
> value |= (s->wcr & IMX2_WDT_WCR_LOCK_MASK);
> }
> s->wcr_locked = true;
> - if (s->wcr_wde_locked) {
> - value &= ~IMX2_WDT_WCR_WDE;
> - value |= (s->wcr & IMX2_WDT_WCR_WDE);
> - } else if (value & IMX2_WDT_WCR_WDE) {
> - s->wcr_wde_locked = true;
> - }
> - if (s->wcr_wdt_locked) {
> - value &= ~IMX2_WDT_WCR_WDT;
> - value |= (s->wcr & IMX2_WDT_WCR_WDT);
> - } else if (value & IMX2_WDT_WCR_WDT) {
> - s->wcr_wdt_locked = true;
> - }
>
> - s->wcr = value;
> + /* WDT and WDE are write one once bits */
> + s->wcr = value | (s->wcr & (IMX2_WDT_WCR_WDT | IMX2_WDT_WCR_WDE));
> if (!(value & IMX2_WDT_WCR_SRS)) {
> s->wrsr = IMX2_WDT_WRSR_SFTW;
> }
In the following codes:
if (!(value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS)) ||
(!(value & IMX2_WDT_WCR_WT) && (value & IMX2_WDT_WCR_WDE))) {
watchdog_perform_action();
}
Since you removed the part:
value |= (s->wcr & IMX2_WDT_WCR_{WDE,WDT})
now 'value' does not contain the preserved WDE/WDT bits (they are only
preserved in s->wcr in this patch), the watchdog_perform_action()
won't be called.
> @@ -234,13 +222,12 @@ static const MemoryRegionOps imx2_wdt_ops = {
>
> static const VMStateDescription vmstate_imx2_wdt = {
> .name = "imx2.wdt",
> + .version_id = 1,
> .fields = (const VMStateField[]) {
> VMSTATE_PTIMER(timer, IMX2WdtState),
> VMSTATE_PTIMER(itimer, IMX2WdtState),
> VMSTATE_BOOL(wicr_locked, IMX2WdtState),
> VMSTATE_BOOL(wcr_locked, IMX2WdtState),
> - VMSTATE_BOOL(wcr_wde_locked, IMX2WdtState),
> - VMSTATE_BOOL(wcr_wdt_locked, IMX2WdtState),
> VMSTATE_UINT16(wcr, IMX2WdtState),
> VMSTATE_UINT16(wsr, IMX2WdtState),
> VMSTATE_UINT16(wrsr, IMX2WdtState),
Regards,
Bin
