[PATCH v5 1/3] regulator: implement basic reference counter
Eugen Hristev
eugen.hristev at collabora.com
Mon May 1 10:12:50 CEST 2023
On 4/30/23 21:21, Jonas Karlman wrote:
> 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."
I think that the best option is to replace calls to regulator_set_enable
with regulator_set_enable_if_allowed.
Tim, what is the driver that is now broken ? I will try to create a
patch and send it your way so you can test ?
>
> 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
>
More information about the U-Boot
mailing list