From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752074AbcF1DS3 (ORCPT ); Mon, 27 Jun 2016 23:18:29 -0400 Received: from regular1.263xmail.com ([211.150.99.131]:42139 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751740AbcF1DS1 (ORCPT ); Mon, 27 Jun 2016 23:18:27 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: wulf@rock-chips.com X-FST-TO: heiko@sntech.de X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: wulf@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <5771EC6C.3070707@rock-chips.com> Date: Tue, 28 Jun 2016 11:18:04 +0800 From: William Wu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Heiko Stuebner CC: gregkh@linuxfoundation.org, balbi@kernel.org, linux-rockchip@lists.infradead.org, briannorris@google.com, dianders@google.com, kever.yang@rock-chips.com, huangtao@rock-chips.com, frank.wang@rock-chips.com, eddie.cai@rock-chips.com, John.Youn@synopsys.com, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, sergei.shtylyov@cogentembedded.com Subject: Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation References: <1464870896-25612-1-git-send-email-william.wu@rock-chips.com> <4674233.DVms812T4a@diego> <576904D0.6060005@rock-chips.com> <1518073.zeMq7N85Id@phil> In-Reply-To: <1518073.zeMq7N85Id@phil> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> [...] >>> >>>>>> +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 > > >