| Message ID | a792b1ea6b7083d400b3a6b38dcca70588fc5587.1617929509.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Set quirks for xhci/dwc3 host mode | expand |
On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote: > We can check for host quirks at runtime base on the controller IP and > version check. Set the following quirks for the DWC_usb31 IP host mode > before creating a platform device for the xHCI driver: > > * XHCI_ISOC_BLOCKED_DISCONNECT > * XHCI_LIMIT_FS_BI_INTR_EP > * XHCI_LOST_DISCONNECT_QUIRK > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index f29a264635aa..a486d7fbb163 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -9,6 +9,7 @@ > > #include <linux/acpi.h> > #include <linux/platform_device.h> > +#include <linux/usb/xhci-quirks.h> > > #include "core.h" > > @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > return irq; > } > > +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv) > +{ > + memset(priv, 0, sizeof(*priv)); > + > + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) { > + priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT; > + priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP; > + priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK; > + } > +} > + > int dwc3_host_init(struct dwc3 *dwc) > { > struct property_entry props[4]; > @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc) > int ret, irq; > struct resource *res; > struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > + struct xhci_plat_priv dwc3_priv; Tying the dwc3 code to the xhci code like this feels really wrong to me, are you sure this is the correct resolution? greg k-h
Greg Kroah-Hartman wrote: > On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote: >> We can check for host quirks at runtime base on the controller IP and >> version check. Set the following quirks for the DWC_usb31 IP host mode >> before creating a platform device for the xHCI driver: >> >> * XHCI_ISOC_BLOCKED_DISCONNECT >> * XHCI_LIMIT_FS_BI_INTR_EP >> * XHCI_LOST_DISCONNECT_QUIRK >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >> index f29a264635aa..a486d7fbb163 100644 >> --- a/drivers/usb/dwc3/host.c >> +++ b/drivers/usb/dwc3/host.c >> @@ -9,6 +9,7 @@ >> >> #include <linux/acpi.h> >> #include <linux/platform_device.h> >> +#include <linux/usb/xhci-quirks.h> >> >> #include "core.h" >> >> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) >> return irq; >> } >> >> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv) >> +{ >> + memset(priv, 0, sizeof(*priv)); >> + >> + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) { >> + priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT; >> + priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP; >> + priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK; >> + } >> +} >> + >> int dwc3_host_init(struct dwc3 *dwc) >> { >> struct property_entry props[4]; >> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc) >> int ret, irq; >> struct resource *res; >> struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); >> + struct xhci_plat_priv dwc3_priv; > > Tying the dwc3 code to the xhci code like this feels really wrong to me, > are you sure this is the correct resolution? > > greg k-h > Can you clarify what feels wrong? The way it's originally implemented already tied them in that way. What we're doing here simply takes advantage of what xhci-plat glue layer can use to set the xhci quirks. With this, we don't have to create new and duplicate DT properties for dwc3 and xhci to set some quirks. With the expanding list of dwc3 DT, I see this as a plus. Thanks, Thinh
On Fri, Apr 09, 2021 at 08:01:15AM +0000, Thinh Nguyen wrote: > Greg Kroah-Hartman wrote: > > On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote: > >> We can check for host quirks at runtime base on the controller IP and > >> version check. Set the following quirks for the DWC_usb31 IP host mode > >> before creating a platform device for the xHCI driver: > >> > >> * XHCI_ISOC_BLOCKED_DISCONNECT > >> * XHCI_LIMIT_FS_BI_INTR_EP > >> * XHCI_LOST_DISCONNECT_QUIRK > >> > >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >> --- > >> drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++ > >> 1 file changed, 21 insertions(+) > >> > >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >> index f29a264635aa..a486d7fbb163 100644 > >> --- a/drivers/usb/dwc3/host.c > >> +++ b/drivers/usb/dwc3/host.c > >> @@ -9,6 +9,7 @@ > >> > >> #include <linux/acpi.h> > >> #include <linux/platform_device.h> > >> +#include <linux/usb/xhci-quirks.h> > >> > >> #include "core.h" > >> > >> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > >> return irq; > >> } > >> > >> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv) > >> +{ > >> + memset(priv, 0, sizeof(*priv)); > >> + > >> + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) { > >> + priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT; > >> + priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP; > >> + priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK; > >> + } > >> +} > >> + > >> int dwc3_host_init(struct dwc3 *dwc) > >> { > >> struct property_entry props[4]; > >> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc) > >> int ret, irq; > >> struct resource *res; > >> struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > >> + struct xhci_plat_priv dwc3_priv; > > > > Tying the dwc3 code to the xhci code like this feels really wrong to me, > > are you sure this is the correct resolution? > > > > greg k-h > > > > Can you clarify what feels wrong? The way it's originally implemented > already tied them in that way. What we're doing here simply takes > advantage of what xhci-plat glue layer can use to set the xhci quirks. > With this, we don't have to create new and duplicate DT properties for > dwc3 and xhci to set some quirks. With the expanding list of dwc3 DT, I > see this as a plus. Ok, still feels odd, but I'll let the xhci maintainer make the decision on it as it's their code to maintain, not mine :) thanks, greg k-h
Greg Kroah-Hartman wrote: > On Fri, Apr 09, 2021 at 08:01:15AM +0000, Thinh Nguyen wrote: >> Greg Kroah-Hartman wrote: >>> On Thu, Apr 08, 2021 at 06:42:32PM -0700, Thinh Nguyen wrote: >>>> We can check for host quirks at runtime base on the controller IP and >>>> version check. Set the following quirks for the DWC_usb31 IP host mode >>>> before creating a platform device for the xHCI driver: >>>> >>>> * XHCI_ISOC_BLOCKED_DISCONNECT >>>> * XHCI_LIMIT_FS_BI_INTR_EP >>>> * XHCI_LOST_DISCONNECT_QUIRK >>>> >>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>> --- >>>> drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>> index f29a264635aa..a486d7fbb163 100644 >>>> --- a/drivers/usb/dwc3/host.c >>>> +++ b/drivers/usb/dwc3/host.c >>>> @@ -9,6 +9,7 @@ >>>> >>>> #include <linux/acpi.h> >>>> #include <linux/platform_device.h> >>>> +#include <linux/usb/xhci-quirks.h> >>>> >>>> #include "core.h" >>>> >>>> @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) >>>> return irq; >>>> } >>>> >>>> +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv) >>>> +{ >>>> + memset(priv, 0, sizeof(*priv)); >>>> + >>>> + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) { >>>> + priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT; >>>> + priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP; >>>> + priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK; >>>> + } >>>> +} >>>> + >>>> int dwc3_host_init(struct dwc3 *dwc) >>>> { >>>> struct property_entry props[4]; >>>> @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc) >>>> int ret, irq; >>>> struct resource *res; >>>> struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); >>>> + struct xhci_plat_priv dwc3_priv; >>> >>> Tying the dwc3 code to the xhci code like this feels really wrong to me, >>> are you sure this is the correct resolution? >>> >>> greg k-h >>> >> >> Can you clarify what feels wrong? The way it's originally implemented >> already tied them in that way. What we're doing here simply takes >> advantage of what xhci-plat glue layer can use to set the xhci quirks. >> With this, we don't have to create new and duplicate DT properties for >> dwc3 and xhci to set some quirks. With the expanding list of dwc3 DT, I >> see this as a plus. > > Ok, still feels odd, but I'll let the xhci maintainer make the decision > on it as it's their code to maintain, not mine :) > Sure. Let me know if you have suggestions to make this better, and I'll try to make the necessary changes. Thanks, Thinh
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index f29a264635aa..a486d7fbb163 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -9,6 +9,7 @@ #include <linux/acpi.h> #include <linux/platform_device.h> +#include <linux/usb/xhci-quirks.h> #include "core.h" @@ -42,6 +43,17 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) return irq; } +static void dwc3_host_init_quirks(struct dwc3 *dwc, struct xhci_plat_priv *priv) +{ + memset(priv, 0, sizeof(*priv)); + + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 190A)) { + priv->quirks |= XHCI_ISOC_BLOCKED_DISCONNECT; + priv->quirks |= XHCI_LIMIT_FS_BI_INTR_EP; + priv->quirks |= XHCI_LOST_DISCONNECT_QUIRK; + } +} + int dwc3_host_init(struct dwc3 *dwc) { struct property_entry props[4]; @@ -49,6 +61,7 @@ int dwc3_host_init(struct dwc3 *dwc) int ret, irq; struct resource *res; struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); + struct xhci_plat_priv dwc3_priv; int prop_idx = 0; irq = dwc3_host_get_irq(dwc); @@ -87,6 +100,14 @@ int dwc3_host_init(struct dwc3 *dwc) goto err; } + dwc3_host_init_quirks(dwc, &dwc3_priv); + + ret = platform_device_add_data(xhci, &dwc3_priv, sizeof(dwc3_priv)); + if (ret) { + dev_err(dwc->dev, "couldn't add platform data to xHCI device\n"); + goto err; + } + memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props)); if (dwc->usb3_lpm_capable)
We can check for host quirks at runtime base on the controller IP and version check. Set the following quirks for the DWC_usb31 IP host mode before creating a platform device for the xHCI driver: * XHCI_ISOC_BLOCKED_DISCONNECT * XHCI_LIMIT_FS_BI_INTR_EP * XHCI_LOST_DISCONNECT_QUIRK Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- drivers/usb/dwc3/host.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)