Date:   Fri, 27 Jan 2023 16:51:27 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc:     Johan Hovold <johan+linaro@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Alessandro Zummo <a.zummo@towertech.it>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        linux-arm-msm@vger.kernel.org, linux-rtc@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset
Message-ID: <Y9Py/+GpI8x8ldDG@hovoldconsulting.com>
References: <20230126142057.25715-1-johan+linaro@kernel.org>
 <20230126142057.25715-17-johan+linaro@kernel.org>
 <Y9PpQkW3Rtm+bi2V@mail.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9PpQkW3Rtm+bi2V@mail.local>
Precedence: bulk

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@kernel.org>
> > ---
> >  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