[PATCH 0/2] extcon: driver for Intel USB MUX

[PATCH 0/2] extcon: driver for Intel USB MUX

[Heikki Krogerus]

Hi,

This is a driver for an internal mux which is available on most modern
Intel platforms that shares an USB port between USB Device Controller
and xHCI. Normally BIOS or ACPI take care of it, but on some platforms
that is not possible, and the OS has to control it.

When the mux needs to be handled by OS, there is always an external
component that detects connection changes in the port behind the mux,
for example PMIC. The driver for that component needs to notify this
driver. The mux itself has no means to detect connection changes on
the port. User selectable kconfig option is deliberately left out. The
driver for the mux needs to be always selected by the drivers for the
component that can notify the mux driver about connection changes.

The only platforms the need the OS to be in control of the mux so far
are Cherrytrail based, and on Cherrytrail SOC the mux control
registers are mapped to xHCI MMIO. The second patch will register an
instance of the mux from pci_quirks.c if the mux is detected and if
the driver has been loaded.

The mux control registers are defined in Cherrytrail datasheets [1]
page 2230-2234.

I think these should go via extcon tree, but it's up to you guys of
course.


[1] http://www.intel.es/content/www/es/es/processors/atom/atom-z8000-datasheet-vol-2.html


Heikki Krogerus (2):
  extcon: add driver for Intel USB mux
  usb: pci-quirks: register USB mux found on Cherrytrail SOC

 drivers/extcon/Kconfig               |   5 ++
 drivers/extcon/Makefile              |   1 +
 drivers/extcon/extcon-intel-usb.c    | 112 +++++++++++++++++++++++++++++++++++
 drivers/usb/host/pci-quirks.c        |  30 +++++++++-
 include/linux/extcon/intel_usb_mux.h |  31 ++++++++++
 5 files changed, 178 insertions(+), 1 deletion(-)
 create mode 100644 drivers/extcon/extcon-intel-usb.c
 create mode 100644 include/linux/extcon/intel_usb_mux.h

-- 
2.6.2

[PATCH 1/2] extcon: add driver for Intel USB mux

[Heikki Krogerus]

Several Intel PCHs and SOCs have an internal mux that is
used to share one USB port between USB Device Controller and
xHCI. The mux is normally handled by System FW/BIOS, but not
always. For those platforms where the FW does not take care
of the mux, this driver is needed.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/extcon/Kconfig               |   5 ++
 drivers/extcon/Makefile              |   1 +
 drivers/extcon/extcon-intel-usb.c    | 112 +++++++++++++++++++++++++++++++++++
 include/linux/extcon/intel_usb_mux.h |  31 ++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 drivers/extcon/extcon-intel-usb.c
 create mode 100644 include/linux/extcon/intel_usb_mux.h

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 0cebbf6..0a7ccb1 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
 	  Used typically if GPIO is used for USB ID pin detection.
 
 endif
+
+config EXTCON_INTEL_USB
+	bool
+	depends on X86 && USB
+	select EXTCON
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index ba787d0..e6e031a 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
 obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
+obj-$(CONFIG_EXTCON_INTEL_USB)	+= extcon-intel-usb.o
diff --git a/drivers/extcon/extcon-intel-usb.c b/drivers/extcon/extcon-intel-usb.c
new file mode 100644
index 0000000..5534781
--- /dev/null
+++ b/drivers/extcon/extcon-intel-usb.c
@@ -0,0 +1,112 @@
+/**
+ * extcon-intel-usb.c - Driver for Intel USB mux
+ *
+ * Copyright (C) 2015 Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/slab.h>
+#include <linux/extcon.h>
+
+#include <linux/extcon/intel_usb_mux.h>
+
+#define INTEL_MUX_CFG0		0x00
+#define INTEL_MUX_CFG1		0x04
+
+#define CFG0_SW_DRD_MODE_MASK	0x3
+#define CFG0_SW_DRD_DYN		0
+#define CFG0_SW_DRD_STATIC_HOST	1
+#define CFG0_SW_DRD_STATIC_DEV	2
+#define CFG0_SW_SYNC_SS_AND_HS	BIT(2)
+#define CFG0_SW_SWITCH_EN	BIT(16)
+#define CFG0_SW_IDPIN		BIT(20)
+#define CFG0_SW_IDPIN_EN	BIT(21)
+#define CFG0_SW_VBUS_VALID	BIT(24)
+
+#define CFG1_MODE		BIT(29)
+
+struct intel_usb_mux {
+	struct notifier_block nb;
+	struct extcon_dev edev;
+	void __iomem *regs;
+	u32 cfg0_ctx;
+};
+
+static const int intel_mux_cable[] = {
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static int intel_usb_mux_notifier(struct notifier_block *nb,
+				  unsigned long old, void *ptr)
+{
+	struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
+	u32 val;
+
+	if (mux->edev.state)
+		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
+	else
+		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
+		      CFG0_SW_DRD_STATIC_DEV;
+
+	writel(val, mux->regs);
+	return NOTIFY_OK;
+}
+
+struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
+					     struct resource *r)
+{
+	struct intel_usb_mux *mux;
+	int ret;
+
+	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	mux->regs = ioremap_nocache(r->start, resource_size(r));
+	if (!mux->regs) {
+		kfree(mux);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
+
+	mux->edev.dev.parent = dev;
+	mux->edev.supported_cable = intel_mux_cable;
+
+	ret = extcon_dev_register(&mux->edev);
+	if (ret)
+		goto err;
+
+	mux->edev.name = "intel_usb_mux";
+	mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
+
+	/* An external source needs to tell us what to do */
+	mux->nb.notifier_call = intel_usb_mux_notifier;
+	ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
+	if (ret) {
+		dev_err(&mux->edev.dev, "failed to register notifier\n");
+		extcon_dev_unregister(&mux->edev);
+		goto err;
+	}
+	return mux;
+err:
+	iounmap(mux->regs);
+	kfree(mux);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_register);
+
+void intel_usb_mux_unregister(struct intel_usb_mux *mux)
+{
+	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
+	extcon_dev_unregister(&mux->edev);
+	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
+	iounmap(mux->regs);
+	kfree(mux);
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
diff --git a/include/linux/extcon/intel_usb_mux.h b/include/linux/extcon/intel_usb_mux.h
new file mode 100644
index 0000000..f18ce52
--- /dev/null
+++ b/include/linux/extcon/intel_usb_mux.h
@@ -0,0 +1,31 @@
+/*
+ * Driver for Intel USB mux
+ *
+ * Copyright (C) 2015 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _INTEL_USB_MUX_H
+#define _INTEL_USB_MUX_H
+
+#include <linux/errno.h>
+
+struct intel_usb_mux;
+
+#ifdef CONFIG_EXTCON_INTEL_USB
+struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
+					     struct resource *r);
+void intel_usb_mux_unregister(struct intel_usb_mux *mux);
+#else
+static inline struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
+							   struct resource *r)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+static inline void intel_usb_mux_unregister(struct intel_usb_mux *mux) { }
+#endif /* CONFIG_EXTCON_INTEL_USB */
+
+#endif /* _INTEL_USB_MUX_H */
-- 
2.6.2

[PATCH 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Heikki Krogerus]

Intel Braswell/Cherrytrail has an internal mux that shares
one USB port between USB Device Controller and xHCI. The
same mux is found on several SOCs from Intel, but only on
a few Cherrytrail based platforms the OS is expected to
configure it. Normally BIOS takes care of it.

The driver for the mux is an "extcon" driver. With this we
only register the mux if it's detected.

Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/host/pci-quirks.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index f940056..4e3016a 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/extcon/intel_usb_mux.h>
 #include "pci-quirks.h"
 #include "xhci-ext-caps.h"
 
@@ -1029,9 +1030,36 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	writel(val, base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
 
 hc_init:
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
 		usb_enable_intel_xhci_ports(pdev);
 
+		/*
+		 * Initialize the internal mux that shares a port between USB
+		 * Device Controller and xHCI on platforms that have it.
+		 */
+#define XHCI_INTEL_VENDOR_CAPS 192
+#define XHCI_INTEL_USB_MUX_OFFSET 0x80d8
+		ext_cap_offset = xhci_find_next_cap_offset(base,
+							XHCI_HCC_PARAMS_OFFSET);
+		ext_cap_offset = xhci_find_ext_cap_by_id(base, ext_cap_offset,
+							XHCI_INTEL_VENDOR_CAPS);
+		if (ext_cap_offset) {
+			struct intel_usb_mux *mux;
+			struct resource r;
+
+			r.start = pci_resource_start(pdev, 0) +
+					XHCI_INTEL_USB_MUX_OFFSET;
+			r.end   = r.start + 8;
+			r.flags = IORESOURCE_MEM;
+
+			mux = intel_usb_mux_register(&pdev->dev, &r);
+			if (IS_ERR(mux) && PTR_ERR(mux) == -ENOTSUPP)
+				dev_dbg(&pdev->dev, "USB mux not supported\n");
+			else if (IS_ERR(mux))
+				dev_err(&pdev->dev, "failed to register mux\n");
+		}
+	}
+
 	op_reg_base = base + XHCI_HC_LENGTH(readl(base));
 
 	/* Wait for the host controller to be ready before writing any
-- 
2.6.2

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

Follow my comments below.

On Tue, Dec 01, 2015 at 03:32:37PM +0200, Heikki Krogerus wrote:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/extcon/Kconfig               |   5 ++
>  drivers/extcon/Makefile              |   1 +
>  drivers/extcon/extcon-intel-usb.c    | 112 +++++++++++++++++++++++++++++++++++
>  include/linux/extcon/intel_usb_mux.h |  31 ++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/extcon/extcon-intel-usb.c
>  create mode 100644 include/linux/extcon/intel_usb_mux.h
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 0cebbf6..0a7ccb1 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
>  	  Used typically if GPIO is used for USB ID pin detection.
>  
>  endif
> +
> +config EXTCON_INTEL_USB
> +	bool
> +	depends on X86 && USB
> +	select EXTCON
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index ba787d0..e6e031a 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
> +obj-$(CONFIG_EXTCON_INTEL_USB)	+= extcon-intel-usb.o
> diff --git a/drivers/extcon/extcon-intel-usb.c b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 0000000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/extcon.h>
> +
> +#include <linux/extcon/intel_usb_mux.h>
> +
> +#define INTEL_MUX_CFG0		0x00
> +#define INTEL_MUX_CFG1		0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK	0x3
> +#define CFG0_SW_DRD_DYN		0
> +#define CFG0_SW_DRD_STATIC_HOST	1
> +#define CFG0_SW_DRD_STATIC_DEV	2
> +#define CFG0_SW_SYNC_SS_AND_HS	BIT(2)
> +#define CFG0_SW_SWITCH_EN	BIT(16)
> +#define CFG0_SW_IDPIN		BIT(20)
> +#define CFG0_SW_IDPIN_EN	BIT(21)
> +#define CFG0_SW_VBUS_VALID	BIT(24)
> +
> +#define CFG1_MODE		BIT(29)
> +
> +struct intel_usb_mux {
> +	struct notifier_block nb;
> +	struct extcon_dev edev;
> +	void __iomem *regs;
> +	u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> +				  unsigned long old, void *ptr)
> +{
> +	struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> +	u32 val;
> +
> +	if (mux->edev.state)
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> +	else
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> +		      CFG0_SW_DRD_STATIC_DEV;
> +
> +	writel(val, mux->regs);
> +	return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +					     struct resource *r)
> +{
> +	struct intel_usb_mux *mux;
> +	int ret;
> +
> +	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux->regs = ioremap_nocache(r->start, resource_size(r));
> +	if (!mux->regs) {
> +		kfree(mux);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> +	mux->edev.dev.parent = dev;
> +	mux->edev.supported_cable = intel_mux_cable;
> +
> +	ret = extcon_dev_register(&mux->edev);
> +	if (ret)
> +		goto err;
> +
> +	mux->edev.name = "intel_usb_mux";
> +	mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> +	/* An external source needs to tell us what to do */
> +	mux->nb.notifier_call = intel_usb_mux_notifier;
> +	ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> +	if (ret) {
> +		dev_err(&mux->edev.dev, "failed to register notifier\n");
> +		extcon_dev_unregister(&mux->edev);
> +		goto err;
> +	}
> +	return mux;
> +err:
> +	iounmap(mux->regs);
> +	kfree(mux);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> +{

There are still 2 pending comments for this unregister function:

1) How about a protection against unbalanced unregistering? In case an
user mistakenly unregisters twice or unregisters without a previous
registering.

2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
set mux to automatic mode again. Or maybe you could save CFG0's initial
value and set it again here. Anyway, when unregistering the mux driver
you need to make sure the mux goes back to its original configuration.

Br, David

> +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> +	extcon_dev_unregister(&mux->edev);
> +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> +	iounmap(mux->regs);
> +	kfree(mux);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> diff --git a/include/linux/extcon/intel_usb_mux.h b/include/linux/extcon/intel_usb_mux.h
> new file mode 100644
> index 0000000..f18ce52
> --- /dev/null
> +++ b/include/linux/extcon/intel_usb_mux.h
> @@ -0,0 +1,31 @@
> +/*
> + * Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _INTEL_USB_MUX_H
> +#define _INTEL_USB_MUX_H
> +
> +#include <linux/errno.h>
> +
> +struct intel_usb_mux;
> +
> +#ifdef CONFIG_EXTCON_INTEL_USB
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +					     struct resource *r);
> +void intel_usb_mux_unregister(struct intel_usb_mux *mux);
> +#else
> +static inline struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +							   struct resource *r)
> +{
> +	return ERR_PTR(-ENOTSUPP);
> +}
> +static inline void intel_usb_mux_unregister(struct intel_usb_mux *mux) { }
> +#endif /* CONFIG_EXTCON_INTEL_USB */
> +
> +#endif /* _INTEL_USB_MUX_H */
> -- 
> 2.6.2
> 

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[Felipe Balbi]

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


Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.

except that this is not exactly a driver. Note that it lacks a
module_init() of its own.

> diff --git a/drivers/extcon/extcon-intel-usb.c b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 0000000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/extcon.h>
> +
> +#include <linux/extcon/intel_usb_mux.h>
> +
> +#define INTEL_MUX_CFG0		0x00
> +#define INTEL_MUX_CFG1		0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK	0x3
> +#define CFG0_SW_DRD_DYN		0
> +#define CFG0_SW_DRD_STATIC_HOST	1
> +#define CFG0_SW_DRD_STATIC_DEV	2
> +#define CFG0_SW_SYNC_SS_AND_HS	BIT(2)
> +#define CFG0_SW_SWITCH_EN	BIT(16)
> +#define CFG0_SW_IDPIN		BIT(20)
> +#define CFG0_SW_IDPIN_EN	BIT(21)
> +#define CFG0_SW_VBUS_VALID	BIT(24)
> +
> +#define CFG1_MODE		BIT(29)
> +
> +struct intel_usb_mux {
> +	struct notifier_block nb;
> +	struct extcon_dev edev;
> +	void __iomem *regs;
> +	u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> +				  unsigned long old, void *ptr)
> +{
> +	struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> +	u32 val;
> +
> +	if (mux->edev.state)
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> +	else
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> +		      CFG0_SW_DRD_STATIC_DEV;
> +
> +	writel(val, mux->regs);
> +	return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +					     struct resource *r)
> +{
> +	struct intel_usb_mux *mux;
> +	int ret;
> +
> +	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux->regs = ioremap_nocache(r->start, resource_size(r));
> +	if (!mux->regs) {
> +		kfree(mux);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> +	mux->edev.dev.parent = dev;
> +	mux->edev.supported_cable = intel_mux_cable;
> +
> +	ret = extcon_dev_register(&mux->edev);
> +	if (ret)
> +		goto err;
> +
> +	mux->edev.name = "intel_usb_mux";
> +	mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> +	/* An external source needs to tell us what to do */
> +	mux->nb.notifier_call = intel_usb_mux_notifier;
> +	ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> +	if (ret) {
> +		dev_err(&mux->edev.dev, "failed to register notifier\n");
> +		extcon_dev_unregister(&mux->edev);
> +		goto err;
> +	}
> +	return mux;
> +err:
> +	iounmap(mux->regs);
> +	kfree(mux);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> +{
> +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> +	extcon_dev_unregister(&mux->edev);
> +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> +	iounmap(mux->regs);
> +	kfree(mux);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);

so who's gonna call these two functions ? IMO, this looks like a recipe
for randbuild breakage.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Felipe Balbi]

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


Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> Intel Braswell/Cherrytrail has an internal mux that shares
> one USB port between USB Device Controller and xHCI. The
> same mux is found on several SOCs from Intel, but only on
> a few Cherrytrail based platforms the OS is expected to
> configure it. Normally BIOS takes care of it.
>
> The driver for the mux is an "extcon" driver. With this we
> only register the mux if it's detected.
>
> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/host/pci-quirks.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index f940056..4e3016a 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -16,6 +16,7 @@
>  #include <linux/export.h>
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/extcon/intel_usb_mux.h>
>  #include "pci-quirks.h"
>  #include "xhci-ext-caps.h"
>  
> @@ -1029,9 +1030,36 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>  	writel(val, base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
>  
>  hc_init:
> -	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
>  		usb_enable_intel_xhci_ports(pdev);
>  
> +		/*
> +		 * Initialize the internal mux that shares a port between USB
> +		 * Device Controller and xHCI on platforms that have it.
> +		 */
> +#define XHCI_INTEL_VENDOR_CAPS 192
> +#define XHCI_INTEL_USB_MUX_OFFSET 0x80d8
> +		ext_cap_offset = xhci_find_next_cap_offset(base,
> +							XHCI_HCC_PARAMS_OFFSET);
> +		ext_cap_offset = xhci_find_ext_cap_by_id(base, ext_cap_offset,
> +							XHCI_INTEL_VENDOR_CAPS);
> +		if (ext_cap_offset) {
> +			struct intel_usb_mux *mux;
> +			struct resource r;
> +
> +			r.start = pci_resource_start(pdev, 0) +
> +					XHCI_INTEL_USB_MUX_OFFSET;
> +			r.end   = r.start + 8;
> +			r.flags = IORESOURCE_MEM;
> +
> +			mux = intel_usb_mux_register(&pdev->dev, &r);
> +			if (IS_ERR(mux) && PTR_ERR(mux) == -ENOTSUPP)
> +				dev_dbg(&pdev->dev, "USB mux not supported\n");
> +			else if (IS_ERR(mux))
> +				dev_err(&pdev->dev, "failed to register mux\n");

IMHO, this should be creating a child device instead of calling
intel_usb_mux_register() directly. That way, your mux driver could
actually _be_ a driver. Seems like all you need to do from this point is
a register a simple platform_device which is a child of xhci, see
platform_device_register_simple() for how to do this.

Or rather, platform_device_register_rsndata() passing xhci's device
pointer as parent.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Felipe,

On Tue, Dec 01, 2015 at 02:34:34PM -0600, Felipe Balbi wrote:

[snip]

> > +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> > +
> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > +{
> > +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> > +	extcon_dev_unregister(&mux->edev);
> > +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> > +	iounmap(mux->regs);
> > +	kfree(mux);
> > +}
> > +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> 
> so who's gonna call these two functions ? IMO, this looks like a recipe
> for randbuild breakage.

There are function stubs on header file when the functions aren't
available.
But also notice CONFIG_EXTCON_INTEL_USB is not user-selectable. It's
automatically selected when a driver that requires it is selected too.

With the 2 cases above, IMHO it should not bring issues with randbuild
tests.

Br, David

> 
> -- 
> balbi

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[Felipe Balbi]

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


Hi,

David Cohen <david.a.cohen@linux.intel.com> writes:
> Hi Felipe,
>
> On Tue, Dec 01, 2015 at 02:34:34PM -0600, Felipe Balbi wrote:
>
> [snip]
>
>> > +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
>> > +
>> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
>> > +{
>> > +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
>> > +	extcon_dev_unregister(&mux->edev);
>> > +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
>> > +	iounmap(mux->regs);
>> > +	kfree(mux);
>> > +}
>> > +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
>> 
>> so who's gonna call these two functions ? IMO, this looks like a recipe
>> for randbuild breakage.
>
> There are function stubs on header file when the functions aren't
> available.
> But also notice CONFIG_EXTCON_INTEL_USB is not user-selectable. It's
> automatically selected when a driver that requires it is selected too.
>
> With the 2 cases above, IMHO it should not bring issues with randbuild
> tests.

right now it won't break, that's correct :-) But things change over time
and this is, at least, fragile. I mean, why couldn't this mux be an
actual driver ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Lu Baolu]

On 12/01/2015 09:32 PM, Heikki Krogerus wrote:
> Intel Braswell/Cherrytrail has an internal mux that shares
> one USB port between USB Device Controller and xHCI. The
> same mux is found on several SOCs from Intel, but only on
> a few Cherrytrail based platforms the OS is expected to
> configure it. Normally BIOS takes care of it.
>
> The driver for the mux is an "extcon" driver. With this we
> only register the mux if it's detected.
>
> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/host/pci-quirks.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index f940056..4e3016a 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -16,6 +16,7 @@
>  #include <linux/export.h>
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/extcon/intel_usb_mux.h>
>  #include "pci-quirks.h"
>  #include "xhci-ext-caps.h"
>  
> @@ -1029,9 +1030,36 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>  	writel(val, base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
>  
>  hc_init:
> -	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
>  		usb_enable_intel_xhci_ports(pdev);
>  
> +		/*
> +		 * Initialize the internal mux that shares a port between USB
> +		 * Device Controller and xHCI on platforms that have it.
> +		 */
> +#define XHCI_INTEL_VENDOR_CAPS 192
> +#define XHCI_INTEL_USB_MUX_OFFSET 0x80d8
> +		ext_cap_offset = xhci_find_next_cap_offset(base,
> +							XHCI_HCC_PARAMS_OFFSET);
> +		ext_cap_offset = xhci_find_ext_cap_by_id(base, ext_cap_offset,
> +							XHCI_INTEL_VENDOR_CAPS);

Hi Heikki,

This helper has been replaced with xhci_find_next_ext_cap() in
commit d5ddcdf(xhci: rework xhci extended capability list parsing functions).

Need to fix this, otherwise it will not pass compile on top of usb-next.

Thanks,
Baolu

> +		if (ext_cap_offset) {
> +			struct intel_usb_mux *mux;
> +			struct resource r;
> +
> +			r.start = pci_resource_start(pdev, 0) +
> +					XHCI_INTEL_USB_MUX_OFFSET;
> +			r.end   = r.start + 8;
> +			r.flags = IORESOURCE_MEM;
> +
> +			mux = intel_usb_mux_register(&pdev->dev, &r);
> +			if (IS_ERR(mux) && PTR_ERR(mux) == -ENOTSUPP)
> +				dev_dbg(&pdev->dev, "USB mux not supported\n");
> +			else if (IS_ERR(mux))
> +				dev_err(&pdev->dev, "failed to register mux\n");
> +		}
> +	}
> +
>  	op_reg_base = base + XHCI_HC_LENGTH(readl(base));
>  
>  	/* Wait for the host controller to be ready before writing any

Re: [PATCH 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Heikki Krogerus]

Hi Felipe,

> IMHO, this should be creating a child device instead of calling
> intel_usb_mux_register() directly. That way, your mux driver could
> actually _be_ a driver. Seems like all you need to do from this point is
> a register a simple platform_device which is a child of xhci, see
> platform_device_register_simple() for how to do this.
> 
> Or rather, platform_device_register_rsndata() passing xhci's device
> pointer as parent.

That was the plan originally, but unfortunately it does not work in
this case. It creates conflict as platform_device_add() call will then
claim part of io memory belonging to xHCI, making xHCI fail to probe.


Thanks,

-- 
heikki

Re: [PATCH 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Heikki Krogerus]

Hi,

> > @@ -1029,9 +1030,36 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
> >  	writel(val, base + ext_cap_offset + XHCI_LEGACY_CONTROL_OFFSET);
> >  
> >  hc_init:
> > -	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> >  		usb_enable_intel_xhci_ports(pdev);
> >  
> > +		/*
> > +		 * Initialize the internal mux that shares a port between USB
> > +		 * Device Controller and xHCI on platforms that have it.
> > +		 */
> > +#define XHCI_INTEL_VENDOR_CAPS 192
> > +#define XHCI_INTEL_USB_MUX_OFFSET 0x80d8
> > +		ext_cap_offset = xhci_find_next_cap_offset(base,
> > +							XHCI_HCC_PARAMS_OFFSET);
> > +		ext_cap_offset = xhci_find_ext_cap_by_id(base, ext_cap_offset,
> > +							XHCI_INTEL_VENDOR_CAPS);
> 
> Hi Heikki,
> 
> This helper has been replaced with xhci_find_next_ext_cap() in
> commit d5ddcdf(xhci: rework xhci extended capability list parsing functions).
> 
> Need to fix this, otherwise it will not pass compile on top of usb-next.

Thanks Baolu. I'll fix it.


Cheers,

-- 
heikki

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[Heikki Krogerus]

Hi David,

<snip>

> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > +{
> 
> There are still 2 pending comments for this unregister function:
> 
> 1) How about a protection against unbalanced unregistering? In case an
> user mistakenly unregisters twice or unregisters without a previous
> registering.

True. You already pointed that out in our off-list thread, but I
forgot. Sorry about that.

> 2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
> set mux to automatic mode again. Or maybe you could save CFG0's initial
> value and set it again here. Anyway, when unregistering the mux driver
> you need to make sure the mux goes back to its original configuration.

This one is already been taken care of..

> > +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> > +	extcon_dev_unregister(&mux->edev);
> > +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);

        ^^^^^^^^^^^^^^^^^^^^


Thanks,

-- 
heikki

Re: [PATCH 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Felipe Balbi]

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


Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> Hi Felipe,
>
>> IMHO, this should be creating a child device instead of calling
>> intel_usb_mux_register() directly. That way, your mux driver could
>> actually _be_ a driver. Seems like all you need to do from this point is
>> a register a simple platform_device which is a child of xhci, see
>> platform_device_register_simple() for how to do this.
>> 
>> Or rather, platform_device_register_rsndata() passing xhci's device
>> pointer as parent.
>
> That was the plan originally, but unfortunately it does not work in
> this case. It creates conflict as platform_device_add() call will then
> claim part of io memory belonging to xHCI, making xHCI fail to probe.

okay, understood.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

On Wed, Dec 02, 2015 at 12:27:10PM +0200, Heikki Krogerus wrote:
> Hi David,
> 
> <snip>
> 
> > > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > > +{
> > 
> > There are still 2 pending comments for this unregister function:
> > 
> > 1) How about a protection against unbalanced unregistering? In case an
> > user mistakenly unregisters twice or unregisters without a previous
> > registering.
> 
> True. You already pointed that out in our off-list thread, but I
> forgot. Sorry about that.
> 
> > 2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
> > set mux to automatic mode again. Or maybe you could save CFG0's initial
> > value and set it again here. Anyway, when unregistering the mux driver
> > you need to make sure the mux goes back to its original configuration.
> 
> This one is already been taken care of..
> 
> > > +	extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> > > +	extcon_dev_unregister(&mux->edev);
> > > +	writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> 
>         ^^^^^^^^^^^^^^^^^^^^

Of course :)
Sorry I missed this part.

Br, David