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

[yangxiaojuan]

On 2022/5/19 下午11:24, Richard Henderson wrote:
> 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.
The  documentation is indeed not detailed, and we find a colleague in 
hadware  department  to confirm your problem.

The rtc_write and rtc_match both are absolute value, rtc_write will add 
counters if rtc_en enabled.
when rtc_write == rtc_match, rtc interrupt will happen. it also applies 
to this situation: rtc_write=0, rtc_match=0, and the rtc_en enabled, irq 
happens immediately. And the same for TOY.

> 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?
If RTC_EN disabled, it do not support to write rtc_write and 
rtc_match[n], and the rtc_write will not add counters, and the interrupt 
is also disabled, EO do not control this. So, we should check rtc_en 
when write rtc_write and rtc_match. And the same for TOY.

Thanks.
Xiaojuan