Hi Eugen, On 2023-04-03 11:59, Eugen Hristev wrote: > On 4/2/23 13:45, Jonas Karlman wrote: >> Hi Eugen, >> >> On 2023-03-31 11:15, Eugen Hristev wrote: >>> Some devices share a regulator supply, when the first one will request >>> regulator disable, the second device will have it's supply cut off before >>> graciously shutting down. Hence there will be timeouts and other failed >>> operations. >>> Implement a reference counter mechanism similar with what is done in >>> Linux, to keep track of enable and disable requests, and only disable the >>> regulator when the last of the consumers has requested shutdown. >>> >>> Signed-off-by: Eugen Hristev <eugen.hris...@collabora.com> >>> Reviewed-by: Simon Glass <s...@chromium.org> >>> --- >>> Changes in v4: >>> - add documentation for error codes >>> Changes in v3: >>> - add error return codes >>> Changes in v2: >>> - add info in header regarding the function >>> >>> drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ >>> drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ >>> 2 files changed, 43 insertions(+) >>> >>> diff --git a/drivers/power/regulator/regulator_common.c >>> b/drivers/power/regulator/regulator_common.c >>> index 93d8196b381e..484a4fc31ef7 100644 >>> --- a/drivers/power/regulator/regulator_common.c >>> +++ b/drivers/power/regulator/regulator_common.c >>> @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice >>> *dev, >>> return 0; >>> } >>> >>> + /* If previously enabled, increase count */ >>> + if (enable && dev_pdata->enable_count > 0) { >>> + dev_pdata->enable_count++; >>> + return -EALREADY; >>> + } >>> + >>> + if (!enable) { >>> + if (dev_pdata->enable_count > 1) { >>> + /* If enabled multiple times, decrease count */ >>> + dev_pdata->enable_count--; >>> + return -EBUSY; >>> + } else if (!dev_pdata->enable_count) { >>> + /* If already disabled, do nothing */ >>> + return -EALREADY; >>> + } >>> + } >>> + >>> ret = dm_gpio_set_value(&dev_pdata->gpio, enable); >>> if (ret) { >>> pr_err("Can't set regulator : %s gpio to: %d\n", dev->name, >>> @@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice >>> *dev, >>> if (!enable && dev_pdata->off_on_delay_us) >>> udelay(dev_pdata->off_on_delay_us); >>> >>> + if (enable) >>> + dev_pdata->enable_count++; >>> + else >>> + dev_pdata->enable_count--; >>> + >>> return 0; >>> } >>> diff --git a/drivers/power/regulator/regulator_common.h >>> b/drivers/power/regulator/regulator_common.h >>> index c10492f01675..0faab447d099 100644 >>> --- a/drivers/power/regulator/regulator_common.h >>> +++ b/drivers/power/regulator/regulator_common.h >>> @@ -13,6 +13,7 @@ struct regulator_common_plat { >>> struct gpio_desc gpio; /* GPIO for regulator enable control */ >>> unsigned int startup_delay_us; >>> unsigned int off_on_delay_us; >>> + unsigned int enable_count; >>> }; >>> >>> int regulator_common_of_to_plat(struct udevice *dev, >>> @@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev, >>> char *enable_gpio_name); >>> int regulator_common_get_enable(const struct udevice *dev, >>> struct regulator_common_plat *dev_pdata); >>> +/* >>> + * Enable or Disable a regulator >>> + * >>> + * This is a reentrant function and subsequent calls that enable will >>> + * increase an internal counter, and disable calls will decrease the >>> counter. >>> + * The actual resource will be enabled when the counter gets to 1 coming >>> from 0, >>> + * and disabled when it reaches 0 coming from 1. >>> + * >>> + * @dev: regulator device >>> + * @dev_pdata: Platform data >>> + * @enable: bool indicating whether to enable or disable the regulator >>> + * @return: >>> + * 0 on Success >>> + * -EBUSY if the regulator cannot be disabled because it's requested by >>> + * another device >>> + * -EALREADY if the regulator has already been enabled or has already been >>> + * disabled >> >> With this new return value added this has potential to break IO voltage >> switching for mmc UHS support. Main pattern for IO voltage switch seem >> to be: disable regulator, set voltage, enable regulator. > > How can the breakage happen in this case ?
This was more of a theoretical issue then anything I have observed. > >> >> regulator_set_enable_if_allowed should probably be updated to return >> success on -EALREADY, but that also has the potential to hide regulators >> enabled at boot not being disabled unless initial enable_count reflects >> the initial regulator state. > > If the regulators enabled at boot are fixed/gpio regulators (which have > the enable count), then at boot , the autoset should call the > enable_common, thus taking into account the enable_count. > > So I think that if I updated regulator_set_enable_if_allowed to return > success on -EALREADY it should be fine. Do you have any other scenario > in mind ? I did more runtime test of this with USB on RK3568 and [1], -EBUSY should possibly also return success. Testing a "usb start" / "usb stop" cycle report the following -EBUSY error with a shared regulator: => usb stop stopping USB.. Host not halted after 16000 microseconds. rockchip_usb2phy_port host-port: PHY: Failed to disable regulator vcc5v0-usb-host-regulator: -16. Host not halted after 16000 microseconds. rockchip_usb2phy_port otg-port: PHY: Failed to disable regulator vcc5v0-usb-host-regulator: -16. Unsure if this should be reported as error or not. >> >> Guessing initial enable_count will match for regulator-boot-on regulators >> in Rockchip and few other platforms/boards because they the call >> regulators_enable_boot_on, does not look like all platform will have >> initial enable_count match regulator initial state. > > If the other platforms do not call regulator enable, then they will have > the enable_count==0, which is expected right ? > You refer to specific platforms where the regulator is already enabled > from hardware but no code is called to enable, and by looking at > enable_count it would appear disabled ? There are such cases ? And those > regulators are not initialized at all ? This was more of a theoretical issue then anything I have observed. There could be a need in the future to initialize the enable_count based on the state of e.g. the gpio at regulator probe time or at first call to set enable/disable if this becomes an issue. I did notice that there have been some calls to regulator enable/disable missing that result in unbalanced enable_count. E.g. in dw_mmc and rk pcie. This looks and works good, as long as set enable/disable calls are balanced so, Tested-by: Jonas Karlman <jo...@kwiboo.se> Reviewed-by: Jonas Karlman <jo...@kwiboo.se> [1] https://github.com/Kwiboo/u-boot-rockchip/commits/rk3568-usb-v1 Regards, Jonas > >> >> Regards, >> Jonas >> >>> + * -EACCES if there is no possibility to enable/disable the regulator >>> + * -ve on different error situation >>> + */ >>> int regulator_common_set_enable(const struct udevice *dev, >>> struct regulator_common_plat *dev_pdata, bool enable); >>> >> >