[PATCH v4 1/2] regulator: implement basic reference counter

[Jonas Karlman]

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.hristev at collabora.com>
>>> Reviewed-by: Simon Glass <sjg at 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 <jonas at kwiboo.se>
Reviewed-by: Jonas Karlman <jonas at 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);
>>>   
>>
>