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

[William Wu <william.wu@rock-chips.com>]

Dear Heiko,

On 06/29/2016 12:41 AM, Heiko Stuebner wrote:
> Hi William,
>
> Am Dienstag, 28. Juni 2016, 11:18:04 schrieb William Wu:
>>>> 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?
> nope ... the consensus for the vio_grf clock was that the driver accessing
> it should control it. So for the usb3_grf clock this would be your dwc3-
> based driver being responsible for the grf-clock.
Ah, I misunderstand before. I'll control usb3_grf in dwc3 driver.
>
>
>> 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?
> you should keep the usb3-grf clock in the dts all the time.
OK, I'll keep it in the dts.
>
>
>>>> 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~
> The difference as I see it is, that the GRF parts are _part_ of the usb3
> controller, while the interconnect really is a separate component,
> connecting the controller to the rest of the system.
>
> ----------	
> |  USB3  |----[Interconnect]----[rest of the system]
> | [+GRF] |	
> ----------	
>
> So the controller binding + driver should handle all clocks it needs to
> operate. We currently don't model and handle the separate interconnect
> though, so that noc clock should be critical for the time being.
Thanks for your professional explanation. Now I think I have
more clearly understood the clocks. And I'll upload a new
patch later.
>
> Heiko
>
>
>
>