[PATCH v3 1/2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 CD pin configuration

[PATCH v3 1/2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 CD pin configuration

[Volodymyr Babchuk]

There are two issues with SDHC2 configuration for SA8155P-ADP,
which prevent use of SDHC2 and causes issues with ethernet:

- Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of
  PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC
  TX. If sdhc driver probes after dwmac driver, it reconfigures
  gpio4 and this breaks Ethernet MAC.

- pinctrl configuration mentions gpio96 as CD pin. It seems it was
  copied from some SM8150 example, because as mentioned above,
  correct CD pin is gpio4 on PMM8155AU_1.

This patch fixes both mentioned issues by providing correct pin handle
and pinctrl configuration.

Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card")
Cc: stable@vger.kernel.org
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

In v3:
 - Moved regulator changes to a separate patch
 - Renamed pinctrl node
 - Moved pinctrl node so it will appear in alphabetical order
In v2:
 - Added "Fixes:" tag
 - CCed stable ML
 - Fixed pinctrl configuration
 - Extended voltage range for L13C voltage regulator
---
 arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 30 ++++++++++--------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
index 5e4287f8c8cd1..b2cf2c988336c 100644
--- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
@@ -367,6 +367,16 @@ queue0 {
 	};
 };
 
+&pmm8155au_1_gpios {
+	pmm8155au_1_sdc2_cd: sdc2-cd-default-state {
+		pins = "gpio4";
+		function = "normal";
+		input-enable;
+		bias-pull-up;
+		power-source = <0>;
+	};
+};
+
 &qupv3_id_1 {
 	status = "okay";
 };
@@ -384,10 +394,10 @@ &remoteproc_cdsp {
 &sdhc_2 {
 	status = "okay";
 
-	cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;
+	cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;
 	pinctrl-names = "default", "sleep";
-	pinctrl-0 = <&sdc2_on>;
-	pinctrl-1 = <&sdc2_off>;
+	pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>;
+	pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>;
 	vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */
 	vmmc-supply = <&vreg_l17a_2p96>;  /* Card power line */
 	bus-width = <4>;
@@ -505,13 +515,6 @@ data-pins {
 			bias-pull-up;		/* pull up */
 			drive-strength = <16>;	/* 16 MA */
 		};
-
-		sd-cd-pins {
-			pins = "gpio96";
-			function = "gpio";
-			bias-pull-up;		/* pull up */
-			drive-strength = <2>;	/* 2 MA */
-		};
 	};
 
 	sdc2_off: sdc2-off-state {
@@ -532,13 +535,6 @@ data-pins {
 			bias-pull-up;		/* pull up */
 			drive-strength = <2>;	/* 2 MA */
 		};
-
-		sd-cd-pins {
-			pins = "gpio96";
-			function = "gpio";
-			bias-pull-up;		/* pull up */
-			drive-strength = <2>;	/* 2 MA */
-		};
 	};
 
 	usb2phy_ac_en1_default: usb2phy-ac-en1-default-state {
-- 
2.44.0

[PATCH v3 2/2] arm64: dts: qcom: sa8155p-adp: lower min volt for L13C regulator

[Volodymyr Babchuk]

Voltage regulator L13C is used by SD card IO interface. In order to
support UHS modes, IO interface voltage needs to be set to 1.8V. This
patch extends minimum voltage range of L13C regulator to allow this.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
index b2cf2c988336c..9e9c7f81096bb 100644
--- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
@@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 {
 
 		vreg_l13c_2p96: ldo13 {
 			regulator-name = "vreg_l13c_2p96";
-			regulator-min-microvolt = <2504000>;
+			regulator-min-microvolt = <1800000>;
 			regulator-max-microvolt = <2960000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
 		};
-- 
2.44.0

Re: [PATCH v3 2/2] arm64: dts: qcom: sa8155p-adp: lower min volt for L13C regulator

[Stephan Gerhold]

On Fri, Apr 12, 2024 at 07:03:26PM +0000, Volodymyr Babchuk wrote:
> Voltage regulator L13C is used by SD card IO interface. In order to
> support UHS modes, IO interface voltage needs to be set to 1.8V. This
> patch extends minimum voltage range of L13C regulator to allow this.
> 

I think this one also deserves a Fixes and stable tag. One could argue
that enabling UHS modes is a new "feature" but I would say the original
commit just missed setting the correct voltages with the intended use
case. I doubt limiting to high speed was intended.

Maybe also add a Suggested-by tag from myself:

Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card")
Cc: stable@vger.kernel.org
Suggested-by: Stephan Gerhold <stephan@gerhold.net>

I believe you do not need to resend just to add these tags, the b4 tool
should pick them up while applying. It looks like this might not work
for the Cc: stable tag though...

Thanks,
Stephan

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> index b2cf2c988336c..9e9c7f81096bb 100644
> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> @@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 {
>  
>  		vreg_l13c_2p96: ldo13 {
>  			regulator-name = "vreg_l13c_2p96";
> -			regulator-min-microvolt = <2504000>;
> +			regulator-min-microvolt = <1800000>;
>  			regulator-max-microvolt = <2960000>;
>  			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>  		};
> -- 
> 2.44.0

Re: [PATCH v3 2/2] arm64: dts: qcom: sa8155p-adp: lower min volt for L13C regulator

[Volodymyr Babchuk]

Hi Stephan,

Stephan Gerhold <stephan@gerhold.net> writes:

> On Fri, Apr 12, 2024 at 07:03:26PM +0000, Volodymyr Babchuk wrote:
>> Voltage regulator L13C is used by SD card IO interface. In order to
>> support UHS modes, IO interface voltage needs to be set to 1.8V. This
>> patch extends minimum voltage range of L13C regulator to allow this.
>> 
>
> I think this one also deserves a Fixes and stable tag. One could argue
> that enabling UHS modes is a new "feature"

Yes, this was my reasoning, actually. I reasoned that the initial commit
("arm64: dts: qcom: sa8155p-adp: Add base dts file") was not obliged to
set voltage ranges correctly as it didn't enabled SDHC2 support. But you
are right,  0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support
for uSD card") should have this change.

> but I would say the original
> commit just missed setting the correct voltages with the intended use
> case. I doubt limiting to high speed was intended.
>
> Maybe also add a Suggested-by tag from myself:

Yes, sorry. I should have asked about adding your Suggested-by tag before
posting this change.

> Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card")
> Cc: stable@vger.kernel.org
> Suggested-by: Stephan Gerhold <stephan@gerhold.net>
>
> I believe you do not need to resend just to add these tags, the b4 tool
> should pick them up while applying. It looks like this might not work
> for the Cc: stable tag though...

Well, we can send separate email to stable@vger.kernel.org after this
patch will be applied... I believe it is better than posting v4.


-- 
WBR, Volodymyr

Re: [PATCH v3 1/2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 CD pin configuration

[Stephan Gerhold]

On Fri, Apr 12, 2024 at 07:03:25PM +0000, Volodymyr Babchuk wrote:
> There are two issues with SDHC2 configuration for SA8155P-ADP,
> which prevent use of SDHC2 and causes issues with ethernet:
> 
> - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of
>   PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC
>   TX. If sdhc driver probes after dwmac driver, it reconfigures
>   gpio4 and this breaks Ethernet MAC.
> 
> - pinctrl configuration mentions gpio96 as CD pin. It seems it was
>   copied from some SM8150 example, because as mentioned above,
>   correct CD pin is gpio4 on PMM8155AU_1.
> 
> This patch fixes both mentioned issues by providing correct pin handle
> and pinctrl configuration.
> 
> Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card")
> Cc: stable@vger.kernel.org
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Thanks for making all the changes and fixing all these issues! FWIW:

Reviewed-by: Stephan Gerhold <stephan@gerhold.net>

> 
> ---
> 
> In v3:
>  - Moved regulator changes to a separate patch
>  - Renamed pinctrl node
>  - Moved pinctrl node so it will appear in alphabetical order
> In v2:
>  - Added "Fixes:" tag
>  - CCed stable ML
>  - Fixed pinctrl configuration
>  - Extended voltage range for L13C voltage regulator
> ---
>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 30 ++++++++++--------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> index 5e4287f8c8cd1..b2cf2c988336c 100644
> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> @@ -367,6 +367,16 @@ queue0 {
>  	};
>  };
>  
> +&pmm8155au_1_gpios {
> +	pmm8155au_1_sdc2_cd: sdc2-cd-default-state {
> +		pins = "gpio4";
> +		function = "normal";
> +		input-enable;
> +		bias-pull-up;
> +		power-source = <0>;
> +	};
> +};
> +
>  &qupv3_id_1 {
>  	status = "okay";
>  };
> @@ -384,10 +394,10 @@ &remoteproc_cdsp {
>  &sdhc_2 {
>  	status = "okay";
>  
> -	cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;
> +	cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;
>  	pinctrl-names = "default", "sleep";
> -	pinctrl-0 = <&sdc2_on>;
> -	pinctrl-1 = <&sdc2_off>;
> +	pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>;
> +	pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>;
>  	vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */
>  	vmmc-supply = <&vreg_l17a_2p96>;  /* Card power line */
>  	bus-width = <4>;
> @@ -505,13 +515,6 @@ data-pins {
>  			bias-pull-up;		/* pull up */
>  			drive-strength = <16>;	/* 16 MA */
>  		};
> -
> -		sd-cd-pins {
> -			pins = "gpio96";
> -			function = "gpio";
> -			bias-pull-up;		/* pull up */
> -			drive-strength = <2>;	/* 2 MA */
> -		};
>  	};
>  
>  	sdc2_off: sdc2-off-state {
> @@ -532,13 +535,6 @@ data-pins {
>  			bias-pull-up;		/* pull up */
>  			drive-strength = <2>;	/* 2 MA */
>  		};
> -
> -		sd-cd-pins {
> -			pins = "gpio96";
> -			function = "gpio";
> -			bias-pull-up;		/* pull up */
> -			drive-strength = <2>;	/* 2 MA */
> -		};
>  	};
>  
>  	usb2phy_ac_en1_default: usb2phy-ac-en1-default-state {
> -- 
> 2.44.0

Re: (subset) [PATCH v3 1/2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 CD pin configuration

[Bjorn Andersson]

On Fri, 12 Apr 2024 19:03:25 +0000, Volodymyr Babchuk wrote:
> There are two issues with SDHC2 configuration for SA8155P-ADP,
> which prevent use of SDHC2 and causes issues with ethernet:
> 
> - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of
>   PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC
>   TX. If sdhc driver probes after dwmac driver, it reconfigures
>   gpio4 and this breaks Ethernet MAC.
> 
> [...]

Applied, thanks!

[2/2] arm64: dts: qcom: sa8155p-adp: lower min volt for L13C regulator
      commit: 5927bc586a3f2a81abcf8134fda5f7639abeeefd

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>