RE: [PATCH] DT: add MDIO node for FMan node

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, November 12, 2014 1:38 AM
> To: Xie Shaohui-B21989
> Cc: Liberman Igal-B31950; linuxppc-dev@xxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Medve Emilian-EMMEDVE1
> Subject: Re: [PATCH] DT: add MDIO node for FMan node
> 
> On Tue, 2014-11-11 at 04:32 -0600, Xie Shaohui-B21989 wrote:
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, November 11, 2014 8:23 AM
> > > To: shh.xie@xxxxxxxxx
> > > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Medve
> > > Emilian-EMMEDVE1; Xie Shaohui-B21989
> > > Subject: Re: [PATCH] DT: add MDIO node for FMan node
> > >
> > > On Tue, 2014-11-04 at 19:56 +0800, shh.xie@xxxxxxxxx wrote:
> > > > From: Shaohui Xie <Shaohui.Xie@xxxxxxxxxxxxx>
> > > >
> > > > This binding is for FMan MDIO, it covers FMan v2 & FMan v3.
> > > >
> > > > Signed-off-by: Shaohui Xie <Shaohui.Xie@xxxxxxxxxxxxx>
> > > > ---
> > > > based on http://patchwork.ozlabs.org/patch/390351/
> > > > for 'next' of
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/scottwood/linux.git
> > >
> > > Are there any other FMan pieces that are missing from the above patch?
> > [S.H] I'm adding Igal for this comment.
> >
> > >
> > > > +- bus-frequency
> > > > +		Usage: optional
> > > > +		Value type: <u32>
> > > > +		Definition: Default MDIO bus clock speed.
> > >
> > > Use clocks/clock-names
> > [S.H] The MDIO uses Fman clock and divides it to a proper value which
> is specified by this property.
> 
> Use clocks/clock-names to describe that relationship.
> 
[S.H] The MDIO node is sub-node and embedded in Fman node, the clocks/clock-names is provided by Fman node, should repeat them in MDIO node? For the default MDIO bus clock speed, maybe "clock-ranges" should be used?

> > >
> > > > +- interrupts
> > > > +		Usage: optional
> > > > +		Value type: <prop-encoded-array>
> > > > +		Definition: MDIO controller event interrupts.
> > >
> > > One interrupt or multiple?
> > [S.H] One for 1 Gb/s, one for 10 Gb/s.
> 
> Then the binding needs to say that there are two (except for "fsl,fman-
> mdio", right?) and which is which.
> 
[S.H] OK.

> > >
> > > > +
> > > > +- type
> > > > +		Usage: required for FMan v3
> > > > +		Value type: <stringlist>
> > > > +		Definition: A standard property.
> > >
> > > What standard is "type" defined in?
> >
> > [S.H] It's to differentiate between the internal and external MDIO,
> > I'm not quite sure about naming it, or what could be better way to
> > differentiate the MDIOs?
> 
> I'm not questioning the need to convey this information.  I'm saying it's
> not "A standard property".  I'd also like to see a better name.
> 
[S.H] since Fman V2 & V3 can be differentiated by compatible, a boolean property "fsl,fman-internal-mdio" seems better, if defined, it indicates an internal MDIO. It looks like below:

fsl,fman-internal-mdio
		Usage: required for internal MDIO
		Value type: Boolean
		Definition: Fman has internal MDIO for internal PCS(Physical
		Coding Sublayer) PHYs and external MDIO for external PHYs.
		The settings and programming routines for internal/external
		MDIO are different. Must be included for internal MDIO.

How about this?

Thanks!
Shaohui
 
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux