Re: [PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
On Tue, Sep 13, 2016 at 03:34:17PM +0200, LABBE Corentin wrote: > On Fri, Sep 09, 2016 at 04:17:10PM +0200, Andrew Lunn wrote: > > > +Optional properties: > > > +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. > > > Default is 0) > > > +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. > > > Default is 0) > > > > What are the units? pS? nS? > > > > Andrew > > No units, only raw number. > I will add a comment for this. And it is likely it will get NACKed by the device tree maintainers. You should use real unit here. Andrew
Re: [PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
On Fri, Sep 09, 2016 at 04:17:10PM +0200, Andrew Lunn wrote: > > +Optional properties: > > +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. > > Default is 0) > > +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. > > Default is 0) > > What are the units? pS? nS? > > Andrew No units, only raw number. I will add a comment for this. Regards Corentin Labbe
Re: [PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
> Hello > > Since the MDIO bus is a part of the sun8i-emac, does I really need > to create such a mdio node ? It is good practice. Part of the issue is that there are no written guidelines, so different drivers do different things. I'm trying to push all new drivers to have an MDIO node. > Anyway I try the following patch to solve your comments, but it > breaks the PHY finding(Could not attach to PHY). > --- a/drivers/net/ethernet/allwinner/sun8i-emac.c > +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c > @@ -2122,7 +2122,7 @@ static int sun8i_emac_probe(struct platform_device > *pdev) > return -EINVAL; > } > > - priv->phy_node = of_parse_phandle(node, "phy", 0); > + priv->phy_node = of_parse_phandle(node, "phy-handle", 0); > if (!priv->phy_node) { > netdev_err(ndev, "No associated PHY\n"); > return -ENODEV; > > > { > I don't see a change here for of_mdiobus_register(). You need to pass the mdio node. Andrew
Re: [PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
On Fri, Sep 09, 2016 at 04:04:13PM +0200, Andrew Lunn wrote: > > +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. > > I've not looked at the code yet, but is this really true? Generally > there is not this limitation. You can point to any Ethernet phy > anyway, so long as it is on am MDIO bus. > > > + > > +Optional properties: > > +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. > > Default is 0) > > +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. > > Default is 0) > > + > > +The TX/RX clock delay chain settings are board specific. > > + > > +Optional properties for "allwinner,sun8i-h3-emac": > > +- allwinner,leds-active-low: EPHY LEDs are active low > > + > > +Example: > > + > > +emac: ethernet@01c0b000 { > > + compatible = "allwinner,sun8i-h3-emac"; > > + syscon = <>; > > + reg = <0x01c0b000 0x104>; > > + reg-names = "emac"; > > + interrupts = ; > > + resets = < RST_BUS_EMAC>, << RST_BUS_EPHY>; > > + reset-names = "ahb", "ephy"; > > + clocks = < CLK_BUS_EMAC>, < CLK_BUS_EPHY>; > > + clock-names = "ahb", "ephy"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + phy = <>; > > ethernet.txt say: > > - phy: the same as "phy-handle" property, not recommended for new bindings. > > This is a new binding, please don't support it. > > > + phy-mode = "mii"; > > + allwinner,leds-active-low; > > + > > + phy1: ethernet-phy@1 { > > + reg = <1>; > > + }; > > It is normal to place these phy nodes inside an container node called > mdio. > Hello Since the MDIO bus is a part of the sun8i-emac, does I really need to create such a mdio node ? All example I found are mdio bus with separate driver. (others driver have the phy directly in [eg]mac node. Anyway I try the following patch to solve your comments, but it breaks the PHY finding(Could not attach to PHY). Regards -->8-- --- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts @@ -166,14 +166,18 @@ status = "okay"; }; + { + reg = <1>; + phy1: ethernet-phy@1 { + reg = <1>; + }; +}; + { - phy = <>; + phy-handle = <>; phy-mode = "mii"; allwinner,leds-active-low; status = "okay"; - phy1: ethernet-phy@1 { - reg = <1>; - }; };/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -474,6 +474,11 @@ #address-cells = <1>; #size-cells = <0>; status = "disabled"; + + mdio: mdio@0 { + #address-cells = <1>; + #size-cells = <0>; + }; }; crypto: crypto@1c15000 { --- a/drivers/net/ethernet/allwinner/sun8i-emac.c +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c @@ -2122,7 +2122,7 @@ static int sun8i_emac_probe(struct platform_device *pdev) return -EINVAL; } - priv->phy_node = of_parse_phandle(node, "phy", 0); + priv->phy_node = of_parse_phandle(node, "phy-handle", 0); if (!priv->phy_node) { netdev_err(ndev, "No associated PHY\n"); return -ENODEV; {
Re: [PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
> +Optional properties: > +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. > Default is 0) > +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. > Default is 0) What are the units? pS? nS? Andrew
Re: [PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
> +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. I've not looked at the code yet, but is this really true? Generally there is not this limitation. You can point to any Ethernet phy anyway, so long as it is on am MDIO bus. > + > +Optional properties: > +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. > Default is 0) > +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. > Default is 0) > + > +The TX/RX clock delay chain settings are board specific. > + > +Optional properties for "allwinner,sun8i-h3-emac": > +- allwinner,leds-active-low: EPHY LEDs are active low > + > +Example: > + > +emac: ethernet@01c0b000 { > + compatible = "allwinner,sun8i-h3-emac"; > + syscon = <>; > + reg = <0x01c0b000 0x104>; > + reg-names = "emac"; > + interrupts = ; > + resets = < RST_BUS_EMAC>, << RST_BUS_EPHY>; > + reset-names = "ahb", "ephy"; > + clocks = < CLK_BUS_EMAC>, < CLK_BUS_EPHY>; > + clock-names = "ahb", "ephy"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + phy = <>; ethernet.txt say: - phy: the same as "phy-handle" property, not recommended for new bindings. This is a new binding, please don't support it. > + phy-mode = "mii"; > + allwinner,leds-active-low; > + > + phy1: ethernet-phy@1 { > + reg = <1>; > + }; It is normal to place these phy nodes inside an container node called mdio. Andrew
[PATCH v3 3/9] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
This patch adds documentation for Device-Tree bindings for the Allwinner sun8i-emac driver. Signed-off-by: Corentin Labbe--- .../bindings/net/allwinner,sun8i-emac.txt | 64 ++ 1 file changed, 64 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 000..4968ee3 --- /dev/null +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt @@ -0,0 +1,64 @@ +* Allwinner sun8i EMAC ethernet controller + +Required properties: +- compatible: should be one of the following string: + "allwinner,sun8i-a83t-emac" + "allwinner,sun8i-h3-emac" + "allwinner,sun50i-a64-emac" +- reg: address and length of the register for the device. +- reg-names: should be "emac" +- syscon: A phandle to the syscon of the SoC +- 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: +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. Default is 0) +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. Default is 0) + +The TX/RX clock delay chain settings are board specific. + +Optional properties for "allwinner,sun8i-h3-emac": +- allwinner,leds-active-low: EPHY LEDs are active low + +Example: + +emac: ethernet@01c0b000 { + compatible = "allwinner,sun8i-h3-emac"; + syscon = <>; + reg = <0x01c0b000 0x104>; + reg-names = "emac"; + interrupts = ; + resets = < RST_BUS_EMAC>, << RST_BUS_EPHY>; + reset-names = "ahb", "ephy"; + clocks = < CLK_BUS_EMAC>, < CLK_BUS_EPHY>; + clock-names = "ahb", "ephy"; + #address-cells = <1>; + #size-cells = <0>; + + phy = <>; + phy-mode = "mii"; + allwinner,leds-active-low; + + phy1: ethernet-phy@1 { + reg = <1>; + }; +}; -- 2.7.3