Message-ID: <1518073.zeMq7N85Id@phil>
Date:	Fri, 24 Jun 2016 21:50:24 +0200
From:	Heiko Stuebner <heiko@...ech.de>
To:	William Wu <william.wu@...k-chips.com>
Cc:	gregkh@...uxfoundation.org, balbi@...nel.org,
	linux-rockchip@...ts.infradead.org, briannorris@...gle.com,
	dianders@...gle.com, kever.yang@...k-chips.com,
	huangtao@...k-chips.com, frank.wang@...k-chips.com,
	eddie.cai@...k-chips.com, John.Youn@...opsys.com,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	sergei.shtylyov@...entembedded.com
Subject: Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

Hi William,

Am Dienstag, 21. Juni 2016, 17:11:44 schrieb William Wu:
> On 06/20/2016 10:44 PM, Heiko Stübner wrote:
> > 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@...k-chips.com>
> > 
> > [...]
> > 
> >>>> +Optional clocks:
> >>>> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.
> > 
> > what does this clock do? Also most likely same argument as below.
> 
> Here is partial rk3399 clk tree about usb3:
> 
> aclk_usb3                       7            7   297000000 0 0
>               aclk_usb3_grf                1            1
> 297000000          0 0
>               aclk_usb3_rksoc_axi_perf     1            1
> 297000000          0 0
>               aclk_usb3otg1                1            1
> 297000000          0 0
>               aclk_usb3otg0                1            1
> 297000000          0 0
>               aclk_usb3_noc                1            1
> 297000000          0 0
> 
> from the clk tree, and check with our IC designers, we can see that:
> 1. aclk_usb3 is the parent clk of aclk_usb3****
> 2. aclk_usb3_grf  is used for both otg0 and otg1 grf, and these usb3 grf
> can be set
> to control otg0 and otg1 controller, but not the phy.
> 3. aclk_usb3otg1 is otg1 controller clk,  aclk_usb3otg0 is otg0
> controller clk.
> 4. aclk_usb3_rksoc_axi_perf is the clk for usb3 performance monitor
> module. 5. aclk_usb3_noc is the clk for soc bus interconnect.
> 
> >>>> +  "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.
> 
> Yes, I agree with you. We can remove the aclk_usb3_rksoc_axi_perf right
> 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.
> As I have described above, the "aclk_usb3_grf" is  used for both otg0
> and otg1 grf,
> and these usb3 grf can be set to control otg0 and otg1 controller, but
> not the phy.
> And we have done a test that remove the grf clk, and the result showed
> that usb3
> controller can't work normally. So I think we need the usb3 grf clk.
> 
> So about the usb3 controller clk management, I think it should contain
> the following clk:
> 1.  aclk_usb3otg1
> 2.  aclk_usb3otg0
> 3.  aclk_usb3_grf

correct, aclk_usb3otgX would then be the busclk for each controller if I'm 
not mistaken and the grf clock should also get enabled, like we also plan on 
doing for the vio_grf clock in the display area.

> 4.  aclk_usb3_noc
> For "aclk_usb3_noc", I discuss with our clk manager, the clk is always
> on before,
> but according to upstream maintainer's suggestion, we need to manage the
> noc clk by each module.

can you point me to this discussion? The bus-interconnect is a very separate 
component, which we don't model so far and thus we have all the noc clocks 
simply marked as critical.

As this clock doesn't belong to the actual usb controller block, but said 
separate component, handling it in the controller seems somehow wrong to me.

So my (current) opinion would again be to mark the noc clock as critical for 
the time being.


> and the follow two clk can be removed:
> 1. aclk_usb3
> 2. aclk_usb3_rksoc_axi_perf
> 
> Is it ok?

yep, apart from the noc-clock, this looks great.


Heiko

Powered by blists - more mailing lists