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 <[email protected]>
Signed-off-by: Jonas Karlman <[email protected]>
---
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
