Hi Eugen,
On Mon, 1 May 2023 at 02:12, Eugen Hristev <eugen.hris...@collabora.com> wrote:
>
> 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.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."
>
> I think that the best option is to replace calls to regulator_set_enable
> with regulator_set_enable_if_allowed.
That sounds good. Or even regulator_set_enable_maybe() since it is
shorter and the question is not whether it is allowed, but whether we
want to for other reasons (i.e other device's desires). I can imagine
having quite a few 'maybe' functions about the place eventually.
Another option would be to make it more explicit, like
regulator_inc_set_enable()/ dec_set_enable(), but that seems a bit
wordy and confusing.
>
> 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
> >
>
Regards,
Simon
