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

[PATCH v2] 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
---
 .../boot/dts/freescale/imx8mm-verdin.dtsi     | 23 +++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
index 7251ad3a0017..9b56a36c5f77 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>;
@@ -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_USDHC2_VSELECT	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 v2] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Francesco Dolcini]

Hello Wojciech,

On Tue, Apr 22, 2025 at 02:46:15PM +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>
> ---
> v1 -> v2: https://lore.kernel.org/all/20250417112012.785420-1-Wojciech.Dubowik@mt.com/
>  - define gpio regulator for LDO5 vin controlled by vselect signal
> ---
>  .../boot/dts/freescale/imx8mm-verdin.dtsi     | 23 +++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> index 7251ad3a0017..9b56a36c5f77 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> @@ -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_USDHC2_VSELECT	0x10>; /* PMIC_USDHC_VSELECT */

This needs to be muxed as GPIO, MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4

Francesco

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

[Frieder Schrempf]

Hi Wojciech,

Am 22.04.25 um 14:46 schrieb Wojciech Dubowik:
> [Sie erhalten nicht häufig E-Mails von wojciech.dubowik@mt.com. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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
> ---
>  .../boot/dts/freescale/imx8mm-verdin.dtsi     | 23 +++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> index 7251ad3a0017..9b56a36c5f77 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>;
> +       };

Please do not describe the SD_VSEL of the PMIC as gpio-regulator. There
already is a regulator node reg_nvcc_sd for the LDO5 of the PMIC.

> +
>         reserved-memory {
>                 #address-cells = <2>;
>                 #size-cells = <2>;
> @@ -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>;

You should reference the reg_nvcc_sd directly here and actually this
should be the only change you need to fix things, no?

>  };
> 
>  &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_USDHC2_VSELECT 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>,

You should keep these pin definitions, but while at it make sure to
enable the SION bit so the PMIC driver can read back the status of the
SD_VSEL signal while the USDHC controller controls it in hardware.

Adding sd-vsel-gpios to the reg_nvcc_sd node makes this work. See [1]
for an example.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8472751c4d96b558d60d0f6aede6b24b64bcb3c9

Thanks
Frieder

>                         <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 v2] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to usdhc2

[Francesco Dolcini]

Hello Frieder,

On Wed, Apr 23, 2025 at 08:50:54AM +0200, Frieder Schrempf wrote:
> Am 22.04.25 um 14:46 schrieb Wojciech Dubowik:
> > [Sie erhalten nicht häufig E-Mails von wojciech.dubowik@mt.com. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > 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
> > ---
> >  .../boot/dts/freescale/imx8mm-verdin.dtsi     | 23 +++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
> > index 7251ad3a0017..9b56a36c5f77 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>;
> > +       };
> 
> Please do not describe the SD_VSEL of the PMIC as gpio-regulator. There
> already is a regulator node reg_nvcc_sd for the LDO5 of the PMIC.
> 
> > +
> >         reserved-memory {
> >                 #address-cells = <2>;
> >                 #size-cells = <2>;
> > @@ -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>;
> 
> You should reference the reg_nvcc_sd directly here and actually this
> should be the only change you need to fix things, no?

If you just do this change you end-up in the situation I described in
the v1 version of this patch
https://lore.kernel.org/all/20250417130342.GA18817@francesco-nb/

With the IO being driven by the SDHCI core, while the linux driver
changes the voltage over i2c.

I was not aware of this sd-vsel-gpios, that if I understand correctly
should handle the concern I raised initially, having the PMIC driver
aware of this GPIO, however I do not see why that solution should be
better than this one.

BTW, is this solution safe from any kind of race condition? You have
this IO driven by the SDHCI IP, and the I2C communication to the PMIC
driven by the mmc driver, with the PMIC driver just reading this GPIO
once when changing/reading the voltage.

With this solution (that I proposed), the sdcard driver just use the
GPIO to select the right voltage and that's it, simple, no un-needed i2c
communication with the PMIC, and the DT clearly describe the way the HW
is designed.

Francesco

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

[Frieder Schrempf]

Hi Francesco,

Am 23.04.25 um 09:08 schrieb Francesco Dolcini:
> Hello Frieder,
> 
> On Wed, Apr 23, 2025 at 08:50:54AM +0200, Frieder Schrempf wrote:
>> Am 22.04.25 um 14:46 schrieb 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
>>> ---
>>>  .../boot/dts/freescale/imx8mm-verdin.dtsi     | 23 +++++++++++++++----
>>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi
>>> index 7251ad3a0017..9b56a36c5f77 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>;
>>> +       };
>>
>> Please do not describe the SD_VSEL of the PMIC as gpio-regulator. There
>> already is a regulator node reg_nvcc_sd for the LDO5 of the PMIC.
>>
>>> +
>>>         reserved-memory {
>>>                 #address-cells = <2>;
>>>                 #size-cells = <2>;
>>> @@ -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>;
>>
>> You should reference the reg_nvcc_sd directly here and actually this
>> should be the only change you need to fix things, no?
> 
> If you just do this change you end-up in the situation I described in
> the v1 version of this patch
> https://lore.kernel.org/all/20250417130342.GA18817@francesco-nb/

Thanks, I missed that discussion.

> 
> With the IO being driven by the SDHCI core, while the linux driver
> changes the voltage over i2c.
> 
> I was not aware of this sd-vsel-gpios, that if I understand correctly
> should handle the concern I raised initially, having the PMIC driver
> aware of this GPIO, however I do not see why that solution should be
> better than this one.

See below, but I'm not totally convinced either. Your solution didn't
occur to me. I came up with this series instead that is now in mainline:

https://patchwork.kernel.org/project/linux-arm-kernel/cover/20241218152842.97483-1-frieder@fris.de/

> 
> BTW, is this solution safe from any kind of race condition? You have
> this IO driven by the SDHCI IP, and the I2C communication to the PMIC
> driven by the mmc driver, with the PMIC driver just reading this GPIO
> once when changing/reading the voltage.

Actually the VSELECT is driven by the USDHC controller, but as far as I
understand control is still in the hands of the MMC driver using
sdhci_start_signal_voltage_switch() and ESDHC_VENDOR_SPEC_VSELECT.

So we actually have control over this and therefore race conditions
between SW and HW shouldn't be a problem.

> 
> With this solution (that I proposed), the sdcard driver just use the
> GPIO to select the right voltage and that's it, simple, no un-needed i2c
> communication with the PMIC, and the DT clearly describe the way the HW
> is designed.

Yes, but your solution relies on the fact that the LDO5 registers
actually have the correct values for 1v8 and 3v3 setup. The bootloader
might have changed these values. I would prefer it if we could have a
solution that puts the LDO5 in a defined state, that is independent from
any external conditions.

Also I'm not sure if two regulator nodes is a "correct" or "good"
description of what is actually a single regulator in hardware. Although
I see your point of describing the control registers and the IO on two
different stages. The problem is the dependency between them. A simple
reference is not really enough. To fully describe it, the GPIO regulator
would have to fetch its voltage setpoints from the LDO5 registers at
runtime.

Best regards
Frieder

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

[Francesco Dolcini]

On Wed, Apr 23, 2025 at 10:00:22AM +0200, Frieder Schrempf wrote:
> Am 23.04.25 um 09:08 schrieb Francesco Dolcini:
> > On Wed, Apr 23, 2025 at 08:50:54AM +0200, Frieder Schrempf wrote:
> >> Am 22.04.25 um 14:46 schrieb 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>

...

> > With this solution (that I proposed), the sdcard driver just use the
> > GPIO to select the right voltage and that's it, simple, no un-needed i2c
> > communication with the PMIC, and the DT clearly describe the way the HW
> > is designed.
> 
> Yes, but your solution relies on the fact that the LDO5 registers
> actually have the correct values for 1v8 and 3v3 setup. The bootloader
> might have changed these values. I would prefer it if we could have a
> solution that puts the LDO5 in a defined state, that is independent from
> any external conditions.

I do not think this is a real concern, the PMIC is programmed during
manufacturing, if the PMIC programming is not correct we have way more
issues ...

Francesco

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

[Frieder Schrempf]

Am 23.04.25 um 12:26 schrieb Francesco Dolcini:
> On Wed, Apr 23, 2025 at 10:00:22AM +0200, Frieder Schrempf wrote:
>> Am 23.04.25 um 09:08 schrieb Francesco Dolcini:
>>> On Wed, Apr 23, 2025 at 08:50:54AM +0200, Frieder Schrempf wrote:
>>>> Am 22.04.25 um 14:46 schrieb 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>
> 
> ...
> 
>>> With this solution (that I proposed), the sdcard driver just use the
>>> GPIO to select the right voltage and that's it, simple, no un-needed i2c
>>> communication with the PMIC, and the DT clearly describe the way the HW
>>> is designed.
>>
>> Yes, but your solution relies on the fact that the LDO5 registers
>> actually have the correct values for 1v8 and 3v3 setup. The bootloader
>> might have changed these values. I would prefer it if we could have a
>> solution that puts the LDO5 in a defined state, that is independent from
>> any external conditions.
> 
> I do not think this is a real concern, the PMIC is programmed during
> manufacturing, if the PMIC programming is not correct we have way more
> issues ...

My point is not about the PMIC having wrong values as factory defaults.
My point is about different bootloaders that have PMIC drivers which
also use a mix of the SD_VSEL IO and the configuration registers for
setting the voltage. We don't know how the bootloader will leave the
register values behind.

An example would be that the bootloader uses SD_VSEL in a different way
and the PMIC driver in the bootloader writes 1v8 to the LDO5CTRL_L
register. Linux will then use the wrong voltage and the SD card will not
work.

So with your approach it would be good if the PMIC driver would also
reset the LDO5 registers to their factory defaults at probe time.

Also the logic for the LDO5 is purely embedded in the PMIC chip, so it
feels kind of wrong to have another regulator for SD_VSEL on the board
level.

If someone wants to check the output voltage of LDO5, they will query
the sysfs path for LDO5 and get back the wrong voltage. It will be hard
to find out that you need to read the voltage of the additional GPIO
regulator.

I don't think your approach is bad and of course you are free to move on
and use it. I'm just trying to find out what would be the best way for
everyone. It would be good to use the same approach on all i.MX8M
boards. Currently we have a mix of (at least):

1. MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT without sd-vsel-gpios readback
(everyone)
2. MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT with sd-vsel-gpios readback
(Kontron)
3. MX8MM_IOMUXC_GPIO1_IO04_GPIO1_IO4 with additional GPIO regulator
(Toradex)