Re: [v3] powerpc/mpc85xx: Update the clock device tree nodes

[Scott Wood <scottwood@freescale.com>]

On Mon, 2013-08-26 at 21:49 -0500, Tang Yuantian-B29983 wrote:
> > > > > +		};
> > > > > +		pll1: pll1@820 {
> > > > > +			#clock-cells = <1>;
> > > > > +			reg = <0x820>;
> > > > > +			compatible = "fsl,core-pll-clock";
> > > > > +			clocks = <&clockgen>;
> > > > > +			clock-output-names = "pll1", "pll1-div2", "pll1-
> > div4";
> > > > > +		};
> > > >
> > > > Please leave a blank line between properties and nodes, and between
> > nodes.
> > > >
> > > OK, will add.
> > >
> > > > What does reg represent?  Where is the binding for this?
> > > >
> > > > The compatible is too vague.
> > > Reg is register offset.
> > 
> > With no size?
> 
> No size is needed.

Yes, it is.  Register blocks have size -- even if it's just a single
register.

> > > It is too later to change since the clock driver is merged for months
> > > although I sent this patch first.
> > 
> > It should not have gone in without an approved binding.  It seems it went
> > in via Mike Turquette (why is a non-ARM-specific tree using linux-arm-
> > kernel as its list, BTW?).  No ack from Ben, Kumar, or me is shown in the
> > commit.
> The Linux common clock framework is not ARM specific. Any other arch can use it.

Sure, it just seemed an odd choice of mailing list for something that
isn't ARM-specific.

> > In any case, you can preserve compatibility with existing trees without
> > using this compatible in new trees.  The driver can check for both
> > compatibles, with a comment indicating that "fsl,core-mux-clock" is
> > deprecated and for compatibility only.
> It is sub-clock node, is it really necessary to think about compatibility?
> I think that's the node clockgen's responsibility.

It describes registers, so yes, you need to consider compatibility.  A
clock provider is not responsible for figuring out how to program
devices that consume its clocks, nor should it make any assumptions
about such devices.
 
> > > Besides, it is not too bad because other arch use the similar name.
> > 
> > I don't follow.  This is a specific Freescale register interface, not a
> > general concept.
> > 
> > In any case, which "similar names" are you referring to?  A search in
> > arch/arm/boot/dts for "mux" with "clk" or "clock" turns up
> > "allwinner,sun4i-apb1-mux-clk" which is much more specific than
> > "fsl,core-mux-clock".
> Ok, I will change the compatible string.
> Do you think "fsl,ppc-core-*" is ok?

No.  How about "fsl,qoriq-chassis1-*" (for e500mc/e5500) and
fsl,qoriq-chassis2-*" (for e6500)?

-Scott