[PATCH v2 0/5] x1e80100 RTC support

[PATCH v2 0/5] x1e80100 RTC support

[Jonathan Marek]

x1e80100 needs a workaround because the RTC alarm is not owned by HLOS.
It also needs the same offset workaround as sc8280xp/etc.

v2: remove duplicated ops and use RTC_FEATURE_ALARM instead

Jonathan Marek (5):
  rtc: pm8xxx: implement no-alarm flag for non-HLOS owned alarm
  dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag
  arm64: dts: qcom: x1e80100-pmics: enable RTC
  arm64: dts: qcom: x1e80100-crd: add rtc offset to set rtc time
  arm64: dts: qcom: x1e78100-t14s: add rtc offset to set rtc time

 .../bindings/rtc/qcom-pm8xxx-rtc.yaml         |  5 +++
 .../qcom/x1e78100-lenovo-thinkpad-t14s.dts    | 11 +++++
 arch/arm64/boot/dts/qcom/x1e80100-crd.dts     | 11 +++++
 arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi  |  8 ++--
 drivers/rtc/rtc-pm8xxx.c                      | 43 +++++++++++++------
 5 files changed, 59 insertions(+), 19 deletions(-)

-- 
2.45.1

[PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Jonathan Marek]

Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
Thus writing to RTC alarm registers and receiving alarm interrupts is not
possible.

Add a no-alarm flag to support RTC on this platform.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
index d274bb7a534b5..210f76a819e90 100644
--- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
@@ -40,6 +40,11 @@ properties:
     description:
       Indicates that the setting of RTC time is allowed by the host CPU.
 
+  no-alarm:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates that RTC alarm is not owned by HLOS (Linux).
+
   nvmem-cells:
     items:
       - description:
-- 
2.45.1

Re: [PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Krzysztof Kozlowski]

On Sun, Oct 13, 2024 at 01:15:27AM -0400, Jonathan Marek wrote:
> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
> Thus writing to RTC alarm registers and receiving alarm interrupts is not
> possible.
> 
> Add a no-alarm flag to support RTC on this platform.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
> index d274bb7a534b5..210f76a819e90 100644
> --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
> @@ -40,6 +40,11 @@ properties:
>      description:
>        Indicates that the setting of RTC time is allowed by the host CPU.
>  
> +  no-alarm:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that RTC alarm is not owned by HLOS (Linux).

This is not even properly used/tested, because you disable the RTC
entirely in your DTS.

I expect here unified property for all Qualcomm devices for this case.
We already have "remotely-controlled" and other flavors. I don't want
each device to express the same with different name...

Also: missing vendor prefix.

Best regards,
Krzysztof

Re: [PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Jonathan Marek]

On 10/14/24 3:34 AM, Krzysztof Kozlowski wrote:
> On Sun, Oct 13, 2024 at 01:15:27AM -0400, Jonathan Marek wrote:
>> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
>> Thus writing to RTC alarm registers and receiving alarm interrupts is not
>> possible.
>>
>> Add a no-alarm flag to support RTC on this platform.
>>
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>> index d274bb7a534b5..210f76a819e90 100644
>> --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>> @@ -40,6 +40,11 @@ properties:
>>       description:
>>         Indicates that the setting of RTC time is allowed by the host CPU.
>>   
>> +  no-alarm:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates that RTC alarm is not owned by HLOS (Linux).
> 
> This is not even properly used/tested, because you disable the RTC
> entirely in your DTS.
> 

What? The next patch in this series is enabling RTC on x1e using this flag

> I expect here unified property for all Qualcomm devices for this case.
> We already have "remotely-controlled" and other flavors. I don't want
> each device to express the same with different name...
> 
> Also: missing vendor prefix.
> 

I don't care what the property is named (as long as its a bool 
property), if you have a name you prefer I will use it.

The existing 'allow-set-time' property (also related to HLOS permissions 
to the RTC) is also specific to this driver doesn't have a vendor prefix.

> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Krzysztof Kozlowski]

On 14/10/2024 14:58, Jonathan Marek wrote:
> On 10/14/24 3:34 AM, Krzysztof Kozlowski wrote:
>> On Sun, Oct 13, 2024 at 01:15:27AM -0400, Jonathan Marek wrote:
>>> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
>>> Thus writing to RTC alarm registers and receiving alarm interrupts is not
>>> possible.
>>>
>>> Add a no-alarm flag to support RTC on this platform.
>>>
>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>> ---
>>>   Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>> index d274bb7a534b5..210f76a819e90 100644
>>> --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>> +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>> @@ -40,6 +40,11 @@ properties:
>>>       description:
>>>         Indicates that the setting of RTC time is allowed by the host CPU.
>>>   
>>> +  no-alarm:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description:
>>> +      Indicates that RTC alarm is not owned by HLOS (Linux).
>>
>> This is not even properly used/tested, because you disable the RTC
>> entirely in your DTS.
>>
> 
> What? The next patch in this series is enabling RTC on x1e using this flag

D'oh, right, I must have looked at wrong diff hunks. I had somehow
impression you add status=reserved, but you just dropped it.

> 
>> I expect here unified property for all Qualcomm devices for this case.
>> We already have "remotely-controlled" and other flavors. I don't want
>> each device to express the same with different name...
>>
>> Also: missing vendor prefix.
>>
> 
> I don't care what the property is named (as long as its a bool 
> property), if you have a name you prefer I will use it.
> 
> The existing 'allow-set-time' property (also related to HLOS permissions 
> to the RTC) is also specific to this driver doesn't have a vendor prefix.

Yeah, that one sneaked in some years ago.

So you can set time, but not alarm? Some previous platforms could not
set time, but could set alarm?

I wonder whether we actually describe the real issue here. It looks like
group of band-aids.

Best regards,
Krzysztof

Re: [PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Jonathan Marek]

On 10/14/24 9:38 AM, Krzysztof Kozlowski wrote:
> On 14/10/2024 14:58, Jonathan Marek wrote:
>> On 10/14/24 3:34 AM, Krzysztof Kozlowski wrote:
>>> On Sun, Oct 13, 2024 at 01:15:27AM -0400, Jonathan Marek wrote:
>>>> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
>>>> Thus writing to RTC alarm registers and receiving alarm interrupts is not
>>>> possible.
>>>>
>>>> Add a no-alarm flag to support RTC on this platform.
>>>>
>>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>>> ---
>>>>    Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>> index d274bb7a534b5..210f76a819e90 100644
>>>> --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>> +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>> @@ -40,6 +40,11 @@ properties:
>>>>        description:
>>>>          Indicates that the setting of RTC time is allowed by the host CPU.
>>>>    
>>>> +  no-alarm:
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +    description:
>>>> +      Indicates that RTC alarm is not owned by HLOS (Linux).
>>>
>>> This is not even properly used/tested, because you disable the RTC
>>> entirely in your DTS.
>>>
>>
>> What? The next patch in this series is enabling RTC on x1e using this flag
> 
> D'oh, right, I must have looked at wrong diff hunks. I had somehow
> impression you add status=reserved, but you just dropped it.
> 
>>
>>> I expect here unified property for all Qualcomm devices for this case.
>>> We already have "remotely-controlled" and other flavors. I don't want
>>> each device to express the same with different name...
>>>
>>> Also: missing vendor prefix.
>>>
>>
>> I don't care what the property is named (as long as its a bool
>> property), if you have a name you prefer I will use it.
>>
>> The existing 'allow-set-time' property (also related to HLOS permissions
>> to the RTC) is also specific to this driver doesn't have a vendor prefix.
> 
> Yeah, that one sneaked in some years ago.
> 
> So you can set time, but not alarm? Some previous platforms could not
> set time, but could set alarm?
> 
> I wonder whether we actually describe the real issue here. It looks like
> group of band-aids.
> 
> Best regards,
> Krzysztof
> 

Firmware can set different permissions for the RTC time (0x61xx) and RTC 
alarm (0x62xx) regions. So it makes sense to have one flag for each region.

RTC time is almost always read-only (not owned by HLOS/Linux), so the 
'allow-set-time' property is almost never used (the driver supports 
using nvmem to store an offset for setting time as a workaround).

The "can set time, but not alarm" combination will probably never be 
used, but the 3 other combinations are possible (the common one is 
"can't set time, but can set alarm").

(in the next patch I deleted the "alarm" region/interrupt from the dts 
but that's wrong, the HW still exists, the patch should be only 
replacing the reserved status with the new flag)

Re: [PATCH v2 2/5] dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag

[Krzysztof Kozlowski]

On 14/10/2024 16:09, Jonathan Marek wrote:
> On 10/14/24 9:38 AM, Krzysztof Kozlowski wrote:
>> On 14/10/2024 14:58, Jonathan Marek wrote:
>>> On 10/14/24 3:34 AM, Krzysztof Kozlowski wrote:
>>>> On Sun, Oct 13, 2024 at 01:15:27AM -0400, Jonathan Marek wrote:
>>>>> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
>>>>> Thus writing to RTC alarm registers and receiving alarm interrupts is not
>>>>> possible.
>>>>>
>>>>> Add a no-alarm flag to support RTC on this platform.
>>>>>
>>>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>>> index d274bb7a534b5..210f76a819e90 100644
>>>>> --- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
>>>>> @@ -40,6 +40,11 @@ properties:
>>>>>        description:
>>>>>          Indicates that the setting of RTC time is allowed by the host CPU.
>>>>>    
>>>>> +  no-alarm:
>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>> +    description:
>>>>> +      Indicates that RTC alarm is not owned by HLOS (Linux).
>>>>
>>>> This is not even properly used/tested, because you disable the RTC
>>>> entirely in your DTS.
>>>>
>>>
>>> What? The next patch in this series is enabling RTC on x1e using this flag
>>
>> D'oh, right, I must have looked at wrong diff hunks. I had somehow
>> impression you add status=reserved, but you just dropped it.
>>
>>>
>>>> I expect here unified property for all Qualcomm devices for this case.
>>>> We already have "remotely-controlled" and other flavors. I don't want
>>>> each device to express the same with different name...
>>>>
>>>> Also: missing vendor prefix.
>>>>
>>>
>>> I don't care what the property is named (as long as its a bool
>>> property), if you have a name you prefer I will use it.
>>>
>>> The existing 'allow-set-time' property (also related to HLOS permissions
>>> to the RTC) is also specific to this driver doesn't have a vendor prefix.
>>
>> Yeah, that one sneaked in some years ago.
>>
>> So you can set time, but not alarm? Some previous platforms could not
>> set time, but could set alarm?
>>
>> I wonder whether we actually describe the real issue here. It looks like
>> group of band-aids.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Firmware can set different permissions for the RTC time (0x61xx) and RTC 
> alarm (0x62xx) regions. So it makes sense to have one flag for each region.
> 
> RTC time is almost always read-only (not owned by HLOS/Linux), so the 
> 'allow-set-time' property is almost never used (the driver supports 
> using nvmem to store an offset for setting time as a workaround).
> 
> The "can set time, but not alarm" combination will probably never be 
> used, but the 3 other combinations are possible (the common one is 
> "can't set time, but can set alarm").
> 
> (in the next patch I deleted the "alarm" region/interrupt from the dts 
> but that's wrong, the HW still exists, the patch should be only 
> replacing the reserved status with the new flag)

OK, let's just add vendor prefix and describe actual hardware property,
e.g. qcom,no-alarm or qcom,alarm-restricted

Best regards,
Krzysztof

[PATCH v2 3/5] arm64: dts: qcom: x1e80100-pmics: enable RTC

[Jonathan Marek]

Only access to RTC alarm is blocked.

Enable RTC access by setting the no-alarm flag.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi b/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
index 5b54ee79f048e..304a98d60472e 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
@@ -220,11 +220,9 @@ pon_resin: resin {
 
 		pmk8550_rtc: rtc@6100 {
 			compatible = "qcom,pmk8350-rtc";
-			reg = <0x6100>, <0x6200>;
-			reg-names = "rtc", "alarm";
-			interrupts = <0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
-			/* Not yet sure what blocks access */
-			status = "reserved";
+			reg = <0x6100>;
+			reg-names = "rtc";
+			no-alarm; /* alarm owned by ADSP */
 		};
 
 		pmk8550_sdam_2: nvram@7100 {
-- 
2.45.1

[PATCH v2 4/5] arm64: dts: qcom: x1e80100-crd: add rtc offset to set rtc time

[Jonathan Marek]

See commit e67b45582c5e for explanation.

Note: the 0xbc offset is arbitrary, it just needs to not be already in use.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
index 6dfc85eda3540..eb6b735c41453 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
@@ -1224,6 +1224,17 @@ edp_bl_en: edp-bl-en-state {
 	};
 };
 
+&pmk8550_rtc {
+	nvmem-cells = <&rtc_offset>;
+	nvmem-cell-names = "offset";
+};
+
+&pmk8550_sdam_2 {
+	rtc_offset: rtc-offset@bc {
+		reg = <0xbc 0x4>;
+	};
+};
+
 &qupv3_0 {
 	status = "okay";
 };
-- 
2.45.1

[PATCH v2 5/5] arm64: dts: qcom: x1e78100-t14s: add rtc offset to set rtc time

[Jonathan Marek]

See commit e67b45582c5e for explanation.

Note: the 0xbc offset is arbitrary, it just needs to not be already in use.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 .../boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts   | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts
index d18461c545547..f05523cb51cd4 100644
--- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts
+++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts
@@ -897,6 +897,17 @@ edp_bl_en: edp-bl-en-state {
 	};
 };
 
+&pmk8550_rtc {
+	nvmem-cells = <&rtc_offset>;
+	nvmem-cell-names = "offset";
+};
+
+&pmk8550_sdam_2 {
+	rtc_offset: rtc-offset@bc {
+		reg = <0xbc 0x4>;
+	};
+};
+
 &qupv3_0 {
 	status = "okay";
 };
-- 
2.45.1

Re: [PATCH v2 0/5] x1e80100 RTC support

[Rob Herring (Arm)]

On Sun, 13 Oct 2024 01:15:25 -0400, Jonathan Marek wrote:
> x1e80100 needs a workaround because the RTC alarm is not owned by HLOS.
> It also needs the same offset workaround as sc8280xp/etc.
> 
> v2: remove duplicated ops and use RTC_FEATURE_ALARM instead
> 
> Jonathan Marek (5):
>   rtc: pm8xxx: implement no-alarm flag for non-HLOS owned alarm
>   dt-bindings: rtc: qcom-pm8xxx: document no-alarm flag
>   arm64: dts: qcom: x1e80100-pmics: enable RTC
>   arm64: dts: qcom: x1e80100-crd: add rtc offset to set rtc time
>   arm64: dts: qcom: x1e78100-t14s: add rtc offset to set rtc time
> 
>  .../bindings/rtc/qcom-pm8xxx-rtc.yaml         |  5 +++
>  .../qcom/x1e78100-lenovo-thinkpad-t14s.dts    | 11 +++++
>  arch/arm64/boot/dts/qcom/x1e80100-crd.dts     | 11 +++++
>  arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi  |  8 ++--
>  drivers/rtc/rtc-pm8xxx.c                      | 43 +++++++++++++------
>  5 files changed, 59 insertions(+), 19 deletions(-)
> 
> --
> 2.45.1
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y qcom/x1e78100-lenovo-thinkpad-t14s.dtb qcom/x1e80100-crd.dtb' for 20241013051859.22800-1-jonathan@marek.ca:

arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtb: pmic@0: rtc@6100: 'oneOf' conditional failed, one must be fixed:
	'interrupts' is a required property
	'interrupts-extended' is a required property
	from schema $id: http://devicetree.org/schemas/mfd/qcom,spmi-pmic.yaml#
arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtb: rtc@6100: 'oneOf' conditional failed, one must be fixed:
	'interrupts' is a required property
	'interrupts-extended' is a required property
	from schema $id: http://devicetree.org/schemas/rtc/qcom-pm8xxx-rtc.yaml#
arch/arm64/boot/dts/qcom/x1e80100-crd.dtb: pmic@0: rtc@6100: 'oneOf' conditional failed, one must be fixed:
	'interrupts' is a required property
	'interrupts-extended' is a required property
	from schema $id: http://devicetree.org/schemas/mfd/qcom,spmi-pmic.yaml#
arch/arm64/boot/dts/qcom/x1e80100-crd.dtb: rtc@6100: 'oneOf' conditional failed, one must be fixed:
	'interrupts' is a required property
	'interrupts-extended' is a required property
	from schema $id: http://devicetree.org/schemas/rtc/qcom-pm8xxx-rtc.yaml#