Hi,
On 10-09-18 12:13, Heikki Krogerus wrote:
On Sun, Sep 09, 2018 at 01:18:45PM +0200, Hans de Goede wrote:
Hi Saranya, Heikki,
On 07-09-18 10:18, Heikki Krogerus wrote:
+Hans
On Fri, Sep 07, 2018 at 12:32:01PM +0530, saranya.gopal@xxxxxxxxx wrote:
From: Saranya Gopal <saranya.gopal@xxxxxxxxx>
This patch adds static DRD mode for host/device
mode switch. This fixes the issue where device
mode was not working after DUT switches to host
mode with 3.0 OTG connector.
What is the DUT?
Anyways this patch cannot go upstream as is, it will break
DRD switching on many Cherry Trail devices, on Cherry Trail
devices the role is often controlled through firmware using
these 2 ACPI methods called from an edge triggered _AEI
handler:
Scope (PCI0)
{
OperationRegion (XOP1, SystemMemory, BR0X, 0x80F8)
Field (XOP1, DWordAcc, NoLock, Preserve)
{
Offset (0x80D4),
, 11,
BT11, 1,
, 8,
BT20, 1,
BT21, 1,
Offset (0x80D7),
BT24, 1
}
Method (CDRH, 1, Serialized)
{
If (DAMT == Zero)
{
BT20 = Zero
If (Arg0 == Zero)
{
BT24 = Zero
}
Else
{
BT24 = One
}
BT11 = One
BT21 = One
}
Else
{
// same thing through IOSF side bus
}
}
Method (CDRD, 1, Serialized)
{
If (DAMT == Zero)
{
BT11 = One
BT20 = One
BT21 = One
If (Arg0 == Zero)
{
BT24 = Zero
}
Else
{
BT24 = One
}
}
Else
{
// same thing through IOSF side bus
}
}
Note that these only control the SW_IDPIN (bit 20) to
change the role and rely on SW_SWITCH_ENABLE (bit 16)
and DRD_CONFIG (bit 0:1) to be all 0.
This patch as suggested breaks these ACPI methods.
Note that even though the mux is changed by firmware the
intel_xhci_usb_set_role() may (and typically will) still
run on these devices, there are 2 ways this can happen:
1) On many devices the firmware only switches between
the host and none roles (it does not set the SW_VBUS_VALID
bit), because it is targeting Windows.
This means that device mode cannot work because the
designware dwc3 controller will not operate as a device
without the SW_VBUS_VALID bit set.
To fix this the axp288_extcon code monitors vbus changes
and when the vbus becomes present it checks if the firmware
put the role-switch in the none role and if it did it changes
the role to device, calling intel_xhci_usb_set_role()
2) The user may manually change the role through sysfs, to
deal with otg-charging hubs (ACA devices) which this hardware
typically cannot detect.
If the proposed change is necessary to fix things on some non
Cherry Trail hardware, you could do a new version of this
patch which only does this on affected hardware.
It seems that the current driver does not work on several Intel
platforms. I think many of these platforms do have ACPI methods that
program the mux, but in most cases they are only executed if a _DSM is
called (for example Joule). The Cherry trail boards that execute those
methods as a result of a GPE seem to be the exception.
I think we've made a mistake with the driver. It now basically assumes
that there is always something like firmware or AML that also
configures the mux, and it really can not work like that. By default
the driver must work so that it is the only entity that programs the
mux, and it is in complete control over it. Platforms where this is
not the case, like the Cherry trails, must be handled as exceptions.
I actually think that the driver should be made like that even if
solutions that were used on Cherry trails were more common (which they
aren't) because of the fact that we simply should never be competing
over a resource with the firmware. If the firmware has control over
the mux, we do not touch it in OS. If the OS is in control of it, the
firmware does not touch it. Any other scenario is an exception, and
should be handled as such for example by using quirks.
Either way the patch as proposed will break CHT platforms, so the
setting of the new bits controlled by the patch needs to be made
conditional. I'm fine with making the new behavior the default and doing
it everywhere except on CHT.
Regards,
Hans
