Am 5. Oktober 2026 15:01:37 UTC schrieb Bin Meng <[email protected]>:
>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.
Fixed in v4 by preserving the value part. Thanks for catching this.
Best regards,
Bernhard
>
>> @@ -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
