Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753537AbdIDL3R (ORCPT ); Mon, 4 Sep 2017 07:29:17 -0400 Received: from mail.lysator.liu.se ([130.236.254.3]:35619 "EHLO mail.lysator.liu.se" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753410AbdIDL3Q (ORCPT ); Mon, 4 Sep 2017 07:29:16 -0400 X-Greylist: delayed 577 seconds by postgrey-1.27 at vger.kernel.org; Mon, 04 Sep 2017 07:29:15 EDT Subject: Re: [PATCH 05/11] mux: Add Intel Cherrytrail USB mux driver To: Hans de Goede , MyungJoo Ham , Chanwoo Choi , Guenter Roeck , Heikki Krogerus , Darren Hart , Andy Shevchenko , Mathias Nyman Cc: platform-driver-x86@vger.kernel.org, devel@driverdev.osuosl.org, Kuppuswamy Sathyanarayanan , Sathyanarayanan Kuppuswamy Natarajan , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , linux-usb@vger.kernel.org References: <20170901214845.7153-1-hdegoede@redhat.com> <20170901214845.7153-6-hdegoede@redhat.com> From: Peter Rosin Message-ID: Date: Mon, 4 Sep 2017 13:19:26 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170901214845.7153-6-hdegoede@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11946 Lines: 392 Hi! Some comments inline... On 2017-09-01 23:48, Hans de Goede wrote: > Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port > USB data lines between the xHCI host controller and the dwc3 gadget > controller. On some Cherrytrail systems this mux is controlled through > AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through > an _AIE ACPI method) so things just work. > > But on other Cherrytrail systems we need to control the mux ourselves > this driver exports the mux through the mux subsys, so that other drivers > can control it if necessary. > > This driver also updates the vbus-valid reporting to the dwc3 gadget > controller, as this uses the same registers as the mux. This is needed > for gadget/device mode to work properly (even on systems which control > the mux from their AML code). > > Note this depends on the xhci driver registering a platform device > named "intel_cht_usb_mux", which has an IOMEM resource 0 which points > to the Intel Vendor Defined XHCI extended capabilities region. > > Signed-off-by: Hans de Goede > --- > Changes in v2: > -Complete rewrite as a stand-alone platform-driver rather then as a phy > driver, since this is just a mux, not a phy > > Changes in v3: > -Make this a mux subsys driver instead of listening to USB_HOST extcon > cable events and responding to those > --- > drivers/mux/Kconfig | 11 ++ > drivers/mux/Makefile | 2 + > drivers/mux/intel_cht_usb_mux.c | 269 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 282 insertions(+) > create mode 100644 drivers/mux/intel_cht_usb_mux.c > > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig > index 19e4e904c9bf..17938918bf93 100644 > --- a/drivers/mux/Kconfig > +++ b/drivers/mux/Kconfig > @@ -34,6 +34,17 @@ config MUX_GPIO > To compile the driver as a module, choose M here: the module will > be called mux-gpio. > > +config MUX_CHT_USB_MUX > + tristate "Intel Cherrytrail USB Multiplexer" > + depends on ACPI && X86 && EXTCON > + help > + This driver adds support for the internal USB mux for muxing the OTG > + USB data lines between the xHCI host controller and the dwc3 gadget > + controller found on Intel Cherrytrail SoCs. > + > + To compile the driver as a module, choose M here: the module will > + be called mux-intel_cht_usb_mux. > + > config MUX_MMIO > tristate "MMIO register bitfield-controlled Multiplexer" > depends on (OF && MFD_SYSCON) || COMPILE_TEST > diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile > index 0e1e59760e3f..a12e812c7966 100644 > --- a/drivers/mux/Makefile > +++ b/drivers/mux/Makefile > @@ -6,8 +6,10 @@ mux-core-objs := core.o > mux-adg792a-objs := adg792a.o > mux-gpio-objs := gpio.o > mux-mmio-objs := mmio.o > +mux-intel_cht_usb_mux-objs := intel_cht_usb_mux.o I dislike underscores in file names (and other names), please use dashes where possible. Also, please keep the list sorted. > > 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_CHT_USB_MUX) += mux-intel_cht_usb_mux.o Dito. > diff --git a/drivers/mux/intel_cht_usb_mux.c b/drivers/mux/intel_cht_usb_mux.c > new file mode 100644 > index 000000000000..7b1621a081d8 > --- /dev/null > +++ b/drivers/mux/intel_cht_usb_mux.c > @@ -0,0 +1,269 @@ > +/* > + * Intel Cherrytrail USB OTG MUX driver > + * > + * Copyright (c) 2016 Hans de Goede > + * > + * Loosely based on android x86 kernel code which is: > + * > + * Copyright (C) 2014 Intel Corp. > + * > + * Author: Wu, Hao > + * > + * 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, or (at your option) > + * any later version. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include /* For the MUX_USB_* defines */ > +#include > +#include > +#include > + > +/* register definition */ > +#define DUAL_ROLE_CFG0 0x68 > +#define SW_VBUS_VALID (1 << 24) > +#define SW_IDPIN_EN (1 << 21) > +#define SW_IDPIN (1 << 20) > + > +#define DUAL_ROLE_CFG1 0x6c > +#define HOST_MODE (1 << 29) > + > +#define DUAL_ROLE_CFG1_POLL_TIMEOUT 1000 > + > +#define DRV_NAME "intel_cht_usb_mux" Dashes, please, if possible. Or are there perhaps a lot of precedent for other cherrytrail driver names? Because consistency across the tree is a tad more important than my issues with underscores... > + > +struct intel_cht_usb_mux { > + struct mutex cfg0_lock; > + void __iomem *base; > + struct extcon_dev *vbus_extcon; > + struct notifier_block vbus_nb; > + struct work_struct vbus_work; > +}; > + > +struct intel_cht_extcon_info { > + const char *hid; > + int hrv; > + const char *extcon; > +}; > + > +struct intel_cht_extcon_info vbus_providers[] = { > + { "INT33F4", -1, "axp288_extcon" }, > + { "INT34D3", 3, "cht_wcove_pwrsrc" }, > +}; Dito. And static const. What about: static const struct intel_cht_extcon_info vbus_providers[] = { { .hid = "INT33F4", .hrv = -1, .extcon = "axp288_extcon", }, { .hid = "INT34D3", .hrv = 3, .extcon = "cht_wcove_pwrsrc", }, }; > + > +static const unsigned int vbus_cable_ids[] = { > + EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP, > + EXTCON_CHG_USB_ACA, EXTCON_CHG_USB_FAST, > +}; > + > +static void intel_cht_usb_mux_set_sw_mode(struct intel_cht_usb_mux *mux) > +{ > + u32 data; > + > + data = readl(mux->base + DUAL_ROLE_CFG0); > + if (!(data & SW_IDPIN_EN)) { > + data |= SW_IDPIN_EN; > + writel(data, mux->base + DUAL_ROLE_CFG0); > + } > +} Is SW_IDPIN_EN a bit that unlocks the other bits in the register for writing? Once? Perhaps it interacts with the mode switch confirmation loop? Anyway, I'd like to see a comment about why this bit needs to be set so many times. > + > +static int intel_cht_usb_mux_set_mux(struct mux_control *mux_ctrl, int state) > +{ > + struct intel_cht_usb_mux *mux = mux_chip_priv(mux_ctrl->chip); The "mux" variable name is used for the mux_control in other drivers, and the private data is named something else. Please fix this all over the driver for consistency across the subsys. > + unsigned long timeout; > + bool host_mode; > + u32 data; > + > + mutex_lock(&mux->cfg0_lock); > + > + intel_cht_usb_mux_set_sw_mode(mux); > + > + /* Set idpin value as requested */ > + data = readl(mux->base + DUAL_ROLE_CFG0); > + switch (state & ~MUX_USB_POLARITY_INV) { > + case MUX_USB_NONE: > + case MUX_USB_DEVICE: > + data |= SW_IDPIN; > + host_mode = false; > + break; > + default: > + data &= ~SW_IDPIN; > + host_mode = true; > + } > + writel(data, mux->base + DUAL_ROLE_CFG0); > + > + mutex_unlock(&mux->cfg0_lock); > + > + /* In most case it takes about 600ms to finish mode switching */ > + timeout = jiffies + msecs_to_jiffies(DUAL_ROLE_CFG1_POLL_TIMEOUT); > + > + /* Polling on CFG1 register to confirm mode switch.*/ > + while (1) { > + data = readl(mux->base + DUAL_ROLE_CFG1); > + if (!!(data & HOST_MODE) == host_mode) > + break; > + > + /* Interval for polling is set to about 5 - 10 ms */ > + usleep_range(5000, 10000); > + > + if (time_after(jiffies, timeout)) { > + dev_warn(&mux_ctrl->chip->dev, > + "Timeout waiting for mux to switch\n"); > + break; > + } > + } > + > + return 0; > +} > + > +static void intel_cht_usb_mux_set_vbus_valid(struct intel_cht_usb_mux *mux, > + bool valid) > +{ > + u32 data; > + > + mutex_lock(&mux->cfg0_lock); > + > + intel_cht_usb_mux_set_sw_mode(mux); > + > + data = readl(mux->base + DUAL_ROLE_CFG0); > + if (valid) > + data |= SW_VBUS_VALID; > + else > + data &= ~SW_VBUS_VALID; > + writel(data, mux->base + DUAL_ROLE_CFG0); > + > + mutex_unlock(&mux->cfg0_lock); > +} > + > +static void intel_cht_usb_mux_vbus_work(struct work_struct *work) > +{ > + struct intel_cht_usb_mux *mux = > + container_of(work, struct intel_cht_usb_mux, vbus_work); > + bool vbus_present = false; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) { > + if (extcon_get_state(mux->vbus_extcon, vbus_cable_ids[i]) > 0) { > + vbus_present = true; > + break; > + } > + } > + > + intel_cht_usb_mux_set_vbus_valid(mux, vbus_present); > +} > + > +static int intel_cht_usb_mux_vbus_extcon_evt(struct notifier_block *nb, > + unsigned long event, void *param) > +{ > + struct intel_cht_usb_mux *mux = > + container_of(nb, struct intel_cht_usb_mux, vbus_nb); > + > + schedule_work(&mux->vbus_work); > + > + return NOTIFY_OK; > +} > + > +static const struct mux_control_ops intel_cht_usb_mux_ops = { > + .set = intel_cht_usb_mux_set_mux, > +}; > + > +static int intel_cht_usb_mux_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct intel_cht_usb_mux *mux; > + struct mux_chip *mux_chip; > + struct resource *res; > + resource_size_t size; > + int i, ret; > + > + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux)); > + if (IS_ERR(mux_chip)) > + return PTR_ERR(mux_chip); > + > + mux_chip->ops = &intel_cht_usb_mux_ops; > + mux_chip->mux[0].states = MUX_USB_STATES; This is "a lie" I think, the mux only supports two states; device and host. Looks good otherwise, if you also consider the remarks from Andy. Cheers, Peter > + mux = mux_chip_priv(mux_chip); > + mutex_init(&mux->cfg0_lock); > + > + /* > + * Besides controlling the mux we also need to control the vbus_valid > + * flag for device/gadget mode to work properly. To do this we monitor > + * the extcon interface exported by the PMIC drivers for the PMICs used > + * with the Cherry Trail SoC. > + * > + * We try to get the extcon_dev before registering the mux as this > + * may lead to us exiting with -EPROBE_DEFER. > + */ > + for (i = 0 ; i < ARRAY_SIZE(vbus_providers); i++) { > + if (!acpi_dev_present(vbus_providers[i].hid, NULL, > + vbus_providers[i].hrv)) > + continue; > + > + mux->vbus_extcon = extcon_get_extcon_dev( > + vbus_providers[i].extcon); > + if (mux->vbus_extcon == NULL) > + return -EPROBE_DEFER; > + > + dev_info(dev, "using extcon '%s' for vbus-valid\n", > + vbus_providers[i].extcon); > + break; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + size = (res->end + 1) - res->start; > + mux->base = devm_ioremap_nocache(dev, res->start, size); > + if (IS_ERR(mux->base)) { > + ret = PTR_ERR(mux->base); > + dev_err(dev, "can't iomap registers: %d\n", ret); > + return ret; > + } > + > + ret = devm_mux_chip_register(dev, mux_chip); > + if (ret < 0) > + return ret; > + > + if (mux->vbus_extcon) { > + INIT_WORK(&mux->vbus_work, intel_cht_usb_mux_vbus_work); > + mux->vbus_nb.notifier_call = intel_cht_usb_mux_vbus_extcon_evt; > + ret = devm_extcon_register_notifier_all(dev, mux->vbus_extcon, > + &mux->vbus_nb); > + if (ret) { > + dev_err(dev, "can't register vbus extcon notifier: %d\n", > + ret); > + return ret; > + } > + > + /* Sync initial mode */ > + schedule_work(&mux->vbus_work); > + } > + > + return 0; > +} > + > +static const struct platform_device_id intel_cht_usb_mux_table[] = { > + { .name = DRV_NAME }, > + {}, > +}; > +MODULE_DEVICE_TABLE(platform, intel_cht_usb_mux_table); > + > +static struct platform_driver intel_cht_usb_mux_driver = { > + .driver = { > + .name = DRV_NAME, > + }, > + .id_table = intel_cht_usb_mux_table, > + .probe = intel_cht_usb_mux_probe, > +}; > + > +module_platform_driver(intel_cht_usb_mux_driver); > + > +MODULE_AUTHOR("Hans de Goede "); > +MODULE_DESCRIPTION("Intel Cherrytrail USB mux driver"); > +MODULE_LICENSE("GPL"); >