[PATCH v5 1/3] regulator: implement basic reference counter

[Jonas Karlman]

Hi Tim,
On 2023-04-28 18:36, Tim Harvey wrote:
> On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev
> <eugen.hristev at collabora.com> wrote:
>>
>> On 4/28/23 02:39, Tim Harvey wrote:
>>> On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
>>> <eugen.hristev at collabora.com> 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 v5:
>>>>   - none
>>>> 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;
>>>> +               }
>>>> +       }
>>>> +
>>>
>>> Eugen,
>>>
>>> Thank you for working on this series!
>>
>> Hi Tim,
>>
>> Thank you for testing and having a look on my patches.
>>>
>>> I wonder if instead of returning a failure you should print an error
>>> here but return 0 in order to not break unbalanced regulator calls
>>
>> Initially I did that, but Simon forced me to return error as you see now.
> 
> Ok, I see that discussion here:
> https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-eugen.hristev@collabora.com/#3086660
> 
>>
>>> (like what is done in clk_disable()). I see that you have another
>>> patch in this series which handles htis for
>>> regulator_set_enable_if_allowed() but that doesn't cover drivers that
>>> call regulator_common_set_enable() directly such as
>>> drivers/power/regulator/fixed.c and
>>> drivers/power/regulator/gpio-regulator.c.
>>
>> I believe Jonas (in CC) fixed those with a patch, but at the moment I am
>> lost in providing you a link to it
> 
> Are you thinking of "pci: pcie_dw_rockchip: Use
> regulator_set_enable_if_allowed"
> https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jonas@kwiboo.se/
> ?
> 
> I added some debug prints to regulator_set_enable and see:
> starting USB...
> Bus usb at 32e40000: regulator_set_enable regulator-usb-otg1 enable
> regulator_set_enable regulator-usb-otg1 enable ret=0
> Bus usb at 32e50000: regulator_set_enable regulator-usb-otg2 enable
> regulator_set_enable regulator-usb-otg2 enable ret=0
> regulator_set_enable regulator-usb-otg2 enable
> regulator_set_enable regulator-usb-otg2 enable ret=-114
> Error enabling VBUS supply (ret=-114)
> regulator_set_enable regulator-usb-otg2 disable
> regulator_set_enable regulator-usb-otg2 disable ret=-16
> probe failed, error -114
> 
> So clearly something is trying to enable regulator-usb-otg2 when it's
> already enabled but I don't think that should be considered an error
> and cause a failure.
> 
> Simon, is this what you were intending with your feedback?
> 
>>>
>>> I know there is an unbalanced call currently on imx8mm that this patch
>>> causes a failure where it previously did not:
>>> u-boot=> usb start && usb tree
>>> starting USB...
>>> Bus usb at 32e40000: Bus usb at 32e50000: Error enabling VBUS supply (ret=-114)
>>> probe failed, error -114
>>>
>>
>> That's a good catch.
>> I expected such things would happen if I would return error in those
>> cases, so it might be that this is not the only case.
>> If you are able to test that board, do you wish me to send you a patch
>> that changes the code to using regulator_set_enable_if_allowed() ?
>>
> 
> Yes, I can test. Are you thinking changing the calls to
> regulator_common_set_enable (used in
> drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a
> wrapper calling regulator_common_set_enable_if_allowed instead? I
> think that would just be the same thing as removing the error as no
> callers of that function would be left.

This is nothing that should be changed in the fixed/gpio/common
regulator code. Such change should happen in the drivers that call the
regulator_set_enable function. In your case the usb driver that tries
to enable/disable the regulator.

> 
> Your series solves a necessary issue where a regulator_disable call
> would disable a regulator that was still being used by another
> consumer but I don't think it should break calls to regulator_enable
> just because another consumer is also using it.
> 

With this patch at least fixed/gpio regulators have a basic reference
counter and because of this the regulator_set_enable may now return a
more detailed errno, "0 on success or -errno val if fails".

The call to regulator_set_enable can be replaced with a call to
regulator_set_enable_if_allowed when the caller requires less strict
errors, "0 on success or if enabling is not supported or -errno val if
fails."

Another option could be to also relax the errors returned by
regulator_set_enable, and instead print an error message for the two new
errno.

Regards,
Jonas

> Best Regards,
> 
> Tim