Hi Heikki, Yes, we will fix this and resubmit a newer version of the patch. Thanks, Saranya > -----Original Message----- > From: Heikki Krogerus [mailto:heikki.krogerus@xxxxxxxxxxxxxxx] > Sent: Wednesday, August 28, 2019 1:12 PM > To: Hans de Goede <hdegoede@xxxxxxxxxx>; Gopal, Saranya > <saranya.gopal@xxxxxxxxx>; Balaji, M <m.balaji@xxxxxxxxx> > Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Nyman, Mathias > <mathias.nyman@xxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/2] usb: roles: intel: Enable static DRD mode for role > switch > > On Tue, Aug 27, 2019 at 03:39:18PM +0200, Hans de Goede wrote: > > 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; > > } > > > That looks good to me. Saranya, Balaji! Can you fix that. I don't > think you need me for this anymore. > > thanks, > > -- > heikki
