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

[Richard Henderson]

On 11/23/24 11:11, Stafford Horne wrote:
> 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.

I think you're reading too much into it -- I just think it's a bug which isn't 
particularly noticeable because the clock is only off by 1ns.


r~

> 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