mbox series

[v2,00/22] rtc: pm8xxx: add support for setting time using nvmem

Message ID 20230202155448.6715-1-johan+linaro@kernel.org
Headers show
Series rtc: pm8xxx: add support for setting time using nvmem | expand

Message

Johan Hovold Feb. 2, 2023, 3:54 p.m. UTC 
This series adds support for setting the RTC time on Qualcomm platforms
where the PMIC RTC time registers are read-only by instead storing an
offset in some other non-volatile memory. This is used to enable the RTC
in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
laptop.

The RTCs in many Qualcomm devices are effectively broken due to the time
registers being read-only. Instead some other non-volatile memory can be
used to store and offset which a driver can take into account. On
machines like the X13s, the UEFI firmware (and Windows) use a UEFI
variable for storing such an offset, but not all Qualcomm systems use
UEFI.

The Qualcomm firmware also does not support any UEFI runtime services,
but Maximilian Luz recently posted a driver for talking to the secure
world directly through the SCM interface and this can be used to access
the UEFI variables:

	https://lore.kernel.org/all/20220723224949.1089973-1-luzmaximilian@gmail.com/

I was initially told that the PMICs in the X13s did not have any spare
battery-backed registers which could have been used to store an RTC
offset so there seemed to be no alternative to try using the UEFI
offset. In the processes however, I learnt that there are in fact some
registers in PMIC that could be used, at least on the SC8280XP CRD and
the X13s. 

This was especially fortunate as it turned out that the firmware on the
CRD I have does not allow updating the UEFI RTC offset even if this
works on the X13s (and on a second CRD with more recent firmware I
learned last week).

As the benefit of sharing the RTC offset with the UEFI firmware (and
Windows) is rather small (e.g. to make sure they never get out sync), I
instead opted for using the PMIC registers on both machines. This also
avoids relying on a fairly complex reverse-engineered firmware driver,
as well as potential issues like flash wear due to RTC drift. Let's keep
it simple.

But as there could be older Qualcomm UEFI machines out there where we
don't have any other non-volatile storage I included the UEFI patches
as an RFC in v1 of this series for reference. In case it turns out there
are systems out there were this could be used (e.g. the Lenovo Yoga C630
laptop) those two patches could be merged as well. An alternative could
be to see if Maximilian's work could be extended to access the time
services directly.

This series first fixes a few issues with the current Qualcomm PMIC RTC
driver before cleaning it up a bit so that support for setting the time
using an offset stored in an nvmem cell can be added.

The final patches enables the RTC on the SC8280XP CRD and X13s and can
be merged by Bjorn once the (non-UEFI) RTC patches are in.

Note that for the SDAM nvmem driver to be autoloaded when built as a
module, you also need this fix:

	https://lore.kernel.org/lkml/20230126133034.27491-1-johan+linaro@kernel.org/

Johan


Changes in v2
 - replace nvme operation dev_err() with dev_dbg() (Alexandre)
 - squash patch adding copyright entry with patch adding nvme support
   (Krzysztof)
 - drop last two dev_err() (new patch)
 - amend dts commit message as setting variables appears to work on some
   CRD
 - drop the two UEFI RFC patches for now due to one EFI dependency not
   yet in mainline and on-going binding discussion

Johan Hovold (22):
  rtc: pm8xxx: fix set-alarm race
  rtc: pm8xxx: drop spmi error messages
  rtc: pm8xxx: use regmap_update_bits()
  rtc: pm8xxx: drop bogus locking
  rtc: pm8xxx: return IRQ_NONE on errors
  rtc: pm8xxx: drop unused register defines
  rtc: pm8xxx: use unaligned le32 helpers
  rtc: pm8xxx: clean up time and alarm debugging
  rtc: pm8xxx: rename struct device pointer
  rtc: pm8xxx: rename alarm irq variable
  rtc: pm8xxx: clean up comments
  rtc: pm8xxx: use u32 for timestamps
  rtc: pm8xxx: refactor read_time()
  rtc: pm8xxx: clean up local declarations
  rtc: pm8xxx: drop error messages
  dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset
  rtc: pm8xxx: add support for nvmem offset
  arm64: defconfig: enable Qualcomm SDAM nvmem driver
  arm64: dts: qcom: sc8280xp-pmics: add pmk8280 rtc
  arm64: dts: qcom: sc8280xp-pmics: add pmk8280 sdam nvram
  arm64: dts: qcom: sc8280xp-crd: enable rtc
  arm64: dts: qcom: sc8280xp-x13s: enable rtc

 .../bindings/rtc/qcom-pm8xxx-rtc.yaml         |  12 +
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts     |  15 +
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |  15 +
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi  |  18 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/rtc/rtc-pm8xxx.c                      | 533 +++++++++---------
 6 files changed, 324 insertions(+), 270 deletions(-)

Comments

Bjorn Andersson Feb. 9, 2023, 4:22 a.m. UTC | #1 
On Thu, 2 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> This series adds support for setting the RTC time on Qualcomm platforms
> where the PMIC RTC time registers are read-only by instead storing an
> offset in some other non-volatile memory. This is used to enable the RTC
> in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
> laptop.
> 
> The RTCs in many Qualcomm devices are effectively broken due to the time
> registers being read-only. Instead some other non-volatile memory can be
> used to store and offset which a driver can take into account. On
> machines like the X13s, the UEFI firmware (and Windows) use a UEFI
> variable for storing such an offset, but not all Qualcomm systems use
> UEFI.
> 
> [...]

Applied, thanks!

[18/22] arm64: defconfig: enable Qualcomm SDAM nvmem driver
        commit: 480ba14b9a95641647a6561d5b246de661589514

Best regards,
Alexandre Belloni Feb. 9, 2023, 10:25 p.m. UTC | #2 
On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> This series adds support for setting the RTC time on Qualcomm platforms
> where the PMIC RTC time registers are read-only by instead storing an
> offset in some other non-volatile memory. This is used to enable the RTC
> in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
> laptop.
> 
> The RTCs in many Qualcomm devices are effectively broken due to the time
> registers being read-only. Instead some other non-volatile memory can be
> used to store and offset which a driver can take into account. On
> machines like the X13s, the UEFI firmware (and Windows) use a UEFI
> variable for storing such an offset, but not all Qualcomm systems use
> UEFI.
> 
> [...]

Applied, thanks!

[01/22] rtc: pm8xxx: fix set-alarm race
        commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6
[02/22] rtc: pm8xxx: drop spmi error messages
        commit: eb245631836b4843199d7176d1597759dda4ee9e
[03/22] rtc: pm8xxx: use regmap_update_bits()
        commit: 182c23bbfea3713206b0da3fbbb7350e197a92dd
[04/22] rtc: pm8xxx: drop bogus locking
        commit: 8d273f33fd090a2c270c67b6ac7fa03f5a7eee3f
[05/22] rtc: pm8xxx: return IRQ_NONE on errors
        commit: cb9bb7b2364bb5f4f51226ce1f9ec6ffda618f0a
[06/22] rtc: pm8xxx: drop unused register defines
        commit: f081b74c1c748a7da972c782c2f974f239a9b51f
[07/22] rtc: pm8xxx: use unaligned le32 helpers
        commit: 79dd75661e4284169768859012a4bf6898cef758
[08/22] rtc: pm8xxx: clean up time and alarm debugging
        commit: c996956fcc5b7756eb04615cc36618acaa85caa9
[09/22] rtc: pm8xxx: rename struct device pointer
        commit: a375510efeda0dfbad205cd1de8b57f63d0779c9
[10/22] rtc: pm8xxx: rename alarm irq variable
        commit: 4727b58fc84daf6d7097ac3528a6517456a5e110
[11/22] rtc: pm8xxx: clean up comments
        commit: 3c3326394ba420608d0665aef846b2268c9c9629
[12/22] rtc: pm8xxx: use u32 for timestamps
        commit: 35d9c472925748a1cb1f5b6cc8ae71cf8138e30f
[13/22] rtc: pm8xxx: refactor read_time()
        commit: da862c3df6add928e2f51d6cadec128a9a1940f3
[14/22] rtc: pm8xxx: clean up local declarations
        commit: 9e5a799138042ac8276e6744c548b0411f371600
[15/22] rtc: pm8xxx: drop error messages
        commit: c94fb939e65155bc889e62396f83ef4317d643ac

Best regards,
Johan Hovold Feb. 10, 2023, 7:53 a.m. UTC | #3 
On Thu, Feb 09, 2023 at 11:25:34PM +0100, Alexandre Belloni wrote:
> 
> On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> > This series adds support for setting the RTC time on Qualcomm platforms
> > where the PMIC RTC time registers are read-only by instead storing an
> > offset in some other non-volatile memory. This is used to enable the RTC
> > in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
> > laptop.
> > 
> > The RTCs in many Qualcomm devices are effectively broken due to the time
> > registers being read-only. Instead some other non-volatile memory can be
> > used to store and offset which a driver can take into account. On
> > machines like the X13s, the UEFI firmware (and Windows) use a UEFI
> > variable for storing such an offset, but not all Qualcomm systems use
> > UEFI.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [01/22] rtc: pm8xxx: fix set-alarm race
>         commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6

...

> [15/22] rtc: pm8xxx: drop error messages
>         commit: c94fb939e65155bc889e62396f83ef4317d643ac

I noticed that you did not apply patches 16 and 17 that add support for
the nvmem offset. Was that on purpose or a mistake?

Johan
Alexandre Belloni Feb. 10, 2023, 9:04 a.m. UTC | #4 
On 10/02/2023 08:53:52+0100, Johan Hovold wrote:
> On Thu, Feb 09, 2023 at 11:25:34PM +0100, Alexandre Belloni wrote:
> > 
> > On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> > > This series adds support for setting the RTC time on Qualcomm platforms
> > > where the PMIC RTC time registers are read-only by instead storing an
> > > offset in some other non-volatile memory. This is used to enable the RTC
> > > in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
> > > laptop.
> > > 
> > > The RTCs in many Qualcomm devices are effectively broken due to the time
> > > registers being read-only. Instead some other non-volatile memory can be
> > > used to store and offset which a driver can take into account. On
> > > machines like the X13s, the UEFI firmware (and Windows) use a UEFI
> > > variable for storing such an offset, but not all Qualcomm systems use
> > > UEFI.
> > > 
> > > [...]
> > 
> > Applied, thanks!
> > 
> > [01/22] rtc: pm8xxx: fix set-alarm race
> >         commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6
> 
> ...
> 
> > [15/22] rtc: pm8xxx: drop error messages
> >         commit: c94fb939e65155bc889e62396f83ef4317d643ac
> 
> I noticed that you did not apply patches 16 and 17 that add support for
> the nvmem offset. Was that on purpose or a mistake?

This was on purpose, I'll handle them tonight.
Johan Hovold Feb. 10, 2023, 9:14 a.m. UTC | #5 
On Fri, Feb 10, 2023 at 10:04:03AM +0100, Alexandre Belloni wrote:
> On 10/02/2023 08:53:52+0100, Johan Hovold wrote:
> > On Thu, Feb 09, 2023 at 11:25:34PM +0100, Alexandre Belloni wrote:
> > > 
> > > On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> > > > This series adds support for setting the RTC time on Qualcomm platforms
> > > > where the PMIC RTC time registers are read-only by instead storing an
> > > > offset in some other non-volatile memory. This is used to enable the RTC
> > > > in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
> > > > laptop.
> > > > 
> > > > The RTCs in many Qualcomm devices are effectively broken due to the time
> > > > registers being read-only. Instead some other non-volatile memory can be
> > > > used to store and offset which a driver can take into account. On
> > > > machines like the X13s, the UEFI firmware (and Windows) use a UEFI
> > > > variable for storing such an offset, but not all Qualcomm systems use
> > > > UEFI.
> > > > 
> > > > [...]
> > > 
> > > Applied, thanks!
> > > 
> > > [01/22] rtc: pm8xxx: fix set-alarm race
> > >         commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6
> > 
> > ...
> > 
> > > [15/22] rtc: pm8xxx: drop error messages
> > >         commit: c94fb939e65155bc889e62396f83ef4317d643ac
> > 
> > I noticed that you did not apply patches 16 and 17 that add support for
> > the nvmem offset. Was that on purpose or a mistake?
> 
> This was on purpose, I'll handle them tonight.

Ok, thanks.

Johan