On Mon, Sep 10, 2018 at 12:17:56PM +0200, Hans de Goede wrote:
> 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.
Yes, I agree that we can not use this patch as it is. We need
a solution that does not break CHT for upstream.
Thanks,
--
heikki
