Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753046AbbKXCji (ORCPT ); Mon, 23 Nov 2015 21:39:38 -0500 Received: from mx0b-0016f401.pphosted.com ([67.231.156.173]:45942 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752566AbbKXCjg (ORCPT ); Mon, 23 Nov 2015 21:39:36 -0500 Date: Tue, 24 Nov 2015 10:35:04 +0800 From: Jisheng Zhang To: Sebastian Hesselbarth , CC: , , , , , , , , , , , , Subject: Re: [PATCH v2 6/6] arm64: dts: berlin4ct: add pll and clock nodes Message-ID: <20151124103504.0a07b258@xhacker> In-Reply-To: <20151123165444.740b457c@xhacker> References: <1448008952-1787-1-git-send-email-jszhang@marvell.com> <1448008952-1787-7-git-send-email-jszhang@marvell.com> <564F8B73.7070403@gmail.com> <20151123152158.483aa6b5@xhacker> <5652CEB2.9030902@gmail.com> <20151123165444.740b457c@xhacker> X-Mailer: Claws Mail 3.13.0 (GTK+ 2.24.28; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2015-11-24_01:,, signatures=0 X-Proofpoint-Spam-Details: rule=inbound_notspam policy=inbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1507310000 definitions=main-1511240046 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5080 Lines: 133 Dear Sebastian, On Mon, 23 Nov 2015 16:54:44 +0800 Jisheng Zhang wrote: > On Mon, 23 Nov 2015 09:30:42 +0100 > Sebastian Hesselbarth wrote: > > > On 23.11.2015 08:21, Jisheng Zhang wrote: > > > On Fri, 20 Nov 2015 22:06:59 +0100 > > > Sebastian Hesselbarth wrote: > > >> On 20.11.2015 09:42, Jisheng Zhang wrote: > > >>> Add syspll, mempll, cpupll, gateclk and berlin-clk nodes. > > >>> > > >>> Signed-off-by: Jisheng Zhang > > >>> --- > > [...] > > >>> + syspll: syspll { > > >>> + compatible = "marvell,berlin-pll"; > > >>> + reg = <0xea0200 0x14>, <0xea0710 4>; > > >>> + #clock-cells = <0>; > > >>> + clocks = <&osc>; > > >>> + bypass-shift = /bits/ 8 <0>; > > >>> + }; > > >>> + > > >>> + gateclk: gateclk { > > >>> + compatible = "marvell,berlin4ct-gateclk"; > > >>> + reg = <0xea0700 4>; > > >>> + #clock-cells = <1>; > > >>> + }; > > >>> + > > >>> + clk: clk { > > >>> + compatible = "marvell,berlin4ct-clk"; > > >>> + reg = <0xea0720 0x144>; > > >> > > >> Looking at the reg ranges, I'd say that they are all clock related > > >> and pretty close to each other: > > >> > > >> gateclk: reg = <0xea0700 4>; > > >> bypass: reg = <0xea0710 4>; > > >> clk: reg = <0xea0720 0x144>; > > > > > > Although these ranges sit close, but we should represent HW structure as you > > > said. > > > > Then how do you argue that you have to share the gate clock register > > with every PLL? The answer is quite simple: You are not separating by > > HW either but existing SW API. > > No, PLLs don't share register any more. You can find what all clock nodes will > look like in: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/387322.html > > > > > If you would really want to just describe the HW, then you'd have to > > have a single node for _all_ clocks that get controlled by 0xea0700/0x4, > > feed some 32+ clocks into the node, and out again. Obviously, this > > isn't what we want, right? > > I represented the HW by "kind", for example, gateclks, common-clks, pllclk. > And let common PLLs follow this rule as well: > > one node for all common plls > > reg <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> > > > > > So, the idea of berlin2 sysctrl nodes (and similar other SoCs) is: Some > > SoCs just dump some functions into a bunch of registers with no > > particular order. We'd never find a good representation for that in DT, > > so we don't bother to try but let the driver implementation deal with > > the mess. Using "simple-mfd" is a nice solution to scattered registers > > please have a look at it and come up with a cleaner separation for bg4 > > clock. > > > > > First of all, let me describe the clks/plls in BG4CT. BG4CT contains: > > > > > > two kinds of PLL: normal PLL and AVPLL. These PLLs are put with their users > > > together. For example: mempll pll registers <0xf7940034, 0x14> is put together > > > with mem controller registers. AVPLL control registers are put with AV devices. > > > > Why didn't you choose to have a memory-controller node that provides > > mempll clock then? I am open to having multiple nodes providing clocks > > but having a node for every clock in any subsystem is something I'll > > not even think about. > > OK. As you said, "SoCs just dump some functions into a bunch of registers with > no particular order", BG4CT is now cleaner, all common-clks are put together, > gate-clks are put together, pllclks are put together, only the common PLLs > are dumped here and there. So how about representing the HW by "kind", one > node for common plls, one node for gateclks, one node for common clks and > one node for pllclks? > > pll: pll { > compatible = "marvell,berlin4ct-pll"; > reg = <0x922000 0x14>, <0x940034 0x14>, <0xea0200 0x14> > #clock-cells = <0>; should be "#clock-cells = <1>;" > clocks = <&osc>; > }; > > pllclk: pllclk { > compatible = "marvell,berlin4ct-pllclk"; > reg = <0xea0710 4> > #clock-cells = <1>; > }; > > gateclk: gateclk { > compatible = "marvell,berlin4ct-gateclk"; > reg = <0xea0700 4>; > #clock-cells = <1>; > }; > > clk: clk { > compatible = "marvell,berlin4ct-clk"; > reg = <0xea0720 0x144>; > #clock-cells = <1>; > clocks = <&pllclk SYSPLLCLK>; > }; > there's no a node for every clock with this proposal, all clks/plls are separated by "kind". Is this OK for you? thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/