Hi, Ran Wang <ran.wang_1@xxxxxxx> writes: > Hi Felipe, > > Sorry for the late reply: > > On Wednesday, November 15, 2017 18:23, Felipe Balbi wrote: that's 1.5 year ago. I really don't remember the details of this conversation >> Ran Wang <ran.wang_1@xxxxxxx> writes: >> >> Ran Wang <ran.wang_1@xxxxxxx> writes: >> >> > Add support for USB3 snooping by asserting bits in register >> >> > DWC3_GSBUSCFG0 for data and descriptor. >> >> >> >> we know *how* to enable a feature :-) It's always the same, you >> >> fiddle with some registers and it works. What you failed to tell us is: >> >> >> >> a) WHY do you need this? >> >> b) WHY do we need another DT property for this? >> >> c) WHAT does this mean for PCI devices? >> > >> > So far I cannot have the answer for you, will get you back after some >> > discussion with my colleagues. >> >> IOW, you have no idea why you need this, right? We're not patching things for >> the sake of patching things. We need to understand what these changes mean >> to the HW before we send out a patch publicly. >> >> Remember that the moment a patch like this is accepted, it has the potential of >> changing behavior for *ALL* users. >> >> >> > + } >> >> > + >> >> > + dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg); >> >> >> >> this will *always* read and write GSBUSCFG0 even for those platforms >> >> which don't need to change anything on this register. You should just >> >> bail out early if !dwc->dma_coherent >> >> >> >> Also, I think dma_coherent is likely not the best name for this property. >> >> >> >> Another question is: Why wasn't this setup properly during >> >> coreConsultant instantiation of the RTL? Do you have devices on the >> >> market already that need this or is this some early FPGA model or test-only >> ASIC? >> > >> > Yes, you are right. Actually I thought that all dwc3 IP will have >> > this register, and it can be controlled by DTS property. >> >> they all *have* the register, however, it's sort of expected that RTL engineer will >> setup good defaults when instantiating the RTL using SNPS' >> coreConsultant tool. >> >> Does your platform work without this patch? > > On Layerscape SoC (such as LS1088A, LS1046A, LS1043A) When I add 'dma-coherent' > to USB nodes without this patch, dwc3 will fail on device enumeration as below: > [ 3.610620] xhci-hcd xhci-hcd.2.auto: WARNING: Host System Error > [ 3.630609] usb usb2-port1: couldn't allocate usb_device Right, and same as before: are these devices in the market or are you dealing with pre-silicon prototypes? >> >> > /* Global Debug Queue/FIFO Space Available Register */ >> >> > #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f) >> >> > #define DWC3_GDBGFIFOSPACE_TYPE(n) (((n) << 5) & 0x1e0) >> >> > @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array { >> >> > * 3 - Reserved >> >> > * @imod_interval: set the interrupt moderation interval in 250ns >> >> > * increments or 0 to disable. >> >> > + * @dma_coherent: set if enable dma-coherent. >> >> >> >> you're not enabling dma coherency, you're enabling cache snooping. >> >> And this property should describe that. Also, keep in mind that >> >> different devices may want different cache types for each of those >> >> fields, so your property would have to be a lot more complex. Something like: >> >> >> >> snps,cache-type = <foobar "cacheable">, <baz "cacheable">, ... >> >> >> >> Then driver would have to parse this properly to setup GSBUSCFG0. > > According to the DesignWare Cores SuperSpeed USB 3.0 Controller Databook (v2.60a), > it has described Type Bit Assignments for all supported master bus type: > AHB, AXI3, AXI4 and Native. I found the bit definition are different among them. > So, for the example you gave above, feel a little bit confused. > Did you mean: > snps,cache-type = <DATA_RD "write allocate">, <DESC_RD "cacheable">, <DATA_WR "bufferable">, <DESC_WR "read allocate"> yeah, something like that. >> > Got it, learn a lot, need more time to digest and test, thanks for >> > your patiently explanation. >> >> no problem, please figure out the answers to my previous questions, without >> which I can't accept your patch. ^^^ I still don't have all the answers -- balbi