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

[Alex Elder]

On 6/25/25 5:01 PM, Alexandre Belloni wrote:
> On 25/06/2025 11:41:15-0500, Alex Elder wrote:
> > +/*
> > + * The P1 hardware documentation states that the register values are
> > + * latched to ensure a consistent time snapshot within the registers,
> > + * but these are in fact unstable due to a bug in the hardware design.
> > + * So we loop until we get two identical readings.
> > + */
> > +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
> > +{
> > +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> > +	struct regmap *regmap = p1->regmap;
> > +	u32 count = RTC_READ_TRIES;
> > +	u8 times[2][6];
> > +	size_t size;
> > +	u32 i = 0;
> > +	u8 *time;
> > +	int ret;
> > +
> > +	size = sizeof(times[0]);
> > +	ret = regmap_bulk_read(regmap, RTC_TIME, times[i], size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	do {
> > +		i = 1 - i;
> > +		ret = regmap_bulk_read(regmap, RTC_TIME, times[i], size);
> > +		if (ret)
> > +			return ret;
> > +	} while (memcmp(times[i], times[1 - i], size) && --count);
>
> Simply checking the seconds is enough, unless you expect regmap_bulk_read to ake
> more than a minute so you don't need a two dimension array.

I hadn't thought it through, but you're right.  I'll still
do bulk reads but will save only the seconds register and
compare it to the previous.

> > +
> > +	if (!count)
> > +		return -EIO;
> > +
> > +	time = &times[0][0];
> > +
> > +	t->tm_sec = time[0] & GENMASK(5, 0);
> > +	t->tm_min = time[1] & GENMASK(5, 0);
> > +	t->tm_hour = time[2] & GENMASK(4, 0);
> > +	t->tm_mday = (time[3] & GENMASK(4, 0)) + 1;
> > +	t->tm_mon = time[4] & GENMASK(3, 0);
> > +	t->tm_year = (time[5] & GENMASK(5, 0)) + 100;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * The P1 hardware documentation states that values in the registers are
> > + * latched so when written they represent a consistent time snapshot.
> > + * Nevertheless, this is not guaranteed by the implementation, so we must
> > + * disable the RTC while updating it.
> > + */
> > +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
> > +{
> > +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> > +	struct regmap *regmap = p1->regmap;
> > +	u8 time[6];
> > +	int ret2;
> > +	int ret;
> > +
> > +	time[0] = t->tm_sec;
> > +	time[1] = t->tm_min;
> > +	time[2] = t->tm_hour;
> > +	time[3] = t->tm_mday - 1;
> > +	time[4] = t->tm_mon;
> > +	time[5] = t->tm_year - 100;
> > +
> > +	/* Disable the RTC to update; re-enable again when done */
> > +	ret = regmap_update_bits(regmap, RTC_CTRL, RTC_EN, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_bulk_write(regmap, RTC_TIME, time, sizeof(time));
> > +
> Honnestly, I'd simply go for
>
> 	if (ret)
> 		return ret;
>
> Here, you are trying to set the time, I'm not sure it is worth trying to
> reenable the rtc while hoping the previous time has been kept.

OK.  I normally at least try to return to the original state
even when errors occur.

I don't know who updates the RTC, but I suppose if any error
occurs writing a value they should assume it's "not working".

> This also shows that p1_rtc_read_time should check that RTC_EN is set, else it
> knows the time has never been set and is invalid.

That's a good point.  I'll add a check.  And with that it should
be fine to return without re-enabling.

Thanks for your review.  I'll wait until tomorrow to send the
next version.

					-Alex

> > +	ret2 = regmap_update_bits(regmap, RTC_CTRL, RTC_EN, RTC_EN);
> > +
> > +	return ret ? : ret2;		/* Return the first error */
>
>
> > +}
> > +
> > +static const struct rtc_class_ops p1_rtc_class_ops = {
> > +	.read_time = p1_rtc_read_time,
> > +	.set_time = p1_rtc_set_time,
> > +};
> > +
> > +static int p1_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct rtc_device *rtc;
> > +	struct p1_rtc *p1;
> > +
> > +	p1 = devm_kzalloc(dev, sizeof(*p1), GFP_KERNEL);
> > +	if (!p1)
> > +		return -ENOMEM;
> > +	dev_set_drvdata(dev, p1);
> > +
> > +	p1->regmap = dev_get_regmap(dev->parent, NULL);
> > +	if (!p1->regmap)
> > +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
> > +
> > +	rtc = devm_rtc_allocate_device(dev);
> > +	if (IS_ERR(rtc))
> > +		return dev_err_probe(dev, PTR_ERR(rtc),
> > +				     "error allocating device\n");
> > +	p1->rtc = rtc;
> > +
> > +	rtc->ops = &p1_rtc_class_ops;
> > +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > +	rtc->range_max = RTC_TIMESTAMP_END_2063;
> > +
> > +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
> > +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> > +
> > +	return devm_rtc_register_device(rtc);
> > +}
> > +
> > +static struct platform_driver p1_rtc_driver = {
> > +	.probe = p1_rtc_probe,
> > +	.driver = {
> > +		.name = MOD_NAME,
> > +	},
> > +};
> > +
> > +module_platform_driver(p1_rtc_driver);
> > +
> > +MODULE_DESCRIPTION("SpacemiT P1 RTC driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:" MOD_NAME);
> > --
> > 2.45.2