Re: [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode

[Stafford Horne <shorne@gmail.com>]

On Sat, Nov 23, 2024 at 07:39:57AM -0600, Richard Henderson wrote:
> On 11/23/24 04:38, Stafford Horne wrote:
> > +    or1k_timer->ttcr = or1k_timer->ttcr_offset +
> > +        (now - or1k_timer->clk_offset + TIMER_PERIOD - 1) / TIMER_PERIOD;
> 
> Better using DIV_ROUND_UP.

Sure, I can change it to that.

> > +        /* Zero the count by applying a negative offset to the counter */
> > +        or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);
> 
> Since UINT32_MAX is -1 in this context, this appears to be off-by-one.
> I think -(ttmr & mask) alone is correct.

Thanks, I did send a mail to Joel asking about this bit.  He didn't respond for 2
weeks to I just sent the patch as is as it appears to work.  As I understand,
yes UINT32_MAX is just -1.  But why the -1?  I guess it's because after
ttcr_offset is updated we call cpu_openrisc_timer_update() which calls
cpu_openrisc_count_update() to update ttcr.  Since a few _ns would have passed
and we are rounding up it will update ttcr to 0.

But maybe I am reading too much into it.

If you think that makes sense I could add a comment as such, also I would prefer
to change to UINT32_MAX to -1.

-Stafford