Hi,
On 02-12-18 18:08, Stephan Gerhold wrote:
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.
Ok.
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.
I've one BYT tablet which is/was Android only, which also has some seriously
borked DSTD, but it has an unlocked BIOS and I flipped the OS choice there
(this is a weird thing BYT BIOSes have) to Windows then all of sudden I got
the right DSTD. Asus BIOS-es are typically locked and don't show this option,
but it is probably worth it to take a look.
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?
Yes, I've not looked into this at al yet, but that sounds like the right
way to do it, esp. since we really want to do this from within the dwc3
driver and not polute the phy driver with this is possible.
It seems unlikely for me that there would be ever two BYT dwc3
PCI devices..
Yes that is pretty much impossible since the DWC3 is inside the SoC.
In any case, it might be easier if you submit that patch at some point.
You have more devices available for testing :)
I would be happy to test a patch for this if you write one. Note I actually
don't have that many devices with a tusb1210, Windows on these devices does
not do OTG, so usually the extra chip for the phy is left out on Windows
only designs.
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.
So does this device not use a Crystal Cove (CRC) PMIC or an AXP288 PMIC ?
Those both have charger detection builtin. Although with the CRC PMIC the
charging and bat-mon stuff is usually taken care of by AML code in the DSTD.
The AXP288 has its own builtin fuelgauge for which the mainline kernel has
a native driver (rather then using the ACPI BAT interface).
Regards,
Hans
