| Message ID | 166783932395.3279.1096141058484230644-5@git.sr.ht (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/timer/imx_epit: imprive and fix compare timer handling | expand |
On Mon, 7 Nov 2022 at 16:42, ~axelheider <axelheider@git.sr.ht> wrote: > > 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... > @@ -205,7 +206,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value, > ptimer_transaction_begin(s->timer_reload); > > /* Update the frequency. Has been done already in case of a reset. */ > - if (!(s->cr & CR_SWR)) { > + if (!(value & CR_SWR)) { > imx_epit_set_freq(s); > } thanks -- PMM
>> >> 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
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)); /* @@ -205,7 +206,7 @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value, ptimer_transaction_begin(s->timer_reload); /* Update the frequency. Has been done already in case of a reset. */ - if (!(s->cr & CR_SWR)) { + if (!(value & CR_SWR)) { imx_epit_set_freq(s); }