[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 1/5] rtc: pm8xxx: implement no-alarm flag for non-HLOS owned alarm

[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>
---
 drivers/rtc/rtc-pm8xxx.c | 43 +++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index c32fba550c8e0..0de55779b04f7 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -61,6 +61,7 @@ struct pm8xxx_rtc {
 	struct rtc_device *rtc;
 	struct regmap *regmap;
 	bool allow_set_time;
+	bool no_alarm;
 	int alarm_irq;
 	const struct pm8xxx_rtc_regs *regs;
 	struct device *dev;
@@ -473,9 +474,13 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	if (!rtc_dd->regmap)
 		return -ENXIO;
 
-	rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
-	if (rtc_dd->alarm_irq < 0)
-		return -ENXIO;
+	rtc_dd->no_alarm = of_property_read_bool(pdev->dev.of_node, "no-alarm");
+
+	if (!rtc_dd->no_alarm) {
+		rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
+		if (rtc_dd->alarm_irq < 0)
+			return -ENXIO;
+	}
 
 	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
 						      "allow-set-time");
@@ -503,7 +508,8 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rtc_dd);
 
-	device_init_wakeup(&pdev->dev, 1);
+	if (!rtc_dd->no_alarm)
+		device_init_wakeup(&pdev->dev, 1);
 
 	rtc_dd->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc_dd->rtc))
@@ -512,27 +518,36 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	rtc_dd->rtc->ops = &pm8xxx_rtc_ops;
 	rtc_dd->rtc->range_max = U32_MAX;
 
-	rc = devm_request_any_context_irq(&pdev->dev, rtc_dd->alarm_irq,
-					  pm8xxx_alarm_trigger,
-					  IRQF_TRIGGER_RISING,
-					  "pm8xxx_rtc_alarm", rtc_dd);
-	if (rc < 0)
-		return rc;
+	if (!rtc_dd->no_alarm) {
+		rc = devm_request_any_context_irq(&pdev->dev, rtc_dd->alarm_irq,
+						  pm8xxx_alarm_trigger,
+						  IRQF_TRIGGER_RISING,
+						  "pm8xxx_rtc_alarm", rtc_dd);
+		if (rc < 0)
+			return rc;
+	}
 
 	rc = devm_rtc_register_device(rtc_dd->rtc);
 	if (rc)
 		return rc;
 
-	rc = dev_pm_set_wake_irq(&pdev->dev, rtc_dd->alarm_irq);
-	if (rc)
-		return rc;
+	if (!rtc_dd->no_alarm) {
+		rc = dev_pm_set_wake_irq(&pdev->dev, rtc_dd->alarm_irq);
+		if (rc)
+			return rc;
+	} else {
+		clear_bit(RTC_FEATURE_ALARM, rtc_dd->rtc->features);
+	}
 
 	return 0;
 }
 
 static void pm8xxx_remove(struct platform_device *pdev)
 {
-	dev_pm_clear_wake_irq(&pdev->dev);
+	struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+	if (!rtc_dd->no_alarm)
+		dev_pm_clear_wake_irq(&pdev->dev);
 }
 
 static struct platform_driver pm8xxx_rtc_driver = {
-- 
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

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#