Message-ID: <17ec22a0-b68b-4ac5-b2bc-986837639a37@kontron.de>
Date: Wed, 23 Apr 2025 10:00:22 +0200
From: Frieder Schrempf <frieder.schrempf@...tron.de>
To: Francesco Dolcini <francesco@...cini.it>
Cc: Wojciech Dubowik <Wojciech.Dubowik@...com>, linux-kernel@...r.kernel.org,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 Philippe Schenker <philippe.schenker@...ulsing.ch>, stable@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: imx8mm-verdin: Link reg_usdhc2_vqmmc to
 usdhc2

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@...r.kernel.org
>>> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@...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

Powered by blists - more mailing lists