Message-ID: <5771EC6C.3070707@rock-chips.com>
Date:	Tue, 28 Jun 2016 11:18:04 +0800
From:	William Wu <william.wu@...k-chips.com>
To:	Heiko Stuebner <heiko@...ech.de>
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

Dear Heiko,

On 06/25/2016 03:50 AM, Heiko Stuebner wrote:
> 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.
OK, so  aclk_usb3_grf should be marked as critical, right?

I found that most of the grf clocks haven't been marked as critical, 
apart from vio_grf. So may I keep the aclk_usb3_grf in usb3 dts, and 
remove it after clock manager adds it to critical clocks?
>
>> 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.
Sorry, it seems that I get the new information about clk management too 
late.:-)

There's no dedicated discussion about noc clk, but similar to grf clock, 
please refer to "https://patchwork.kernel.org/patch/9171467/" for add 
pclk_vio_grf to critical clock on the RK3399, and you have agreed on 
that mark vio grf clk as critical. So I agree with your opinion, thanks~
>
>> 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
Best regards,
      William Wu
>
>
>

Powered by blists - more mailing lists