[PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Volodymyr Babchuk]

Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
PMM8155AU_1, not to internal TLMM. This change fixes two issues at
once: not working ethernet (because GPIO4 is used for MAC TX) and SD
detection.

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 5e4287f8c8cd1..6b08ce246b78c 100644
--- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
@@ -384,7 +384,7 @@ &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>;
-- 
2.44.0

Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Krzysztof Kozlowski]

On 10/04/2024 15:41, Volodymyr Babchuk wrote:
> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
> detection.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Please provide Fixes tag.

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Krzysztof Kozlowski]

On 10/04/2024 15:44, Krzysztof Kozlowski wrote:
> On 10/04/2024 15:41, Volodymyr Babchuk wrote:
>> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
>> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
>> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
>> detection.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> Please provide Fixes tag.

Ah, and Cc stable one.

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Konrad Dybcio]

On 4/10/24 15:41, Volodymyr Babchuk wrote:
> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
> detection.

Oh wow..

Konrad

Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Stephan Gerhold]

On Wed, Apr 10, 2024 at 01:41:30PM +0000, Volodymyr Babchuk wrote:
> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
> detection.
> 
> 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 5e4287f8c8cd1..6b08ce246b78c 100644
> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> @@ -384,7 +384,7 @@ &remoteproc_cdsp {
>  &sdhc_2 {
>  	status = "okay";
>  
> -	cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;
> +	cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;

Good catch!

>  	pinctrl-names = "default", "sleep";
>  	pinctrl-0 = <&sdc2_on>;
>  	pinctrl-1 = <&sdc2_off>;

These two pinctrl configure "gpio96" for "sd-cd-pins". Yet another wrong
GPIO? Should probably drop that and add proper pinctrl for the actual
GPIO in the PMIC. Seems like Qualcomm configured the PMIC GPIO with
pull-up downstream [1] (not sure if this is the right file). It might be
redundant if there is an external pull-up on the board, but only the
schematic could tell that for sure.

FWIW: Looking closer at the broken commit, the regulator voltage setup
of &sdhc_2 looks suspicious too. Typically one would want a 1.8V capable
regulator for the vqmmc-supply to properly use ultra high-speed modes,
but &vreg_l13c_2p96 seems to be configured with 2.504V-2.960V at the
moment. On downstream it seems to be 1.8V-2.96V [2] (again, not sure if
this is the right file). I would recommend re-checking this too and
testing if UHS cards are detected correctly (if you have the board).

&vreg_l17a_2p96 has the same 2.504V-2.960V, but has 2.704V-2.960V
downstream [3]. This is close at least, might be fine as-is (but I'm not
convinced there is a good reason to differ there).

Thanks,
Stephan

[1]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/484af352989c912db8f3b6393fc090006029066f/arch/arm64/boot/dts/qcom/sa8155-pmic-overlay.dtsi#L206-214
[2]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/484af352989c912db8f3b6393fc090006029066f/arch/arm64/boot/dts/qcom/sa8155-regulator.dtsi#L635-642
[3]: https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/484af352989c912db8f3b6393fc090006029066f/arch/arm64/boot/dts/qcom/sa8155-regulator.dtsi#L363-370

Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Volodymyr Babchuk]

Hi Stephan,

Stephan Gerhold <stephan@gerhold.net> writes:

> On Wed, Apr 10, 2024 at 01:41:30PM +0000, Volodymyr Babchuk wrote:
>> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
>> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
>> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
>> detection.
>> 
>> 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 5e4287f8c8cd1..6b08ce246b78c 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> @@ -384,7 +384,7 @@ &remoteproc_cdsp {
>>  &sdhc_2 {
>>  	status = "okay";
>>  
>> -	cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;
>> +	cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;
>
> Good catch!

Yeah... It took time to understand why ethernet is not working
sometimes. It appeared that there were race between SDHC and MAC
initialization.

>
>>  	pinctrl-names = "default", "sleep";
>>  	pinctrl-0 = <&sdc2_on>;
>>  	pinctrl-1 = <&sdc2_off>;
>
> These two pinctrl configure "gpio96" for "sd-cd-pins". Yet another wrong
> GPIO? Should probably drop that and add proper pinctrl for the actual
> GPIO in the PMIC. Seems like Qualcomm configured the PMIC GPIO with
> pull-up downstream [1] (not sure if this is the right file). It might be
> redundant if there is an external pull-up on the board, but only the
> schematic could tell that for sure.

If I only had schematic for this board... gpio96 puzzles me as well. I
can understand where wrong GPIO4 come from. But gpio96 origin is
completely unclear. I have user manual for the board, it mention
functions of some GPIOs, but there is nothing about gpio96. Anyways, I
removed it from the DTS (see diff below) and I see no issues with SD card.

> FWIW: Looking closer at the broken commit, the regulator voltage setup
> of &sdhc_2 looks suspicious too. Typically one would want a 1.8V capable
> regulator for the vqmmc-supply to properly use ultra high-speed modes,
> but &vreg_l13c_2p96 seems to be configured with 2.504V-2.960V at the
> moment. On downstream it seems to be 1.8V-2.96V [2] (again, not sure if
> this is the right file). I would recommend re-checking this too and
> testing if UHS cards are detected correctly (if you have the board).

This is actually a good catch. I changed the voltage range to 1.8V-2.96V and
now my card detects in UHS/SDR104 mode. Prior to this change it worked only
in HS mode.

> &vreg_l17a_2p96 has the same 2.504V-2.960V, but has 2.704V-2.960V
> downstream [3]. This is close at least, might be fine as-is (but I'm not
> convinced there is a good reason to differ there).
>

Well, I believe that downstream configuration is more correct, but I
can't prove it, so I'll leave it as is.

Diff for additional changes looks like this:

diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
index 6b08ce246b78c..b2a3496ff68ad 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>;
                };
@@ -386,8 +386,8 @@ &sdhc_2 {
 
        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_on>;
+       pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_off>;
        vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */
        vmmc-supply = <&vreg_l17a_2p96>;  /* Card power line */
        bus-width = <4>;
@@ -505,13 +505,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 +525,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 {
@@ -604,3 +590,25 @@ phy-reset-pins {
                };
        };
 };
+
+&pmm8155au_1_gpios {
+       pmm8155au_1_sdc2_on: pmm8155au_1-sdc2-on-state {
+               sd-cd-pin {
+                       pins = "gpio4";
+                       function = "normal";
+                       input-enable;
+                       bias-pull-up;
+                       power-source = <0>;
+               };
+       };
+
+       pmm8155au_1_sdc2_off: pmm8155au_1-sdc2-off-state {
+               sd-cd-pin {
+                       pins = "gpio4";
+                       function = "normal";
+                       input-enable;
+                       bias-pull-up;
+                       power-source = <0>;
+               };
+       };
+};


I am planning to send v2 tomorrow.

-- 
WBR, Volodymyr

Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2 CD

[Stephan Gerhold]

On Wed, Apr 10, 2024 at 09:57:02PM +0000, Volodymyr Babchuk wrote:
> Stephan Gerhold <stephan@gerhold.net> writes:
> > On Wed, Apr 10, 2024 at 01:41:30PM +0000, Volodymyr Babchuk wrote:
> >> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
> >> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
> >> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
> >> detection.
> >> 
> >> 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 5e4287f8c8cd1..6b08ce246b78c 100644
> >> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> >> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> >> @@ -384,7 +384,7 @@ &remoteproc_cdsp {
> >>  &sdhc_2 {
> >>  	status = "okay";
> >>  
> >> -	cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;
> >> +	cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;
> >
> > Good catch!
> 
> Yeah... It took time to understand why ethernet is not working
> sometimes. It appeared that there were race between SDHC and MAC
> initialization.
> 
> >
> >>  	pinctrl-names = "default", "sleep";
> >>  	pinctrl-0 = <&sdc2_on>;
> >>  	pinctrl-1 = <&sdc2_off>;
> >
> > These two pinctrl configure "gpio96" for "sd-cd-pins". Yet another wrong
> > GPIO? Should probably drop that and add proper pinctrl for the actual
> > GPIO in the PMIC. Seems like Qualcomm configured the PMIC GPIO with
> > pull-up downstream [1] (not sure if this is the right file). It might be
> > redundant if there is an external pull-up on the board, but only the
> > schematic could tell that for sure.
> 
> If I only had schematic for this board... gpio96 puzzles me as well. I
> can understand where wrong GPIO4 come from. But gpio96 origin is
> completely unclear. I have user manual for the board, it mention
> functions of some GPIOs, but there is nothing about gpio96. Anyways, I
> removed it from the DTS (see diff below) and I see no issues with SD card.
> 

I believe this might have been mistakenly copied from some SM8150
example, there you can occasionally find cd-gpios defined as <&tlmm 96>.
E.g. in sm8150-pinctrl.dtsi downstream:

https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/blob/484af352989c912db8f3b6393fc090006029066f/arch/arm64/boot/dts/qcom/sm8150-pinctrl.dtsi#L70-81

> > FWIW: Looking closer at the broken commit, the regulator voltage setup
> > of &sdhc_2 looks suspicious too. Typically one would want a 1.8V capable
> > regulator for the vqmmc-supply to properly use ultra high-speed modes,
> > but &vreg_l13c_2p96 seems to be configured with 2.504V-2.960V at the
> > moment. On downstream it seems to be 1.8V-2.96V [2] (again, not sure if
> > this is the right file). I would recommend re-checking this too and
> > testing if UHS cards are detected correctly (if you have the board).
> 
> This is actually a good catch. I changed the voltage range to 1.8V-2.96V and
> now my card detects in UHS/SDR104 mode. Prior to this change it worked only
> in HS mode.
> 

Yay!

> > &vreg_l17a_2p96 has the same 2.504V-2.960V, but has 2.704V-2.960V
> > downstream [3]. This is close at least, might be fine as-is (but I'm not
> > convinced there is a good reason to differ there).
> >
> 
> Well, I believe that downstream configuration is more correct, but I
> can't prove it, so I'll leave it as is.
> 

Ack.

> Diff for additional changes looks like this:
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> index 6b08ce246b78c..b2a3496ff68ad 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>;
>                 };
> @@ -386,8 +386,8 @@ &sdhc_2 {
>  
>         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_on>;
> +       pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_off>;
>         vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */
>         vmmc-supply = <&vreg_l17a_2p96>;  /* Card power line */
>         bus-width = <4>;
> @@ -505,13 +505,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 +525,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 {
> @@ -604,3 +590,25 @@ phy-reset-pins {
>                 };
>         };
>  };
> +
> +&pmm8155au_1_gpios {
> +       pmm8155au_1_sdc2_on: pmm8155au_1-sdc2-on-state {
> +               sd-cd-pin {
> +                       pins = "gpio4";
> +                       function = "normal";
> +                       input-enable;
> +                       bias-pull-up;
> +                       power-source = <0>;
> +               };
> +       };
> +
> +       pmm8155au_1_sdc2_off: pmm8155au_1-sdc2-off-state {
> +               sd-cd-pin {
> +                       pins = "gpio4";
> +                       function = "normal";
> +                       input-enable;
> +                       bias-pull-up;
> +                       power-source = <0>;
> +               };
> +       };

Since the configuration is the same for both on and off state, you can
combine this into one node and then reference it twice. Also, the single
subnode should not be needed:

	pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd-state {
		pins = "gpio4";
		function = "normal";
		input-enable;
		bias-pull-up;
		power-source = <0>;
	};

&sdhc_2 {
	pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>;
	pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>;
};

Looks good to me otherwise, thanks for cleaning this up!

> 
> I am planning to send v2 tomorrow.
> 

Thanks!
Stephan

[PATCH v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration

[Volodymyr Babchuk]

There are multiple 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.

- L13C voltage regulator limits minimal voltage to 2.504V, which
  prevents use 1.8V to power SD card, which in turns does not allow
  card to work in UHS mode.

This patch fixes all the mentioned issues.

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 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 | 32 +++++++++++-------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
index 5e4287f8c8cd1..b9d56bda96759 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>;
 		};
@@ -384,10 +384,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 +505,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 +525,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 {
@@ -604,3 +590,13 @@ phy-reset-pins {
 		};
 	};
 };
+
+&pmm8155au_1_gpios {
+	pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd {
+			pins = "gpio4";
+			function = "normal";
+			input-enable;
+			bias-pull-up;
+			power-source = <0>;
+	};
+};
-- 
2.44.0

Re: [PATCH v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration

[Krzysztof Kozlowski]

On 11/04/2024 13:55, Volodymyr Babchuk wrote:
> There are multiple 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.
> 
> - L13C voltage regulator limits minimal voltage to 2.504V, which
>   prevents use 1.8V to power SD card, which in turns does not allow
>   card to work in UHS mode.

That's not really related. One issue, one commit.

> 
> This patch fixes all the mentioned issues.
> 
> 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 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 | 32 +++++++++++-------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> index 5e4287f8c8cd1..b9d56bda96759 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>;
>  		};
> @@ -384,10 +384,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 +505,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 +525,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 {
> @@ -604,3 +590,13 @@ phy-reset-pins {
>  		};
>  	};
>  };
> +
> +&pmm8155au_1_gpios {
> +	pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd {

No underscores in node names.

Please also follow tlmm style of naming nodes.

Also does not look like node is placed in alphabetical place. Where did
you put it?

Best regards,
Krzysztof

Re: [PATCH v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration

[Volodymyr Babchuk]

Hi Krzysztof,

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 11/04/2024 13:55, Volodymyr Babchuk wrote:
>> There are multiple 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.
>> 
>> - L13C voltage regulator limits minimal voltage to 2.504V, which
>>   prevents use 1.8V to power SD card, which in turns does not allow
>>   card to work in UHS mode.
>
> That's not really related. One issue, one commit.
>
>> 
>> This patch fixes all the mentioned issues.
>> 
>> 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 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 | 32 +++++++++++-------------
>>  1 file changed, 14 insertions(+), 18 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> index 5e4287f8c8cd1..b9d56bda96759 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>;
>>  		};
>> @@ -384,10 +384,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 +505,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 +525,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 {
>> @@ -604,3 +590,13 @@ phy-reset-pins {
>>  		};
>>  	};
>>  };
>> +
>> +&pmm8155au_1_gpios {
>> +	pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd {
>
> No underscores in node names.

Fill fix.

> Please also follow tlmm style of naming nodes.

Just to be on the same page, will "pmm8155au_1_sdc2_cd: sdc2-cd-pins" be fine?

> Also does not look like node is placed in alphabetical place. Where did
> you put it?

I can't say that the file is sorted in the first place:

# grep "^&" arch/arm64/boot/dts/qcom/sa8155p-adp.dts
&apps_rsc {
&ethernet {
&qupv3_id_1 {
&remoteproc_adsp {
&remoteproc_cdsp {
&sdhc_2 {
&uart2 {
&uart9 {
&ufs_mem_hc {
&ufs_mem_phy {
&usb_1 {
&usb_1_dwc3 {
&usb_1_hsphy {
&usb_1_qmpphy {
&usb_2 {
&usb_2_dwc3 {
&usb_2_hsphy {
&usb_2_qmpphy {
&pcie0 {
&pcie0_phy {
&pcie1_phy {
&tlmm {
&pmm8155au_1_gpios {


So, I can put after &pci1 to have it grouped with other entries that
start with p*, or I can put right after &ethernet to make it appear in
alphabetical order. It is your call.

-- 
WBR, Volodymyr

Re: [PATCH v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration

[Krzysztof Kozlowski]

On 12/04/2024 17:24, Volodymyr Babchuk wrote:
> 
> Hi Krzysztof,
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> 
>> On 11/04/2024 13:55, Volodymyr Babchuk wrote:
>>> There are multiple 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.
>>>
>>> - L13C voltage regulator limits minimal voltage to 2.504V, which
>>>   prevents use 1.8V to power SD card, which in turns does not allow
>>>   card to work in UHS mode.
>>
>> That's not really related. One issue, one commit.
>>
>>>
>>> This patch fixes all the mentioned issues.
>>>
>>> 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 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 | 32 +++++++++++-------------
>>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>> index 5e4287f8c8cd1..b9d56bda96759 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>;
>>>  		};
>>> @@ -384,10 +384,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 +505,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 +525,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 {
>>> @@ -604,3 +590,13 @@ phy-reset-pins {
>>>  		};
>>>  	};
>>>  };
>>> +
>>> +&pmm8155au_1_gpios {
>>> +	pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd {
>>
>> No underscores in node names.
> 
> Fill fix.
> 
>> Please also follow tlmm style of naming nodes.
> 
> Just to be on the same page, will "pmm8155au_1_sdc2_cd: sdc2-cd-pins" be fine?

pins are for sublevel, so you want -state. Just like other pmic GPIOs.

> 
>> Also does not look like node is placed in alphabetical place. Where did
>> you put it?
> 
> I can't say that the file is sorted in the first place:
> 
> # grep "^&" arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> &apps_rsc {
> &ethernet {
> &qupv3_id_1 {
> &remoteproc_adsp {
> &remoteproc_cdsp {
> &sdhc_2 {
> &uart2 {
> &uart9 {
> &ufs_mem_hc {
> &ufs_mem_phy {
> &usb_1 {
> &usb_1_dwc3 {
> &usb_1_hsphy {
> &usb_1_qmpphy {
> &usb_2 {
> &usb_2_dwc3 {
> &usb_2_hsphy {
> &usb_2_qmpphy {

Was sorted till here...

> &pcie0 {
> &pcie0_phy {
> &pcie1_phy {
> &tlmm {

and here second sorting started...

> &pmm8155au_1_gpios {

and you started third.

> 
> 
> So, I can put after &pci1 to have it grouped with other entries that
> start with p*, or I can put right after &ethernet to make it appear in
> alphabetical order. It is your call.

Please put it in the first group, so after ethernet. If this gets ever
sorted, then at least one less move.

Best regards,
Krzysztof

Re: [PATCH v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration

[Stephan Gerhold]

On Thu, Apr 11, 2024 at 11:55:55AM +0000, Volodymyr Babchuk wrote:
> There are multiple 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.
> 
> - L13C voltage regulator limits minimal voltage to 2.504V, which
>   prevents use 1.8V to power SD card, which in turns does not allow
>   card to work in UHS mode.
> 
> This patch fixes all the mentioned issues.
> 
> 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 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 | 32 +++++++++++-------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> index 5e4287f8c8cd1..b9d56bda96759 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>;
>  		};
> @@ -384,10 +384,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 +505,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 +525,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 {
> @@ -604,3 +590,13 @@ phy-reset-pins {
>  		};
>  	};
>  };
> +
> +&pmm8155au_1_gpios {
> +	pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd {
> +			pins = "gpio4";
> +			function = "normal";
> +			input-enable;
> +			bias-pull-up;
> +			power-source = <0>;

Nitpick: There is one indentation level too much here (remove a tab).

Barely worth mentioning, but I guess there will be a v3 to address
Krzysztof's comments. :)

Thanks,
Stephan