diff mbox series

[3/3] arm64: dts: qcom: sc8280xp-crd: Add PCIe CLKREQ# sleep state

Message ID 20231227-topic-8280_pcie_dts-v1-3-13d12b1698ff@linaro.org
State New
Headers show
Series SC8280XP preparatory PCIe fixes | expand

Commit Message

Konrad Dybcio Dec. 27, 2023, 10:28 p.m. UTC
The CLKREQ pin should not be muxed to its active function when the RC
is asleep. Add the missing pin sleep states to resolve that.

Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller")
Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem")
Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 78 ++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 27 deletions(-)

Comments

Johan Hovold Dec. 29, 2023, 12:55 p.m. UTC | #1
On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote:
> The CLKREQ pin should not be muxed to its active function when the RC
> is asleep. 

You forgot to explain *why* you think this is needed.

Note that this is only appears to be done for one upstream Qualcomm SoC
(msm8996) currently, and that, notably, there is no driver support for
actually changing the pin state.

> Add the missing pin sleep states to resolve that.

> Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller")
> Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem")
> Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD")

So not sure these Fixes tags are warranted either.

> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 78 ++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index ffc4406422ae..58c0c2d10cb3 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -530,8 +530,9 @@ &pcie2a {
>  
>  	vddpe-3v3-supply = <&vreg_nvme>;
>  
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&pcie2a_default>;
> +	pinctrl-0 = <&pcie2a_default>, <&pcie2a_clkreq_default>;
> +	pinctrl-1 = <&pcie2a_default>, <&pcie2a_clkreq_sleep>;
> +	pinctrl-names = "default", "sleep";

Johan
Manivannan Sadhasivam Dec. 29, 2023, 5:14 p.m. UTC | #2
On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote:
> The CLKREQ pin should not be muxed to its active function when the RC
> is asleep. Add the missing pin sleep states to resolve that.
> 

I don't understand why it should not be muxed and wondering what is the actual
issue you are seeing that lead to this conclusion.

- Mani

> Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller")
> Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem")
> Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 78 ++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index ffc4406422ae..58c0c2d10cb3 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -530,8 +530,9 @@ &pcie2a {
>  
>  	vddpe-3v3-supply = <&vreg_nvme>;
>  
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&pcie2a_default>;
> +	pinctrl-0 = <&pcie2a_default>, <&pcie2a_clkreq_default>;
> +	pinctrl-1 = <&pcie2a_default>, <&pcie2a_clkreq_sleep>;
> +	pinctrl-names = "default", "sleep";
>  
>  	status = "okay";
>  };
> @@ -549,8 +550,9 @@ &pcie3a {
>  
>  	vddpe-3v3-supply = <&vreg_wwan>;
>  
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&pcie3a_default>;
> +	pinctrl-0 = <&pcie3a_default>, <&pcie3a_clkreq_default>;
> +	pinctrl-1 = <&pcie3a_default>, <&pcie3a_clkreq_sleep>;
> +	pinctrl-names = "default", "sleep";
>  
>  	status = "okay";
>  };
> @@ -568,8 +570,9 @@ &pcie4 {
>  
>  	vddpe-3v3-supply = <&vreg_wlan>;
>  
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&pcie4_default>;
> +	pinctrl-0 = <&pcie4_default>, <&pcie4_clkreq_default>;
> +	pinctrl-1 = <&pcie4_default>, <&pcie4_clkreq_sleep>;
> +	pinctrl-names = "default", "sleep";
>  
>  	status = "okay";
>  };
> @@ -835,13 +838,6 @@ nvme_reg_en: nvme-reg-en-state {
>  	};
>  
>  	pcie2a_default: pcie2a-default-state {
> -		clkreq-n-pins {
> -			pins = "gpio142";
> -			function = "pcie2a_clkreq";
> -			drive-strength = <2>;
> -			bias-pull-up;
> -		};
> -
>  		perst-n-pins {
>  			pins = "gpio143";
>  			function = "gpio";
> @@ -857,14 +853,21 @@ wake-n-pins {
>  	       };
>  	};
>  
> -	pcie3a_default: pcie3a-default-state {
> -		clkreq-n-pins {
> -			pins = "gpio150";
> -			function = "pcie3a_clkreq";
> -			drive-strength = <2>;
> -			bias-pull-up;
> -		};
> +	pcie2a_clkreq_default: pcie2a-clkreq-default-state {
> +		pins = "gpio142";
> +		function = "pcie2a_clkreq";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
> +	pcie2a_clkreq_sleep: pcie2a-clkreq-sleep-state {
> +		pins = "gpio142";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
>  
> +	pcie3a_default: pcie3a-default-state {
>  		perst-n-pins {
>  			pins = "gpio151";
>  			function = "gpio";
> @@ -880,14 +883,21 @@ wake-n-pins {
>  		};
>  	};
>  
> -	pcie4_default: pcie4-default-state {
> -		clkreq-n-pins {
> -			pins = "gpio140";
> -			function = "pcie4_clkreq";
> -			drive-strength = <2>;
> -			bias-pull-up;
> -		};
> +	pcie3a_clkreq_default: pcie3a-clkreq-default-state {
> +		pins = "gpio150";
> +		function = "pcie3a_clkreq";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
>  
> +	pcie3a_clkreq_sleep: pcie3a-clkreq-sleep-state {
> +		pins = "gpio150";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
> +	pcie4_default: pcie4-default-state {
>  		perst-n-pins {
>  			pins = "gpio141";
>  			function = "gpio";
> @@ -903,6 +913,20 @@ wake-n-pins {
>  		};
>  	};
>  
> +	pcie4_clkreq_default: pcie4-clkreq-default-state {
> +		pins = "gpio140";
> +		function = "pcie4_clkreq";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
> +	pcie4_clkreq_sleep: pcie4-clkreq-sleep-state {
> +		pins = "gpio140";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
>  	sdc2_default_state: sdc2-default-state {
>  		clk-pins {
>  			pins = "sdc2_clk";
> 
> -- 
> 2.43.0
> 
>
Konrad Dybcio Dec. 30, 2023, 2:10 p.m. UTC | #3
On 29.12.2023 18:14, Manivannan Sadhasivam wrote:
> On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote:
>> The CLKREQ pin should not be muxed to its active function when the RC
>> is asleep. Add the missing pin sleep states to resolve that.
>>
> 
> I don't understand why it should not be muxed and wondering what is the actual
> issue you are seeing that lead to this conclusion.

According to my knowledge, demuxing this pin prevents the RC from
getting (possibly erronous) reference clock request signals when
running the RC shutdown sequence. Reading this again, the fixes
tags don't really fit this commit.

What's perhaps more interesting is that on msm8996, Qualcomm vendor
kernels used to remove the pull-up on the CLKREQ pin in its sleep
state, while on msm8998 through sm8550 they keep it there, always..
Perhaps an oversight?

Konrad
Konrad Dybcio Dec. 30, 2023, 2:11 p.m. UTC | #4
On 29.12.2023 13:55, Johan Hovold wrote:
> On Wed, Dec 27, 2023 at 11:28:28PM +0100, Konrad Dybcio wrote:
>> The CLKREQ pin should not be muxed to its active function when the RC
>> is asleep. 
> 
> You forgot to explain *why* you think this is needed.
You're right, I was in a flurry of patchsending..

> 
> Note that this is only appears to be done for one upstream Qualcomm SoC
> (msm8996) currently, and that, notably, there is no driver support for
> actually changing the pin state.
Please see my reply to Mani.

> 
>> Add the missing pin sleep states to resolve that.
> 
>> Fixes: d907fe5acbf1 ("arm64: dts: qcom: sc8280xp-crd: enable WiFi controller")
>> Fixes: 17e2ccaf65d1 ("arm64: dts: qcom: sc8280xp-crd: enable SDX55 modem")
>> Fixes: 6a1ec5eca73c ("arm64: dts: qcom: sc8280xp-crd: enable NVMe SSD")
> 
> So not sure these Fixes tags are warranted either.
Agreed!

Konrad
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index ffc4406422ae..58c0c2d10cb3 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -530,8 +530,9 @@  &pcie2a {
 
 	vddpe-3v3-supply = <&vreg_nvme>;
 
-	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2a_default>;
+	pinctrl-0 = <&pcie2a_default>, <&pcie2a_clkreq_default>;
+	pinctrl-1 = <&pcie2a_default>, <&pcie2a_clkreq_sleep>;
+	pinctrl-names = "default", "sleep";
 
 	status = "okay";
 };
@@ -549,8 +550,9 @@  &pcie3a {
 
 	vddpe-3v3-supply = <&vreg_wwan>;
 
-	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3a_default>;
+	pinctrl-0 = <&pcie3a_default>, <&pcie3a_clkreq_default>;
+	pinctrl-1 = <&pcie3a_default>, <&pcie3a_clkreq_sleep>;
+	pinctrl-names = "default", "sleep";
 
 	status = "okay";
 };
@@ -568,8 +570,9 @@  &pcie4 {
 
 	vddpe-3v3-supply = <&vreg_wlan>;
 
-	pinctrl-names = "default";
-	pinctrl-0 = <&pcie4_default>;
+	pinctrl-0 = <&pcie4_default>, <&pcie4_clkreq_default>;
+	pinctrl-1 = <&pcie4_default>, <&pcie4_clkreq_sleep>;
+	pinctrl-names = "default", "sleep";
 
 	status = "okay";
 };
@@ -835,13 +838,6 @@  nvme_reg_en: nvme-reg-en-state {
 	};
 
 	pcie2a_default: pcie2a-default-state {
-		clkreq-n-pins {
-			pins = "gpio142";
-			function = "pcie2a_clkreq";
-			drive-strength = <2>;
-			bias-pull-up;
-		};
-
 		perst-n-pins {
 			pins = "gpio143";
 			function = "gpio";
@@ -857,14 +853,21 @@  wake-n-pins {
 	       };
 	};
 
-	pcie3a_default: pcie3a-default-state {
-		clkreq-n-pins {
-			pins = "gpio150";
-			function = "pcie3a_clkreq";
-			drive-strength = <2>;
-			bias-pull-up;
-		};
+	pcie2a_clkreq_default: pcie2a-clkreq-default-state {
+		pins = "gpio142";
+		function = "pcie2a_clkreq";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
+	pcie2a_clkreq_sleep: pcie2a-clkreq-sleep-state {
+		pins = "gpio142";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
 
+	pcie3a_default: pcie3a-default-state {
 		perst-n-pins {
 			pins = "gpio151";
 			function = "gpio";
@@ -880,14 +883,21 @@  wake-n-pins {
 		};
 	};
 
-	pcie4_default: pcie4-default-state {
-		clkreq-n-pins {
-			pins = "gpio140";
-			function = "pcie4_clkreq";
-			drive-strength = <2>;
-			bias-pull-up;
-		};
+	pcie3a_clkreq_default: pcie3a-clkreq-default-state {
+		pins = "gpio150";
+		function = "pcie3a_clkreq";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
 
+	pcie3a_clkreq_sleep: pcie3a-clkreq-sleep-state {
+		pins = "gpio150";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
+	pcie4_default: pcie4-default-state {
 		perst-n-pins {
 			pins = "gpio141";
 			function = "gpio";
@@ -903,6 +913,20 @@  wake-n-pins {
 		};
 	};
 
+	pcie4_clkreq_default: pcie4-clkreq-default-state {
+		pins = "gpio140";
+		function = "pcie4_clkreq";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
+	pcie4_clkreq_sleep: pcie4-clkreq-sleep-state {
+		pins = "gpio140";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
 	sdc2_default_state: sdc2-default-state {
 		clk-pins {
 			pins = "sdc2_clk";