[PATCH 0/2] usb: roles: intel: Use static mode by default

[PATCH 0/2] usb: roles: intel: Use static mode by default

[Heikki Krogerus]

Hi Hans,

These have been in my queue for a while now. For some reason I've been
under the impression that there was still an issue with them, but that
was a misunderstanding. Saranya and Balaji, I'm really sorry about
that.

Hans, I don't know if you remember these, but they address an issue
where the device mode does not work (I think on APL). I believe static
mode is used always except on Cherrytrail. You had reported that using the
static mode creates a conflict on some CHT boards that have ACPI tables that
also write to the mux registers. To prevent the use of the static mode on
Cherrytrail the property is used.

thanks,

Saranya Gopal (2):
  usb: xhci: ext-caps: Add property to disable Intel SW switch
  usb: roles: intel: Enable static DRD mode for role switch

 drivers/usb/host/xhci-ext-caps.c              | 18 +++++++++++++
 .../usb/roles/intel-xhci-usb-role-switch.c    | 26 ++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

-- 
2.23.0.rc1

[PATCH 1/2] usb: xhci: ext-caps: Add property to disable Intel SW switch

[Heikki Krogerus]

From: Saranya Gopal <saranya.gopal@intel.com>

In platforms like Intel Cherrytrail, 'SW switch enable' bit
should not be enabled for role switch. This patch adds a
property to Intel USB Role Switch platform driver to denote
that SW switch should be disabled in Cherrytrail devices.

Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
Signed-off-by: Balaji Manoharan <m.balaji@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/host/xhci-ext-caps.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/usb/host/xhci-ext-caps.c b/drivers/usb/host/xhci-ext-caps.c
index 399113f9fc5c..f498160df969 100644
--- a/drivers/usb/host/xhci-ext-caps.c
+++ b/drivers/usb/host/xhci-ext-caps.c
@@ -6,11 +6,20 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/pci.h>
 #include "xhci.h"
 
 #define USB_SW_DRV_NAME		"intel_xhci_usb_sw"
 #define USB_SW_RESOURCE_SIZE	0x400
 
+#define PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI	0x22b5
+
+static const struct property_entry role_switch_props[] = {
+	PROPERTY_ENTRY_BOOL("sw_switch_disable"),
+	{},
+};
+
 static void xhci_intel_unregister_pdev(void *arg)
 {
 	platform_device_unregister(arg);
@@ -21,6 +30,7 @@ static int xhci_create_intel_xhci_sw_pdev(struct xhci_hcd *xhci, u32 cap_offset)
 	struct usb_hcd *hcd = xhci_to_hcd(xhci);
 	struct device *dev = hcd->self.controller;
 	struct platform_device *pdev;
+	struct pci_dev *pci = to_pci_dev(dev);
 	struct resource	res = { 0, };
 	int ret;
 
@@ -43,6 +53,14 @@ static int xhci_create_intel_xhci_sw_pdev(struct xhci_hcd *xhci, u32 cap_offset)
 		return ret;
 	}
 
+	if (pci->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
+		ret = platform_device_add_properties(pdev, role_switch_props);
+		if (ret) {
+			dev_err(dev, "failed to register device properties\n");
+			return ret;
+		}
+	}
+
 	pdev->dev.parent = dev;
 
 	ret = platform_device_add(pdev);
-- 
2.23.0.rc1

[PATCH 2/2] usb: roles: intel: Enable static DRD mode for role switch

[Heikki Krogerus]

From: Saranya Gopal <saranya.gopal@intel.com>

Enable static DRD mode in Intel platforms which guarantees
successful role switch all the time. This fixes issues like
software role switch failure after cold boot and issue with
role switch when USB 3.0 cable is used. But, do not enable
static DRD mode for Cherrytrail devices which rely on firmware
for role switch.

Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
Signed-off-by: Balaji Manoharan <m.balaji@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../usb/roles/intel-xhci-usb-role-switch.c    | 26 ++++++++++++++++++-
 1 file changed, 25 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 7325a84dd1c8..9101ff94c8d2 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/usb/role.h>
 
 /* register definition */
@@ -26,6 +27,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)
@@ -37,6 +41,7 @@
 struct intel_xhci_usb_data {
 	struct usb_role_switch *role_sw;
 	void __iomem *base;
+	bool disable_sw_switch;
 };
 
 static const struct software_node intel_xhci_usb_node = {
@@ -63,23 +68,39 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 
 	pm_runtime_get_sync(dev);
 
-	/* Set idpin value as requested */
+	/*
+	 * Set idpin value as requested.
+	 * Since some devices rely on firmware setting DRD_CONFIG and
+	 * SW_SWITCH_EN_CFG0 bits to be zero for role switch,
+	 * do not set these bits for those devices.
+	 */
 	val = readl(data->base + DUAL_ROLE_CFG0);
 	switch (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;
+		if (!data->disable_sw_switch) {
+			val &= ~SW_DRD_STATIC_DEV_CFG0;
+			val |= SW_DRD_STATIC_HOST_CFG0;
+		}
 		break;
 	case USB_ROLE_DEVICE:
 		val |= SW_IDPIN;
 		val |= SW_VBUS_VALID;
+		if (!data->disable_sw_switch) {
+			val &= ~SW_DRD_STATIC_HOST_CFG0;
+			val |= SW_DRD_STATIC_DEV_CFG0;
+		}
 		break;
 	}
 	val |= SW_IDPIN_EN;
+	if (!data->disable_sw_switch)
+		val |= SW_SWITCH_EN_CFG0;
 
 	writel(val, data->base + DUAL_ROLE_CFG0);
 
@@ -156,6 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	sw_desc.allow_userspace_control = true,
 	sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
 
+	data->disable_sw_switch = device_property_read_bool(dev,
+						"sw_switch_disable");
+
 	data->role_sw = usb_role_switch_register(dev, &sw_desc);
 	if (IS_ERR(data->role_sw)) {
 		fwnode_handle_put(sw_desc.fwnode);
-- 
2.23.0.rc1

Re: [PATCH 0/2] usb: roles: intel: Use static mode by default

[Hans de Goede]

Hi Heikki,

On 26-08-19 16:32, Heikki Krogerus wrote:
> Hi Hans,
> 
> These have been in my queue for a while now. For some reason I've been
> under the impression that there was still an issue with them, but that
> was a misunderstanding. Saranya and Balaji, I'm really sorry about
> that.
> 
> Hans, I don't know if you remember these, but they address an issue
> where the device mode does not work (I think on APL). I believe static
> mode is used always except on Cherrytrail. You had reported that using the
> static mode creates a conflict on some CHT boards that have ACPI tables that
> also write to the mux registers. To prevent the use of the static mode on
> Cherrytrail the property is used.

I've given this a test-run on a Cherry Trail device which used ACPI
_AEI handlers to do the mux switching as well as on 2 models (1 with
micro-usb id-pin, one with a Type-C fusb302 controller) where the
kernel does the switching.

I can confirm that with these patches applied things still work fine
on all 3 models.

With that said I do have some review remarks on the second patch
I will reply to that patch with my remarks.

Regards,

Hans



> Saranya Gopal (2):
>    usb: xhci: ext-caps: Add property to disable Intel SW switch
>    usb: roles: intel: Enable static DRD mode for role switch
> 
>   drivers/usb/host/xhci-ext-caps.c              | 18 +++++++++++++
>   .../usb/roles/intel-xhci-usb-role-switch.c    | 26 ++++++++++++++++++-
>   2 files changed, 43 insertions(+), 1 deletion(-)
> 

Re: [PATCH 2/2] usb: roles: intel: Enable static DRD mode for role switch

[Hans de Goede]

Hi,

On 26-08-19 16:32, Heikki Krogerus wrote:
> From: Saranya Gopal <saranya.gopal@intel.com>
> 
> Enable static DRD mode in Intel platforms which guarantees
> successful role switch all the time. This fixes issues like
> software role switch failure after cold boot and issue with
> role switch when USB 3.0 cable is used. But, do not enable
> static DRD mode for Cherrytrail devices which rely on firmware
> for role switch.
> 
> Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> Signed-off-by: Balaji Manoharan <m.balaji@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>   .../usb/roles/intel-xhci-usb-role-switch.c    | 26 ++++++++++++++++++-
>   1 file changed, 25 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 7325a84dd1c8..9101ff94c8d2 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -19,6 +19,7 @@
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/property.h>
>   #include <linux/usb/role.h>
>   
>   /* register definition */
> @@ -26,6 +27,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)

Nitpick: Please drop the _CFG0 postfix, if anything this
should be a prefix applied to *all* defines for the bits
in this register

> +#define SW_DRD_STATIC_HOST_CFG0		1
> +#define SW_DRD_STATIC_DEV_CFG0		2

So bits 0-1 together define the drd-mode. The datasheet
calls the combination DRD_CONFIG, without a SW_ prefix,
with the following values being defined:

00: Dynamic role-switch
01: Static Host mode
10: Static device mode
11: Reserved

Notice this is an enum, so the use of bit-ops to clear the
other state below is wrong. It happens to work, but this is
not how a multi-bit setting should be modified.

I suggest using the following defines instead:

#define DRD_CONFIG_DYNAMIC		0
#define DRD_CONFIG_STATIC_HOST		1
#define DRD_CONFIG_STATIC_DEVICE	2
#define DRD_CONFIG_MASK			3

>   #define DUAL_ROLE_CFG1			0x6c
>   #define HOST_MODE			BIT(29)
> @@ -37,6 +41,7 @@
>   struct intel_xhci_usb_data {
>   	struct usb_role_switch *role_sw;
>   	void __iomem *base;
> +	bool disable_sw_switch;

I would prefer for this to be enable_sw_switch and the negation
when reading the property.

>   };
>   
>   static const struct software_node intel_xhci_usb_node = {
> @@ -63,23 +68,39 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
>   
>   	pm_runtime_get_sync(dev);
>   
> -	/* Set idpin value as requested */
> +	/*
> +	 * Set idpin value as requested.
> +	 * Since some devices rely on firmware setting DRD_CONFIG and
> +	 * SW_SWITCH_EN_CFG0 bits to be zero for role switch,
> +	 * do not set these bits for those devices.
> +	 */
>   	val = readl(data->base + DUAL_ROLE_CFG0);
>   	switch (role) {
>   	case USB_ROLE_NONE:
>   		val |= SW_IDPIN;
>   		val &= ~SW_VBUS_VALID;
> +		val &= ~(SW_DRD_STATIC_DEV_CFG0 | SW_DRD_STATIC_HOST_CFG0);
>   		break;

Right, so this should be:

		val &= ~DRD_CONFIG_MASK;

Also ideally this should also have a if (!data->disable_sw_switch)
for consistency.

>   	case USB_ROLE_HOST:
>   		val &= ~SW_IDPIN;
>   		val &= ~SW_VBUS_VALID;
> +		if (!data->disable_sw_switch) {
> +			val &= ~SW_DRD_STATIC_DEV_CFG0;

So this clearing is wrong, it happens to work, but is not
how modifying a "bit-set" / enum should be done, this should be:

			val &= ~DRD_CONFIG_MASK;

> +			val |= SW_DRD_STATIC_HOST_CFG0;
> +		}
>   		break;
>   	case USB_ROLE_DEVICE:
>   		val |= SW_IDPIN;
>   		val |= SW_VBUS_VALID;
> +		if (!data->disable_sw_switch) {
> +			val &= ~SW_DRD_STATIC_HOST_CFG0;
> +			val |= SW_DRD_STATIC_DEV_CFG0;
> +		}

Idem.


>   		break;
>   	}
>   	val |= SW_IDPIN_EN;
> +	if (!data->disable_sw_switch)
> +		val |= SW_SWITCH_EN_CFG0;

So we now have a lot of "if (!data->disable_sw_switch)" checks,

IMHO it would be better / cleaner to do this:

	u32 glk, val, drd_config;

	...

  	val = readl(data->base + DUAL_ROLE_CFG0);
  	switch (role) {
  	case USB_ROLE_NONE:
  		val |= SW_IDPIN;
  		val &= ~SW_VBUS_VALID;
		drd_config = DRD_CONFIG_DYNAMIC;
  		break;
  	case USB_ROLE_HOST:
  		val &= ~SW_IDPIN;
  		val &= ~SW_VBUS_VALID;
		drd_config = DRD_CONFIG_STATIC_HOST;
  		break;
  	case USB_ROLE_DEVICE:
  		val |= SW_IDPIN;
  		val |= SW_VBUS_VALID;
		drd_config = DRD_CONFIG_STATIC_DEVICE;
  		break;
  	}
  	val |= SW_IDPIN_EN;

	if (!data->disable_sw_switch) {
		val &= ~DRD_CONFIG_MASK;
		val |= SW_SWITCH_EN_CFG0 | drd_config;
	}

	...

Regards,

Hans



>   
>   	writel(val, data->base + DUAL_ROLE_CFG0);
>   
> @@ -156,6 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
>   	sw_desc.allow_userspace_control = true,
>   	sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
>   
> +	data->disable_sw_switch = device_property_read_bool(dev,
> +						"sw_switch_disable");
> +
>   	data->role_sw = usb_role_switch_register(dev, &sw_desc);
>   	if (IS_ERR(data->role_sw)) {
>   		fwnode_handle_put(sw_desc.fwnode);
> 

Re: [PATCH 2/2] usb: roles: intel: Enable static DRD mode for role switch

[Heikki Krogerus]

On Tue, Aug 27, 2019 at 03:39:18PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 26-08-19 16:32, Heikki Krogerus wrote:
> > From: Saranya Gopal <saranya.gopal@intel.com>
> > 
> > Enable static DRD mode in Intel platforms which guarantees
> > successful role switch all the time. This fixes issues like
> > software role switch failure after cold boot and issue with
> > role switch when USB 3.0 cable is used. But, do not enable
> > static DRD mode for Cherrytrail devices which rely on firmware
> > for role switch.
> > 
> > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> > Signed-off-by: Balaji Manoharan <m.balaji@intel.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >   .../usb/roles/intel-xhci-usb-role-switch.c    | 26 ++++++++++++++++++-
> >   1 file changed, 25 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 7325a84dd1c8..9101ff94c8d2 100644
> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > @@ -19,6 +19,7 @@
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/pm_runtime.h>
> > +#include <linux/property.h>
> >   #include <linux/usb/role.h>
> >   /* register definition */
> > @@ -26,6 +27,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)
> 
> Nitpick: Please drop the _CFG0 postfix, if anything this
> should be a prefix applied to *all* defines for the bits
> in this register
> 
> > +#define SW_DRD_STATIC_HOST_CFG0		1
> > +#define SW_DRD_STATIC_DEV_CFG0		2
> 
> So bits 0-1 together define the drd-mode. The datasheet
> calls the combination DRD_CONFIG, without a SW_ prefix,
> with the following values being defined:
> 
> 00: Dynamic role-switch
> 01: Static Host mode
> 10: Static device mode
> 11: Reserved
> 
> Notice this is an enum, so the use of bit-ops to clear the
> other state below is wrong. It happens to work, but this is
> not how a multi-bit setting should be modified.
> 
> I suggest using the following defines instead:
> 
> #define DRD_CONFIG_DYNAMIC		0
> #define DRD_CONFIG_STATIC_HOST		1
> #define DRD_CONFIG_STATIC_DEVICE	2
> #define DRD_CONFIG_MASK			3
> 
> >   #define DUAL_ROLE_CFG1			0x6c
> >   #define HOST_MODE			BIT(29)
> > @@ -37,6 +41,7 @@
> >   struct intel_xhci_usb_data {
> >   	struct usb_role_switch *role_sw;
> >   	void __iomem *base;
> > +	bool disable_sw_switch;
> 
> I would prefer for this to be enable_sw_switch and the negation
> when reading the property.
> 
> >   };
> >   static const struct software_node intel_xhci_usb_node = {
> > @@ -63,23 +68,39 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> >   	pm_runtime_get_sync(dev);
> > -	/* Set idpin value as requested */
> > +	/*
> > +	 * Set idpin value as requested.
> > +	 * Since some devices rely on firmware setting DRD_CONFIG and
> > +	 * SW_SWITCH_EN_CFG0 bits to be zero for role switch,
> > +	 * do not set these bits for those devices.
> > +	 */
> >   	val = readl(data->base + DUAL_ROLE_CFG0);
> >   	switch (role) {
> >   	case USB_ROLE_NONE:
> >   		val |= SW_IDPIN;
> >   		val &= ~SW_VBUS_VALID;
> > +		val &= ~(SW_DRD_STATIC_DEV_CFG0 | SW_DRD_STATIC_HOST_CFG0);
> >   		break;
> 
> Right, so this should be:
> 
> 		val &= ~DRD_CONFIG_MASK;
> 
> Also ideally this should also have a if (!data->disable_sw_switch)
> for consistency.
> 
> >   	case USB_ROLE_HOST:
> >   		val &= ~SW_IDPIN;
> >   		val &= ~SW_VBUS_VALID;
> > +		if (!data->disable_sw_switch) {
> > +			val &= ~SW_DRD_STATIC_DEV_CFG0;
> 
> So this clearing is wrong, it happens to work, but is not
> how modifying a "bit-set" / enum should be done, this should be:
> 
> 			val &= ~DRD_CONFIG_MASK;
> 
> > +			val |= SW_DRD_STATIC_HOST_CFG0;
> > +		}
> >   		break;
> >   	case USB_ROLE_DEVICE:
> >   		val |= SW_IDPIN;
> >   		val |= SW_VBUS_VALID;
> > +		if (!data->disable_sw_switch) {
> > +			val &= ~SW_DRD_STATIC_HOST_CFG0;
> > +			val |= SW_DRD_STATIC_DEV_CFG0;
> > +		}
> 
> Idem.
> 
> 
> >   		break;
> >   	}
> >   	val |= SW_IDPIN_EN;
> > +	if (!data->disable_sw_switch)
> > +		val |= SW_SWITCH_EN_CFG0;
> 
> So we now have a lot of "if (!data->disable_sw_switch)" checks,
> 
> IMHO it would be better / cleaner to do this:
> 
> 	u32 glk, val, drd_config;
> 
> 	...
> 
>  	val = readl(data->base + DUAL_ROLE_CFG0);
>  	switch (role) {
>  	case USB_ROLE_NONE:
>  		val |= SW_IDPIN;
>  		val &= ~SW_VBUS_VALID;
> 		drd_config = DRD_CONFIG_DYNAMIC;
>  		break;
>  	case USB_ROLE_HOST:
>  		val &= ~SW_IDPIN;
>  		val &= ~SW_VBUS_VALID;
> 		drd_config = DRD_CONFIG_STATIC_HOST;
>  		break;
>  	case USB_ROLE_DEVICE:
>  		val |= SW_IDPIN;
>  		val |= SW_VBUS_VALID;
> 		drd_config = DRD_CONFIG_STATIC_DEVICE;
>  		break;
>  	}
>  	val |= SW_IDPIN_EN;
> 
> 	if (!data->disable_sw_switch) {
> 		val &= ~DRD_CONFIG_MASK;
> 		val |= SW_SWITCH_EN_CFG0 | drd_config;
> 	}


That looks good to me. Saranya, Balaji! Can you fix that. I don't
think you need me for this anymore.

thanks,

-- 
heikki

RE: [PATCH 2/2] usb: roles: intel: Enable static DRD mode for role switch

[Gopal, Saranya]

Hi Heikki,

Yes, we will fix this and resubmit a newer version of the patch.

Thanks,
Saranya

> -----Original Message-----
> From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> Sent: Wednesday, August 28, 2019 1:12 PM
> To: Hans de Goede <hdegoede@redhat.com>; Gopal, Saranya
> <saranya.gopal@intel.com>; Balaji, M <m.balaji@intel.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Nyman, Mathias
> <mathias.nyman@intel.com>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 2/2] usb: roles: intel: Enable static DRD mode for role
> switch
> 
> On Tue, Aug 27, 2019 at 03:39:18PM +0200, Hans de Goede wrote:
> > Hi,
> >
> > On 26-08-19 16:32, Heikki Krogerus wrote:
> > > From: Saranya Gopal <saranya.gopal@intel.com>
> > >
> > > Enable static DRD mode in Intel platforms which guarantees
> > > successful role switch all the time. This fixes issues like
> > > software role switch failure after cold boot and issue with
> > > role switch when USB 3.0 cable is used. But, do not enable
> > > static DRD mode for Cherrytrail devices which rely on firmware
> > > for role switch.
> > >
> > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> > > Signed-off-by: Balaji Manoharan <m.balaji@intel.com>
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >   .../usb/roles/intel-xhci-usb-role-switch.c    | 26 ++++++++++++++++++-
> > >   1 file changed, 25 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 7325a84dd1c8..9101ff94c8d2 100644
> > > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > > @@ -19,6 +19,7 @@
> > >   #include <linux/module.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/pm_runtime.h>
> > > +#include <linux/property.h>
> > >   #include <linux/usb/role.h>
> > >   /* register definition */
> > > @@ -26,6 +27,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)
> >
> > Nitpick: Please drop the _CFG0 postfix, if anything this
> > should be a prefix applied to *all* defines for the bits
> > in this register
> >
> > > +#define SW_DRD_STATIC_HOST_CFG0		1
> > > +#define SW_DRD_STATIC_DEV_CFG0		2
> >
> > So bits 0-1 together define the drd-mode. The datasheet
> > calls the combination DRD_CONFIG, without a SW_ prefix,
> > with the following values being defined:
> >
> > 00: Dynamic role-switch
> > 01: Static Host mode
> > 10: Static device mode
> > 11: Reserved
> >
> > Notice this is an enum, so the use of bit-ops to clear the
> > other state below is wrong. It happens to work, but this is
> > not how a multi-bit setting should be modified.
> >
> > I suggest using the following defines instead:
> >
> > #define DRD_CONFIG_DYNAMIC		0
> > #define DRD_CONFIG_STATIC_HOST		1
> > #define DRD_CONFIG_STATIC_DEVICE	2
> > #define DRD_CONFIG_MASK			3
> >
> > >   #define DUAL_ROLE_CFG1			0x6c
> > >   #define HOST_MODE			BIT(29)
> > > @@ -37,6 +41,7 @@
> > >   struct intel_xhci_usb_data {
> > >   	struct usb_role_switch *role_sw;
> > >   	void __iomem *base;
> > > +	bool disable_sw_switch;
> >
> > I would prefer for this to be enable_sw_switch and the negation
> > when reading the property.
> >
> > >   };
> > >   static const struct software_node intel_xhci_usb_node = {
> > > @@ -63,23 +68,39 @@ static int intel_xhci_usb_set_role(struct device *dev,
> enum usb_role role)
> > >   	pm_runtime_get_sync(dev);
> > > -	/* Set idpin value as requested */
> > > +	/*
> > > +	 * Set idpin value as requested.
> > > +	 * Since some devices rely on firmware setting DRD_CONFIG and
> > > +	 * SW_SWITCH_EN_CFG0 bits to be zero for role switch,
> > > +	 * do not set these bits for those devices.
> > > +	 */
> > >   	val = readl(data->base + DUAL_ROLE_CFG0);
> > >   	switch (role) {
> > >   	case USB_ROLE_NONE:
> > >   		val |= SW_IDPIN;
> > >   		val &= ~SW_VBUS_VALID;
> > > +		val &= ~(SW_DRD_STATIC_DEV_CFG0 |
> SW_DRD_STATIC_HOST_CFG0);
> > >   		break;
> >
> > Right, so this should be:
> >
> > 		val &= ~DRD_CONFIG_MASK;
> >
> > Also ideally this should also have a if (!data->disable_sw_switch)
> > for consistency.
> >
> > >   	case USB_ROLE_HOST:
> > >   		val &= ~SW_IDPIN;
> > >   		val &= ~SW_VBUS_VALID;
> > > +		if (!data->disable_sw_switch) {
> > > +			val &= ~SW_DRD_STATIC_DEV_CFG0;
> >
> > So this clearing is wrong, it happens to work, but is not
> > how modifying a "bit-set" / enum should be done, this should be:
> >
> > 			val &= ~DRD_CONFIG_MASK;
> >
> > > +			val |= SW_DRD_STATIC_HOST_CFG0;
> > > +		}
> > >   		break;
> > >   	case USB_ROLE_DEVICE:
> > >   		val |= SW_IDPIN;
> > >   		val |= SW_VBUS_VALID;
> > > +		if (!data->disable_sw_switch) {
> > > +			val &= ~SW_DRD_STATIC_HOST_CFG0;
> > > +			val |= SW_DRD_STATIC_DEV_CFG0;
> > > +		}
> >
> > Idem.
> >
> >
> > >   		break;
> > >   	}
> > >   	val |= SW_IDPIN_EN;
> > > +	if (!data->disable_sw_switch)
> > > +		val |= SW_SWITCH_EN_CFG0;
> >
> > So we now have a lot of "if (!data->disable_sw_switch)" checks,
> >
> > IMHO it would be better / cleaner to do this:
> >
> > 	u32 glk, val, drd_config;
> >
> > 	...
> >
> >  	val = readl(data->base + DUAL_ROLE_CFG0);
> >  	switch (role) {
> >  	case USB_ROLE_NONE:
> >  		val |= SW_IDPIN;
> >  		val &= ~SW_VBUS_VALID;
> > 		drd_config = DRD_CONFIG_DYNAMIC;
> >  		break;
> >  	case USB_ROLE_HOST:
> >  		val &= ~SW_IDPIN;
> >  		val &= ~SW_VBUS_VALID;
> > 		drd_config = DRD_CONFIG_STATIC_HOST;
> >  		break;
> >  	case USB_ROLE_DEVICE:
> >  		val |= SW_IDPIN;
> >  		val |= SW_VBUS_VALID;
> > 		drd_config = DRD_CONFIG_STATIC_DEVICE;
> >  		break;
> >  	}
> >  	val |= SW_IDPIN_EN;
> >
> > 	if (!data->disable_sw_switch) {
> > 		val &= ~DRD_CONFIG_MASK;
> > 		val |= SW_SWITCH_EN_CFG0 | drd_config;
> > 	}
> 
> 
> That looks good to me. Saranya, Balaji! Can you fix that. I don't
> think you need me for this anymore.
> 
> thanks,
> 
> --
> heikki