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

[Richard Henderson]

On 5/10/22 02:11, yangxiaojuan wrote:
> > Why does only register 0 affect expire time, and not all 3 registers?
> Thanks, the toymatch[1]/[2] should also affect expire time. I fixed it like 
> this:
>
> +static void rtc_toymatch_write(LS7ARtcState *s, struct tm *tm, uint64_t val)
> +{
> +    int64_t alarm_offset, year_diff, expire_time;
> +
> +    qemu_get_timedate(tm, s->offset);
> +    tm->tm_sec = FIELD_EX32(val, TOY_MATCH, SEC);
> +    tm->tm_min = FIELD_EX32(val, TOY_MATCH, MIN);
> +    tm->tm_hour = FIELD_EX32(val, TOY_MATCH, HOUR);
> +    tm->tm_mday = FIELD_EX32(val, TOY_MATCH, DAY);
> +    tm->tm_mon = FIELD_EX32(val, TOY_MATCH, MON) - 1;
> +    year_diff = FIELD_EX32(val, TOY_MATCH, MON);
> +    year_diff = year_diff - (tm->tm_year & TOY_MATCH_YEAR_MASK);
> +    tm->tm_year = tm->tm_year + year_diff;
> +    alarm_offset = qemu_timedate_diff(tm) - s->offset;
> +    if ((alarm_offset < 0) && (alarm_offset > -5)) {
> +        alarm_offset = 0;
> +    }
> +    expire_time = qemu_clock_get_ms(rtc_clock);
> +    expire_time += ((alarm_offset * 1000) + 100);
> +    timer_mod(s->timer, expire_time);
> +}
>
> ...
> case SYS_TOYMATCH0:
>       s->toymatch[0] = val;
> +     rtc_toymatch_write(s, &tm, val);
>       break;
> case SYS_TOYMATCH1:
>      s->toymatch[1] = val;
> +    rtc_toymatch_write(s, &tm, val);
>      break;
> case SYS_TOYMATCH2:
>       s->toymatch[2] = val;
> +     rtc_toymatch_write(s, &tm, val);
>       break;

You either have to have 6 timer objects, or you have to pick the minimum expire time 
between the 3 toymatch and the 3 rtcmatch values.

> ...
> > > +    case SYS_RTCCTRL:
> > > +        s->cntrctl = val;
> > > +        break;
> >
> > Need to check REN, TEN, and EO fields.
> Thanks, i fixed the rtc_ctrl writing function like this:
> ...
>       case SYS_RTCCTRL:
> -        s->cntrctl = val;
> +        if (FIELD_EX32(val, RTC_CTRL, RTCEN) &&
> +            FIELD_EX32(val, RTC_CTRL, TOYEN) &&
> +            FIELD_EX32(val, RTC_CTRL, EO)) {
> +            s->cntrctl = val;
> +        }

Uh, no, that's not what I meant by "check".

If TOYEN is off, then the toymatch timers must stop, at minimum; it might also mean that 
the time of day should not advance -- the documentation isn't clear.

If RTCEN is off, then similarly rtcmatch timers must stop and the rtcread value must not 
advance.

If EO is off, I would expect all of the above to stop, because the clock source 
is stopped.

You'd always record the store to s->cntrctl.


> > > +    case SYS_RTCMATCH0:
> > > +        s->rtcmatch[0] = val;
> > > +        break;
> > > +    case SYS_RTCMATCH1:
> > > +        val = s->rtcmatch[1];
> > > +        break;
> > > +    case SYS_RTCMATCH2:
> > > +        val = s->rtcmatch[2];
> > > +        break;
> >
> > Why do these not affect expire time?
> Sorry, i could not understand this very clearly, could you please explain it in more 
> detail? Thanks very much.

You're not raising an interrupt for any rtcmatch, as you did from toymatch0.


r~