Hi, On Sun, Sep 28, 2014 at 05:18:58PM +0800, Huang Rui wrote: > > > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > > > Sent: Friday, September 26, 2014 5:54 PM > > > > > > > > On Fri, Sep 26, 2014 at 11:18:48PM +0000, Paul Zimmerman wrote: > > > > > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > > > > > Sent: Friday, September 26, 2014 2:40 PM > > > > > > > > > > > > On Fri, Sep 26, 2014 at 08:57:19PM +0000, Paul Zimmerman wrote: > > > > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > > > > > > > index 0fcc0a3..8277065 100644 > > > > > > > > > --- a/drivers/usb/dwc3/gadget.c > > > > > > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > > > > > > @@ -2635,6 +2635,7 @@ static irqreturn_t dwc3_interrupt(int irq, void *_dwc) > > > > > > > > > */ > > > > > > > > > int dwc3_gadget_init(struct dwc3 *dwc) > > > > > > > > > { > > > > > > > > > + u32 reg; > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req), > > > > > > > > > @@ -2689,6 +2690,13 @@ int dwc3_gadget_init(struct dwc3 *dwc) > > > > > > > > > if (ret) > > > > > > > > > goto err4; > > > > > > > > > > > > > > > > > > + if (dwc->quirks & DWC3_AMD_NL_PLAT) { > > > > > > > > > + reg = dwc3_readl(dwc->regs, DWC3_DCTL); > > > > > > > > > + reg |= DWC3_DCTL_LPM_ERRATA(0xf); > > > > > > > > > > > > > > > > weird, why would Synopsys put this here ? It seems like this is only > > > > > > > > useful when LPM Errata is enabled and that's, apparently, a host-side > > > > > > > > thing. > > > > > > > > > > > > > > > > Paul, can you comment ? > > > > > > > > > > > > > > These bits contribute to how the device responds to an LPM transaction > > > > > > > from the host. If DCFG.LPMCap=1, they set the BESL value above which > > > > > > > the core will send a NYET. If DCFG.LPMCap=0, they have no effect. So > > > > > > > it's definitely a device-side thing, but only if the core is configured > > > > > > > with LPM Errata support enabled. > > > > > > > > > > > > right, and how can SW detect if LPM Errata was enabled ? From the host > > > > > > point of view, we can check bit 20 of xHCI capability register. What > > > > > > about device ? I can't seem to find anything :-s > > > > > > > > > > I just talked to one of our RTL designers. You're right, there is no > > > > > way to tell from the Device registers alone whether the controller is > > > > > configured with LPM Errata support or not. > > > > > > > > > > You can tell for sure if it is *not* enabled, by checking GSNPSID, and > > > > > if the version is earlier than 2.40a, the feature wasn't available. > > > > > > > > alright, we can use this for something like: > > > > > > > > WARN(rev < 2.40a && (flags & LPM_ERRATA || of_property_read_bool("lpm-errata")), > > > > "LPM Errata not available on dwc3 revisions <= 2.40a\n"); > > > > > > > > > So for Device-mode only controllers, I guess you will need a DT property > > > > > for this. > > > > > > > > right, DT property and platform_data feature flag, or something. I don't > > > > wanna call it a quirk though, although it _is_ an erratum WRT LPM > > > > support. Dunno. Any strong feelings ? > > > > > > Well, it's called LPM Errata because the feature was added to the USB > > > spec as an erratum. It's not an erratum to our controller. But I don't > > > have any strong feelings about how the driver support is implemented. > > > > how about adding a "has_lpm_erratum" to struct dwc3 which gets > > initialized either through pdata or DT ? Then above WARN() would become: > > > > WARN(dwc->revision < DWC3_REVISION_240A && dwc->has_lpm_erratum", > > "LPM Erratum not available on dwc3 revisisions < 2.40a\n"); > > > > Then we're not really calling it a quirk. In fact Huang, when respining > > your series, let's convert your quirk bits into single-bit fields inside > > struct dwc3 and struct platform_data. Like so: > > > > Looks like a good suggestion. > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 4d4e854..e233ce1 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -695,11 +695,13 @@ static int dwc3_probe(struct platform_device *pdev) > > > > if (node) { > > dwc->maximum_speed = of_usb_get_maximum_speed(node); > > + dwc->has_lpm_erratum = of_property_read_bool(node, "snps,has-lpm-erratum"); > > > > dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); > > dwc->dr_mode = of_usb_get_dr_mode(node); > > } else if (pdata) { > > dwc->maximum_speed = pdata->maximum_speed; > > + dwc->has_lpm_erratum = pdata->has_lpm_erratum; > > > dwc->needs_fifo_resize = pdata->tx_fifo_resize; > > dwc->dr_mode = pdata->dr_mode; > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 66f6256..23e1504 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -661,6 +661,8 @@ struct dwc3_scratchpad_array { > > * @ep0_bounced: true when we used bounce buffer > > * @ep0_expect_in: true when we expect a DATA IN transfer > > * @has_hibernation: true when dwc3 was configured with Hibernation > > + * @has_lpm_erratum: true when core was configured with LPM Erratum. Note that > > + * there's now way for software to detect this in runtime. > > * @is_selfpowered: true when we are selfpowered > > * @needs_fifo_resize: not all users might want fifo resizing, flag it > > * @pullups_connected: true when Run/Stop bit is set > > @@ -764,6 +766,7 @@ struct dwc3 { > > unsigned ep0_bounced:1; > > unsigned ep0_expect_in:1; > > unsigned has_hibernation:1; > > + unsigned has_lpm_erratum:1; > > unsigned is_selfpowered:1; > > unsigned needs_fifo_resize:1; > > unsigned pullups_connected:1; > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 68497b3..112352e 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -1577,6 +1577,13 @@ static int dwc3_gadget_start(struct usb_gadget *g, > > } > > dwc3_writel(dwc->regs, DWC3_DCFG, reg); > > > > + if (dwc->has_lpm_erratum) { > > + reg = dwc3_readl(dwc->regs, DWC3_DCTL); > > + /* REVISIT should this be configurable ? */ > > + reg |= DWC3_DCTL_LPM_ERRATA(0xf); > > + dwc3_writel(dwc->regs, DWC3_DCTL, reg); > > + } > > + > > dwc->start_config_issued = false; > > > > /* Start with SuperSpeed Default */ > > diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h > > index 7db34f0..4bcec7c 100644 > > --- a/drivers/usb/dwc3/platform_data.h > > +++ b/drivers/usb/dwc3/platform_data.h > > @@ -24,4 +24,6 @@ struct dwc3_platform_data { > > enum usb_device_speed maximum_speed; > > enum usb_dr_mode dr_mode; > > bool tx_fifo_resize; > > + > > + unsigned has_lpm_erratum:1; > > }; > > > > note that I have also moved DCTL access from gadget_init to > > gadget_start. > > > > Yes, I see. Will update it into V2. > > A question, the dwc3 controller is the PCI-E device in my platform, > but the class code of PCI header is 0x0c0330, the same with xHC. > That's because it need to meet the windows enviroment. The dwc3 > controller acted as only host mode to bind with windows xhci driver. > But on linux, sometimes, we auto-bind with xhci driver as well (class > code 0x0c0330) despite we use Pid/Vid on dwc3 driver. Then I need > rmmod xhci_hcd module and modprobe dwc3_pci module manually. > > Any idea to resolve this issue? that probably won't be the case on final Silicon. I reckon you're using Synopsys' HAPS for development, right ? That's just how Synopsys designed the PCIe wrapper to make it easy to test with xHCI. Please make sure to not use the same PCI header as xHCI on final silicon, otherwise SW engineers' lifes will become more difficult :-) Meanwhile, you can just blacklist xhci under /etc/modprobe.d/. Create a new file (say xhci-blacklist.conf) and add this to it: blacklist xhci-hcd Save and restart. xhci-hcd will never load by default anymore, but you can still manually load it. Heikki, can you confirm if your DWC3 incarnations present this same "feature" ? :-) -- balbi
Attachment:
signature.asc
Description: Digital signature
