| Message ID | 20240415105405.432679-1-robert.marko@sartura.hr |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: dwc3-generic: fix support without DM_REGULATOR | expand |
On 15/04/2024 11:53, Robert Marko wrote: > Recent addition of vbus-supply support has broke platform which dont use > controllable regulators for USB. > > Issue is that even withou DM_REGULATOR being enabled regulator related > functions will still build as there is a stub in regulator.h but they will > simply return -ENOSYS which will then make dwc3_generic_host_probe() > return the same error thus breaking probe. Rather than stubbing out the code, could you check for -ENOSYS and ignore the error in that case? I believe there's only one place where this matters (marked below). > > Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > --- > drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c > index 7a00529a2a..784d3ec2ed 100644 > --- a/drivers/usb/dwc3/dwc3-generic.c > +++ b/drivers/usb/dwc3/dwc3-generic.c > @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct udevice *dev) > if (rc) > return rc; > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); > if (rc) > debug("%s: No vbus regulator found: %d\n", dev->name, rc); > @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct udevice *dev) > rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); > if (rc) > return rc; Here, if (rc && rc != -ENOSYS) or even if (CONFIG_IS_ENABLED(DM_REGULATOR) && rc) to be verbose (maybe not the preferred style though). All the other regulator_* calls either ignore the result or only print a debug message. > +#endif > > hccr = (struct xhci_hccr *)priv->gen_priv.base; > hcor = (struct xhci_hcor *)(priv->gen_priv.base + > HC_LENGTH(xhci_readl(&(hccr)->cr_capbase))); > > rc = xhci_register(dev, hccr, hcor); > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > if (rc) > regulator_set_enable_if_allowed(priv->vbus_supply, false); > +#endif > > return rc; > } > @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct udevice *dev) > static int dwc3_generic_host_remove(struct udevice *dev) > { > struct dwc3_generic_host_priv *priv = dev_get_priv(dev); > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > int rc; > +#endif > > /* This function always returns 0 */ > xhci_deregister(dev); > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > rc = regulator_set_enable_if_allowed(priv->vbus_supply, false); > if (rc) > debug("%s: Failed to disable vbus regulator: %d\n", dev->name, rc); > +#endif > > return dwc3_generic_remove(dev, &priv->gen_priv); > }
On Mon, Apr 15, 2024 at 12:57 PM Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 15/04/2024 11:53, Robert Marko wrote: > > Recent addition of vbus-supply support has broke platform which dont use > > controllable regulators for USB. > > > > Issue is that even withou DM_REGULATOR being enabled regulator related > > functions will still build as there is a stub in regulator.h but they will > > simply return -ENOSYS which will then make dwc3_generic_host_probe() > > return the same error thus breaking probe. > > Rather than stubbing out the code, could you check for -ENOSYS and > ignore the error in that case? I believe there's only one place where > this matters (marked below). Sure, that was my first approach but it did not seem right to me. But if its OK with you, I can do that. Regards, Robert > > > > Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > --- > > drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c > > index 7a00529a2a..784d3ec2ed 100644 > > --- a/drivers/usb/dwc3/dwc3-generic.c > > +++ b/drivers/usb/dwc3/dwc3-generic.c > > @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct udevice *dev) > > if (rc) > > return rc; > > > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > > rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); > > if (rc) > > debug("%s: No vbus regulator found: %d\n", dev->name, rc); > > @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct udevice *dev) > > rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); > > if (rc) > > return rc; > > Here, if (rc && rc != -ENOSYS) > or even if (CONFIG_IS_ENABLED(DM_REGULATOR) && rc) to be verbose (maybe > not the preferred style though). > > All the other regulator_* calls either ignore the result or only print a > debug message. > > +#endif > > > > hccr = (struct xhci_hccr *)priv->gen_priv.base; > > hcor = (struct xhci_hcor *)(priv->gen_priv.base + > > HC_LENGTH(xhci_readl(&(hccr)->cr_capbase))); > > > > rc = xhci_register(dev, hccr, hcor); > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > > if (rc) > > regulator_set_enable_if_allowed(priv->vbus_supply, false); > > +#endif > > > > return rc; > > } > > @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct udevice *dev) > > static int dwc3_generic_host_remove(struct udevice *dev) > > { > > struct dwc3_generic_host_priv *priv = dev_get_priv(dev); > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > > int rc; > > +#endif > > > > /* This function always returns 0 */ > > xhci_deregister(dev); > > > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > > rc = regulator_set_enable_if_allowed(priv->vbus_supply, false); > > if (rc) > > debug("%s: Failed to disable vbus regulator: %d\n", dev->name, rc); > > +#endif > > > > return dwc3_generic_remove(dev, &priv->gen_priv); > > } > > -- > // Caleb (they/them)
Hello all, On 2024-04-15 13:11, Robert Marko wrote: > On Mon, Apr 15, 2024 at 12:57 PM Caleb Connolly > <caleb.connolly@linaro.org> wrote: >> >> On 15/04/2024 11:53, Robert Marko wrote: >> > Recent addition of vbus-supply support has broke platform which dont use >> > controllable regulators for USB. >> > >> > Issue is that even withou DM_REGULATOR being enabled regulator related >> > functions will still build as there is a stub in regulator.h but they will >> > simply return -ENOSYS which will then make dwc3_generic_host_probe() >> > return the same error thus breaking probe. >> >> Rather than stubbing out the code, could you check for -ENOSYS and >> ignore the error in that case? I believe there's only one place where >> this matters (marked below). > > Sure, that was my first approach but it did not seem right to me. > But if its OK with you, I can do that. FWIW, ifdef'ing the code out seems like a cleaner approach to me. >> > Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") >> > Signed-off-by: Robert Marko <robert.marko@sartura.hr> >> > --- >> > drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c >> > index 7a00529a2a..784d3ec2ed 100644 >> > --- a/drivers/usb/dwc3/dwc3-generic.c >> > +++ b/drivers/usb/dwc3/dwc3-generic.c >> > @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct udevice *dev) >> > if (rc) >> > return rc; >> > >> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >> > rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); >> > if (rc) >> > debug("%s: No vbus regulator found: %d\n", dev->name, rc); >> > @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct udevice *dev) >> > rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); >> > if (rc) >> > return rc; >> >> Here, if (rc && rc != -ENOSYS) >> or even if (CONFIG_IS_ENABLED(DM_REGULATOR) && rc) to be verbose >> (maybe >> not the preferred style though). >> >> All the other regulator_* calls either ignore the result or only print >> a >> debug message. >> > +#endif >> > >> > hccr = (struct xhci_hccr *)priv->gen_priv.base; >> > hcor = (struct xhci_hcor *)(priv->gen_priv.base + >> > HC_LENGTH(xhci_readl(&(hccr)->cr_capbase))); >> > >> > rc = xhci_register(dev, hccr, hcor); >> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >> > if (rc) >> > regulator_set_enable_if_allowed(priv->vbus_supply, false); >> > +#endif >> > >> > return rc; >> > } >> > @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct udevice *dev) >> > static int dwc3_generic_host_remove(struct udevice *dev) >> > { >> > struct dwc3_generic_host_priv *priv = dev_get_priv(dev); >> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >> > int rc; >> > +#endif >> > >> > /* This function always returns 0 */ >> > xhci_deregister(dev); >> > >> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >> > rc = regulator_set_enable_if_allowed(priv->vbus_supply, false); >> > if (rc) >> > debug("%s: Failed to disable vbus regulator: %d\n", dev->name, rc); >> > +#endif >> > >> > return dwc3_generic_remove(dev, &priv->gen_priv); >> > } >> >> -- >> // Caleb (they/them)
On 15/04/2024 12:14, Dragan Simic wrote: > Hello all, > > On 2024-04-15 13:11, Robert Marko wrote: >> On Mon, Apr 15, 2024 at 12:57 PM Caleb Connolly >> <caleb.connolly@linaro.org> wrote: >>> >>> On 15/04/2024 11:53, Robert Marko wrote: >>> > Recent addition of vbus-supply support has broke platform which >>> dont use >>> > controllable regulators for USB. >>> > >>> > Issue is that even withou DM_REGULATOR being enabled regulator related >>> > functions will still build as there is a stub in regulator.h but >>> they will >>> > simply return -ENOSYS which will then make dwc3_generic_host_probe() >>> > return the same error thus breaking probe. >>> >>> Rather than stubbing out the code, could you check for -ENOSYS and >>> ignore the error in that case? I believe there's only one place where >>> this matters (marked below). >> >> Sure, that was my first approach but it did not seem right to me. >> But if its OK with you, I can do that. > > FWIW, ifdef'ing the code out seems like a cleaner approach to me. Given that the stub functions exist, imho it makes sense to use them. It's also easy to forget to add those #ifdefs if there are other regulator changes in future patches. But overall I don't feel too strongly about this, for what it does this code is fine, I'll defer to Marek. > >>> > Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus >>> regulator") >>> > Signed-off-by: Robert Marko <robert.marko@sartura.hr> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> >>> > --- >>> > drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++ >>> > 1 file changed, 8 insertions(+) >>> > >>> > diff --git a/drivers/usb/dwc3/dwc3-generic.c >>> b/drivers/usb/dwc3/dwc3-generic.c >>> > index 7a00529a2a..784d3ec2ed 100644 >>> > --- a/drivers/usb/dwc3/dwc3-generic.c >>> > +++ b/drivers/usb/dwc3/dwc3-generic.c >>> > @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct >>> udevice *dev) >>> > if (rc) >>> > return rc; >>> > >>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>> > rc = device_get_supply_regulator(dev, "vbus-supply", >>> &priv->vbus_supply); >>> > if (rc) >>> > debug("%s: No vbus regulator found: %d\n", dev->name, >>> rc); >>> > @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct >>> udevice *dev) >>> > rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); >>> > if (rc) >>> > return rc; >>> >>> Here, if (rc && rc != -ENOSYS) >>> or even if (CONFIG_IS_ENABLED(DM_REGULATOR) && rc) to be verbose (maybe >>> not the preferred style though). >>> >>> All the other regulator_* calls either ignore the result or only print a >>> debug message. >>> > +#endif >>> > >>> > hccr = (struct xhci_hccr *)priv->gen_priv.base; >>> > hcor = (struct xhci_hcor *)(priv->gen_priv.base + >>> > HC_LENGTH(xhci_readl(&(hccr)->cr_capbase))); >>> > >>> > rc = xhci_register(dev, hccr, hcor); >>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>> > if (rc) >>> > regulator_set_enable_if_allowed(priv->vbus_supply, >>> false); >>> > +#endif >>> > >>> > return rc; >>> > } >>> > @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct >>> udevice *dev) >>> > static int dwc3_generic_host_remove(struct udevice *dev) >>> > { >>> > struct dwc3_generic_host_priv *priv = dev_get_priv(dev); >>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>> > int rc; >>> > +#endif >>> > >>> > /* This function always returns 0 */ >>> > xhci_deregister(dev); >>> > >>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>> > rc = regulator_set_enable_if_allowed(priv->vbus_supply, false); >>> > if (rc) >>> > debug("%s: Failed to disable vbus regulator: %d\n", >>> dev->name, rc); >>> > +#endif >>> > >>> > return dwc3_generic_remove(dev, &priv->gen_priv); >>> > } >>> >>> -- >>> // Caleb (they/them)
Hello Caleb, On 2024-04-15 13:18, Caleb Connolly wrote: > On 15/04/2024 12:14, Dragan Simic wrote: >> Hello all, >> >> On 2024-04-15 13:11, Robert Marko wrote: >>> On Mon, Apr 15, 2024 at 12:57 PM Caleb Connolly >>> <caleb.connolly@linaro.org> wrote: >>>> >>>> On 15/04/2024 11:53, Robert Marko wrote: >>>> > Recent addition of vbus-supply support has broke platform which >>>> dont use >>>> > controllable regulators for USB. >>>> > >>>> > Issue is that even withou DM_REGULATOR being enabled regulator related >>>> > functions will still build as there is a stub in regulator.h but >>>> they will >>>> > simply return -ENOSYS which will then make dwc3_generic_host_probe() >>>> > return the same error thus breaking probe. >>>> >>>> Rather than stubbing out the code, could you check for -ENOSYS and >>>> ignore the error in that case? I believe there's only one place >>>> where >>>> this matters (marked below). >>> >>> Sure, that was my first approach but it did not seem right to me. >>> But if its OK with you, I can do that. >> >> FWIW, ifdef'ing the code out seems like a cleaner approach to me. > > Given that the stub functions exist, imho it makes sense to use them. > It's also easy to forget to add those #ifdefs if there are other > regulator changes in future patches. > > But overall I don't feel too strongly about this, for what it does this > code is fine, I'll defer to Marek. Oh, sorry, I totally failed to notice that the stubs already exist for the regulator_set_enable_if_allowed() function and its friends, which are actually ifdef'ed code. Having that in mind, I agree that introducing more ifdef's isn't the preferred way to go. Maybe there's something awkward about the stubs and the way they need to be used, but that should be resolved by improving the whole stubs thing, instead of introducing more ifdef's in the code that ends up using the stubs. >>>> > Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus >>>> regulator") >>>> > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> >>>> > --- >>>> > drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++ >>>> > 1 file changed, 8 insertions(+) >>>> > >>>> > diff --git a/drivers/usb/dwc3/dwc3-generic.c >>>> b/drivers/usb/dwc3/dwc3-generic.c >>>> > index 7a00529a2a..784d3ec2ed 100644 >>>> > --- a/drivers/usb/dwc3/dwc3-generic.c >>>> > +++ b/drivers/usb/dwc3/dwc3-generic.c >>>> > @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct >>>> udevice *dev) >>>> > if (rc) >>>> > return rc; >>>> > >>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>>> > rc = device_get_supply_regulator(dev, "vbus-supply", >>>> &priv->vbus_supply); >>>> > if (rc) >>>> > debug("%s: No vbus regulator found: %d\n", dev->name, >>>> rc); >>>> > @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct >>>> udevice *dev) >>>> > rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); >>>> > if (rc) >>>> > return rc; >>>> >>>> Here, if (rc && rc != -ENOSYS) >>>> or even if (CONFIG_IS_ENABLED(DM_REGULATOR) && rc) to be verbose >>>> (maybe >>>> not the preferred style though). >>>> >>>> All the other regulator_* calls either ignore the result or only >>>> print a >>>> debug message. >>>> > +#endif >>>> > >>>> > hccr = (struct xhci_hccr *)priv->gen_priv.base; >>>> > hcor = (struct xhci_hcor *)(priv->gen_priv.base + >>>> > HC_LENGTH(xhci_readl(&(hccr)->cr_capbase))); >>>> > >>>> > rc = xhci_register(dev, hccr, hcor); >>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>>> > if (rc) >>>> > regulator_set_enable_if_allowed(priv->vbus_supply, >>>> false); >>>> > +#endif >>>> > >>>> > return rc; >>>> > } >>>> > @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct >>>> udevice *dev) >>>> > static int dwc3_generic_host_remove(struct udevice *dev) >>>> > { >>>> > struct dwc3_generic_host_priv *priv = dev_get_priv(dev); >>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>>> > int rc; >>>> > +#endif >>>> > >>>> > /* This function always returns 0 */ >>>> > xhci_deregister(dev); >>>> > >>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>>> > rc = regulator_set_enable_if_allowed(priv->vbus_supply, false); >>>> > if (rc) >>>> > debug("%s: Failed to disable vbus regulator: %d\n", >>>> dev->name, rc); >>>> > +#endif >>>> > >>>> > return dwc3_generic_remove(dev, &priv->gen_priv); >>>> > } >>>> >>>> -- >>>> // Caleb (they/them)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index 7a00529a2a..784d3ec2ed 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct udevice *dev) if (rc) return rc; +#if CONFIG_IS_ENABLED(DM_REGULATOR) rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); if (rc) debug("%s: No vbus regulator found: %d\n", dev->name, rc); @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct udevice *dev) rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); if (rc) return rc; +#endif hccr = (struct xhci_hccr *)priv->gen_priv.base; hcor = (struct xhci_hcor *)(priv->gen_priv.base + HC_LENGTH(xhci_readl(&(hccr)->cr_capbase))); rc = xhci_register(dev, hccr, hcor); +#if CONFIG_IS_ENABLED(DM_REGULATOR) if (rc) regulator_set_enable_if_allowed(priv->vbus_supply, false); +#endif return rc; } @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct udevice *dev) static int dwc3_generic_host_remove(struct udevice *dev) { struct dwc3_generic_host_priv *priv = dev_get_priv(dev); +#if CONFIG_IS_ENABLED(DM_REGULATOR) int rc; +#endif /* This function always returns 0 */ xhci_deregister(dev); +#if CONFIG_IS_ENABLED(DM_REGULATOR) rc = regulator_set_enable_if_allowed(priv->vbus_supply, false); if (rc) debug("%s: Failed to disable vbus regulator: %d\n", dev->name, rc); +#endif return dwc3_generic_remove(dev, &priv->gen_priv); }
Recent addition of vbus-supply support has broke platform which dont use controllable regulators for USB. Issue is that even withou DM_REGULATOR being enabled regulator related functions will still build as there is a stub in regulator.h but they will simply return -ENOSYS which will then make dwc3_generic_host_probe() return the same error thus breaking probe. Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Robert Marko <robert.marko@sartura.hr> --- drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++ 1 file changed, 8 insertions(+)