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

[Yuantian Tang]

Thanks for your review. I will send next version of patch.

Thanks,
Yuantian


> -----Original Message-----
> From: Wood Scott-B07421
> Sent: 2014年1月10日 星期五 5:19
> To: Tang Yuantian-B29983
> Cc: Mark Rutland; 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 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
>