[PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Alex Elder]

On 6/23/25 1:54 PM, Mateusz Jończyk wrote:
> W dniu 22.06.2025 o 05:29, Alex Elder pisze:
>> Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
>> only setting and reading the time are supported.
>>
>> The PMIC is implemented as a multi-function device.  This RTC is
>> probed based on this driver being named in a MFD cell in the simple
>> MFD I2C driver.
>>
>> Signed-off-by: Alex Elder <elder at riscstar.com>
>> ---
>> v3: - Added this driver to the series, in response to Lee Jones saying
>>        more than one MFD sub-device was required to be acceptable
> [snip]
>> +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
>> +{
>> +    struct p1_rtc *p1 = dev_get_drvdata(dev);
>> +    u8 time[tu_count];
>> +    int ret;
>> +
>> +    ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, 
>> sizeof(time));
>> +    if (ret)
>> +        return ret;
> 
> Hello,
> 
> Are you sure that you read the time parts consistently here? I mean:
> is there a risk that the clock is updating below you - so that for
> example during the transition

The documentation says this:

   If need to read the calendar value currently recorded inside
   the PMIC, need to read RTC_COUNT_S first. At the same time,
   this operation will latch all current calendar values to the
   registers RTC_COUNT_S ~ RTC_COUNT_Y.

The RTC_COUNT_S register is the first one read in the I2C
bulk request.  So I concluded that the request would record
a consistent timestamp in all 6 registers.

That said, the downstream code did a loop as you describe.  I
will ask the vendor about this.


Related:  I disabled the RTC while updating (which is what the
downstream code did).  But based on what the documentation says
I shouldn't need to do that either.

   If need to reset the current calendar value, need to
   configure each calendar value (RTC_COUNT_S ~ RTC_COUNT_Y)
   in order. After finally writing the register RTC_COUNT_Y,
   PMIC will update the calendar value configured by the
   current user to the internal timer of the RTC module, and
   so on.

Here too, the last register written in the bulk request is
RTC_COUNT_Y, so again I think a consistent value stored in
the 6 registers will be updated at once.

Either way, I'll make an inquiry about both of these.  I'll
explain what I learned in the next version of this series,
and will update the code accordingly.

> 12:59:59 -> 13:00:00
> 
> you are going to read 12:00:00 or 12:59:00?
> 
> If so, you could for example read the time in a loop until you get
> two same readouts.
> 
>> +
>> +    t->tm_sec = time[tu_second] & GENMASK(5, 0);
>> +    t->tm_min = time[tu_minute] & GENMASK(5, 0);
>> +    t->tm_hour = time[tu_hour] & GENMASK(4, 0);
>> +    t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
>> +    t->tm_mon = time[tu_month] & GENMASK(3, 0);
>> +    t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;
> 
> Is it necessary to use the bitmasks here? Are the higher bits undefined
> in hardware? If so, is it necessary to mask them while writing the time
> in p1_rtc_set_time()?

It is probably not necessary to do the bitmask.  But the upper
bits are marked "reserved" and so I the correct thing to do is
to explicitly ignore them (by masking them off).  Is there a
reason you'd rather they not be masked?

There should be no need to mask them in the set_time() function.
The core code verifies all values are in range before it calls
rtc_class_ops->set_time().  So the upper bits should all be zero
by the time p1_rtc_set_time() is called.

>> +    /* tm_wday, tm_yday, and tm_isdst aren't used */
>> +
>> +    return 0;
>> +}
>> +
>> +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
>> +{
>> +    struct p1_rtc *p1 = dev_get_drvdata(dev);
>> +    u8 time[tu_count];
>> +    int ret;
>> +
>> +    time[tu_second] = t->tm_sec;
>> +    time[tu_minute] = t->tm_min;
>> +    time[tu_hour] = t->tm_hour;
>> +    time[tu_day] = t->tm_mday - 1;
>> +    time[tu_month] = t->tm_mon;
>> +    time[tu_year] = t->tm_year - 100;
>> +
>> +    /* Disable the RTC to update; re-enable again when done */
>> +    ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, 
>> sizeof(time));
>> +
>> +    (void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);

> Perhaps you shouldn't ignore any errors here - failing to restart
> the clock should make p1_rtc_set_time() return an error code.

I intentionally return any error from the bulk write call.

But you're right:  if that doesn't have an error and re-enabling
the RTC does, that error should be returned.

I'm hoping there's no need to disable the clock after all,
and that will simplify things here.  But either way, I'll
take your suggestion (but still return the first error if
more than one occurs).

Thanks a lot for the review.

					-Alex

>> +
>> +    return ret;
>> +}
>> +
> 
> Greetings,
> 
> Mateusz
>