Re: [PATCH v3 08/10] Doc: usb: ci-hdrc-usb2: add tx(rx)-burst-config-dword for binding doc

[Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

On Fri, Aug 14, 2015 at 12:02:37PM +0200, Lucas Stach wrote:
> Am Freitag, den 14.08.2015, 16:40 +0800 schrieb Peter Chen:
> > On Fri, Aug 14, 2015 at 10:37:30AM +0200, Lucas Stach wrote:
> > > Am Freitag, den 07.08.2015, 15:15 +0800 schrieb Peter Chen:
> > > > It is used to override the default setting for burst size, changing
> > > > burst size takes effect only when the SBUSCFG.AHBBRST = 0.
> > > > 
> > > > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > index d52a747..d71ef07 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > @@ -37,6 +37,14 @@ Optional properties:
> > > >    property is used to change AHB burst configuration, check the chipidea
> > > >    spec for meaning of each value. If this property is not existed, it
> > > >    will use the reset value.
> > > > +- tx-burst-size-dword: it is vendor dependent, the tx burst size in dword
> > > > +  (4 bytes), This register represents the maximum length of a the burst
> > > > +  in 32-bit words while moving data from system memory to the USB
> > > > +  bus, changing this value takes effect only the SBUSCFG.AHBBRST is 0.
> > > 
> > > The last bits about SBUSCFG.AHBBRST don't make any sense in the DT
> > > binding. The DT writer doesn't know about the SBUSCFG.AHBBRST value and
> > > should not care either.
> > > If someone sets the burst size to something
> > > non-standard in the DT, the driver should make sure to enable to
> > > necessary bits to make this setting take effect.
> > 
> > The SBUSCFG.AHBBRST property description is just above this one,
> > and the user should read the spec for changing these system
> > configuration parameters.
> > 
> This is not clear from the description. This should read something like
> "The value of this property will only take effect if property
> ahb-burst-config is set to 0."

Thanks, will change.

> 
> The DT binding should be coherent and understandable without reading the
> DW USB spec and/or the code. Please take some care about this.
> 
> > This requirement is from the spec, and the code logic makes sure
> > the burst size changes only when SBUSCFG.AHBBRST is 0, since the
> > hardware will only change burst size if SBUSCFG.AHBBRST is 0.
> > 
> > > 
> > > Also both those descriptions are missing a description to what value the
> > > burst sizes will be set if the DT properties are not found. If it's
> > > implementation defined spell this out in the doc.
> > 
> > It is optional property, if without this property, it will use the
> > default value.
> > 
> Then specify this in the binding. Again, the binding is a spec that one
> should be able to understand without reading the code.
> 
> "If this property is missing the reset defaults of the hardware
> implementation will be used."
> 

Thanks, will change.

> > > 
> > > Are there really cases where it makes sense to set RX and TX burst sizes
> > > to different values?
> > 
> > Yes, the default burst size is the same for all SoCs, but the bus
> > burst size may be larger for some SoCs.
> > 
> This question wasn't meant to dispute the RX/TX burst size
> configuration, but I'm questioning if it really makes sense to set them
> to different values. Do we really need 2 properties for this, or is 1
> enough?
> 
> Do you see any use case where someone would like to do something like:
> tx-burst-size-dword = <0x8>;
> rx-burst-size-dword = <0x10>;
> 

For Freelscae i.mx current design, the tx/rx burst size are always the same,
but not sure if it is always true for others or for future design.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html