| Message ID | 20170927073414.17361-6-clabbe.montjoie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Sep 27, 2017 at 07:34:08AM +0000, 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> This should be squashed with patch 1. Maxime
Hi Corentin > +Required properties for the mdio-mux node: > + - compatible = "mdio-mux" This is too generic. Please add a more specific compatible for this particular mux. You can keep "mdio-mux", since that is what the MDIO subsystem will look for. > +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 So the last thing you said is that the mux is not the problem here. Something else is locking up. Did you discover what? I really would like phy-is-integrated to go away. Andrew
On 09/27/2017 12:34 AM, 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 | 138 +++++++++++++++++++-- > 1 file changed, 126 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > index 725f3b187886..e2ef4683df08 100644 > --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > @@ -4,18 +4,18 @@ This device is a platform glue layer for stmmac. > Please see stmmac.txt for the other unchanged properties. > > Required properties: > -- compatible: should be one of the following string: > +- compatible: must be one of the following string: > "allwinner,sun8i-a83t-emac" > "allwinner,sun8i-h3-emac" > "allwinner,sun8i-v3s-emac" > "allwinner,sun50i-a64-emac" > - reg: address and length of the register for the device. > - interrupts: interrupt for the device > -- interrupt-names: should be "macirq" > +- interrupt-names: must be "macirq" > - clocks: A phandle to the reference clock for this device > -- clock-names: should be "stmmaceth" > +- clock-names: must be "stmmaceth" > - resets: A phandle to the reset control for this device > -- reset-names: should be "stmmaceth" > +- reset-names: must be "stmmaceth" > - phy-mode: See ethernet.txt > - phy-handle: See ethernet.txt > - #address-cells: shall be 1 > @@ -39,23 +39,38 @@ 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: with compatible "snps,dwmac-mdio" > > Required properties of the mdio node: > - #address-cells: shall be 1 > - #size-cells: shall be 0 > > -The device node referenced by "phy" or "phy-handle" should be a child node > +The device node referenced by "phy" or "phy-handle" must 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 that the mdio node have a mdio-mux child > +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: 1 for internal MDIO bus, 2 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 > +- Must be a child of the integrated mdio > > -Example: > - > +Example with integrated PHY: > emac: ethernet@1c0b000 { > compatible = "allwinner,sun8i-h3-emac"; > syscon = <&syscon>; > @@ -72,13 +87,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>; Sorry for chiming in so late, but why don't we have the mdio-mux be the root node here in the mdio bus hierarchy? I understand that with this binding proposed here, we need to have patch 11 included (which btw, should come before any DTS change), but this does not seem to accurately model the HW. The mux itself is not a child node of the MDIO bus controller, it does not really belong in that address space although it does mangle the MDIO bus controller address space between the two ends of the mux. If this has been debated before, apologies for missing that part of the discussion. > + > + 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@2 { > + reg = <2>; > + #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 = <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@2 { > + reg = <2>; > + #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>; > }; > }; > }; >
On Wed, Sep 27, 2017 at 09:53:15PM -0700, Florian Fainelli wrote: > > > On 09/27/2017 12:34 AM, 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 | 138 +++++++++++++++++++-- > > 1 file changed, 126 insertions(+), 12 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > > index 725f3b187886..e2ef4683df08 100644 > > --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > > +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt > > @@ -4,18 +4,18 @@ This device is a platform glue layer for stmmac. > > Please see stmmac.txt for the other unchanged properties. > > > > Required properties: > > -- compatible: should be one of the following string: > > +- compatible: must be one of the following string: > > "allwinner,sun8i-a83t-emac" > > "allwinner,sun8i-h3-emac" > > "allwinner,sun8i-v3s-emac" > > "allwinner,sun50i-a64-emac" > > - reg: address and length of the register for the device. > > - interrupts: interrupt for the device > > -- interrupt-names: should be "macirq" > > +- interrupt-names: must be "macirq" > > - clocks: A phandle to the reference clock for this device > > -- clock-names: should be "stmmaceth" > > +- clock-names: must be "stmmaceth" > > - resets: A phandle to the reset control for this device > > -- reset-names: should be "stmmaceth" > > +- reset-names: must be "stmmaceth" > > - phy-mode: See ethernet.txt > > - phy-handle: See ethernet.txt > > - #address-cells: shall be 1 > > @@ -39,23 +39,38 @@ 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: with compatible "snps,dwmac-mdio" > > > > Required properties of the mdio node: > > - #address-cells: shall be 1 > > - #size-cells: shall be 0 > > > > -The device node referenced by "phy" or "phy-handle" should be a child node > > +The device node referenced by "phy" or "phy-handle" must 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 that the mdio node have a mdio-mux child > > +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: 1 for internal MDIO bus, 2 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 > > +- Must be a child of the integrated mdio > > > > -Example: > > - > > +Example with integrated PHY: > > emac: ethernet@1c0b000 { > > compatible = "allwinner,sun8i-h3-emac"; > > syscon = <&syscon>; > > @@ -72,13 +87,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>; > > Sorry for chiming in so late, but why don't we have the mdio-mux be the > root node here in the mdio bus hierarchy? I understand that with this > binding proposed here, we need to have patch 11 included (which btw, > should come before any DTS change), but this does not seem to accurately > model the HW. > > The mux itself is not a child node of the MDIO bus controller, it does > not really belong in that address space although it does mangle the MDIO > bus controller address space between the two ends of the mux. > > If this has been debated before, apologies for missing that part of the > discussion. > I have done it as asked by Rob. https://lkml.org/lkml/2017/9/13/422 https://lkml.org/lkml/2017/9/19/849 Regards
On Wed, Sep 27, 2017 at 04:02:10PM +0200, Andrew Lunn wrote: > Hi Corentin > > > +Required properties for the mdio-mux node: > > + - compatible = "mdio-mux" > > This is too generic. Please add a more specific compatible for this > particular mux. You can keep "mdio-mux", since that is what the MDIO > subsystem will look for. > I will add allwinner,sun8i-h3-mdio-mux > > +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 > > So the last thing you said is that the mux is not the problem > here. Something else is locking up. Did you discover what? > > I really would like phy-is-integrated to go away. > I have found the problem: by enabling ephy clk/reset the timeout does not occur anymore. So we could remove phy-is-integrated by: Moving internal phy clk/reset handling in mdio_mux_syscon_switch_fn() But this means: - getting internalphy node always by manually get internal_mdio/internal_phy (and not by the given phyhandle) - doing some unnecessary tasks (enable/scan/disable) when external_phy is needed Regards
On Thu, Sep 28, 2017 at 09:37:08AM +0200, Corentin Labbe wrote: > On Wed, Sep 27, 2017 at 04:02:10PM +0200, Andrew Lunn wrote: > > Hi Corentin > > > > > +Required properties for the mdio-mux node: > > > + - compatible = "mdio-mux" > > > > This is too generic. Please add a more specific compatible for this > > particular mux. You can keep "mdio-mux", since that is what the MDIO > > subsystem will look for. > > > > I will add allwinner,sun8i-h3-mdio-mux > > > > +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 > > > > So the last thing you said is that the mux is not the problem > > here. Something else is locking up. Did you discover what? > > > > I really would like phy-is-integrated to go away. > > > > I have found the problem: by enabling ephy clk/reset the timeout does not occur anymore. > So we could remove phy-is-integrated by: > Moving internal phy clk/reset handling in mdio_mux_syscon_switch_fn() > But this means: > - getting internalphy node always by manually get internal_mdio/internal_phy (and not by the given phyhandle) > - doing some unnecessary tasks (enable/scan/disable) when external_phy is needed > Hello I have get rid of phy-is-integrated, but mdio_mux_syscon_switch_fn need to enable/disable ephy clk/reset. And so access to internal PHY node. But current DT made this ugly: (need to find mdio-mux then internalmdio then internal PHY) Since MAC cannot reset/choose internal MDIO without ephy clk/rst, could we interpret this as thoses clk/rst must be set in emac node. This will simplify a lot the code. Regards
diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt index 725f3b187886..e2ef4683df08 100644 --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt @@ -4,18 +4,18 @@ This device is a platform glue layer for stmmac. Please see stmmac.txt for the other unchanged properties. Required properties: -- compatible: should be one of the following string: +- compatible: must be one of the following string: "allwinner,sun8i-a83t-emac" "allwinner,sun8i-h3-emac" "allwinner,sun8i-v3s-emac" "allwinner,sun50i-a64-emac" - reg: address and length of the register for the device. - interrupts: interrupt for the device -- interrupt-names: should be "macirq" +- interrupt-names: must be "macirq" - clocks: A phandle to the reference clock for this device -- clock-names: should be "stmmaceth" +- clock-names: must be "stmmaceth" - resets: A phandle to the reset control for this device -- reset-names: should be "stmmaceth" +- reset-names: must be "stmmaceth" - phy-mode: See ethernet.txt - phy-handle: See ethernet.txt - #address-cells: shall be 1 @@ -39,23 +39,38 @@ 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: with compatible "snps,dwmac-mdio" Required properties of the mdio node: - #address-cells: shall be 1 - #size-cells: shall be 0 -The device node referenced by "phy" or "phy-handle" should be a child node +The device node referenced by "phy" or "phy-handle" must 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 that the mdio node have a mdio-mux child +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: 1 for internal MDIO bus, 2 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 +- Must be a child of the integrated mdio -Example: - +Example with integrated PHY: emac: ethernet@1c0b000 { compatible = "allwinner,sun8i-h3-emac"; syscon = <&syscon>; @@ -72,13 +87,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 = <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@2 { + reg = <2>; + #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 = <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@2 { + reg = <2>; + #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 | 138 +++++++++++++++++++-- 1 file changed, 126 insertions(+), 12 deletions(-)