Hi,
On 16-02-18 14:47, Andy Shevchenko wrote:
On Fri, Feb 16, 2018 at 12:47 PM, Hans de Goede <hdegoede@xxxxxxxxxx> 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
