Hi,
On 03-12-18 17:21, Stephan Gerhold wrote:
On Mon, Dec 03, 2018 at 11:50:53AM +0100, Hans de Goede wrote:
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.
The BIOS is not locked on my device either. Otherwise I could have
probably never installed something other than Android. It has a "locked"
Android bootloader, but they did not enable UEFI Secure Boot...
I just checked for the OS selection and there is a "Windows 8.X" /
"Android" selection in Advanced -> LPSS & SCC Configuration -> OS
Selection. I'm considering trying it out, but I have to admit that I'm
a bit cautious with changing anything in the BIOS because I'm not sure how
to recover if my tablet does not boot anymore at all after the changes. :(
If you ever feel adventurous in my experience the worst which can happen
is the LCD panel not lighting up, but you can then still recover be remembering
the order of keys to hit (on a keyboard connected over the otg) to load
optimal defaults and then safe the settings.
Also I don't expect toggling that setting to cause an issue like the tablet not
booting at all, but no guarantees.
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.
Alright, thanks! I will try it within the next weeks and see if I can
get it working.
Thanks.
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).
Hmm, it does use a Crystal Cove (CRC) PMIC but in the Android kernel
this was all done manually instead of through ACPI/AML code. On the
other hand, I've had it quite often that things were manually implemented
in the Android kernel but also work through some other way...
(e.g. DPTF thermal zones through ACPI)
The stock implementation has roughly the following:
- crystal_cove_pwrsrc driver which can only detect VBUS, not the
charger type (see [1]) - otherwise similar to extcon-intel-cht-wc
- charger type detection using the TUSB1211 PHY in the dwc3 code [2]
- bq24192 charger driver, although ASUS seems to have replaced most
of it with custom code, and kept all the unused code (...)
- uPI uG31xx fuel gauge [3], I believe this was custom chosen by ASUS;
haven't seen it on devices from other vendors
- a GPIO for OTG ID detection, used in bq24192 to enable the 5V boost
Especially the custom fuel gauge makes me think that the standard ACPI
procedure used on Windows devices is not really going to work here, but
I don't know for sure.
Is there any documentation/code for charger detection with the CRC PMIC?
It looks like I was wrong here, I checked the DSTDs of my devices with
a CRC PMIC and this seems to be handled by the embedded controller, which
your device likely does not have.
I have seen parts of your work for CHT Whiskey Cove for example, but
nothing for CRC..
I've uploaded a dump of the DSDT at [4] if you know what to look for.
Otherwise don't worry, it's not too important for me :)
My current solution works (mostly) - it's not great, but especially the
fuelgauge driver is in no shape for mainline. And given that this seems
to be device specific, it's easier for me to rebase it on top of
mainline updates. I will probably keep using it until my device dies,
and then let it rest in peace :)
Ok, quite a funky BYT implementation you have gotten yourself there :)
Regards,
Hans
[1]:
https://github.com/me176c-dev/me176c-kernel/blob/73143e30d9aae3d0a3875d4c5cfcbfc9040e9b04/kernel/drivers/platform/x86/intel_crystalcove_pwrsrc.c#L60
[2]:
https://github.com/me176c-dev/me176c-kernel/blob/73143e30d9aae3d0a3875d4c5cfcbfc9040e9b04/kernel/drivers/usb/dwc3/dwc3-intel-byt.c#L722-L920
[3]: https://www.upi-semi.com/en-article-upi-642-1611
[4]:
https://github.com/me176c-dev/me176c-acpi/blob/f48c78c11b0819b899f988407b6ece3d8c2cca71/dsdt.dsl
Regards,
Hans
