| Message ID | 1469001800-11615-4-git-send-email-clabbe.montjoie@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote: > This patch adds documentation for Device-Tree bindings for the > Allwinner sun8i-emac driver. > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > --- > .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > new file mode 100644 > index 0000000..4bf4e53 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > @@ -0,0 +1,65 @@ > +* Allwinner sun8i EMAC ethernet controller > + > +Required properties: > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", > + or "allwinner,sun50i-a64-emac" List one per line. > +- reg: address and length of the register sets for the device. > +- reg-names: should be "emac" and "syscon", matching the register sets > +- interrupts: interrupt for the device > +- clocks: A phandle to the reference clock for this device > +- clock-names: should be "ahb" > +- resets: A phandle to the reset control for this device > +- reset-names: should be "ahb" > +- phy-mode: See ethernet.txt > +- phy or phy-handle: See ethernet.txt > +- #address-cells: shall be 1 > +- #size-cells: shall be 0 > + > +"allwinner,sun8i-h3-emac" also requires: > +- clocks: an extra phandle to the reference clock for the EPHY > +- clock-names: an extra "ephy" entry matching the clocks property > +- resets: an extra phandle to the reset control for the EPHY > +- resets-names: an extra "ephy" entry matching the resets property > + > +See ethernet.txt in the same directory for generic bindings for ethernet > +controllers. > + > +The device node referenced by "phy" or "phy-handle" should be a child node > +of this node. See phy.txt for the generic PHY bindings. > + > +Optional properties: > +- phy-supply: phandle to a regulator if the PHY needs one > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > + This is sometimes found with RGMII PHYs, which use a second > + regulator for the lower I/O voltage. I previously said these should go in the phy node, and you said you would remove them. Rob -- 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
Hi, On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote: > This patch adds documentation for Device-Tree bindings for the > Allwinner sun8i-emac driver. > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > --- > .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > new file mode 100644 > index 0000000..4bf4e53 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > @@ -0,0 +1,65 @@ > +* Allwinner sun8i EMAC ethernet controller > + > +Required properties: > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", > + or "allwinner,sun50i-a64-emac" > +- reg: address and length of the register sets for the device. > +- reg-names: should be "emac" and "syscon", matching the register sets Blindly mapping a register of some other device on the SoC doesn't look very reasonable. > +- interrupts: interrupt for the device > +- clocks: A phandle to the reference clock for this device > +- clock-names: should be "ahb" > +- resets: A phandle to the reset control for this device > +- reset-names: should be "ahb" > +- phy-mode: See ethernet.txt > +- phy or phy-handle: See ethernet.txt > +- #address-cells: shall be 1 > +- #size-cells: shall be 0 > + > +"allwinner,sun8i-h3-emac" also requires: > +- clocks: an extra phandle to the reference clock for the EPHY > +- clock-names: an extra "ephy" entry matching the clocks property > +- resets: an extra phandle to the reset control for the EPHY > +- resets-names: an extra "ephy" entry matching the resets property Shouldn't that be attached to the phy itself? > +See ethernet.txt in the same directory for generic bindings for ethernet > +controllers. > + > +The device node referenced by "phy" or "phy-handle" should be a child node > +of this node. See phy.txt for the generic PHY bindings. > + > +Optional properties: > +- phy-supply: phandle to a regulator if the PHY needs one > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > + This is sometimes found with RGMII PHYs, which use a second > + regulator for the lower I/O voltage. > +- allwinner,tx-delay: The setting of the TX clock delay chain > +- allwinner,rx-delay: The setting of the RX clock delay chain In which unit? What is the default value? > + > +The TX/RX clock delay chain settings are board specific. > + > +Optional properties for "allwinner,sun8i-h3-emac": > +- allwinner,use-internal-phy: Use the H3 SoC's internal E(thernet) PHY Can't that be derived from the presence of the phy property? > +- allwinner,leds-active-low: EPHY LEDs are active low That also seems PHY related. Overall, I feel like we really need a phy node for the internal phy. Maxime
On Wed, Jul 20, 2016 at 02:15:33PM -0500, Rob Herring wrote: > On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote: > > This patch adds documentation for Device-Tree bindings for the > > Allwinner sun8i-emac driver. > > > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > > --- > > .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ > > 1 file changed, 65 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > new file mode 100644 > > index 0000000..4bf4e53 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > @@ -0,0 +1,65 @@ > > +* Allwinner sun8i EMAC ethernet controller > > + > > +Required properties: > > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", > > + or "allwinner,sun50i-a64-emac" > > List one per line. > ok > > +- reg: address and length of the register sets for the device. > > +- reg-names: should be "emac" and "syscon", matching the register sets > > +- interrupts: interrupt for the device > > +- clocks: A phandle to the reference clock for this device > > +- clock-names: should be "ahb" > > +- resets: A phandle to the reset control for this device > > +- reset-names: should be "ahb" > > +- phy-mode: See ethernet.txt > > +- phy or phy-handle: See ethernet.txt > > +- #address-cells: shall be 1 > > +- #size-cells: shall be 0 > > + > > +"allwinner,sun8i-h3-emac" also requires: > > +- clocks: an extra phandle to the reference clock for the EPHY > > +- clock-names: an extra "ephy" entry matching the clocks property > > +- resets: an extra phandle to the reset control for the EPHY > > +- resets-names: an extra "ephy" entry matching the resets property > > + > > +See ethernet.txt in the same directory for generic bindings for ethernet > > +controllers. > > + > > +The device node referenced by "phy" or "phy-handle" should be a child node > > +of this node. See phy.txt for the generic PHY bindings. > > + > > +Optional properties: > > +- phy-supply: phandle to a regulator if the PHY needs one > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > > + This is sometimes found with RGMII PHYs, which use a second > > + regulator for the lower I/O voltage. > > I previously said these should go in the phy node, and you said you > would remove them. > > Rob Sorry, I forgot to remove them. I have removed them now. Regards Thanks LABBE Corentin -- 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 Thu, Jul 21, 2016 at 09:55:19AM +0200, Maxime Ripard wrote: > Hi, > > On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote: > > This patch adds documentation for Device-Tree bindings for the > > Allwinner sun8i-emac driver. > > > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > > --- > > .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ > > 1 file changed, 65 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > new file mode 100644 > > index 0000000..4bf4e53 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > @@ -0,0 +1,65 @@ > > +* Allwinner sun8i EMAC ethernet controller > > + > > +Required properties: > > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", > > + or "allwinner,sun50i-a64-emac" > > +- reg: address and length of the register sets for the device. > > +- reg-names: should be "emac" and "syscon", matching the register sets > > Blindly mapping a register of some other device on the SoC doesn't > look very reasonable. > As we discuss after this mail on IRC, this register is dedicated to EMAC. > > +- interrupts: interrupt for the device > > +- clocks: A phandle to the reference clock for this device > > +- clock-names: should be "ahb" > > +- resets: A phandle to the reset control for this device > > +- reset-names: should be "ahb" > > +- phy-mode: See ethernet.txt > > +- phy or phy-handle: See ethernet.txt > > +- #address-cells: shall be 1 > > +- #size-cells: shall be 0 > > + > > +"allwinner,sun8i-h3-emac" also requires: > > +- clocks: an extra phandle to the reference clock for the EPHY > > +- clock-names: an extra "ephy" entry matching the clocks property > > +- resets: an extra phandle to the reset control for the EPHY > > +- resets-names: an extra "ephy" entry matching the resets property > > Shouldn't that be attached to the phy itself? > Ok I will move them. > > +See ethernet.txt in the same directory for generic bindings for ethernet > > +controllers. > > + > > +The device node referenced by "phy" or "phy-handle" should be a child node > > +of this node. See phy.txt for the generic PHY bindings. > > + > > +Optional properties: > > +- phy-supply: phandle to a regulator if the PHY needs one > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > > + This is sometimes found with RGMII PHYs, which use a second > > + regulator for the lower I/O voltage. > > +- allwinner,tx-delay: The setting of the TX clock delay chain > > +- allwinner,rx-delay: The setting of the RX clock delay chain > > In which unit? What is the default value? > The unit is unknown to me, but I have added a comment for the default and acceptable range value. > > + > > +The TX/RX clock delay chain settings are board specific. > > + > > +Optional properties for "allwinner,sun8i-h3-emac": > > +- allwinner,use-internal-phy: Use the H3 SoC's internal E(thernet) PHY > > Can't that be derived from the presence of the phy property? > Yes, I have reworked the "variant" of the driver for easily handling this. > > +- allwinner,leds-active-low: EPHY LEDs are active low > > That also seems PHY related. Overall, I feel like we really need a phy > node for the internal phy. > Moved also in the phy node. > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Best regards Thanks LABBE Corentin -- 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 Thu, Jul 28, 2016 at 03:40:31PM +0200, LABBE Corentin wrote: > On Thu, Jul 21, 2016 at 09:55:19AM +0200, Maxime Ripard wrote: > > Hi, > > > > On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote: > > > This patch adds documentation for Device-Tree bindings for the > > > Allwinner sun8i-emac driver. > > > > > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > > > --- > > > .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ > > > 1 file changed, 65 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > > > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > new file mode 100644 > > > index 0000000..4bf4e53 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > @@ -0,0 +1,65 @@ > > > +* Allwinner sun8i EMAC ethernet controller > > > + > > > +Required properties: > > > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", > > > + or "allwinner,sun50i-a64-emac" > > > +- reg: address and length of the register sets for the device. > > > +- reg-names: should be "emac" and "syscon", matching the register sets > > > > Blindly mapping a register of some other device on the SoC doesn't > > look very reasonable. > > As we discuss after this mail on IRC, this register is dedicated to EMAC. I don't think we did. It's still right in the middle of some other hardware block register space. You actually have a syscon driver to do just that, why not use it? > > > +See ethernet.txt in the same directory for generic bindings for ethernet > > > +controllers. > > > + > > > +The device node referenced by "phy" or "phy-handle" should be a child node > > > +of this node. See phy.txt for the generic PHY bindings. > > > + > > > +Optional properties: > > > +- phy-supply: phandle to a regulator if the PHY needs one > > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > > > + This is sometimes found with RGMII PHYs, which use a second > > > + regulator for the lower I/O voltage. > > > +- allwinner,tx-delay: The setting of the TX clock delay chain > > > +- allwinner,rx-delay: The setting of the RX clock delay chain > > > > In which unit? What is the default value? > > The unit is unknown to me, but I have added a comment for the > default and acceptable range value. That's unfortunate. We'll see how the DT maintainers feel about that. Maxime
On Thu, Jul 28, 2016 at 08:49:16PM +0200, Maxime Ripard wrote: > On Thu, Jul 28, 2016 at 03:40:31PM +0200, LABBE Corentin wrote: > > On Thu, Jul 21, 2016 at 09:55:19AM +0200, Maxime Ripard wrote: > > > Hi, > > > > > > On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote: > > > > This patch adds documentation for Device-Tree bindings for the > > > > Allwinner sun8i-emac driver. > > > > > > > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > > > > --- > > > > .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ > > > > 1 file changed, 65 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > > new file mode 100644 > > > > index 0000000..4bf4e53 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt > > > > @@ -0,0 +1,65 @@ > > > > +* Allwinner sun8i EMAC ethernet controller > > > > + > > > > +Required properties: > > > > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", > > > > + or "allwinner,sun50i-a64-emac" > > > > +- reg: address and length of the register sets for the device. > > > > +- reg-names: should be "emac" and "syscon", matching the register sets > > > > > > Blindly mapping a register of some other device on the SoC doesn't > > > look very reasonable. > > > > As we discuss after this mail on IRC, this register is dedicated to EMAC. > > I don't think we did. It's still right in the middle of some other > hardware block register space. You actually have a syscon driver to do > just that, why not use it? > I will try with syscon driver > > > > +See ethernet.txt in the same directory for generic bindings for ethernet > > > > +controllers. > > > > + > > > > +The device node referenced by "phy" or "phy-handle" should be a child node > > > > +of this node. See phy.txt for the generic PHY bindings. > > > > + > > > > +Optional properties: > > > > +- phy-supply: phandle to a regulator if the PHY needs one > > > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > > > > + This is sometimes found with RGMII PHYs, which use a second > > > > + regulator for the lower I/O voltage. > > > > +- allwinner,tx-delay: The setting of the TX clock delay chain > > > > +- allwinner,rx-delay: The setting of the RX clock delay chain > > > > > > In which unit? What is the default value? > > > > The unit is unknown to me, but I have added a comment for the > > default and acceptable range value. > > That's unfortunate. We'll see how the DT maintainers feel about that. > I have searched for txdelay in Documentation, and found a few driver that give the units (us/ps). But in that case, the value in ps/us must be found in a table indexed by the Xxdelay value. So the settings seems always a raw number, and for sun8i-emac nothing in user manual could help to find what each value is/related to. So the good value is either found by "try and test" or "copy the value found in fex file". Regards LABBE Corentin -- 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 Fri, Jul 29, 2016 at 10:15:19AM +0200, LABBE Corentin wrote: > > > > > +See ethernet.txt in the same directory for generic bindings for ethernet > > > > > +controllers. > > > > > + > > > > > +The device node referenced by "phy" or "phy-handle" should be a child node > > > > > +of this node. See phy.txt for the generic PHY bindings. > > > > > + > > > > > +Optional properties: > > > > > +- phy-supply: phandle to a regulator if the PHY needs one > > > > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. > > > > > + This is sometimes found with RGMII PHYs, which use a second > > > > > + regulator for the lower I/O voltage. > > > > > +- allwinner,tx-delay: The setting of the TX clock delay chain > > > > > +- allwinner,rx-delay: The setting of the RX clock delay chain > > > > > > > > In which unit? What is the default value? > > > > > > The unit is unknown to me, but I have added a comment for the > > > default and acceptable range value. > > > > That's unfortunate. We'll see how the DT maintainers feel about that. > > > > I have searched for txdelay in Documentation, and found a few driver > that give the units (us/ps). > > But in that case, the value in ps/us must be found in a table > indexed by the Xxdelay value. > > So the settings seems always a raw number, and for sun8i-emac > nothing in user manual could help to find what each value is/related > to. > > So the good value is either found by "try and test" or "copy the > value found in fex file". What I meant was that, just like you found out already, most of the time the properties should be in absolute units, so that it doesn't depend on some clock rate most likely in that case. Maxime
diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt new file mode 100644 index 0000000..4bf4e53 --- /dev/null +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt @@ -0,0 +1,65 @@ +* Allwinner sun8i EMAC ethernet controller + +Required properties: +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac", + or "allwinner,sun50i-a64-emac" +- reg: address and length of the register sets for the device. +- reg-names: should be "emac" and "syscon", matching the register sets +- interrupts: interrupt for the device +- clocks: A phandle to the reference clock for this device +- clock-names: should be "ahb" +- resets: A phandle to the reset control for this device +- reset-names: should be "ahb" +- phy-mode: See ethernet.txt +- phy or phy-handle: See ethernet.txt +- #address-cells: shall be 1 +- #size-cells: shall be 0 + +"allwinner,sun8i-h3-emac" also requires: +- clocks: an extra phandle to the reference clock for the EPHY +- clock-names: an extra "ephy" entry matching the clocks property +- resets: an extra phandle to the reset control for the EPHY +- resets-names: an extra "ephy" entry matching the resets property + +See ethernet.txt in the same directory for generic bindings for ethernet +controllers. + +The device node referenced by "phy" or "phy-handle" should be a child node +of this node. See phy.txt for the generic PHY bindings. + +Optional properties: +- phy-supply: phandle to a regulator if the PHY needs one +- phy-io-supply: phandle to a regulator if the PHY needs a another one for I/O. + This is sometimes found with RGMII PHYs, which use a second + regulator for the lower I/O voltage. +- allwinner,tx-delay: The setting of the TX clock delay chain +- allwinner,rx-delay: The setting of the RX clock delay chain + +The TX/RX clock delay chain settings are board specific. + +Optional properties for "allwinner,sun8i-h3-emac": +- allwinner,use-internal-phy: Use the H3 SoC's internal E(thernet) PHY +- allwinner,leds-active-low: EPHY LEDs are active low + +When the internal PHY is requested, the implementation shall configure the +internal PHY to use the address specified in the child PHY node. + +Example: + +emac: ethernet@01c0b000 { + compatible = "allwinner,sun8i-h3-emac"; + reg = <0x01c0b000 0x104>, <0x01c00030 0x4>; + reg-names = "emac", "syscon"; + interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&bus_gates 17>, <&bus_gates 128>; + clock-names = "ahb", "ephy"; + resets = <&ahb_rst 17>, <&ahb_rst 66>; + reset-names = "ahb", "ephy"; + phy = <&phy1>; + allwinner,use-internal-phy; + allwinner,leds-active-low; + + phy1: ethernet-phy@1 { + reg = <1>; + }; +};
This patch adds documentation for Device-Tree bindings for the Allwinner sun8i-emac driver. Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> --- .../bindings/net/allwinner,sun8i-emac.txt | 65 ++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt