| Message ID | 1567075327-24016-2-git-send-email-saranya.gopal@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] xhci-ext-caps.c: Add property to disable Intel SW switch | expand |
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@intel.com> > Signed-off-by: Balaji Manoharan <m.balaji@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=d2a90ebb65536a6deeb9daf5aa8e0700e8cbb43a 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? 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@redhat.com> Reviewed-by: Hans de Goede <hdegoede@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); >
On Thu, Aug 29, 2019 at 04:12:07PM +0530, 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@intel.com> > Signed-off-by: Balaji Manoharan <m.balaji@intel.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > 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); > -- > 1.9.1 thanks,
> -----Original Message----- > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb- > owner@vger.kernel.org] On Behalf Of Hans de Goede > Sent: Thursday, August 29, 2019 4:34 PM > To: Gopal, Saranya <saranya.gopal@intel.com>; > heikki.krogerus@linux.intel.com > Cc: gregkh@linuxfoundation.org; Nyman, Mathias > <mathias.nyman@intel.com>; linux-usb@vger.kernel.org; Balaji, M > <m.balaji@intel.com>; Rafał Psota <rafalzaq@gmail.com>; Rafael J. Wysocki > <rjw@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.gopal@intel.com> > > Signed-off-by: Balaji Manoharan <m.balaji@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 <hdegoede@redhat.com> > Reviewed-by: Hans de Goede <hdegoede@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); > >
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);