On 27/02/2023 10:26:09+0800, Binbin Zhou wrote:
> > > > > +static inline void ls2x_rtc_alarm_regs_to_time(struct ls2x_rtc_regs *regs,
> > > > > + struct rtc_time *tm)
> > > > > +{
> > > > > + tm->tm_sec = FIELD_GET(TOY_MATCH_SEC, regs->reg0);
> > > > > + tm->tm_min = FIELD_GET(TOY_MATCH_MIN, regs->reg0);
> > > > > + tm->tm_hour = FIELD_GET(TOY_MATCH_HOUR, regs->reg0);
> > > > > + tm->tm_mday = FIELD_GET(TOY_MATCH_DAY, regs->reg0);
> > > > > + tm->tm_mon = FIELD_GET(TOY_MATCH_MON, regs->reg0) - 1;
> > > > > + /*
> > > > > + * The rtc SYS_TOYMATCH0/YEAR bit field is only 6 bits, so it means 63
> > > > > + * years at most. Therefore, The RTC alarm years can be set from 1900
> > > > > + * to 1963. This causes the initialization of alarm fail during call
> > > > > + * __rtc_read_alarm.
> > > > > + * We add 64 years offset to ls2x_rtc_read_alarm. After adding the
> > > > > + * offset, the RTC alarm clock can be set from 1964 to 2027.
> > > > > + */
> > > > > + tm->tm_year = FIELD_GET(TOY_MATCH_YEAR, regs->reg0) + 64;
> > > >
> > > > This is not symmetric with ls2x_rtc_time_to_alarm_regs, how can it work?
> > >
> > > This is to avoid an "invalid alarm value" at boot time, which of
> > > course should not be a good solution.
> > > When the alarm value is read at boot time, "year" is not yet set to
> > > the proper value so the year is always set to 1900.
> >
> > Why isn't it set at boot time? Isn't the register persisting after a
> > reboot?
> > Setting a bogus alarm is not a solution.
> >
>
> Hi, Alexandre:
>
> Sorry, I seem to have misled the issue.
> This is a hardware bug, as we know from the datasheet, the year field
> in the TOY_MATCH register has only 6 bits (bit[31:26]), so the maximum
> value is 63. For example, 2023 can only be read here as 1959,
> resulting in an invalid alarm.
> The current workaround: after reading the year field in
> ls2x_rtc_read_alarm(), manually add 64 or a multiple of 64 (equivalent
> to completing the high bits), which also ensures that the reading and
> writing is consistent.
My first complain was that this is not symmetric with
ls2x_rtc_time_to_alarm_regs. If you are adding 64 when reading the
alarm, you need to remove 64 when setting the alarm.
Now I get that FIELD_PREP will drop the overflowing bits.
Instead of having support for the 1964 to 2027 range, you should
probably aim for 2000 to 2064.
Also, this makes me realize that you are not setting the year properly,
the datasheet I have says that the supported year goes from 00 to 99.
This is also what you set in .probe.
Removing 100 from tm_year when setting and adding it back when reading
would fix all of that.
> > > The LS7A and Loongson-2K datasheets also do not mention any latching
> > > happening. Reading TOY_READ1_REG first is probably just a matter of
> > > habit.
> > > I tried using regmap_bulk_xxx() and it also reads and writes time
> > > properly. In the next version I will rewrite this part of the code.
> > >
> > > Example:
> > >
> > > #define LS2X_NUM_TIME_REGS 2
> > >
> > > u32 rtc_data[LS2X_NUM_TIME_REGS];
> > > struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> > >
> > > ret = regmap_bulk_read(priv->regmap, TOY_READ0_REG, rtc_data,
> > > LS2X_NUM_TIME_REGS);
> > >
> >
> > Doing a bulk read doesn't guarantee the atomicity of the operation. You
> > really must check whether a register changed while reading the other
> > one.
> >
>
> How about protecting with mutex?
>
No, this would fix multiple processes accessing a variable, here what
you have are two unsynchronized hardware registers.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
