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

[Marco Felsch]

On 23-02-14, Frieder Schrempf wrote:
> Hi Marco,
> 
> On 14.02.23 09:10, Marco Felsch wrote:
> > On 23-02-13, Marek Vasut wrote:
> >> On 2/13/23 20:56, Marco Felsch wrote:
> >>> Hi Marek, Frieder,
> >>
> >> Hi,
> >>
> >>> On 23-02-13, Marek Vasut wrote:
> >>>> On 2/13/23 17:15, Marco Felsch wrote:
> >>>>
> >>>> [...]
> >>>>
> >>>>>> @@ -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...
> >>>>>
> >>>>>>    		>;
> >>>>>>    	};
> >>>>>>    };
> >>>>>> 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.
> >>
> >>> 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@xxxxxxx/
> https://lore.kernel.org/lkml/20230213155833.1644366-6-frieder@xxxxxxx/
> 
> Sorry, my scripts didn't cc everyone for all patches, which is probably
> why you missed these changes.

Ah okay this brings light into the dark :) Thanks for the pointers, just
received the DT changes.

Regards,
  Marco