Re: [PATCH 5/7] usb: misc: add driver for Intel drcfg controlled port mux

[Lu Baolu]

On 03/04/2016 12:14 AM, Greg Kroah-Hartman wrote:
> On Thu, Mar 03, 2016 at 02:37:42PM +0800, Lu Baolu wrote:
>> Several Intel PCHs and SOCs have an internal mux that is used to
>> share one USB port between device controller and host controller.
>> The mux is handled through the Dual Role Configuration Register.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
>> Reviewed-by: Felipe Balbi <balbi@xxxxxxxxxx>
>> ---
>>  MAINTAINERS                        |   1 +
>>  drivers/usb/misc/Kconfig           |   7 ++
>>  drivers/usb/misc/Makefile          |   1 +
>>  drivers/usb/misc/intel-mux-drcfg.c | 174 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 183 insertions(+)
>>  create mode 100644 drivers/usb/misc/intel-mux-drcfg.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0f321e4..20eb873 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11396,6 +11396,7 @@ S:	Supported
>>  F:	drivers/usb/misc/mux.c
>>  F:	include/linux/usb/mux.h
>>  F:	drivers/usb/misc/intel-mux-gpio.c
>> +F:	drivers/usb/misc/intel-mux-drcfg.c
>>  
>>  USB PRINTER DRIVER (usblp)
>>  M:	Pete Zaitcev <zaitcev@xxxxxxxxxx>
>> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
>> index 33e6386..befd910 100644
>> --- a/drivers/usb/misc/Kconfig
>> +++ b/drivers/usb/misc/Kconfig
>> @@ -281,3 +281,10 @@ config INTEL_MUX_GPIO
>>  	help
>>  	  Say Y here to enable support for Intel dual role port mux
>>  	  controlled by GPIOs.
>> +
>> +config INTEL_MUX_DRCFG
>> +	tristate "Intel dual role port mux controlled by register"
>> +	select USB_MUX
>> +	help
>> +	  Say Y here to enable support for Intel dual role port mux
>> +	  controlled by the Dual Role Configuration Registers.
>> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
>> index da4fb4e..c4d19a0 100644
>> --- a/drivers/usb/misc/Makefile
>> +++ b/drivers/usb/misc/Makefile
>> @@ -32,3 +32,4 @@ obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
>>  
>>  obj-$(CONFIG_USB_MUX)			+= mux.o
>>  obj-$(CONFIG_INTEL_MUX_GPIO)		+= intel-mux-gpio.o
>> +obj-$(CONFIG_INTEL_MUX_DRCFG)		+= intel-mux-drcfg.o
>> diff --git a/drivers/usb/misc/intel-mux-drcfg.c b/drivers/usb/misc/intel-mux-drcfg.c
>> new file mode 100644
>> index 0000000..29081c5
>> --- /dev/null
>> +++ b/drivers/usb/misc/intel-mux-drcfg.c
>> @@ -0,0 +1,174 @@
>> +/**
>> + * intel-mux-drcfg.c - Driver for Intel USB mux via register
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + * Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>> + * Author: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> + *
>> + * 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/module.h>
>> +#include <linux/property.h>
>> +#include <linux/usb/mux.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define INTEL_MUX_CFG0		0x00
>> +#define INTEL_MUX_CFG1		0x04
>> +#define CFG0_SW_IDPIN		BIT(20)
>> +#define CFG0_SW_IDPIN_EN	BIT(21)
>> +#define CFG0_SW_VBUS_VALID	BIT(24)
>> +#define CFG1_SW_MODE		BIT(29)
>> +#define CFG1_POLL_TIMEOUT	1000
>> +
>> +struct intel_usb_mux {
>> +	struct usb_mux_dev umdev;
>> +	void __iomem *regs;
>> +	u32 cfg0_ctx;
>> +};
>> +
>> +static inline int intel_mux_drcfg_switch(struct usb_mux_dev *umdev, bool host)
>> +{
>> +	struct intel_usb_mux *mux;
>> +	unsigned long timeout;
>> +	u32 data;
>> +
>> +	mux = container_of(umdev, struct intel_usb_mux, umdev);
>> +
>> +	/* Check and set mux to SW controlled mode */
>> +	data = readl(mux->regs + INTEL_MUX_CFG0);
>> +	if (!(data & CFG0_SW_IDPIN_EN)) {
>> +		data |= CFG0_SW_IDPIN_EN;
>> +		writel(data, mux->regs + INTEL_MUX_CFG0);
>> +	}
>> +
>> +	/*
>> +	 * Configure CFG0 to switch the mux and VBUS_VALID bit is
>> +	 * required for device mode.
>> +	 */
>> +	data = readl(mux->regs + INTEL_MUX_CFG0);
>> +	if (host)
>> +		data &= ~(CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID);
>> +	else
>> +		data |= (CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID);
>> +	writel(data, mux->regs + INTEL_MUX_CFG0);
>> +
>> +	/*
>> +	 * Polling CFG1 for safety, most case it takes about 600ms
>> +	 * to finish mode switching, set TIMEOUT long enough.
>> +	 */
>> +	timeout = jiffies + msecs_to_jiffies(CFG1_POLL_TIMEOUT);
>> +
>> +	/* Polling on CFG1 register to confirm mode switch. */
>> +	while (!time_after(jiffies, timeout)) {
>> +		data = readl(mux->regs + INTEL_MUX_CFG1);
>> +		if (!(host ^ (data & CFG1_SW_MODE)))
>> +			return 0;
>> +		/* interval for polling is set to about 5ms */
>> +		usleep_range(5000, 5100);
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int intel_mux_drcfg_cable_set(struct usb_mux_dev *umdev)
>> +{
>> +	dev_dbg(umdev->dev, "drcfg mux switch to HOST\n");
>> +
>> +	return intel_mux_drcfg_switch(umdev, true);
>> +}
>> +
>> +static int intel_mux_drcfg_cable_unset(struct usb_mux_dev *umdev)
>> +{
>> +	dev_dbg(umdev->dev, "drcfg mux switch to DEVICE\n");
>> +
>> +	return intel_mux_drcfg_switch(umdev, false);
>> +}
>> +
>> +static int intel_mux_drcfg_probe(struct platform_device *pdev)
>> +{
>> +	struct intel_usb_mux *mux;
>> +	struct usb_mux_dev *umdev;
>> +	struct device *dev = &pdev->dev;
>> +	u64 start, size;
>> +	int ret;
>> +
>> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +	if (!mux)
>> +		return -ENOMEM;
>> +
>> +	ret = device_property_read_u64(dev, "reg-start", &start);
>> +	ret |= device_property_read_u64(dev, "reg-size", &size);
>> +	if (ret)
>> +		return -ENODEV;
> And you leaked memory :(

Did you mean device_property_read_*() allocated memory
and I need to release it before return error?

>
>> +
>> +	mux->regs = devm_ioremap_nocache(dev, start, size);
>> +	if (!mux->regs)
>> +		return -ENOMEM;
> Memory gone :(

devm interface will manage the resources by itself, or not?

>
>> +
>> +	mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
>> +
>> +	umdev = &mux->umdev;
>> +	umdev->dev = dev;
>> +	umdev->cable_name = "USB-HOST";
>> +	umdev->cable_set_cb = intel_mux_drcfg_cable_set;
>> +	umdev->cable_unset_cb = intel_mux_drcfg_cable_unset;
>> +
>> +	ret = usb_mux_register(umdev);
>> +	if (ret)
>> +		writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> So if an error happens you just keep going?

No. If error happens, I will restore the register and return error.

Thanks,
-Baolu