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

[yangxiaojuan]

On 2022/5/8 上午5:55, Richard Henderson wrote:
> On 4/29/22 05:07, Xiaojuan Yang wrote:
> > +/*
> > + * Shift bits and filed mask
> > + */
> > +#define TOY_MON_MASK   0x3f
> > +#define TOY_DAY_MASK   0x1f
> > +#define TOY_HOUR_MASK  0x1f
> > +#define TOY_MIN_MASK   0x3f
> > +#define TOY_SEC_MASK   0x3f
> > +#define TOY_MSEC_MASK  0xf
> > +
> > +#define TOY_MON_SHIFT  26
> > +#define TOY_DAY_SHIFT  21
> > +#define TOY_HOUR_SHIFT 16
> > +#define TOY_MIN_SHIFT  10
> > +#define TOY_SEC_SHIFT  4
> > +#define TOY_MSEC_SHIFT 0
> > +
> > +#define TOY_MATCH_YEAR_MASK  0x3f
> > +#define TOY_MATCH_MON_MASK   0xf
> > +#define TOY_MATCH_DAY_MASK   0x1f
> > +#define TOY_MATCH_HOUR_MASK  0x1f
> > +#define TOY_MATCH_MIN_MASK   0x3f
> > +#define TOY_MATCH_SEC_MASK   0x3f
> > +
> > +#define TOY_MATCH_YEAR_SHIFT 26
> > +#define TOY_MATCH_MON_SHIFT  22
> > +#define TOY_MATCH_DAY_SHIFT  17
> > +#define TOY_MATCH_HOUR_SHIFT 12
> > +#define TOY_MATCH_MIN_SHIFT  6
> > +#define TOY_MATCH_SEC_SHIFT  0
>
> While this is ok, it would be better to use <hw/registerfields.h>
> This becomes
>
> FIELD(TOY, MON, 26, 6)
> FIELD(TOY, DAY, 21, 5)
> FIELD(TOY, HOUR, 16, 5)
>
> You then extract with FIELD_EX32(val, TOY, MON), etc.
>
> I will also mention that "millisec" is misnamed in the documentation.  
> Since it represents units of 0.1 seconds, the prefix should be "deci".
Thanks very much, i changed it to this format:

+FIELD(TOY, MON, 26, 6)
+FIELD(TOY, DAY, 21, 5)
+FIELD(TOY, HOUR, 16, 5)
+FIELD(TOY, MIN, 10, 6)
+FIELD(TOY, SEC, 4, 6)
+FIELD(TOY, MSEC, 0, 4)
+
+FIELD(TOY_MATCH, YEAR, 26, 6)
+FIELD(TOY_MATCH, MON, 22, 4)
+FIELD(TOY_MATCH, DAY, 17, 5)
+FIELD(TOY_MATCH, HOUR, 12, 5)
+FIELD(TOY_MATCH, MIN, 6, 6)
+FIELD(TOY_MATCH, SEC, 0, 6)
+
+FIELD(RTC_CTRL, RTCEN, 13, 1)
+FIELD(RTC_CTRL, TOYEN, 11, 1)
+FIELD(RTC_CTRL, EO, 8, 1)

 get time from the value, like this:
...
case SYS_TOYWRITE0:
         qemu_get_timedate(&tm, s->offset);
+        tm.tm_sec = FIELD_EX32(val, TOY, SEC);
+        tm.tm_min = FIELD_EX32(val, TOY, MIN);
+        tm.tm_hour = FIELD_EX32(val, TOY, HOUR);
+        tm.tm_mday = FIELD_EX32(val, TOY, DAY);
+        tm.tm_mon = FIELD_EX32(val, TOY, MON) - 1;
...

> > +#define TOY_ENABLE_BIT (1U << 11)
> ...
> > +enum {
> > +    TOYEN = 1UL << 11,
> > +    RTCEN = 1UL << 13,
> > +};
>
> You have two copies of the same bit.  It would be best to fill in the 
> rest of the bits in RTCCTRL, using FIELD().
>
> > +    case SYS_RTCREAD0:
> > +        val = s->rtccount;
> > +        break;
>
> Surely this needs to examine qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) and 
> scale the offset back to 32kHz.
>
> > +    case SYS_TOYMATCH0:
> > +        s->toymatch[0] = val;
> > +        qemu_get_timedate(&tm, s->offset);
> > +        tm.tm_sec = (val >> TOY_MATCH_SEC_SHIFT) & TOY_MATCH_SEC_MASK;
> > +        tm.tm_min = (val >> TOY_MATCH_MIN_SHIFT) & TOY_MATCH_MIN_MASK;
> > +        tm.tm_hour = ((val >> TOY_MATCH_HOUR_SHIFT) & 
> > TOY_MATCH_HOUR_MASK);
> > +        tm.tm_mday = ((val >> TOY_MATCH_DAY_SHIFT) & 
> > TOY_MATCH_DAY_MASK);
> > +        tm.tm_mon = ((val >> TOY_MATCH_MON_SHIFT) & 
> > TOY_MATCH_MON_MASK) - 1;
> > +        year_diff = ((val >> TOY_MATCH_YEAR_SHIFT) & 
> > TOY_MATCH_YEAR_MASK);
> > +        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);
> > +        break;
> > +    case SYS_TOYMATCH1:
> > +        s->toymatch[1] = val;
> > +        break;
> > +    case SYS_TOYMATCH2:
> > +        s->toymatch[2] = val;
> > +        break;
>
> 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;
...
> > +    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;
+        }
...
> > +    case SYS_RTCWRTIE0:
> > +        s->rtccount = val;
> > +        break;
>
> Need to compute a new rtc_offset from QEMU_CLOCK_VIRTUAL, to match 
> RTCREAD0 above.
>
> > +    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.
> > +    d->timer = timer_new_ms(rtc_clock, toy_timer, d);
> > +    timer_mod(d->timer, qemu_clock_get_ms(rtc_clock) + 100);
>
> Where does this number come from?  In any case, after reset I would 
> expect (1) the rtc and toy to be disabled (documentation says must 
> initialize rtcctrl bits) and (2) I would expect all of the comparison 
> registers to be 0 and thus no timer enabled.
> I guess this 100 milliseconds thing is trying to match 1 decisecond 
> from TOYMATCH0?
Thanks, I think i should delete the it.

Thanks.
Xiaojuan