[PATCH v3 0/4] net: stmmac: Detect PHY location with phy-is-integrated

[PATCH v3 0/4] net: stmmac: Detect PHY location with phy-is-integrated

[Corentin Labbe]

Hello

The current way to find if the PHY is internal is to compare DT phy-mode
and emac_variant/internal_phy.
But it will negate a possible future SoC where an external PHY use the
same phy mode than the integrated one.

This patchs series adds a new way to find if the PHY is integrated, via
the phy-is-integrated DT property.

Since it exists both integrated and external ethernet-phy@1, they are merged in
the final DTB and so share all properties.
For avoiding this, and better represent the reality, we use a MDIO mux.

Note that sun8i-v3s-emac have also an integrated PHY, but since it lacks
any external PHY support, it is not necessary to add MDIO mux to it.

The first patch should go via the sunxi tree.
Note that this serie will need backporting the patch
"Documentation: net: phy: Add phy-is-integrated binding" which is in net-next

Thanks
Regards

Changes since v2:
- Add a MDIO mux for creating distinction between integrated and external MDIO.
- phy-is-integrated is not set in dtsi.

Changes since v1:
- Dropped phy-is-integrated documentation patch since another same patch was already merged
- Moved phy-is-integrated from SoC dtsi to final board DT.

Corentin Labbe (4):
  ARM: dts: sunxi: h3/h5: represent the mdio switch used by
    sun8i-h3-emac
  net: stmmac: dwmac-sun8i: choose internal PHY via phy-is-integrated
  net: stmmac: register parent MDIO node for sun8i-h3-emac
  dt-bindings: net: dwmac-sun8i: update documentation about integrated
    PHY

 .../devicetree/bindings/net/dwmac-sun8i.txt        | 112 +++++++++++++++++++--
 arch/arm/boot/dts/sunxi-h3-h5.dtsi                 |  28 +++++-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c  |  16 +--
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  12 ++-
 4 files changed, 146 insertions(+), 22 deletions(-)

-- 
2.13.0

[PATCH v3 1/4] ARM: dts: sunxi: h3/h5: represent the mdio switch used by sun8i-h3-emac

[Corentin Labbe]

Since dwmac-sun8i could use either an integrated PHY or an external PHY
(which could be at same MDIO address), we need to represent this selection
by a MDIO switch.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 4b599b5d26f6..74fdfcc9dfd3 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -417,14 +417,32 @@
 			#size-cells = <0>;
 			status = "disabled";
 
-			mdio: mdio {
+			mdio_parent: mdio {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				int_mii_phy: ethernet-phy@1 {
-					compatible = "ethernet-phy-ieee802.3-c22";
+			};
+			mdio-mux {
+				compatible = "allwinner,sun8i-h3-mdio-switch";
+				mdio-parent-bus = <&mdio_parent>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				internal_mdio: mdio@1 {
 					reg = <1>;
-					clocks = <&ccu CLK_BUS_EPHY>;
-					resets = <&ccu RST_BUS_EPHY>;
+					#address-cells = <1>;
+					#size-cells = <0>;
+					int_mii_phy: ethernet-phy@1 {
+						compatible = "ethernet-phy-ieee802.3-c22";
+						reg = <1>;
+						clocks = <&ccu CLK_BUS_EPHY>;
+						resets = <&ccu RST_BUS_EPHY>;
+						phy-is-integrated;
+					};
+				};
+				mdio: mdio@0 {
+					reg = <0>;
+					#address-cells = <1>;
+					#size-cells = <0>;
 				};
 			};
 		};
-- 
2.13.0

[PATCH v3 2/4] net: stmmac: dwmac-sun8i: choose internal PHY via phy-is-integrated

[Corentin Labbe]

The current way to find if the phy is internal is to compare DT phy-mode
and emac_variant/internal_phy.
But it will negate a possible future SoC where an external PHY use the
same phy mode than the internal one.

This patch adds a new way to find if the PHY is internal, via
the phy-is-integrated property.

Since the internal_phy variable does not need anymore to contain the xMII mode
used by the internal PHY, it is still used for knowing the presence of an
internal PHY, so it is modified to a boolean soc_has_internal_phy.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index fffd6d5fc907..672553b652bd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -41,14 +41,14 @@
  *				This value is used for disabling properly EMAC
  *				and used as a good starting value in case of the
  *				boot process(uboot) leave some stuff.
- * @internal_phy:		Does the MAC embed an internal PHY
+ * @soc_has_internal_phy:	Does the MAC embed an internal PHY
  * @support_mii:		Does the MAC handle MII
  * @support_rmii:		Does the MAC handle RMII
  * @support_rgmii:		Does the MAC handle RGMII
  */
 struct emac_variant {
 	u32 default_syscon_value;
-	int internal_phy;
+	bool soc_has_internal_phy;
 	bool support_mii;
 	bool support_rmii;
 	bool support_rgmii;
@@ -75,7 +75,7 @@ struct sunxi_priv_data {
 
 static const struct emac_variant emac_variant_h3 = {
 	.default_syscon_value = 0x58000,
-	.internal_phy = PHY_INTERFACE_MODE_MII,
+	.soc_has_internal_phy = true,
 	.support_mii = true,
 	.support_rmii = true,
 	.support_rgmii = true
@@ -83,20 +83,20 @@ static const struct emac_variant emac_variant_h3 = {
 
 static const struct emac_variant emac_variant_v3s = {
 	.default_syscon_value = 0x38000,
-	.internal_phy = PHY_INTERFACE_MODE_MII,
+	.soc_has_internal_phy = true,
 	.support_mii = true
 };
 
 static const struct emac_variant emac_variant_a83t = {
 	.default_syscon_value = 0,
-	.internal_phy = 0,
+	.soc_has_internal_phy = false,
 	.support_mii = true,
 	.support_rgmii = true
 };
 
 static const struct emac_variant emac_variant_a64 = {
 	.default_syscon_value = 0,
-	.internal_phy = 0,
+	.soc_has_internal_phy = false,
 	.support_mii = true,
 	.support_rmii = true,
 	.support_rgmii = true
@@ -648,7 +648,7 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 			 "Current syscon value is not the default %x (expect %x)\n",
 			 val, reg);
 
-	if (gmac->variant->internal_phy) {
+	if (gmac->variant->soc_has_internal_phy) {
 		if (!gmac->use_internal_phy) {
 			/* switch to external PHY interface */
 			reg &= ~H3_EPHY_SELECT;
@@ -932,7 +932,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 	}
 
 	plat_dat->interface = of_get_phy_mode(dev->of_node);
-	if (plat_dat->interface == gmac->variant->internal_phy) {
+	if (of_property_read_bool(plat_dat->phy_node, "phy-is-integrated")) {
 		dev_info(&pdev->dev, "Will use internal PHY\n");
 		gmac->use_internal_phy = true;
 		gmac->ephy_clk = of_clk_get(plat_dat->phy_node, 0);
-- 
2.13.0

Re: [PATCH v3 2/4] net: stmmac: dwmac-sun8i: choose internal PHY via phy-is-integrated

[Chen-Yu Tsai]

On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
<clabbe.montjoie@gmail.com> wrote:
> The current way to find if the phy is internal is to compare DT phy-mode
> and emac_variant/internal_phy.
> But it will negate a possible future SoC where an external PHY use the
> same phy mode than the internal one.
>
> This patch adds a new way to find if the PHY is internal, via
> the phy-is-integrated property.
>
> Since the internal_phy variable does not need anymore to contain the xMII mode
> used by the internal PHY, it is still used for knowing the presence of an
> internal PHY, so it is modified to a boolean soc_has_internal_phy.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>

Acked-by: Chen-Yu Tsai <wens@csie.org>

Re: [PATCH v3 2/4] net: stmmac: dwmac-sun8i: choose internal PHY via phy-is-integrated

[Florian Fainelli]

On 08/18/2017 05:21 AM, Corentin Labbe wrote:
> The current way to find if the phy is internal is to compare DT phy-mode
> and emac_variant/internal_phy.
> But it will negate a possible future SoC where an external PHY use the
> same phy mode than the internal one.
> 
> This patch adds a new way to find if the PHY is internal, via
> the phy-is-integrated property.
> 
> Since the internal_phy variable does not need anymore to contain the xMII mode
> used by the internal PHY, it is still used for knowing the presence of an
> internal PHY, so it is modified to a boolean soc_has_internal_phy.
> 
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

In case of a MDIO switch, the registered MDIO node should be
the parent of the PHY. Otherwise of_phy_connect will fail.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index a366b3747eeb..ca3cc99d8960 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -312,10 +312,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 	static const struct of_device_id need_mdio_ids[] = {
 		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
 		{ .compatible = "allwinner,sun8i-a83t-emac" },
-		{ .compatible = "allwinner,sun8i-h3-emac" },
 		{ .compatible = "allwinner,sun8i-v3s-emac" },
 		{ .compatible = "allwinner,sun50i-a64-emac" },
 	};
+	static const struct of_device_id need_mdio_mux_ids[] = {
+		{ .compatible = "allwinner,sun8i-h3-emac" },
+	};
 
 	/* If phy-handle property is passed from DT, use it as the PHY */
 	plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
@@ -332,7 +334,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 		mdio = false;
 	}
 
-	if (of_match_node(need_mdio_ids, np)) {
+	/*
+	 * In case of a MDIO switch/mux, the registered MDIO node should be
+	 * the parent of the PHY. Otherwise of_phy_connect will fail.
+	 */
+	if (of_match_node(need_mdio_mux_ids, np)) {
+		 plat->mdio_node =  of_get_parent(plat->phy_node);
+	} else if (of_match_node(need_mdio_ids, np)) {
 		plat->mdio_node = of_get_child_by_name(np, "mdio");
 	} else {
 		/**
-- 
2.13.0

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Chen-Yu Tsai]

On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
<clabbe.montjoie@gmail.com> wrote:
> In case of a MDIO switch, the registered MDIO node should be
> the parent of the PHY. Otherwise of_phy_connect will fail.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index a366b3747eeb..ca3cc99d8960 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -312,10 +312,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>         static const struct of_device_id need_mdio_ids[] = {
>                 { .compatible = "snps,dwc-qos-ethernet-4.10" },
>                 { .compatible = "allwinner,sun8i-a83t-emac" },
> -               { .compatible = "allwinner,sun8i-h3-emac" },
>                 { .compatible = "allwinner,sun8i-v3s-emac" },
>                 { .compatible = "allwinner,sun50i-a64-emac" },
>         };
> +       static const struct of_device_id need_mdio_mux_ids[] = {
> +               { .compatible = "allwinner,sun8i-h3-emac" },
> +       };
>
>         /* If phy-handle property is passed from DT, use it as the PHY */
>         plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
> @@ -332,7 +334,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>                 mdio = false;
>         }
>
> -       if (of_match_node(need_mdio_ids, np)) {
> +       /*
> +        * In case of a MDIO switch/mux, the registered MDIO node should be
> +        * the parent of the PHY. Otherwise of_phy_connect will fail.
> +        */
> +       if (of_match_node(need_mdio_mux_ids, np)) {
> +                plat->mdio_node =  of_get_parent(plat->phy_node);

Extra space before of_get_parent.

Also this is going to fail horribly if a fixed link is used.

ChenYu

> +       } else if (of_match_node(need_mdio_ids, np)) {
>                 plat->mdio_node = of_get_child_by_name(np, "mdio");
>         } else {
>                 /**
> --
> 2.13.0
>

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Sat, Aug 19, 2017 at 01:05:21AM +0800, Chen-Yu Tsai wrote:
> On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
> <clabbe.montjoie@gmail.com> wrote:
> > In case of a MDIO switch, the registered MDIO node should be
> > the parent of the PHY. Otherwise of_phy_connect will fail.
> >
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index a366b3747eeb..ca3cc99d8960 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -312,10 +312,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> >         static const struct of_device_id need_mdio_ids[] = {
> >                 { .compatible = "snps,dwc-qos-ethernet-4.10" },
> >                 { .compatible = "allwinner,sun8i-a83t-emac" },
> > -               { .compatible = "allwinner,sun8i-h3-emac" },
> >                 { .compatible = "allwinner,sun8i-v3s-emac" },
> >                 { .compatible = "allwinner,sun50i-a64-emac" },
> >         };
> > +       static const struct of_device_id need_mdio_mux_ids[] = {
> > +               { .compatible = "allwinner,sun8i-h3-emac" },
> > +       };
> >
> >         /* If phy-handle property is passed from DT, use it as the PHY */
> >         plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
> > @@ -332,7 +334,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> >                 mdio = false;
> >         }
> >
> > -       if (of_match_node(need_mdio_ids, np)) {
> > +       /*
> > +        * In case of a MDIO switch/mux, the registered MDIO node should be
> > +        * the parent of the PHY. Otherwise of_phy_connect will fail.
> > +        */
> > +       if (of_match_node(need_mdio_mux_ids, np)) {
> > +                plat->mdio_node =  of_get_parent(plat->phy_node);
> 
> Extra space before of_get_parent.
> 
> Also this is going to fail horribly if a fixed link is used.
> 

Hello

I will add an extra patch for handling fixed-link for both need_mdio_mux_ids/need_mdio_ids cases.

Thanks
Regards

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Andrew Lunn]

On Sat, Aug 19, 2017 at 08:50:25PM +0200, Corentin Labbe wrote:
> On Sat, Aug 19, 2017 at 01:05:21AM +0800, Chen-Yu Tsai wrote:
> > On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
> > <clabbe.montjoie@gmail.com> wrote:
> > > In case of a MDIO switch, the registered MDIO node should be
> > > the parent of the PHY. Otherwise of_phy_connect will fail.

Hi Corentin

Sorry, I missed this patch series. Looking at patchwork...

Can you represent the MDIO mux using 

Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt

It would be better if you could reuse existing infrastructure than
invent something new.

   Andrew

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Sat, Aug 19, 2017 at 10:38:36PM +0200, Andrew Lunn wrote:
> On Sat, Aug 19, 2017 at 08:50:25PM +0200, Corentin Labbe wrote:
> > On Sat, Aug 19, 2017 at 01:05:21AM +0800, Chen-Yu Tsai wrote:
> > > On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
> > > <clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > In case of a MDIO switch, the registered MDIO node should be
> > > > the parent of the PHY. Otherwise of_phy_connect will fail.
> 
> Hi Corentin
> 
> Sorry, I missed this patch series. Looking at patchwork...

That's my fault, I forgot to set you in recipient like in last send.

> 
> Can you represent the MDIO mux using 
> 
> Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
> 
> It would be better if you could reuse existing infrastructure than
> invent something new.
> 

I think we cannot use mdio-mux-mmioreg since the register for doing the switch is in middle of the "System Control" and shared with other functions.
This is why we use a sycon/regmap for selecting the MDIO.

Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Andrew Lunn]

> I think we cannot use mdio-mux-mmioreg since the register for doing
> the switch is in middle of the "System Control" and shared with
> other functions.  This is why we use a sycon/regmap for selecting
> the MDIO.

You could add a mdio-mux-regmap.c.

However, it probably need restructuring of the stmmac mdio code, to
make the mdio bus usable as a separate driver. You need stmmac mdio
to probe first, then mdio-mux-remap should probe, and then lastly
stmmmac mac driver.

With stmmac mdio and stmmac mac being in one driver, there is no time
in the middle to allow the mux driver to probe.

It is some effort, but a nice cleanup and generalization.

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Chen-Yu Tsai]

On Sun, Aug 20, 2017 at 10:25 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> I think we cannot use mdio-mux-mmioreg since the register for doing
>> the switch is in middle of the "System Control" and shared with
>> other functions.  This is why we use a sycon/regmap for selecting
>> the MDIO.
>
> You could add a mdio-mux-regmap.c.
>
> However, it probably need restructuring of the stmmac mdio code, to
> make the mdio bus usable as a separate driver. You need stmmac mdio
> to probe first, then mdio-mux-remap should probe, and then lastly
> stmmmac mac driver.
>
> With stmmac mdio and stmmac mac being in one driver, there is no time
> in the middle to allow the mux driver to probe.
>
> It is some effort, but a nice cleanup and generalization.

I'm not sure mdio-mux is the right thing here.

Looking at mdio-mux, it seems to provide a way to access multiplexed
MDIO buses. It says nothing about the MAC<->PHY connection.

With our hardware, and likely Rockchip's as well, the muxed connections
include the MDIO and MII connections, which are muxed at the same time.
It's likely to cause some confusion or even bugs if we implement it as
a separate driver.

ChenYu

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Andrew Lunn]

> With our hardware, and likely Rockchip's as well, the muxed connections
> include the MDIO and MII connections

Ah, i did not realise the MII was muxed as well. Then i agree, an MDIO
mux is wrong.

However, please try to make the binding not look like an mdio mux. We
don't want people misunderstanding the binding and thinking it is an
mdio mux.

     Andrew

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

Hi Andrew,

On Mon, Aug 21, 2017 at 03:20:15PM +0200, Andrew Lunn wrote:
> > With our hardware, and likely Rockchip's as well, the muxed connections
> > include the MDIO and MII connections
> 
> Ah, i did not realise the MII was muxed as well. Then i agree, an MDIO
> mux is wrong.
> 
> However, please try to make the binding not look like an mdio mux. We
> don't want people misunderstanding the binding and thinking it is an
> mdio mux.

Do you have any suggestion on what the binding would look like?

All muxes are mostly always represented the same way afaik, or do you
want to simply introduce a new compatible / property?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Andrew Lunn]

> All muxes are mostly always represented the same way afaik, or do you
> want to simply introduce a new compatible / property?

+      	  mdio-mux {
+		compatible = "allwinner,sun8i-h3-mdio-switch";
+		mdio-parent-bus = <&mdio_parent>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		internal_mdio: mdio@1 {
			reg = <1>;
-			clocks = <&ccu CLK_BUS_EPHY>;
-			resets = <&ccu RST_BUS_EPHY>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			int_mii_phy: ethernet-phy@1 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <1>;
+				clocks = <&ccu CLK_BUS_EPHY>;
+				resets = <&ccu RST_BUS_EPHY>;
+				phy-is-integrated;
+			};
+		};
+		mdio: mdio@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 		};
 		
Hi Maxim

Anybody who knows the MDIO-mux code/binding, knows that it is a run
time mux. You swap the mux per MDIO transaction. You can access all
the PHY and switches on the mux'ed MDIO bus.

However here, it is effectively a boot-time MUX. You cannot change it
on the fly. What happens when somebody has a phandle to a PHY on the
internal and a phandle to a phy on the external? Does the driver at
least return -EINVAL, or -EBUSY? Is there a representation which
eliminates this possibility?

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Mon, Aug 21, 2017 at 04:23:21PM +0200, Andrew Lunn wrote:
> > All muxes are mostly always represented the same way afaik, or do you
> > want to simply introduce a new compatible / property?
> 
> +      	  mdio-mux {
> +		compatible = "allwinner,sun8i-h3-mdio-switch";
> +		mdio-parent-bus = <&mdio_parent>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		internal_mdio: mdio@1 {
> 			reg = <1>;
> -			clocks = <&ccu CLK_BUS_EPHY>;
> -			resets = <&ccu RST_BUS_EPHY>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			int_mii_phy: ethernet-phy@1 {
> +				compatible = "ethernet-phy-ieee802.3-c22";
> +				reg = <1>;
> +				clocks = <&ccu CLK_BUS_EPHY>;
> +				resets = <&ccu RST_BUS_EPHY>;
> +				phy-is-integrated;
> +			};
> +		};
> +		mdio: mdio@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
>  		};
>  		
> Hi Maxim
> 
> Anybody who knows the MDIO-mux code/binding, knows that it is a run
> time mux. You swap the mux per MDIO transaction. You can access all
> the PHY and switches on the mux'ed MDIO bus.
> 
> However here, it is effectively a boot-time MUX. You cannot change it
> on the fly. What happens when somebody has a phandle to a PHY on the
> internal and a phandle to a phy on the external? Does the driver at
> least return -EINVAL, or -EBUSY? Is there a representation which
> eliminates this possibility?
> 

Exactly you can change it on the fly, but you need to reset the MAC for enabling the new configuration.
The stmmac driver does not handle mdio-mux. It is why I have a patch which automaticly select parent MDIO node of the PHY node.

For representation we could keep the current. (With a big comment stating that it is a switch)

We can also add a mdio-switch type node, or add a mdio-switch property to mdio-mux.

Regards

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Chen-Yu Tsai]

On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>> All muxes are mostly always represented the same way afaik, or do you
>> want to simply introduce a new compatible / property?
>
> +         mdio-mux {
> +               compatible = "allwinner,sun8i-h3-mdio-switch";
> +               mdio-parent-bus = <&mdio_parent>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               internal_mdio: mdio@1 {
>                         reg = <1>;
> -                       clocks = <&ccu CLK_BUS_EPHY>;
> -                       resets = <&ccu RST_BUS_EPHY>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       int_mii_phy: ethernet-phy@1 {
> +                               compatible = "ethernet-phy-ieee802.3-c22";
> +                               reg = <1>;
> +                               clocks = <&ccu CLK_BUS_EPHY>;
> +                               resets = <&ccu RST_BUS_EPHY>;
> +                               phy-is-integrated;
> +                       };
> +               };
> +               mdio: mdio@0 {
> +                       reg = <0>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
>                 };
>
> Hi Maxim
>
> Anybody who knows the MDIO-mux code/binding, knows that it is a run
> time mux. You swap the mux per MDIO transaction. You can access all
> the PHY and switches on the mux'ed MDIO bus.
>
> However here, it is effectively a boot-time MUX. You cannot change it
> on the fly. What happens when somebody has a phandle to a PHY on the
> internal and a phandle to a phy on the external? Does the driver at
> least return -EINVAL, or -EBUSY? Is there a representation which
> eliminates this possibility?

There is only one controller. Either you use the internal PHY, which
is then directly coupled (no magnetics needed) to the RJ45 port, or
you use an external PHY over MII/RMII/RGMII. You could supposedly
have both on a board, and let the user choose one. But why bother
with the extra complexity and cost? Either you use the internal PHY
at 100M, or an external RGMII PHY for gigabit speeds.

So I think what you are saying is either impossible or engineering-wise
a very stupid design, like using an external MAC with a discrete PHY
connected to the internal MAC's MDIO bus, while using the internal MAC
with the internal PHY.

Now can we please decide on something? We're a week and a half from
the 4.13 release. If mdio-mux is wrong, then we could have two mdio
nodes (internal-mdio & external-mdio).

Regards
ChenYu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Florian Fainelli]

On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
> On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>>> All muxes are mostly always represented the same way afaik, or do you
>>> want to simply introduce a new compatible / property?
>>
>> +         mdio-mux {
>> +               compatible = "allwinner,sun8i-h3-mdio-switch";
>> +               mdio-parent-bus = <&mdio_parent>;
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               internal_mdio: mdio@1 {
>>                         reg = <1>;
>> -                       clocks = <&ccu CLK_BUS_EPHY>;
>> -                       resets = <&ccu RST_BUS_EPHY>;
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       int_mii_phy: ethernet-phy@1 {
>> +                               compatible = "ethernet-phy-ieee802.3-c22";
>> +                               reg = <1>;
>> +                               clocks = <&ccu CLK_BUS_EPHY>;
>> +                               resets = <&ccu RST_BUS_EPHY>;
>> +                               phy-is-integrated;
>> +                       };
>> +               };
>> +               mdio: mdio@0 {
>> +                       reg = <0>;
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>>                 };
>>
>> Hi Maxim
>>
>> Anybody who knows the MDIO-mux code/binding, knows that it is a run
>> time mux. You swap the mux per MDIO transaction. You can access all
>> the PHY and switches on the mux'ed MDIO bus.
>>
>> However here, it is effectively a boot-time MUX. You cannot change it
>> on the fly. What happens when somebody has a phandle to a PHY on the
>> internal and a phandle to a phy on the external? Does the driver at
>> least return -EINVAL, or -EBUSY? Is there a representation which
>> eliminates this possibility?
> 
> There is only one controller. Either you use the internal PHY, which
> is then directly coupled (no magnetics needed) to the RJ45 port, or
> you use an external PHY over MII/RMII/RGMII. You could supposedly
> have both on a board, and let the user choose one. But why bother
> with the extra complexity and cost? Either you use the internal PHY
> at 100M, or an external RGMII PHY for gigabit speeds.

I agree, there is no point in over-engineering any of this. I don't
think there is actually any MDIO mux per-se in that the MDIO clock and
data lines are muxed, however there has to be some kind of built-in port
multiplexer that lets you chose between connecting to the internal PHY
and any external PHY/MAC, but that is not what a "mdio-mux" node represents.

> 
> So I think what you are saying is either impossible or engineering-wise
> a very stupid design, like using an external MAC with a discrete PHY
> connected to the internal MAC's MDIO bus, while using the internal MAC
> with the internal PHY.
> 
> Now can we please decide on something? We're a week and a half from
> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> nodes (internal-mdio & external-mdio).

I really don't see a need for a mdio-mux in the first place, just have
one MDIO controller (current state) sub-node which describes the
built-in STMMAC MDIO controller and declare the internal PHY as a child
node (along with 'phy-is-integrated'). If a different configuration is
used, then just put the external PHY as a child node there.

If fixed-link is required, the mdio node becomes unused anyway.

Works for everyone?
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Tue, Aug 22, 2017 at 09:40:24AM -0700, Florian Fainelli wrote:
> On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
> > On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> >>> All muxes are mostly always represented the same way afaik, or do you
> >>> want to simply introduce a new compatible / property?
> >>
> >> +         mdio-mux {
> >> +               compatible = "allwinner,sun8i-h3-mdio-switch";
> >> +               mdio-parent-bus = <&mdio_parent>;
> >> +               #address-cells = <1>;
> >> +               #size-cells = <0>;
> >> +
> >> +               internal_mdio: mdio@1 {
> >>                         reg = <1>;
> >> -                       clocks = <&ccu CLK_BUS_EPHY>;
> >> -                       resets = <&ccu RST_BUS_EPHY>;
> >> +                       #address-cells = <1>;
> >> +                       #size-cells = <0>;
> >> +                       int_mii_phy: ethernet-phy@1 {
> >> +                               compatible = "ethernet-phy-ieee802.3-c22";
> >> +                               reg = <1>;
> >> +                               clocks = <&ccu CLK_BUS_EPHY>;
> >> +                               resets = <&ccu RST_BUS_EPHY>;
> >> +                               phy-is-integrated;
> >> +                       };
> >> +               };
> >> +               mdio: mdio@0 {
> >> +                       reg = <0>;
> >> +                       #address-cells = <1>;
> >> +                       #size-cells = <0>;
> >>                 };
> >>
> >> Hi Maxim
> >>
> >> Anybody who knows the MDIO-mux code/binding, knows that it is a run
> >> time mux. You swap the mux per MDIO transaction. You can access all
> >> the PHY and switches on the mux'ed MDIO bus.
> >>
> >> However here, it is effectively a boot-time MUX. You cannot change it
> >> on the fly. What happens when somebody has a phandle to a PHY on the
> >> internal and a phandle to a phy on the external? Does the driver at
> >> least return -EINVAL, or -EBUSY? Is there a representation which
> >> eliminates this possibility?
> > 
> > There is only one controller. Either you use the internal PHY, which
> > is then directly coupled (no magnetics needed) to the RJ45 port, or
> > you use an external PHY over MII/RMII/RGMII. You could supposedly
> > have both on a board, and let the user choose one. But why bother
> > with the extra complexity and cost? Either you use the internal PHY
> > at 100M, or an external RGMII PHY for gigabit speeds.
> 
> I agree, there is no point in over-engineering any of this. I don't
> think there is actually any MDIO mux per-se in that the MDIO clock and
> data lines are muxed, however there has to be some kind of built-in port
> multiplexer that lets you chose between connecting to the internal PHY
> and any external PHY/MAC, but that is not what a "mdio-mux" node represents.
> 
> > 
> > So I think what you are saying is either impossible or engineering-wise
> > a very stupid design, like using an external MAC with a discrete PHY
> > connected to the internal MAC's MDIO bus, while using the internal MAC
> > with the internal PHY.
> > 
> > Now can we please decide on something? We're a week and a half from
> > the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> > nodes (internal-mdio & external-mdio).
> 
> I really don't see a need for a mdio-mux in the first place, just have
> one MDIO controller (current state) sub-node which describes the
> built-in STMMAC MDIO controller and declare the internal PHY as a child
> node (along with 'phy-is-integrated'). If a different configuration is
> used, then just put the external PHY as a child node there.
> 
> If fixed-link is required, the mdio node becomes unused anyway.
> 
> Works for everyone?

If we put an external PHY with reg=1 as a child of internal MDIO, il will be merged with internal PHY node and get phy-is-integrated.

Does two MDIO node "internal-mdio" and "mdio" works for you ?
(We keep "mdio" for external MDIO for reducing the number of patchs)

Thanks
Regards

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 4b599b5d26f6..d5e7cf0d9454 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -417,7 +417,8 @@
 			#size-cells = <0>;
 			status = "disabled";
 
-			mdio: mdio {
+			/* Only one MDIO is usable at the time */
+			internal_mdio: mdio@1 {
 				#address-cells = <1>;
 				#size-cells = <0>;
 				int_mii_phy: ethernet-phy@1 {
@@ -425,8 +426,13 @@
 					reg = <1>;
 					clocks = <&ccu CLK_BUS_EPHY>;
 					resets = <&ccu RST_BUS_EPHY>;
+					phy-is-integrated;
 				};
 			};
+			mdio: mdio@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
 		};
 
 		spi0: spi@01c68000 {

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Florian Fainelli]

On 08/22/2017 11:11 AM, Corentin Labbe wrote:
> On Tue, Aug 22, 2017 at 09:40:24AM -0700, Florian Fainelli wrote:
>> On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
>>> On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>> All muxes are mostly always represented the same way afaik, or do you
>>>>> want to simply introduce a new compatible / property?
>>>>
>>>> +         mdio-mux {
>>>> +               compatible = "allwinner,sun8i-h3-mdio-switch";
>>>> +               mdio-parent-bus = <&mdio_parent>;
>>>> +               #address-cells = <1>;
>>>> +               #size-cells = <0>;
>>>> +
>>>> +               internal_mdio: mdio@1 {
>>>>                         reg = <1>;
>>>> -                       clocks = <&ccu CLK_BUS_EPHY>;
>>>> -                       resets = <&ccu RST_BUS_EPHY>;
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <0>;
>>>> +                       int_mii_phy: ethernet-phy@1 {
>>>> +                               compatible = "ethernet-phy-ieee802.3-c22";
>>>> +                               reg = <1>;
>>>> +                               clocks = <&ccu CLK_BUS_EPHY>;
>>>> +                               resets = <&ccu RST_BUS_EPHY>;
>>>> +                               phy-is-integrated;
>>>> +                       };
>>>> +               };
>>>> +               mdio: mdio@0 {
>>>> +                       reg = <0>;
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <0>;
>>>>                 };
>>>>
>>>> Hi Maxim
>>>>
>>>> Anybody who knows the MDIO-mux code/binding, knows that it is a run
>>>> time mux. You swap the mux per MDIO transaction. You can access all
>>>> the PHY and switches on the mux'ed MDIO bus.
>>>>
>>>> However here, it is effectively a boot-time MUX. You cannot change it
>>>> on the fly. What happens when somebody has a phandle to a PHY on the
>>>> internal and a phandle to a phy on the external? Does the driver at
>>>> least return -EINVAL, or -EBUSY? Is there a representation which
>>>> eliminates this possibility?
>>>
>>> There is only one controller. Either you use the internal PHY, which
>>> is then directly coupled (no magnetics needed) to the RJ45 port, or
>>> you use an external PHY over MII/RMII/RGMII. You could supposedly
>>> have both on a board, and let the user choose one. But why bother
>>> with the extra complexity and cost? Either you use the internal PHY
>>> at 100M, or an external RGMII PHY for gigabit speeds.
>>
>> I agree, there is no point in over-engineering any of this. I don't
>> think there is actually any MDIO mux per-se in that the MDIO clock and
>> data lines are muxed, however there has to be some kind of built-in port
>> multiplexer that lets you chose between connecting to the internal PHY
>> and any external PHY/MAC, but that is not what a "mdio-mux" node represents.
>>
>>>
>>> So I think what you are saying is either impossible or engineering-wise
>>> a very stupid design, like using an external MAC with a discrete PHY
>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>> with the internal PHY.
>>>
>>> Now can we please decide on something? We're a week and a half from
>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>> nodes (internal-mdio & external-mdio).
>>
>> I really don't see a need for a mdio-mux in the first place, just have
>> one MDIO controller (current state) sub-node which describes the
>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>> node (along with 'phy-is-integrated'). If a different configuration is
>> used, then just put the external PHY as a child node there.
>>
>> If fixed-link is required, the mdio node becomes unused anyway.
>>
>> Works for everyone?
> 
> If we put an external PHY with reg=1 as a child of internal MDIO, il will be merged with internal PHY node and get phy-is-integrated.

Then have the .dtsi file contain just the mdio node, but no internal or
external PHY and push all the internal and external PHY node definition
(in its entirety) to the per-board DTS file, does not that work?

> 
> Does two MDIO node "internal-mdio" and "mdio" works for you ?
> (We keep "mdio" for external MDIO for reducing the number of patchs)

How does that solve the problem and not create a new one where both MDIO
nodes end-up being registered? Does that mean you force the writer of
the board-level DTS to systematically disable the internal MDIO node
when using an external PHY and vice versa? How is that better than not
being explicit like I suggested earlier?

> 
> Thanks
> Regards
> 
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index 4b599b5d26f6..d5e7cf0d9454 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -417,7 +417,8 @@
>  			#size-cells = <0>;
>  			status = "disabled";
>  
> -			mdio: mdio {
> +			/* Only one MDIO is usable at the time */
> +			internal_mdio: mdio@1 {
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  				int_mii_phy: ethernet-phy@1 {
> @@ -425,8 +426,13 @@
>  					reg = <1>;
>  					clocks = <&ccu CLK_BUS_EPHY>;
>  					resets = <&ccu RST_BUS_EPHY>;
> +					phy-is-integrated;
>  				};
>  			};
> +			mdio: mdio@0 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
>  		};
>  
>  		spi0: spi@01c68000 {
> 


-- 
Florian

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> On 08/22/2017 11:11 AM, Corentin Labbe wrote:
> > On Tue, Aug 22, 2017 at 09:40:24AM -0700, Florian Fainelli wrote:
> >> On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
> >>> On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> >>>>> All muxes are mostly always represented the same way afaik, or do you
> >>>>> want to simply introduce a new compatible / property?
> >>>>
> >>>> +         mdio-mux {
> >>>> +               compatible = "allwinner,sun8i-h3-mdio-switch";
> >>>> +               mdio-parent-bus = <&mdio_parent>;
> >>>> +               #address-cells = <1>;
> >>>> +               #size-cells = <0>;
> >>>> +
> >>>> +               internal_mdio: mdio@1 {
> >>>>                         reg = <1>;
> >>>> -                       clocks = <&ccu CLK_BUS_EPHY>;
> >>>> -                       resets = <&ccu RST_BUS_EPHY>;
> >>>> +                       #address-cells = <1>;
> >>>> +                       #size-cells = <0>;
> >>>> +                       int_mii_phy: ethernet-phy@1 {
> >>>> +                               compatible = "ethernet-phy-ieee802.3-c22";
> >>>> +                               reg = <1>;
> >>>> +                               clocks = <&ccu CLK_BUS_EPHY>;
> >>>> +                               resets = <&ccu RST_BUS_EPHY>;
> >>>> +                               phy-is-integrated;
> >>>> +                       };
> >>>> +               };
> >>>> +               mdio: mdio@0 {
> >>>> +                       reg = <0>;
> >>>> +                       #address-cells = <1>;
> >>>> +                       #size-cells = <0>;
> >>>>                 };
> >>>>
> >>>> Hi Maxim
> >>>>
> >>>> Anybody who knows the MDIO-mux code/binding, knows that it is a run
> >>>> time mux. You swap the mux per MDIO transaction. You can access all
> >>>> the PHY and switches on the mux'ed MDIO bus.
> >>>>
> >>>> However here, it is effectively a boot-time MUX. You cannot change it
> >>>> on the fly. What happens when somebody has a phandle to a PHY on the
> >>>> internal and a phandle to a phy on the external? Does the driver at
> >>>> least return -EINVAL, or -EBUSY? Is there a representation which
> >>>> eliminates this possibility?
> >>>
> >>> There is only one controller. Either you use the internal PHY, which
> >>> is then directly coupled (no magnetics needed) to the RJ45 port, or
> >>> you use an external PHY over MII/RMII/RGMII. You could supposedly
> >>> have both on a board, and let the user choose one. But why bother
> >>> with the extra complexity and cost? Either you use the internal PHY
> >>> at 100M, or an external RGMII PHY for gigabit speeds.
> >>
> >> I agree, there is no point in over-engineering any of this. I don't
> >> think there is actually any MDIO mux per-se in that the MDIO clock and
> >> data lines are muxed, however there has to be some kind of built-in port
> >> multiplexer that lets you chose between connecting to the internal PHY
> >> and any external PHY/MAC, but that is not what a "mdio-mux" node represents.
> >>
> >>>
> >>> So I think what you are saying is either impossible or engineering-wise
> >>> a very stupid design, like using an external MAC with a discrete PHY
> >>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>> with the internal PHY.
> >>>
> >>> Now can we please decide on something? We're a week and a half from
> >>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>> nodes (internal-mdio & external-mdio).
> >>
> >> I really don't see a need for a mdio-mux in the first place, just have
> >> one MDIO controller (current state) sub-node which describes the
> >> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >> node (along with 'phy-is-integrated'). If a different configuration is
> >> used, then just put the external PHY as a child node there.
> >>
> >> If fixed-link is required, the mdio node becomes unused anyway.
> >>
> >> Works for everyone?
> > 
> > If we put an external PHY with reg=1 as a child of internal MDIO, il will be merged with internal PHY node and get phy-is-integrated.
> 
> Then have the .dtsi file contain just the mdio node, but no internal or
> external PHY and push all the internal and external PHY node definition
> (in its entirety) to the per-board DTS file, does not that work?
> 

It is near what I sent in v2 of this serie. (only one MDIO with internal PHY, but phy-is-integrated is only set per-board DTS)
But at that time Rob said to use a mdio-mux.

> > 
> > Does two MDIO node "internal-mdio" and "mdio" works for you ?
> > (We keep "mdio" for external MDIO for reducing the number of patchs)
> 
> How does that solve the problem and not create a new one where both MDIO
> nodes end-up being registered? Does that mean you force the writer of
> the board-level DTS to systematically disable the internal MDIO node
> when using an external PHY and vice versa? How is that better than not
> being explicit like I suggested earlier?
> 

Only one node is registered.
stmmac register only MDIO called "mdio".
So it is why I have added a patch "register parent MDIO node for sun8i-h3-emac" which for H3 only register the PHY's parent MDIO

But yes back to your solution "only one mdio with internal PHY," and phy-is-integrated only set on board DT which use internal PHY will work.

Regards

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

Hi Florian,

On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> >>> So I think what you are saying is either impossible or engineering-wise
> >>> a very stupid design, like using an external MAC with a discrete PHY
> >>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>> with the internal PHY.
> >>>
> >>> Now can we please decide on something? We're a week and a half from
> >>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>> nodes (internal-mdio & external-mdio).
> >>
> >> I really don't see a need for a mdio-mux in the first place, just have
> >> one MDIO controller (current state) sub-node which describes the
> >> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >> node (along with 'phy-is-integrated'). If a different configuration is
> >> used, then just put the external PHY as a child node there.
> >>
> >> If fixed-link is required, the mdio node becomes unused anyway.
> >>
> >> Works for everyone?
> > 
> > If we put an external PHY with reg=1 as a child of internal MDIO,
> > il will be merged with internal PHY node and get
> > phy-is-integrated.
> 
> Then have the .dtsi file contain just the mdio node, but no internal or
> external PHY and push all the internal and external PHY node definition
> (in its entirety) to the per-board DTS file, does not that work?

If possible, I'd really like to have the internal PHY in the
DTSI. It's always there in hardware anyway, and duplicating the PHY,
with its clock, reset line, and whatever info we might need in the
future in each and every board DTS that uses it will be very error
prone and we will have the usual bunch of issues that come up with
duplication.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Florian Fainelli]

On 08/23/2017 12:49 AM, Maxime Ripard wrote:
> Hi Florian,
> 
> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>> with the internal PHY.
>>>>>
>>>>> Now can we please decide on something? We're a week and a half from
>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>> nodes (internal-mdio & external-mdio).
>>>>
>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>> one MDIO controller (current state) sub-node which describes the
>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>> used, then just put the external PHY as a child node there.
>>>>
>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>
>>>> Works for everyone?
>>>
>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>> il will be merged with internal PHY node and get
>>> phy-is-integrated.
>>
>> Then have the .dtsi file contain just the mdio node, but no internal or
>> external PHY and push all the internal and external PHY node definition
>> (in its entirety) to the per-board DTS file, does not that work?
> 
> If possible, I'd really like to have the internal PHY in the
> DTSI. It's always there in hardware anyway, and duplicating the PHY,
> with its clock, reset line, and whatever info we might need in the
> future in each and every board DTS that uses it will be very error
> prone and we will have the usual bunch of issues that come up with
> duplication.

OK, then what if you put the internal PHY in the DTSI, mark it with a
status = "disabled" property, and have the per-board DTS put a status =
"okay" property along with a "phy-is-integrated" boolean property? Would
that work?

What I really don't think is necessary is:

- duplicating the "mdio" controller node for external vs. internal PHY,
because this is not accurate, there is just one MDIO controller, but
there may be different kinds of MDIO/PHY devices attached

- having the STMMAC driver MDIO probing code having to deal with a
"mdio" sub-node or an "internal-mdio" sub-node because this is confusing
and requiring more driver-level changes that are error prone

Thanks
-- 
Florian

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3003 bytes --]

On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
> > Hi Florian,
> > 
> > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> >>>>> So I think what you are saying is either impossible or engineering-wise
> >>>>> a very stupid design, like using an external MAC with a discrete PHY
> >>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>>>> with the internal PHY.
> >>>>>
> >>>>> Now can we please decide on something? We're a week and a half from
> >>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>>>> nodes (internal-mdio & external-mdio).
> >>>>
> >>>> I really don't see a need for a mdio-mux in the first place, just have
> >>>> one MDIO controller (current state) sub-node which describes the
> >>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >>>> node (along with 'phy-is-integrated'). If a different configuration is
> >>>> used, then just put the external PHY as a child node there.
> >>>>
> >>>> If fixed-link is required, the mdio node becomes unused anyway.
> >>>>
> >>>> Works for everyone?
> >>>
> >>> If we put an external PHY with reg=1 as a child of internal MDIO,
> >>> il will be merged with internal PHY node and get
> >>> phy-is-integrated.
> >>
> >> Then have the .dtsi file contain just the mdio node, but no internal or
> >> external PHY and push all the internal and external PHY node definition
> >> (in its entirety) to the per-board DTS file, does not that work?
> > 
> > If possible, I'd really like to have the internal PHY in the
> > DTSI. It's always there in hardware anyway, and duplicating the PHY,
> > with its clock, reset line, and whatever info we might need in the
> > future in each and every board DTS that uses it will be very error
> > prone and we will have the usual bunch of issues that come up with
> > duplication.
> 
> OK, then what if you put the internal PHY in the DTSI, mark it with a
> status = "disabled" property, and have the per-board DTS put a status =
> "okay" property along with a "phy-is-integrated" boolean property? Would
> that work?

Yeah, that would work for me.

> What I really don't think is necessary is:
> 
> - duplicating the "mdio" controller node for external vs. internal PHY,
> because this is not accurate, there is just one MDIO controller, but
> there may be different kinds of MDIO/PHY devices attached

Agreed.

> - having the STMMAC driver MDIO probing code having to deal with a
> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
> and requiring more driver-level changes that are error prone

I don't really have an opinion on that one, so I'll defer to your
judgment of what's best :)

I guess we have an agreement. Andrew, is that ok for you too?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
> > Hi Florian,
> > 
> > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> >>>>> So I think what you are saying is either impossible or engineering-wise
> >>>>> a very stupid design, like using an external MAC with a discrete PHY
> >>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>>>> with the internal PHY.
> >>>>>
> >>>>> Now can we please decide on something? We're a week and a half from
> >>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>>>> nodes (internal-mdio & external-mdio).
> >>>>
> >>>> I really don't see a need for a mdio-mux in the first place, just have
> >>>> one MDIO controller (current state) sub-node which describes the
> >>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >>>> node (along with 'phy-is-integrated'). If a different configuration is
> >>>> used, then just put the external PHY as a child node there.
> >>>>
> >>>> If fixed-link is required, the mdio node becomes unused anyway.
> >>>>
> >>>> Works for everyone?
> >>>
> >>> If we put an external PHY with reg=1 as a child of internal MDIO,
> >>> il will be merged with internal PHY node and get
> >>> phy-is-integrated.
> >>
> >> Then have the .dtsi file contain just the mdio node, but no internal or
> >> external PHY and push all the internal and external PHY node definition
> >> (in its entirety) to the per-board DTS file, does not that work?
> > 
> > If possible, I'd really like to have the internal PHY in the
> > DTSI. It's always there in hardware anyway, and duplicating the PHY,
> > with its clock, reset line, and whatever info we might need in the
> > future in each and every board DTS that uses it will be very error
> > prone and we will have the usual bunch of issues that come up with
> > duplication.
> 
> OK, then what if you put the internal PHY in the DTSI, mark it with a
> status = "disabled" property, and have the per-board DTS put a status =
> "okay" property along with a "phy-is-integrated" boolean property? Would
> that work?

No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
So that adding a 'status = "disabled"' does not bring anything.

> 
> What I really don't think is necessary is:
> 
> - duplicating the "mdio" controller node for external vs. internal PHY,
> because this is not accurate, there is just one MDIO controller, but
> there may be different kinds of MDIO/PHY devices attached

For me, if we want to represent the reality, we need two MDIO:
- since two PHY at the same address could co-exists
- since they are isolated so not on the same MDIO bus

> - having the STMMAC driver MDIO probing code having to deal with a
> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
> and requiring more driver-level changes that are error prone

My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
have to be changed to something like "register_parent_mdio"


So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
Really having two MDIO seems cleaner.

Regards

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Corentin Labbe]

On Thu, Aug 24, 2017 at 10:21:24AM +0200, Corentin Labbe wrote:
> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
> > On 08/23/2017 12:49 AM, Maxime Ripard wrote:
> > > Hi Florian,
> > > 
> > > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> > >>>>> So I think what you are saying is either impossible or engineering-wise
> > >>>>> a very stupid design, like using an external MAC with a discrete PHY
> > >>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
> > >>>>> with the internal PHY.
> > >>>>>
> > >>>>> Now can we please decide on something? We're a week and a half from
> > >>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> > >>>>> nodes (internal-mdio & external-mdio).
> > >>>>
> > >>>> I really don't see a need for a mdio-mux in the first place, just have
> > >>>> one MDIO controller (current state) sub-node which describes the
> > >>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
> > >>>> node (along with 'phy-is-integrated'). If a different configuration is
> > >>>> used, then just put the external PHY as a child node there.
> > >>>>
> > >>>> If fixed-link is required, the mdio node becomes unused anyway.
> > >>>>
> > >>>> Works for everyone?
> > >>>
> > >>> If we put an external PHY with reg=1 as a child of internal MDIO,
> > >>> il will be merged with internal PHY node and get
> > >>> phy-is-integrated.
> > >>
> > >> Then have the .dtsi file contain just the mdio node, but no internal or
> > >> external PHY and push all the internal and external PHY node definition
> > >> (in its entirety) to the per-board DTS file, does not that work?
> > > 
> > > If possible, I'd really like to have the internal PHY in the
> > > DTSI. It's always there in hardware anyway, and duplicating the PHY,
> > > with its clock, reset line, and whatever info we might need in the
> > > future in each and every board DTS that uses it will be very error
> > > prone and we will have the usual bunch of issues that come up with
> > > duplication.
> > 
> > OK, then what if you put the internal PHY in the DTSI, mark it with a
> > status = "disabled" property, and have the per-board DTS put a status =
> > "okay" property along with a "phy-is-integrated" boolean property? Would
> > that work?
> 
> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
> So that adding a 'status = "disabled"' does not bring anything.
> 
> > 
> > What I really don't think is necessary is:
> > 
> > - duplicating the "mdio" controller node for external vs. internal PHY,
> > because this is not accurate, there is just one MDIO controller, but
> > there may be different kinds of MDIO/PHY devices attached
> 
> For me, if we want to represent the reality, we need two MDIO:
> - since two PHY at the same address could co-exists
> - since they are isolated so not on the same MDIO bus
> 
> > - having the STMMAC driver MDIO probing code having to deal with a
> > "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
> > and requiring more driver-level changes that are error prone
> 
> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
> have to be changed to something like "register_parent_mdio"
> 
> 
> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
> Really having two MDIO seems cleaner.
> 

Hello

I have speaked with Neil Amstrong, and he said that they get the same problem on amlogic.
They use a mdio-mux-mmioreg, (see eth-phy-mux in arch/arm64/boot/dts/amlogic/meson-gxl.dtsi)
So tomorow, i will send a patch series which do the same with the exception that we need a mdio-mux-syscon (which is easy/simple to do).
Since their setup use stmmac, it means that we will need 0 changes on stmmac.

Regards

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Florian Fainelli]

On 08/24/2017 01:21 AM, Corentin Labbe wrote:
> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>> Hi Florian,
>>>
>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>> with the internal PHY.
>>>>>>>
>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>
>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>> used, then just put the external PHY as a child node there.
>>>>>>
>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>
>>>>>> Works for everyone?
>>>>>
>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>> il will be merged with internal PHY node and get
>>>>> phy-is-integrated.
>>>>
>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>> external PHY and push all the internal and external PHY node definition
>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>
>>> If possible, I'd really like to have the internal PHY in the
>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>> with its clock, reset line, and whatever info we might need in the
>>> future in each and every board DTS that uses it will be very error
>>> prone and we will have the usual bunch of issues that come up with
>>> duplication.
>>
>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>> status = "disabled" property, and have the per-board DTS put a status =
>> "okay" property along with a "phy-is-integrated" boolean property? Would
>> that work?
> 
> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.

Is not there is a mistake in the unit address not matching the "reg"
property, or am I not looking at the right tree?

&mdio {
        ext_rgmii_phy: ethernet-phy@1 {
                compatible = "ethernet-phy-ieee802.3-c22";
                reg = <0>;
        };
};

If the PHY is really at MDIO address 0, then it should be
ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
merging?


> So that adding a 'status = "disabled"' does not bring anything.
> 
>>
>> What I really don't think is necessary is:
>>
>> - duplicating the "mdio" controller node for external vs. internal PHY,
>> because this is not accurate, there is just one MDIO controller, but
>> there may be different kinds of MDIO/PHY devices attached
> 
> For me, if we want to represent the reality, we need two MDIO:
> - since two PHY at the same address could co-exists
> - since they are isolated so not on the same MDIO bus

Is that really true? It might be, but from experience with e.g:
bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
bus, which is convenient, except when you have an address conflict.

> 
>> - having the STMMAC driver MDIO probing code having to deal with a
>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>> and requiring more driver-level changes that are error prone
> 
> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
> have to be changed to something like "register_parent_mdio"
> 
> 
> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
> Really having two MDIO seems cleaner.

The only valid thing that you have provided so far is this merging
problem. Anything else ranging from "we will face with lots of small
patch for adding phy-is-integrated" to "Really having two MDIO seems
cleaner." are hard to receive as technical arguments for correctness.

What happens if someone connects an external PHY at the same MDIO
address than the internal PHY, which one do you get responses from? If
you shutdown the internal PHY and it stops responding, then this
probably becomes deterministic, but it still supports the fact there is
just one MDIO bus controller per MAC.

Anyway, do whatever works best for you I guess.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Chen-Yu Tsai]

On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 08/24/2017 01:21 AM, Corentin Labbe wrote:
>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>>> Hi Florian,
>>>>
>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>>> with the internal PHY.
>>>>>>>>
>>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>>
>>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>>> used, then just put the external PHY as a child node there.
>>>>>>>
>>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>>
>>>>>>> Works for everyone?
>>>>>>
>>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>>> il will be merged with internal PHY node and get
>>>>>> phy-is-integrated.
>>>>>
>>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>>> external PHY and push all the internal and external PHY node definition
>>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>>
>>>> If possible, I'd really like to have the internal PHY in the
>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>>> with its clock, reset line, and whatever info we might need in the
>>>> future in each and every board DTS that uses it will be very error
>>>> prone and we will have the usual bunch of issues that come up with
>>>> duplication.
>>>
>>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>>> status = "disabled" property, and have the per-board DTS put a status =
>>> "okay" property along with a "phy-is-integrated" boolean property? Would
>>> that work?
>>
>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
>
> Is not there is a mistake in the unit address not matching the "reg"
> property, or am I not looking at the right tree?
>
> &mdio {
>         ext_rgmii_phy: ethernet-phy@1 {
>                 compatible = "ethernet-phy-ieee802.3-c22";
>                 reg = <0>;
>         };
> };
>
> If the PHY is really at MDIO address 0, then it should be
> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
> merging?

That is wrong. The board described in the example likely has a Realtek
RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address,
so it still works, but is the wrong representation.

>
>
>> So that adding a 'status = "disabled"' does not bring anything.
>>
>>>
>>> What I really don't think is necessary is:
>>>
>>> - duplicating the "mdio" controller node for external vs. internal PHY,
>>> because this is not accurate, there is just one MDIO controller, but
>>> there may be different kinds of MDIO/PHY devices attached
>>
>> For me, if we want to represent the reality, we need two MDIO:
>> - since two PHY at the same address could co-exists
>> - since they are isolated so not on the same MDIO bus
>
> Is that really true? It might be, but from experience with e.g:
> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
> bus, which is convenient, except when you have an address conflict.

There's a mux in the hardware: either the internal MDIO+MII lines
from the internal PHY are connected to the MAC, or the external
MDIO+MII lines from the pin controller are connected. I believe
this was already mentioned?

>
>>
>>> - having the STMMAC driver MDIO probing code having to deal with a
>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>>> and requiring more driver-level changes that are error prone
>>
>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
>> have to be changed to something like "register_parent_mdio"
>>
>>
>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
>> Really having two MDIO seems cleaner.
>
> The only valid thing that you have provided so far is this merging
> problem. Anything else ranging from "we will face with lots of small
> patch for adding phy-is-integrated" to "Really having two MDIO seems
> cleaner." are hard to receive as technical arguments for correctness.
>
> What happens if someone connects an external PHY at the same MDIO
> address than the internal PHY, which one do you get responses from? If
> you shutdown the internal PHY and it stops responding, then this
> probably becomes deterministic, but it still supports the fact there is
> just one MDIO bus controller per MAC.

Depends on whichever set of pins/lines are selected. But yeah, there's
only one MDIO bus controller in the MAC.

ChenYu

> Anyway, do whatever works best for you I guess.
> --
> Florian

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Florian Fainelli]

On 08/24/2017 07:54 PM, Chen-Yu Tsai wrote:
> On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 08/24/2017 01:21 AM, Corentin Labbe wrote:
>>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>>>> Hi Florian,
>>>>>
>>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>>>> with the internal PHY.
>>>>>>>>>
>>>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>>>
>>>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>>>> used, then just put the external PHY as a child node there.
>>>>>>>>
>>>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>>>
>>>>>>>> Works for everyone?
>>>>>>>
>>>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>>>> il will be merged with internal PHY node and get
>>>>>>> phy-is-integrated.
>>>>>>
>>>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>>>> external PHY and push all the internal and external PHY node definition
>>>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>>>
>>>>> If possible, I'd really like to have the internal PHY in the
>>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>>>> with its clock, reset line, and whatever info we might need in the
>>>>> future in each and every board DTS that uses it will be very error
>>>>> prone and we will have the usual bunch of issues that come up with
>>>>> duplication.
>>>>
>>>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>>>> status = "disabled" property, and have the per-board DTS put a status =
>>>> "okay" property along with a "phy-is-integrated" boolean property? Would
>>>> that work?
>>>
>>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
>>
>> Is not there is a mistake in the unit address not matching the "reg"
>> property, or am I not looking at the right tree?
>>
>> &mdio {
>>         ext_rgmii_phy: ethernet-phy@1 {
>>                 compatible = "ethernet-phy-ieee802.3-c22";
>>                 reg = <0>;
>>         };
>> };
>>
>> If the PHY is really at MDIO address 0, then it should be
>> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
>> merging?
> 
> That is wrong. The board described in the example likely has a Realtek
> RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address,
> so it still works, but is the wrong representation.
> 
>>
>>
>>> So that adding a 'status = "disabled"' does not bring anything.
>>>
>>>>
>>>> What I really don't think is necessary is:
>>>>
>>>> - duplicating the "mdio" controller node for external vs. internal PHY,
>>>> because this is not accurate, there is just one MDIO controller, but
>>>> there may be different kinds of MDIO/PHY devices attached
>>>
>>> For me, if we want to represent the reality, we need two MDIO:
>>> - since two PHY at the same address could co-exists
>>> - since they are isolated so not on the same MDIO bus
>>
>> Is that really true? It might be, but from experience with e.g:
>> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
>> bus, which is convenient, except when you have an address conflict.
> 
> There's a mux in the hardware: either the internal MDIO+MII lines
> from the internal PHY are connected to the MAC, or the external
> MDIO+MII lines from the pin controller are connected. I believe
> this was already mentioned?

There is obviously a mux for the data lines and clock to switch between
internal PHY and external PHYs, that does not mean there is one for MDIO
and MDC lines, which is what is being suggested to be used here, does
the mux also takes care of these lines?

> 
>>
>>>
>>>> - having the STMMAC driver MDIO probing code having to deal with a
>>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>>>> and requiring more driver-level changes that are error prone
>>>
>>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
>>> have to be changed to something like "register_parent_mdio"
>>>
>>>
>>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
>>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
>>> Really having two MDIO seems cleaner.
>>
>> The only valid thing that you have provided so far is this merging
>> problem. Anything else ranging from "we will face with lots of small
>> patch for adding phy-is-integrated" to "Really having two MDIO seems
>> cleaner." are hard to receive as technical arguments for correctness.
>>
>> What happens if someone connects an external PHY at the same MDIO
>> address than the internal PHY, which one do you get responses from? If
>> you shutdown the internal PHY and it stops responding, then this
>> probably becomes deterministic, but it still supports the fact there is
>> just one MDIO bus controller per MAC.
> 
> Depends on whichever set of pins/lines are selected. But yeah, there's
> only one MDIO bus controller in the MAC.

OK, so one MDIO controller, but what about the MDIO/MDC lines then, are
they also muxed, like the data/clock lines or not?
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Chen-Yu Tsai]

On Fri, Aug 25, 2017 at 11:05 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>
> On 08/24/2017 07:54 PM, Chen-Yu Tsai wrote:
>> On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 08/24/2017 01:21 AM, Corentin Labbe wrote:
>>>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>>>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>>>>> Hi Florian,
>>>>>>
>>>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>>>>> with the internal PHY.
>>>>>>>>>>
>>>>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>>>>
>>>>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>>>>> used, then just put the external PHY as a child node there.
>>>>>>>>>
>>>>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>>>>
>>>>>>>>> Works for everyone?
>>>>>>>>
>>>>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>>>>> il will be merged with internal PHY node and get
>>>>>>>> phy-is-integrated.
>>>>>>>
>>>>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>>>>> external PHY and push all the internal and external PHY node definition
>>>>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>>>>
>>>>>> If possible, I'd really like to have the internal PHY in the
>>>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>>>>> with its clock, reset line, and whatever info we might need in the
>>>>>> future in each and every board DTS that uses it will be very error
>>>>>> prone and we will have the usual bunch of issues that come up with
>>>>>> duplication.
>>>>>
>>>>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>>>>> status = "disabled" property, and have the per-board DTS put a status =
>>>>> "okay" property along with a "phy-is-integrated" boolean property? Would
>>>>> that work?
>>>>
>>>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
>>>
>>> Is not there is a mistake in the unit address not matching the "reg"
>>> property, or am I not looking at the right tree?
>>>
>>> &mdio {
>>>         ext_rgmii_phy: ethernet-phy@1 {
>>>                 compatible = "ethernet-phy-ieee802.3-c22";
>>>                 reg = <0>;
>>>         };
>>> };
>>>
>>> If the PHY is really at MDIO address 0, then it should be
>>> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
>>> merging?
>>
>> That is wrong. The board described in the example likely has a Realtek
>> RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address,
>> so it still works, but is the wrong representation.
>>
>>>
>>>
>>>> So that adding a 'status = "disabled"' does not bring anything.
>>>>
>>>>>
>>>>> What I really don't think is necessary is:
>>>>>
>>>>> - duplicating the "mdio" controller node for external vs. internal PHY,
>>>>> because this is not accurate, there is just one MDIO controller, but
>>>>> there may be different kinds of MDIO/PHY devices attached
>>>>
>>>> For me, if we want to represent the reality, we need two MDIO:
>>>> - since two PHY at the same address could co-exists
>>>> - since they are isolated so not on the same MDIO bus
>>>
>>> Is that really true? It might be, but from experience with e.g:
>>> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
>>> bus, which is convenient, except when you have an address conflict.
>>
>> There's a mux in the hardware: either the internal MDIO+MII lines
>> from the internal PHY are connected to the MAC, or the external
>> MDIO+MII lines from the pin controller are connected. I believe
>> this was already mentioned?
>
> There is obviously a mux for the data lines and clock to switch between
> internal PHY and external PHYs, that does not mean there is one for MDIO
> and MDC lines, which is what is being suggested to be used here, does
> the mux also takes care of these lines?
>
>>
>>>
>>>>
>>>>> - having the STMMAC driver MDIO probing code having to deal with a
>>>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>>>>> and requiring more driver-level changes that are error prone
>>>>
>>>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
>>>> have to be changed to something like "register_parent_mdio"
>>>>
>>>>
>>>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
>>>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
>>>> Really having two MDIO seems cleaner.
>>>
>>> The only valid thing that you have provided so far is this merging
>>> problem. Anything else ranging from "we will face with lots of small
>>> patch for adding phy-is-integrated" to "Really having two MDIO seems
>>> cleaner." are hard to receive as technical arguments for correctness.
>>>
>>> What happens if someone connects an external PHY at the same MDIO
>>> address than the internal PHY, which one do you get responses from? If
>>> you shutdown the internal PHY and it stops responding, then this
>>> probably becomes deterministic, but it still supports the fact there is
>>> just one MDIO bus controller per MAC.
>>
>> Depends on whichever set of pins/lines are selected. But yeah, there's
>> only one MDIO bus controller in the MAC.
>
> OK, so one MDIO controller, but what about the MDIO/MDC lines then, are
> they also muxed, like the data/clock lines or not?

Just tested. Yes the MDIO/MDC lines are also muxed and controlled through
the same mux bit.

ChenYu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Florian Fainelli]

On 08/24/2017 08:41 PM, Chen-Yu Tsai wrote:
> On Fri, Aug 25, 2017 at 11:05 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>> On 08/24/2017 07:54 PM, Chen-Yu Tsai wrote:
>>> On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 08/24/2017 01:21 AM, Corentin Labbe wrote:
>>>>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
>>>>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
>>>>>>> Hi Florian,
>>>>>>>
>>>>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
>>>>>>>>>>> So I think what you are saying is either impossible or engineering-wise
>>>>>>>>>>> a very stupid design, like using an external MAC with a discrete PHY
>>>>>>>>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>>>>>>>>>> with the internal PHY.
>>>>>>>>>>>
>>>>>>>>>>> Now can we please decide on something? We're a week and a half from
>>>>>>>>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>>>>>>>>>> nodes (internal-mdio & external-mdio).
>>>>>>>>>>
>>>>>>>>>> I really don't see a need for a mdio-mux in the first place, just have
>>>>>>>>>> one MDIO controller (current state) sub-node which describes the
>>>>>>>>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>>>>>>>>>> node (along with 'phy-is-integrated'). If a different configuration is
>>>>>>>>>> used, then just put the external PHY as a child node there.
>>>>>>>>>>
>>>>>>>>>> If fixed-link is required, the mdio node becomes unused anyway.
>>>>>>>>>>
>>>>>>>>>> Works for everyone?
>>>>>>>>>
>>>>>>>>> If we put an external PHY with reg=1 as a child of internal MDIO,
>>>>>>>>> il will be merged with internal PHY node and get
>>>>>>>>> phy-is-integrated.
>>>>>>>>
>>>>>>>> Then have the .dtsi file contain just the mdio node, but no internal or
>>>>>>>> external PHY and push all the internal and external PHY node definition
>>>>>>>> (in its entirety) to the per-board DTS file, does not that work?
>>>>>>>
>>>>>>> If possible, I'd really like to have the internal PHY in the
>>>>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY,
>>>>>>> with its clock, reset line, and whatever info we might need in the
>>>>>>> future in each and every board DTS that uses it will be very error
>>>>>>> prone and we will have the usual bunch of issues that come up with
>>>>>>> duplication.
>>>>>>
>>>>>> OK, then what if you put the internal PHY in the DTSI, mark it with a
>>>>>> status = "disabled" property, and have the per-board DTS put a status =
>>>>>> "okay" property along with a "phy-is-integrated" boolean property? Would
>>>>>> that work?
>>>>>
>>>>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged.
>>>>
>>>> Is not there is a mistake in the unit address not matching the "reg"
>>>> property, or am I not looking at the right tree?
>>>>
>>>> &mdio {
>>>>         ext_rgmii_phy: ethernet-phy@1 {
>>>>                 compatible = "ethernet-phy-ieee802.3-c22";
>>>>                 reg = <0>;
>>>>         };
>>>> };
>>>>
>>>> If the PHY is really at MDIO address 0, then it should be
>>>> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the
>>>> merging?
>>>
>>> That is wrong. The board described in the example likely has a Realtek
>>> RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address,
>>> so it still works, but is the wrong representation.
>>>
>>>>
>>>>
>>>>> So that adding a 'status = "disabled"' does not bring anything.
>>>>>
>>>>>>
>>>>>> What I really don't think is necessary is:
>>>>>>
>>>>>> - duplicating the "mdio" controller node for external vs. internal PHY,
>>>>>> because this is not accurate, there is just one MDIO controller, but
>>>>>> there may be different kinds of MDIO/PHY devices attached
>>>>>
>>>>> For me, if we want to represent the reality, we need two MDIO:
>>>>> - since two PHY at the same address could co-exists
>>>>> - since they are isolated so not on the same MDIO bus
>>>>
>>>> Is that really true? It might be, but from experience with e.g:
>>>> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO
>>>> bus, which is convenient, except when you have an address conflict.
>>>
>>> There's a mux in the hardware: either the internal MDIO+MII lines
>>> from the internal PHY are connected to the MAC, or the external
>>> MDIO+MII lines from the pin controller are connected. I believe
>>> this was already mentioned?
>>
>> There is obviously a mux for the data lines and clock to switch between
>> internal PHY and external PHYs, that does not mean there is one for MDIO
>> and MDC lines, which is what is being suggested to be used here, does
>> the mux also takes care of these lines?
>>
>>>
>>>>
>>>>>
>>>>>> - having the STMMAC driver MDIO probing code having to deal with a
>>>>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
>>>>>> and requiring more driver-level changes that are error prone
>>>>>
>>>>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids")
>>>>> have to be changed to something like "register_parent_mdio"
>>>>>
>>>>>
>>>>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes.
>>>>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean.
>>>>> Really having two MDIO seems cleaner.
>>>>
>>>> The only valid thing that you have provided so far is this merging
>>>> problem. Anything else ranging from "we will face with lots of small
>>>> patch for adding phy-is-integrated" to "Really having two MDIO seems
>>>> cleaner." are hard to receive as technical arguments for correctness.
>>>>
>>>> What happens if someone connects an external PHY at the same MDIO
>>>> address than the internal PHY, which one do you get responses from? If
>>>> you shutdown the internal PHY and it stops responding, then this
>>>> probably becomes deterministic, but it still supports the fact there is
>>>> just one MDIO bus controller per MAC.
>>>
>>> Depends on whichever set of pins/lines are selected. But yeah, there's
>>> only one MDIO bus controller in the MAC.
>>
>> OK, so one MDIO controller, but what about the MDIO/MDC lines then, are
>> they also muxed, like the data/clock lines or not?
> 
> Just tested. Yes the MDIO/MDC lines are also muxed and controlled through
> the same mux bit.

Alright then the mdio-mux seems appropriate, thanks.
-- 
Florian

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Andrew Lunn]

> > Just tested. Yes the MDIO/MDC lines are also muxed and controlled through
> > the same mux bit.
> 
> Alright then the mdio-mux seems appropriate, thanks.

Please add a comment that it muxes the MII lines as well.  That is not
normal for an mdio-mux, so we should document it in the dtsi file.

Thanks
       Andrew

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Tue, Aug 22, 2017 at 11:39:22PM +0800, Chen-Yu Tsai wrote:
> Now can we please decide on something? We're a week and a half from
> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> nodes (internal-mdio & external-mdio).

I can only emphasize this. I'm afraid that if we don't have an
agreement and a patch implementing it with the proper acks by the end
of the week, we'll have to revert all the DT bits and the bindings.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Corentin Labbe]

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        | 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)
 
 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:
+  - "allwinner,sun8i-h3-emac",
+- a mdio-mux node with compatible = "allwinner,sun8i-h3-mdio-switch"
+  - mdio-parent-bus: The parent bus is "mdio_parent"
+  - two child mdio, one for the integrated mdio, one for the external mdio
+
+Required properties of the integrated phy node with the following compatibles:
   - "allwinner,sun8i-h3-emac",
   - "allwinner,sun8i-v3s-emac":
 - 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 {
+		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;
+	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

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

[Chen-Yu Tsai]

On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe
<clabbe.montjoie@gmail.com> 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        | 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
>

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

[Corentin Labbe]

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@gmail.com> 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        | 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