On Sun, Dec 09, 2018 at 04:28:07PM +0100, Hans de Goede wrote:
> Hi,
>
> On 09-12-18 16:07, Stephan Gerhold wrote:
> > On Sun, Dec 09, 2018 at 04:44:16PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 6, 2018 at 8:49 PM Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> > > >
> > > > Commit 211f658b7b40 ("usb: dwc3: pci: Use devm functions to get
> > > > the phy GPIOs") changed the code to claim the PHY GPIOs permanently
> > > > for Intel Baytrail devices.
> > > >
> > > > This causes issues when the actual PHY driver attempts to claim the
> > > > same GPIO descriptors. For example, tusb1210 now fails to probe with:
> > > >
> > > > tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)
> > > >
> > > > dwc3-pci needs to turn on the PHY once before dwc3 is loaded, but
> > > > usually the PHY driver will then hold the GPIOs to turn off the
> > > > PHY when requested (e.g. during suspend).
> > > >
> > > > To fix the problem, this reverts the commit to restore the old
> > > > behavior to put the GPIOs immediately after usage.
> > > >
> > > > Link: https://www.spinics.net/lists/linux-usb/msg174681.html
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > > > ---
> > > > drivers/usb/dwc3/dwc3-pci.c | 8 ++++----
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > > > index 842795856bf4..fdc6e4e403e8 100644
> > > > --- a/drivers/usb/dwc3/dwc3-pci.c
> > > > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > > > @@ -170,20 +170,20 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
> > > > * put the gpio descriptors again here because the phy driver
> > > > * might want to grab them, too.
> > > > */
> > > > - gpio = devm_gpiod_get_optional(&pdev->dev, "cs",
> > > > - GPIOD_OUT_LOW);
> > > > + gpio = gpiod_get_optional(&pdev->dev, "cs", GPIOD_OUT_LOW);
> > > > if (IS_ERR(gpio))
> > > > return PTR_ERR(gpio);
> > > >
> > > > gpiod_set_value_cansleep(gpio, 1);
> > > > + gpiod_put(gpio);
> > > >
> > > > - gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> > > > - GPIOD_OUT_LOW);
> > > > + gpio = gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
> > > > if (IS_ERR(gpio))
> > > > return PTR_ERR(gpio);
> > > >
> > > > if (gpio) {
> > > > gpiod_set_value_cansleep(gpio, 1);
> > >
> > > > + gpiod_put(gpio);
> > > > usleep_range(10000, 11000);
> > >
> > > If something happens to GPIO line in between of these lines, the sleep
> > > might become obsolete. Shouldn't gpiod_put() be placed after?
> >
> > That's a good point, but I believe this would be more appropriately
> > fixed in a separate patch, since this is just an exact revert of
> > 211f658b7b40 ("usb: dwc3: pci: Use devm functions to get the phy GPIOs")
> > (This is the way it was written when it was added to mainline 4 years
> > ago...)
> >
> > I can send a separate patch for this, or would you like to?
>
> Properly fixing this would require releasing *both* GPIOs *after* the
> ULPI vendor and product IDs have been read. Which will require adding
> some generic callback just for this to the generic non platform/pci
> specific dwc3 code. Which IMHO is not worth the trouble since in
> practice this is not really a problem.
Hmm, yeah, I was thinking about a specific situation where it would have
made a difference if both GPIOs were put after the delay (not after the
ULPI vendor/product IDs have been read). But now that I think about it
again, it does not make much sense... Never mind. (Maybe Andy has
another comment here..)
>
> Regards,
>
> Hans
>
