Hi,
thanks for the quick reply! :)
On Sun, Dec 02, 2018 at 03:41:29PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01-12-18 13:22, Stephan Gerhold wrote:
> > Hi,
> >
> > I have been updating my Intel Baytrail tablet from 4.14 to 4.19 and
> > noticed that the tusb1210 PHY driver now fails to probe on 4.19.x and
> > 4.20-rc4:
> >
> > tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)
> >
> > The commit that broke this is 211f658b7b40 ("usb: dwc3: pci: Use devm
> > functions to get the phy GPIOs") because it now claims the phy GPIOs
> > permanently so the PHY driver (here: tusb1210) cannot get them.
> >
> > Gadget mode still works with this error and even completely without the
> > tusb1210 driver, but I have been using the PHY driver additionally
> > because it has the advantage that it will also power off the PHY during
> > suspend. (Plus I have some custom hacky code for charger detection in
> > it at the moment, but I did not have it applied for testing this
> > problem..)
> >
> > The comment that still exists above the changed code also mentions why
> > the GPIO descriptors were put immediately after use:
> >
> > /*
> > * These GPIOs will turn on the USB2 PHY. Note that we have to
> > * put the gpio descriptors again here because the phy driver
> > * might want to grab them, too.
> > */
> >
> > Reverting the commit makes everything work again for me. Does this
> > change fix a problem on another device or was it mostly just cleanup?
>
> It was just a cleanup, I did not realize that another driver would be
> claiming the GPIOs and I noticed them not being claimed in
> "cat /sys/kernel/debug/gpio" while testing.
>
> But it makes sense that the PHY driver itsef wants to use the GPIOs,
> which is likely why the dwc3-pci code was written the way it was
> in the first place.
>
> So yes my commit should be reverted. Do you want to submit a revert,
> or shall I ?
Hmm, let me try to send it :) I have it mostly prepared locally, just
wanted to ask before I send it.
>
> > I wanted to suggest that turning on the PHY may not actually be
> > necessary in dwc3-pci since the tusb1210 driver has the same code in its
> > probe() method. However, when I tested it it did not work at all because
> > the tusb1210 driver is not even loaded if the PHY is not turned on
> > before dwc3 is loaded...
> > (The ULPI vendor/product ID is wrong with the PHY turned off)
>
> Right, the phy must be turned on, so that the dwc3 driver can create
> the struct device representing the phy with the right vendor/product id
> in its modalias. This is why the dwc3 code turns it on.
>
> > Additional note: The probe error above happens only if:
> > - The tablet has a TUSB1210/TUSB1211 external PHY (not sure if this is
> > the case for all Baytrail tablets..)
> > - CONFIG_USB_DWC3_ULPI and CONFIG_PHY_TUSB1210 are enabled
> > - GPIOs are defined in the ACPI DSDT table (Unlike the ACPI GPIOs, the
> > fallback GPIO mappings are not inherited to the ULPI device..)
>
> This is why I was not seeing this problem, all my devices lack the GPIO
> definition in the ACPI tables. I've put looking into adding the fallback
> GPIOs to the tusb1210 driver too on my TODO list, but this is rather low
> priority for me.
Somewhat off topic, but:
To be exact my device does not have them in the (original) ACPI table
either. At the moment I override the ACPI DSDT table because there are
some things I haven't found other workarounds for. (It's horrible..)
Some examples:
- The BCM2E3A Bluetooth device is listed as child under the wrong UART
controller device, so the kernel never manages to power it up properly
- The rt5640 (audio codec) device has one of the Bluetooth GPIOs listed
as interrupt instead of the correct one
- The GPIOs for the goodix touchscreen were hardcoded in the stock kernel,
and are therefore entirely missing in the ACPI table
Granted, this device was never intended to be used with a generic
kernel (see my response to your question what hardware I have below),
but nevertheless it is really annoying. So when the fallback mappings
didn't exist yet it was easier for me to add them to the DSDT myself.
As for adding the fallback GPIOs for tusb1210: I was considering this
as well, but the main difficulty for me was that the ULPI device gets
a dynamic device name based on the one from dwc3, e.g. "dwc3.0.auto.ulpi"
Do you think it would be enough to define another static
`gpiod_lookup_table` and replace its `dev_id` with the correct one at
runtime? It seems unlikely for me that there would be ever two BYT dwc3
PCI devices..
In any case, it might be easier if you submit that patch at some point.
You have more devices available for testing :)
>
> May I ask what hardware you have? You really should not need to do
> phy hacks for the charger detection (I've written and/or touched most
> of the charger-detect code for BYT/CHT platforms).
Sure! It's a ASUS MeMO Pad 7 (ME176C), a BYT-T tablet that was shipped
with Android. As commonly done for Android devices, it has an absolutely
horrible custom charger- and battery driver in the stock kernel... (sigh)
As far as I was able to figure out based on the stock kernel, at least the
Android devices do charger detection through a TUSB1211 PHY. This can be
seen in drivers/usb/dwc3/dwc3-intel-byt.c in the stock kernel for these
devices, e.g. in [1].
At the moment I have some custom code that does something similar to [1]
(more simple and less reliable) in the tusb1210 driver, and then hands
the result to the horrible battery/charger drivers that I copied (with a
lot of cleanup) from the stock kernel.
[1]: https://github.com/me176c-dev/me176c-kernel/blob/73143e30d9aae3d0a3875d4c5cfcbfc9040e9b04/kernel/drivers/usb/dwc3/dwc3-intel-byt.c#L722-L920
>
> Regards,
>
> Hans
>
