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

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

[Heikki Krogerus]

Changes since v1:
- Using xhci_find_next_ext_cap() as suggested by Baolu
- Protection agains unbalanced uregistering as suggested by David Cohen

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    | 118 +++++++++++++++++++++++++++++++++++
 drivers/usb/host/pci-quirks.c        |  26 +++++++-
 include/linux/extcon/intel_usb_mux.h |  31 +++++++++
 5 files changed, 180 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

[PATCHv2 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    | 118 +++++++++++++++++++++++++++++++++++
 include/linux/extcon/intel_usb_mux.h |  31 +++++++++
 4 files changed, 155 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..3da6039
--- /dev/null
+++ b/drivers/extcon/extcon-intel-usb.c
@@ -0,0 +1,118 @@
+/**
+ * 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)
+{
+	if (!mux)
+		return;
+
+	if (WARN_ON(IS_ERR_OR_NULL(extcon_get_extcon_dev(mux->edev.name))))
+		return;
+
+	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: [PATCHv2 1/2] extcon: add driver for Intel USB mux

[Chanwoo Choi]

Hi Heikki,

I'm sorry for delay reply.

On 2015년 12월 03일 18:29, 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    | 118 +++++++++++++++++++++++++++++++++++
>  include/linux/extcon/intel_usb_mux.h |  31 +++++++++
>  4 files changed, 155 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..3da6039
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,118 @@
> +/**
> + * 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)
> +{
> +	if (!mux)
> +		return;
> +
> +	if (WARN_ON(IS_ERR_OR_NULL(extcon_get_extcon_dev(mux->edev.name))))
> +		return;
> +
> +	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);

I do never want to add some specific funtcion for only this driver.
I think is not appropriate way.
- intel_usb_mux_unregister
- intel_usb_mux_register

The client driver using extcon driver should use the standard extcon API
for code consistency. Also, I'll do the more detailed review for this patch.

> 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) { }

ditto.

> +#endif /* CONFIG_EXTCON_INTEL_USB */
> +
> +#endif /* _INTEL_USB_MUX_H */
> 

Thanks,
Chanwoo Choi

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

[Heikki Krogerus]

Hi,

> I do never want to add some specific funtcion for only this driver.
> I think is not appropriate way.
> - intel_usb_mux_unregister
> - intel_usb_mux_register
> 
> The client driver using extcon driver should use the standard extcon API
> for code consistency. Also, I'll do the more detailed review for this patch.

The internal mux we are controlling here is physically separate
device. Ideally we could populate child device for it, but since that
is not possible because of the resource conflict, we use the library
approach, which is really not that uncommon.

I don't think I agree with your point even at general level. The
control required to handle this mux, even though simple, is enough to
deserve to be separated from xHCI code. xHCI should not need to care
about anything else expect does it have the mux, i.e. does it need to
register it or not. It should not need to care about how it needs to
be controlled or even what it is. We may decide to create something
else out of it instead of an extcon device later.

But in any case, the mux is available on all new Intel platforms, but
it needs to be controlled by OS only in few "special" cases. We can
not force xHCI (or pci-quirks.c to be more precise) to be aware of
these "special" cases. The only way to make it work like that would
bet by using ifdefs, and we really really don't want that.

And please also note that, though for now we only expect the mux
control registers to be part of xHCI MMIO, that is not always the
case. The control registers are part of the device controller MMIO on
some platforms. We do not want to duplicate the whole control of the
mux if/when we need the OS to be in control of it on a platform that
has those control registers mapped somewhere else then xHCI MMIO,

So I would say that we have pretty good justification for separating
the mux control, which means unfortunately custom API in this case.

But if you would prefer that we put the files somewhere else then
drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you
like, we can put it to drivers/usb/host/ as that is where
pci-quirks.c is. That way I think we can also put the header to
include/usb/.


Thanks,

-- 
heikki

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

[Chanwoo Choi]

Hi,

On 2015년 12월 04일 17:51, Heikki Krogerus wrote:
> Hi,
> 
>> I do never want to add some specific funtcion for only this driver.
>> I think is not appropriate way.
>> - intel_usb_mux_unregister
>> - intel_usb_mux_register
>>
>> The client driver using extcon driver should use the standard extcon API
>> for code consistency. Also, I'll do the more detailed review for this patch.
> 
> The internal mux we are controlling here is physically separate
> device. Ideally we could populate child device for it, but since that
> is not possible because of the resource conflict, we use the library
> approach, which is really not that uncommon.

New added functions for only specific device driver is not library. 

The all device drivers which is included in some framework should
connect to the other device driver through only framework API as following:
	--------------------          ----------------
	| EXTCON framework |<-------->| USB framework |
	--------------------          -----------------	
		|                            |
		|                            |
	extcon-intel-usb.c             pci-quirks.c

But, in this case, added funticon is just direct call function
without any standard API. The below case is never appropriate implementation.

	--------------------          ----------------
	| EXTCON framework |          | USB framework |
	--------------------          -----------------	
		|                            |
		|                            |
	extcon-intel-usb.c <--------  pci-quirks.c

> 
> I don't think I agree with your point even at general level. The
> control required to handle this mux, even though simple, is enough to
> deserve to be separated from xHCI code. xHCI should not need to care
> about anything else expect does it have the mux, i.e. does it need to
> register it or not. It should not need to care about how it needs to
> be controlled or even what it is. We may decide to create something
> else out of it instead of an extcon device later.
> 
> But in any case, the mux is available on all new Intel platforms, but
> it needs to be controlled by OS only in few "special" cases. We can
> not force xHCI (or pci-quirks.c to be more precise) to be aware of
> these "special" cases. The only way to make it work like that would
> bet by using ifdefs, and we really really don't want that.
> 
> And please also note that, though for now we only expect the mux
> control registers to be part of xHCI MMIO, that is not always the
> case. The control registers are part of the device controller MMIO on
> some platforms. We do not want to duplicate the whole control of the
> mux if/when we need the OS to be in control of it on a platform that
> has those control registers mapped somewhere else then xHCI MMIO,
> 
> So I would say that we have pretty good justification for separating
> the mux control, which means unfortunately custom API in this case.
> 
> But if you would prefer that we put the files somewhere else then
> drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you
> like, we can put it to drivers/usb/host/ as that is where
> pci-quirks.c is. That way I think we can also put the header to
> include/usb/.

There are the two type of extcon drivers as following:
- provider extcon driver which use the devm_extcon_dev_register() and extcon_set_cable_state().
- client extcon driver which use the extcon_register_notifier() and extcon_set_cable_state() usually.
The drivers/extcon directory only includes the provider extcon driver.

You make the extcon-intel-usb.c as provider extcon driver.
At the same time, this driver is used for client extcon driver
by using the extcon_register_notifier(). If you want to recevie
the notification from extcon_register_notifier(), the extcon device
should update the state of external connector throught extcon_set_cable_state().
But, this driver don' use the extcon_set_cable_state().

I think that you should divide it according to role.

Again, the usage case of extcon have to consist of both provider extcon driver
and client extcon driver. If there is no provider extcon driver,
client extcon driver don't receive the any notification of external connector
from provider extcon driver.

Thanks,
Chanwoo Choi

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

[Heikki Krogerus]

Hi,

On Mon, Dec 07, 2015 at 10:24:22AM +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 2015년 12월 04일 17:51, Heikki Krogerus wrote:
> > Hi,
> > 
> >> I do never want to add some specific funtcion for only this driver.
> >> I think is not appropriate way.
> >> - intel_usb_mux_unregister
> >> - intel_usb_mux_register
> >>
> >> The client driver using extcon driver should use the standard extcon API
> >> for code consistency. Also, I'll do the more detailed review for this patch.
> > 
> > The internal mux we are controlling here is physically separate
> > device. Ideally we could populate child device for it, but since that
> > is not possible because of the resource conflict, we use the library
> > approach, which is really not that uncommon.
> 
> New added functions for only specific device driver is not library. 
> 
> The all device drivers which is included in some framework should
> connect to the other device driver through only framework API as following:
> 	--------------------          ----------------
> 	| EXTCON framework |<-------->| USB framework |
> 	--------------------          -----------------	
> 		|                            |
> 		|                            |
> 	extcon-intel-usb.c             pci-quirks.c
> 
> But, in this case, added funticon is just direct call function
> without any standard API. The below case is never appropriate implementation.
> 
> 	--------------------          ----------------
> 	| EXTCON framework |          | USB framework |
> 	--------------------          -----------------	
> 		|                            |
> 		|                            |
> 	extcon-intel-usb.c <--------  pci-quirks.c

Man.. Cal it what you want, but like I said, exposing driver specific
API is not ideal, but it is acceptable in special cases like this
where we simply are not able to populate child device. If nothing
else, then at least the fact that the code for the mux would otherwise
need to be duplicated, is enough to justify it.

> > I don't think I agree with your point even at general level. The
> > control required to handle this mux, even though simple, is enough to
> > deserve to be separated from xHCI code. xHCI should not need to care
> > about anything else expect does it have the mux, i.e. does it need to
> > register it or not. It should not need to care about how it needs to
> > be controlled or even what it is. We may decide to create something
> > else out of it instead of an extcon device later.
> > 
> > But in any case, the mux is available on all new Intel platforms, but
> > it needs to be controlled by OS only in few "special" cases. We can
> > not force xHCI (or pci-quirks.c to be more precise) to be aware of
> > these "special" cases. The only way to make it work like that would
> > bet by using ifdefs, and we really really don't want that.
> > 
> > And please also note that, though for now we only expect the mux
> > control registers to be part of xHCI MMIO, that is not always the
> > case. The control registers are part of the device controller MMIO on
> > some platforms. We do not want to duplicate the whole control of the
> > mux if/when we need the OS to be in control of it on a platform that
> > has those control registers mapped somewhere else then xHCI MMIO,
> > 
> > So I would say that we have pretty good justification for separating
> > the mux control, which means unfortunately custom API in this case.
> > 
> > But if you would prefer that we put the files somewhere else then
> > drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you
> > like, we can put it to drivers/usb/host/ as that is where
> > pci-quirks.c is. That way I think we can also put the header to
> > include/usb/.
> 
> There are the two type of extcon drivers as following:
> - provider extcon driver which use the devm_extcon_dev_register() and extcon_set_cable_state().
> - client extcon driver which use the extcon_register_notifier() and extcon_set_cable_state() usually.
> The drivers/extcon directory only includes the provider extcon driver.
> 
> You make the extcon-intel-usb.c as provider extcon driver.
> At the same time, this driver is used for client extcon driver
> by using the extcon_register_notifier(). If you want to recevie
> the notification from extcon_register_notifier(), the extcon device
> should update the state of external connector throught extcon_set_cable_state().
> But, this driver don' use the extcon_set_cable_state().
> 
> I think that you should divide it according to role.
> 
> Again, the usage case of extcon have to consist of both provider extcon driver
> and client extcon driver. If there is no provider extcon driver,
> client extcon driver don't receive the any notification of external connector
> from provider extcon driver.

What you are saying is that it is OK for both "client" and "provider"
to change the state, but only client is allowed to react to a state
change? You got to admit that the roles are pretty obscure here...

In any case, I'm using the framework the way it allows itself to be
used. If you want your framework to be used in a particular way, then
you need to protect it from being used otherwise. Now anything with a
handle to the extcon_dev can use it in every way possible. You are not
documenting how you want the framework to be used. The only document
for extcon in Documentation/extcon/ is about porting stuff from some
Android specific "switch class" to extcon. Nowhere do you define what
is a "client" and what is a "provider" and what are they meant for.

If you want to limit what a "client" can do, you need to separate the
API for it. Use the gpio API as an example. Check how the consumers
and drivers are separated into their own headers in
include/linux/gpio/. Keep the include/extcon.h header as legacy and
deprecate it.

And if you do that, the framework should really be improved with a few
basic things regarding the "clients". At least start using some kind
of ref counting with them. Now nothing really prevents a "provider"
from being removed even if it has users (clients), or does it? This
basically also shows how obscure the line between a "client" and
"provider" is at the moment.

Right now with what we have I see nothing wrong with my approach.


Cheers,

-- 
heikki

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

[Chanwoo Choi]

Hi,

On 2015년 12월 07일 21:52, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Dec 07, 2015 at 10:24:22AM +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2015년 12월 04일 17:51, Heikki Krogerus wrote:
>>> Hi,
>>>
>>>> I do never want to add some specific funtcion for only this driver.
>>>> I think is not appropriate way.
>>>> - intel_usb_mux_unregister
>>>> - intel_usb_mux_register
>>>>
>>>> The client driver using extcon driver should use the standard extcon API
>>>> for code consistency. Also, I'll do the more detailed review for this patch.
>>>
>>> The internal mux we are controlling here is physically separate
>>> device. Ideally we could populate child device for it, but since that
>>> is not possible because of the resource conflict, we use the library
>>> approach, which is really not that uncommon.
>>
>> New added functions for only specific device driver is not library. 
>>
>> The all device drivers which is included in some framework should
>> connect to the other device driver through only framework API as following:
>> 	--------------------          ----------------
>> 	| EXTCON framework |<-------->| USB framework |
>> 	--------------------          -----------------	
>> 		|                            |
>> 		|                            |
>> 	extcon-intel-usb.c             pci-quirks.c
>>
>> But, in this case, added funticon is just direct call function
>> without any standard API. The below case is never appropriate implementation.
>>
>> 	--------------------          ----------------
>> 	| EXTCON framework |          | USB framework |
>> 	--------------------          -----------------	
>> 		|                            |
>> 		|                            |
>> 	extcon-intel-usb.c <--------  pci-quirks.c
> 
> Man.. Cal it what you want, but like I said, exposing driver specific
> API is not ideal, but it is acceptable in special cases like this
> where we simply are not able to populate child device. If nothing
> else, then at least the fact that the code for the mux would otherwise
> need to be duplicated, is enough to justify it.
> 
>>> I don't think I agree with your point even at general level. The
>>> control required to handle this mux, even though simple, is enough to
>>> deserve to be separated from xHCI code. xHCI should not need to care
>>> about anything else expect does it have the mux, i.e. does it need to
>>> register it or not. It should not need to care about how it needs to
>>> be controlled or even what it is. We may decide to create something
>>> else out of it instead of an extcon device later.
>>>
>>> But in any case, the mux is available on all new Intel platforms, but
>>> it needs to be controlled by OS only in few "special" cases. We can
>>> not force xHCI (or pci-quirks.c to be more precise) to be aware of
>>> these "special" cases. The only way to make it work like that would
>>> bet by using ifdefs, and we really really don't want that.
>>>
>>> And please also note that, though for now we only expect the mux
>>> control registers to be part of xHCI MMIO, that is not always the
>>> case. The control registers are part of the device controller MMIO on
>>> some platforms. We do not want to duplicate the whole control of the
>>> mux if/when we need the OS to be in control of it on a platform that
>>> has those control registers mapped somewhere else then xHCI MMIO,
>>>
>>> So I would say that we have pretty good justification for separating
>>> the mux control, which means unfortunately custom API in this case.
>>>
>>> But if you would prefer that we put the files somewhere else then
>>> drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you
>>> like, we can put it to drivers/usb/host/ as that is where
>>> pci-quirks.c is. That way I think we can also put the header to
>>> include/usb/.
>>
>> There are the two type of extcon drivers as following:
>> - provider extcon driver which use the devm_extcon_dev_register() and extcon_set_cable_state().
>> - client extcon driver which use the extcon_register_notifier() and extcon_set_cable_state() usually.
>> The drivers/extcon directory only includes the provider extcon driver.
>>
>> You make the extcon-intel-usb.c as provider extcon driver.
>> At the same time, this driver is used for client extcon driver
>> by using the extcon_register_notifier(). If you want to recevie
>> the notification from extcon_register_notifier(), the extcon device
>> should update the state of external connector throught extcon_set_cable_state().
>> But, this driver don' use the extcon_set_cable_state().
>>
>> I think that you should divide it according to role.
>>
>> Again, the usage case of extcon have to consist of both provider extcon driver
>> and client extcon driver. If there is no provider extcon driver,
>> client extcon driver don't receive the any notification of external connector
>> from provider extcon driver.
> 
> What you are saying is that it is OK for both "client" and "provider"
> to change the state, but only client is allowed to react to a state
> change? You got to admit that the roles are pretty obscure here...

Yes, only client driver is able to take some behavior after receiving
the notification from extcon provider driver.

> 
> In any case, I'm using the framework the way it allows itself to be
> used. If you want your framework to be used in a particular way, then
> you need to protect it from being used otherwise. Now anything with a
> handle to the extcon_dev can use it in every way possible. You are not
> documenting how you want the framework to be used. The only document
> for extcon in Documentation/extcon/ is about porting stuff from some
> Android specific "switch class" to extcon. Nowhere do you define what
> is a "client" and what is a "provider" and what are they meant for.

I agree. The extcon framework uses the 'extcon_dev' structure for both
provider and client driver. As you mentioned, it is the problem.
So, I'll resolve this issue to protect change the state of external connector
from client driver.

Unitl now, I'm focusing on expand the usage case of extcon framework
and improve the extcon core feature. As you comment, It is necessary task
for extcon framework. The improvement of extcon framework is going on.

> 
> If you want to limit what a "client" can do, you need to separate the
> API for it. Use the gpio API as an example. Check how the consumers
> and drivers are separated into their own headers in
> include/linux/gpio/. Keep the include/extcon.h header as legacy and
> deprecate it.
> 
> And if you do that, the framework should really be improved with a few
> basic things regarding the "clients". At least start using some kind
> of ref counting with them. Now nothing really prevents a "provider"
> from being removed even if it has users (clients), or does it? This
> basically also shows how obscure the line between a "client" and
> "provider" is at the moment.

I agree. There are not enough document for extcon framework.
After updating the extcon framework above, I'll make the appropriate
document for guide.

> 
> Right now with what we have I see nothing wrong with my approach.

I can't agree to add specific function for only one device driver.
As I commented, it is not appropriate way.

Thanks,
Chanwoo Choi

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

[Heikki Krogerus]

Hi,

On Tue, Dec 08, 2015 at 10:17:47AM +0900, Chanwoo Choi wrote:
> I can't agree to add specific function for only one device driver.
> As I commented, it is not appropriate way.

OK, I'll prepare something else for this.


Thanks,

-- 
heikki

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

[Sergei Shtylyov]

On 12/03/2015 12:29 PM, 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>
[...]
> diff --git a/drivers/extcon/extcon-intel-usb.c b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 0000000..3da6039
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,118 @@
[...]
> +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);

    I don't see where are you calling extcon_set_cable_state() fot the 
"USB-HOST" cable...
This doesn't seem a legitimate extcon driver to me... :-/

> +	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);

    So in reality this is an extcon client, not a provider? BTW, this API 
isn't recommended...

MBR, Sergei

[PATCHv2 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 | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 26cb8c8..ee875e1 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"
 
@@ -1022,9 +1023,32 @@ 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
+		if (xhci_find_next_ext_cap(base, 0, XHCI_INTEL_VENDOR_CAPS)) {
+			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: [PATCHv2 2/2] usb: pci-quirks: register USB mux found on Cherrytrail SOC

[Sergei Shtylyov]

Hello.

On 12/03/2015 12:29 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.

    Hm, I had somewhat identical case on the Renesas SoC: the 2 channel mux 
was mapped to the USB device PHY register space, so I chose to implement a PHY 
driver, not extcon...
    I don't quite understand how mux maps to the extcon core -- doesn't it 
provide support only the input signals?

> 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 | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 26cb8c8..ee875e1 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
[...]
> @@ -1022,9 +1023,32 @@ 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
> +		if (xhci_find_next_ext_cap(base, 0, XHCI_INTEL_VENDOR_CAPS)) {
> +			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)

    I think you can drop IS_ERR() check here...

> +				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

MBR, Sergei