| Message ID | 1392033008-20752-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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 > - 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. 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. 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. virtually yours Gerhard Sittig
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
On Tue, Feb 18, 2014 at 10:09 +0800, Shawn Guo wrote: > > 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. The binding text says otherwise. It claims to apply for "fsl,<soc>-fec" compatibles. 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. 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 > > > - 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. > > 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. See above, the binding is _not_ specific to i.MX, it's a FEC binding. Which happens to apply to several architectures, as the FEC is used in several SoCs. And keep in mind that the device tree binding is OS agnostic, Linux details just are not the reference. virtually yours Gerhard Sittig
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
On Tue, Feb 18, 2014 at 22:46 +0800, Shawn Guo wrote: > > On Tue, Feb 18, 2014 at 02:44:30PM +0100, Gerhard Sittig wrote: > > [ ... ] > > > > > > - 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? Have there been i.MX device trees in mainline releases which lack the clock specs? If so, making the clock specs mandatory breaks backwards compatibility for these existing device trees as well. I assume that listing the clocks as optional keeps the binding most compatible. You might as well list them as recommended, if optional is "too weak" for you. I guess that listing the clocks as recommended, and telling which clock names apply to which SoC variants, allows to keep using the binding for all current implementations which were written against this spec. This way you might focus on i.MX and say "the following SoCs require the following clock names", and still leave the PowerPC stuff or other FEC users for a followup patch. virtually yours Gerhard Sittig
On Sat, Feb 22, 2014 at 07:28:06PM +0100, Gerhard Sittig wrote: > On Tue, Feb 18, 2014 at 22:46 +0800, Shawn Guo wrote: > > > > On Tue, Feb 18, 2014 at 02:44:30PM +0100, Gerhard Sittig wrote: > > > [ ... ] > > > > > > > > - 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? > > Have there been i.MX device trees in mainline releases which lack > the clock specs? If so, making the clock specs mandatory breaks > backwards compatibility for these existing device trees as well. > > I assume that listing the clocks as optional keeps the binding > most compatible. You might as well list them as recommended, if > optional is "too weak" for you. Why should we keep the binding compatible when the driver requires the clocks? The clocks are not at all optional for the driver. We made the clocks mandatory at a time when the bindings didn't make any promises about stability, so Shawn only documents what the driver requires anyway. If anything, we should doument that this binding is not valid for the PowerPC variants of the FEC. Sascha
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".
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(-)