Re: [PATCH qemu.git v2 5/9] hw/timer/imx_epit: do not persist CR.SWR bit

[Axel Heider <axelheider@gmx.de>]

>>
>> From: Axel Heider <axel.heider@hensoldt.net>
>> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
>> ---
>>   hw/timer/imx_epit.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
>> index 5315d9633e..6af460946f 100644
>> --- a/hw/timer/imx_epit.c
>> +++ b/hw/timer/imx_epit.c
>> @@ -191,8 +191,9 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value,
>>       case 0: /* CR */
>>
>>           oldcr = s->cr;
>> -        s->cr = value & 0x03ffffff;
>> -        if (s->cr & CR_SWR) {
>> +        /* SWR bit is never persisted, it clears itself once reset is done */
>> +        s->cr = (value & ~CR_SWR) & 0x03ffffff;
>> +        if (value & CR_SWR) {
>>               /* handle the reset */
>>               imx_epit_reset(DEVICE(s));
>>               /*
>>
>
> There's a comment just below here that says "can we just 'break'
> in this case?". That's there because last time we had to touch
> this device we didn't have enough specific knowledge of the hardware
> or test cases so we made a refactor that left the code with the same
> probably-incorrect behaviour it had before the refactor. But, since
> you're working on this device anyway: can we simply add the "break"
> after imx_epit_reset() ?
>
> If we can just say "if CR_SWR is set then the device resets like
> a hardware reset", then this all simplifies out a lot and this
> patch isn't necessary at all. (imx_epit_reset() clears the CR_SWR bit.)
> I'm fairly sure we ought to be able to do that, and the missing 'break'
> was just a bug...


You are right, this patch is not needed. Actually, the refactoring in
the other patches in this series makes this quite obvious then anyway.
And you comment fully holds, eventually all the setup is skipped in
case of a reset.

I will drop this patch from the set. Thanks for the review.

Axel