Hi Tim, On 2023-04-28 18:36, Tim Harvey wrote: > On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev > <eugen.hris...@collabora.com> wrote: >> >> On 4/28/23 02:39, Tim Harvey wrote: >>> On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev >>> <eugen.hris...@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.hris...@collabora.com> >>>> Reviewed-by: Simon Glass <s...@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.hris...@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-jo...@kwiboo.se/ > ? > > I added some debug prints to regulator_set_enable and see: > starting USB... > Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable > regulator_set_enable regulator-usb-otg1 enable ret=0 > Bus usb@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@32e40000: Bus usb@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