Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1714865 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-18 14:30 +0200
    Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac Chen-Yu Tsai <wens@csie.org> - 2017-08-18 19:10 +0200
      Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-19 21:00 +0200
        Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Andrew Lunn <andrew@lunn.ch> - 2017-08-19 22:40 +0200
          Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-20 09:00 +0200
            Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Andrew Lunn <andrew@lunn.ch> - 2017-08-20 16:30 +0200
              Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac Chen-Yu Tsai <wens@csie.org> - 2017-08-21 10:20 +0200
                Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Andrew Lunn <andrew@lunn.ch> - 2017-08-21 15:30 +0200
                  Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-08-21 15:40 +0200
                    Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Andrew Lunn <andrew@lunn.ch> - 2017-08-21 16:30 +0200
                      Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-22 10:10 +0200
                      Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac Chen-Yu Tsai <wens@csie.org> - 2017-08-22 17:40 +0200
                        Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Florian Fainelli <f.fainelli@gmail.com> - 2017-08-22 18:50 +0200
                          Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-22 20:20 +0200
                            Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Florian Fainelli <f.fainelli@gmail.com> - 2017-08-22 20:40 +0200
                              Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-22 21:40 +0200
                              Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-08-23 09:50 +0200
                                Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Florian Fainelli <f.fainelli@gmail.com> - 2017-08-23 18:40 +0200
                                  Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-08-24 10:20 +0200
                                  Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-24 10:30 +0200
                                    Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Corentin Labbe <clabbe.montjoie@gmail.com> - 2017-08-24 21:50 +0200
                                    Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Florian Fainelli <f.fainelli@gmail.com> - 2017-08-24 22:00 +0200
                                      Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac Chen-Yu Tsai <wens@csie.org> - 2017-08-25 05:00 +0200
                                        Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Florian Fainelli <f.fainelli@gmail.com> - 2017-08-25 05:10 +0200
                                          Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac Chen-Yu Tsai <wens@csie.org> - 2017-08-25 05:50 +0200
                                            Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Florian Fainelli <f.fainelli@gmail.com> - 2017-08-25 06:00 +0200
                        Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for  sun8i-h3-emac Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-08-22 19:00 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

#1715321

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
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

#1716161

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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1717508

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.

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

[toc] | [prev] | [next] | [standalone]

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

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?
-- 
Florian

[toc] | [prev] | [next] | [standalone]

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

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 {

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web