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. :(
> > 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.
>
> > > 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?
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 :)
[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
