Am 28. September 2026 18:22:49 UTC schrieb Peter Maydell
<[email protected]>:
>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.
Okay, I'll split the patch: One which fixes the bug by fixing the typo,
suitable for backporting. Then another one resolving the redundant boolean
attributes which breaks migration.
>
>> 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.
Note that `s->wcr_wdt_locked = false;` is missing in the existing code, so
retaining the IMX2_WDT_WCR_WDT position preserves behavior, hence no mentioning
in the commit message.
WDT is cleared on POR while WDE is cleared on "system reset". So indeed WDT
needs to be cleared when reset represents "cold start power on". I'll add a
(backporteable) patch.
>
>> 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.
Okay, I'll add a comment in the patch where the redundant attributes are
removed.
Best regards,
Bernhard
>
>thanks
>-- PMM
