Hi,
On 26-08-19 16:32, Heikki Krogerus wrote:
From: Saranya Gopal <saranya.gopal@xxxxxxxxx>
Enable static DRD mode in Intel platforms which guarantees
successful role switch all the time. This fixes issues like
software role switch failure after cold boot and issue with
role switch when USB 3.0 cable is used. But, do not enable
static DRD mode for Cherrytrail devices which rely on firmware
for role switch.
Signed-off-by: Saranya Gopal <saranya.gopal@xxxxxxxxx>
Signed-off-by: Balaji Manoharan <m.balaji@xxxxxxxxx>
Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
---
.../usb/roles/intel-xhci-usb-role-switch.c | 26 ++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 7325a84dd1c8..9101ff94c8d2 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -19,6 +19,7 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/property.h>
#include <linux/usb/role.h>
/* register definition */
@@ -26,6 +27,9 @@
#define SW_VBUS_VALID BIT(24)
#define SW_IDPIN_EN BIT(21)
#define SW_IDPIN BIT(20)
+#define SW_SWITCH_EN_CFG0 BIT(16)
Nitpick: Please drop the _CFG0 postfix, if anything this
should be a prefix applied to *all* defines for the bits
in this register
+#define SW_DRD_STATIC_HOST_CFG0 1
+#define SW_DRD_STATIC_DEV_CFG0 2
So bits 0-1 together define the drd-mode. The datasheet
calls the combination DRD_CONFIG, without a SW_ prefix,
with the following values being defined:
00: Dynamic role-switch
01: Static Host mode
10: Static device mode
11: Reserved
Notice this is an enum, so the use of bit-ops to clear the
other state below is wrong. It happens to work, but this is
not how a multi-bit setting should be modified.
I suggest using the following defines instead:
#define DRD_CONFIG_DYNAMIC 0
#define DRD_CONFIG_STATIC_HOST 1
#define DRD_CONFIG_STATIC_DEVICE 2
#define DRD_CONFIG_MASK 3
#define DUAL_ROLE_CFG1 0x6c
#define HOST_MODE BIT(29)
@@ -37,6 +41,7 @@
struct intel_xhci_usb_data {
struct usb_role_switch *role_sw;
void __iomem *base;
+ bool disable_sw_switch;
I would prefer for this to be enable_sw_switch and the negation
when reading the property.
};
static const struct software_node intel_xhci_usb_node = {
@@ -63,23 +68,39 @@ static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
pm_runtime_get_sync(dev);
- /* Set idpin value as requested */
+ /*
+ * Set idpin value as requested.
+ * Since some devices rely on firmware setting DRD_CONFIG and
+ * SW_SWITCH_EN_CFG0 bits to be zero for role switch,
+ * do not set these bits for those devices.
+ */
val = readl(data->base + DUAL_ROLE_CFG0);
switch (role) {
case USB_ROLE_NONE:
val |= SW_IDPIN;
val &= ~SW_VBUS_VALID;
+ val &= ~(SW_DRD_STATIC_DEV_CFG0 | SW_DRD_STATIC_HOST_CFG0);
break;
Right, so this should be:
val &= ~DRD_CONFIG_MASK;
Also ideally this should also have a if (!data->disable_sw_switch)
for consistency.
case USB_ROLE_HOST:
val &= ~SW_IDPIN;
val &= ~SW_VBUS_VALID;
+ if (!data->disable_sw_switch) {
+ val &= ~SW_DRD_STATIC_DEV_CFG0;
So this clearing is wrong, it happens to work, but is not
how modifying a "bit-set" / enum should be done, this should be:
val &= ~DRD_CONFIG_MASK;
+ val |= SW_DRD_STATIC_HOST_CFG0;
+ }
break;
case USB_ROLE_DEVICE:
val |= SW_IDPIN;
val |= SW_VBUS_VALID;
+ if (!data->disable_sw_switch) {
+ val &= ~SW_DRD_STATIC_HOST_CFG0;
+ val |= SW_DRD_STATIC_DEV_CFG0;
+ }
Idem.
break;
}
val |= SW_IDPIN_EN;
+ if (!data->disable_sw_switch)
+ val |= SW_SWITCH_EN_CFG0;
So we now have a lot of "if (!data->disable_sw_switch)" checks,
IMHO it would be better / cleaner to do this:
u32 glk, val, drd_config;
...
val = readl(data->base + DUAL_ROLE_CFG0);
switch (role) {
case USB_ROLE_NONE:
val |= SW_IDPIN;
val &= ~SW_VBUS_VALID;
drd_config = DRD_CONFIG_DYNAMIC;
break;
case USB_ROLE_HOST:
val &= ~SW_IDPIN;
val &= ~SW_VBUS_VALID;
drd_config = DRD_CONFIG_STATIC_HOST;
break;
case USB_ROLE_DEVICE:
val |= SW_IDPIN;
val |= SW_VBUS_VALID;
drd_config = DRD_CONFIG_STATIC_DEVICE;
break;
}
val |= SW_IDPIN_EN;
if (!data->disable_sw_switch) {
val &= ~DRD_CONFIG_MASK;
val |= SW_SWITCH_EN_CFG0 | drd_config;
}
...
Regards,
Hans
writel(val, data->base + DUAL_ROLE_CFG0);
@@ -156,6 +177,9 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
sw_desc.allow_userspace_control = true,
sw_desc.fwnode = software_node_fwnode(&intel_xhci_usb_node);
+ data->disable_sw_switch = device_property_read_bool(dev,
+ "sw_switch_disable");
+
data->role_sw = usb_role_switch_register(dev, &sw_desc);
if (IS_ERR(data->role_sw)) {
fwnode_handle_put(sw_desc.fwnode);
