| Message ID | 20230502065104.32242-1-eugen.hristev@collabora.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | usb: ehci-mx6: replace regulator_set_enable with *_if_allowed | expand |
On 5/2/23 08:51, Eugen Hristev wrote: > regulator_set_enable_if_allowed already handles cases when the > regulator is already enabled, or already disabled, and does not > treat these as errors. > With this change, the driver can work correctly even if the regulator > is already taken or already disabled by another consumer. Can that ever happen for Vbus supply (the 5V coming out of USB port) ? Can you elaborate how ?
On 5/2/23 12:18, Marek Vasut wrote: > On 5/2/23 08:51, Eugen Hristev wrote: >> regulator_set_enable_if_allowed already handles cases when the >> regulator is already enabled, or already disabled, and does not >> treat these as errors. >> With this change, the driver can work correctly even if the regulator >> is already taken or already disabled by another consumer. > > Can that ever happen for Vbus supply (the 5V coming out of USB port) ? > Can you elaborate how ? Hi Marek, Recently I developed a series of patches to add a reference counter for regulators : https://marc.info/?l=u-boot&m=168191189309879&w=2 But with this series, having a regulator already enabled or already disabled results in an error returned by regulator_set_enable Hence, one option is to replace calls with regulator_set_enable_if_allowed There is a discussion ongoing here: https://marc.info/?l=u-boot&m=168295920316621&w=2 Eugen
On 5/2/23 12:41, Eugen Hristev wrote: > On 5/2/23 12:18, Marek Vasut wrote: >> On 5/2/23 08:51, Eugen Hristev wrote: >>> regulator_set_enable_if_allowed already handles cases when the >>> regulator is already enabled, or already disabled, and does not >>> treat these as errors. >>> With this change, the driver can work correctly even if the regulator >>> is already taken or already disabled by another consumer. >> >> Can that ever happen for Vbus supply (the 5V coming out of USB port) ? >> Can you elaborate how ? > > > Hi Marek, Hi, > Recently I developed a series of patches to add a reference counter for > regulators : > > https://marc.info/?l=u-boot&m=168191189309879&w=2 Ah yes, this is super cool stuff, thanks ! > But with this series, having a regulator already enabled or already > disabled results in an error returned by regulator_set_enable > > Hence, one option is to replace calls with regulator_set_enable_if_allowed > > There is a discussion ongoing here: > > https://marc.info/?l=u-boot&m=168295920316621&w=2 Pardon my ignorance, but uh ... how does Linux deal with regulators which are enabled by prior stage, like U-Boot ? Does it set their enable refcount to =1 or =0 ?
On 5/2/23 13:53, Marek Vasut wrote: > On 5/2/23 12:41, Eugen Hristev wrote: >> On 5/2/23 12:18, Marek Vasut wrote: >>> On 5/2/23 08:51, Eugen Hristev wrote: >>>> regulator_set_enable_if_allowed already handles cases when the >>>> regulator is already enabled, or already disabled, and does not >>>> treat these as errors. >>>> With this change, the driver can work correctly even if the regulator >>>> is already taken or already disabled by another consumer. >>> >>> Can that ever happen for Vbus supply (the 5V coming out of USB port) ? >>> Can you elaborate how ? >> >> >> Hi Marek, > > Hi, > >> Recently I developed a series of patches to add a reference counter >> for regulators : >> >> https://marc.info/?l=u-boot&m=168191189309879&w=2 > > Ah yes, this is super cool stuff, thanks ! > >> But with this series, having a regulator already enabled or already >> disabled results in an error returned by regulator_set_enable >> >> Hence, one option is to replace calls with >> regulator_set_enable_if_allowed >> >> There is a discussion ongoing here: >> >> https://marc.info/?l=u-boot&m=168295920316621&w=2 > > Pardon my ignorance, but uh ... how does Linux deal with regulators > which are enabled by prior stage, like U-Boot ? Does it set their enable > refcount to =1 or =0 ? Please correct me if I am wrong, I did not dig too much into regulators, but , from my understanding : Normally software(in this case Linux) reads the state of the regulator when it's probed, and sets the refcount accordingly. If the state cannot be read then regulator-boot-on tells Linux that it should have been left enabled by prior stage In Uboot I believe that regulators that have 'regulator-boot-on' are added to a list and set to enable during probe, and the probe is forced even if Uboot normally uses lazy probing (should have the same behavior for regulator-always-on property) So if a regulator is enabled by an initial stage but it's not described with 'regulator-boot-on' , appears to be a bad hardware description in DT.
On Mon, May 1, 2023 at 11:51 PM Eugen Hristev <eugen.hristev@collabora.com> wrote: > > regulator_set_enable_if_allowed already handles cases when the > regulator is already enabled, or already disabled, and does not > treat these as errors. > With this change, the driver can work correctly even if the regulator > is already taken or already disabled by another consumer. > > Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> > --- > Hi Tim, > > I have not tested this as I do not have a mx6 board. can you please > try it ? > > Thanks, > Eugen Eugen, This does resolve the issue that occurs after your refcount series [1]. However after thinking about this more and seeing Marek's responses this wasn't an issue of multiple consumers sharing the same regulator. Instead this particular issue was that the vbus regulator is getting enabled twice without being disabled. Adding a print in regulator_set_enable shows me: --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -165,6 +165,7 @@ int regulator_set_enable(struct udevice *dev, bool enable) struct dm_regulator_uclass_plat *uc_pdata; int ret, old_enable = 0; +printf("%s %s %s\n", __func__, dev->name, enable ? "enable" : "disable"); if (!ops || !ops->set_enable) return -ENOSYS; u-boot=> usb start starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable ^^^ from ehci_usb_probe Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable ^^^ from ehci_usb_probe regulator_set_enable regulator-usb-otg2 enable ^^^ from mx6_init_after_reset - 2nd enable without a disable So while I think this patch does make sense to cover the case where a regulator could be shared there probably is a follow-on patch needed to balance the regulator calls (unrelated to your series). Marek, Should vbus regulator enable really be in init_after_reset call? Best Regards, Tim [1] https://patchwork.ozlabs.org/project/uboot/list/?series=351536
On 5/2/23 18:13, Tim Harvey wrote: > On Mon, May 1, 2023 at 11:51 PM Eugen Hristev > <eugen.hristev@collabora.com> wrote: >> >> regulator_set_enable_if_allowed already handles cases when the >> regulator is already enabled, or already disabled, and does not >> treat these as errors. >> With this change, the driver can work correctly even if the regulator >> is already taken or already disabled by another consumer. >> >> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> >> --- >> Hi Tim, >> >> I have not tested this as I do not have a mx6 board. can you please >> try it ? >> >> Thanks, >> Eugen > > Eugen, > > This does resolve the issue that occurs after your refcount series [1]. > > However after thinking about this more and seeing Marek's responses > this wasn't an issue of multiple consumers sharing the same regulator. > Instead this particular issue was that the vbus regulator is getting > enabled twice without being disabled. Adding a print in > regulator_set_enable shows me: > --- a/drivers/power/regulator/regulator-uclass.c > +++ b/drivers/power/regulator/regulator-uclass.c > @@ -165,6 +165,7 @@ int regulator_set_enable(struct udevice *dev, bool enable) > struct dm_regulator_uclass_plat *uc_pdata; > int ret, old_enable = 0; > > +printf("%s %s %s\n", __func__, dev->name, enable ? "enable" : "disable"); > if (!ops || !ops->set_enable) > return -ENOSYS; > > u-boot=> usb start > starting USB... > Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable > ^^^ from ehci_usb_probe > Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable > ^^^ from ehci_usb_probe > regulator_set_enable regulator-usb-otg2 enable > ^^^ from mx6_init_after_reset - 2nd enable without a disable > > So while I think this patch does make sense to cover the case where a > regulator could be shared Does such a case really still exist after the discovery you made above ? > there probably is a follow-on patch needed > to balance the regulator calls (unrelated to your series). > > Marek, > > Should vbus regulator enable really be in init_after_reset call? Yes, but I am not entirely convinced the VBUS should be enabled in ehci_usb_probe(), because in ehci_usb_probe() resp. in ehci_register() which is called at the end, we might detect that the port is configured as PERIPHERAL and we don't want to enable VBUS in that case . So I suspect regulator_set_enable() should rather be dropped from ehci_usb_probe() . What do you think ?
On 5/2/23 13:18, Eugen Hristev wrote: > On 5/2/23 13:53, Marek Vasut wrote: >> On 5/2/23 12:41, Eugen Hristev wrote: >>> On 5/2/23 12:18, Marek Vasut wrote: >>>> On 5/2/23 08:51, Eugen Hristev wrote: >>>>> regulator_set_enable_if_allowed already handles cases when the >>>>> regulator is already enabled, or already disabled, and does not >>>>> treat these as errors. >>>>> With this change, the driver can work correctly even if the regulator >>>>> is already taken or already disabled by another consumer. >>>> >>>> Can that ever happen for Vbus supply (the 5V coming out of USB port) ? >>>> Can you elaborate how ? >>> >>> >>> Hi Marek, >> >> Hi, >> >>> Recently I developed a series of patches to add a reference counter >>> for regulators : >>> >>> https://marc.info/?l=u-boot&m=168191189309879&w=2 >> >> Ah yes, this is super cool stuff, thanks ! >> >>> But with this series, having a regulator already enabled or already >>> disabled results in an error returned by regulator_set_enable >>> >>> Hence, one option is to replace calls with >>> regulator_set_enable_if_allowed >>> >>> There is a discussion ongoing here: >>> >>> https://marc.info/?l=u-boot&m=168295920316621&w=2 >> >> Pardon my ignorance, but uh ... how does Linux deal with regulators >> which are enabled by prior stage, like U-Boot ? Does it set their >> enable refcount to =1 or =0 ? > > Please correct me if I am wrong, I did not dig too much into regulators, > but , from my understanding : > > Normally software(in this case Linux) reads the state of the regulator > when it's probed, and sets the refcount accordingly. If the state cannot > be read then regulator-boot-on tells Linux that it should have been left > enabled by prior stage > In Uboot I believe that regulators that have 'regulator-boot-on' are > added to a list and set to enable during probe, and the probe is forced > even if Uboot normally uses lazy probing (should have the same behavior > for regulator-always-on property) > > So if a regulator is enabled by an initial stage but it's not described > with 'regulator-boot-on' , appears to be a bad hardware description in DT. Thanks for checking this. It seems like Tim found the root cause of the problem in the meantime, so let's see where that discussion moves on to.
On 5/4/23 00:52, Marek Vasut wrote: > On 5/2/23 18:13, Tim Harvey wrote: >> On Mon, May 1, 2023 at 11:51 PM Eugen Hristev >> <eugen.hristev@collabora.com> wrote: >>> >>> regulator_set_enable_if_allowed already handles cases when the >>> regulator is already enabled, or already disabled, and does not >>> treat these as errors. >>> With this change, the driver can work correctly even if the regulator >>> is already taken or already disabled by another consumer. >>> >>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> >>> --- >>> Hi Tim, >>> >>> I have not tested this as I do not have a mx6 board. can you please >>> try it ? >>> >>> Thanks, >>> Eugen >> >> Eugen, >> >> This does resolve the issue that occurs after your refcount series [1]. >> >> However after thinking about this more and seeing Marek's responses >> this wasn't an issue of multiple consumers sharing the same regulator. >> Instead this particular issue was that the vbus regulator is getting >> enabled twice without being disabled. This kind of issues can be uncovered when we add the refcounter :) On one hand I am happy to uncover such bugs. However, this leads me to wonder, even without a refcount, even without my series, enabling a regulator that is already enabled should return -EALREADY On the other hand, it appears it will take longer to get the refcount applied though. Adding a print in >> regulator_set_enable shows me: >> --- a/drivers/power/regulator/regulator-uclass.c >> +++ b/drivers/power/regulator/regulator-uclass.c >> @@ -165,6 +165,7 @@ int regulator_set_enable(struct udevice *dev, bool >> enable) >> struct dm_regulator_uclass_plat *uc_pdata; >> int ret, old_enable = 0; >> >> +printf("%s %s %s\n", __func__, dev->name, enable ? "enable" : >> "disable"); >> if (!ops || !ops->set_enable) >> return -ENOSYS; >> >> u-boot=> usb start >> starting USB... >> Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable >> ^^^ from ehci_usb_probe >> Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable >> ^^^ from ehci_usb_probe >> regulator_set_enable regulator-usb-otg2 enable >> ^^^ from mx6_init_after_reset - 2nd enable without a disable >> >> So while I think this patch does make sense to cover the case where a >> regulator could be shared > > Does such a case really still exist after the discovery you made above ? > >> there probably is a follow-on patch needed >> to balance the regulator calls (unrelated to your series). >> >> Marek, >> >> Should vbus regulator enable really be in init_after_reset call? > > Yes, but I am not entirely convinced the VBUS should be enabled in > ehci_usb_probe(), because in ehci_usb_probe() resp. in ehci_register() > which is called at the end, we might detect that the port is configured > as PERIPHERAL and we don't want to enable VBUS in that case . > > So I suspect regulator_set_enable() should rather be dropped from > ehci_usb_probe() . What do you think ?
On 5/4/23 10:12, Eugen Hristev wrote: > On 5/4/23 00:52, Marek Vasut wrote: >> On 5/2/23 18:13, Tim Harvey wrote: >>> On Mon, May 1, 2023 at 11:51 PM Eugen Hristev >>> <eugen.hristev@collabora.com> wrote: >>>> >>>> regulator_set_enable_if_allowed already handles cases when the >>>> regulator is already enabled, or already disabled, and does not >>>> treat these as errors. >>>> With this change, the driver can work correctly even if the regulator >>>> is already taken or already disabled by another consumer. >>>> >>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> >>>> --- >>>> Hi Tim, >>>> >>>> I have not tested this as I do not have a mx6 board. can you please >>>> try it ? >>>> >>>> Thanks, >>>> Eugen >>> >>> Eugen, >>> >>> This does resolve the issue that occurs after your refcount series [1]. >>> >>> However after thinking about this more and seeing Marek's responses >>> this wasn't an issue of multiple consumers sharing the same regulator. >>> Instead this particular issue was that the vbus regulator is getting >>> enabled twice without being disabled. > > This kind of issues can be uncovered when we add the refcounter :) Yeah, and that's a good thing. The refcounter is much appreciated (am I repeating myself?) > On one hand I am happy to uncover such bugs. > However, this leads me to wonder, even without a refcount, even without > my series, enabling a regulator that is already enabled should return > -EALREADY I don't that so, you can have regulators which supply multiple things and which are claimed by multiple driver instances, at which point they will each enable the regulator (refcount++) and that shouldn't end up returning -EALREADY . Right ? One such example is buck regulator supplying e.g. SD and eMMC Vcc . > On the other hand, it appears it will take longer to get the refcount > applied though. I think we need to sort out the fallout, but that's probably still fine.
On 5/4/23 14:41, Marek Vasut wrote: > On 5/4/23 10:12, Eugen Hristev wrote: >> On 5/4/23 00:52, Marek Vasut wrote: >>> On 5/2/23 18:13, Tim Harvey wrote: >>>> On Mon, May 1, 2023 at 11:51 PM Eugen Hristev >>>> <eugen.hristev@collabora.com> wrote: >>>>> >>>>> regulator_set_enable_if_allowed already handles cases when the >>>>> regulator is already enabled, or already disabled, and does not >>>>> treat these as errors. >>>>> With this change, the driver can work correctly even if the regulator >>>>> is already taken or already disabled by another consumer. >>>>> >>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> >>>>> --- >>>>> Hi Tim, >>>>> >>>>> I have not tested this as I do not have a mx6 board. can you please >>>>> try it ? >>>>> >>>>> Thanks, >>>>> Eugen >>>> >>>> Eugen, >>>> >>>> This does resolve the issue that occurs after your refcount series [1]. >>>> >>>> However after thinking about this more and seeing Marek's responses >>>> this wasn't an issue of multiple consumers sharing the same regulator. >>>> Instead this particular issue was that the vbus regulator is getting >>>> enabled twice without being disabled. >> >> This kind of issues can be uncovered when we add the refcounter :) > > Yeah, and that's a good thing. The refcounter is much appreciated (am I > repeating myself?) > >> On one hand I am happy to uncover such bugs. >> However, this leads me to wonder, even without a refcount, even >> without my series, enabling a regulator that is already enabled should >> return -EALREADY > > I don't that so, you can have regulators which supply multiple things > and which are claimed by multiple driver instances, at which point they > will each enable the regulator (refcount++) and that shouldn't end up > returning -EALREADY . Right ? That was my thought. And I was returning success if regulator was already enabled. Then Simon asked me to return error codes on each path (review on the v2 of the patches): https://marc.info/?l=u-boot&m=168012024923488&w=2 > > One such example is buck regulator supplying e.g. SD and eMMC Vcc . > >> On the other hand, it appears it will take longer to get the refcount >> applied though. > > I think we need to sort out the fallout, but that's probably still fine.
On 5/4/23 13:51, Eugen Hristev wrote: > On 5/4/23 14:41, Marek Vasut wrote: >> On 5/4/23 10:12, Eugen Hristev wrote: >>> On 5/4/23 00:52, Marek Vasut wrote: >>>> On 5/2/23 18:13, Tim Harvey wrote: >>>>> On Mon, May 1, 2023 at 11:51 PM Eugen Hristev >>>>> <eugen.hristev@collabora.com> wrote: >>>>>> >>>>>> regulator_set_enable_if_allowed already handles cases when the >>>>>> regulator is already enabled, or already disabled, and does not >>>>>> treat these as errors. >>>>>> With this change, the driver can work correctly even if the regulator >>>>>> is already taken or already disabled by another consumer. >>>>>> >>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> >>>>>> --- >>>>>> Hi Tim, >>>>>> >>>>>> I have not tested this as I do not have a mx6 board. can you please >>>>>> try it ? >>>>>> >>>>>> Thanks, >>>>>> Eugen >>>>> >>>>> Eugen, >>>>> >>>>> This does resolve the issue that occurs after your refcount series >>>>> [1]. >>>>> >>>>> However after thinking about this more and seeing Marek's responses >>>>> this wasn't an issue of multiple consumers sharing the same regulator. >>>>> Instead this particular issue was that the vbus regulator is getting >>>>> enabled twice without being disabled. >>> >>> This kind of issues can be uncovered when we add the refcounter :) >> >> Yeah, and that's a good thing. The refcounter is much appreciated (am >> I repeating myself?) >> >>> On one hand I am happy to uncover such bugs. >>> However, this leads me to wonder, even without a refcount, even >>> without my series, enabling a regulator that is already enabled >>> should return -EALREADY >> >> I don't that so, you can have regulators which supply multiple things >> and which are claimed by multiple driver instances, at which point >> they will each enable the regulator (refcount++) and that shouldn't >> end up returning -EALREADY . Right ? > > That was my thought. And I was returning success if regulator was > already enabled. > Then Simon asked me to return error codes on each path (review on the v2 > of the patches): > > https://marc.info/?l=u-boot&m=168012024923488&w=2 I would argue the regulator API should behave the same way as Linux one, to avoid confusing people developing on both. Simon ?
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 91633f013a55..7806a4de71e0 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -480,9 +480,9 @@ static int mx6_init_after_reset(struct ehci_ctrl *dev) #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) { int ret; - ret = regulator_set_enable(priv->vbus_supply, - (type == USB_INIT_DEVICE) ? - false : true); + ret = regulator_set_enable_if_allowed(priv->vbus_supply, + (type == USB_INIT_DEVICE) ? + false : true); if (ret && ret != -ENOSYS) { printf("Error enabling VBUS supply (ret=%i)\n", ret); return ret; @@ -707,9 +707,9 @@ static int ehci_usb_probe(struct udevice *dev) #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) { - ret = regulator_set_enable(priv->vbus_supply, - (type == USB_INIT_DEVICE) ? - false : true); + ret = regulator_set_enable_if_allowed(priv->vbus_supply, + (type == USB_INIT_DEVICE) ? + false : true); if (ret && ret != -ENOSYS) { printf("Error enabling VBUS supply (ret=%i)\n", ret); goto err_clk; @@ -748,7 +748,7 @@ err_regulator: #endif #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) - regulator_set_enable(priv->vbus_supply, false); + regulator_set_enable_if_allowed(priv->vbus_supply, false); #endif err_clk: #if CONFIG_IS_ENABLED(CLK) @@ -772,7 +772,7 @@ int ehci_usb_remove(struct udevice *dev) #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) - regulator_set_enable(priv->vbus_supply, false); + regulator_set_enable_if_allowed(priv->vbus_supply, false); #endif #if CONFIG_IS_ENABLED(CLK)
regulator_set_enable_if_allowed already handles cases when the regulator is already enabled, or already disabled, and does not treat these as errors. With this change, the driver can work correctly even if the regulator is already taken or already disabled by another consumer. Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> --- Hi Tim, I have not tested this as I do not have a mx6 board. can you please try it ? Thanks, Eugen drivers/usb/host/ehci-mx6.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)