arm64: dts: qcom: sc8280xp-pmics: add explicit rtc interrupt parent

  Unless explicitly specified the interrupt-parent property is inherited
from the parent node on Linux even though this may not be in full
compliance with the devicetree specification.

Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
Specify interrupt parent explicitly"), add an explicit interrupt parent
also for the PMIC RTC node for the benefit of other operating systems
which may be confused by this omission.

Note that any such OS must still implement a fallback to the root
interrupt domain as most devicetrees are written under the assumption
that the interrupt parent is inherited.

Reported-by: Patrick Wildt <patrick@blueri.se>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 27.06.2023 10:53, Johan Hovold wrote:
> Unless explicitly specified the interrupt-parent property is inherited
> from the parent node on Linux even though this may not be in full
> compliance with the devicetree specification.
> 
> Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> Specify interrupt parent explicitly"), add an explicit interrupt parent
> also for the PMIC RTC node for the benefit of other operating systems
> which may be confused by this omission.
> 
> Note that any such OS must still implement a fallback to the root
> interrupt domain as most devicetrees are written under the assumption
> that the interrupt parent is inherited.
> 
> Reported-by: Patrick Wildt <patrick@blueri.se>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Maybe a warning in of core should be introduced..

Or maybe dtc could learn not to compile such DTS :P

Konrad
>  arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> index a0ba535bb6c9..80ee12ded4f4 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> @@ -101,7 +101,7 @@ pmk8280_rtc: rtc@6100 {
>  			compatible = "qcom,pmk8350-rtc";
>  			reg = <0x6100>, <0x6200>;
>  			reg-names = "rtc", "alarm";
> -			interrupts = <0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
> +			interrupts-extended = <&spmi_bus 0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
>  			wakeup-source;
>  			status = "disabled";
>  		};
  

On Tue, Jun 27, 2023 at 10:53:06AM +0200, Johan Hovold wrote:
> Unless explicitly specified the interrupt-parent property is inherited
> from the parent node on Linux even though this may not be in full
> compliance with the devicetree specification.
> 
> Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> Specify interrupt parent explicitly"), add an explicit interrupt parent
> also for the PMIC RTC node for the benefit of other operating systems
> which may be confused by this omission.
> 
> Note that any such OS must still implement a fallback to the root
> interrupt domain as most devicetrees are written under the assumption
> that the interrupt parent is inherited.
> 
> Reported-by: Patrick Wildt <patrick@blueri.se>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

It is good to encode this in the binding and fix other such instances.

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> index a0ba535bb6c9..80ee12ded4f4 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> @@ -101,7 +101,7 @@ pmk8280_rtc: rtc@6100 {
>  			compatible = "qcom,pmk8350-rtc";
>  			reg = <0x6100>, <0x6200>;
>  			reg-names = "rtc", "alarm";
> -			interrupts = <0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
> +			interrupts-extended = <&spmi_bus 0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
>  			wakeup-source;
>  			status = "disabled";
>  		};
> -- 
> 2.39.3
>
  

On Tue, Jun 27, 2023 at 06:54:06PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jun 27, 2023 at 10:53:06AM +0200, Johan Hovold wrote:
> > Unless explicitly specified the interrupt-parent property is inherited
> > from the parent node on Linux even though this may not be in full
> > compliance with the devicetree specification.
> > 
> > Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> > Specify interrupt parent explicitly"), add an explicit interrupt parent
> > also for the PMIC RTC node for the benefit of other operating systems
> > which may be confused by this omission.
> > 
> > Note that any such OS must still implement a fallback to the root
> > interrupt domain as most devicetrees are written under the assumption
> > that the interrupt parent is inherited.
> > 
> > Reported-by: Patrick Wildt <patrick@blueri.se>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> It is good to encode this in the binding and fix other such instances.

Not sure about that. Perhaps the spec should be updated to match reality
instead... We have many more instances like this, even for this very
SoC, but apparently OpenBSD or whatever OS needs this falls back to the
root domain then.

Changing this for the rtc node for consistency after you changed the
others is a no-brainer, but not sure about trying to do this tree-wide.
We already have too many of these one-line DT cleanups...

Johan
  

On Tue, Jun 27, 2023 at 05:27:32PM +0200, Johan Hovold wrote:
> On Tue, Jun 27, 2023 at 06:54:06PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jun 27, 2023 at 10:53:06AM +0200, Johan Hovold wrote:
> > > Unless explicitly specified the interrupt-parent property is inherited
> > > from the parent node on Linux even though this may not be in full
> > > compliance with the devicetree specification.
> > > 
> > > Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> > > Specify interrupt parent explicitly"), add an explicit interrupt parent
> > > also for the PMIC RTC node for the benefit of other operating systems
> > > which may be confused by this omission.
> > > 
> > > Note that any such OS must still implement a fallback to the root
> > > interrupt domain as most devicetrees are written under the assumption
> > > that the interrupt parent is inherited.
> > > 
> > > Reported-by: Patrick Wildt <patrick@blueri.se>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > 
> > It is good to encode this in the binding and fix other such instances.
> 
> Not sure about that. Perhaps the spec should be updated to match reality
> instead... We have many more instances like this, even for this very
> SoC, but apparently OpenBSD or whatever OS needs this falls back to the
> root domain then.
> 

Just because linux is doing it in a different way doesn't warrant an amendment
to the spec IMO.

> Changing this for the rtc node for consistency after you changed the
> others is a no-brainer, but not sure about trying to do this tree-wide.
> We already have too many of these one-line DT cleanups...
> 

I agree that this is going to be a one-line cleanup but someone has to do it.
(not asking you to do since I also skipped it during 2d5cab9232ba). We can put
it in the back burner.

- Mani

> Johan
  

On Wed, Jun 28, 2023 at 10:55:57AM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jun 27, 2023 at 05:27:32PM +0200, Johan Hovold wrote:
> > On Tue, Jun 27, 2023 at 06:54:06PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Jun 27, 2023 at 10:53:06AM +0200, Johan Hovold wrote:
> > > > Unless explicitly specified the interrupt-parent property is inherited
> > > > from the parent node on Linux even though this may not be in full
> > > > compliance with the devicetree specification.
> > > > 
> > > > Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> > > > Specify interrupt parent explicitly"), add an explicit interrupt parent
> > > > also for the PMIC RTC node for the benefit of other operating systems
> > > > which may be confused by this omission.
> > > > 
> > > > Note that any such OS must still implement a fallback to the root
> > > > interrupt domain as most devicetrees are written under the assumption
> > > > that the interrupt parent is inherited.
> > > > 
> > > > Reported-by: Patrick Wildt <patrick@blueri.se>
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > 
> > > It is good to encode this in the binding and fix other such instances.
> > 
> > Not sure about that. Perhaps the spec should be updated to match reality
> > instead... We have many more instances like this, even for this very
> > SoC, but apparently OpenBSD or whatever OS needs this falls back to the
> > root domain then.
> > 
> 
> Just because linux is doing it in a different way doesn't warrant an amendment
> to the spec IMO.

My point is that it's apparently not just Linux as most devicetrees work
this way at least for the root domain. And then it may be time to update
the spec in some way.
 
> > Changing this for the rtc node for consistency after you changed the
> > others is a no-brainer, but not sure about trying to do this tree-wide.
> > We already have too many of these one-line DT cleanups...
> > 
> 
> I agree that this is going to be a one-line cleanup but someone has to do it.
> (not asking you to do since I also skipped it during 2d5cab9232ba). We can put
> it in the back burner.

So that may actually amount to a ten-thousand line diff or so... And
then it's probably better to just update the spec.

Johan
  

On Wed, Jun 28, 2023 at 08:47:00AM +0200, Johan Hovold wrote:
> On Wed, Jun 28, 2023 at 10:55:57AM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jun 27, 2023 at 05:27:32PM +0200, Johan Hovold wrote:
> > > On Tue, Jun 27, 2023 at 06:54:06PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Jun 27, 2023 at 10:53:06AM +0200, Johan Hovold wrote:
> > > > > Unless explicitly specified the interrupt-parent property is inherited
> > > > > from the parent node on Linux even though this may not be in full
> > > > > compliance with the devicetree specification.
> > > > > 
> > > > > Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> > > > > Specify interrupt parent explicitly"), add an explicit interrupt parent
> > > > > also for the PMIC RTC node for the benefit of other operating systems
> > > > > which may be confused by this omission.
> > > > > 
> > > > > Note that any such OS must still implement a fallback to the root
> > > > > interrupt domain as most devicetrees are written under the assumption
> > > > > that the interrupt parent is inherited.
> > > > > 
> > > > > Reported-by: Patrick Wildt <patrick@blueri.se>
> > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > > 
> > > > It is good to encode this in the binding and fix other such instances.
> > > 
> > > Not sure about that. Perhaps the spec should be updated to match reality
> > > instead... We have many more instances like this, even for this very
> > > SoC, but apparently OpenBSD or whatever OS needs this falls back to the
> > > root domain then.
> > > 
> > 
> > Just because linux is doing it in a different way doesn't warrant an amendment
> > to the spec IMO.
> 
> My point is that it's apparently not just Linux as most devicetrees work
> this way at least for the root domain. And then it may be time to update
> the spec in some way.

I'm not sure about the point you're trying to make.  In OpenBSD's
implementation, which I think complies with the spec, for non-extended
interrupts we check the node's (or all its parents') interrupt-parent
property.

Technically the SPMI arbiter could define an interrupt-parent that
points to itself, because it's using interrupts-extended anyway to
point to the PDC.  But that would feel a bit stupid and not really
correct.  Alternatively each child node could have interrupt-parent.

That said, I understand the point that it might make sense to amend
the spec so that if a parent node is an interrupt-controller, that's
most probably interrupt parent, unless an interrupt-parent property
shows up before.

I would like to add that OpenBSD supports a number of SoCs for quite
some years and this is the first time I've hit an issue with interrupts
that were not designed in a way for the current spec to work.  That said
we obviously support quite fewer SoCs/boards in total compared to Linux.

Cheers,
Patrick
  

On 28.06.2023 10:31, Patrick Wildt wrote:
> On Wed, Jun 28, 2023 at 08:47:00AM +0200, Johan Hovold wrote:
>> On Wed, Jun 28, 2023 at 10:55:57AM +0530, Manivannan Sadhasivam wrote:
>>> On Tue, Jun 27, 2023 at 05:27:32PM +0200, Johan Hovold wrote:
>>>> On Tue, Jun 27, 2023 at 06:54:06PM +0530, Manivannan Sadhasivam wrote:
>>>>> On Tue, Jun 27, 2023 at 10:53:06AM +0200, Johan Hovold wrote:
>>>>>> Unless explicitly specified the interrupt-parent property is inherited
>>>>>> from the parent node on Linux even though this may not be in full
>>>>>> compliance with the devicetree specification.
>>>>>>
>>>>>> Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
>>>>>> Specify interrupt parent explicitly"), add an explicit interrupt parent
>>>>>> also for the PMIC RTC node for the benefit of other operating systems
>>>>>> which may be confused by this omission.
>>>>>>
>>>>>> Note that any such OS must still implement a fallback to the root
>>>>>> interrupt domain as most devicetrees are written under the assumption
>>>>>> that the interrupt parent is inherited.
>>>>>>
>>>>>> Reported-by: Patrick Wildt <patrick@blueri.se>
>>>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>>>
>>>>> It is good to encode this in the binding and fix other such instances.
>>>>
>>>> Not sure about that. Perhaps the spec should be updated to match reality
>>>> instead... We have many more instances like this, even for this very
>>>> SoC, but apparently OpenBSD or whatever OS needs this falls back to the
>>>> root domain then.
>>>>
>>>
>>> Just because linux is doing it in a different way doesn't warrant an amendment
>>> to the spec IMO.
>>
>> My point is that it's apparently not just Linux as most devicetrees work
>> this way at least for the root domain. And then it may be time to update
>> the spec in some way.
> 
> I'm not sure about the point you're trying to make.  In OpenBSD's
> implementation, which I think complies with the spec, for non-extended
> interrupts we check the node's (or all its parents') interrupt-parent
> property.
> 
> Technically the SPMI arbiter could define an interrupt-parent that
> points to itself, because it's using interrupts-extended anyway to
> point to the PDC.  But that would feel a bit stupid and not really
> correct.  Alternatively each child node could have interrupt-parent.
> 
> That said, I understand the point that it might make sense to amend
> the spec so that if a parent node is an interrupt-controller, that's
> most probably interrupt parent, unless an interrupt-parent property
> shows up before.
> 
> I would like to add that OpenBSD supports a number of SoCs for quite
> some years and this is the first time I've hit an issue with interrupts
> that were not designed in a way for the current spec to work.  That said
> we obviously support quite fewer SoCs/boards in total compared to Linux.
Linux does something out of spec here. We should either comply or amend
the dt spec. Perhaps that's a question for Rob Herring.

Konrad
> 
> Cheers,
> Patrick
  

On Wed, Jun 28, 2023 at 10:31:57AM +0200, Patrick Wildt wrote:
> On Wed, Jun 28, 2023 at 08:47:00AM +0200, Johan Hovold wrote:

> > My point is that it's apparently not just Linux as most devicetrees work
> > this way at least for the root domain. And then it may be time to update
> > the spec in some way.
> 
> I'm not sure about the point you're trying to make.  In OpenBSD's
> implementation, which I think complies with the spec, for non-extended
> interrupts we check the node's (or all its parents') interrupt-parent
> property.

My point is that that is not compliant with the spec either which only
says that in case 'interrupt-parent' is missing in a node for an
interrupt-generating device, then the interrupt parent is assumed to be
the devicetree parent (which must then also be an interrupt controller
or nexus).

There is no provision for any recursive lookup in the spec currently.

> Technically the SPMI arbiter could define an interrupt-parent that
> points to itself, because it's using interrupts-extended anyway to
> point to the PDC.  But that would feel a bit stupid and not really
> correct.  Alternatively each child node could have interrupt-parent.

I agree that that would not really be correct (e.g. as 'interrupt' and
'interrupt-extended' are supposed to be mutually exclusive).

> That said, I understand the point that it might make sense to amend
> the spec so that if a parent node is an interrupt-controller, that's
> most probably interrupt parent,

This bit is already in the spec.

> unless an interrupt-parent property
> shows up before.

But this seems to suggest that you really meant to say "ancestor" in the
first clause?

> I would like to add that OpenBSD supports a number of SoCs for quite
> some years and this is the first time I've hit an issue with interrupts
> that were not designed in a way for the current spec to work.  That said
> we obviously support quite fewer SoCs/boards in total compared to Linux.

So OpenBSD apparently implements something similar to Linux (recursive
lookup of 'interrupt-parent' properties), but not the part about
stopping the recursion when hitting an interrupt controller.

Neither part appears to be spec compliant, but you only care about
updating DTs that do not comply to the latter bit. That seems reasonable
and should importantly not require adding tens of thousands of
'interrupt-parent' properties to the DTs in mainline.

Johan
  

On Tue, 27 Jun 2023 10:53:06 +0200, Johan Hovold wrote:
> Unless explicitly specified the interrupt-parent property is inherited
> from the parent node on Linux even though this may not be in full
> compliance with the devicetree specification.
> 
> Following commit 2d5cab9232ba ("arm64: dts: qcom: sc8280xp-pmics:
> Specify interrupt parent explicitly"), add an explicit interrupt parent
> also for the PMIC RTC node for the benefit of other operating systems
> which may be confused by this omission.
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: qcom: sc8280xp-pmics: add explicit rtc interrupt parent
      commit: 55c9b1bf29dad107b3871bbb250c00df80a68791

Best regards,