RE: [PATCH] usb: dwc3: Enable the USB snooping

[Ran Wang <ran.wang_1@nxp.com>]

Hi Felipe,

    Sorry for the late reply:

On Wednesday, November 15, 2017 18:23, Felipe Balbi wrote:
> 
> Hi,
> 
> Ran Wang <ran.wang_1@nxp.com> writes:
> >> Ran Wang <ran.wang_1@nxp.com> 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

> >> > +
> >> >  /* 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">

> > 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.
> 
> >> In any
> >> case, I still want to know why do you really need this? What's the reason?
> >> What happens if you don't fix GSBUSCFG0? What's the value you have
> >> there by default? Why isn't that default good enough?
> >
> > So far the Layerscape SoC (such as LS1088A) has enabled this feature
> > and I have tested it. Once we add dma-coherent on DTS without this
> > Patch, dwc3 will fail on device enumeration as below:
> > [   15.124031] xhci-hcd xhci-hcd.0.auto: Error while assigning device slot ID
> > [   15.130912] xhci-hcd xhci-hcd.0.auto: Max number of devices this xHCI host
> supports is 127.
> > [   15.139268] usb usb1-port1: couldn't allocate usb_device
> 
> okay, so without these changes, your host doesn't work. What is the default
> value on your platform without these changes? (revert patch, boot platform, let
> it fail, get register output from our regdump in debugfs)

The register I dumped is as below (partial register):
root@ls1046ardb:/sys/kernel/debug/2f00000.usb# cat regdump     
GSBUSCFG0 = 0x00100080
GSBUSCFG1 = 0x00000700
GTXTHRCFG = 0x00000000
GRXTHRCFG = 0x00000000
GCTL = 0x30c11004
GEVTEN = 0x00000000
GSTS = 0x3e800001
GUCTL1 = 0x0000018a
GSNPSID = 0x5533280a
GGPIO = 0x00000000
GUID = 0x00050100
 GUCTL = 0x0200c010
GBUSERRADDR0 = 0x00000000
GBUSERRADDR1 = 0x00000000
GPRTBIMAP0 = 0x00000000
GPRTBIMAP1 = 0x00000000
...

The value of GSBUSCFG0 is the same to what I dump in bootloader(U-boot)
Which mean that's the default value.

Regards,
Ran