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.
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
