[PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Wojciech Dubowik]

Define vqmmc regulator-gpio for usdhc2 with vin-supply
coming from LDO5.

Without this definition LDO5 will be powered down, disabling
SD card after bootup. This has been introduced in commit
f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5").

Fixes: f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5")

Cc: stable@vger.kernel.org
Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
v1 -> v2: https://lore.kernel.org/all/20250417112012.785420-1-Wojciech.Dubowik@mt.com/
 - define gpio regulator for LDO5 vin controlled by vselect signal
v2 -> v3: https://lore.kernel.org/all/20250422130127.GA238494@francesco-nb/
 - specify vselect as gpio
---
 .../boot/dts/freescale/imx8mm-verdin.dtsi     | 25 +++++++++++++++----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
index 7251ad3a0017..b46566f3ce20 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
@@ -144,6 +144,19 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
 		startup-delay-us = <20000>;
 	};
 
+	reg_usdhc2_vqmmc: regulator-usdhc2-vqmmc {
+		compatible = "regulator-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_usdhc2_vsel>;
+		gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+		regulator-max-microvolt = <3300000>;
+		regulator-min-microvolt = <1800000>;
+		states = <1800000 0x1>,
+			 <3300000 0x0>;
+		regulator-name = "PMIC_USDHC_VSELECT";
+		vin-supply = <&reg_nvcc_sd>;
+	};
+
 	reserved-memory {
 		#address-cells = <2>;
 		#size-cells = <2>;
@@ -269,7 +282,7 @@ &gpio1 {
 			  "SODIMM_19",
 			  "",
 			  "",
-			  "",
+			  "PMIC_USDHC_VSELECT",
 			  "",
 			  "",
 			  "",
@@ -785,6 +798,7 @@ &usdhc2 {
 	pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_cd>;
 	pinctrl-3 = <&pinctrl_usdhc2_sleep>, <&pinctrl_usdhc2_cd_sleep>;
 	vmmc-supply = <&reg_usdhc2_vmmc>;
+	vqmmc-supply = <&reg_usdhc2_vqmmc>;
 };
 
 &wdog1 {
@@ -1206,13 +1220,17 @@ pinctrl_usdhc2_pwr_en: usdhc2pwrengrp {
 			<MX8MM_IOMUXC_NAND_CLE_GPIO3_IO5		0x6>;	/* SODIMM 76 */
 	};
 
+	pinctrl_usdhc2_vsel: usdhc2vselgrp {
+		fsl,pins =
+			<MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4	0x10>; /* PMIC_USDHC_VSELECT */
+	};
+
 	/*
 	 * Note: Due to ERR050080 we use discrete external on-module resistors pulling-up to the
 	 * on-module +V3.3_1.8_SD (LDO5) rail and explicitly disable the internal pull-ups here.
 	 */
 	pinctrl_usdhc2: usdhc2grp {
 		fsl,pins =
-			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x10>,
 			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x90>,	/* SODIMM 78 */
 			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x90>,	/* SODIMM 74 */
 			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x90>,	/* SODIMM 80 */
@@ -1223,7 +1241,6 @@ pinctrl_usdhc2: usdhc2grp {
 
 	pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
 		fsl,pins =
-			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x10>,
 			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x94>,
 			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x94>,
 			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x94>,
@@ -1234,7 +1251,6 @@ pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
 
 	pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
 		fsl,pins =
-			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x10>,
 			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x96>,
 			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x96>,
 			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x96>,
@@ -1246,7 +1262,6 @@ pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
 	/* Avoid backfeeding with removed card power */
 	pinctrl_usdhc2_sleep: usdhc2slpgrp {
 		fsl,pins =
-			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x0>,
 			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x0>,
 			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x0>,
 			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x0>,
-- 
2.47.2

Re: [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Manuel Traut]

On Tue, Apr 22, 2025 at 04:01:57PM +0200, Wojciech Dubowik wrote:
> Define vqmmc regulator-gpio for usdhc2 with vin-supply
> coming from LDO5.
> 
> Without this definition LDO5 will be powered down, disabling
> SD card after bootup. This has been introduced in commit
> f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5").
> 
> Fixes: f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5")

Tested on 6.15-rc2. SDCard stays present now. It also fixes the issue
for >=6.12.23

> Cc: stable@vger.kernel.org
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
Tested-by: Manuel Traut <manuel.traut@mt.com>

> ---
> v1 -> v2: https://lore.kernel.org/all/20250417112012.785420-1-Wojciech.Dubowik@mt.com/
>  - define gpio regulator for LDO5 vin controlled by vselect signal
> v2 -> v3: https://lore.kernel.org/all/20250422130127.GA238494@francesco-nb/
>  - specify vselect as gpio
> ---
>  .../boot/dts/freescale/imx8mm-verdin.dtsi     | 25 +++++++++++++++----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> index 7251ad3a0017..b46566f3ce20 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> @@ -144,6 +144,19 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
>  		startup-delay-us = <20000>;
>  	};
>  
> +	reg_usdhc2_vqmmc: regulator-usdhc2-vqmmc {
> +		compatible = "regulator-gpio";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_usdhc2_vsel>;
> +		gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-min-microvolt = <1800000>;
> +		states = <1800000 0x1>,
> +			 <3300000 0x0>;
> +		regulator-name = "PMIC_USDHC_VSELECT";
> +		vin-supply = <&reg_nvcc_sd>;
> +	};
> +
>  	reserved-memory {
>  		#address-cells = <2>;
>  		#size-cells = <2>;
> @@ -269,7 +282,7 @@ &gpio1 {
>  			  "SODIMM_19",
>  			  "",
>  			  "",
> -			  "",
> +			  "PMIC_USDHC_VSELECT",
>  			  "",
>  			  "",
>  			  "",
> @@ -785,6 +798,7 @@ &usdhc2 {
>  	pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_cd>;
>  	pinctrl-3 = <&pinctrl_usdhc2_sleep>, <&pinctrl_usdhc2_cd_sleep>;
>  	vmmc-supply = <&reg_usdhc2_vmmc>;
> +	vqmmc-supply = <&reg_usdhc2_vqmmc>;
>  };
>  
>  &wdog1 {
> @@ -1206,13 +1220,17 @@ pinctrl_usdhc2_pwr_en: usdhc2pwrengrp {
>  			<MX8MM_IOMUXC_NAND_CLE_GPIO3_IO5		0x6>;	/* SODIMM 76 */
>  	};
>  
> +	pinctrl_usdhc2_vsel: usdhc2vselgrp {
> +		fsl,pins =
> +			<MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4	0x10>; /* PMIC_USDHC_VSELECT */
> +	};
> +
>  	/*
>  	 * Note: Due to ERR050080 we use discrete external on-module resistors pulling-up to the
>  	 * on-module +V3.3_1.8_SD (LDO5) rail and explicitly disable the internal pull-ups here.
>  	 */
>  	pinctrl_usdhc2: usdhc2grp {
>  		fsl,pins =
> -			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x10>,
>  			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x90>,	/* SODIMM 78 */
>  			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x90>,	/* SODIMM 74 */
>  			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x90>,	/* SODIMM 80 */
> @@ -1223,7 +1241,6 @@ pinctrl_usdhc2: usdhc2grp {
>  
>  	pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
>  		fsl,pins =
> -			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x10>,
>  			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x94>,
>  			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x94>,
>  			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x94>,
> @@ -1234,7 +1251,6 @@ pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
>  
>  	pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
>  		fsl,pins =
> -			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x10>,
>  			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x96>,
>  			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x96>,
>  			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x96>,
> @@ -1246,7 +1262,6 @@ pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
>  	/* Avoid backfeeding with removed card power */
>  	pinctrl_usdhc2_sleep: usdhc2slpgrp {
>  		fsl,pins =
> -			<MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x0>,
>  			<MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK		0x0>,
>  			<MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD		0x0>,
>  			<MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0		0x0>,
> -- 
> 2.47.2
> 

Re: [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Francesco Dolcini]

On Tue, Apr 22, 2025 at 04:01:57PM +0200, Wojciech Dubowik wrote:
> Define vqmmc regulator-gpio for usdhc2 with vin-supply
> coming from LDO5.
> 
> Without this definition LDO5 will be powered down, disabling
> SD card after bootup. This has been introduced in commit
> f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5").
> 
> Fixes: f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5")
> 
no empty lines in between commit message tags, not sure if Shawn can fix
this up or you need to send a v4.

> Cc: stable@vger.kernel.org
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>

Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>

I would backport this to also older kernel, so to me

Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for verdin imx8m mini")

Francesco

Re: [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Philippe Schenker]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]



On Wed, 2025-04-23 at 11:53 +0200, Francesco Dolcini wrote:
> On Tue, Apr 22, 2025 at 04:01:57PM +0200, Wojciech Dubowik wrote:
> > Define vqmmc regulator-gpio for usdhc2 with vin-supply
> > coming from LDO5.
> > 
> > Without this definition LDO5 will be powered down, disabling
> > SD card after bootup. This has been introduced in commit
> > f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5").
> > 
> > Fixes: f5aab0438ef1 ("regulator: pca9450: Fix enable register for
> > LDO5")
> > 
> no empty lines in between commit message tags, not sure if Shawn can
> fix
> this up or you need to send a v4.
> 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> 
> Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> I would backport this to also older kernel, so to me
> 
> Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for
> verdin imx8m mini")

NACK for the proposed Fixes, this introduces a new Kconfig which could
have side-effects in users of current stable kernels.
> 
> Francesco

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 717 bytes --]

Re: [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Francesco Dolcini]

On Wed, Apr 23, 2025 at 10:16:43AM +0000, Philippe Schenker wrote:
> 
> 
> On Wed, 2025-04-23 at 11:53 +0200, Francesco Dolcini wrote:
> > On Tue, Apr 22, 2025 at 04:01:57PM +0200, Wojciech Dubowik wrote:
> > > Define vqmmc regulator-gpio for usdhc2 with vin-supply
> > > coming from LDO5.
> > > 
> > > Without this definition LDO5 will be powered down, disabling
> > > SD card after bootup. This has been introduced in commit
> > > f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5").
> > > 
> > > Fixes: f5aab0438ef1 ("regulator: pca9450: Fix enable register for
> > > LDO5")
> > > 
> > no empty lines in between commit message tags, not sure if Shawn can
> > fix
> > this up or you need to send a v4.
> > 
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> > 
> > Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > I would backport this to also older kernel, so to me
> > 
> > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for
> > verdin imx8m mini")
> 
> NACK for the proposed Fixes, this introduces a new Kconfig which could
> have side-effects in users of current stable kernels.

The driver for "regulator-gpio" compatible? I do not agree with your argument,
sorry. 

The previous description was not correct. There was an unused
regulator in the DT that was not switched off just by chance.

Francesco

Re: [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Philippe Schenker]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On Wed, 2025-04-23 at 12:21 +0200, Francesco Dolcini wrote:
> > > 
> > > I would backport this to also older kernel, so to me
> > > 
> > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support
> > > for
> > > verdin imx8m mini")
> > 
> > NACK for the proposed Fixes, this introduces a new Kconfig which
> > could
> > have side-effects in users of current stable kernels.
> 
> The driver for "regulator-gpio" compatible? I do not agree with your
> argument,
> sorry. 
> 
> The previous description was not correct. There was an unused
> regulator in the DT that was not switched off just by chance.
> 
> Francesco
> 
My previous reasoning about the driver is one point. The other is that
the initial implementation in 6a57f224f734 ("arm64: dts: freescale: add
initial support for verdin imx8m mini") was not wrong at all it was
just different.

My concern is for existing users of stable kernels that you change the
underlying implementation of how the SD voltage gets switched. And
adding the tag


Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for
verdin imx8m mini")

to this patch would get this new implementation also to stable kernels
not affected by the issue introduced in f5aab0438ef1 ("regulator:
pca9450: Fix enable register for LDO5")

Philippe

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 717 bytes --]

Re: EXTERNAL - [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Wojciech Dubowik]

On Wed, Apr 23, 2025 at 11:23:09AM +0000, Philippe Schenker wrote:
> On Wed, 2025-04-23 at 12:21 +0200, Francesco Dolcini wrote:
> > > > 
> > > > I would backport this to also older kernel, so to me
> > > > 
> > > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support
> > > > for
> > > > verdin imx8m mini")
> > > 
> > > NACK for the proposed Fixes, this introduces a new Kconfig which
> > > could
> > > have side-effects in users of current stable kernels.
> > 
> > The driver for "regulator-gpio" compatible? I do not agree with your
> > argument,
> > sorry. 
> > 
> > The previous description was not correct. There was an unused
> > regulator in the DT that was not switched off just by chance.
> > 
> > Francesco
> > 
> My previous reasoning about the driver is one point. The other is that
> the initial implementation in 6a57f224f734 ("arm64: dts: freescale: add
> initial support for verdin imx8m mini") was not wrong at all it was
> just different.
> 
> My concern is for existing users of stable kernels that you change the
> underlying implementation of how the SD voltage gets switched. And
> adding the tag
> 
> 
> Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for
> verdin imx8m mini")
> 
> to this patch would get this new implementation also to stable kernels
> not affected by the issue introduced in f5aab0438ef1 ("regulator:
> pca9450: Fix enable register for LDO5")

I will wait a day and send V4 with what I beleive was result of this
discussion.

Wojtek

> 
> Philippe

Re: EXTERNAL - [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Francesco Dolcini]

On Thu, Apr 24, 2025 at 08:16:11AM +0200, Wojciech Dubowik wrote:
> On Wed, Apr 23, 2025 at 11:23:09AM +0000, Philippe Schenker wrote:
> > On Wed, 2025-04-23 at 12:21 +0200, Francesco Dolcini wrote:
> > > > > 
> > > > > I would backport this to also older kernel, so to me
> > > > > 
> > > > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support
> > > > > for
> > > > > verdin imx8m mini")
> > > > 
> > > > NACK for the proposed Fixes, this introduces a new Kconfig which
> > > > could
> > > > have side-effects in users of current stable kernels.
> > > 
> > > The driver for "regulator-gpio" compatible? I do not agree with your
> > > argument,
> > > sorry. 
> > > 
> > > The previous description was not correct. There was an unused
> > > regulator in the DT that was not switched off just by chance.
> > > 
> > > Francesco
> > > 
> > My previous reasoning about the driver is one point. The other is that
> > the initial implementation in 6a57f224f734 ("arm64: dts: freescale: add
> > initial support for verdin imx8m mini") was not wrong at all it was
> > just different.
> > 
> > My concern is for existing users of stable kernels that you change the
> > underlying implementation of how the SD voltage gets switched. And
> > adding the tag
> > 
> > 
> > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for
> > verdin imx8m mini")
> > 
> > to this patch would get this new implementation also to stable kernels
> > not affected by the issue introduced in f5aab0438ef1 ("regulator:
> > pca9450: Fix enable register for LDO5")
> 
> I will wait a day and send V4 with what I beleive was result of this
> discussion.

My opinion is that we should backport this fix. The DT description was
wrong, that change on the PMIC driver just made the issue visible,
the DT is about the HW description.

FWIW, I tested this change with both v6.1 and v6.6 and it works correctly, as
expected.

Francesco

Re: EXTERNAL - [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Philippe Schenker]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]



On Thu, 2025-04-24 at 08:16 +0200, Wojciech Dubowik wrote:
> On Wed, Apr 23, 2025 at 11:23:09AM +0000, Philippe Schenker wrote:
> > On Wed, 2025-04-23 at 12:21 +0200, Francesco Dolcini wrote:
> > > > > 
> > > > > I would backport this to also older kernel, so to me
> > > > > 
> > > > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial
> > > > > support
> > > > > for
> > > > > verdin imx8m mini")
> > > > 
> > > > NACK for the proposed Fixes, this introduces a new Kconfig
> > > > which
> > > > could
> > > > have side-effects in users of current stable kernels.
> > > 
> > > The driver for "regulator-gpio" compatible? I do not agree with
> > > your
> > > argument,
> > > sorry. 
> > > 
> > > The previous description was not correct. There was an unused
> > > regulator in the DT that was not switched off just by chance.
> > > 
> > > Francesco
> > > 
> > My previous reasoning about the driver is one point. The other is
> > that
> > the initial implementation in 6a57f224f734 ("arm64: dts: freescale:
> > add
> > initial support for verdin imx8m mini") was not wrong at all it was
> > just different.
> > 
> > My concern is for existing users of stable kernels that you change
> > the
> > underlying implementation of how the SD voltage gets switched. And
> > adding the tag
> > 
> > 
> > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support
> > for
> > verdin imx8m mini")
> > 
> > to this patch would get this new implementation also to stable
> > kernels
> > not affected by the issue introduced in f5aab0438ef1 ("regulator:
> > pca9450: Fix enable register for LDO5")
> 
> I will wait a day and send V4 with what I beleive was result of this
> discussion.

Sorry if it was confusing. I'm perfectly fine with your v3. I was just
not agreeing with the Fixes-Tag proposed by Francesco.

> 
> Wojtek
> 
> > 
> > Philippe
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 717 bytes --]

Re: EXTERNAL - [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Philippe Schenker]

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]



On Thu, 2025-04-24 at 09:21 +0200, Francesco Dolcini wrote:
> On Thu, Apr 24, 2025 at 08:16:11AM +0200, Wojciech Dubowik wrote:
> > On Wed, Apr 23, 2025 at 11:23:09AM +0000, Philippe Schenker wrote:
> > > On Wed, 2025-04-23 at 12:21 +0200, Francesco Dolcini wrote:
> > > > > > 
> > > > > > I would backport this to also older kernel, so to me
> > > > > > 
> > > > > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial
> > > > > > support
> > > > > > for
> > > > > > verdin imx8m mini")
> > > > > 
> > > > > NACK for the proposed Fixes, this introduces a new Kconfig
> > > > > which
> > > > > could
> > > > > have side-effects in users of current stable kernels.
> > > > 
> > > > The driver for "regulator-gpio" compatible? I do not agree with
> > > > your
> > > > argument,
> > > > sorry. 
> > > > 
> > > > The previous description was not correct. There was an unused
> > > > regulator in the DT that was not switched off just by chance.
> > > > 
> > > > Francesco
> > > > 
> > > My previous reasoning about the driver is one point. The other is
> > > that
> > > the initial implementation in 6a57f224f734 ("arm64: dts:
> > > freescale: add
> > > initial support for verdin imx8m mini") was not wrong at all it
> > > was
> > > just different.
> > > 
> > > My concern is for existing users of stable kernels that you
> > > change the
> > > underlying implementation of how the SD voltage gets switched.
> > > And
> > > adding the tag
> > > 
> > > 
> > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support
> > > for
> > > verdin imx8m mini")
> > > 
> > > to this patch would get this new implementation also to stable
> > > kernels
> > > not affected by the issue introduced in f5aab0438ef1 ("regulator:
> > > pca9450: Fix enable register for LDO5")
> > 
> > I will wait a day and send V4 with what I beleive was result of
> > this
> > discussion.
> 
> My opinion is that we should backport this fix. The DT description
> was
> wrong, that change on the PMIC driver just made the issue visible,
> the DT is about the HW description.

From what I understand it was not wrong but it became wrong with the
PMIC driver changes.

My wish in general is that we are very careful of what is backported to
stable kernels. I do not want to edit my device-trees that are running
just fine. I agree and think it's good this is going into mainline but
I still do not agree on backporting it to every stable kernel the
Verdin iMX8M Mini is in.

Philippe

> 
> FWIW, I tested this change with both v6.1 and v6.6 and it works
> correctly, as
> expected.
> 
> Francesco

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 717 bytes --]

Re: [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Philippe Schenker]

On Tue, 2025-04-22 at 16:01 +0200, Wojciech Dubowik wrote:
> Define vqmmc regulator-gpio for usdhc2 with vin-supply
> coming from LDO5.
> 
> Without this definition LDO5 will be powered down, disabling
> SD card after bootup. This has been introduced in commit
> f5aab0438ef1 ("regulator: pca9450: Fix enable register for LDO5").
> 
> Fixes: f5aab0438ef1 ("regulator: pca9450: Fix enable register for
> LDO5")
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>

Reviewed-by: Philippe Schenker <philippe.schenker@impulsing.ch>

> ---
> v1 -> v2: https://lore.kernel.org/all/20250417112012.785420-1-
> Wojciech.Dubowik@mt.com/
>  - define gpio regulator for LDO5 vin controlled by vselect signal
> v2 -> v3:
> https://lore.kernel.org/all/20250422130127.GA238494@francesco-nb/
>  - specify vselect as gpio
> ---
>  .../boot/dts/freescale/imx8mm-verdin.dtsi     | 25 +++++++++++++++--
> --
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> index 7251ad3a0017..b46566f3ce20 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> @@ -144,6 +144,19 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
>   startup-delay-us = <20000>;
>   };
>  
> + reg_usdhc2_vqmmc: regulator-usdhc2-vqmmc {
> + compatible = "regulator-gpio";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usdhc2_vsel>;
> + gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> + regulator-max-microvolt = <3300000>;
> + regulator-min-microvolt = <1800000>;
> + states = <1800000 0x1>,
> + <3300000 0x0>;
> + regulator-name = "PMIC_USDHC_VSELECT";
> + vin-supply = <&reg_nvcc_sd>;
> + };
> +
>   reserved-memory {
>   #address-cells = <2>;
>   #size-cells = <2>;
> @@ -269,7 +282,7 @@ &gpio1 {
>     "SODIMM_19",
>     "",
>     "",
> -   "",
> +   "PMIC_USDHC_VSELECT",
>     "",
>     "",
>     "",
> @@ -785,6 +798,7 @@ &usdhc2 {
>   pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_cd>;
>   pinctrl-3 = <&pinctrl_usdhc2_sleep>, <&pinctrl_usdhc2_cd_sleep>;
>   vmmc-supply = <&reg_usdhc2_vmmc>;
> + vqmmc-supply = <&reg_usdhc2_vqmmc>;
>  };
>  
>  &wdog1 {
> @@ -1206,13 +1220,17 @@ pinctrl_usdhc2_pwr_en: usdhc2pwrengrp {
>   <MX8MM_IOMUXC_NAND_CLE_GPIO3_IO5 0x6>; /* SODIMM 76 */
>   };
>  
> + pinctrl_usdhc2_vsel: usdhc2vselgrp {
> + fsl,pins =
> + <MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4 0x10>; /* PMIC_USDHC_VSELECT */
> + };
> +
>   /*
>   * Note: Due to ERR050080 we use discrete external on-module
> resistors pulling-up to the
>   * on-module +V3.3_1.8_SD (LDO5) rail and explicitly disable the
> internal pull-ups here.
>   */
>   pinctrl_usdhc2: usdhc2grp {
>   fsl,pins =
> - <MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x10>,
>   <MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK 0x90>, /* SODIMM 78 */
>   <MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD 0x90>, /* SODIMM 74 */
>   <MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0 0x90>, /* SODIMM 80 */
> @@ -1223,7 +1241,6 @@ pinctrl_usdhc2: usdhc2grp {
>  
>   pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
>   fsl,pins =
> - <MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x10>,
>   <MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK 0x94>,
>   <MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD 0x94>,
>   <MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0 0x94>,
> @@ -1234,7 +1251,6 @@ pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
>  
>   pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
>   fsl,pins =
> - <MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x10>,
>   <MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK 0x96>,
>   <MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD 0x96>,
>   <MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0 0x96>,
> @@ -1246,7 +1262,6 @@ pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
>   /* Avoid backfeeding with removed card power */
>   pinctrl_usdhc2_sleep: usdhc2slpgrp {
>   fsl,pins =
> - <MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x0>,
>   <MX8MM_IOMUXC_SD2_CLK_USDHC2_CLK 0x0>,
>   <MX8MM_IOMUXC_SD2_CMD_USDHC2_CMD 0x0>,
>   <MX8MM_IOMUXC_SD2_DATA0_USDHC2_DATA0 0x0>,

Re: EXTERNAL - [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Francesco Dolcini]

On Thu, Apr 24, 2025 at 08:20:32AM +0000, Philippe Schenker wrote:
> 
> 
> On Thu, 2025-04-24 at 09:21 +0200, Francesco Dolcini wrote:
> > On Thu, Apr 24, 2025 at 08:16:11AM +0200, Wojciech Dubowik wrote:
> > > On Wed, Apr 23, 2025 at 11:23:09AM +0000, Philippe Schenker wrote:
> > > > On Wed, 2025-04-23 at 12:21 +0200, Francesco Dolcini wrote:
> > > > > > > 
> > > > > > > I would backport this to also older kernel, so to me
> > > > > > > 
> > > > > > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial
> > > > > > > support
> > > > > > > for
> > > > > > > verdin imx8m mini")
> > > > > > 
> > > > > > NACK for the proposed Fixes, this introduces a new Kconfig
> > > > > > which
> > > > > > could
> > > > > > have side-effects in users of current stable kernels.
> > > > > 
> > > > > The driver for "regulator-gpio" compatible? I do not agree with
> > > > > your
> > > > > argument,
> > > > > sorry. 
> > > > > 
> > > > > The previous description was not correct. There was an unused
> > > > > regulator in the DT that was not switched off just by chance.
> > > > > 
> > > > > Francesco
> > > > > 
> > > > My previous reasoning about the driver is one point. The other is
> > > > that
> > > > the initial implementation in 6a57f224f734 ("arm64: dts:
> > > > freescale: add
> > > > initial support for verdin imx8m mini") was not wrong at all it
> > > > was
> > > > just different.
> > > > 
> > > > My concern is for existing users of stable kernels that you
> > > > change the
> > > > underlying implementation of how the SD voltage gets switched.
> > > > And
> > > > adding the tag
> > > > 
> > > > 
> > > > Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support
> > > > for
> > > > verdin imx8m mini")
> > > > 
> > > > to this patch would get this new implementation also to stable
> > > > kernels
> > > > not affected by the issue introduced in f5aab0438ef1 ("regulator:
> > > > pca9450: Fix enable register for LDO5")
> > > 
> > > I will wait a day and send V4 with what I beleive was result of
> > > this
> > > discussion.
> > 
> > My opinion is that we should backport this fix. The DT description
> > was
> > wrong, that change on the PMIC driver just made the issue visible,
> > the DT is about the HW description.
> 
> From what I understand it was not wrong but it became wrong with the
> PMIC driver changes.

It was wrong. You had a regulator described as not used, and therefore the OS
was free to switch it off. For a bug in the driver this regulator was not
switched off by the old kernels.

The DT description was never correct.

With that said, hopefully it makes no difference in pratice,
f5aab0438ef17f01c5ecd25e61ae6a03f82a4586 will be backported, and therefore also
any fix to it should.

If for some unfortunate reason we do not backport also this one, you'll get a
non-working SD card on your LTS kernel.

Francesco

Re: EXTERNAL - [PATCH v3] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Philippe Schenker]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Thu, 2025-04-24 at 10:34 +0200, Francesco Dolcini wrote:
> > > 
> > > My opinion is that we should backport this fix. The DT
> > > description
> > > was
> > > wrong, that change on the PMIC driver just made the issue
> > > visible,
> > > the DT is about the HW description.
> > 
> > From what I understand it was not wrong but it became wrong with
> > the
> > PMIC driver changes.
> 
> It was wrong. You had a regulator described as not used, and
> therefore the OS
> was free to switch it off. For a bug in the driver this regulator was
> not
> switched off by the old kernels.
> 
> The DT description was never correct.
> 
> With that said, hopefully it makes no difference in pratice,
> f5aab0438ef17f01c5ecd25e61ae6a03f82a4586 will be backported, and
> therefore also
> any fix to it should.

Aah I overlooked the Fixes: tag there. Yes in this case we need your
Fixes tag, you are of course correct as always.

@Wojciech, please send a v4 with:

Fixes: 6a57f224f734 ("arm64: dts: freescale: add initial support for
verdin imx8m mini")

Philippe

> 
> If for some unfortunate reason we do not backport also this one,
> you'll get a
> non-working SD card on your LTS kernel.
> 
> Francesco

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 717 bytes --]