[PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

In some Intel SOCs, a single USB port is shared between USB device and
host controller and an internal mux is used to control the selection of
port by host/device controllers. This driver adds support for the USB
internal mux, and all consumers of this mux can use interfaces provided
by mux subsytem to control the state of the internal mux.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/mux/Kconfig               |  13 ++++
 drivers/mux/Makefile              |   1 +
 drivers/mux/mux-intel-usb.c       | 132 ++++++++++++++++++++++++++++++++++++++
 include/linux/mux/mux-intel-usb.h |  22 +++++++
 4 files changed, 168 insertions(+)
 create mode 100644 drivers/mux/mux-intel-usb.c
 create mode 100644 include/linux/mux/mux-intel-usb.h

diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index e8f1df7..0e3af09 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -56,4 +56,17 @@ config MUX_MMIO
 	  To compile the driver as a module, choose M here: the module will
 	  be called mux-mmio.
 
+config MUX_INTEL_USB
+	tristate "Intel USB Mux"
+	depends on USB
+	help
+	  In some Intel SOCs, a single USB port is shared between USB
+	  device and host controller and an internal mux is used to
+	  control the selection of port by host/device controllers. This
+	  driver adds support for the USB internal mux, and all consumers
+	  of this mux can use interfaces provided by mux subsytem to control
+	  the state of the internal mux.
+
+	  To compile the driver as a module, choose M here.
+
 endif
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index 6bac5b0..9154616 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
 obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
 obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
 obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
+obj-$(CONFIG_MUX_INTEL_USB)	+= mux-intel-usb.o
diff --git a/drivers/mux/mux-intel-usb.c b/drivers/mux/mux-intel-usb.c
new file mode 100644
index 0000000..587e9bb
--- /dev/null
+++ b/drivers/mux/mux-intel-usb.c
@@ -0,0 +1,132 @@
+/*
+ * Intel USB Multiplexer Driver
+ *
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
+ *
+ * This driver is written based on extcon-intel-usb driver submitted by
+ * 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/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mux/driver.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/mux/mux-intel-usb.h>
+#include <linux/io.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 mux_intel_usb {
+	struct device *dev;
+	void __iomem *regs;
+};
+
+static int mux_intel_usb_set(struct mux_control *mux_ctrl, int state)
+{
+	struct mux_intel_usb *mux = mux_chip_priv(mux_ctrl->chip);
+	u32 val;
+
+	dev_info(mux->dev, "Intel USB mux set called");
+
+	switch (state) {
+	case INTEL_USB_MUX_STATE_HOST:
+		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
+		break;
+	case INTEL_USB_MUX_STATE_DEVICE:
+		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
+		      CFG0_SW_DRD_STATIC_DEV;
+		break;
+	default:
+		return 0;
+	};
+
+	writel(val, mux->regs);
+
+	return 0;
+}
+
+static const struct mux_control_ops mux_intel_usb_ops = {
+	.set = mux_intel_usb_set,
+};
+
+static int mux_intel_usb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mux_chip *mux_chip;
+	struct mux_intel_usb *mux;
+	struct resource *res;
+	int ret;
+
+	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux));
+	if (IS_ERR(mux_chip))
+		return PTR_ERR(mux_chip);
+
+	mux = mux_chip_priv(mux_chip);
+	mux->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get mem resource\n");
+		return -ENXIO;
+	}
+
+	mux->regs = devm_ioremap_nocache(dev, res->start, resource_size(res));
+	if (!mux->regs) {
+		dev_err(mux->dev, "mux regs io remap failed\n");
+		return -ENOMEM;
+	}
+
+	mux_chip->ops = &mux_intel_usb_ops;
+	mux_chip->mux->states = INTEL_USB_MUX_STATE_MAX;
+	mux_chip->mux->idle_state = MUX_IDLE_AS_IS;
+
+	ret = devm_mux_chip_register(dev, mux_chip);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev, "%u-way mux-controller registered\n",
+		 mux_chip->mux->states);
+
+	return 0;
+}
+
+static const struct platform_device_id mux_intel_usb_id[] = {
+        { .name = "intel-usb-mux", },
+        { }
+};
+MODULE_DEVICE_TABLE(platform, mux_intel_usb_id);
+
+static struct platform_driver mux_intel_usb_driver = {
+	.driver = {
+		.name = "intel-usb-mux",
+	},
+	.probe = mux_intel_usb_probe,
+	.id_table = mux_intel_usb_id,
+};
+module_platform_driver(mux_intel_usb_driver);
+
+MODULE_DESCRIPTION("Intel USB multiplexer driver");
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mux/mux-intel-usb.h b/include/linux/mux/mux-intel-usb.h
new file mode 100644
index 0000000..e77516c
--- /dev/null
+++ b/include/linux/mux/mux-intel-usb.h
@@ -0,0 +1,22 @@
+/*
+ * Intel USB multiplexer header file
+ *
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@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.
+ */
+
+#ifndef _LINUX_MUX_MUX_INTEL_USB_H
+#define _LINUX_MUX_MUX_INTEL_USB_H
+
+#include <linux/mux/consumer.h>
+
+#define INTEL_USB_MUX_STATE_HOST		0
+#define INTEL_USB_MUX_STATE_DEVICE		1
+#define INTEL_USB_MUX_STATE_MAX			2
+
+#endif /* _LINUX_MUX_MUX_INTEL_USB_H */
-- 
2.7.4

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Peter Rosin]

On 2017-05-30 02:47, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> In some Intel SOCs, a single USB port is shared between USB device and
> host controller and an internal mux is used to control the selection of
> port by host/device controllers. This driver adds support for the USB
> internal mux, and all consumers of this mux can use interfaces provided
> by mux subsytem to control the state of the internal mux.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Hi!

A few things make me curious...

1. How are platform devices w/o any match table probed? Do you have to
   manually load them, or what?

2. What are these "all the consumers of this mux" that you mention, and how
   do they find the correct mux control to interact with?

> ---
>  drivers/mux/Kconfig               |  13 ++++
>  drivers/mux/Makefile              |   1 +
>  drivers/mux/mux-intel-usb.c       | 132 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mux/mux-intel-usb.h |  22 +++++++
>  4 files changed, 168 insertions(+)
>  create mode 100644 drivers/mux/mux-intel-usb.c
>  create mode 100644 include/linux/mux/mux-intel-usb.h
> 
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index e8f1df7..0e3af09 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -56,4 +56,17 @@ config MUX_MMIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-mmio.
>  
> +config MUX_INTEL_USB
> +	tristate "Intel USB Mux"
> +	depends on USB

Why depend on USB?

> +	help
> +	  In some Intel SOCs, a single USB port is shared between USB
> +	  device and host controller and an internal mux is used to
> +	  control the selection of port by host/device controllers. This
> +	  driver adds support for the USB internal mux, and all consumers
> +	  of this mux can use interfaces provided by mux subsytem to control
> +	  the state of the internal mux.
> +
> +	  To compile the driver as a module, choose M here.
> +
>  endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 6bac5b0..9154616 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> +obj-$(CONFIG_MUX_INTEL_USB)	+= mux-intel-usb.o

Alphabetical order, please.

> diff --git a/drivers/mux/mux-intel-usb.c b/drivers/mux/mux-intel-usb.c
> new file mode 100644
> index 0000000..587e9bb
> --- /dev/null
> +++ b/drivers/mux/mux-intel-usb.c
> @@ -0,0 +1,132 @@
> +/*
> + * Intel USB Multiplexer Driver
> + *
> + * Copyright (C) 2017 Intel Corporation
> + *
> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
> + *
> + * This driver is written based on extcon-intel-usb driver submitted by
> + * 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/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mux/driver.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/mux/mux-intel-usb.h>
> +#include <linux/io.h>

Alphabetical order, please.

> +
> +#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 mux_intel_usb {
> +	struct device *dev;
> +	void __iomem *regs;
> +};
> +
> +static int mux_intel_usb_set(struct mux_control *mux_ctrl, int state)
> +{
> +	struct mux_intel_usb *mux = mux_chip_priv(mux_ctrl->chip);
> +	u32 val;
> +
> +	dev_info(mux->dev, "Intel USB mux set called");

Isn't this very spammy?

Cheers,
peda

> +
> +	switch (state) {
> +	case INTEL_USB_MUX_STATE_HOST:
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> +		break;
> +	case INTEL_USB_MUX_STATE_DEVICE:
> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> +		      CFG0_SW_DRD_STATIC_DEV;
> +		break;
> +	default:
> +		return 0;
> +	};
> +
> +	writel(val, mux->regs);
> +
> +	return 0;
> +}
> +
> +static const struct mux_control_ops mux_intel_usb_ops = {
> +	.set = mux_intel_usb_set,
> +};
> +
> +static int mux_intel_usb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mux_chip *mux_chip;
> +	struct mux_intel_usb *mux;
> +	struct resource *res;
> +	int ret;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux));
> +	if (IS_ERR(mux_chip))
> +		return PTR_ERR(mux_chip);
> +
> +	mux = mux_chip_priv(mux_chip);
> +	mux->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get mem resource\n");
> +		return -ENXIO;
> +	}
> +
> +	mux->regs = devm_ioremap_nocache(dev, res->start, resource_size(res));
> +	if (!mux->regs) {
> +		dev_err(mux->dev, "mux regs io remap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	mux_chip->ops = &mux_intel_usb_ops;
> +	mux_chip->mux->states = INTEL_USB_MUX_STATE_MAX;
> +	mux_chip->mux->idle_state = MUX_IDLE_AS_IS;
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev, "%u-way mux-controller registered\n",
> +		 mux_chip->mux->states);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id mux_intel_usb_id[] = {
> +        { .name = "intel-usb-mux", },
> +        { }
> +};
> +MODULE_DEVICE_TABLE(platform, mux_intel_usb_id);
> +
> +static struct platform_driver mux_intel_usb_driver = {
> +	.driver = {
> +		.name = "intel-usb-mux",
> +	},
> +	.probe = mux_intel_usb_probe,
> +	.id_table = mux_intel_usb_id,
> +};
> +module_platform_driver(mux_intel_usb_driver);
> +
> +MODULE_DESCRIPTION("Intel USB multiplexer driver");
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mux/mux-intel-usb.h b/include/linux/mux/mux-intel-usb.h
> new file mode 100644
> index 0000000..e77516c
> --- /dev/null
> +++ b/include/linux/mux/mux-intel-usb.h
> @@ -0,0 +1,22 @@
> +/*
> + * Intel USB multiplexer header file
> + *
> + * Copyright (C) 2017 Intel Corporation
> + *
> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@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.
> + */
> +
> +#ifndef _LINUX_MUX_MUX_INTEL_USB_H
> +#define _LINUX_MUX_MUX_INTEL_USB_H
> +
> +#include <linux/mux/consumer.h>
> +
> +#define INTEL_USB_MUX_STATE_HOST		0
> +#define INTEL_USB_MUX_STATE_DEVICE		1
> +#define INTEL_USB_MUX_STATE_MAX			2
> +
> +#endif /* _LINUX_MUX_MUX_INTEL_USB_H */
> 

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Andy Shevchenko]

On Tue, May 30, 2017 at 3:47 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> In some Intel SOCs, a single USB port is shared between USB device and

SoCs

> host controller and an internal mux is used to control the selection of
> port by host/device controllers. This driver adds support for the USB
> internal mux, and all consumers of this mux can use interfaces provided
> by mux subsytem to control the state of the internal mux.

> +config MUX_INTEL_USB
> +       tristate "Intel USB Mux"

It's indeed Intel's IP? I would rather believe that it is some 3rd
party known IP block with platform specific soldering.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

Hi Peter,

Thanks for your comments.

On 05/30/2017 06:40 AM, Peter Rosin wrote:
> On 2017-05-30 02:47, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> In some Intel SOCs, a single USB port is shared between USB device and
>> host controller and an internal mux is used to control the selection of
>> port by host/device controllers. This driver adds support for the USB
>> internal mux, and all consumers of this mux can use interfaces provided
>> by mux subsytem to control the state of the internal mux.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Hi!
>
> A few things make me curious...
>
> 1. How are platform devices w/o any match table probed? Do you have to
>     manually load them, or what?
Yes, for now, I am manually creating this device from dwc3 driver to 
test it. But in future I am planning to get ACPI ID for this device to 
probe it from BIOS.
>
> 2. What are these "all the consumers of this mux" that you mention,
Currently the only consumer for this mux device is, Broxton USB PHY 
driver. Its not yet upstreamed. I hoping to get this driver merged first 
before submitting the other driver for review.
>   and how
>     do they find the correct mux control to interact with?
Your current mux_control_get() API has tight dependency on device tree 
node and hence we can't use it for this use case.

So I am planning to add a new API to get the mux-control based on 
mux-device name. API interface looks some thing like below. I haven't 
finalized the patch yet. I will send it you for review in next few days. 
Let me know if you agree with this idea.

struct mux_chip *devm_mux_chip_get_by_index(struct device *dev,
                 const char *parent_name, unsigned int index)

struct mux_control *devm_mux_control_get_by_index(struct device *dev,
                 struct mux_chip *mux_chip, unsigned int index)
>
>> ---
>>   drivers/mux/Kconfig               |  13 ++++
>>   drivers/mux/Makefile              |   1 +
>>   drivers/mux/mux-intel-usb.c       | 132 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/mux/mux-intel-usb.h |  22 +++++++
>>   4 files changed, 168 insertions(+)
>>   create mode 100644 drivers/mux/mux-intel-usb.c
>>   create mode 100644 include/linux/mux/mux-intel-usb.h
>>
>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>> index e8f1df7..0e3af09 100644
>> --- a/drivers/mux/Kconfig
>> +++ b/drivers/mux/Kconfig
>> @@ -56,4 +56,17 @@ config MUX_MMIO
>>   	  To compile the driver as a module, choose M here: the module will
>>   	  be called mux-mmio.
>>   
>> +config MUX_INTEL_USB
>> +	tristate "Intel USB Mux"
>> +	depends on USB
> Why depend on USB?
This device register mapping comes from DWC3 USB host controller. So I 
thought there is no point in enabling it, if USB is disabled.
>
>> +	help
>> +	  In some Intel SOCs, a single USB port is shared between USB
>> +	  device and host controller and an internal mux is used to
>> +	  control the selection of port by host/device controllers. This
>> +	  driver adds support for the USB internal mux, and all consumers
>> +	  of this mux can use interfaces provided by mux subsytem to control
>> +	  the state of the internal mux.
>> +
>> +	  To compile the driver as a module, choose M here.
>> +
>>   endif
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> index 6bac5b0..9154616 100644
>> --- a/drivers/mux/Makefile
>> +++ b/drivers/mux/Makefile
>> @@ -6,3 +6,4 @@ obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>>   obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>>   obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
>>   obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
>> +obj-$(CONFIG_MUX_INTEL_USB)	+= mux-intel-usb.o
> Alphabetical order, please.
I will fix it in next version.
>
>> diff --git a/drivers/mux/mux-intel-usb.c b/drivers/mux/mux-intel-usb.c
>> new file mode 100644
>> index 0000000..587e9bb
>> --- /dev/null
>> +++ b/drivers/mux/mux-intel-usb.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * Intel USB Multiplexer Driver
>> + *
>> + * Copyright (C) 2017 Intel Corporation
>> + *
>> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
>> + *
>> + * This driver is written based on extcon-intel-usb driver submitted by
>> + * 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/err.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/driver.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/mux/mux-intel-usb.h>
>> +#include <linux/io.h>
> Alphabetical order, please.
ok. I will fix it in next version.
>
>> +
>> +#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 mux_intel_usb {
>> +	struct device *dev;
>> +	void __iomem *regs;
>> +};
>> +
>> +static int mux_intel_usb_set(struct mux_control *mux_ctrl, int state)
>> +{
>> +	struct mux_intel_usb *mux = mux_chip_priv(mux_ctrl->chip);
>> +	u32 val;
>> +
>> +	dev_info(mux->dev, "Intel USB mux set called");
> Isn't this very spammy?
>
> Cheers,
> peda
yes.  Added it for debug/testing purpose. Will remove it.
>
>> +
>> +	switch (state) {
>> +	case INTEL_USB_MUX_STATE_HOST:
>> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
>> +		break;
>> +	case INTEL_USB_MUX_STATE_DEVICE:
>> +		val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
>> +		      CFG0_SW_DRD_STATIC_DEV;
>> +		break;
>> +	default:
>> +		return 0;
>> +	};
>> +
>> +	writel(val, mux->regs);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mux_control_ops mux_intel_usb_ops = {
>> +	.set = mux_intel_usb_set,
>> +};
>> +
>> +static int mux_intel_usb_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct mux_chip *mux_chip;
>> +	struct mux_intel_usb *mux;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux));
>> +	if (IS_ERR(mux_chip))
>> +		return PTR_ERR(mux_chip);
>> +
>> +	mux = mux_chip_priv(mux_chip);
>> +	mux->dev = dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "Failed to get mem resource\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	mux->regs = devm_ioremap_nocache(dev, res->start, resource_size(res));
>> +	if (!mux->regs) {
>> +		dev_err(mux->dev, "mux regs io remap failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	mux_chip->ops = &mux_intel_usb_ops;
>> +	mux_chip->mux->states = INTEL_USB_MUX_STATE_MAX;
>> +	mux_chip->mux->idle_state = MUX_IDLE_AS_IS;
>> +
>> +	ret = devm_mux_chip_register(dev, mux_chip);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	dev_info(dev, "%u-way mux-controller registered\n",
>> +		 mux_chip->mux->states);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id mux_intel_usb_id[] = {
>> +        { .name = "intel-usb-mux", },
>> +        { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, mux_intel_usb_id);
>> +
>> +static struct platform_driver mux_intel_usb_driver = {
>> +	.driver = {
>> +		.name = "intel-usb-mux",
>> +	},
>> +	.probe = mux_intel_usb_probe,
>> +	.id_table = mux_intel_usb_id,
>> +};
>> +module_platform_driver(mux_intel_usb_driver);
>> +
>> +MODULE_DESCRIPTION("Intel USB multiplexer driver");
>> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mux/mux-intel-usb.h b/include/linux/mux/mux-intel-usb.h
>> new file mode 100644
>> index 0000000..e77516c
>> --- /dev/null
>> +++ b/include/linux/mux/mux-intel-usb.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Intel USB multiplexer header file
>> + *
>> + * Copyright (C) 2017 Intel Corporation
>> + *
>> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@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.
>> + */
>> +
>> +#ifndef _LINUX_MUX_MUX_INTEL_USB_H
>> +#define _LINUX_MUX_MUX_INTEL_USB_H
>> +
>> +#include <linux/mux/consumer.h>
>> +
>> +#define INTEL_USB_MUX_STATE_HOST		0
>> +#define INTEL_USB_MUX_STATE_DEVICE		1
>> +#define INTEL_USB_MUX_STATE_MAX			2
>> +
>> +#endif /* _LINUX_MUX_MUX_INTEL_USB_H */
>>
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

Hi Andy,


On 05/30/2017 09:20 AM, Andy Shevchenko wrote:
> On Tue, May 30, 2017 at 3:47 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> In some Intel SOCs, a single USB port is shared between USB device and
> SoCs
>
>> host controller and an internal mux is used to control the selection of
>> port by host/device controllers. This driver adds support for the USB
>> internal mux, and all consumers of this mux can use interfaces provided
>> by mux subsytem to control the state of the internal mux.
>> +config MUX_INTEL_USB
>> +       tristate "Intel USB Mux"
> It's indeed Intel's IP?
Register map to control this MUX comes from Intel vendor defined XHCI 
extended cap region of SOC.
> I would rather believe that it is some 3rd
> party known IP block with platform specific soldering.
I don't think its platform specific support. I believe its a SOC 
specific thing( mainly for CHT and APL SoCs).
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Andy Shevchenko]

On Tue, May 30, 2017 at 9:21 PM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> On Tue, May 30, 2017 at 3:47 AM,
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

>>> +       tristate "Intel USB Mux"
>>
>> It's indeed Intel's IP?
>
> Register map to control this MUX comes from Intel vendor defined XHCI
> extended cap region of SOC.
>>
>> I would rather believe that it is some 3rd
>> party known IP block with platform specific soldering.
>
> I don't think its platform specific support. I believe its a SOC specific
> thing( mainly for CHT and APL SoCs).

Okay, the best people to give a feedback here are Heikki and Hans.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Peter Rosin]

On 2017-05-30 19:47, sathyanarayanan kuppuswamy wrote:
> Hi Peter,
> 
> Thanks for your comments.
> 
> On 05/30/2017 06:40 AM, Peter Rosin wrote:
>> On 2017-05-30 02:47, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>
>>> In some Intel SOCs, a single USB port is shared between USB device and
>>> host controller and an internal mux is used to control the selection of
>>> port by host/device controllers. This driver adds support for the USB
>>> internal mux, and all consumers of this mux can use interfaces provided
>>> by mux subsytem to control the state of the internal mux.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Hi!
>>
>> A few things make me curious...
>>
>> 1. How are platform devices w/o any match table probed? Do you have to
>>     manually load them, or what?
> Yes, for now, I am manually creating this device from dwc3 driver to 
> test it. But in future I am planning to get ACPI ID for this device to 
> probe it from BIOS.
>>
>> 2. What are these "all the consumers of this mux" that you mention,
> Currently the only consumer for this mux device is, Broxton USB PHY 
> driver. Its not yet upstreamed. I hoping to get this driver merged first 
> before submitting the other driver for review.

Is any other consumer in the charts at all? Can this existing consumer
ever make use of some other mux? If the answer to both those questions
are 'no', then I do not see much point in involving the mux subsystem at
all. The Broxton USB PHY driver could just as well write to the register
all by itself, no?

>>   and how
>>     do they find the correct mux control to interact with?
> Your current mux_control_get() API has tight dependency on device tree 
> node and hence we can't use it for this use case.

Yes, that was all I needed, so far. Trying to keep things simple...

> So I am planning to add a new API to get the mux-control based on 
> mux-device name. API interface looks some thing like below. I haven't 
> finalized the patch yet. I will send it you for review in next few days. 
> Let me know if you agree with this idea.
> 
> struct mux_chip *devm_mux_chip_get_by_index(struct device *dev,
>                  const char *parent_name, unsigned int index)
> 
> struct mux_control *devm_mux_control_get_by_index(struct device *dev,
>                  struct mux_chip *mux_chip, unsigned int index)

I don't like exposing struct mux_chip to clients.

My lose plan was to try to dig into the device name if the of_node is
not present, and thus overload the mux_control_get() api. Not sure about
how to handle non-DT muxes with more than one mux-control though, maybe
add an index argument that is ignored when it's not needed? But you
don't really need the index either, so that can wait...

Cheers,
peda

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Hans de Goede]

Hi,

On 30-05-17 20:50, Andy Shevchenko wrote:
> On Tue, May 30, 2017 at 9:21 PM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>> On Tue, May 30, 2017 at 3:47 AM,
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>>>> +       tristate "Intel USB Mux"
>>>
>>> It's indeed Intel's IP?
>>
>> Register map to control this MUX comes from Intel vendor defined XHCI
>> extended cap region of SOC.
>>>
>>> I would rather believe that it is some 3rd
>>> party known IP block with platform specific soldering.
>>
>> I don't think its platform specific support. I believe its a SOC specific
>> thing( mainly for CHT and APL SoCs).
> 
> Okay, the best people to give a feedback here are Heikki and Hans.

Interesting, I have been working on this myself too (and posted
a version of my driver for the mux block a while back), see:

https://www.spinics.net/lists/linux-usb/msg151032.html
https://www.spinics.net/lists/linux-usb/msg151033.html
https://www.spinics.net/lists/linux-usb/msg151034.html

I was actually planning on posting a new version of this set
soon. I still need to to modify the first patch to trigger
bu PCI-ids to avoid registering non existing mux platform
device on non CHT / APL Intel platforms.

I've since changed the second patch into a platform driver,
see:

https://github.com/jwrdegoede/linux-sunxi/commit/e51057609102c35dd35b4a5965139aff81fbefb8

Note that this patch has 2 differences compared to
sathyanarayanan's patch:

1) It ties directly into the extcon framework and responds
to extcon events instead of using the mux framework,
actually this is the first time I hear about a mux framework
at all. Is there a git tree with the patches for this somewhere ?

2) It controls the ID and VBUS bits separately, this is
important because on some Cherry Trail devices the ID bit
is automatically set be ACPI code (through a gpio irq
event handler), but the ACPI code does not update the
VBUS bit causing the otg port on these devices to not
work in device mode.

2. also means that this no longer really is a mux driver ...

I've run a whole lot of tests with these patches on
various Cherry Trail devices using either ACPI code
to set the ID pin or the already merged
drivers/extcon/extcon-intel-int3496.c
driver for the tablets where the ACPI code defines
a INT3496 ACPI device instead of handling the id
pin / bit itself.

And this driver works well in both scenarios. Recently
I've become aware of one potential problem, which is
integrating this with devices which have a USB-C
connector. I've one CHT device with a USB-C connector
and a FUSB302 USB controller, in that case we need
the mux driver to react to data / changes from the
FUSB302 driver. One way to do this would be for the
FUSB302 driver to also register an extcon driver.

But there still is a lot of work TBD wrt getting USB-C
to work on these devices (where it is not all handled
in firmware like it is on regular x86 laptops). So it
might be best to just move forward with my version of
this driver for now, which at least makes the micro-usb
connector found on many of these devices work properly
in both host and device mode all the time.

Regards,

Hans

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Peter Rosin]

On 2017-05-31 14:21, Hans de Goede wrote:
> actually this is the first time I hear about a mux framework
> at all. Is there a git tree with the patches for this somewhere ?

https://gitlab.com/peda-linux/mux.git in the "mux" branch.

Series posted here:
https://lkml.org/lkml/2017/5/14/160

Cheers,
peda

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Hans de Goede]

Hi,

On 31-05-17 15:05, Peter Rosin wrote:
> On 2017-05-31 14:21, Hans de Goede wrote:
>> actually this is the first time I hear about a mux framework
>> at all. Is there a git tree with the patches for this somewhere ?
> 
> https://gitlab.com/peda-linux/mux.git in the "mux" branch.
> 
> Series posted here:
> https://lkml.org/lkml/2017/5/14/160

Thank you.

I see that mux_control_get() currently relies on devicetree describing
the mux, that is not going to work on non devicetree platforms like
x86 where the relation typically is not described ad all (*) ?

Typically there would be a global list of mux_controls maintained
by mux_[de]register and then mux_control_get() would walk this list
until it finds a matching name. The names to register would then be
passed in by platform data/code when registering and likewise the
consumer would be passed a unique name to pass into mux_control_get()
through platform data / code, would that work for you ?

Note one option would be to set the names to use when registering
a mux chip through device_properties, this is what the power-supply
subsys is currently doing more or less.

Regards,

Hans




*) Work is under way to allow describing topologies in ACPI but it is
not in the standard yet, and we also need to deal with already existing
devices.

> 
> Cheers,
> peda
> 

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Peter Rosin]

On 2017-05-31 16:18, Hans de Goede wrote:
> Hi,
> 
> On 31-05-17 15:05, Peter Rosin wrote:
>> On 2017-05-31 14:21, Hans de Goede wrote:
>>> actually this is the first time I hear about a mux framework
>>> at all. Is there a git tree with the patches for this somewhere ?
>>
>> https://gitlab.com/peda-linux/mux.git in the "mux" branch.
>>
>> Series posted here:
>> https://lkml.org/lkml/2017/5/14/160
> 
> Thank you.
> 
> I see that mux_control_get() currently relies on devicetree describing
> the mux, that is not going to work on non devicetree platforms like
> x86 where the relation typically is not described ad all (*) ?

Yes, I'm aware of this. I wanted to keep things simple. Also, see
my reply on the other branch of this discussion.

https://lkml.org/lkml/2017/5/31/58

> Typically there would be a global list of mux_controls maintained
> by mux_[de]register and then mux_control_get() would walk this list
> until it finds a matching name. The names to register would then be
> passed in by platform data/code when registering and likewise the
> consumer would be passed a unique name to pass into mux_control_get()
> through platform data / code, would that work for you ?
> 
> Note one option would be to set the names to use when registering
> a mux chip through device_properties, this is what the power-supply
> subsys is currently doing more or less.

I had this lose plan to match by the struct device name, but if that
is not working the above seems fine too...

Cheers,
peda

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Hans de Goede]

Hi,

On 31-05-17 17:30, Peter Rosin wrote:
> On 2017-05-31 16:18, Hans de Goede wrote:
>> Hi,
>>
>> On 31-05-17 15:05, Peter Rosin wrote:
>>> On 2017-05-31 14:21, Hans de Goede wrote:
>>>> actually this is the first time I hear about a mux framework
>>>> at all. Is there a git tree with the patches for this somewhere ?
>>>
>>> https://gitlab.com/peda-linux/mux.git in the "mux" branch.
>>>
>>> Series posted here:
>>> https://lkml.org/lkml/2017/5/14/160
>>
>> Thank you.
>>
>> I see that mux_control_get() currently relies on devicetree describing
>> the mux, that is not going to work on non devicetree platforms like
>> x86 where the relation typically is not described ad all (*) ?
> 
> Yes, I'm aware of this.

Ok, good.

> I wanted to keep things simple. Also, see
> my reply on the other branch of this discussion.
> 
> https://lkml.org/lkml/2017/5/31/58
> 
>> Typically there would be a global list of mux_controls maintained
>> by mux_[de]register and then mux_control_get() would walk this list
>> until it finds a matching name. The names to register would then be
>> passed in by platform data/code when registering and likewise the
>> consumer would be passed a unique name to pass into mux_control_get()
>> through platform data / code, would that work for you ?
>>
>> Note one option would be to set the names to use when registering
>> a mux chip through device_properties, this is what the power-supply
>> subsys is currently doing more or less.
> 
> I had this lose plan to match by the struct device name, but if that
> is not working the above seems fine too...

There are 2 problems with using dev_name() :

1) dev_name() will be coupled to the mux_chip and one mux_chip may
have multiple controllers (you could add a number postfix to work
around this)

2) The caller of mux_control_get() wants a fixed name, where as
dev_name is not always fixed, e.g. with mux-chips which communicate
via i2c it is something like 7-0064 where 7 is the i2c bus-number
which depends on probe ordering and thus may not even be the same
every boot.

Regards,

Hans

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

Hi Hans,


On 05/31/2017 05:21 AM, Hans de Goede wrote:
> Hi,
>
> On 30-05-17 20:50, Andy Shevchenko wrote:
>> On Tue, May 30, 2017 at 9:21 PM, sathyanarayanan kuppuswamy
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>> On Tue, May 30, 2017 at 3:47 AM,
>>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>>>>> +       tristate "Intel USB Mux"
>>>>
>>>> It's indeed Intel's IP?
>>>
>>> Register map to control this MUX comes from Intel vendor defined XHCI
>>> extended cap region of SOC.
>>>>
>>>> I would rather believe that it is some 3rd
>>>> party known IP block with platform specific soldering.
>>>
>>> I don't think its platform specific support. I believe its a SOC 
>>> specific
>>> thing( mainly for CHT and APL SoCs).
>>
>> Okay, the best people to give a feedback here are Heikki and Hans.
>
> Interesting, I have been working on this myself too (and posted
> a version of my driver for the mux block a while back), see:
>
> https://www.spinics.net/lists/linux-usb/msg151032.html
> https://www.spinics.net/lists/linux-usb/msg151033.html
> https://www.spinics.net/lists/linux-usb/msg151034.html
>
> I was actually planning on posting a new version of this set
> soon. I still need to to modify the first patch to trigger
> bu PCI-ids to avoid registering non existing mux platform
> device on non CHT / APL Intel platforms.
>
> I've since changed the second patch into a platform driver,
> see:
>
> https://github.com/jwrdegoede/linux-sunxi/commit/e51057609102c35dd35b4a5965139aff81fbefb8 
>
>
> Note that this patch has 2 differences compared to
> sathyanarayanan's patch:
>
> 1) It ties directly into the extcon framework and responds
> to extcon events instead of using the mux framework,
What about devices with no ID events ? At least the APL device that I am
using does not have external IRQ for VBUS and ID.
> actually this is the first time I hear about a mux framework
> at all. Is there a git tree with the patches for this somewhere ?
>
> 2) It controls the ID and VBUS bits separately, this is
> important because on some Cherry Trail devices the ID bit
> is automatically set be ACPI code (through a gpio irq
> event handler), but the ACPI code does not update the
> VBUS bit causing the otg port on these devices to not
> work in device mode.
Even if you listen to ID and VBUS events separately, I think the end
result is selecting either SW host or SW device mode right ? Then
why not abstract MUX functionality outside the your driver and
control the MUX from your driver appropriately ? or do we get
any advantage is modifying just VBUS_VALID or SW_IDPIN bits
separately ?

>
> 2. also means that this no longer really is a mux driver ...
> I've run a whole lot of tests with these patches on
> various Cherry Trail devices using either ACPI code
> to set the ID pin or the already merged
> drivers/extcon/extcon-intel-int3496.c
> driver for the tablets where the ACPI code defines
> a INT3496 ACPI device instead of handling the id
> pin / bit itself.
>
> And this driver works well in both scenarios. Recently
> I've become aware of one potential problem, which is
> integrating this with devices which have a USB-C
> connector. I've one CHT device with a USB-C connector
> and a FUSB302 USB controller, in that case we need
> the mux driver to react to data / changes from the
> FUSB302 driver. One way to do this would be for the
> FUSB302 driver to also register an extcon driver.
>
> But there still is a lot of work TBD wrt getting USB-C
> to work on these devices (where it is not all handled
> in firmware like it is on regular x86 laptops). So it
> might be best to just move forward with my version of
> this driver for now, which at least makes the micro-usb
> connector found on many of these devices work properly
> in both host and device mode all the time.
>
> Regards,
>
> Hans
>
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

On 05/31/2017 08:30 AM, Peter Rosin wrote:
> On 2017-05-31 16:18, Hans de Goede wrote:
>> Hi,
>>
>> On 31-05-17 15:05, Peter Rosin wrote:
>>> On 2017-05-31 14:21, Hans de Goede wrote:
>>>> actually this is the first time I hear about a mux framework
>>>> at all. Is there a git tree with the patches for this somewhere ?
>>> https://gitlab.com/peda-linux/mux.git in the "mux" branch.
>>>
>>> Series posted here:
>>> https://lkml.org/lkml/2017/5/14/160
>> Thank you.
>>
>> I see that mux_control_get() currently relies on devicetree describing
>> the mux, that is not going to work on non devicetree platforms like
>> x86 where the relation typically is not described ad all (*) ?
> Yes, I'm aware of this. I wanted to keep things simple. Also, see
> my reply on the other branch of this discussion.
>
> https://lkml.org/lkml/2017/5/31/58
>
>> Typically there would be a global list of mux_controls maintained
>> by mux_[de]register and then mux_control_get() would walk this list
>> until it finds a matching name. The names to register would then be
>> passed in by platform data/code when registering and likewise the
>> consumer would be passed a unique name to pass into mux_control_get()
>> through platform data / code, would that work for you ?
>>
>> Note one option would be to set the names to use when registering
>> a mux chip through device_properties, this is what the power-supply
>> subsys is currently doing more or less.
> I had this lose plan to match by the struct device name, but if that
> is not working the above seems fine too...
By device name do you mean mux chip device name or the mux platform 
device name ?

If you mean former, since you are using ID framework, mux chip device 
name changes
dynamically. So we can't use a static name to identify this device from 
other drivers.
>
> Cheers,
> peda
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

Hi,


On 05/31/2017 11:27 AM, Hans de Goede wrote:
> Hi,
>
> On 31-05-17 17:30, Peter Rosin wrote:
>> On 2017-05-31 16:18, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 31-05-17 15:05, Peter Rosin wrote:
>>>> On 2017-05-31 14:21, Hans de Goede wrote:
>>>>> actually this is the first time I hear about a mux framework
>>>>> at all. Is there a git tree with the patches for this somewhere ?
>>>>
>>>> https://gitlab.com/peda-linux/mux.git in the "mux" branch.
>>>>
>>>> Series posted here:
>>>> https://lkml.org/lkml/2017/5/14/160
>>>
>>> Thank you.
>>>
>>> I see that mux_control_get() currently relies on devicetree describing
>>> the mux, that is not going to work on non devicetree platforms like
>>> x86 where the relation typically is not described ad all (*) ?
>>
>> Yes, I'm aware of this.
>
> Ok, good.
>
>> I wanted to keep things simple. Also, see
>> my reply on the other branch of this discussion.
>>
>> https://lkml.org/lkml/2017/5/31/58
>>
>>> Typically there would be a global list of mux_controls maintained
>>> by mux_[de]register and then mux_control_get() would walk this list
>>> until it finds a matching name. The names to register would then be
>>> passed in by platform data/code when registering and likewise the
>>> consumer would be passed a unique name to pass into mux_control_get()
>>> through platform data / code, would that work for you ?
>>>
>>> Note one option would be to set the names to use when registering
>>> a mux chip through device_properties, this is what the power-supply
>>> subsys is currently doing more or less.
>>
>> I had this lose plan to match by the struct device name, but if that
>> is not working the above seems fine too...
>
> There are 2 problems with using dev_name() :
>
> 1) dev_name() will be coupled to the mux_chip and one mux_chip may
> have multiple controllers (you could add a number postfix to work
> around this)
I think maintaining unique mux control names across all mux devices will be
bit difficult to maintain. Unless its auto created as Hans mentioned using
device name + chip index  + control index.

or , we can have unique names for mux-chips and get mux-control handles 
based
on index numbers. This should at least reduce some of the search time.

mux_control_get("mux chip name",  index)
>
>
> 2) The caller of mux_control_get() wants a fixed name, where as
> dev_name is not always fixed, e.g. with mux-chips which communicate
> via i2c it is something like 7-0064 where 7 is the i2c bus-number
> which depends on probe ordering and thus may not even be the same
> every boot.
>
> Regards,
>
> Hans
>
>
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

Hi,


On 05/30/2017 11:29 PM, Peter Rosin wrote:
> On 2017-05-30 19:47, sathyanarayanan kuppuswamy wrote:
>> Hi Peter,
>>
>> Thanks for your comments.
>>
>> On 05/30/2017 06:40 AM, Peter Rosin wrote:
>>> On 2017-05-30 02:47, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>
>>>> In some Intel SOCs, a single USB port is shared between USB device and
>>>> host controller and an internal mux is used to control the selection of
>>>> port by host/device controllers. This driver adds support for the USB
>>>> internal mux, and all consumers of this mux can use interfaces provided
>>>> by mux subsytem to control the state of the internal mux.
>>>>
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> Hi!
>>>
>>> A few things make me curious...
>>>
>>> 1. How are platform devices w/o any match table probed? Do you have to
>>>      manually load them, or what?
>> Yes, for now, I am manually creating this device from dwc3 driver to
>> test it. But in future I am planning to get ACPI ID for this device to
>> probe it from BIOS.
>>> 2. What are these "all the consumers of this mux" that you mention,
>> Currently the only consumer for this mux device is, Broxton USB PHY
>> driver. Its not yet upstreamed. I hoping to get this driver merged first
>> before submitting the other driver for review.
> Is any other consumer in the charts at all? Can this existing consumer
> ever make use of some other mux? If the answer to both those questions
> are 'no', then I do not see much point in involving the mux subsystem at
> all. The Broxton USB PHY driver could just as well write to the register
> all by itself, no?
>
>>>    and how
>>>      do they find the correct mux control to interact with?
>> Your current mux_control_get() API has tight dependency on device tree
>> node and hence we can't use it for this use case.
> Yes, that was all I needed, so far. Trying to keep things simple...
>
>> So I am planning to add a new API to get the mux-control based on
>> mux-device name. API interface looks some thing like below. I haven't
>> finalized the patch yet. I will send it you for review in next few days.
>> Let me know if you agree with this idea.
>>
>> struct mux_chip *devm_mux_chip_get_by_index(struct device *dev,
>>                   const char *parent_name, unsigned int index)
>>
>> struct mux_control *devm_mux_control_get_by_index(struct device *dev,
>>                   struct mux_chip *mux_chip, unsigned int index)
> I don't like exposing struct mux_chip to clients.
>
> My lose plan was to try to dig into the device name if the of_node is
> not present, and thus overload the mux_control_get() api.
I think this would make the implementation bit complex.

Why not maintain uniformity ? Like searching for mux control based on
device name or using unique chip name + index. This would work for both
DT and non DT cases.
>   Not sure about
> how to handle non-DT muxes with more than one mux-control though, maybe
> add an index argument that is ignored when it's not needed? But you
> don't really need the index either, so that can wait...
>
> Cheers,
> peda
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Hans de Goede]

HI,

On 01-06-17 01:12, sathyanarayanan kuppuswamy wrote:
> Hi Hans,
> 
> 
> On 05/31/2017 05:21 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 30-05-17 20:50, Andy Shevchenko wrote:
>>> On Tue, May 30, 2017 at 9:21 PM, sathyanarayanan kuppuswamy
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>> On Tue, May 30, 2017 at 3:47 AM,
>>>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>
>>>>>> +       tristate "Intel USB Mux"
>>>>>
>>>>> It's indeed Intel's IP?
>>>>
>>>> Register map to control this MUX comes from Intel vendor defined XHCI
>>>> extended cap region of SOC.
>>>>>
>>>>> I would rather believe that it is some 3rd
>>>>> party known IP block with platform specific soldering.
>>>>
>>>> I don't think its platform specific support. I believe its a SOC specific
>>>> thing( mainly for CHT and APL SoCs).
>>>
>>> Okay, the best people to give a feedback here are Heikki and Hans.
>>
>> Interesting, I have been working on this myself too (and posted
>> a version of my driver for the mux block a while back), see:
>>
>> https://www.spinics.net/lists/linux-usb/msg151032.html
>> https://www.spinics.net/lists/linux-usb/msg151033.html
>> https://www.spinics.net/lists/linux-usb/msg151034.html
>>
>> I was actually planning on posting a new version of this set
>> soon. I still need to to modify the first patch to trigger
>> bu PCI-ids to avoid registering non existing mux platform
>> device on non CHT / APL Intel platforms.
>>
>> I've since changed the second patch into a platform driver,
>> see:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/e51057609102c35dd35b4a5965139aff81fbefb8
>>
>> Note that this patch has 2 differences compared to
>> sathyanarayanan's patch:
>>
>> 1) It ties directly into the extcon framework and responds
>> to extcon events instead of using the mux framework,
> What about devices with no ID events ? At least the APL device that I am
> using does not have external IRQ for VBUS and ID.
>> actually this is the first time I hear about a mux framework
>> at all. Is there a git tree with the patches for this somewhere ?
>>
>> 2) It controls the ID and VBUS bits separately, this is
>> important because on some Cherry Trail devices the ID bit
>> is automatically set be ACPI code (through a gpio irq
>> event handler), but the ACPI code does not update the
>> VBUS bit causing the otg port on these devices to not
>> work in device mode.
> Even if you listen to ID and VBUS events separately, I think the end
> result is selecting either SW host or SW device mode right ?

No, the ID bit causes the mux to get thrown either way, the
VBUS bit causes the device controller to see a connect or
not.

> Then why not abstract MUX functionality outside the your driver and
> control the MUX from your driver appropriately ? or do we get
> any advantage is modifying just VBUS_VALID or SW_IDPIN bits
> separately ?

They MUST be controlled separately because on some systems
the SW_IDPIN bit is controller by ACPI AML code, where as
the VBUS_VALID needs to be set by the kernel.

We could model the VBUS_VALID bit as a separate mux_controller,
that would be a bit weird though ...

Regards,

Hans