| Message ID | 1392033008-20752-1-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| 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 > > -- 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
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
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
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(-)