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

[Jonas Karlman]

Hi Ye Li,

On 11/7/2025 8:31 AM, Ye Li wrote:
> 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 at riseup.net>
>>>> Signed-off-by: Jonas Karlman <jonas at 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.

Please note that my example above was not from a real board only for
illustration purposes.

The vin-supply used for Rockchip devices on affected boards are
typically only fixed-regulator that are always-on and cannot be
controlled, e.g. dc-in or some always-on step-down converter.
These are included in the DT for more accurate description of HW.

For those boards, including vin-supply regulators in the SPL control FDT
serve no purpose beside wasting space and boot time for udevice binding
and probing, because of this they been left out on purpose since U-Boot
did not require or cared about a vin-supply prior to this patch.

If and how a vin-supply should be flagged with bootph-pre-ram or if the
vin-supply prop is scrubbed from SPL control FDT is not related to the
revert of this commit, it was just the main issue that help discover a
change in behavior that deserve discussion on ML before being merged.

> 
> 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.

Personally I do not think there is a need to fail at probe time because
a regulator-uclass udevice for a referenced vin-supply cannot be
resolved. A debug message could be printed if that is the case, however
require a working driver for the vin-supply is a change in behavior that
should really be documented in the commit message of a patch.

> 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.

Agreed, the main issue with the original patch what that it changed
probe behavior and also force-enabled any regulator that happened to 
be referenced as a vin-supply regardless if such regulator is even
probed or enabled.

This revert has now landed in master, please send a new series if you
have need for proper vin-supply handling by fixed/gpio-regulators.

Regards,
Jonas

> 
> 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
>>>>
>>
>