| Message ID | 20241015004945.3676-1-jonathan@marek.ca |
|---|---|
| Headers | show |
| Series | x1e80100 RTC support | expand |
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
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
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
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 >
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
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 >
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
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)
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.
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 > >
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