[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 38/43] hw/loongarch: Add LoongArch ls7a rtc device support
From: |
yangxiaojuan |
Subject: |
Re: [PATCH v3 38/43] hw/loongarch: Add LoongArch ls7a rtc device support |
Date: |
Tue, 10 May 2022 17:11:59 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 |
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