Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

Hi Marco,

On 14.02.23 09:10, Marco Felsch wrote:

On 23-02-13, Marek Vasut wrote:

quoted

On 2/13/23 20:56, Marco Felsch wrote:

quoted

Hi Marek, Frieder,

Hi,

quoted

On 23-02-13, Marek Vasut wrote:

quoted

On 2/13/23 17:15, Marco Felsch wrote:

[...]

quoted

quoted

@@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
   			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
   			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
   			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0

The VSELECT pin should be driven by the (u)sdhc core...

quoted

   		>;
   	};
   };

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
index 5172883717d1..90daaf54e704 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi

@@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
   				regulator-name = "NVCC_SD (LDO5)";
   				regulator-min-microvolt = <1800000>;
   				regulator-max-microvolt = <3300000>;
+				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;

and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
muxed as GPIO, which is not the case. So I think that u-boot have a bug
within the (u)sdhc core.

The trick here is that the VSELECT is operated by the usdhc block as a
function pin, but the PMIC driver can read the current state of the VSELECT
pin by reading out the GPIO block SR register. Since the IOMUX SION bit is
set on the VSELECT pin, the state of the pin is reflected in the GPIO block
SR register even if the pin is muxed as function pin.

Thanks for this explanation :) Why does the regulator driver need to
know the current state of this pin?

Because that regulator has an input pin which selects between two states of
that regulator, L and H, and whatever L or H is depends on what is
configured into the regulator via I2C. To correctly report the state of the
regulator, you have to know the state of that input (selector) pin.

quoted

Since the voltage switching requires
some cmd's before the actual voltage level switch. So this must be
handled within the core.

Also after checking the driver, adding the sd-vsel-gpios will request
the specified gpio as output-high.

The GPIO would have to be requested as input, obviously.

But that isn't the case. According the driver comment they just want to
make sure that this GPIO is high to ensure that the correct regulator
config registers are used.

It seems like you look at the wrong code. We previously had the
sd-vsel-gpios used as you describe, as an output set to a fixed high
level to make sure that the SD_VSEL state matches the driver using the H
register. This code is reverted in patch 3 and patch 5 implements the
sd-vsel-gpios as input as described by Marek. See:

https://lore.kernel.org/lkml/20230213155833.1644366-4-frieder@fris.de/ (local)
https://lore.kernel.org/lkml/20230213155833.1644366-6-frieder@fris.de/ (local)

Sorry, my scripts didn't cc everyone for all patches, which is probably
why you missed these changes.

quoted

quoted

Out of curiosity, what's the bug you
triggering within U-Boot?

AFAICT the readback of the initial state of the regulator (see paragraph
above), which affects Linux all the same.

According the binding the driver should check this and apply the value
to the corresponding L/H register but unfortunately this isn't the case
yet. Does U-Boot handle this correctly?

See above.

Thanks
Frieder

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel