On Mon, 21 Sept 2026 at 22:47, Bernhard Beschow <[email protected]> wrote:
>
> Guests such as u-boot may experience inadvertant reboots when configuring
> the WCR register. Apparently there is a typo in the WCR locked bits
> handling: The locked values for s->wcr must come from itself rather than
> from s->wicr. While at it remove the redundant locked attributes for the
> bits which stick to one once written with one.
"while at it" is one of those phrases that tends to suggest
this should be more than one commit. It's doing a lot more
than fixing the bug where we use wicr when we should have used wcr.
> Note that removing the locked attributes required a migration break. Defer
> that to a subsequent patch to help backporting.
>
> Fixes: daca13d495a1 ("hw/watchdog: Implement full i.MX watchdog
> support")
> cc: qemu-stable
> Signed-off-by: Bernhard Beschow <[email protected]>
> ---
> hw/watchdog/wdt_imx2.c | 19 +++----------------
> 1 file changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
> index c83d4107c1..aaf4695db2 100644
> --- a/hw/watchdog/wdt_imx2.c
> +++ b/hw/watchdog/wdt_imx2.c
> @@ -59,9 +59,8 @@ static void imx2_wdt_reset(DeviceState *dev)
>
> s->wicr_locked = false;
> s->wcr_locked = false;
> - s->wcr_wde_locked = false;
>
> - s->wcr = IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS;
> + s->wcr = IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS | (s->wcr &
> IMX2_WDT_WCR_WDT);
This doesn't look right. Reset should be "cold start power on", so
the reset value of WCR shouldn't depend on what s->wcr happens
to contain now. It's also not mentioned in the commit message.
> s->wsr = 0;
> s->wrsr &= ~(IMX2_WDT_WRSR_TOUT | IMX2_WDT_WRSR_SFTW);
> s->wicr = IMX2_WDT_WICR_WICT_DEF;
> @@ -156,23 +155,11 @@ static void imx2_wdt_write(void *opaque, hwaddr addr,
> case IMX2_WDT_WCR:
> if (s->wcr_locked) {
> value &= ~IMX2_WDT_WCR_LOCK_MASK;
> - value |= (s->wicr & 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->wicr & ~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->wicr & ~IMX2_WDT_WCR_WDT);
> - } else if (value & IMX2_WDT_WCR_WDT) {
> - s->wcr_wdt_locked = true;
> - }
>
> - s->wcr = value;
> + s->wcr = value | (s->wcr & (IMX2_WDT_WCR_WDT | IMX2_WDT_WCR_WDE));
This looks correct, but I think it would be helpful to have a
comment that notes that WDZST, WDBG and WDW are "write-once only"
whereas WDT and WDE are "write one once only". The difference is
a bit subtle otherwise.
thanks
-- PMM
