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.
Thanks,
--
heikki
