[PATCH v3 0/4] usb: typec: fixes for Cherry Trails

[PATCH v3 0/4] usb: typec: fixes for Cherry Trails

[Heikki Krogerus]

Hi,

This is third version of patches that fix various problems I
encountered while testing my USB Type-C Alternate Mode
patches with GPD Win board (Intel Cherry Trail based).

I added one more patch to this series. Sathyanarayanan
reported an issue where the mux does not get configured when
xHCI is suspended. To fix that we need to enable runtime PM
in the mux driver.

Other changes since v2: constifying struct
usb_role_switch_desc sw_desc, and improvements to the commit
messages.

Link to the original patches:
https://lkml.org/lkml/2018/4/30/350


Heikki Krogerus (4):
  usb: roles: intel_xhci: Always allow user control
  platform: x86: intel_cht_int33fe: Fix dependencies
  usb: typec: fusb302: Fix debugfs issue
  usb: roles: intel_xhci: Enable runtime PM

 drivers/i2c/busses/Kconfig                    |  3 +-
 drivers/platform/x86/Kconfig                  |  4 +-
 .../usb/roles/intel-xhci-usb-role-switch.c    | 37 ++++++++-----------
 drivers/usb/typec/fusb302/fusb302.c           |  1 +
 4 files changed, 19 insertions(+), 26 deletions(-)

-- 
2.17.0

[PATCH v3 1/4] usb: roles: intel_xhci: Always allow user control

[Heikki Krogerus]

Trying to determine the USB port type with this mux is very
difficult. To simplify the situation, always allow user
control, even if the port is USB Type-C port.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../usb/roles/intel-xhci-usb-role-switch.c    | 23 ++-----------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 28102127b9d5..6e922b50b674 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -38,20 +38,6 @@ struct intel_xhci_usb_data {
 	void __iomem *base;
 };
 
-struct intel_xhci_acpi_match {
-	const char *hid;
-	int hrv;
-};
-
-/*
- * ACPI IDs for PMICs which do not support separate data and power role
- * detection (USB ACA detection for micro USB OTG), we allow userspace to
- * change the role manually on these.
- */
-static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
-	{ "INT33F4",  3 }, /* X-Powers AXP288 PMIC */
-};
-
 static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 {
 	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
@@ -127,9 +113,10 @@ static enum usb_role intel_xhci_usb_get_role(struct device *dev)
 	return role;
 }
 
-static struct usb_role_switch_desc sw_desc = {
+static const struct usb_role_switch_desc sw_desc = {
 	.set = intel_xhci_usb_set_role,
 	.get = intel_xhci_usb_get_role,
+	.allow_userspace_control = true,
 };
 
 static int intel_xhci_usb_probe(struct platform_device *pdev)
@@ -137,7 +124,6 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct intel_xhci_usb_data *data;
 	struct resource *res;
-	int i;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -150,11 +136,6 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	if (!data->base)
 		return -ENOMEM;
 
-	for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++)
-		if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1",
-				     allow_userspace_ctrl_ids[i].hrv))
-			sw_desc.allow_userspace_control = true;
-
 	platform_set_drvdata(pdev, data);
 
 	data->role_sw = usb_role_switch_register(dev, &sw_desc);
-- 
2.17.0

[PATCH v3 2/4] platform: x86: intel_cht_int33fe: Fix dependencies

[Heikki Krogerus]

The driver will not probe unless bq24190 is loaded, so
making it a dependency.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/Kconfig   | 3 +--
 drivers/platform/x86/Kconfig | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 99edffae27f6..4f8df2ec87b1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -202,8 +202,7 @@ config I2C_CHT_WC
 
 	  Note this controller is hooked up to a TI bq24292i charger-IC,
 	  combined with a FUSB302 Type-C port-controller as such it is advised
-	  to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
-	  (the fusb302 driver currently is in drivers/staging).
+	  to also select CONFIG_TYPEC_FUSB302=m.
 
 config I2C_NFORCE2
 	tristate "Nvidia nForce2, nForce3 and nForce4"
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 566644bb496a..f27cb186437d 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -866,6 +866,7 @@ config ACPI_CMPC
 config INTEL_CHT_INT33FE
 	tristate "Intel Cherry Trail ACPI INT33FE Driver"
 	depends on X86 && ACPI && I2C && REGULATOR
+	depends on CHARGER_BQ24190=y || (CHARGER_BQ24190=m && m)
 	---help---
 	  This driver add support for the INT33FE ACPI device found on
 	  some Intel Cherry Trail devices.
@@ -877,8 +878,7 @@ config INTEL_CHT_INT33FE
 	  i2c drivers for these chips can bind to the them.
 
 	  If you enable this driver it is advised to also select
-	  CONFIG_TYPEC_FUSB302=m, CONFIG_CHARGER_BQ24190=m and
-	  CONFIG_BATTERY_MAX17042=m.
+	  CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m.
 
 config INTEL_INT0002_VGPIO
 	tristate "Intel ACPI INT0002 Virtual GPIO driver"
-- 
2.17.0

[PATCH v3 3/4] usb: typec: fusb302: Fix debugfs issue

[Heikki Krogerus]

Removing the "fusb302" debugfs directory when unloading
the driver. That allows the driver to be loaded more then
one time. The directory will not get actually removed until
it is empty, so only after the last instance has been
removed.

This fixes an issue where the driver can't be re-loaded if
it has been unloaded as the "fusb302" debugfs directory
already exists.

Fixes: 76f0c53d08b9 ("usb: typec: fusb302: Move out of staging")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/fusb302/fusb302.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
index eba6bb890b17..9c1eba9ea004 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -234,6 +234,7 @@ static int fusb302_debugfs_init(struct fusb302_chip *chip)
 static void fusb302_debugfs_exit(struct fusb302_chip *chip)
 {
 	debugfs_remove(chip->dentry);
+	debugfs_remove(rootdir);
 }
 
 #else
-- 
2.17.0

Re: [PATCH v3 3/4] usb: typec: fusb302: Fix debugfs issue

[Guenter Roeck]

On 05/24/2018 01:18 AM, Heikki Krogerus wrote:
> Removing the "fusb302" debugfs directory when unloading
> the driver. That allows the driver to be loaded more then
> one time. The directory will not get actually removed until
> it is empty, so only after the last instance has been
> removed.
> 
> This fixes an issue where the driver can't be re-loaded if
> it has been unloaded as the "fusb302" debugfs directory
> already exists.
> 
> Fixes: 76f0c53d08b9 ("usb: typec: fusb302: Move out of staging")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/fusb302/fusb302.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index eba6bb890b17..9c1eba9ea004 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -234,6 +234,7 @@ static int fusb302_debugfs_init(struct fusb302_chip *chip)
>   static void fusb302_debugfs_exit(struct fusb302_chip *chip)
>   {
>   	debugfs_remove(chip->dentry);
> +	debugfs_remove(rootdir);
>   }
>   
>   #else
> 

[PATCH v3 4/4] usb: roles: intel_xhci: Enable runtime PM

[Heikki Krogerus]

This fixes an issue where the mux does not get configured
when the parent device is suspended. The registers for this
mux are mapped to the parent device MMIO (usually xHCI PCI
device), so in order for the driver to be able to program
the registers, the parent device must be resumed.

Reported-by: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@intel.com>
Fixes: f6fb9ec02be1 ("usb: roles: Add Intel xHCI USB role switch driver")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/roles/intel-xhci-usb-role-switch.c | 14 +++++++++++++-
 1 file changed, 13 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 6e922b50b674..1fb3dd0f1dfa 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/usb/role.h>
 
 /* register definition */
@@ -56,6 +57,8 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 		return -EIO;
 	}
 
+	pm_runtime_get_sync(dev);
+
 	/* Set idpin value as requested */
 	val = readl(data->base + DUAL_ROLE_CFG0);
 	switch (role) {
@@ -84,13 +87,17 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 	/* Polling on CFG1 register to confirm mode switch.*/
 	do {
 		val = readl(data->base + DUAL_ROLE_CFG1);
-		if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST))
+		if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
+			pm_runtime_put(dev);
 			return 0;
+		}
 
 		/* Interval for polling is set to about 5 - 10 ms */
 		usleep_range(5000, 10000);
 	} while (time_before(jiffies, timeout));
 
+	pm_runtime_put(dev);
+
 	dev_warn(dev, "Timeout waiting for role-switch\n");
 	return -ETIMEDOUT;
 }
@@ -101,7 +108,9 @@ static enum usb_role intel_xhci_usb_get_role(struct device *dev)
 	enum usb_role role;
 	u32 val;
 
+	pm_runtime_get_sync(dev);
 	val = readl(data->base + DUAL_ROLE_CFG0);
+	pm_runtime_put(dev);
 
 	if (!(val & SW_IDPIN))
 		role = USB_ROLE_HOST;
@@ -142,6 +151,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	if (IS_ERR(data->role_sw))
 		return PTR_ERR(data->role_sw);
 
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 }
 
-- 
2.17.0

Re: [PATCH v3 4/4] usb: roles: intel_xhci: Enable runtime PM

[sathyanarayanan kuppuswamy]

Hi,


On 05/24/2018 01:18 AM, Heikki Krogerus wrote:
> This fixes an issue where the mux does not get configured
> when the parent device is suspended. The registers for this
> mux are mapped to the parent device MMIO (usually xHCI PCI
> device), so in order for the driver to be able to program
> the registers, the parent device must be resumed.
>
> Reported-by: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@intel.com>
> Fixes: f6fb9ec02be1 ("usb: roles: Add Intel xHCI USB role switch driver")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>   drivers/usb/roles/intel-xhci-usb-role-switch.c | 14 +++++++++++++-
>   1 file changed, 13 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 6e922b50b674..1fb3dd0f1dfa 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -18,6 +18,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/usb/role.h>
>   
>   /* register definition */
> @@ -56,6 +57,8 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
>   		return -EIO;
>   	}
>   
> +	pm_runtime_get_sync(dev);
> +
>   	/* Set idpin value as requested */
>   	val = readl(data->base + DUAL_ROLE_CFG0);
>   	switch (role) {
> @@ -84,13 +87,17 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
>   	/* Polling on CFG1 register to confirm mode switch.*/
>   	do {
>   		val = readl(data->base + DUAL_ROLE_CFG1);
> -		if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST))
> +		if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) {
> +			pm_runtime_put(dev);
>   			return 0;
> +		}
>   
>   		/* Interval for polling is set to about 5 - 10 ms */
>   		usleep_range(5000, 10000);
>   	} while (time_before(jiffies, timeout));
>   
> +	pm_runtime_put(dev);
> +
>   	dev_warn(dev, "Timeout waiting for role-switch\n");
>   	return -ETIMEDOUT;
>   }
> @@ -101,7 +108,9 @@ static enum usb_role intel_xhci_usb_get_role(struct device *dev)
>   	enum usb_role role;
>   	u32 val;
>   
> +	pm_runtime_get_sync(dev);
>   	val = readl(data->base + DUAL_ROLE_CFG0);
> +	pm_runtime_put(dev);
>   
>   	if (!(val & SW_IDPIN))
>   		role = USB_ROLE_HOST;
> @@ -142,6 +151,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
>   	if (IS_ERR(data->role_sw))
>   		return PTR_ERR(data->role_sw);
>   
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>   	return 0;
>   }
>   

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer