[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

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 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

[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 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

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

[Jonathan Marek]

Only access to RTC alarm is blocked.

Enable RTC access by setting the qcom,no-alarm flag.

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

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

[PATCH v3 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

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

[Johan Hovold]

On Mon, Oct 14, 2024 at 08:47:29PM -0400, Jonathan Marek wrote:
> See commit e67b45582c5e for explanation.

It's good that you reference commit e67b45582c5e ("arm64: dts: qcom:
sc8280xp-crd: enable rtc") but your commit message still needs to be
self-contained and provide the explanation here in some form (e.g.
quoted or paraphrased).

Also spell out the commit summary in parenthesis when referring to
commits as I did above.

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

How did you verify that nothing is using this offset on this platform? I
assume we need someone with access to the docs to make sure it's not in
use as we did for sc8280xp.

Johan

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

[Jonathan Marek]

On 10/16/24 2:51 AM, Johan Hovold wrote:
> On Mon, Oct 14, 2024 at 08:47:29PM -0400, Jonathan Marek wrote:
>> See commit e67b45582c5e for explanation.
> 
> It's good that you reference commit e67b45582c5e ("arm64: dts: qcom:
> sc8280xp-crd: enable rtc") but your commit message still needs to be
> self-contained and provide the explanation here in some form (e.g.
> quoted or paraphrased).
> 
> Also spell out the commit summary in parenthesis when referring to
> commits as I did above.
> 
>> Note: the 0xbc offset is arbitrary, it just needs to not be already in use.
> 
> How did you verify that nothing is using this offset on this platform? I
> assume we need someone with access to the docs to make sure it's not in
> use as we did for sc8280xp.
> 

AFAIK qcom allocate things from the start of the SDAM, so allocating 
from the end of the SDAM should be safe. And AFAIK this is supposed to 
be a general purpose HLOS (linux/windows) SDAM block, so should be 
mostly free to use.

(its possible windows uses this offset for something, I don't know about 
that)

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

[Johan Hovold]

On Wed, Oct 16, 2024 at 09:31:00AM -0400, Jonathan Marek wrote:
> On 10/16/24 2:51 AM, Johan Hovold wrote:
> > On Mon, Oct 14, 2024 at 08:47:29PM -0400, Jonathan Marek wrote:

> >> Note: the 0xbc offset is arbitrary, it just needs to not be already in use.
> > 
> > How did you verify that nothing is using this offset on this platform? I
> > assume we need someone with access to the docs to make sure it's not in
> > use as we did for sc8280xp.
>
> AFAIK qcom allocate things from the start of the SDAM, so allocating 
> from the end of the SDAM should be safe. And AFAIK this is supposed to 
> be a general purpose HLOS (linux/windows) SDAM block, so should be 
> mostly free to use.

From what I understand these registers are also used for things like
programmable LEDs (e.g. see 24e2d05d1b68 ("leds: Add driver for Qualcomm
LPG")). And who knows what else.

It would be good if someone from Qualcomm could confirm that these bytes
are free for use before merging. I've started asking around.

Johan

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

[Konrad Dybcio]

On 15.10.2024 2:47 AM, Jonathan Marek wrote:
> 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>;
> +	};
> +};

Setting random bits in SDAM is a very very very very bad idea

I'll try to get a good spot for the offset internally

Konrad

[PATCH v3 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 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