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

[Felipe Balbi]

Hi,

Ran Wang <ran.wang_1@xxxxxxx> writes:
> Hi Felipe,
>
> On Thursday, May 30, 2019 17:09, Ran Wang wrote:
>> 
>> <snip>
>> > >> >> >  /* 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.
>> 
>> I think DATA_RD  should be a macro, right? So, where I can put its define?
>> Create a dwc3.h in include/dt-bindings/usb/ ?
>
> Could you please give me some advice here? I'd like to prepare next version patch after
> getting this settled.
>
>> Another question about this remain open is: DWC3 data book's Table 6-5 Cache
>> Type Bit Assignments show that bits definition will differ per MBUS_TYPEs as
>> below:
>> ----------------------------------------------------------------
>>  MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]
>>  ----------------------------------------------------------------
>>  AHB      |Cacheable     |Bufferable   |Privilegge |Data
>>  AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable
>>  AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable
>>  AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable
>>  Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI
>>  ----------------------------------------------------------------
>>  Note: The AHB, AXI3, AXI4, and PCIe busses use different names for certain
>>  signals, which have the same meaning:
>>    Bufferable = Posted
>>    Cacheable = Modifiable = Snoop (negation of No Snoop)
>> 
>> For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to use
>> snps,cache-type = <DATA_RD  "write allocate">, to cover all MBUS_TYPE?
>> (you can notice that AHB and AXI3's cacheable are on different bit) Or I just need
>> to handle AXI3 case?
>
> Also on this open. Thank you in advance.

You could pass two strings and let the driver process them. Something
like:

	snps,cache_type = <"data_wr" "write allocate">, <"desc_rd" "cacheable">...

And so on. The only thing missing is for the mbus_type to be known by
the driver. Is that something we can figure out on any of the HWPARAMS
registers or does it have to be told explicitly?

Another option would be to pass a string followed by one hex digit for
the bits:

	snps,cache_type = <"data_wr" 0x8>, <"desc_rd" 0x2>...;

Then we don't need to describe mbus_type since the bits are what matters.

Rob, any comments?

-- 
balbi

Attachment: signature.asc
Description: PGP signature