x1e80100 RTC support
Subsystems (4)
v3 cover x1e80100 RTC support
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(-)
Signature · 1 line
2.45.1
PATCH v3 1/5 rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
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.
--- drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-)
drivers/rtc/rtc-pm8xxx.c +30 -14
@@ -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 = {
Signature · 1 line
2.45.1
Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
Quoted 6 lines · 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.
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
Quoted 95 lines
Signed-off-by Jonathan Marek <[email protected]>--- drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-)drivers/rtc/rtc-pm8xxx.c +30
-14@@ -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 = {Signature · 1 line
2.45.1
Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
In reply to Jonathan MarekQuoted 5 lines · 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.
An alternative may be to drop the alarm interrupt from DT and use that as an indicator.
Quoted 10 lines
Signed-off-by Jonathan Marek <[email protected]>--- drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-)drivers/rtc/rtc-pm8xxx.c +0
-0
drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 64
Quoted 4 lines
drivers/rtc/rtc-pm8xxx.c +1
-0@@ -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)?
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 479
Quoted 13 lines
drivers/rtc/rtc-pm8xxx.c +3
-3int 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.
Quoted 40 lines
drivers/rtc/rtc-pm8xxx.c +15
-7+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;
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 540
Quoted 10 lines
drivers/rtc/rtc-pm8xxx.c +6
-3- 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.
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 544
Quoted 4 lines
drivers/rtc/rtc-pm8xxx.c +1
-0+ }return 0 ; }
Johan
Alexandre Belloni
maintainerRe: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
Quoted 85 lines · Johan Hovold
Quoted 5 lines · 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.
An alternative may be to drop the alarm interrupt from DT and use that as an indicator.
Quoted 10 lines
Signed-off-by Jonathan Marek <[email protected]>--- drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-)drivers/rtc/rtc-pm8xxx.c +0
-0drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 64
Quoted 4 lines
drivers/rtc/rtc-pm8xxx.c +1
-0@@ -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_alarmor similar to avoid the double negation in your conditionals (!no_alarm)?drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 479
Quoted 13 lines
drivers/rtc/rtc-pm8xxx.c +3
-3int 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.
Quoted 41 lines
drivers/rtc/rtc-pm8xxx.c +15
-7+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;
Quoted 7 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 538
Quoted 7 lines
drivers/rtc/rtc-pm8xxx.c +4
-3- 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.
Quoted 12 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 540
Quoted 2 lines
drivers/rtc/rtc-pm8xxx.c +2
-0+ }else { +clear_bit (RTC_FEATURE_ALARM , rtc_dd->rtc->features);I assume that you should be clearing the feature bit before registering the RTC.
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 544
Quoted 4 lines
drivers/rtc/rtc-pm8xxx.c +1
-0+ }return 0 ; }Johan
Signature · 3 lines
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
In reply to Johan HovoldQuoted 10 lines · Johan Hovold
Quoted 5 lines · 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.
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)
Quoted 5 lines
Quoted 5 lines
Signed-off-by Jonathan Marek <[email protected]>--- drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-)
Quoted 13 lines
drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 64
Quoted 9 lines
drivers/rtc/rtc-pm8xxx.c +1
-0@@ -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_alarmor 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.
Quoted 7 lines
Quoted 7 lines
drivers/rtc/rtc-pm8xxx.c +0
-0int 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 ;
Quoted 9 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 479
Quoted 6 lines
drivers/rtc/rtc-pm8xxx.c +3
-3- 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?
Quoted 41 lines
Quoted 41 lines
drivers/rtc/rtc-pm8xxx.c +15
-7+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;
Quoted 13 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 540
Quoted 9 lines
drivers/rtc/rtc-pm8xxx.c +6
-3- 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.
Quoted 7 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 544
Quoted 4 lines
drivers/rtc/rtc-pm8xxx.c +1
-0+ }return 0 ; }Johan
Re: [PATCH v3 1/5] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
Quoted 6 lines · Jonathan Marek
Quoted 5 lines · Johan Hovold
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 2 lines
(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.
Quoted 19 lines
Quoted 5 lines
Quoted 5 lines
Signed-off-by Jonathan Marek <[email protected]>--- drivers/rtc/rtc-pm8xxx.c | 44 +++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 14 deletions(-)Quoted 13 lines
drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 64
Quoted 9 lines
drivers/rtc/rtc-pm8xxx.c +1
-0@@ -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_alarmor similar to avoid the double negation in your conditionals (!no_alarm)?
Quoted 2 lines
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).
Quoted 17 lines
Quoted 7 lines
Quoted 7 lines
drivers/rtc/rtc-pm8xxx.c +0
-0int 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 ;Quoted 9 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 479
Quoted 6 lines
drivers/rtc/rtc-pm8xxx.c +3
-3- 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.
Quoted 1 line
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.
Quoted 5 lines
Quoted 5 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 484
Quoted 5 lines
drivers/rtc/rtc-pm8xxx.c +5
-0+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
Quoted 17 lines · Johan Hovold
Quoted 17 lines
Quoted 16 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 479
Quoted 13 lines
int alarm_irq; const struct pm8xxx_rtc_regs *regs; struct device *dev;drivers/rtc/rtc-pm8xxx.c +3
-3@@ -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.
Quoted 5 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 484
Quoted 5 lines
drivers/rtc/rtc-pm8xxx.c +5
-0+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
Quoted 17 lines · Jonathan Marek
Quoted 16 lines · Johan Hovold
Quoted 16 lines
Quoted 15 lines
drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_probe(struct platform_device *pdev) · line 479
Quoted 13 lines
int alarm_irq; const struct pm8xxx_rtc_regs *regs; struct device *dev;drivers/rtc/rtc-pm8xxx.c +3
-3@@ -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.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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 newof_property_read_bool()line.
Ah, sorry, my bad.
Quoted 2 lines
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
In reply to Jonathan MarekQualcomm 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.
--- Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++ 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +5 -0
@@ -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:
Signature · 1 line
2.45.1
Re: [PATCH v3 2/5] dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag
Quoted 8 lines · 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 <[email protected]>---
Best regards,
Krzysztof
Re: [PATCH v3 2/5] dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag
In reply to Jonathan MarekQuoted 5 lines · 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.
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
In reply to Jonathan MarekOnly access to RTC alarm is blocked.
Enable RTC access by setting the qcom,no-alarm flag.
--- arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi +1 -2
@@ -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 {
Signature · 1 line
2.45.1
PATCH v3 4/5 arm64: dts: qcom: x1e80100-crd: add rtc offset to set rtc time
In reply to Jonathan MarekSee commit e67b45582c5e for explanation.
Note: the 0xbc offset is arbitrary, it just needs to not be already in use.
--- arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 11 +++++++++++ 1 file changed, 11 insertions(+)
arch/arm64/boot/dts/qcom/x1e80100-crd.dts +11 -0
@@ -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"; };
Signature · 1 line
2.45.1
Re: [PATCH v3 4/5] arm64: dts: qcom: x1e80100-crd: add rtc offset to set rtc time
Quoted 1 line · Jonathan Marek
See commit
e67b45582c5efor 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.
Quoted 1 line
Note: the
0xbcoffset 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
Quoted 8 lines · Johan Hovold
Quoted 1 line · Jonathan Marek
See commit
e67b45582c5efor 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).
Quoted 9 lines
Also spell out the commit summary in parenthesis when referring to commits as I did above.
Quoted 1 line
Note: the
0xbcoffset 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
Quoted 5 lines · Jonathan Marek
Quoted 4 lines · Johan Hovold
Quoted 1 line · Jonathan Marek
Note: the
0xbcoffset is arbitrary, it just needs to not be already in use.
Quoted 8 lines
Quoted 3 lines
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
In reply to Jonathan MarekQuoted 17 lines · Jonathan Marek
See commit
e67b45582c5efor explanation.Note: the
0xbcoffset is arbitrary, it just needs to not be already in use.Signed-off-by Jonathan Marek <[email protected]>--- arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 11 +++++++++++ 1 file changed, 11 insertions(+)arch/arm64/boot/dts/qcom/x1e80100-crd.dts +0
-0@@ -1224,6 +1224,17 @@ edp_bl_en: edp-bl-en-state { }; };
arch/arm64/boot/dts/qcom/x1e80100-crd.dts edp_bl_en: edp-bl-en-state { · line 1236
Quoted 10 lines
arch/arm64/boot/dts/qcom/x1e80100-crd.dts +10
-0+&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
In reply to Jonathan MarekSee commit e67b45582c5e for explanation.
Note: the 0xbc offset is arbitrary, it just needs to not be already in use.
--- .../boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts | 11 +++++++++++ 1 file changed, 11 insertions(+)
arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts +11 -0
@@ -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"; };
Signature · 1 line
2.45.1
Alexandre Belloni
maintainerHello,
Did I miss v4 of this series?
Quoted 26 lines · 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_ALARMinstead v3:
- renamed flag to qcom,no-alarm
- don't remove alarm registers/interrupt from dtsJonathan 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(-)Signature · 1 line
2.45.1
Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Alexandre and Jonathan,
Quoted 1 line · Alexandre Belloni
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/[email protected]/
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).
Quoted 9 lines · Jonathan Marek
Quoted 8 lines
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_ALARMinstead v3:
- renamed flag to qcom,no-alarm
- don't remove alarm registers/interrupt from dts
Quoted 6 lines
Quoted 6 lines
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