| Message ID | 20230202155448.6715-18-johan+linaro@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: pm8xxx: add support for setting time using nvmem | expand |
On 2.02.2023 16:54, 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> > --- That's gonna be a stupid question, but just to make sure.. SDAM is rewritable, right? So that when somebody sets the time to year 2077 by mistake, they won't have to put up with it for the next 50 years? :D Konrad > drivers/rtc/rtc-pm8xxx.c | 141 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 129 insertions(+), 12 deletions(-) > > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c > index eff2782beeed..372494e82f40 100644 > --- a/drivers/rtc/rtc-pm8xxx.c > +++ b/drivers/rtc/rtc-pm8xxx.c > @@ -1,8 +1,13 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved. > +/* > + * pm8xxx RTC driver > + * > + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved. > + * Copyright (c) 2023, Linaro Limited > */ > #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 +54,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 +64,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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc); > + return rc; > + } > + > + if (len != sizeof(u32)) { > + dev_dbg(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; > +} > + > +static int pm8xxx_rtc_write_nvmem_offset(struct pm8xxx_rtc *rtc_dd, u32 offset) > +{ > + u8 buf[sizeof(u32)]; > + int rc; > + > + put_unaligned_le32(offset, buf); > + > + rc = nvmem_cell_write(rtc_dd->nvmem_cell, buf, sizeof(buf)); > + if (rc < 0) { > + dev_dbg(rtc_dd->dev, "failed to write nvmem offset: %d\n", rc); > + return rc; > + } > + > + return 0; > +} > + > +static int pm8xxx_rtc_read_offset(struct pm8xxx_rtc *rtc_dd) > +{ > + if (!rtc_dd->nvmem_cell) > + return 0; > + > + return pm8xxx_rtc_read_nvmem_offset(rtc_dd); > +} > + > static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs) > { > const struct pm8xxx_rtc_regs *regs = rtc_dd->regs; > @@ -90,6 +149,33 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs) > return 0; > } > > +static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs) > +{ > + u32 raw_secs; > + u32 offset; > + int rc; > + > + if (!rtc_dd->nvmem_cell) > + return -ENODEV; > + > + rc = pm8xxx_rtc_read_raw(rtc_dd, &raw_secs); > + if (rc) > + return rc; > + > + offset = secs - raw_secs; > + > + if (offset == rtc_dd->offset) > + return 0; > + > + rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset); > + if (rc) > + return rc; > + > + rtc_dd->offset = offset; > + > + return 0; > +} > + > /* > * Steps to write the RTC registers. > * 1. Disable alarm if enabled. > @@ -99,23 +185,15 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs) > * 5. Enable rtc if disabled in step 2. > * 6. Enable alarm if disabled in step 1. > */ > -static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) > +static int __pm8xxx_rtc_set_time(struct pm8xxx_rtc *rtc_dd, u32 secs) > { > - struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); > const struct pm8xxx_rtc_regs *regs = rtc_dd->regs; > u8 value[NUM_8_BIT_RTC_REGS]; > bool alarm_enabled; > - u32 secs; > int rc; > > - if (!rtc_dd->allow_set_time) > - return -ENODEV; > - > - secs = rtc_tm_to_time64(tm); > put_unaligned_le32(secs, value); > > - dev_dbg(dev, "set time: %ptRd %ptRt (%u)\n", tm, tm, secs); > - > rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl, > regs->alarm_en, 0, &alarm_enabled); > if (rc) > @@ -158,6 +236,27 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) > return 0; > } > > +static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); > + u32 secs; > + int rc; > + > + secs = rtc_tm_to_time64(tm); > + > + if (rtc_dd->allow_set_time) > + rc = __pm8xxx_rtc_set_time(rtc_dd, secs); > + else > + rc = pm8xxx_rtc_update_offset(rtc_dd, secs); > + > + if (rc) > + return rc; > + > + dev_dbg(dev, "set time: %ptRd %ptRt (%u + %u)\n", tm, tm, > + secs - rtc_dd->offset, rtc_dd->offset); > + return 0; > +} > + > static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm) > { > struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); > @@ -168,10 +267,11 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm) > if (rc) > return rc; > > + secs += rtc_dd->offset; > rtc_time64_to_tm(secs, tm); > > - dev_dbg(dev, "read time: %ptRd %ptRt (%u)\n", tm, tm, secs); > - > + dev_dbg(dev, "read time: %ptRd %ptRt (%u + %u)\n", tm, tm, > + secs - rtc_dd->offset, rtc_dd->offset); > return 0; > } > > @@ -184,6 +284,7 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm) > int rc; > > secs = rtc_tm_to_time64(&alarm->time); > + secs -= rtc_dd->offset; > put_unaligned_le32(secs, value); > > rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl, > @@ -223,6 +324,7 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm) > return rc; > > secs = get_unaligned_le32(value); > + secs += rtc_dd->offset; > rtc_time64_to_tm(secs, &alarm->time); > > rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg); > @@ -378,9 +480,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"); > + if (IS_ERR(rtc_dd->nvmem_cell)) { > + rc = PTR_ERR(rtc_dd->nvmem_cell); > + if (rc != -ENOENT) > + return rc; > + rtc_dd->nvmem_cell = NULL; > + } > + > rtc_dd->regs = match->data; > rtc_dd->dev = &pdev->dev; > > + if (!rtc_dd->allow_set_time) { > + rc = pm8xxx_rtc_read_offset(rtc_dd); > + if (rc) > + return rc; > + } > + > rc = pm8xxx_rtc_enable(rtc_dd); > if (rc) > return rc; > @@ -435,3 +551,4 @@ MODULE_ALIAS("platform:rtc-pm8xxx"); > MODULE_DESCRIPTION("PMIC8xxx RTC driver"); > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Anirudh Ghayal <aghayal@codeaurora.org>"); > +MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
On Fri, Feb 03, 2023 at 04:31:54AM +0100, Konrad Dybcio wrote: > > > On 2.02.2023 16:54, 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> > > --- > That's gonna be a stupid question, but just to make sure.. > > SDAM is rewritable, right? So that when somebody sets the time to > year 2077 by mistake, they won't have to put up with it for the next > 50 years? :D Heh, yes, it is re-writeable. Otherwise, using SDAM wouldn't really have been an alternative to using the UEFI offset. :) These registers are reset if you lose battery power so you'd be back at 1970 as expected if that ever happens too. Johan
On 2/2/23 07:54, 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 s/can not/cannot/ > 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 | 141 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 129 insertions(+), 12 deletions(-) > > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c > index eff2782beeed..372494e82f40 100644 > --- a/drivers/rtc/rtc-pm8xxx.c > +++ b/drivers/rtc/rtc-pm8xxx.c > @@ -1,8 +1,13 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved. > +/* > + * pm8xxx RTC driver > + * > + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved. > + * Copyright (c) 2023, Linaro Limited > */ > #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 +54,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 +64,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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc); Why is dev_dbg() used instead of dev_err() for newly added error messages? Also, why do these conditions warrant error logging when some of the previous patches in this series removed older error logging? Thanks, David
On Mon, Feb 06, 2023 at 07:16:54PM -0800, David Collins wrote: > On 2/2/23 07:54, Johan Hovold wrote: > > +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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc); > > Why is dev_dbg() used instead of dev_err() for newly added error > messages? Also, why do these conditions warrant error logging when some > of the previous patches in this series removed older error logging? I would have used dev_err() here and did so for v1, but Alexandre prefers dev_dbg() for errors that are unlikely to be seen by regular users but that can still be useful to developers (e.g. when enabling the rtc on a new platform). One or two of the spmi errors I removed falls in the same category in so far that the control and time registers may write-protected on some platforms, but such errors are currently logged by the spmi controller driver. Johan
diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c index eff2782beeed..372494e82f40 100644 --- a/drivers/rtc/rtc-pm8xxx.c +++ b/drivers/rtc/rtc-pm8xxx.c @@ -1,8 +1,13 @@ // SPDX-License-Identifier: GPL-2.0-only -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved. +/* + * pm8xxx RTC driver + * + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved. + * Copyright (c) 2023, Linaro Limited */ #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 +54,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 +64,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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc); + return rc; + } + + if (len != sizeof(u32)) { + dev_dbg(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; +} + +static int pm8xxx_rtc_write_nvmem_offset(struct pm8xxx_rtc *rtc_dd, u32 offset) +{ + u8 buf[sizeof(u32)]; + int rc; + + put_unaligned_le32(offset, buf); + + rc = nvmem_cell_write(rtc_dd->nvmem_cell, buf, sizeof(buf)); + if (rc < 0) { + dev_dbg(rtc_dd->dev, "failed to write nvmem offset: %d\n", rc); + return rc; + } + + return 0; +} + +static int pm8xxx_rtc_read_offset(struct pm8xxx_rtc *rtc_dd) +{ + if (!rtc_dd->nvmem_cell) + return 0; + + return pm8xxx_rtc_read_nvmem_offset(rtc_dd); +} + static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs) { const struct pm8xxx_rtc_regs *regs = rtc_dd->regs; @@ -90,6 +149,33 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs) return 0; } +static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs) +{ + u32 raw_secs; + u32 offset; + int rc; + + if (!rtc_dd->nvmem_cell) + return -ENODEV; + + rc = pm8xxx_rtc_read_raw(rtc_dd, &raw_secs); + if (rc) + return rc; + + offset = secs - raw_secs; + + if (offset == rtc_dd->offset) + return 0; + + rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset); + if (rc) + return rc; + + rtc_dd->offset = offset; + + return 0; +} + /* * Steps to write the RTC registers. * 1. Disable alarm if enabled. @@ -99,23 +185,15 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs) * 5. Enable rtc if disabled in step 2. * 6. Enable alarm if disabled in step 1. */ -static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) +static int __pm8xxx_rtc_set_time(struct pm8xxx_rtc *rtc_dd, u32 secs) { - struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); const struct pm8xxx_rtc_regs *regs = rtc_dd->regs; u8 value[NUM_8_BIT_RTC_REGS]; bool alarm_enabled; - u32 secs; int rc; - if (!rtc_dd->allow_set_time) - return -ENODEV; - - secs = rtc_tm_to_time64(tm); put_unaligned_le32(secs, value); - dev_dbg(dev, "set time: %ptRd %ptRt (%u)\n", tm, tm, secs); - rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl, regs->alarm_en, 0, &alarm_enabled); if (rc) @@ -158,6 +236,27 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) return 0; } +static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) +{ + struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); + u32 secs; + int rc; + + secs = rtc_tm_to_time64(tm); + + if (rtc_dd->allow_set_time) + rc = __pm8xxx_rtc_set_time(rtc_dd, secs); + else + rc = pm8xxx_rtc_update_offset(rtc_dd, secs); + + if (rc) + return rc; + + dev_dbg(dev, "set time: %ptRd %ptRt (%u + %u)\n", tm, tm, + secs - rtc_dd->offset, rtc_dd->offset); + return 0; +} + static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm) { struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); @@ -168,10 +267,11 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm) if (rc) return rc; + secs += rtc_dd->offset; rtc_time64_to_tm(secs, tm); - dev_dbg(dev, "read time: %ptRd %ptRt (%u)\n", tm, tm, secs); - + dev_dbg(dev, "read time: %ptRd %ptRt (%u + %u)\n", tm, tm, + secs - rtc_dd->offset, rtc_dd->offset); return 0; } @@ -184,6 +284,7 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm) int rc; secs = rtc_tm_to_time64(&alarm->time); + secs -= rtc_dd->offset; put_unaligned_le32(secs, value); rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl, @@ -223,6 +324,7 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm) return rc; secs = get_unaligned_le32(value); + secs += rtc_dd->offset; rtc_time64_to_tm(secs, &alarm->time); rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg); @@ -378,9 +480,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"); + if (IS_ERR(rtc_dd->nvmem_cell)) { + rc = PTR_ERR(rtc_dd->nvmem_cell); + if (rc != -ENOENT) + return rc; + rtc_dd->nvmem_cell = NULL; + } + rtc_dd->regs = match->data; rtc_dd->dev = &pdev->dev; + if (!rtc_dd->allow_set_time) { + rc = pm8xxx_rtc_read_offset(rtc_dd); + if (rc) + return rc; + } + rc = pm8xxx_rtc_enable(rtc_dd); if (rc) return rc; @@ -435,3 +551,4 @@ MODULE_ALIAS("platform:rtc-pm8xxx"); MODULE_DESCRIPTION("PMIC8xxx RTC driver"); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Anirudh Ghayal <aghayal@codeaurora.org>"); +MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
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 | 141 +++++++++++++++++++++++++++++++++++---- 1 file changed, 129 insertions(+), 12 deletions(-)