Re: [PATCH v3 4/4] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY

From: Corentin Labbe
Date: Sat Aug 19 2017 - 14:34:19 EST

On Sat, Aug 19, 2017 at 12:57:07AM +0800, Chen-Yu Tsai wrote:
> On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
> <clabbe.montjoie@xxxxxxxxx> wrote:
> > This patch add documentation about the MDIO switch used on sun8i-h3-emac
> > for integrated PHY.
> >
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@xxxxxxxxx>
> > ---
> > .../devicetree/bindings/net/dwmac-sun8i.txt | 112 +++++++++++++++++++--
> > 1 file changed, 105 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> > index 725f3b187886..631084122532 100644
> > --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> > +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> > @@ -39,7 +39,7 @@ Optional properties for the following compatibles:
> > - allwinner,leds-active-low: EPHY LEDs are active low
> >
> > Required child node of emac:
> > -- mdio bus node: should be named mdio
> > +- mdio bus node: should be named mdio (or mdio_parent in case of H3)
>
> The node would still be named "mdio". You are confusing the node name
> with the label. But you don't really need the mdio node for the H3.
> You could say here that it is not needed for the H3. See below.
>
> >
> > Required properties of the mdio node:
> > - #address-cells: shall be 1
> > @@ -48,14 +48,21 @@ Required properties of the mdio node:
> > The device node referenced by "phy" or "phy-handle" should be a child node
> > of the mdio node. See phy.txt for the generic PHY bindings.
> >
> > -Required properties of the phy node with the following compatibles:
> > +Required mdio-mux nodes for the following compatibles:
>
> The following compatibles require an mdio-mux node:
>
> > + - "allwinner,sun8i-h3-emac",
>
> Drop the comma. It's already a list.
>
> > +- a mdio-mux node with compatible = "allwinner,sun8i-h3-mdio-switch"
>
> Required properties for the mdio-mux node:
> - compatible: "allwinner,sun8i-h3-mdio-switch"
>
> > + - mdio-parent-bus: The parent bus is "mdio_parent"
>
> This is optional in the mdio-mux binding. Since you have the mdio-mux under
> the EMAC node, it should be obvious that it is connected to the EMAC. The
> parent phandle is more useful for muxes that are separate from the controller,
> such as an external GPIO-controlled mux. So you could just drop it.
>
> > + - two child mdio, one for the integrated mdio, one for the external mdio
> > +
> > +Required properties of the integrated phy node with the following compatibles:
>
> The following compatibles require a PHY node representing the integrated
> PHY, under the integrated MDIO bus node if an mdio-mux node is used:
>
> > - "allwinner,sun8i-h3-emac",
> > - "allwinner,sun8i-v3s-emac":
>
> Required properties of the integrated PHY node:
>
> > - clocks: a phandle to the reference clock for the EPHY
> > - resets: a phandle to the reset control for the EPHY
> > +- phy-is-integrated
> > +- Should be a child of the integrated mdio
> >
> > -Example:
> > -
> > +Example with integrated PHY:
> > emac: ethernet@1c0b000 {
> > compatible = "allwinner,sun8i-h3-emac";
> > syscon = <&syscon>;
> > @@ -75,10 +82,101 @@ emac: ethernet@1c0b000 {
> > mdio: mdio {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > - int_mii_phy: ethernet-phy@1 {
> > + };
> > + mdio-mux {
>
> Drop the mdio node as mentioned above. And also have spaces between
> blocks to be consistent in styling. It's also easier to read.
>
> > + compatible = "allwinner,sun8i-h3-mdio-switch";
> > + mdio-parent-bus = <&mdio>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + int_mdio: mdio@1 {
> > + reg = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + int_mii_phy: ethernet-phy@1 {
> > + reg = <1>;
> > + clocks = <&ccu CLK_BUS_EPHY>;
> > + resets = <&ccu RST_BUS_EPHY>;
> > + phy-is-integrated
> > + };
> > + };
> > + ext_mdio: mdio@0 {
> > + reg = <0>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + };
> > +};
> > +
> > +Example with external PHY:
> > +emac: ethernet@1c0b000 {
> > + compatible = "allwinner,sun8i-h3-emac";
> > + syscon = <&syscon>;
> > + reg = <0x01c0b000 0x104>;
> > + interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "macirq";
> > + resets = <&ccu RST_BUS_EMAC>;
> > + reset-names = "stmmaceth";
> > + clocks = <&ccu CLK_BUS_EMAC>;
> > + clock-names = "stmmaceth";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + phy-handle = <&ext_rgmii_phy>;
> > + phy-mode = "rgmii";
> > + allwinner,leds-active-low;
>
> You don't need this when using the external PHY.
>
> Regards
> ChenYu
>
> > + mdio: mdio {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + };
> > + mdio-mux {
> > + compatible = "allwinner,sun8i-h3-mdio-switch";
> > + mdio-parent-bus = <&mdio>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + int_mdio: mdio@1 {
> > + reg = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + int_mii_phy: ethernet-phy@1 {
> > + reg = <1>;
> > + clocks = <&ccu CLK_BUS_EPHY>;
> > + resets = <&ccu RST_BUS_EPHY>;
> > + phy-is-integrated
> > + };
> > + };
> > + ext_mdio: mdio@0 {
> > + reg = <0>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + ext_rgmii_phy: ethernet-phy@1 {
> > + reg = <1>;
> > + };
> > + };
> > +};
> > +
> > +Example with SoC without integrated PHY
> > +
> > +emac: ethernet@1c0b000 {
> > + compatible = "allwinner,sun8i-a83t-emac";
> > + syscon = <&syscon>;
> > + reg = <0x01c0b000 0x104>;
> > + interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "macirq";
> > + resets = <&ccu RST_BUS_EMAC>;
> > + reset-names = "stmmaceth";
> > + clocks = <&ccu CLK_BUS_EMAC>;
> > + clock-names = "stmmaceth";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + phy-handle = <&ext_rgmii_phy>;
> > + phy-mode = "rgmii";
> > + mdio: mdio {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + ext_rgmii_phy: ethernet-phy@1 {
> > reg = <1>;
> > - clocks = <&ccu CLK_BUS_EPHY>;
> > - resets = <&ccu RST_BUS_EPHY>;
> > };
> > };
> > };
> > --
> > 2.13.0
> >

Thanks I will fix all in next version.
Regards