| Message ID | 20230126142057.25715-19-johan+linaro@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | rtc: pm8xxx: add support for setting time using nvmem | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Thu, Jan 26, 2023 at 03:20:51PM +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 a > driver can take into account. > > Add a 'qcom,uefi-rtc-info' boolean flag which indicates that the RTC > offset is stored in a Qualcomm specific UEFI variable so that the RTC > time can be updated on such platforms. > > The UEFI variable is > > 882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo > > and holds a 12-byte structure where the first four bytes is a GPS time > offset in little-endian byte order. Can't you just try to read the UEFI variable and use it if that succeeds? I don't like this in DT because what if lots of devices start storing lots of things in vendor specific UEFI variables. It doesn't scale. Rob
[ +CC: Ard ] On Mon, Jan 30, 2023 at 12:49:44PM -0600, Rob Herring wrote: > On Thu, Jan 26, 2023 at 03:20:51PM +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 a > > driver can take into account. > > > > Add a 'qcom,uefi-rtc-info' boolean flag which indicates that the RTC > > offset is stored in a Qualcomm specific UEFI variable so that the RTC > > time can be updated on such platforms. > > > > The UEFI variable is > > > > 882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo > > > > and holds a 12-byte structure where the first four bytes is a GPS time > > offset in little-endian byte order. > > Can't you just try to read the UEFI variable and use it if that > succeeds? Generally, yes. The problem here is that this UEFI variable is not used on all devices using these PMICs and I need a way to determine whether to wait for the UEFI variables to become available or not (e.g. when efivars support is built as module, yes, that's a thing now...). > I don't like this in DT because what if lots of devices start storing > lots of things in vendor specific UEFI variables. It doesn't scale. I hope we won't see that even if we already have some devices for x86 platforms storing MAC addresses and such in UEFI variables. They currently access the UEFI firmware directly (i.e. not using the efivars abstraction) and simply assume UEFI is always there. With the Google SMI efivars implementation or the new Qualcomm SMC-based one, we need a way to determine whether to wait for efivars to become registered. For drivers where efivars is always needed we can just probe defer, but in this case we should not wait unless the DT indicates that the RTC offset is stored in UEFI on this particular machine. Just as the nvmem-cell property indicates that the offset is stored in some abstract nvmem, it seems reasonable to describe the offset being stored in UEFI when that is the case (even if it is indeed generally possible to probe for the latter). An alternative might be to describe the efivars fw dependency in DT too (e.g. for device links), but I believe you have already expressed some concerns over that: https://lore.kernel.org/lkml/20230130210530.GA3339716-robh@kernel.org/ Johan
On Wed, 1 Feb 2023 at 17:09, Johan Hovold <johan@kernel.org> wrote: > > [ +CC: Ard ] > > On Mon, Jan 30, 2023 at 12:49:44PM -0600, Rob Herring wrote: > > On Thu, Jan 26, 2023 at 03:20:51PM +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 a > > > driver can take into account. > > > > > > Add a 'qcom,uefi-rtc-info' boolean flag which indicates that the RTC > > > offset is stored in a Qualcomm specific UEFI variable so that the RTC > > > time can be updated on such platforms. > > > > > > The UEFI variable is > > > > > > 882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo > > > > > > and holds a 12-byte structure where the first four bytes is a GPS time > > > offset in little-endian byte order. > > > > Can't you just try to read the UEFI variable and use it if that > > succeeds? > > Generally, yes. The problem here is that this UEFI variable is not used > on all devices using these PMICs and I need a way to determine whether > to wait for the UEFI variables to become available or not (e.g. when > efivars support is built as module, yes, that's a thing now...). > Could we read this variable at boot and pass it to the kernel in a different way? That way, we only have to defer the ability to set the RTC, right? > > I don't like this in DT because what if lots of devices start storing > > lots of things in vendor specific UEFI variables. It doesn't scale. > > I hope we won't see that even if we already have some devices for x86 > platforms storing MAC addresses and such in UEFI variables. They > currently access the UEFI firmware directly (i.e. not using the efivars > abstraction) and simply assume UEFI is always there. > > With the Google SMI efivars implementation or the new Qualcomm SMC-based > one, we need a way to determine whether to wait for efivars to become > registered. For drivers where efivars is always needed we can just probe > defer, but in this case we should not wait unless the DT indicates that > the RTC offset is stored in UEFI on this particular machine. > > Just as the nvmem-cell property indicates that the offset is stored in > some abstract nvmem, it seems reasonable to describe the offset being > stored in UEFI when that is the case (even if it is indeed generally > possible to probe for the latter). > > An alternative might be to describe the efivars fw dependency in DT too > (e.g. for device links), but I believe you have already expressed some > concerns over that: > > https://lore.kernel.org/lkml/20230130210530.GA3339716-robh@kernel.org/ > > Johan
diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml index b95a69cc9ae0..774c34c3f8f6 100644 --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml @@ -50,6 +50,12 @@ properties: items: - const: offset + qcom,uefi-rtc-info: + type: boolean + description: + RTC offset is stored as a four-byte GPS time offset in a 12-byte UEFI + variable 882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo + wakeup-source: true required:
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 a driver can take into account. Add a 'qcom,uefi-rtc-info' boolean flag which indicates that the RTC offset is stored in a Qualcomm specific UEFI variable so that the RTC time can be updated on such platforms. The UEFI variable is 882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo and holds a 12-byte structure where the first four bytes is a GPS time offset in little-endian byte order. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 6 ++++++ 1 file changed, 6 insertions(+)