Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Heiko Stübner]

Hi William,

Am Freitag, 17. Juni 2016, 17:18:59 schrieb William Wu:
> On 06/17/2016 07:15 AM, Heiko Stübner wrote:
> > Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
> >> This patch adds the devicetree documentation required for Rockchip
> >> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
> >> 
> >> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> >> and host mode (SS, HS, FS, LS).
> >> 
> >> Signed-off-by: William Wu <william.wu@xxxxxxxxxxxxxx>

[...]

> >> +Optional clocks:
> >> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.

what does this clock do? Also most likely same argument as below.


> >> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all
> >> platforms.
> > 
> > The clock names looks pretty strange. What are they for? Especially as
> > nothing seems to use them right now.
> 
> "aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor
> module, you can refer to the GRF_USB3_PERF_xxx. And we don't use the usb3
> performance monitor control registers right now.

ok, then I'd suggest not defining the clock for now.

For one, there are more perf blocks in the GRF (usb3, pcie, hdcp22, gmac, gpu, 
etc) which all seem to share a somewhat similar design, so they will maybe 
result in a separate driver of some form for the performance monitors.

And secondly, it is somewhat easy to add new optional properties, but you 
cannot remove anything defined previously. So if we later decide to handle all 
the performance monitors differently, you can't remove that clock from the 
binding again. (or at least only with quite a bit of hassle).

So as this clock isn't used at all yet, I guess it should not get included 
now.


> >> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
> > 
> > for my own education, which part of the GRF does this clock supply?
> 
> "aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX

Hmm, this looks more like it belongs to the otg phy? 
Anyway, also seems unused right now, so same argument as above applies here.


Heiko