Re: [PATCH] Revert "power: regulator: Add vin-supply for GPIO and Fixed regulators"

[Ye Li <ye.li@oss.nxp.com>]

Hi Jonas,

On 11/3/2025 6:36 PM, Jonas Karlman wrote:
> Hi Peng,
> 
> On 11/3/2025 9:51 AM, Peng Fan wrote:
>> On Sat, Nov 01, 2025 at 08:34:26PM +0000, Jonas Karlman wrote:
>>> Rockchip boards may depend on a working MMC regulator in SPL to
>>> successfully load FIT payload from MMC. Typically, these boards only
>>> include the vmmc-supply regulator and not its vin-supply in SPL control
>>> FDT.
>>>
>>> The commit f98d812e5353 ("power: regulator: Add vin-supply for GPIO and
>>> Fixed regulators") breaks loading FIT from MMC in SPL on some of these
>>> boards due to now requiring the vin-supply to be included in the SPL
>>> control FDT.
>>>
>>> The commit also strangely enables any found vin-supply in
>>> regulator_common_of_to_plat() and not when a regulator is enabled or as
>>> part of regulator_autoset().
>>>
>>> Revert the commit to fix FIT loading in SPL on broken boards.
>>>
>>> If a board needs to have its vin-supply enabled, two options come to
>>> mind:
>>>
>>> - Add regulator-always-on prop to the regulator in the -u-boot.dtsi for
>>>   any board.
>>>
>>> - Implement full support for reference counting of regulators and then
>>>   update the regulator-uclass to enable any found vin-supply when a
>>>   regulator is enabled.
>>>
>>> This reverts commit f98d812e5353408ef77a46bad1f1cdc793ff8a03.
>>>
>>> Reported-by: Dang Huynh <danct12@riseup.net>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> drivers/power/regulator/regulator_common.c | 10 ----------
>>> drivers/power/regulator/regulator_common.h |  1 -
>>> 2 files changed, 11 deletions(-)
>>>
>>> diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c
>>> index 3ed713ce5019..685d8735fa5c 100644
>>> --- a/drivers/power/regulator/regulator_common.c
>>> +++ b/drivers/power/regulator/regulator_common.c
>>> @@ -44,16 +44,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
>>> 			dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0);
>>> 	}
>>>
>>> -	ret = device_get_supply_regulator(dev, "vin-supply", &plat->vin_supply);
>>> -	if (ret) {
>>> -		debug("Regulator vin regulator not defined: %d\n", ret);
>>> -		if (ret != -ENOENT)
>>
>> I understand it might be not proper to enable vin-supply in
>> function regulator_common_of_to_plat().
>> But I have a question, there is -ENOENT check in the original patch. If the
>> board does not have vin-supply, there should be no issue. Or I miss something?
> 
> As mentioned in the commit message the SPL control FDT does not contain
> the regulator references as a vin-supply, this result in -ENODEV to be
> returned. I.e. following where the vcc_io is not included in SPL control
> FDT.
> 
>    vcc_io: regulator@ {
>    }
>    vcc_sd: regulator@ {
>    	bootph-pre-ram;
>    	vin-supply = <&vcc_io>;
>    }
>    sdmmc: mmc@ {
>    	bootph-pre-ram;
>    	vmmc-supply = <&vcc_sd>;
>    }
> 
> With the change introduced in this commit the core gpio/fixed regulator
> behavior changed and this now result in unbootable boards.
> 
> Other reasons why I prefer a full revert instead of a workaround:
> - The implementation changed core gpio/fixed regulator function without
>    any review-by or acked-by.
> - The patch was quickly merged after only being on the list for 1 week,
>    the imx pull-request that included this did not even mention regulator
>    changes.
> - The implementation is not optimal, looking up vin-supply could be okay
>    but not failing because it is missing, and enabling it during probe
>    when we already have working enable count and regulator autoset to
>    handle any needed auto-enable.
> - The commit message does not give any insights on why this is needed.
> 
> My suggestion is that this is reverted and if needed it should can be
> submitted as a separated patch/series with a proper implementation.
> 
> Regards,
> Jonas

Shouldn't the reason be the broken SPL FDT? When one node is used in SPL 
FDT, the nodes referred by its properties should also add 
bootph-pre-ram. In your case, it is obviously missed for vcc_io. You 
can't assume driver never use this property.

If a new change for vin-supply is submitted in future, how can it avoid 
ubootable with these broken FDT?  It is appropriate for current 
implementation to get the vin_supply in probe and only check ENOENT. The 
ENODEV means a explicit problem that can't be ignored. The contentious 
point is if the vin-supply enablement should move to set enable. I agree 
with this proposal, but it is not related with the boot failure.

Best regard,
Ye Li>
>>
>> Thanks,
>> Peng
>>
>>> -			return ret;
>>> -	}
>>> -
>>> -	if (plat->vin_supply)
>>> -		regulator_set_enable_if_allowed(plat->vin_supply, true);
>>> -
>>> 	return 0;
>>> }
>>>
>>> diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h
>>> index 799c968d0b66..d4962899d830 100644
>>> --- a/drivers/power/regulator/regulator_common.h
>>> +++ b/drivers/power/regulator/regulator_common.h
>>> @@ -14,7 +14,6 @@ struct regulator_common_plat {
>>> 	unsigned int startup_delay_us;
>>> 	unsigned int off_on_delay_us;
>>> 	unsigned int enable_count;
>>> -	struct udevice *vin_supply;
>>> };
>>>
>>> int regulator_common_of_to_plat(struct udevice *dev,
>>> -- 
>>> 2.51.0
>>>
>