> -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Hans de Goede > Sent: Thursday, August 29, 2019 4:34 PM > To: Gopal, Saranya <saranya.gopal@xxxxxxxxx>; > heikki.krogerus@xxxxxxxxxxxxxxx > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Nyman, Mathias > <mathias.nyman@xxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Balaji, M > <m.balaji@xxxxxxxxx>; Rafał Psota <rafalzaq@xxxxxxxxx>; Rafael J. Wysocki > <rjw@xxxxxxxxxxxxx> > Subject: Re: [PATCH v4 2/2] usb: roles: intel: Enable static DRD mode for role > switch > > Hi Saranya, > > On 29-08-19 12:42, Saranya Gopal wrote: > > 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> > > Unfortunately this patch conflicts with a patch to > drivers/usb/roles/intel-xhci-usb-role-switch.c from Heikki > which is already in -next, see: > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux- > pm.git/log/?h=devprop > > And specifically this commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux- > pm.git/commit/?h=devprop&id=d2a90ebb65536a6deeb9daf5aa8e0700e8cbb43 > a > > So you need to rebase on op of that branch and then the subsys > maintainers need to figure out how to merge this, I guess > the usb-next tree can merge Rafael's devprop branch for this? > Sure, let me rebase on top of that branch. Thanks, Saranya > I've manually resolved the conflict locally and given this new version > a test-run on a Cherry Trail device and things still work as they should > there, so with the conflict fixed this series is: > > Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx> > Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > Regards, > > Hans > > > > --- > > changes since v3: Initialized drd_config variable to fix warning > > changes since v2: Revised SoB tags > > changes since v1: Added drd_config to avoid multiple if checks > > Other minor changes suggested by Hans > > > > drivers/usb/roles/intel-xhci-usb-role-switch.c | 27 > ++++++++++++++++++++++++-- > > 1 file changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c > b/drivers/usb/roles/intel-xhci-usb-role-switch.c > > index 277de96..88d0416 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,12 @@ > > #define SW_VBUS_VALID BIT(24) > > #define SW_IDPIN_EN BIT(21) > > #define SW_IDPIN BIT(20) > > +#define SW_SWITCH_EN BIT(16) > > + > > +#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 +44,7 @@ > > struct intel_xhci_usb_data { > > struct usb_role_switch *role_sw; > > void __iomem *base; > > + bool enable_sw_switch; > > }; > > > > static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) > > @@ -45,6 +53,7 @@ static int intel_xhci_usb_set_role(struct device *dev, > enum usb_role role) > > unsigned long timeout; > > acpi_status status; > > u32 glk, val; > > + u32 drd_config = DRD_CONFIG_DYNAMIC; > > > > /* > > * On many CHT devices ACPI event (_AEI) handlers read / modify / > > @@ -59,24 +68,35 @@ 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 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; > > + 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->enable_sw_switch) { > > + val &= ~DRD_CONFIG_MASK; > > + val |= SW_SWITCH_EN | drd_config; > > + } > > writel(val, data->base + DUAL_ROLE_CFG0); > > > > acpi_release_global_lock(glk); > > @@ -147,6 +167,9 @@ static int intel_xhci_usb_probe(struct platform_device > *pdev) > > > > platform_set_drvdata(pdev, data); > > > > + data->enable_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)) > > return PTR_ERR(data->role_sw); > >
