| Message ID | 1536303721-28606-1-git-send-email-saranya.gopal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V6] roles: Fix USB 3.0 OTG issue on Intel platform | expand |
On Fri, Sep 07, 2018 at 12:32:01PM +0530, saranya.gopal@intel.com wrote: > From: Saranya Gopal <saranya.gopal@intel.com> > > 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. > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > Signed-off-by: M Balaji <m.balaji@intel.com> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > changes since V5: Corrected the name format in From and Signed-off-by > changes since V4: Removed change-Id > changes since V3: Added Reviewed-by Sathyanarayanan tag > changes since V2: Incorporated Heikki's review comments and added Reviewed-by Heikki tag > changes since V1: none > > drivers/usb/roles/intel-xhci-usb-role-switch.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c > index dad2d19..0d1ea82 100644 > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c > @@ -25,6 +25,9 @@ > #define SW_VBUS_VALID BIT(24) > #define SW_IDPIN_EN BIT(21) > #define SW_IDPIN BIT(20) > +#define SW_SWITCH_EN_CFG0 BIT(16) Why is this not in the correct sorted order with the items above it? Is this a patch that should go to the stable trees? If so, which ones? thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Friday, September 07, 2018 12:49 PM > To: Gopal, Saranya <saranya.gopal@intel.com> > Cc: linux-usb@vger.kernel.org; heikki.krogerus@linux.intel.com; Kuppuswamy, > Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>; K V, Abhilash > <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com> > Subject: Re: [PATCH V6] roles: Fix USB 3.0 OTG issue on Intel platform > > On Fri, Sep 07, 2018 at 12:32:01PM +0530, saranya.gopal@intel.com wrote: > > From: Saranya Gopal <saranya.gopal@intel.com> > > > > 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. > > > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > Signed-off-by: M Balaji <m.balaji@intel.com> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Reviewed-by: Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> > > --- > > changes since V5: Corrected the name format in From and Signed-off-by > > changes since V4: Removed change-Id > > changes since V3: Added Reviewed-by Sathyanarayanan tag > > changes since V2: Incorporated Heikki's review comments and added > Reviewed-by Heikki tag > > changes since V1: none > > > > drivers/usb/roles/intel-xhci-usb-role-switch.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c > b/drivers/usb/roles/intel-xhci-usb-role-switch.c > > index dad2d19..0d1ea82 100644 > > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c > > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c > > @@ -25,6 +25,9 @@ > > #define SW_VBUS_VALID BIT(24) > > #define SW_IDPIN_EN BIT(21) > > #define SW_IDPIN BIT(20) > > +#define SW_SWITCH_EN_CFG0 BIT(16) > > Why is this not in the correct sorted order with the items above it? I added only the last one. Do you want me to sort the existing macros in BIT sorted order? > > Is this a patch that should go to the stable trees? If so, which ones? It is not applicable to any stable tree. Thanks, Saranya > > thanks, > > greg k-h
+Hans On Fri, Sep 07, 2018 at 12:32:01PM +0530, saranya.gopal@intel.com wrote: > From: Saranya Gopal <saranya.gopal@intel.com> > > 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. > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > Signed-off-by: M Balaji <m.balaji@intel.com> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > changes since V5: Corrected the name format in From and Signed-off-by > changes since V4: Removed change-Id > changes since V3: Added Reviewed-by Sathyanarayanan tag > changes since V2: Incorporated Heikki's review comments and added Reviewed-by Heikki tag > changes since V1: none > > drivers/usb/roles/intel-xhci-usb-role-switch.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c > index dad2d19..0d1ea82 100644 > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c > @@ -25,6 +25,9 @@ > #define SW_VBUS_VALID BIT(24) > #define SW_IDPIN_EN BIT(21) > #define SW_IDPIN BIT(20) > +#define SW_SWITCH_EN_CFG0 BIT(16) > +#define SW_DRD_STATIC_HOST_CFG0 1 > +#define SW_DRD_STATIC_DEV_CFG0 2 > > #define DUAL_ROLE_CFG1 0x6c > #define HOST_MODE BIT(29) > @@ -83,17 +86,22 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) > case USB_ROLE_NONE: > val |= SW_IDPIN; > val &= ~SW_VBUS_VALID; > + val &= ~(SW_DRD_STATIC_DEV_CFG0 | SW_DRD_STATIC_HOST_CFG0); > break; > case USB_ROLE_HOST: > val &= ~SW_IDPIN; > val &= ~SW_VBUS_VALID; > + val &= ~SW_DRD_STATIC_DEV_CFG0; > + val |= SW_DRD_STATIC_HOST_CFG0; > break; > case USB_ROLE_DEVICE: > val |= SW_IDPIN; > val |= SW_VBUS_VALID; > + val &= ~SW_DRD_STATIC_HOST_CFG0; > + val |= SW_DRD_STATIC_DEV_CFG0; > break; > } > - val |= SW_IDPIN_EN; > + val |= SW_IDPIN_EN | SW_SWITCH_EN_CFG0; > > writel(val, data->base + DUAL_ROLE_CFG0); > > -- > 2.7.4 Thanks,
Hi Greg, "Gopal, Saranya" <saranya.gopal@intel.com> writes: >> > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c >> b/drivers/usb/roles/intel-xhci-usb-role-switch.c >> > index dad2d19..0d1ea82 100644 >> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c >> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c >> > @@ -25,6 +25,9 @@ >> > #define SW_VBUS_VALID BIT(24) >> > #define SW_IDPIN_EN BIT(21) >> > #define SW_IDPIN BIT(20) >> > +#define SW_SWITCH_EN_CFG0 BIT(16) >> >> Why is this not in the correct sorted order with the items above it? they are sorted in descending order. /* register definition */ #define DUAL_ROLE_CFG0 0x68 #define SW_VBUS_VALID BIT(24) #define SW_IDPIN_EN BIT(21) #define SW_IDPIN BIT(20)
On Fri, Sep 07, 2018 at 11:33:23AM +0300, Felipe Balbi wrote: > > Hi Greg, > > "Gopal, Saranya" <saranya.gopal@intel.com> writes: > >> > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c > >> b/drivers/usb/roles/intel-xhci-usb-role-switch.c > >> > index dad2d19..0d1ea82 100644 > >> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c > >> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c > >> > @@ -25,6 +25,9 @@ > >> > #define SW_VBUS_VALID BIT(24) > >> > #define SW_IDPIN_EN BIT(21) > >> > #define SW_IDPIN BIT(20) > >> > +#define SW_SWITCH_EN_CFG0 BIT(16) > >> > >> Why is this not in the correct sorted order with the items above it? > > they are sorted in descending order. > > /* register definition */ > #define DUAL_ROLE_CFG0 0x68 > #define SW_VBUS_VALID BIT(24) > #define SW_IDPIN_EN BIT(21) > #define SW_IDPIN BIT(20) Ugh, you are right, that's what I get for trying to review code before my coffee has kicked in... Sorry about that. greg k-h
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@intel.com wrote: >> From: Saranya Gopal <saranya.gopal@intel.com> >> >> 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. Regards, Hans >> >> Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> >> Signed-off-by: M Balaji <m.balaji@intel.com> >> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> >> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> changes since V5: Corrected the name format in From and Signed-off-by >> changes since V4: Removed change-Id >> changes since V3: Added Reviewed-by Sathyanarayanan tag >> changes since V2: Incorporated Heikki's review comments and added Reviewed-by Heikki tag >> changes since V1: none >> >> drivers/usb/roles/intel-xhci-usb-role-switch.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c >> index dad2d19..0d1ea82 100644 >> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c >> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c >> @@ -25,6 +25,9 @@ >> #define SW_VBUS_VALID BIT(24) >> #define SW_IDPIN_EN BIT(21) >> #define SW_IDPIN BIT(20) >> +#define SW_SWITCH_EN_CFG0 BIT(16) >> +#define SW_DRD_STATIC_HOST_CFG0 1 >> +#define SW_DRD_STATIC_DEV_CFG0 2 >> >> #define DUAL_ROLE_CFG1 0x6c >> #define HOST_MODE BIT(29) >> @@ -83,17 +86,22 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) >> case USB_ROLE_NONE: >> val |= SW_IDPIN; >> val &= ~SW_VBUS_VALID; >> + val &= ~(SW_DRD_STATIC_DEV_CFG0 | SW_DRD_STATIC_HOST_CFG0); >> break; >> case USB_ROLE_HOST: >> val &= ~SW_IDPIN; >> val &= ~SW_VBUS_VALID; >> + val &= ~SW_DRD_STATIC_DEV_CFG0; >> + val |= SW_DRD_STATIC_HOST_CFG0; >> break; >> case USB_ROLE_DEVICE: >> val |= SW_IDPIN; >> val |= SW_VBUS_VALID; >> + val &= ~SW_DRD_STATIC_HOST_CFG0; >> + val |= SW_DRD_STATIC_DEV_CFG0; >> break; >> } >> - val |= SW_IDPIN_EN; >> + val |= SW_IDPIN_EN | SW_SWITCH_EN_CFG0; >> >> writel(val, data->base + DUAL_ROLE_CFG0); >> >> -- >> 2.7.4 > > Thanks, >
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@intel.com wrote: > > > From: Saranya Gopal <saranya.gopal@intel.com> > > > > > > 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,
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@intel.com wrote: >>>> From: Saranya Gopal <saranya.gopal@intel.com> >>>> >>>> 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
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@intel.com wrote: > > > > > From: Saranya Gopal <saranya.gopal@intel.com> > > > > > > > > > > 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,
diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c index dad2d19..0d1ea82 100644 --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c @@ -25,6 +25,9 @@ #define SW_VBUS_VALID BIT(24) #define SW_IDPIN_EN BIT(21) #define SW_IDPIN BIT(20) +#define SW_SWITCH_EN_CFG0 BIT(16) +#define SW_DRD_STATIC_HOST_CFG0 1 +#define SW_DRD_STATIC_DEV_CFG0 2 #define DUAL_ROLE_CFG1 0x6c #define HOST_MODE BIT(29) @@ -83,17 +86,22 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) case USB_ROLE_NONE: val |= SW_IDPIN; val &= ~SW_VBUS_VALID; + val &= ~(SW_DRD_STATIC_DEV_CFG0 | SW_DRD_STATIC_HOST_CFG0); break; case USB_ROLE_HOST: val &= ~SW_IDPIN; val &= ~SW_VBUS_VALID; + val &= ~SW_DRD_STATIC_DEV_CFG0; + val |= SW_DRD_STATIC_HOST_CFG0; break; case USB_ROLE_DEVICE: val |= SW_IDPIN; val |= SW_VBUS_VALID; + val &= ~SW_DRD_STATIC_HOST_CFG0; + val |= SW_DRD_STATIC_DEV_CFG0; break; } - val |= SW_IDPIN_EN; + val |= SW_IDPIN_EN | SW_SWITCH_EN_CFG0; writel(val, data->base + DUAL_ROLE_CFG0);