Re: [PATCH] usb: roles: intel_xhci: Don't use PMIC for port type identification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 04/20/2018 04:35 PM, Heikki Krogerus wrote:
On Fri, Apr 20, 2018 at 11:16:05AM +0200, Hans de Goede wrote:
Hi Heikki,

On 20-04-18 10:06, Heikki Krogerus wrote:
This will add an array of known USB Type-C Port devices that
will be used as a blacklist for enabling userspace-control,
and remove the PMIC ACPI HID which was used for the same
purpose.

It turns out that on some CHT based platforms the X-Powers
PMIC is handled in firmware. The ACPI HID for it is
therefore not usable for determining the port type. The
driver now searches for known USB Type-C port devices
instead.

Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
---
Hi Hans,

So it seems that we can't rely on the PMIC. This is my proposal for a
fix. I'm in practice just using blacklist instead of whitelist for
identifying the port type.

Let me know if it's OK to you.

I'm afraid that this is not going to work, almost all CHT devices
(and some BYT devices to) define an INT33FE device, here is a quick grep
on a directory where I store dsdt files from various CHT and BYT devices:

[hans@shalem ~]$ ls /home/archive/hwinfo | wc -l
64
[hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l
36

So 36 out of 64 devices define an INT33FE device and at least half
of the devices there are BYT.

That does not tell you anything. You need to check the status of the
device to know if it's there or not:

         % grep . /sys/bus/acpi/devices/INT33FE*/status

The INT33FE device is described as a "XPOWER Battery Device", so I'm
not sure why you are checking for this to determine if the Type-C
connector is controlled by firmware.

Well, INT33F4 has DOS Device Name set to "XPOWER PMIC Controller". It
does not give any more hints regarding the USB connector.

INT33F4 is the AXP288 PMIC controller and in general boards with that
PMIC do not have Type-C functionality, as described in my
list of devices some devices do have a Type-C connector, but they
use it as a glorified micro-usb connector.

I only used INT33FE quite simply because it is the HID used in the
driver that populates the device for fusb302.

INT33FE is some weird messed up PMIC co-device used for battery
monitoring combined with a proprietary opregion which is registered
by the INT33FE driver.

The driver registering the fusb302 device has the following checks
to make sure the INT33FE it is dealing with is paired with a
Whiskey Cove PMIC:


        status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
        if (ACPI_FAILURE(status)) {
                dev_err(dev, "Error getting PTYPE\n");
                return -ENODEV;
        }

        /*
         * The same ACPI HID is used for different configurations check PTYP
         * to ensure that we are dealing with the expected config.
         */
        if (ptyp != EXPECTED_PTYPE)
                return -ENODEV;

        /* Check presence of INT34D3 (hardware-rev 3) expected for ptype == 4 */
        if (!acpi_dev_present("INT34D3", "1", 3)) {
                dev_err(dev, "Error PTYPE == %d, but no INT34D3 device\n",
                        EXPECTED_PTYPE);
                return -ENODEV;
        }

At least for CHT there seems to be a 1:1 relationship between which PMIC
is used and if Type-C functionality is available.



But I can see now that _HID INT33FE is in practice used as the
identifier for a "type" of devices instead of identifier for specific
IP. That to be honest is pretty scary. _CID could have been used for
that purpose, but every IP should have its own _HID. It's of course
too late to complain about that.

INT33FE does not indeed seem like usable for this case.

I have a bunch of CHT devices with a Type-C connector:

GPD win:
--------
This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller,
pi3usb30532. Which is all driven by the tcpm code.

Chuwi Hi8 Pro and Chuwi Vi8 Plus:
---------------------------------
These use a Type-C connector as a glorified micro-usb connector,
the Hi8 Pro supports Superspeed using a hardwired asmedia switch
to deal with upright / upside-down insertion. The Vi8 Plus is
USB2 only.

Host/device mode is determine by treating the Cc pin as an id-pin
(it has a gpio connected which is either low or high depending
on the Cc being pulled up/down).

These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger
detection there is no PD and no 3A / 1.5A / 0.5A detection based
on the Cc pull up resistor, resulting in charging at 0.5 A
when using a Type-C charger which does not do BC1.2 on its
USB2 lines.

As said these really treat the Type-C connector as a micro-sub
connector, so we need userspace control to be able to switch
to host mode when using an otg charging-hub (so that the user can
still use devices attached to the hub while charging).

Asus T100HA and Lenovo Ideapad Miix 320:
----------------------------------------
These have an USB host only Type-C connector, which does
do Superspeed (likely contain a fixed switch to deal with upright /
upside-down insertion).

These cannot charge at all through the Type-C connector (they
do turn of the 5v boost when used in device mode), theoretically
the port may work in device mode (depending on which lanes they
used), but the device-mode USB controller is disabled by the BIOS
with no option to change it.

In these cases we don't really need the mux. Actually, we probable
should not even register the driver for it in these cases.

I also have 5 different CHT devices with a micro-usb connector:
-Cube iWork8 Air
-Jumper Ezpad mini 3
-Pipo W2s
-Teclast X80 pro
-Lenovo Ideapad Miix 310

All of which use an AXP288 PMIC and can do charging, host and
device mode through their micro-usb connector with the
exception of the Lenovo Ideapad Miix 310 where the micro-usb
is host mode only (like the Type-C on the 320) and there is
a separate power-barrel connector for charging.

All these define an INT33FE device, although at least on
the Cube and Lenovo ones the INT33FE's device _STA returns 0,
so your check should not match, but on others the _STA for
the INT33FE device does return 0x0f.

TL;DR: Checking the INT33FE device to determine if a Type-C
connector is present is not a good way to handle this.

Fair enough.

Can you describe the device on which you are seeing
userspace control being enabled even though it should not be
enabled and maybe mail me an ACPI dump for it?

The other way arround. The user-space control is not enabled when it
should be.

It's somekind of an automotive platform. I don't have the ACPI dump,
nor access to one, but I can ask for it. Right now I just know that
INT33F4 is not enabled on that platform, but user space control is
needed.

So what PMIC are they using ? Is is a CHT platform? I've done all
my work on CHT platforms so the current whitelist is PMIC based
and CHT only really.

Braswells use CHT xHCI PCI ID AFAIK, so I checked the ACPI tables I
have for them. There was no node for INT33F4 at all. It could be that
they are using a Braswell based board.

In any case, it seems to be really difficult to reliable determine the
cases where we need the userspace control. Perhaps we should just
enable the control always with this mux driver after all? It's not
ideal when the port is Type-C port, but I don't think there is any
real risk of, for example, the user being able to break his/her board
with it at least.

Always enabling user control of the mux is fine with me.

Regards,

Hans


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux