| Message ID | 0b96cb765bb154cf0e83a436e7fed8882f566cf9.1606149078.git.joglekar@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add logic to consolidate TRBs for Synopsys xHC | expand |
On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: > This commit adds the platform device data to setup > the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts > which are PCI devices does not use OF to create platform device > but create xhci-plat platform device at runtime. So > this patch allows parent device to supply the quirk > through platform data. > > Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> > --- > drivers/usb/dwc3/host.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) What changed from previous versions? > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index e195176580de..0434bc8cec12 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -11,6 +11,11 @@ > #include <linux/platform_device.h> > > #include "core.h" > +#include "../host/xhci-plat.h" That feels really wrong. Are you sure about that? thanks, greg k-h
Hi Greg, On 1/4/2021 1:55 PM, Greg Kroah-Hartman wrote: > On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: >> This commit adds the platform device data to setup >> the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts >> which are PCI devices does not use OF to create platform device >> but create xhci-plat platform device at runtime. So >> this patch allows parent device to supply the quirk >> through platform data. >> >> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> >> --- >> drivers/usb/dwc3/host.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) > What changed from previous versions? Resent the patch as it was missed for review by Felipe and I saw your mail to resend the patch if not reviewed. Other two patches from series are picked up by Mathias, this one is remaining for review. > >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >> index e195176580de..0434bc8cec12 100644 >> --- a/drivers/usb/dwc3/host.c >> +++ b/drivers/usb/dwc3/host.c >> @@ -11,6 +11,11 @@ >> #include <linux/platform_device.h> >> >> #include "core.h" >> +#include "../host/xhci-plat.h" > That feels really wrong. Are you sure about that? To use the struct xhci_plat_priv this was included, can you suggest alternative? > > thanks, > > greg k-h Thanks & Regards, Tejas Joglekar
On Mon, Jan 04, 2021 at 09:32:13AM +0000, Tejas Joglekar wrote: > Hi Greg, > On 1/4/2021 1:55 PM, Greg Kroah-Hartman wrote: > > On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: > >> This commit adds the platform device data to setup > >> the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts > >> which are PCI devices does not use OF to create platform device > >> but create xhci-plat platform device at runtime. So > >> this patch allows parent device to supply the quirk > >> through platform data. > >> > >> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> > >> --- > >> drivers/usb/dwc3/host.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > > What changed from previous versions? > > Resent the patch as it was missed for review by Felipe and I saw your mail > > to resend the patch if not reviewed. Other two patches from series are > > picked up by Mathias, this one is remaining for review. Ah, how was I supposed to guess that? :) > >> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >> index e195176580de..0434bc8cec12 100644 > >> --- a/drivers/usb/dwc3/host.c > >> +++ b/drivers/usb/dwc3/host.c > >> @@ -11,6 +11,11 @@ > >> #include <linux/platform_device.h> > >> > >> #include "core.h" > >> +#include "../host/xhci-plat.h" > > That feels really wrong. Are you sure about that? > To use the struct xhci_plat_priv this was included, can you suggest alternative? If that is the "normal" way to do this with the xhci driver, ok, but I would like to get an ack from Mathias for this before taking it. thanks, greg k-h
Hi Mathias, On 1/4/2021 9:13 PM, Greg Kroah-Hartman wrote: > On Mon, Jan 04, 2021 at 09:32:13AM +0000, Tejas Joglekar wrote: >> Hi Greg, >> On 1/4/2021 1:55 PM, Greg Kroah-Hartman wrote: >>> On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: >>>> This commit adds the platform device data to setup >>>> the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts >>>> which are PCI devices does not use OF to create platform device >>>> but create xhci-plat platform device at runtime. So >>>> this patch allows parent device to supply the quirk >>>> through platform data. >>>> >>>> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> >>>> --- >>>> drivers/usb/dwc3/host.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>> What changed from previous versions? >> Resent the patch as it was missed for review by Felipe and I saw your mail >> >> to resend the patch if not reviewed. Other two patches from series are >> >> picked up by Mathias, this one is remaining for review. > Ah, how was I supposed to guess that? :) > >>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>> index e195176580de..0434bc8cec12 100644 >>>> --- a/drivers/usb/dwc3/host.c >>>> +++ b/drivers/usb/dwc3/host.c >>>> @@ -11,6 +11,11 @@ >>>> #include <linux/platform_device.h> >>>> >>>> #include "core.h" >>>> +#include "../host/xhci-plat.h" >>> That feels really wrong. Are you sure about that? >> To use the struct xhci_plat_priv this was included, can you suggest alternative? > If that is the "normal" way to do this with the xhci driver, ok, but I > would like to get an ack from Mathias for this before taking it. > Can you please review this patch which is including the xhci-plat header? Let me know if anything should be modified. Ack if this patch looks ok so Greg can take it. > thanks, > > greg k-h
On 5.1.2021 11.30, Tejas Joglekar wrote: > Hi Mathias, > On 1/4/2021 9:13 PM, Greg Kroah-Hartman wrote: >> On Mon, Jan 04, 2021 at 09:32:13AM +0000, Tejas Joglekar wrote: >>> Hi Greg, >>> On 1/4/2021 1:55 PM, Greg Kroah-Hartman wrote: >>>> On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: >>>>> This commit adds the platform device data to setup >>>>> the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts >>>>> which are PCI devices does not use OF to create platform device >>>>> but create xhci-plat platform device at runtime. So >>>>> this patch allows parent device to supply the quirk >>>>> through platform data. >>>>> >>>>> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> >>>>> --- >>>>> drivers/usb/dwc3/host.c | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>> What changed from previous versions? >>> Resent the patch as it was missed for review by Felipe and I saw your mail >>> >>> to resend the patch if not reviewed. Other two patches from series are >>> >>> picked up by Mathias, this one is remaining for review. >> Ah, how was I supposed to guess that? :) >> >>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>>> index e195176580de..0434bc8cec12 100644 >>>>> --- a/drivers/usb/dwc3/host.c >>>>> +++ b/drivers/usb/dwc3/host.c >>>>> @@ -11,6 +11,11 @@ >>>>> #include <linux/platform_device.h> >>>>> >>>>> #include "core.h" >>>>> +#include "../host/xhci-plat.h" >>>> That feels really wrong. Are you sure about that? >>> To use the struct xhci_plat_priv this was included, can you suggest alternative? >> If that is the "normal" way to do this with the xhci driver, ok, but I >> would like to get an ack from Mathias for this before taking it. >> > Can you please review this patch which is including the xhci-plat header? Let me > > know if anything should be modified. Ack if this patch looks ok so Greg can > > take it. > This doesn't look right. dwc3 shouldn't need to know about xhci platform private structures, besides, this patch now adds the quirk to all xhci platform devices created by dwc3. I haven't touched dwc3 at all, but I'd guess you probably need to add a new entry to the dwc3_pci_id_table[] in dwc3-pci.c, add a device property, and then look for that property in xhci-plat.c, and set the quirk. -Mathias
On 21-01-06 00:35:04, Mathias Nyman wrote: > On 5.1.2021 11.30, Tejas Joglekar wrote: > > Hi Mathias, > > On 1/4/2021 9:13 PM, Greg Kroah-Hartman wrote: > >> On Mon, Jan 04, 2021 at 09:32:13AM +0000, Tejas Joglekar wrote: > >>> Hi Greg, > >>> On 1/4/2021 1:55 PM, Greg Kroah-Hartman wrote: > >>>> On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: > >>>>> This commit adds the platform device data to setup > >>>>> the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts > >>>>> which are PCI devices does not use OF to create platform device > >>>>> but create xhci-plat platform device at runtime. So > >>>>> this patch allows parent device to supply the quirk > >>>>> through platform data. > >>>>> > >>>>> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> > >>>>> --- > >>>>> drivers/usb/dwc3/host.c | 10 ++++++++++ > >>>>> 1 file changed, 10 insertions(+) > >>>> What changed from previous versions? > >>> Resent the patch as it was missed for review by Felipe and I saw your mail > >>> > >>> to resend the patch if not reviewed. Other two patches from series are > >>> > >>> picked up by Mathias, this one is remaining for review. > >> Ah, how was I supposed to guess that? :) > >> > >>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > >>>>> index e195176580de..0434bc8cec12 100644 > >>>>> --- a/drivers/usb/dwc3/host.c > >>>>> +++ b/drivers/usb/dwc3/host.c > >>>>> @@ -11,6 +11,11 @@ > >>>>> #include <linux/platform_device.h> > >>>>> > >>>>> #include "core.h" > >>>>> +#include "../host/xhci-plat.h" > >>>> That feels really wrong. Are you sure about that? > >>> To use the struct xhci_plat_priv this was included, can you suggest alternative? > >> If that is the "normal" way to do this with the xhci driver, ok, but I > >> would like to get an ack from Mathias for this before taking it. > >> > > Can you please review this patch which is including the xhci-plat header? Let me > > > > know if anything should be modified. Ack if this patch looks ok so Greg can > > > > take it. > > > > This doesn't look right. > > dwc3 shouldn't need to know about xhci platform private structures, Then, how dwc3 host let xhci know which xhci quirks it needs to use? > besides, this patch now adds the quirk to all xhci platform devices created by dwc3. > > I haven't touched dwc3 at all, but I'd guess you probably need to add a new entry to > the dwc3_pci_id_table[] in dwc3-pci.c, add a device property, and then look for that > property in xhci-plat.c, and set the quirk. > For non-PCI devices, it uses common platform bus, the specific platform drivers use platform data to pass platform quirks to common driver, so I added below patch. 46034a999c07 usb: host: xhci-plat: add platform data support At platform data, the specific host driver could add xhci quirks and let xhci handle later.
Hello Mathias, On 1/6/2021 4:05 AM, Mathias Nyman wrote: > On 5.1.2021 11.30, Tejas Joglekar wrote: >> Hi Mathias, >> On 1/4/2021 9:13 PM, Greg Kroah-Hartman wrote: >>> On Mon, Jan 04, 2021 at 09:32:13AM +0000, Tejas Joglekar wrote: >>>> Hi Greg, >>>> On 1/4/2021 1:55 PM, Greg Kroah-Hartman wrote: >>>>> On Mon, Jan 04, 2021 at 01:38:43PM +0530, Tejas Joglekar wrote: >>>>>> This commit adds the platform device data to setup >>>>>> the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts >>>>>> which are PCI devices does not use OF to create platform device >>>>>> but create xhci-plat platform device at runtime. So >>>>>> this patch allows parent device to supply the quirk >>>>>> through platform data. >>>>>> >>>>>> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> >>>>>> --- >>>>>> drivers/usb/dwc3/host.c | 10 ++++++++++ >>>>>> 1 file changed, 10 insertions(+) >>>>> What changed from previous versions? >>>> Resent the patch as it was missed for review by Felipe and I saw your mail >>>> >>>> to resend the patch if not reviewed. Other two patches from series are >>>> >>>> picked up by Mathias, this one is remaining for review. >>> Ah, how was I supposed to guess that? :) >>> >>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c >>>>>> index e195176580de..0434bc8cec12 100644 >>>>>> --- a/drivers/usb/dwc3/host.c >>>>>> +++ b/drivers/usb/dwc3/host.c >>>>>> @@ -11,6 +11,11 @@ >>>>>> #include <linux/platform_device.h> >>>>>> >>>>>> #include "core.h" >>>>>> +#include "../host/xhci-plat.h" >>>>> That feels really wrong. Are you sure about that? >>>> To use the struct xhci_plat_priv this was included, can you suggest alternative? >>> If that is the "normal" way to do this with the xhci driver, ok, but I >>> would like to get an ack from Mathias for this before taking it. >>> >> Can you please review this patch which is including the xhci-plat header? Let me >> >> know if anything should be modified. Ack if this patch looks ok so Greg can >> >> take it. >> > This doesn't look right. > > dwc3 shouldn't need to know about xhci platform private structures, > besides, this patch now adds the quirk to all xhci platform devices created by dwc3. As per understanding the TRB cache is present and used in all dwc3 controllers, so we would need the quirk for all. > I haven't touched dwc3 at all, but I'd guess you probably need to add a new entry to > the dwc3_pci_id_table[] in dwc3-pci.c, add a device property, and then look for that > property in xhci-plat.c, and set the quirk. The first approach was the same as you mentioned where I added device property with dt-binding for enabling the quirk, but Rob was not OK to add new bindings for each new quirk. With PCI devices compatible string would not work and dt bindings solution was not given clearance, I referred other approach taken by Peter in commit ed22764847e8100f0af9af91ccfa58e5c559bd47, (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ed22764847e8100f0af9af91ccfa58e5c559bd47) where he used xhci platform data to pass the quirk. I followed the same approach and added the quirk. I think another way might be to make the xhci platform data structure, public so dwc3 can use it ? > -Mathias Thanks & Regards, Tejas Joglekar
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index e195176580de..0434bc8cec12 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -11,6 +11,11 @@ #include <linux/platform_device.h> #include "core.h" +#include "../host/xhci-plat.h" + +static const struct xhci_plat_priv dwc3_pdata = { + .quirks = XHCI_SG_TRB_CACHE_SIZE_QUIRK, +}; static int dwc3_host_get_irq(struct dwc3 *dwc) { @@ -87,6 +92,11 @@ int dwc3_host_init(struct dwc3 *dwc) goto err; } + ret = platform_device_add_data(xhci, &dwc3_pdata, sizeof(dwc3_pdata)); + 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)
This commit adds the platform device data to setup the XHCI_SG_TRB_CACHE_SIZE_QUIRK quirk. DWC3 hosts which are PCI devices does not use OF to create platform device but create xhci-plat platform device at runtime. So this patch allows parent device to supply the quirk through platform data. Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> --- drivers/usb/dwc3/host.c | 10 ++++++++++ 1 file changed, 10 insertions(+)