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

[sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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