dt/bindings: update fsl-fec regarding compatible and clocks

Message ID 1392033008-20752-1-git-send-email-shawn.guo@linaro.org
State New
Headers show

Commit Message

Shawn Guo Feb. 10, 2014, 11:50 a.m.
Update fsl-fec to explicitly list the supported compatible strings
and add missing 'clocks' and 'clock-names' properties.  It does not
change anything about how kernel drive works.  Instead, it just reflects
how kernel driver works today.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Shawn Guo Feb. 17, 2014, 5:30 a.m. | #1
Hi Rob and DT folks,

On Mon, Feb 10, 2014 at 07:50:08PM +0800, Shawn Guo wrote:
> Update fsl-fec to explicitly list the supported compatible strings
> and add missing 'clocks' and 'clock-names' properties.  It does not
> change anything about how kernel drive works.  Instead, it just reflects
> how kernel driver works today.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Any comments?  Or can it be applied?

Shawn

> ---
>  Documentation/devicetree/bindings/net/fsl-fec.txt |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> index 845ff84..3ebd395 100644
> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -1,9 +1,26 @@
>  * Freescale Fast Ethernet Controller (FEC)
>  
>  Required properties:
> -- compatible : Should be "fsl,<soc>-fec"
> +- compatible : Should contain one of the following:
> +		"fsl,imx25-fec"
> +		"fsl,imx27-fec"
> +		"fsl,imx28-fec"
> +		"fsl,imx6q-fec"
> +		"fsl,mvf600-fec"
>  - reg : Address and length of the register set for the device
>  - interrupts : Should contain fec interrupt
> +- clocks: phandle to the clocks feeding the FEC controller and phy. The
> +  following two are required:
> +   - "ipg": the peripheral access clock
> +   - "ahb": the bus clock for MAC
> +  The following two are optional:
> +   - "ptp": the sampling clock for PTP (IEEE 1588).  On SoC like i.MX6Q,
> +     the clock could come from either the internal clock control module
> +     or external oscillator via pad depending on board design.
> +   - "enet_out": the phy reference clock provided by SoC via pad, which
> +     is available on SoC like i.MX28.
> +- clock-names: Must contain the clock names described just above
> +
>  - phy-mode : String, operation mode of the PHY interface.
>    Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
>    "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi", "smii".
> -- 
> 1.7.9.5
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo Feb. 18, 2014, 2:09 a.m. | #2
On Mon, Feb 17, 2014 at 10:24:39PM +0100, Gerhard Sittig wrote:
> On Mon, Feb 10, 2014 at 19:50 +0800, Shawn Guo wrote:
> > 
> > Update fsl-fec to explicitly list the supported compatible strings
> > and add missing 'clocks' and 'clock-names' properties.  It does not
> > change anything about how kernel drive works.  Instead, it just reflects
> > how kernel driver works today.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/net/fsl-fec.txt |   19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > index 845ff84..3ebd395 100644
> > --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
> > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > @@ -1,9 +1,26 @@
> >  * Freescale Fast Ethernet Controller (FEC)
> >  
> >  Required properties:
> > -- compatible : Should be "fsl,<soc>-fec"
> > +- compatible : Should contain one of the following:
> > +		"fsl,imx25-fec"
> > +		"fsl,imx27-fec"
> > +		"fsl,imx28-fec"
> > +		"fsl,imx6q-fec"
> > +		"fsl,mvf600-fec"
> 
> This appears to miss all the PowerPC based SoCs.  See
>   git grep 'fsl,.*-fec' arch/*/boot/dts

Hmm, this is a binding for IMX FEC/ENET, and the driver is
drivers/net/ethernet/freescale/fec_main.c.  I think I've listed all the
compatibles that the driver supports.

> 
> >  - reg : Address and length of the register set for the device
> >  - interrupts : Should contain fec interrupt
> > +- clocks: phandle to the clocks feeding the FEC controller and phy. The
> > +  following two are required:
> > +   - "ipg": the peripheral access clock
> > +   - "ahb": the bus clock for MAC
> > +  The following two are optional:
> > +   - "ptp": the sampling clock for PTP (IEEE 1588).  On SoC like i.MX6Q,
> > +     the clock could come from either the internal clock control module
> > +     or external oscillator via pad depending on board design.
> > +   - "enet_out": the phy reference clock provided by SoC via pad, which
> > +     is available on SoC like i.MX28.
> > +- clock-names: Must contain the clock names described just above
> > +
> 
> Listing 'clocks' under the "required properties" all of a sudden
> invalidates existing device trees, if they don't carry the
> property which before the change was not required, not even
> documented.

Since the day we move to device tree clock lookup, the driver fec_main
does not probe at all if the property is absent.

> 
> The PowerPC based chips probably have differing sets of clocks.
> I'm aware of the MPC512x, where one "per" clock is sufficient,
> and even this spec is optional.  Other machines may not have yet
> been converted to CCF.

Again, the binding is created for IMX FEC/ENET controller and the driver
fec_main, so I'm not sure you should look at this binding for
PowerPC/MPC512x stuff at all.

> 
> Your description needs to get rephrased, it triggers Mark
> Rutland's regular "clocks are not just phandles" reply.  See how
> he suggested quite a few times a better wording.

Can you give a pointer or good example?  I worded it following an
example [1] found on recent linux-next/spi tree. 

Shawn

[1] https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/commit/?h=topic/sunxi&id=3558fe900e8af6c3bfadeff24a12ffb19ac9b108

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Guo Feb. 18, 2014, 2:46 p.m. | #3
On Tue, Feb 18, 2014 at 02:44:30PM +0100, Gerhard Sittig wrote:
> > > > @@ -1,9 +1,26 @@
> > > >  * Freescale Fast Ethernet Controller (FEC)
> > > >  
> > > >  Required properties:
> > > > -- compatible : Should be "fsl,<soc>-fec"
> > > > +- compatible : Should contain one of the following:
> > > > +		"fsl,imx25-fec"
> > > > +		"fsl,imx27-fec"
> > > > +		"fsl,imx28-fec"
> > > > +		"fsl,imx6q-fec"
> > > > +		"fsl,mvf600-fec"
> > > 
> > > This appears to miss all the PowerPC based SoCs.  See
> > >   git grep 'fsl,.*-fec' arch/*/boot/dts
> > 
> > Hmm, this is a binding for IMX FEC/ENET, and the driver is
> > drivers/net/ethernet/freescale/fec_main.c.
> 
> The binding text says otherwise.  It claims to apply for
> "fsl,<soc>-fec" compatibles.

I should really list the compatibles specifically when I was creating
the document at day one.  But honestly, I did not intend to cover
PowerPC chips with this document.

> 
> It's funny how the first line of the source you point to talks
> about being a FEC driver for MPC8xx. :)  But that doesn't matter
> here, as it's just a comment in some code.
> 
> > I think I've listed all the compatibles that the driver
> > supports.
> 
> You got it backwards.  The binding is not the after-the-fact
> documentation of a specific Linux driver.  Instead the Linux
> driver is (supposed to be) an implementation of what the binding
> specifies.

Yes, theoretically.  But practically, well ...

> And in this case, there are several drivers, each
> managing a subset of the compatibles space, each supposed to
> follow the spec.  See
> 
>   git grep 'fsl,.*-fec' drivers/net/ethernet
> 

The spec was created without considering those drivers other than
fec_main.  For example, the 'phy-mode' is documented as a required
property in the spec.  But I do not think that's the case for drivers
fec_mpc52xx and fs_enet-main.

> > > >  - reg : Address and length of the register set for the device
> > > >  - interrupts : Should contain fec interrupt
> > > > +- clocks: phandle to the clocks feeding the FEC controller and phy. The
> > > > +  following two are required:
> > > > +   - "ipg": the peripheral access clock
> > > > +   - "ahb": the bus clock for MAC
> > > > +  The following two are optional:
> > > > +   - "ptp": the sampling clock for PTP (IEEE 1588).  On SoC like i.MX6Q,
> > > > +     the clock could come from either the internal clock control module
> > > > +     or external oscillator via pad depending on board design.
> > > > +   - "enet_out": the phy reference clock provided by SoC via pad, which
> > > > +     is available on SoC like i.MX28.
> > > > +- clock-names: Must contain the clock names described just above
> > > > +
> > > 
> > > Listing 'clocks' under the "required properties" all of a sudden
> > > invalidates existing device trees, if they don't carry the
> > > property which before the change was not required, not even
> > > documented.
> > 
> > Since the day we move to device tree clock lookup, the driver fec_main
> > does not probe at all if the property is absent.
> 
> That's an implementation detail.  It's not what the spec says,
> and neither is what the spec is to blindly follow after the / a
> driver created the fact.  Instead, a binding gets designed, and
> the software follows.
> 
> In reality, the doc may be behind as developers are more
> concerned about the code.  But still when you "update" the
> binding, don't break compatibility!  Even if you'd adjust all
> drivers you can spot, it's still only Linux and not all device
> tree users.

So what's your suggestion?  Add the properties as the optional?

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 845ff84..3ebd395 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -1,9 +1,26 @@ 
 * Freescale Fast Ethernet Controller (FEC)
 
 Required properties:
-- compatible : Should be "fsl,<soc>-fec"
+- compatible : Should contain one of the following:
+		"fsl,imx25-fec"
+		"fsl,imx27-fec"
+		"fsl,imx28-fec"
+		"fsl,imx6q-fec"
+		"fsl,mvf600-fec"
 - reg : Address and length of the register set for the device
 - interrupts : Should contain fec interrupt
+- clocks: phandle to the clocks feeding the FEC controller and phy. The
+  following two are required:
+   - "ipg": the peripheral access clock
+   - "ahb": the bus clock for MAC
+  The following two are optional:
+   - "ptp": the sampling clock for PTP (IEEE 1588).  On SoC like i.MX6Q,
+     the clock could come from either the internal clock control module
+     or external oscillator via pad depending on board design.
+   - "enet_out": the phy reference clock provided by SoC via pad, which
+     is available on SoC like i.MX28.
+- clock-names: Must contain the clock names described just above
+
 - phy-mode : String, operation mode of the PHY interface.
   Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
   "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi", "smii".