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

[Richard Henderson]

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".

> +#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?

> +    case SYS_RTCCTRL:
> +        s->cntrctl = val;
> +        break;

Need to check REN, TEN, and EO fields.

> +    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?

> +    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?



r~