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:

quoted

On 23.11.2015 08:21, Jisheng Zhang wrote:  

quoted

On Fri, 20 Nov 2015 22:06:59 +0100
Sebastian Hesselbarth wrote:    

quoted

On 20.11.2015 09:42, Jisheng Zhang wrote:    

quoted

Add syspll, mempll, cpupll, gateclk and berlin-clk nodes.

Signed-off-by: Jisheng Zhang <redacted>
---    

[...]  

quoted

quoted

quoted

+		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

quoted

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>

quoted

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.
  

quoted

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