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

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

[Saranya Gopal]

In platforms like 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>
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 changes from v2: Added suggested-by tag
 changes from v1: none

 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 399113f..f498160 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);
-- 
1.9.1

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

[Saranya Gopal]

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>
---
 changes from v2: Revised SoB tags
 changes from v1: Added DRD_CONFIG_MASK and drd_config to avoid multiple if checks
                  Also, other minor changes suggested by Hans

 drivers/usb/roles/intel-xhci-usb-role-switch.c | 28 +++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 277de96..a903bef 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,12 @@
 #define SW_VBUS_VALID			BIT(24)
 #define SW_IDPIN_EN			BIT(21)
 #define SW_IDPIN			BIT(20)
+#define SW_SWITCH_EN			BIT(16)
+
+#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 +44,7 @@
 struct intel_xhci_usb_data {
 	struct usb_role_switch *role_sw;
 	void __iomem *base;
+	bool enable_sw_switch;
 };
 
 static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
@@ -44,7 +52,7 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
 	unsigned long timeout;
 	acpi_status status;
-	u32 glk, val;
+	u32 glk, val, drd_config;
 
 	/*
 	 * On many CHT devices ACPI event (_AEI) handlers read / modify /
@@ -59,24 +67,35 @@ 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 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;
+		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->enable_sw_switch) {
+		val &= ~DRD_CONFIG_MASK;
+		val |= SW_SWITCH_EN | drd_config;
+	}
 	writel(val, data->base + DUAL_ROLE_CFG0);
 
 	acpi_release_global_lock(glk);
@@ -147,6 +166,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
+	data->enable_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))
 		return PTR_ERR(data->role_sw);
-- 
1.9.1

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3864 bytes --]

Hi Saranya,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc6 next-20190827]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Saranya-Gopal/xhci-ext-caps-c-Add-property-to-disable-Intel-SW-switch/20190829-031957
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/usb/roles/intel-xhci-usb-role-switch.c: In function 'intel_xhci_usb_set_role':
>> drivers/usb/roles/intel-xhci-usb-role-switch.c:55:16: warning: 'drd_config' may be used uninitialized in this function [-Wmaybe-uninitialized]
     u32 glk, val, drd_config;
                   ^~~~~~~~~~

vim +/drd_config +55 drivers/usb/roles/intel-xhci-usb-role-switch.c

    49	
    50	static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
    51	{
    52		struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
    53		unsigned long timeout;
    54		acpi_status status;
  > 55		u32 glk, val, drd_config;
    56	
    57		/*
    58		 * On many CHT devices ACPI event (_AEI) handlers read / modify /
    59		 * write the cfg0 register, just like we do. Take the ACPI lock
    60		 * to avoid us racing with the AML code.
    61		 */
    62		status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
    63		if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
    64			dev_err(dev, "Error could not acquire lock\n");
    65			return -EIO;
    66		}
    67	
    68		pm_runtime_get_sync(dev);
    69	
    70		/*
    71		 * Set idpin value as requested.
    72		 * Since some devices rely on firmware setting DRD_CONFIG and
    73		 * SW_SWITCH_EN bits to be zero for role switch,
    74		 * do not set these bits for those devices.
    75		 */
    76		val = readl(data->base + DUAL_ROLE_CFG0);
    77		switch (role) {
    78		case USB_ROLE_NONE:
    79			val |= SW_IDPIN;
    80			val &= ~SW_VBUS_VALID;
    81			drd_config = DRD_CONFIG_DYNAMIC;
    82			break;
    83		case USB_ROLE_HOST:
    84			val &= ~SW_IDPIN;
    85			val &= ~SW_VBUS_VALID;
    86			drd_config = DRD_CONFIG_STATIC_HOST;
    87			break;
    88		case USB_ROLE_DEVICE:
    89			val |= SW_IDPIN;
    90			val |= SW_VBUS_VALID;
    91			drd_config = DRD_CONFIG_STATIC_DEVICE;
    92			break;
    93		}
    94		val |= SW_IDPIN_EN;
    95		if (data->enable_sw_switch) {
    96			val &= ~DRD_CONFIG_MASK;
    97			val |= SW_SWITCH_EN | drd_config;
    98		}
    99		writel(val, data->base + DUAL_ROLE_CFG0);
   100	
   101		acpi_release_global_lock(glk);
   102	
   103		/* In most case it takes about 600ms to finish mode switching */
   104		timeout = jiffies + msecs_to_jiffies(DUAL_ROLE_CFG1_POLL_TIMEOUT);
   105	
   106		/* Polling on CFG1 register to confirm mode switch.*/
   107		do {
   108			val = readl(data->base + DUAL_ROLE_CFG1);
   109			if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
   110				pm_runtime_put(dev);
   111				return 0;
   112			}
   113	
   114			/* Interval for polling is set to about 5 - 10 ms */
   115			usleep_range(5000, 10000);
   116		} while (time_before(jiffies, timeout));
   117	
   118		pm_runtime_put(dev);
   119	
   120		dev_warn(dev, "Timeout waiting for role-switch\n");
   121		return -ETIMEDOUT;
   122	}
   123	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69510 bytes --]

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

[Heikki Krogerus]

Hi,

>  static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> @@ -44,7 +52,7 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
>  	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
>  	unsigned long timeout;
>  	acpi_status status;
> -	u32 glk, val;
> +	u32 glk, val, drd_config;

I guess you need to fix that. While at it, please introduce it on its
own line:

        u32 drd_config = DRD_CONFIG_DYNAMIC;


thanks,

-- 
heikki