lkml.org 
[lkml]   [2016]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@rock-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
>
>
>


\
 
 \ /
  Last update: 2016-06-28 06:01    [W:3.265 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site