[PATCH v3 0/5] x1e80100 RTC support

[PATCH v3 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
v3:
 - renamed flag to qcom,no-alarm
 - don't remove alarm registers/interrupt from dts

Jonathan Marek (5):
  rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
  dt-bindings: rtc: qcom-pm8xxx: document qcom,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  |  3 +-
 drivers/rtc/rtc-pm8xxx.c                      | 44 +++++++++++++------
 5 files changed, 58 insertions(+), 16 deletions(-)

-- 
2.45.1

[PATCH v3 1/5] rtc: pm8xxx: implement qcom,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 qcom,no-alarm flag to support RTC on this platform.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index c32fba550c8e0..1e78939625622 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,14 @@ 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,
+						 "qcom,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 +509,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 +519,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 v3 2/5] dt-bindings: rtc: qcom-pm8xxx: document qcom,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 qcom,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..23a5316efadba 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.
 
+  qcom,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 v3 2/5] dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag

[Krzysztof Kozlowski]

On 15/10/2024 02:47, 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 qcom,no-alarm flag to support RTC on this platform.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Bjorn Andersson]

On Mon, Oct 14, 2024 at 08:47:26PM -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 qcom,no-alarm flag to support RTC on this platform.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Alexandre, please pick up the driver and dt-binding patch (i.e. patch 1
& 2) through your tree, and I can pick the dts patches through the qcom
tree.

Regards,
Bjorn

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index c32fba550c8e0..1e78939625622 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,14 @@ 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,
> +						 "qcom,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 +509,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 +519,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
> 

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Johan Hovold]

On Mon, Oct 14, 2024 at 08:47:26PM -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 qcom,no-alarm flag to support RTC on this platform.

An alternative may be to drop the alarm interrupt from DT and use that
as an indicator.

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index c32fba550c8e0..1e78939625622 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;

How about inverting this one and naming it has_alarm or similar to avoid
the double negation in your conditionals (!no_alarm)?

>  	int alarm_irq;
>  	const struct pm8xxx_rtc_regs *regs;
>  	struct device *dev;
> @@ -473,9 +474,14 @@ 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,
> +						 "qcom,no-alarm");
> +

Stray newline.

> +	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 +509,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 +519,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);

I assume that you should be clearing the feature bit before registering
the RTC.

> +	}
>  
>  	return 0;
>  }

Johan

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

[Johan Hovold]

On Mon, Oct 14, 2024 at 08:47:27PM -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 qcom,no-alarm flag to support RTC on this platform.

Could dropping the alarm interrupt be an alternative to another vendor
property? Some existing RTC drivers use this.

It also seems like having a dedicated compatible for this platform is
warranted, and would also suffice to determine when the alarm registers
are writable.

Note that the binding update should go before the driver patch in the
series.

Johan

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Alexandre Belloni]

On 16/10/2024 08:42:46+0200, Johan Hovold wrote:
> On Mon, Oct 14, 2024 at 08:47:26PM -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 qcom,no-alarm flag to support RTC on this platform.
> 
> An alternative may be to drop the alarm interrupt from DT and use that
> as an indicator.
> 
> > Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> > ---
> >  drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++-------------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> > index c32fba550c8e0..1e78939625622 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;
> 
> How about inverting this one and naming it has_alarm or similar to avoid
> the double negation in your conditionals (!no_alarm)?
> 
> >  	int alarm_irq;
> >  	const struct pm8xxx_rtc_regs *regs;
> >  	struct device *dev;
> > @@ -473,9 +474,14 @@ 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,
> > +						 "qcom,no-alarm");
> > +
> 
> Stray newline.
> 
> > +	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 +509,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 +519,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;

Also, probe must not fail after devm_rtc_allocate_device has been
called.so you could fix this with this patch.

> > +	} else {
> > +		clear_bit(RTC_FEATURE_ALARM, rtc_dd->rtc->features);
> 
> I assume that you should be clearing the feature bit before registering
> the RTC.
> 
> > +	}
> >  
> >  	return 0;
> >  }
> 
> Johan

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Jonathan Marek]

On 10/16/24 2:42 AM, Johan Hovold wrote:
> On Mon, Oct 14, 2024 at 08:47:26PM -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 qcom,no-alarm flag to support RTC on this platform.
> 
> An alternative may be to drop the alarm interrupt from DT and use that
> as an indicator.
> 

That wouldn't be right, the registers/interrupt still exist and should 
be described in DT.

(if you have firmware that allows access to the alarm, now you only have 
to delete the qcom,no-alarm property in your dts to use it)

>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++-------------
>>   1 file changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
>> index c32fba550c8e0..1e78939625622 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;
> 
> How about inverting this one and naming it has_alarm or similar to avoid
> the double negation in your conditionals (!no_alarm)?
> 

My reasoning is that the DT flag has to be negative, and its better to 
use the same name as the DT flag, but inverting it is OK.

>>   	int alarm_irq;
>>   	const struct pm8xxx_rtc_regs *regs;
>>   	struct device *dev;
>> @@ -473,9 +474,14 @@ 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,
>> +						 "qcom,no-alarm");
>> +
> 
> Stray newline.
> 

That's not a stray newline?

>> +	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 +509,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 +519,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);
> 
> I assume that you should be clearing the feature bit before registering
> the RTC.
> 

Right, it just needs to be after devm_rtc_allocate_device, not 
devm_rtc_register_device.

>> +	}
>>   
>>   	return 0;
>>   }
> 
> Johan
> 

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Johan Hovold]

On Wed, Oct 16, 2024 at 08:44:26AM -0400, Jonathan Marek wrote:
> On 10/16/24 2:42 AM, Johan Hovold wrote:
> > On Mon, Oct 14, 2024 at 08:47:26PM -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 qcom,no-alarm flag to support RTC on this platform.
> > 
> > An alternative may be to drop the alarm interrupt from DT and use that
> > as an indicator.
> 
> That wouldn't be right, the registers/interrupt still exist and should 
> be described in DT.

Yeah, the registers are still there, and are probably readable too
(IIRC), but the OS will never receive any interrupts.

> (if you have firmware that allows access to the alarm, now you only have 
> to delete the qcom,no-alarm property in your dts to use it)

Fair enough. And the new flag mirrors the old.

> >> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> >> ---
> >>   drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++-------------
> >>   1 file changed, 30 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> >> index c32fba550c8e0..1e78939625622 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;
> > 
> > How about inverting this one and naming it has_alarm or similar to avoid
> > the double negation in your conditionals (!no_alarm)?
> > 
> 
> My reasoning is that the DT flag has to be negative, and its better to 
> use the same name as the DT flag, but inverting it is OK.

I agree about the dt parameter, but I still I prefer a non-negated
variable (similar to allow_set_time).

> >>   	int alarm_irq;
> >>   	const struct pm8xxx_rtc_regs *regs;
> >>   	struct device *dev;
> >> @@ -473,9 +474,14 @@ 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,
> >> +						 "qcom,no-alarm");
> >> +
> > 
> > Stray newline.
> > 
> 
> That's not a stray newline?

There was no empty line between the assignment and check before this
change, but now there is even though there should not be.
 
> >> +	if (!rtc_dd->no_alarm) {
> >> +		rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
> >> +		if (rtc_dd->alarm_irq < 0)
> >> +			return -ENXIO;
> >> +	}

Johan

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Jonathan Marek]

On 10/16/24 9:02 AM, Johan Hovold wrote:
>>>>    	int alarm_irq;
>>>>    	const struct pm8xxx_rtc_regs *regs;
>>>>    	struct device *dev;
>>>> @@ -473,9 +474,14 @@ 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,
>>>> +						 "qcom,no-alarm");
>>>> +
>>>
>>> Stray newline.
>>>
>>
>> That's not a stray newline?
> 
> There was no empty line between the assignment and check before this
> change, but now there is even though there should not be.
>   

There was no empty line between the "alarm_irq" assignment and check, 
and there still isn't. That empty line separating the new 
of_property_read_bool() line.

I could move both of_property_read_bool() lines together to make it look 
better.

>>>> +	if (!rtc_dd->no_alarm) {
>>>> +		rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
>>>> +		if (rtc_dd->alarm_irq < 0)
>>>> +			return -ENXIO;
>>>> +	}
> 
> Johan
> 

Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Johan Hovold]

On Wed, Oct 16, 2024 at 09:12:08AM -0400, Jonathan Marek wrote:
> On 10/16/24 9:02 AM, Johan Hovold wrote:
> >>>>    	int alarm_irq;
> >>>>    	const struct pm8xxx_rtc_regs *regs;
> >>>>    	struct device *dev;
> >>>> @@ -473,9 +474,14 @@ 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,
> >>>> +						 "qcom,no-alarm");
> >>>> +
> >>>
> >>> Stray newline.
> >>
> >> That's not a stray newline?
> > 
> > There was no empty line between the assignment and check before this
> > change, but now there is even though there should not be.
> 
> There was no empty line between the "alarm_irq" assignment and check, 
> and there still isn't. That empty line separating the new 
> of_property_read_bool() line.

Ah, sorry, my bad.

> I could move both of_property_read_bool() lines together to make it look 
> better.

Yeah, that sounds like a good idea.

Johan

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

[Alexandre Belloni]

Hello,

Did I miss v4 of this series?

On 14/10/2024 20:47: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
> v3:
>  - renamed flag to qcom,no-alarm
>  - don't remove alarm registers/interrupt from dts
> 
> Jonathan Marek (5):
>   rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
>   dt-bindings: rtc: qcom-pm8xxx: document qcom,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  |  3 +-
>  drivers/rtc/rtc-pm8xxx.c                      | 44 +++++++++++++------
>  5 files changed, 58 insertions(+), 16 deletions(-)
> 
> -- 
> 2.45.1
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Johan Hovold]

Hi Alexandre and Jonathan,

On Mon, Jan 13, 2025 at 12:35:38AM +0100, Alexandre Belloni wrote:
> Did I miss v4 of this series?

We are still waiting for Qualcomm to provide a free and safe-to-use
allocation for the RTC offset in the PMIC.

I got tired of reminding the Qualcomm folks about this and instead
revived my patches for using the UEFI variable that the firmware (and
Windows) use for the offset.

I included Jonathan's v3 series after making the changes that we
discussed here (and a few more).

The result is here:

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

and should allow us to enable the RTC on X1E today (and also avoids
running into the same SDAM allocation issue for the next platform).

> On 14/10/2024 20:47: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
> > v3:
> >  - renamed flag to qcom,no-alarm
> >  - don't remove alarm registers/interrupt from dts
> > 
> > Jonathan Marek (5):
> >   rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
> >   dt-bindings: rtc: qcom-pm8xxx: document qcom,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

Johan