HI Felipe, On Tuesday, May 28, 2019 18:20, Felipe Balbi wrote: > > 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? I think I have answered this as blow: to fix issue we found. > >> >> b) WHY do we need another DT property for this? I agreed with your suggestion of using ' snps,cache-type = <foobar "cacheable">, ...' > >> >> c) WHAT does this mean for PCI devices? According to DWC3 data book, I think this (PCI) mean to the case of 'master bus type = Native' The data book describes this feature as 'system bus DMA option for the master bus, which may be configured as AHB, AXI, or Native.' On Table 6-5, it says when MBUS_TYPE is Native, the definition of 4 transfer types control bits [3-0] is 'Same as AXI'. However, as to the code implementation to be generic to both PCI and AXI, I admit I don't have a perfect solution so far, only 2 proposals with concerns: a. Create another module driver like dwc3-exynos.c (arch/arm/boot/dts/wxynos54xx.dtsi) to contain above programming code. However, it will touch the same reg range of DWC3 I think this is not good. b. Add #ifdef CONFIG_ARCH_LAYERSCAPE in drivers/usb/dwc3/core.c to constrain hacking code can only take effect for Layerscape (AXI case). I know it look ugly. Do you have any better advice on this (besides changed power on default value from HW perspective)? > >> > > >> > 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 Yes, noted. > >> >> > >> >> Also, I think dma_coherent is likely not the best name for this property. OK > >> >> > >> >> 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? Several Layerscape platforms like LS1043ARDB, LS1046ARDB, etc. are already on the market and have this issue. So I have to work out a SW patch to fix them. > >> > > >> > 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? Already in the market, need SW fix. > >> >> > /* 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 If I still missed any question, please let me know, thanks for your time. Regards, Ran