| Message ID | 20180216104751.8371-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Fri, Feb 16, 2018 at 11:47:48AM +0100, Hans de Goede wrote: > diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile > index 060643a1b5c8..7d1b8c82b208 100644 > --- a/drivers/usb/Makefile > +++ b/drivers/usb/Makefile > @@ -65,3 +65,5 @@ obj-$(CONFIG_USB_COMMON) += common/ > obj-$(CONFIG_USBIP_CORE) += usbip/ > > obj-$(CONFIG_TYPEC) += typec/ > + > +obj-$(CONFIG_USB_ROLE_SWITCH) += roles/ > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig > new file mode 100644 > index 000000000000..e5c2bfaf8ba5 > --- /dev/null > +++ b/drivers/usb/roles/Kconfig > @@ -0,0 +1,14 @@ > +if USB_ROLE_SWITCH > + > +config USB_ROLES_INTEL_XHCI > + tristate "Intel XHCI USB Role Switch" > + depends on ACPI && X86 && EXTCON Hold on... There is no dependency on extcon, or is there? Thanks,
On Fri, Feb 16, 2018 at 12:47 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Various Intel SoCs (Cherry Trail, Broxton and others) have an internal USB > role switch for swiching the OTG USB data lines between the xHCI host > controller and the dwc3 gadget controller. > > Note on some Cherry Trail systems there is ACPI/AML code listening to > edge interrupts on the id-pin (through an _AIE ACPI method) and switching > the role between ROLE_HOST and ROLE_NONE based on the id-pin. Note it does > not set the role to ROLE_DEVICE, because device-mode is usually not used > under Windows. > > The presence of AML code which modifies the cfg0 reg (on some systems) > means that we our read/write/modify of cfg0 may race with the AML code > doing the same to avoid this we take the global ACPI lock while doing > the read/write/modify. > +/* 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) Does it make sense to use BIT() macro above? > +struct intel_xhci_acpi_match { > + const char *hid; > + int hrv; > +}; Consider to unify with struct acpi_ac_bl. > +static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = { > + { "INT33F4", 3 }, /* X-Powers AXP288 PMIC */ > +}; > + > +static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) > +{ > + struct intel_xhci_usb_data *data = dev_get_drvdata(dev); > + unsigned long timeout; > + acpi_status status; > + u32 glk = -1U; I prefer to see consistency and moreover less confusing set, like ~0U > + u32 val; > + > + /* > + * On many CHT devices ACPI event (_AEI) handlers read / modify / > + * write the cfg0 register, just like we do. Take the ACPI lock > + * to avoid us racing with the AML code. > + */ > + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk); FOREVER?! Wouldn't be slightly long under certain circumstances? > + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) { > + dev_err(dev, "Error could not acquire lock\n"); > + return -EIO; > + } > + acpi_release_global_lock(glk); > + /* Polling on CFG1 register to confirm mode switch.*/ > + do { > + val = readl(data->base + DUAL_ROLE_CFG1); > + if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) I would prefer ^ instead of first ==, but it's up to you. > + return 0; > + > + /* Interval for polling is set to about 5 - 10 ms */ > + usleep_range(5000, 10000); > + } while (time_before(jiffies, timeout)); > + > + dev_warn(dev, "Timeout waiting for role-switch\n"); > + return -ETIMEDOUT; > +} > +static int intel_xhci_usb_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct intel_xhci_usb_data *data; > + struct resource *res; > + resource_size_t size; > + int i, ret; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + size = (res->end + 1) - res->start; resource_size() > + data->base = devm_ioremap_nocache(dev, res->start, size); So, what's wrong with devm_ioremap_resource() ? ...which also prints an error message. > + if (IS_ERR(data->base)) { > + ret = PTR_ERR(data->base); > + dev_err(dev, "Error iomaping registers: %d\n", ret); At least printing return code is useless. Driver core does this. > + return ret; > + } > + > + data->role_sw = usb_role_switch_register(dev, &sw_desc); > + if (IS_ERR(data->role_sw)) { > + ret = PTR_ERR(data->role_sw); > + dev_err(dev, "Error registering role-switch: %d\n", ret); Ditto. > + return ret; > + } > + > + return 0; > +} > +static const struct platform_device_id intel_xhci_usb_table[] = { > + { .name = DRV_NAME }, > + {}, No comma, please. > +};
Hi, On 16-02-18 14:38, Heikki Krogerus wrote: > On Fri, Feb 16, 2018 at 11:47:48AM +0100, Hans de Goede wrote: >> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile >> index 060643a1b5c8..7d1b8c82b208 100644 >> --- a/drivers/usb/Makefile >> +++ b/drivers/usb/Makefile >> @@ -65,3 +65,5 @@ obj-$(CONFIG_USB_COMMON) += common/ >> obj-$(CONFIG_USBIP_CORE) += usbip/ >> >> obj-$(CONFIG_TYPEC) += typec/ >> + >> +obj-$(CONFIG_USB_ROLE_SWITCH) += roles/ >> diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig >> new file mode 100644 >> index 000000000000..e5c2bfaf8ba5 >> --- /dev/null >> +++ b/drivers/usb/roles/Kconfig >> @@ -0,0 +1,14 @@ >> +if USB_ROLE_SWITCH >> + >> +config USB_ROLES_INTEL_XHCI >> + tristate "Intel XHCI USB Role Switch" >> + depends on ACPI && X86 && EXTCON > > Hold on... There is no dependency on extcon, or is there? You are right, this is an old leftover, removed for v2 of this patchset which I'm preparing atm. Regards, Hans
Hi, On 16-02-18 14:47, Andy Shevchenko wrote: > On Fri, Feb 16, 2018 at 12:47 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Various Intel SoCs (Cherry Trail, Broxton and others) have an internal USB >> role switch for swiching the OTG USB data lines between the xHCI host >> controller and the dwc3 gadget controller. >> >> Note on some Cherry Trail systems there is ACPI/AML code listening to >> edge interrupts on the id-pin (through an _AIE ACPI method) and switching >> the role between ROLE_HOST and ROLE_NONE based on the id-pin. Note it does >> not set the role to ROLE_DEVICE, because device-mode is usually not used >> under Windows. >> >> The presence of AML code which modifies the cfg0 reg (on some systems) >> means that we our read/write/modify of cfg0 may race with the AML code >> doing the same to avoid this we take the global ACPI lock while doing >> the read/write/modify. > >> +/* 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) > > Does it make sense to use BIT() macro above? Yes, fixed for v2. > > >> +struct intel_xhci_acpi_match { >> + const char *hid; >> + int hrv; >> +}; > > Consider to unify with struct acpi_ac_bl. That is not a bad idea, but probably best done as a follow-up commit, since this patch-set already touches enough subsystems as is. I just added this to my TODO: -Add acpi_find_dev_present() helprt which takes an array of and returns a pointer to (or NULL): struct acpi_dev_present_match { const char *hid; const char *uid; int hrv; }; And use this in drivers/acpi/ac.c drivers/acpi/battery.c, drivers/usb/roles/intel-xhci-usb-role-switch.c, ... >> +static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = { >> + { "INT33F4", 3 }, /* X-Powers AXP288 PMIC */ >> +}; >> + >> +static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) >> +{ >> + struct intel_xhci_usb_data *data = dev_get_drvdata(dev); >> + unsigned long timeout; >> + acpi_status status; > >> + u32 glk = -1U; > > I prefer to see consistency and moreover less confusing set, like > > ~0U Looks like a chose a bad example as user of the acpi_acquire_global_lock() function, others don't init this at all because it is not necessary. > >> + u32 val; >> + >> + /* >> + * On many CHT devices ACPI event (_AEI) handlers read / modify / >> + * write the cfg0 register, just like we do. Take the ACPI lock >> + * to avoid us racing with the AML code. >> + */ >> + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk); > > FOREVER?! > Wouldn't be slightly long under certain circumstances? The mode-switch itself may take up-to 600ms, so I don't think any delays caused by this will be a problem. This is just some weird ACPI-subsys-ism where instead of just having a mutex and using mutex_trylock() where necessary, the ACPICA code has its own private timeout handling. I'm sure if I were to just do a mutex_lock() here nobody would fall over that. The FOREVER just makes this look scarier then it really is, in theory any mutex_lock() call can wait forever. >> + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) { >> + dev_err(dev, "Error could not acquire lock\n"); >> + return -EIO; >> + } > >> + acpi_release_global_lock(glk); > >> + /* Polling on CFG1 register to confirm mode switch.*/ >> + do { >> + val = readl(data->base + DUAL_ROLE_CFG1); > >> + if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) > > I would prefer ^ instead of first ==, but it's up to you. > >> + return 0; >> + >> + /* Interval for polling is set to about 5 - 10 ms */ >> + usleep_range(5000, 10000); >> + } while (time_before(jiffies, timeout)); >> + >> + dev_warn(dev, "Timeout waiting for role-switch\n"); >> + return -ETIMEDOUT; >> +} > >> +static int intel_xhci_usb_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct intel_xhci_usb_data *data; >> + struct resource *res; >> + resource_size_t size; >> + int i, ret; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + size = (res->end + 1) - res->start; > > resource_size() Fixed for v2. >> + data->base = devm_ioremap_nocache(dev, res->start, size); > > So, what's wrong with devm_ioremap_resource() ? > ...which also prints an error message. Nothing, I inherited this from the Android X86 patches this is based on, fixed for v2. > >> + if (IS_ERR(data->base)) { >> + ret = PTR_ERR(data->base); > >> + dev_err(dev, "Error iomaping registers: %d\n", ret); > > At least printing return code is useless. Driver core does this. > >> + return ret; >> + } >> + > >> + data->role_sw = usb_role_switch_register(dev, &sw_desc); >> + if (IS_ERR(data->role_sw)) { >> + ret = PTR_ERR(data->role_sw); > >> + dev_err(dev, "Error registering role-switch: %d\n", ret); > > Ditto. Ok, both dropped. > >> + return ret; >> + } >> + >> + return 0; >> +} > >> +static const struct platform_device_id intel_xhci_usb_table[] = { >> + { .name = DRV_NAME }, > >> + {}, > > No comma, please. Fixed for v2. Regards, Hans
diff --git a/MAINTAINERS b/MAINTAINERS index 3a6334da07d1..385ef9080287 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14385,6 +14385,12 @@ S: Maintained F: Documentation/hid/hiddev.txt F: drivers/hid/usbhid/ +USB INTEL XHCI ROLE MUX DRIVER +M: Hans de Goede <hdegoede@redhat.com> +L: linux-usb@vger.kernel.org +S: Maintained +F: drivers/usb/roles/intel-xhci-usb-role-switch.c + USB ISP116X DRIVER M: Olav Kongas <ok@artecdesign.ee> L: linux-usb@vger.kernel.org diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig index a79198f5f1a6..3f123edba6e8 100644 --- a/drivers/usb/Kconfig +++ b/drivers/usb/Kconfig @@ -165,6 +165,8 @@ source "drivers/usb/gadget/Kconfig" source "drivers/usb/typec/Kconfig" +source "drivers/usb/roles/Kconfig" + config USB_LED_TRIG bool "USB LED Triggers" depends on LEDS_CLASS && LEDS_TRIGGERS diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile index 060643a1b5c8..7d1b8c82b208 100644 --- a/drivers/usb/Makefile +++ b/drivers/usb/Makefile @@ -65,3 +65,5 @@ obj-$(CONFIG_USB_COMMON) += common/ obj-$(CONFIG_USBIP_CORE) += usbip/ obj-$(CONFIG_TYPEC) += typec/ + +obj-$(CONFIG_USB_ROLE_SWITCH) += roles/ diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig new file mode 100644 index 000000000000..e5c2bfaf8ba5 --- /dev/null +++ b/drivers/usb/roles/Kconfig @@ -0,0 +1,14 @@ +if USB_ROLE_SWITCH + +config USB_ROLES_INTEL_XHCI + tristate "Intel XHCI USB Role Switch" + depends on ACPI && X86 && EXTCON + help + Driver for the internal USB role switch for switching the USB data + lines between the xHCI host controller and the dwc3 gadget controller + found on various Intel SoCs. + + To compile the driver as a module, choose M here: the module will + be called intel-xhci-usb-role-switch. + +endif # USB_ROLE_SWITCH diff --git a/drivers/usb/roles/Makefile b/drivers/usb/roles/Makefile new file mode 100644 index 000000000000..e44b179ba275 --- /dev/null +++ b/drivers/usb/roles/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_USB_ROLES_INTEL_XHCI) += intel-xhci-usb-role-switch.o diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c new file mode 100644 index 000000000000..b46c44b0e028 --- /dev/null +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Intel XHCI (Cherry Trail, Broxton and others) USB OTG role switch driver + * + * Copyright (c) 2016-2017 Hans de Goede <hdegoede@redhat.com> + * + * Loosely based on android x86 kernel code which is: + * + * Copyright (C) 2014 Intel Corp. + * + * Author: Wu, Hao + */ + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/usb/role.h> + +/* 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_xhci_usb_sw" + +struct intel_xhci_usb_data { + struct usb_role_switch *role_sw; + void __iomem *base; +}; + +struct intel_xhci_acpi_match { + const char *hid; + int hrv; +}; + +/* + * ACPI IDs for PMICs which do not support separate data and power role + * detection (USB ACA detection for micro USB OTG), we allow userspace to + * change the role manually on these. + */ +static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = { + { "INT33F4", 3 }, /* X-Powers AXP288 PMIC */ +}; + +static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) +{ + struct intel_xhci_usb_data *data = dev_get_drvdata(dev); + unsigned long timeout; + acpi_status status; + u32 glk = -1U; + u32 val; + + /* + * On many CHT devices ACPI event (_AEI) handlers read / modify / + * write the cfg0 register, just like we do. Take the ACPI lock + * to avoid us racing with the AML code. + */ + status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk); + if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) { + dev_err(dev, "Error could not acquire lock\n"); + return -EIO; + } + + /* Set idpin value as requested */ + val = readl(data->base + DUAL_ROLE_CFG0); + switch (role) { + case USB_ROLE_NONE: + val |= SW_IDPIN; + val &= ~SW_VBUS_VALID; + break; + case USB_ROLE_HOST: + val &= ~SW_IDPIN; + val &= ~SW_VBUS_VALID; + break; + case USB_ROLE_DEVICE: + val |= SW_IDPIN; + val |= SW_VBUS_VALID; + break; + } + val |= SW_IDPIN_EN; + + writel(val, data->base + DUAL_ROLE_CFG0); + + acpi_release_global_lock(glk); + + /* 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.*/ + do { + val = readl(data->base + DUAL_ROLE_CFG1); + if (!!(val & HOST_MODE) == (role == USB_ROLE_HOST)) + return 0; + + /* Interval for polling is set to about 5 - 10 ms */ + usleep_range(5000, 10000); + } while (time_before(jiffies, timeout)); + + dev_warn(dev, "Timeout waiting for role-switch\n"); + return -ETIMEDOUT; +} + +static enum usb_role intel_xhci_usb_get_role(struct device *dev) +{ + struct intel_xhci_usb_data *data = dev_get_drvdata(dev); + enum usb_role role; + u32 val; + + val = readl(data->base + DUAL_ROLE_CFG0); + + if (!(val & SW_IDPIN)) + role = USB_ROLE_HOST; + else if (val & SW_VBUS_VALID) + role = USB_ROLE_DEVICE; + else + role = USB_ROLE_NONE; + + return role; +} + +static struct usb_role_switch_desc sw_desc = { + .set = intel_xhci_usb_set_role, + .get = intel_xhci_usb_get_role, +}; + +static int intel_xhci_usb_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct intel_xhci_usb_data *data; + struct resource *res; + resource_size_t size; + int i, ret; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + size = (res->end + 1) - res->start; + data->base = devm_ioremap_nocache(dev, res->start, size); + if (IS_ERR(data->base)) { + ret = PTR_ERR(data->base); + dev_err(dev, "Error iomaping registers: %d\n", ret); + return ret; + } + + for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++) + if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1", + allow_userspace_ctrl_ids[i].hrv)) + sw_desc.allow_userspace_control = true; + + platform_set_drvdata(pdev, data); + + data->role_sw = usb_role_switch_register(dev, &sw_desc); + if (IS_ERR(data->role_sw)) { + ret = PTR_ERR(data->role_sw); + dev_err(dev, "Error registering role-switch: %d\n", ret); + return ret; + } + + return 0; +} + +int intel_xhci_usb_remove(struct platform_device *pdev) +{ + struct intel_xhci_usb_data *data = platform_get_drvdata(pdev); + + usb_role_switch_unregister(data->role_sw); + return 0; +} + +static const struct platform_device_id intel_xhci_usb_table[] = { + { .name = DRV_NAME }, + {}, +}; +MODULE_DEVICE_TABLE(platform, intel_xhci_usb_table); + +static struct platform_driver intel_xhci_usb_driver = { + .driver = { + .name = DRV_NAME, + }, + .id_table = intel_xhci_usb_table, + .probe = intel_xhci_usb_probe, + .remove = intel_xhci_usb_remove, +}; + +module_platform_driver(intel_xhci_usb_driver); + +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); +MODULE_DESCRIPTION("Intel XHCI USB role switch driver"); +MODULE_LICENSE("GPL");
Various Intel SoCs (Cherry Trail, Broxton and others) have an internal USB role switch for swiching the OTG USB data lines between the xHCI host controller and the dwc3 gadget controller. Note on some Cherry Trail systems there is ACPI/AML code listening to edge interrupts on the id-pin (through an _AIE ACPI method) and switching the role between ROLE_HOST and ROLE_NONE based on the id-pin. Note it does not set the role to ROLE_DEVICE, because device-mode is usually not used under Windows. The presence of AML code which modifies the cfg0 reg (on some systems) means that we our read/write/modify of cfg0 may race with the AML code doing the same to avoid this we take the global ACPI lock while doing the read/write/modify. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- MAINTAINERS | 6 + drivers/usb/Kconfig | 2 + drivers/usb/Makefile | 2 + drivers/usb/roles/Kconfig | 14 ++ drivers/usb/roles/Makefile | 1 + drivers/usb/roles/intel-xhci-usb-role-switch.c | 201 +++++++++++++++++++++++++ 6 files changed, 226 insertions(+) create mode 100644 drivers/usb/roles/Kconfig create mode 100644 drivers/usb/roles/Makefile create mode 100644 drivers/usb/roles/intel-xhci-usb-role-switch.c