On Fri, Jan 27, 2023 at 04:09:54PM +0100, Alexandre Belloni wrote:
> On 26/01/2023 15:20:49+0100, Johan Hovold wrote:
> > On many Qualcomm platforms the PMIC RTC control and time registers are
> > read-only so that the RTC time can not be updated. Instead an offset
> > needs be stored in some machine-specific non-volatile memory, which the
> > driver can take into account.
> >
> > Add support for storing a 32-bit offset from the Epoch in an nvmem cell
> > so that the RTC time can be set on such platforms.
> >
> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > ---
> > drivers/rtc/rtc-pm8xxx.c | 134 +++++++++++++++++++++++++++++++++++----
> > 1 file changed, 123 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> > index 922aef0f0241..09816b9f6282 100644
> > --- a/drivers/rtc/rtc-pm8xxx.c
> > +++ b/drivers/rtc/rtc-pm8xxx.c
> > @@ -3,6 +3,7 @@
> > */
> > #include <linux/of.h>
> > #include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > #include <linux/init.h>
> > #include <linux/rtc.h>
> > #include <linux/platform_device.h>
> > @@ -49,6 +50,8 @@ struct pm8xxx_rtc_regs {
> > * @alarm_irq: alarm irq number
> > * @regs: register description
> > * @dev: device structure
> > + * @nvmem_cell: nvmem cell for offset
> > + * @offset: offset from epoch in seconds
> > */
> > struct pm8xxx_rtc {
> > struct rtc_device *rtc;
> > @@ -57,8 +60,60 @@ struct pm8xxx_rtc {
> > int alarm_irq;
> > const struct pm8xxx_rtc_regs *regs;
> > struct device *dev;
> > + struct nvmem_cell *nvmem_cell;
> > + u32 offset;
> > };
> >
> > +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> > +{
> > + size_t len;
> > + void *buf;
> > + int rc;
> > +
> > + buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> > + if (IS_ERR(buf)) {
> > + rc = PTR_ERR(buf);
> > + dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
>
> You removed many dev_err strings in your previous patch and now this is
> verbose. Honestly, there is not much to do apart from reying the
> operation so I don't think the strings are worth it.
There's a difference. The SPMI ones are basically equivalent to mmio
reads, which we also don't expect to fail (and other spmi drivers also
ignore them).
These nvmem error paths I actually hit during development and it could
help someone trying to enable this feature on a new platform.
> > + return rc;
> > + }
> > +
> > + if (len != sizeof(u32)) {
> > + dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> > + kfree(buf);
> > + return -EINVAL;
> > + }
> > +
> > + rtc_dd->offset = get_unaligned_le32(buf);
> > +
> > + kfree(buf);
> > +
> > + return 0;
> > +}
> > @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
> > rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
> > "allow-set-time");
> >
> > + rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
>
> Maybe we should get something more specific than just "offset" so this
> could be parsed in the RTC core at some point (this is the second RTC to
> behave like this)
Yes, that thought crossed my mind, but it's an nvmem cell name (label)
and not a generic devicetree property. If you look at the binding
document I think the name makes sense given the current description, and
I'm not sure changing to something like 'base' would be much of an
improvement.
I also don't expect there to be more broken RTCs out there like these
ones. Hopefully Qualcomm will even get this fixed at some point
themselves.
And I assume you were think of the old Atmel driver which uses a timer
counter and a scratch register as a base? That one is also a bit
different in that the timer can be reset, just not set.
> > + if (IS_ERR(rtc_dd->nvmem_cell)) {
> > + rc = PTR_ERR(rtc_dd->nvmem_cell);
> > + if (rc != -ENOENT)
> > + return rc;
> > + rtc_dd->nvmem_cell = NULL;
> > + }
Johan
