| Message ID | 20170908071156.5115-6-clabbe.montjoie@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: stmmac: dwmac-sun8i: Handle integrated PHY | expand |
On Fri, Sep 08, 2017 at 09:11:51AM +0200, Corentin Labbe wrote: > This patch add documentation about the MDIO switch used on sun8i-h3-emac > for integrated PHY. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > .../devicetree/bindings/net/dwmac-sun8i.txt | 127 +++++++++++++++++++-- > 1 file changed, 120 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > index 725f3b187886..3fa0e54825ea 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 labelled mdio labels do not end up in the final DT (while the names do) so why are you making this change? > > Required properties of the mdio node: > - #address-cells: shall be 1 > @@ -48,14 +48,28 @@ 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: > +The following compatibles require an mdio-mux node called "mdio-mux": > + - "allwinner,sun8i-h3-emac" > + - "allwinner,sun8i-v3s-emac": > +Required properties for the mdio-mux node: > + - compatible = "mdio-mux" > + - one child mdio for the integrated mdio > + - one child mdio for the external mdio if present (V3s have none) > +Required properties for the mdio-mux children node: > + - reg: 0 for internal MDIO bus, 1 for external MDIO bus > + > +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 I'm not sure what you mean by that, you ask that it should (so not required?) be a child of the integrated mdio... > > -Example: > - > +Example with integrated PHY: > emac: ethernet@1c0b000 { > compatible = "allwinner,sun8i-h3-emac"; > syscon = <&syscon>; > @@ -72,13 +86,112 @@ emac: ethernet@1c0b000 { > phy-handle = <&int_mii_phy>; > phy-mode = "mii"; > allwinner,leds-active-low; > + > + mdio0: mdio { (You don't label it mdio here, unlike what was asked before) > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "snps,dwmac-mdio"; > + }; I think Rob wanted that node gone? > + mdio-mux { > + compatible = "mdio-mux"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + int_mdio: mdio@1 { > + reg = <0>; > + #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 > + }; > + }; ... And in your example it's a child of the mdio mux? > + ext_mdio: mdio@0 { > + reg = <1>; > + #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; > + > + mdio0: mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "snps,dwmac-mdio"; > + }; > + > + mdio-mux { > + compatible = "mdio-mux"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + int_mdio: mdio@1 { > + reg = <0>; > + #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 = <1>; > + #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 { > + compatible = "snps,dwmac-mdio"; > #address-cells = <1>; > #size-cells = <0>; > - int_mii_phy: ethernet-phy@1 { > + ext_rgmii_phy: ethernet-phy@1 { > reg = <1>; > - clocks = <&ccu CLK_BUS_EPHY>; > - resets = <&ccu RST_BUS_EPHY>; > }; > }; > }; > -- > 2.13.5 >
On Fri, Sep 08, 2017 at 09:25:38AM +0200, Maxime Ripard wrote: > On Fri, Sep 08, 2017 at 09:11:51AM +0200, Corentin Labbe wrote: > > This patch add documentation about the MDIO switch used on sun8i-h3-emac > > for integrated PHY. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > .../devicetree/bindings/net/dwmac-sun8i.txt | 127 +++++++++++++++++++-- > > 1 file changed, 120 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > > index 725f3b187886..3fa0e54825ea 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 labelled mdio > > labels do not end up in the final DT (while the names do) so why are > you making this change? > I misunderstood label/name. Anyway, this contrainst should leave due to "snps,dwmac-mdio MDIOs are automatically registered" > > > > Required properties of the mdio node: > > - #address-cells: shall be 1 > > @@ -48,14 +48,28 @@ 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: > > +The following compatibles require an mdio-mux node called "mdio-mux": > > + - "allwinner,sun8i-h3-emac" > > + - "allwinner,sun8i-v3s-emac": > > +Required properties for the mdio-mux node: > > + - compatible = "mdio-mux" > > + - one child mdio for the integrated mdio > > + - one child mdio for the external mdio if present (V3s have none) > > +Required properties for the mdio-mux children node: > > + - reg: 0 for internal MDIO bus, 1 for external MDIO bus > > + > > +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 > > I'm not sure what you mean by that, you ask that it should (so not > required?) be a child of the integrated mdio... > I will change words to "must" > > > > -Example: > > - > > +Example with integrated PHY: > > emac: ethernet@1c0b000 { > > compatible = "allwinner,sun8i-h3-emac"; > > syscon = <&syscon>; > > @@ -72,13 +86,112 @@ emac: ethernet@1c0b000 { > > phy-handle = <&int_mii_phy>; > > phy-mode = "mii"; > > allwinner,leds-active-low; > > + > > + mdio0: mdio { > > (You don't label it mdio here, unlike what was asked before) > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "snps,dwmac-mdio"; > > + }; > > I think Rob wanted that node gone? > MDIO mux does not work without a parent MDIO, either gived by "parent-bus" or directly via mdio_mux_init() (like it is the case in dwmac-sun8i) > > + mdio-mux { > > + compatible = "mdio-mux"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + int_mdio: mdio@1 { > > + reg = <0>; > > + #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 > > + }; > > + }; > > ... And in your example it's a child of the mdio mux? > So I confirm, integrated PHY must be a child of integrated MDIO (that must be a child of mdio-mux). The example is good. > > + ext_mdio: mdio@0 { > > + reg = <1>; > > + #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; > > + > > + mdio0: mdio { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "snps,dwmac-mdio"; > > + }; > > + > > + mdio-mux { > > + compatible = "mdio-mux"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + int_mdio: mdio@1 { > > + reg = <0>; > > + #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 = <1>; > > + #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 { > > + compatible = "snps,dwmac-mdio"; > > #address-cells = <1>; > > #size-cells = <0>; > > - int_mii_phy: ethernet-phy@1 { > > + ext_rgmii_phy: ethernet-phy@1 { > > reg = <1>; > > - clocks = <&ccu CLK_BUS_EPHY>; > > - resets = <&ccu RST_BUS_EPHY>; > > }; > > }; > > }; > > -- > > 2.13.5 > > > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Thanks for the review, I will fix all reported problem in next version. Regards Corentin Labbe
On Fri, Sep 08, 2017 at 09:43:25AM +0200, Corentin Labbe wrote: > On Fri, Sep 08, 2017 at 09:25:38AM +0200, Maxime Ripard wrote: > > On Fri, Sep 08, 2017 at 09:11:51AM +0200, Corentin Labbe wrote: > > > This patch add documentation about the MDIO switch used on sun8i-h3-emac > > > for integrated PHY. > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > .../devicetree/bindings/net/dwmac-sun8i.txt | 127 +++++++++++++++++++-- > > > 1 file changed, 120 insertions(+), 7 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > > > index 725f3b187886..3fa0e54825ea 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 labelled mdio > > > > labels do not end up in the final DT (while the names do) so why are > > you making this change? > > > > I misunderstood label/name. > Anyway, this contrainst should leave due to "snps,dwmac-mdio MDIOs are automatically registered" > > > > > > > Required properties of the mdio node: > > > - #address-cells: shall be 1 > > > @@ -48,14 +48,28 @@ 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: > > > +The following compatibles require an mdio-mux node called "mdio-mux": > > > + - "allwinner,sun8i-h3-emac" > > > + - "allwinner,sun8i-v3s-emac": > > > +Required properties for the mdio-mux node: > > > + - compatible = "mdio-mux" > > > + - one child mdio for the integrated mdio > > > + - one child mdio for the external mdio if present (V3s have none) > > > +Required properties for the mdio-mux children node: > > > + - reg: 0 for internal MDIO bus, 1 for external MDIO bus > > > + > > > +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 > > > > I'm not sure what you mean by that, you ask that it should (so not > > required?) be a child of the integrated mdio... > > > > I will change words to "must" > > > > > > > -Example: > > > - > > > +Example with integrated PHY: > > > emac: ethernet@1c0b000 { > > > compatible = "allwinner,sun8i-h3-emac"; > > > syscon = <&syscon>; > > > @@ -72,13 +86,112 @@ emac: ethernet@1c0b000 { > > > phy-handle = <&int_mii_phy>; > > > phy-mode = "mii"; > > > allwinner,leds-active-low; > > > + > > > + mdio0: mdio { > > > > (You don't label it mdio here, unlike what was asked before) > > > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + compatible = "snps,dwmac-mdio"; > > > + }; > > > > I think Rob wanted that node gone? > > > > MDIO mux does not work without a parent MDIO, either gived by "parent-bus" or directly via mdio_mux_init() (like it is the case in dwmac-sun8i) Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"? If the latter, then I think the node is fine, but then the mux should be a child node of it. IOW, the child of an MDIO controller should either be a mux node or slave devices. > > > > + mdio-mux { > > > + compatible = "mdio-mux"; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + int_mdio: mdio@1 { > > > + reg = <0>; unit address of 1 and reg prop of 0 don't match. Build your dtb with W=2. > > > + #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 Missing ; > > > + }; > > > + }; > > > > ... And in your example it's a child of the mdio mux? > > > > So I confirm, integrated PHY must be a child of integrated MDIO (that must be a child of mdio-mux). > The example is good. > > > > + ext_mdio: mdio@0 { > > > + reg = <1>; Another unit address mismatch. Rob
On Wed, Sep 13, 2017 at 01:20:04PM -0500, Rob Herring wrote: > On Fri, Sep 08, 2017 at 09:43:25AM +0200, Corentin Labbe wrote: > > On Fri, Sep 08, 2017 at 09:25:38AM +0200, Maxime Ripard wrote: > > > On Fri, Sep 08, 2017 at 09:11:51AM +0200, Corentin Labbe wrote: > > > > This patch add documentation about the MDIO switch used on sun8i-h3-emac > > > > for integrated PHY. > > > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > --- > > > > .../devicetree/bindings/net/dwmac-sun8i.txt | 127 +++++++++++++++++++-- > > > > 1 file changed, 120 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > > > > index 725f3b187886..3fa0e54825ea 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 labelled mdio > > > > > > labels do not end up in the final DT (while the names do) so why are > > > you making this change? > > > > > > > I misunderstood label/name. > > Anyway, this contrainst should leave due to "snps,dwmac-mdio MDIOs are automatically registered" > > > > > > > > > > Required properties of the mdio node: > > > > - #address-cells: shall be 1 > > > > @@ -48,14 +48,28 @@ 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: > > > > +The following compatibles require an mdio-mux node called "mdio-mux": > > > > + - "allwinner,sun8i-h3-emac" > > > > + - "allwinner,sun8i-v3s-emac": > > > > +Required properties for the mdio-mux node: > > > > + - compatible = "mdio-mux" > > > > + - one child mdio for the integrated mdio > > > > + - one child mdio for the external mdio if present (V3s have none) > > > > +Required properties for the mdio-mux children node: > > > > + - reg: 0 for internal MDIO bus, 1 for external MDIO bus > > > > + > > > > +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 > > > > > > I'm not sure what you mean by that, you ask that it should (so not > > > required?) be a child of the integrated mdio... > > > > > > > I will change words to "must" > > > > > > > > > > -Example: > > > > - > > > > +Example with integrated PHY: > > > > emac: ethernet@1c0b000 { > > > > compatible = "allwinner,sun8i-h3-emac"; > > > > syscon = <&syscon>; > > > > @@ -72,13 +86,112 @@ emac: ethernet@1c0b000 { > > > > phy-handle = <&int_mii_phy>; > > > > phy-mode = "mii"; > > > > allwinner,leds-active-low; > > > > + > > > > + mdio0: mdio { > > > > > > (You don't label it mdio here, unlike what was asked before) > > > > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + compatible = "snps,dwmac-mdio"; > > > > + }; > > > > > > I think Rob wanted that node gone? > > > > > > > MDIO mux does not work without a parent MDIO, either gived by "parent-bus" or directly via mdio_mux_init() (like it is the case in dwmac-sun8i) > > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"? > If the latter, then I think the node is fine, but then the mux should be > a child node of it. IOW, the child of an MDIO controller should either > be a mux node or slave devices. > It will be snps,dwmac-mdio but putting mdio-mux as a child of it (the mdio node) give me: [ 18.175338] libphy: stmmac: probed [ 18.175379] mdio_bus stmmac-0: /soc/ethernet@1c30000/mdio/mdio-mux has invalid PHY address [ 18.175408] mdio_bus stmmac-0: scan phy mdio-mux at address 0 [ 18.175450] mdio_bus stmmac-0: scan phy mdio-mux at address 1 [ 18.175482] mdio_bus stmmac-0: scan phy mdio-mux at address 2 [ 18.175513] mdio_bus stmmac-0: scan phy mdio-mux at address 3 [ 18.175544] mdio_bus stmmac-0: scan phy mdio-mux at address 4 [ 18.175575] mdio_bus stmmac-0: scan phy mdio-mux at address 5 [ 18.175607] mdio_bus stmmac-0: scan phy mdio-mux at address 6 [ 18.175638] mdio_bus stmmac-0: scan phy mdio-mux at address 7 [ 18.175669] mdio_bus stmmac-0: scan phy mdio-mux at address 8 [ 18.175700] mdio_bus stmmac-0: scan phy mdio-mux at address 9 [ 18.175731] mdio_bus stmmac-0: scan phy mdio-mux at address 10 [ 18.175762] mdio_bus stmmac-0: scan phy mdio-mux at address 11 [ 18.175795] mdio_bus stmmac-0: scan phy mdio-mux at address 12 [ 18.175827] mdio_bus stmmac-0: scan phy mdio-mux at address 13 [ 18.175858] mdio_bus stmmac-0: scan phy mdio-mux at address 14 [ 18.175889] mdio_bus stmmac-0: scan phy mdio-mux at address 15 [ 18.175919] mdio_bus stmmac-0: scan phy mdio-mux at address 16 [ 18.175951] mdio_bus stmmac-0: scan phy mdio-mux at address 17 [ 18.175982] mdio_bus stmmac-0: scan phy mdio-mux at address 18 [ 18.176014] mdio_bus stmmac-0: scan phy mdio-mux at address 19 [ 18.176045] mdio_bus stmmac-0: scan phy mdio-mux at address 20 [ 18.176076] mdio_bus stmmac-0: scan phy mdio-mux at address 21 [ 18.176107] mdio_bus stmmac-0: scan phy mdio-mux at address 22 [ 18.176139] mdio_bus stmmac-0: scan phy mdio-mux at address 23 [ 18.176170] mdio_bus stmmac-0: scan phy mdio-mux at address 24 [ 18.176202] mdio_bus stmmac-0: scan phy mdio-mux at address 25 [ 18.176233] mdio_bus stmmac-0: scan phy mdio-mux at address 26 [ 18.176271] mdio_bus stmmac-0: scan phy mdio-mux at address 27 [ 18.176320] mdio_bus stmmac-0: scan phy mdio-mux at address 28 [ 18.176371] mdio_bus stmmac-0: scan phy mdio-mux at address 29 [ 18.176420] mdio_bus stmmac-0: scan phy mdio-mux at address 30 [ 18.176452] mdio_bus stmmac-0: scan phy mdio-mux at address 31 Adding a fake <reg> to mdio-mux remove it. Does it is acceptable ? or perhaps patching of_mdiobus_register() to not scan node with compatible "mdio-mux". > > > > > > + mdio-mux { > > > > + compatible = "mdio-mux"; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + int_mdio: mdio@1 { > > > > + reg = <0>; > > unit address of 1 and reg prop of 0 don't match. Build your dtb with > W=2. > reg are arbitrary value (like mdio-mux-mmioreg), but in our case it is easy to fix this warning. Thanks
> > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"? > > If the latter, then I think the node is fine, but then the mux should be > > a child node of it. IOW, the child of an MDIO controller should either > > be a mux node or slave devices. Hi Rob Up until now, children of an MDIO bus have been MDIO devices. Those MDIO devices are either Ethernet PHYs, Ethernet Switches, or the oddball devices that Broadcom iProc has, like generic PHYs. We have never had MDIO-muxes as MDIO children. A Mux is not an MDIO device, and does not have the properties of an MDIO device. It is not addressable on the MDIO bus. The current MUXes are addressed via GPIOs or MMIO. There other similar cases. i2c-mux-gpio is not a child of an i2c bus, nor i2c-mux-reg or gpio-mux. nxp,pca9548 is however a child of the i2c bus, because it is an i2c device itself... If the MDIO mux was an MDIO device, i would agree with you. Bit it is not, so lets not make it a child. Andrew
On Thu, Sep 14, 2017 at 09:19:49PM +0200, Andrew Lunn wrote: > > > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"? > > > If the latter, then I think the node is fine, but then the mux should be > > > a child node of it. IOW, the child of an MDIO controller should either > > > be a mux node or slave devices. > > Hi Rob > > Up until now, children of an MDIO bus have been MDIO devices. Those > MDIO devices are either Ethernet PHYs, Ethernet Switches, or the > oddball devices that Broadcom iProc has, like generic PHYs. > > We have never had MDIO-muxes as MDIO children. A Mux is not an MDIO > device, and does not have the properties of an MDIO device. It is not > addressable on the MDIO bus. The current MUXes are addressed via GPIOs > or MMIO. > > There other similar cases. i2c-mux-gpio is not a child of an i2c bus, > nor i2c-mux-reg or gpio-mux. nxp,pca9548 is however a child of the i2c > bus, because it is an i2c device itself... > > If the MDIO mux was an MDIO device, i would agree with you. Bit it is > not, so lets not make it a child. > > Andrew Hello Rob, could you anwser/confirm please. I wait on this for sending the next version. Thanks Regards Corentin Labbe
On Thu, Sep 14, 2017 at 2:19 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"? >> > If the latter, then I think the node is fine, but then the mux should be >> > a child node of it. IOW, the child of an MDIO controller should either >> > be a mux node or slave devices. > > Hi Rob > > Up until now, children of an MDIO bus have been MDIO devices. Those > MDIO devices are either Ethernet PHYs, Ethernet Switches, or the > oddball devices that Broadcom iProc has, like generic PHYs. > > We have never had MDIO-muxes as MDIO children. A Mux is not an MDIO > device, and does not have the properties of an MDIO device. It is not > addressable on the MDIO bus. The current MUXes are addressed via GPIOs > or MMIO. The DT parent/child relationship defines the bus topology. We describe MDIO buses in that way and if a mux is sitting between the controller and the devices, then the DT hierarchy should reflect that. Now sometimes we have 2 options for what interface has the parent/child relationship (e.g. an I2C controlled USB hub chip), but in this case we don't. > There other similar cases. i2c-mux-gpio is not a child of an i2c bus, > nor i2c-mux-reg or gpio-mux. nxp,pca9548 is however a child of the i2c > bus, because it is an i2c device itself... Some are i2c controlled mux devices, but some can be GPIO controlled. > > If the MDIO mux was an MDIO device, i would agree with you. Bit it is > not, so lets not make it a child. > > Andrew > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Sep 19, 2017 at 09:49:52PM -0500, Rob Herring wrote: > On Thu, Sep 14, 2017 at 2:19 PM, Andrew Lunn <andrew@lunn.ch> wrote: > >> > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"? > >> > If the latter, then I think the node is fine, but then the mux should be > >> > a child node of it. IOW, the child of an MDIO controller should either > >> > be a mux node or slave devices. > > > > Hi Rob > > > > Up until now, children of an MDIO bus have been MDIO devices. Those > > MDIO devices are either Ethernet PHYs, Ethernet Switches, or the > > oddball devices that Broadcom iProc has, like generic PHYs. > > > > We have never had MDIO-muxes as MDIO children. A Mux is not an MDIO > > device, and does not have the properties of an MDIO device. It is not > > addressable on the MDIO bus. The current MUXes are addressed via GPIOs > > or MMIO. > > The DT parent/child relationship defines the bus topology. We describe > MDIO buses in that way and if a mux is sitting between the controller > and the devices, then the DT hierarchy should reflect that. Now > sometimes we have 2 options for what interface has the parent/child > relationship (e.g. an I2C controlled USB hub chip), but in this case > we don't. > Putting mdio-mux as a child of it (the mdio node) give me: [ 18.175338] libphy: stmmac: probed [ 18.175379] mdio_bus stmmac-0: /soc/ethernet@1c30000/mdio/mdio-mux has invalid PHY address [ 18.175408] mdio_bus stmmac-0: scan phy mdio-mux at address 0 [ 18.175450] mdio_bus stmmac-0: scan phy mdio-mux at address 1 [ 18.175482] mdio_bus stmmac-0: scan phy mdio-mux at address 2 [ 18.175513] mdio_bus stmmac-0: scan phy mdio-mux at address 3 [ 18.175544] mdio_bus stmmac-0: scan phy mdio-mux at address 4 [ 18.175575] mdio_bus stmmac-0: scan phy mdio-mux at address 5 [ 18.175607] mdio_bus stmmac-0: scan phy mdio-mux at address 6 [ 18.175638] mdio_bus stmmac-0: scan phy mdio-mux at address 7 [ 18.175669] mdio_bus stmmac-0: scan phy mdio-mux at address 8 [ 18.175700] mdio_bus stmmac-0: scan phy mdio-mux at address 9 [ 18.175731] mdio_bus stmmac-0: scan phy mdio-mux at address 10 [ 18.175762] mdio_bus stmmac-0: scan phy mdio-mux at address 11 [ 18.175795] mdio_bus stmmac-0: scan phy mdio-mux at address 12 [ 18.175827] mdio_bus stmmac-0: scan phy mdio-mux at address 13 [ 18.175858] mdio_bus stmmac-0: scan phy mdio-mux at address 14 [ 18.175889] mdio_bus stmmac-0: scan phy mdio-mux at address 15 [ 18.175919] mdio_bus stmmac-0: scan phy mdio-mux at address 16 [ 18.175951] mdio_bus stmmac-0: scan phy mdio-mux at address 17 [ 18.175982] mdio_bus stmmac-0: scan phy mdio-mux at address 18 [ 18.176014] mdio_bus stmmac-0: scan phy mdio-mux at address 19 [ 18.176045] mdio_bus stmmac-0: scan phy mdio-mux at address 20 [ 18.176076] mdio_bus stmmac-0: scan phy mdio-mux at address 21 [ 18.176107] mdio_bus stmmac-0: scan phy mdio-mux at address 22 [ 18.176139] mdio_bus stmmac-0: scan phy mdio-mux at address 23 [ 18.176170] mdio_bus stmmac-0: scan phy mdio-mux at address 24 [ 18.176202] mdio_bus stmmac-0: scan phy mdio-mux at address 25 [ 18.176233] mdio_bus stmmac-0: scan phy mdio-mux at address 26 [ 18.176271] mdio_bus stmmac-0: scan phy mdio-mux at address 27 [ 18.176320] mdio_bus stmmac-0: scan phy mdio-mux at address 28 [ 18.176371] mdio_bus stmmac-0: scan phy mdio-mux at address 29 [ 18.176420] mdio_bus stmmac-0: scan phy mdio-mux at address 30 [ 18.176452] mdio_bus stmmac-0: scan phy mdio-mux at address 31 Adding a fake <reg> to mdio-mux remove it, but I found that a bit ugly. Or perhaps patching of_mdiobus_register() to not scan node with compatible "mdio-mux". What do you think ? Regards
diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt index 725f3b187886..3fa0e54825ea 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 labelled mdio Required properties of the mdio node: - #address-cells: shall be 1 @@ -48,14 +48,28 @@ 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: +The following compatibles require an mdio-mux node called "mdio-mux": + - "allwinner,sun8i-h3-emac" + - "allwinner,sun8i-v3s-emac": +Required properties for the mdio-mux node: + - compatible = "mdio-mux" + - one child mdio for the integrated mdio + - one child mdio for the external mdio if present (V3s have none) +Required properties for the mdio-mux children node: + - reg: 0 for internal MDIO bus, 1 for external MDIO bus + +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>; @@ -72,13 +86,112 @@ emac: ethernet@1c0b000 { phy-handle = <&int_mii_phy>; phy-mode = "mii"; allwinner,leds-active-low; + + mdio0: mdio { + #address-cells = <1>; + #size-cells = <0>; + compatible = "snps,dwmac-mdio"; + }; + + mdio-mux { + compatible = "mdio-mux"; + #address-cells = <1>; + #size-cells = <0>; + + int_mdio: mdio@1 { + reg = <0>; + #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 = <1>; + #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; + + mdio0: mdio { + #address-cells = <1>; + #size-cells = <0>; + compatible = "snps,dwmac-mdio"; + }; + + mdio-mux { + compatible = "mdio-mux"; + #address-cells = <1>; + #size-cells = <0>; + + int_mdio: mdio@1 { + reg = <0>; + #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 = <1>; + #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 { + compatible = "snps,dwmac-mdio"; #address-cells = <1>; #size-cells = <0>; - int_mii_phy: ethernet-phy@1 { + ext_rgmii_phy: ethernet-phy@1 { reg = <1>; - clocks = <&ccu CLK_BUS_EPHY>; - resets = <&ccu RST_BUS_EPHY>; }; }; };
This patch add documentation about the MDIO switch used on sun8i-h3-emac for integrated PHY. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- .../devicetree/bindings/net/dwmac-sun8i.txt | 127 +++++++++++++++++++-- 1 file changed, 120 insertions(+), 7 deletions(-)