Re: [PATCH v4 38/43] hw/loongarch: Add LoongArch ls7a rtc device support

[Richard Henderson]

On 5/19/22 06:04, yangxiaojuan wrote:
> On 2022/5/19 上午3:59, Richard Henderson wrote:
> > On 5/17/22 04:30, Xiaojuan Yang wrote:
> >
> > > +static void ls7a_stop_toymatch(LS7ARtcState *s)
> > > +{
> > > +    int i;
> > > +    uint64_t now;
> > > +
> > > +    now = qemu_clock_get_ms(rtc_clock);
> > > +    for (i = 0; i < TIMER_NUMS; i++) {
> > > +        if (s->toy_timer[i].flag) {
> > > +            s->toy_timer[i].enable_offset = s->toy_timer[i].timer->expire_time
> > > +                                            - now;
> > > +            timer_del(s->toy_timer[i].timer);
> >
> > I don't think you need to check flag here, or update enable_offset.
> > Just an unconditional timer_del to stop the timer callback from firing.
> Thanks very much, and i fixed it like this: Is this modification appropriate?
> static void ls7a_rtc_stop(LS7ARtcState *s)
> {
>      int i;
>      int64_t rtc_val, rtc_diff, now;
>      now = ls7a_rtc_ticks();
>
>      for (i = 0; i < TIMER_NUMS; i++) {
>          if (s->rtc_timer[i].flag) {
>              rtc_val = s->rtcmatch[i];
>              rtc_diff = rtc_val - now - s->offset_rtc;
>              s->rtc_timer[i].save_offset = rtc_diff;
>          }
>          timer_del(s->rtc_timer[i].timer);
> }

I think you should drop "flag" entirely.  I don't see what it accomplishes.  It does not 
correspond to a bit in the state of the La7a rtc device.

Do you know if the documentation is incomplete, and rtcmatch[n] == 0 is a magic number 
that indicates that the match value is disabled?  Because if there is no magic number for 
disabling the match, I would expect *any* number, including 0, to match every 2**32 / 
2**15 seconds ~= 36 hours.

It also occurs to me that EO is probably the thing that controls whether RTC "ticks", and 
RTCEN is probably the thing that controls whether RTC match interrupts are delivered, and 
the same for TOY.  Can you confirm this?


r~