Re: [PATCH v5 1/3] regulator: implement basic reference counter
Hi Eugen,
On Mon, 1 May 2023 at 02:12, Eugen Hristev 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
> >> wrote:
> >>>
> >>> On 4/28/23 02:39, Tim Harvey wrote:
> On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
> 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
> > Reviewed-by: Simon Glass
> > ---
> > 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@32e4: regulator_set_enable regulator-usb-otg1 enable
> >> regulator_set_enable regulator-usb-otg1 enable ret=0
> >> Bus usb@32e5: 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@32e4:
Re: [PATCH v5 1/3] regulator: implement basic reference counter
On Mon, May 1, 2023 at 1:12 AM Eugen Hristev
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
> >> wrote:
> >>>
> >>> On 4/28/23 02:39, Tim Harvey wrote:
> On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
> 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
> > Reviewed-by: Simon Glass
> > ---
> > 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@32e4: regulator_set_enable regulator-usb-otg1 enable
> >> regulator_set_enable regulator-usb-otg1 enable ret=0
> >> Bus usb@32e5: 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@32e4: Bus
Re: [PATCH v5 1/3] regulator: implement basic reference counter
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
wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
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
Reviewed-by: Simon Glass
---
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@32e4: regulator_set_enable regulator-usb-otg1 enable
regulator_set_enable regulator-usb-otg1 enable ret=0
Bus usb@32e5: 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@32e4: Bus usb@32e5: 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
Re: [PATCH v5 1/3] regulator: implement basic reference counter
Hi Tim,
On 2023-04-28 18:36, Tim Harvey wrote:
> On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev
> wrote:
>>
>> On 4/28/23 02:39, Tim Harvey wrote:
>>> On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
>>> 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
Reviewed-by: Simon Glass
---
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@32e4: regulator_set_enable regulator-usb-otg1 enable
> regulator_set_enable regulator-usb-otg1 enable ret=0
> Bus usb@32e5: 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@32e4: Bus usb@32e5: 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
Re: [PATCH v5 1/3] regulator: implement basic reference counter
Hi Eugen,
On Fri, 28 Apr 2023 at 01:39, Eugen Hristev wrote:
>
> On 4/28/23 02:39, Tim Harvey wrote:
> > On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
> > 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
> >> Reviewed-by: Simon Glass
> >> ---
> >> 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.
If you want to have a special function that consumes errors, you can
add one, e.g. that wraps the one that does report errors. My objection
is when something goes wrong and it is silently ignored, since it
tends to make problems build up, people add work-arounds, etc.
>
> > (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
> >
> > 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@32e4: Bus usb@32e5: 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() ?
Regards,
Simon
Re: [PATCH v5 1/3] regulator: implement basic reference counter
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev
wrote:
>
> On 4/28/23 02:39, Tim Harvey wrote:
> > On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
> > 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
> >> Reviewed-by: Simon Glass
> >> ---
> >> 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@32e4: regulator_set_enable regulator-usb-otg1 enable
regulator_set_enable regulator-usb-otg1 enable ret=0
Bus usb@32e5: 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@32e4: Bus usb@32e5: 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
Re: [PATCH v5 1/3] regulator: implement basic reference counter
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
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
Reviewed-by: Simon Glass
---
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.
(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
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@32e4: Bus usb@32e5: 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() ?
Best Regards,
Tim
Re: [PATCH v5 1/3] regulator: implement basic reference counter
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev
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
> Reviewed-by: Simon Glass
> ---
> 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!
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
(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 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@32e4: Bus usb@32e5: Error enabling VBUS supply (ret=-114)
probe failed, error -114
Best Regards,
Tim
Re: [PATCH v5 1/3] regulator: implement basic reference counter
On 4/19/23 15:45, Eugen Hristev 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
> Reviewed-by: Simon Glass
> ---
> 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;
> + }
> + }
> +
> ret = dm_gpio_set_value(_pdata->gpio, enable);
> if (ret) {
> pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
> @@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev,
> if (!enable && dev_pdata->off_on_delay_us)
> udelay(dev_pdata->off_on_delay_us);
>
> + if (enable)
> + dev_pdata->enable_count++;
> + else
> + dev_pdata->enable_count--;
> +
> return 0;
> }
> diff --git a/drivers/power/regulator/regulator_common.h
> b/drivers/power/regulator/regulator_common.h
> index c10492f01675..0faab447d099 100644
> --- a/drivers/power/regulator/regulator_common.h
> +++ b/drivers/power/regulator/regulator_common.h
> @@ -13,6 +13,7 @@ struct regulator_common_plat {
> struct gpio_desc gpio; /* GPIO for regulator enable control */
> unsigned int startup_delay_us;
> unsigned int off_on_delay_us;
> + unsigned int enable_count;
> };
>
> int regulator_common_of_to_plat(struct udevice *dev,
> @@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev,
> char *enable_gpio_name);
> int regulator_common_get_enable(const struct udevice *dev,
> struct regulator_common_plat *dev_pdata);
> +/*
> + * Enable or Disable a regulator
> + *
> + * This is a reentrant function and subsequent calls that enable will
> + * increase an internal counter, and disable calls will decrease the counter.
> + * The actual resource will be enabled when the counter gets to 1 coming
> from 0,
> + * and disabled when it reaches 0 coming from 1.
> + *
> + * @dev: regulator device
> + * @dev_pdata: Platform data
> + * @enable: bool indicating whether to enable or disable the regulator
> + * @return:
> + * 0 on Success
> + * -EBUSY if the regulator cannot be disabled because it's requested by
> + *another device
> + * -EALREADY if the regulator has already been enabled or has already been
> + *disabled
> + * -EACCES if there is no possibility to enable/disable the regulator
> + * -ve on different error situation
> + */
> int regulator_common_set_enable(const struct udevice *dev,
> struct regulator_common_plat *dev_pdata, bool enable);
>
Reviewed-by: Patrice Chotard
Patrice
[PATCH v5 1/3] regulator: implement basic reference counter
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
Reviewed-by: Simon Glass
---
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;
+ }
+ }
+
ret = dm_gpio_set_value(_pdata->gpio, enable);
if (ret) {
pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
@@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev,
if (!enable && dev_pdata->off_on_delay_us)
udelay(dev_pdata->off_on_delay_us);
+ if (enable)
+ dev_pdata->enable_count++;
+ else
+ dev_pdata->enable_count--;
+
return 0;
}
diff --git a/drivers/power/regulator/regulator_common.h
b/drivers/power/regulator/regulator_common.h
index c10492f01675..0faab447d099 100644
--- a/drivers/power/regulator/regulator_common.h
+++ b/drivers/power/regulator/regulator_common.h
@@ -13,6 +13,7 @@ struct regulator_common_plat {
struct gpio_desc gpio; /* GPIO for regulator enable control */
unsigned int startup_delay_us;
unsigned int off_on_delay_us;
+ unsigned int enable_count;
};
int regulator_common_of_to_plat(struct udevice *dev,
@@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev,
char *enable_gpio_name);
int regulator_common_get_enable(const struct udevice *dev,
struct regulator_common_plat *dev_pdata);
+/*
+ * Enable or Disable a regulator
+ *
+ * This is a reentrant function and subsequent calls that enable will
+ * increase an internal counter, and disable calls will decrease the counter.
+ * The actual resource will be enabled when the counter gets to 1 coming from
0,
+ * and disabled when it reaches 0 coming from 1.
+ *
+ * @dev: regulator device
+ * @dev_pdata: Platform data
+ * @enable: bool indicating whether to enable or disable the regulator
+ * @return:
+ * 0 on Success
+ * -EBUSY if the regulator cannot be disabled because it's requested by
+ *another device
+ * -EALREADY if the regulator has already been enabled or has already been
+ *disabled
+ * -EACCES if there is no possibility to enable/disable the regulator
+ * -ve on different error situation
+ */
int regulator_common_set_enable(const struct udevice *dev,
struct regulator_common_plat *dev_pdata, bool enable);
--
2.34.1
