| Message ID | 20230627085306.6033-1-johan+linaro@kernel.org |
|---|---|
| State | Accepted |
| Commit | 55c9b1bf29dad107b3871bbb250c00df80a68791 |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-pmics: add explicit rtc interrupt parent | expand |
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 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,
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"; };
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(-)