Date:   Mon, 13 Feb 2023 13:18:48 -0800
From:   Bjorn Andersson <andersson@kernel.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        konrad.dybcio@linaro.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Patrick Wildt <patrick@blueri.se>
Subject: Re: [PATCH] arm64: dts: qcom: sc8280xp-pmics: Specify interrupt
 parent explicitly
Message-ID: <20230213211848.ktj3koac3nbnhz3k@ripper>
References: <20230213090118.11527-1-manivannan.sadhasivam@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230213090118.11527-1-manivannan.sadhasivam@linaro.org>
Precedence: bulk

On Mon, Feb 13, 2023 at 02:31:18PM +0530, Manivannan Sadhasivam wrote:
> Nodes like pwrkey, resin, iadc, adc-tm, temp-alarm which are the grand
> children of spmi_bus node represent the interrupt generating devices but
> don't have "interrupt-parent" property.
> 
> As per the devicetree spec v0.3, section 2.4:
> 
> "The physical wiring of an interrupt source to an interrupt controller is
> represented in the devicetree with the interrupt-parent property. Nodes
> that represent interrupt-generating devices contain an interrupt-parent
> property which has a phandle value that points to the device to which the
> device’s interrupts are routed, typically an interrupt controller. If an
> interrupt-generating device does not have an interrupt-parent property,
> its interrupt parent is assumed to be its devicetree parent."
> 
> This clearly says that if the "interrupt-parent" property is absent, then
> the immediate devicetree parent will be assumed as the interrupt parent.
> But the immediate parents of these nodes are not interrupt controllers
> themselves.
> 
> This may lead to failure while wiring the interrupt for these nodes by an
> operating system. But a few operating systems like Linux, workaround this
> issue by walking up the parent nodes until it finds the "interrupt-cells"
> property. Then the node that has the "interrupt-cells" property will be
> used as the interrupt parent.
> 
> But this workaround is not as per the DT spec and is not being implemented
> by other operating systems such as OpenBSD.
> 
> Hence, fix this issue by adding the "interrupts-extended" property that
> explicitly specifies the spmi_bus node as the interrupt parent. Note that
> the "interrupts-extended" property is chosen over "interrupt-parent" as it
> allows specifying both interrupt parent phandle and interrupt specifiers in
> a single property.
> 
> Reported-by: Patrick Wildt <patrick@blueri.se>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks Mani, I had forgotten about this issue again.

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

> ---
> 
> I just fixed one dtsi since wanted to make sure that everyone agrees to
> this implementation, otherwise I'll end up wasting too much time fixing
> every other DTs making use of these nodes.
> 
> Next step would be to fix the bindings of these nodes to include interrupt-
> parent or interrupts-extended properties and affected DTs.
> 
>  arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> index f2c0b71b5d8e..df7d28f7ae60 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> @@ -64,14 +64,14 @@ pmk8280_pon: pon@1300 {
>  
>  			pmk8280_pon_pwrkey: pwrkey {
>  				compatible = "qcom,pmk8350-pwrkey";
> -				interrupts = <0x0 0x13 0x7 IRQ_TYPE_EDGE_BOTH>;
> +				interrupts-extended = <&spmi_bus 0x0 0x13 0x7 IRQ_TYPE_EDGE_BOTH>;
>  				linux,code = <KEY_POWER>;
>  				status = "disabled";
>  			};
>  
>  			pmk8280_pon_resin: resin {
>  				compatible = "qcom,pmk8350-resin";
> -				interrupts = <0x0 0x13 0x6 IRQ_TYPE_EDGE_BOTH>;
> +				interrupts-extended = <&spmi_bus 0x0 0x13 0x6 IRQ_TYPE_EDGE_BOTH>;
>  				status = "disabled";
>  			};
>  		};
> @@ -79,7 +79,7 @@ pmk8280_pon_resin: resin {
>  		pmk8280_vadc: adc@3100 {
>  			compatible = "qcom,spmi-adc7";
>  			reg = <0x3100>;
> -			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> +			interrupts-extended = <&spmi_bus 0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			#io-channel-cells = <1>;
> @@ -89,7 +89,7 @@ pmk8280_vadc: adc@3100 {
>  		pmk8280_adc_tm: adc-tm@3400 {
>  			compatible = "qcom,spmi-adc-tm5-gen2";
>  			reg = <0x3400>;
> -			interrupts = <0x0 0x34 0x0 IRQ_TYPE_EDGE_RISING>;
> +			interrupts-extended = <&spmi_bus 0x0 0x34 0x0 IRQ_TYPE_EDGE_RISING>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			#thermal-sensor-cells = <1>;
> @@ -106,7 +106,7 @@ pmc8280_1: pmic@1 {
>  		pm8280_1_temp_alarm: temp-alarm@a00 {
>  			compatible = "qcom,spmi-temp-alarm";
>  			reg = <0xa00>;
> -			interrupts = <0x1 0xa 0x0 IRQ_TYPE_EDGE_BOTH>;
> +			interrupts-extended = <&spmi_bus 0x1 0xa 0x0 IRQ_TYPE_EDGE_BOTH>;
>  			#thermal-sensor-cells = <0>;
>  		};
>  
> @@ -158,7 +158,7 @@ pmc8280_2: pmic@3 {
>  		pm8280_2_temp_alarm: temp-alarm@a00 {
>  			compatible = "qcom,spmi-temp-alarm";
>  			reg = <0xa00>;
> -			interrupts = <0x2 0xa 0x0 IRQ_TYPE_EDGE_BOTH>;
> +			interrupts-extended = <&spmi_bus 0x2 0xa 0x0 IRQ_TYPE_EDGE_BOTH>;
>  			#thermal-sensor-cells = <0>;
>  		};
>  
> -- 
> 2.25.1
>