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
