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

[Tim Harvey]

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.

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.

Best Regards,

Tim