[PATCH v4] powerpc/mpc85xx: Update the clock device tree nodes

[Tang Yuantian-B29983]

> > > On Wed, 2013-09-11 at 20:31 -0500, Tang Yuantian-B29983 wrote:
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: 2013年9月12日 星期四 9:10
> > > > > To: Tang Yuantian-B29983
> > > > > Cc: galak at kernel.crashing.org; linuxppc-dev at lists.ozlabs.org;
> > > > > devicetree at vger.kernel.org; Li Yang-Leo-R58472
> > > > > Subject: Re: [PATCH v4] powerpc/mpc85xx: Update the clock device
> > > > > tree nodes
> > > > >
> > > > > This description of "reg" is overly specific (assumes how the
> > > > > parent node's ranges are set up), incomplete (there's a size as
> > > > > well as the offset), and does not apply to the clockgen node
> > > > > itself (you probably shouldn't lump them together like this).
> > > > >
> > > > Do you mean I should explain the REG of clockgen and its child
> > > > node
> > > respectively?
> > > >
> > > > > > +- clocks : shall be the input parent clock phandle for the
> clock.
> > > > >
> > > > > Not required on the clockgen node
> > > > >
> > > > Required by child node of clockgen.
> > >
> > > My point is that you're lumping several different types of nodes
> > > together with one binding, when some parts of the binding are not
> > > applicable to the clockgen node.
> > >
> > Not several, just two types of nodes.
> > One is clockgen node, the other is PLL and mux nodes.
> 
> clockgen + PLL + mux = 3 = several :-)
> 
> > The reason they lumped together is that the clockgen node is not only
> > IP block Node but also a clock provider node
> 
> I don't understand why that merits lumping them together.
> 
> Just describe them separately.
> 
It is not that easy to separate them because clockgen node plays two types
Of roles. Take REG property as example:
As IP block node, REG should be reg = <0xe1000 0x1000>, while as
Clock provider node, it should be reg = <0xabc 0x4> or no reg at all(for fixed clock).

> > At first, I want to add a extra fixed-clock node and move the
> > clock-frequency of clockgen Node to it, but it is against the backward
> > compatibility
> 
> Right.
> 
> > which I think it is not a big deal, Because nobody hasn't used it yet.
> 
> The point is it will require updating U-Boot to use it, versus existing
> U-Boots which already patch up the clock-frequency in the clockgen node.
> And there's nothing semantically wrong with the way it currently is.
> 
Yes, nothing wrong about it.
But we will keep adding the clockgen-x.y node all the time in uboot.
If we have one extra node to keep clock-frequency, it would be updated only once.

> > If I add a extra node with the clock-frequency property and don't move
> > the clock-frequency property of clockgen, that would be redundant
> > because both clockgen node and the extra node have the same clock-
> frequency node.
> > So, I choose what I did now.
> 
> I'm not complaining about how you structured the nodes, just how you
> documented them.
> 
As I said it is hard to document clockgen node if we don't separate its
two roles.
I think the following structure is better.

+   clockgen: global-utilities at e1000 {
+       compatible = "fsl,p5020-clockgen", "fsl,qoriq-clockgen-1.0";
+       reg = <0xe1000 0x1000>;
+       #address-cells = <1>;
+       #size-cells = <1>;
	  
	  Sysclk:sysclk {
	  	compatible = "fsl,qoriq-sysclk", "fixed-clock";
       	clock-output-names = "sysclk";
       	#clock-cells = <0>;   
		clock-frequency = <0>;		
	  }
+       pll0: pll0 at 800 {
+           #clock-cells = <1>;
+           reg = <0x800 0x4>;
+           compatible = "fsl,qoriq-chassis1-core-pll";
+           clocks = <& Sysclk >;
+           clock-output-names = "pll0", "pll0-div2";
+       };

Regards,
Yuantian

> -Scott
>