答复: [v7] clk: corenet: Adds the clock binding

[Scott Wood]

On Wed, 2014-01-08 at 20:57 -0600, Tang Yuantian-B29983 wrote:
> Thanks for you review.
> See my response inline.
> 
> Thanks,
> Yuantian
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: 2014年1月9日 星期四 2:44
> > To: Mark Rutland
> > Cc: Tang Yuantian-B29983; galak at kernel.crashing.org;
> > devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org
> > Subject: Re: 答复: [v7] clk: corenet: Adds the clock binding
> > 
> > On Wed, 2014-01-08 at 09:30 +0000, Mark Rutland wrote:
> > > On Wed, Jan 08, 2014 at 08:53:56AM +0000, Yuantian Tang wrote:
> > > >
> > > > ________________________________________
> > > > 发件人: Wood Scott-B07421
> > > > 发送时间: 2014年1月8日 8:21
> > > > 收件人: Tang Yuantian-B29983
> > > > 抄送: galak at kernel.crashing.org; mark.rutland at arm.com;
> > > > devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org
> > > > 主题: Re: [v7] clk: corenet: Adds the clock binding
> > > >
> > > > On Wed, Nov 20, 2013 at 05:04:49PM +0800, tang yuantian wrote:
> > > > > +Recommended properties:
> > > > > +- ranges: Allows valid translation between child's address space
> > and
> > > > > +     parent's. Must be present if the device has sub-nodes.
> > > > > +- #address-cells: Specifies the number of cells used to represent
> > > > > +     physical base addresses.  Must be present if the device has
> > > > > +     sub-nodes and set to 1 if present
> > > > > +- #size-cells: Specifies the number of cells used to represent
> > > > > +     the size of an address. Must be present if the device has
> > > > > +     sub-nodes and set to 1 if present
> > > >
> > > > Why are we specifying #address-cells/#size-cells here?
> > > >
> > > > A: it has sub-nodes which have REG property, don't we need to
> > > > specify #address-cells/#size-cells?
> > >
> > > If a node has a reg entry, its parent should have #size-cells and
> > > #address-cells to allow it to be parsed properly.
> > 
> > Yes, but why do we need to specify in this binding how many cells there
> > will be, especially since this binding only addresses the clock provider
> > aspect of the clockgen nodes (e.g. it doesn't describe the reg)?  Or
> > rather, it's partially describing the non-clock aspect, and doesn't
> > address the clock aspect at all AFAICT.
> > 
> First of all, they are not "Required properties", they are optional.
> If present, we should give them a value of 1.

Why does it matter, so long as the values translate properly?  It's not
as if you're defining a special reg format.  It's not that big of a
deal, but it seems unnecessary.

> Then, yes, this binding describes clockgen node which is "CLOCK BLOCK".

Sorry, I missed where "reg" was documented due to the
required/recommended split.

> > Where does the actual input clock frequency go?  U-Boot puts it in the
> > clockgen node itself as clock-frequency, but that isn't described in the
> > binding.  How does that relate to the sysclk node?  If "fsl,qoriq-sysclk-
> > 1.0" is supposed to indicate that clock-frequency can be found in the
> > parent node, that isn't specified by the binding, nor is clock-frequency
> > shown in the example.
> > 
> clock-frequency is a wired property.

Do you mean "weird"?

> It is in clockgen node right now.
> But it should be placed somewhere in clock nodes.

If we were doing this from scratch, yes, but there's existing U-Boot
code that we want to be compatible with.

> If I describe here, I would be asked why it is related to clockgen node?

That's not a good reason to leave it undocumented.

> > What is the difference between "fsl,qoriq-sysclk-1.0" and "fsl,qoriq-
> > sysclk-2.0"?  How does the concept of a fixed input clock change?
> >
> Technically, there is no difference between *sysclk-1.0 and *-2.0, just like
> Clockgen-2.0 and 1.0. Naming like this just to indicate they belong to chassis 2.0 
> and 1.0 respectively.

I guess it's OK if it encourages people to consider switching to the
standard fixed-clock for future chassis.

So the only thing that really needs to be fixed is the missing
clock-frequency documentation.

-Scott