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 at 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 > > >
