> -----Original Message-----
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of Hans de Goede
> Sent: Thursday, August 29, 2019 4:34 PM
> To: Gopal, Saranya <saranya.go...@intel.com>;
> heikki.kroge...@linux.intel.com
> Cc: gre...@linuxfoundation.org; Nyman, Mathias
> <mathias.ny...@intel.com>; linux-usb@vger.kernel.org; Balaji, M
> <m.bal...@intel.com>; Rafał Psota <rafal...@gmail.com>; Rafael J. Wysocki
> <r...@rjwysocki.net>
> 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.go...@intel.com>
> > Signed-off-by: Balaji Manoharan <m.bal...@intel.com>
>
> 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 <hdego...@redhat.com>
> Reviewed-by: Hans de Goede <hdego...@redhat.com>
>
> 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);
> >
