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
