[PATCH v4 1/4] net: axienet: setup mdio unconditionally

[PATCH v4 1/4] net: axienet: setup mdio unconditionally

[Andy Chiu]

The call to axienet_mdio_setup should not depend on whether "phy-node"
pressents on the DT. Besides, since `lp->phy_node` is used if PHY is in
SGMII or 100Base-X modes, move it into the if statement. And the next patch
will remove `lp->phy_node` from driver's private structure and do an
of_node_put on it right away after use since it is not used elsewhere.

Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 6fd5157f0a6d..5d41b8de840a 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2064,15 +2064,14 @@ static int axienet_probe(struct platform_device *pdev)
 	if (ret)
 		goto cleanup_clk;
 
-	lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
-	if (lp->phy_node) {
-		ret = axienet_mdio_setup(lp);
-		if (ret)
-			dev_warn(&pdev->dev,
-				 "error registering MDIO bus: %d\n", ret);
-	}
+	ret = axienet_mdio_setup(lp);
+	if (ret)
+		dev_warn(&pdev->dev,
+			 "error registering MDIO bus: %d\n", ret);
+
 	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
 	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
+		lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
 		if (!lp->phy_node) {
 			dev_err(&pdev->dev, "phy-handle required for 1000BaseX/SGMII\n");
 			ret = -EINVAL;
-- 
2.34.1

[PATCH v4 2/4] net: axienet: factor out phy_node in struct axienet_local

[Andy Chiu]

the struct member `phy_node` of struct axienet_local is not used by the
driver anymore after initialization. It might be a remnent of old code
and could be removed.

Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet.h      |  2 --
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 13 +++++--------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index 0f9c88dd1a4a..d5c1e5c4a508 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -433,8 +433,6 @@ struct axienet_local {
 	struct net_device *ndev;
 	struct device *dev;
 
-	struct device_node *phy_node;
-
 	struct phylink *phylink;
 	struct phylink_config phylink_config;
 
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 5d41b8de840a..496a9227e760 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2071,19 +2071,21 @@ static int axienet_probe(struct platform_device *pdev)
 
 	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
 	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
-		lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
-		if (!lp->phy_node) {
+		np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
+		if (!np) {
 			dev_err(&pdev->dev, "phy-handle required for 1000BaseX/SGMII\n");
 			ret = -EINVAL;
 			goto cleanup_mdio;
 		}
-		lp->pcs_phy = of_mdio_find_device(lp->phy_node);
+		lp->pcs_phy = of_mdio_find_device(np);
 		if (!lp->pcs_phy) {
 			ret = -EPROBE_DEFER;
+			of_node_put(np);
 			goto cleanup_mdio;
 		}
 		lp->pcs.ops = &axienet_pcs_ops;
 		lp->pcs.poll = true;
+		of_node_put(np);
 	}
 
 	lp->phylink_config.dev = &ndev->dev;
@@ -2124,8 +2126,6 @@ static int axienet_probe(struct platform_device *pdev)
 		put_device(&lp->pcs_phy->dev);
 	if (lp->mii_bus)
 		axienet_mdio_teardown(lp);
-	of_node_put(lp->phy_node);
-
 cleanup_clk:
 	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
 	clk_disable_unprepare(lp->axi_clk);
@@ -2154,9 +2154,6 @@ static int axienet_remove(struct platform_device *pdev)
 	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
 	clk_disable_unprepare(lp->axi_clk);
 
-	of_node_put(lp->phy_node);
-	lp->phy_node = NULL;
-
 	free_netdev(ndev);
 
 	return 0;
-- 
2.34.1

Re: [PATCH v4 2/4] net: axienet: factor out phy_node in struct axienet_local

[Robert Hancock]

On Mon, 2022-03-21 at 23:25 +0800, Andy Chiu wrote:
> the struct member `phy_node` of struct axienet_local is not used by the
> driver anymore after initialization. It might be a remnent of old code
> and could be removed.
> 
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet.h      |  2 --
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 13 +++++--------
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index 0f9c88dd1a4a..d5c1e5c4a508 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -433,8 +433,6 @@ struct axienet_local {
>  	struct net_device *ndev;
>  	struct device *dev;
>  
> -	struct device_node *phy_node;
> -
>  	struct phylink *phylink;
>  	struct phylink_config phylink_config;
>  
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 5d41b8de840a..496a9227e760 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2071,19 +2071,21 @@ static int axienet_probe(struct platform_device
> *pdev)
>  
>  	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
>  	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
> -		lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-
> handle", 0);
> -		if (!lp->phy_node) {
> +		np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> +		if (!np) {
>  			dev_err(&pdev->dev, "phy-handle required for
> 1000BaseX/SGMII\n");
>  			ret = -EINVAL;
>  			goto cleanup_mdio;
>  		}
> -		lp->pcs_phy = of_mdio_find_device(lp->phy_node);
> +		lp->pcs_phy = of_mdio_find_device(np);
>  		if (!lp->pcs_phy) {
>  			ret = -EPROBE_DEFER;
> +			of_node_put(np);
>  			goto cleanup_mdio;
>  		}
>  		lp->pcs.ops = &axienet_pcs_ops;
>  		lp->pcs.poll = true;
> +		of_node_put(np);
>  	}
>  
>  	lp->phylink_config.dev = &ndev->dev;
> @@ -2124,8 +2126,6 @@ static int axienet_probe(struct platform_device *pdev)
>  		put_device(&lp->pcs_phy->dev);
>  	if (lp->mii_bus)
>  		axienet_mdio_teardown(lp);
> -	of_node_put(lp->phy_node);
> -
>  cleanup_clk:
>  	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
>  	clk_disable_unprepare(lp->axi_clk);
> @@ -2154,9 +2154,6 @@ static int axienet_remove(struct platform_device *pdev)
>  	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
>  	clk_disable_unprepare(lp->axi_clk);
>  
> -	of_node_put(lp->phy_node);
> -	lp->phy_node = NULL;
> -
>  	free_netdev(ndev);
>  
>  	return 0;

Reviewed-by: Robert Hancock <robert.hancock@calian.com>

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

[PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Andy Chiu]

Document the new pcs-handle attribute to support connecting to an
external PHY in SGMII or 1000Base-X modes through the internal PCS/PMA
PHY.

Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
---
 Documentation/devicetree/bindings/net/xilinx_axienet.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
index b8e4894bc634..ba720a2ea5fc 100644
--- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
+++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
@@ -26,7 +26,8 @@ Required properties:
 		  specified, the TX/RX DMA interrupts should be on that node
 		  instead, and only the Ethernet core interrupt is optionally
 		  specified here.
-- phy-handle	: Should point to the external phy device.
+- phy-handle	: Should point to the external phy device if exists. Pointing
+		  this to the PCS/PMA PHY is deprecated and should be avoided.
 		  See ethernet.txt file in the same directory.
 - xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the hardware
 
@@ -68,6 +69,11 @@ Optional properties:
 		  required through the core's MDIO interface (i.e. always,
 		  unless the PHY is accessed through a different bus).
 
+ - pcs-handle: 	  Phandle to the internal PCS/PMA PHY in SGMII or 1000Base-X
+		  modes, where "pcs-handle" should be preferably used to point
+		  to the PCS/PMA PHY, and "phy-handle" should point to an
+		  external PHY if exists.
+
 Example:
 	axi_ethernet_eth: ethernet@40c00000 {
 		compatible = "xlnx,axi-ethernet-1.00.a";
-- 
2.34.1

RE: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Andy Chiu <andy.chiu@sifive.com>
> Sent: Monday, March 21, 2022 8:55 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>; robert.hancock@calian.com;
> Michal Simek <michals@xilinx.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; linux@armlinux.org.uk; andrew@lunn.ch;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; Andy Chiu
> <andy.chiu@sifive.com>; Greentime Hu <greentime.hu@sifive.com>
> Subject: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle
> attribute
> 
> Document the new pcs-handle attribute to support connecting to an external
> PHY in SGMII or 1000Base-X modes through the internal PCS/PMA PHY.
> 
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  Documentation/devicetree/bindings/net/xilinx_axienet.txt | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> index b8e4894bc634..ba720a2ea5fc 100644
> --- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> +++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> @@ -26,7 +26,8 @@ Required properties:
>  		  specified, the TX/RX DMA interrupts should be on that node
>  		  instead, and only the Ethernet core interrupt is optionally
>  		  specified here.
> -- phy-handle	: Should point to the external phy device.
> +- phy-handle	: Should point to the external phy device if exists. Pointing
> +		  this to the PCS/PMA PHY is deprecated and should be
> avoided.
>  		  See ethernet.txt file in the same directory.
>  - xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the hardware
> 
> @@ -68,6 +69,11 @@ Optional properties:
>  		  required through the core's MDIO interface (i.e. always,
>  		  unless the PHY is accessed through a different bus).
> 
> + - pcs-handle: 	  Phandle to the internal PCS/PMA PHY in SGMII or 1000Base-X
> +		  modes, where "pcs-handle" should be preferably used to
> point
> +		  to the PCS/PMA PHY, and "phy-handle" should point to an
> +		  external PHY if exists.

I would like to have Rob feedback on this pcs-handle DT property.

The use case is generic i.e. require separate handle to internal SGMII
and external Phy so would prefer this new DT convention is 
standardized or we discuss possible approaches on how to handle
both phys and not add it as vendor specific property in the first 
place.


> +
>  Example:
>  	axi_ethernet_eth: ethernet@40c00000 {
>  		compatible = "xlnx,axi-ethernet-1.00.a";
> --
> 2.34.1

Re: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Robert Hancock]

On Mon, 2022-03-21 at 15:42 +0000, Radhey Shyam Pandey wrote:
> > -----Original Message-----
> > From: Andy Chiu <andy.chiu@sifive.com>
> > Sent: Monday, March 21, 2022 8:55 PM
> > To: Radhey Shyam Pandey <radheys@xilinx.com>; robert.hancock@calian.com;
> > Michal Simek <michals@xilinx.com>
> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> > robh+dt@kernel.org; linux@armlinux.org.uk; andrew@lunn.ch;
> > netdev@vger.kernel.org; devicetree@vger.kernel.org; Andy Chiu
> > <andy.chiu@sifive.com>; Greentime Hu <greentime.hu@sifive.com>
> > Subject: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle
> > attribute
> > 
> > Document the new pcs-handle attribute to support connecting to an external
> > PHY in SGMII or 1000Base-X modes through the internal PCS/PMA PHY.
> > 
> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >  Documentation/devicetree/bindings/net/xilinx_axienet.txt | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > index b8e4894bc634..ba720a2ea5fc 100644
> > --- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > +++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > @@ -26,7 +26,8 @@ Required properties:
> >  		  specified, the TX/RX DMA interrupts should be on that node
> >  		  instead, and only the Ethernet core interrupt is optionally
> >  		  specified here.
> > -- phy-handle	: Should point to the external phy device.
> > +- phy-handle	: Should point to the external phy device if exists.
> > Pointing
> > +		  this to the PCS/PMA PHY is deprecated and should be
> > avoided.
> >  		  See ethernet.txt file in the same directory.
> >  - xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the
> > hardware
> > 
> > @@ -68,6 +69,11 @@ Optional properties:
> >  		  required through the core's MDIO interface (i.e. always,
> >  		  unless the PHY is accessed through a different bus).
> > 
> > + - pcs-handle: 	  Phandle to the internal PCS/PMA PHY in SGMII or
> > 1000Base-X
> > +		  modes, where "pcs-handle" should be preferably used to
> > point
> > +		  to the PCS/PMA PHY, and "phy-handle" should point to an
> > +		  external PHY if exists.
> 
> I would like to have Rob feedback on this pcs-handle DT property.
> 
> The use case is generic i.e. require separate handle to internal SGMII
> and external Phy so would prefer this new DT convention is 
> standardized or we discuss possible approaches on how to handle
> both phys and not add it as vendor specific property in the first 
> place.
> 

The pcs-handle property is currently used in the Freescale dpaa2 driver, so
there's some precedent for it. But I suppose if it's intended to be a standard
mechanism it should be documented more globally?

> 
> > +
> >  Example:
> >  	axi_ethernet_eth: ethernet@40c00000 {
> >  		compatible = "xlnx,axi-ethernet-1.00.a";
> > --
> > 2.34.1

Re: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Rob Herring]

On Mon, Mar 21, 2022 at 03:42:52PM +0000, Radhey Shyam Pandey wrote:
> > -----Original Message-----
> > From: Andy Chiu <andy.chiu@sifive.com>
> > Sent: Monday, March 21, 2022 8:55 PM
> > To: Radhey Shyam Pandey <radheys@xilinx.com>; robert.hancock@calian.com;
> > Michal Simek <michals@xilinx.com>
> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> > robh+dt@kernel.org; linux@armlinux.org.uk; andrew@lunn.ch;
> > netdev@vger.kernel.org; devicetree@vger.kernel.org; Andy Chiu
> > <andy.chiu@sifive.com>; Greentime Hu <greentime.hu@sifive.com>
> > Subject: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle
> > attribute
> > 
> > Document the new pcs-handle attribute to support connecting to an external
> > PHY in SGMII or 1000Base-X modes through the internal PCS/PMA PHY.
> > 
> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >  Documentation/devicetree/bindings/net/xilinx_axienet.txt | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > index b8e4894bc634..ba720a2ea5fc 100644
> > --- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > +++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > @@ -26,7 +26,8 @@ Required properties:
> >  		  specified, the TX/RX DMA interrupts should be on that node
> >  		  instead, and only the Ethernet core interrupt is optionally
> >  		  specified here.
> > -- phy-handle	: Should point to the external phy device.
> > +- phy-handle	: Should point to the external phy device if exists. Pointing
> > +		  this to the PCS/PMA PHY is deprecated and should be
> > avoided.
> >  		  See ethernet.txt file in the same directory.
> >  - xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the hardware
> > 
> > @@ -68,6 +69,11 @@ Optional properties:
> >  		  required through the core's MDIO interface (i.e. always,
> >  		  unless the PHY is accessed through a different bus).
> > 
> > + - pcs-handle: 	  Phandle to the internal PCS/PMA PHY in SGMII or 1000Base-X
> > +		  modes, where "pcs-handle" should be preferably used to
> > point
> > +		  to the PCS/PMA PHY, and "phy-handle" should point to an
> > +		  external PHY if exists.
> 
> I would like to have Rob feedback on this pcs-handle DT property.
> 
> The use case is generic i.e. require separate handle to internal SGMII
> and external Phy so would prefer this new DT convention is 
> standardized or we discuss possible approaches on how to handle
> both phys and not add it as vendor specific property in the first 
> place.

IMO, you should use 'phys' for the internal PCS phy. That's aligned with 
other uses like PCIe, SATA, etc. (there is phy h/w that will do PCS, 
PCIe, SATA). 'phy-handle' is for the ethernet PHY.

Rob

Re: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Robert Hancock]

On Mon, 2022-03-21 at 18:44 -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 03:42:52PM +0000, Radhey Shyam Pandey wrote:
> > > -----Original Message-----
> > > From: Andy Chiu <andy.chiu@sifive.com>
> > > Sent: Monday, March 21, 2022 8:55 PM
> > > To: Radhey Shyam Pandey <radheys@xilinx.com>; robert.hancock@calian.com;
> > > Michal Simek <michals@xilinx.com>
> > > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> > > robh+dt@kernel.org; linux@armlinux.org.uk; andrew@lunn.ch;
> > > netdev@vger.kernel.org; devicetree@vger.kernel.org; Andy Chiu
> > > <andy.chiu@sifive.com>; Greentime Hu <greentime.hu@sifive.com>
> > > Subject: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle
> > > attribute
> > > 
> > > Document the new pcs-handle attribute to support connecting to an
> > > external
> > > PHY in SGMII or 1000Base-X modes through the internal PCS/PMA PHY.
> > > 
> > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > > Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> > > ---
> > >  Documentation/devicetree/bindings/net/xilinx_axienet.txt | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > > b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > > index b8e4894bc634..ba720a2ea5fc 100644
> > > --- a/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > > +++ b/Documentation/devicetree/bindings/net/xilinx_axienet.txt
> > > @@ -26,7 +26,8 @@ Required properties:
> > >  		  specified, the TX/RX DMA interrupts should be on that node
> > >  		  instead, and only the Ethernet core interrupt is optionally
> > >  		  specified here.
> > > -- phy-handle	: Should point to the external phy device.
> > > +- phy-handle	: Should point to the external phy device if exists.
> > > Pointing
> > > +		  this to the PCS/PMA PHY is deprecated and should be
> > > avoided.
> > >  		  See ethernet.txt file in the same directory.
> > >  - xlnx,rxmem	: Set to allocated memory buffer for Rx/Tx in the
> > > hardware
> > > 
> > > @@ -68,6 +69,11 @@ Optional properties:
> > >  		  required through the core's MDIO interface (i.e. always,
> > >  		  unless the PHY is accessed through a different bus).
> > > 
> > > + - pcs-handle: 	  Phandle to the internal PCS/PMA PHY in SGMII or
> > > 1000Base-X
> > > +		  modes, where "pcs-handle" should be preferably used to
> > > point
> > > +		  to the PCS/PMA PHY, and "phy-handle" should point to an
> > > +		  external PHY if exists.
> > 
> > I would like to have Rob feedback on this pcs-handle DT property.
> > 
> > The use case is generic i.e. require separate handle to internal SGMII
> > and external Phy so would prefer this new DT convention is 
> > standardized or we discuss possible approaches on how to handle
> > both phys and not add it as vendor specific property in the first 
> > place.
> 
> IMO, you should use 'phys' for the internal PCS phy. That's aligned with 
> other uses like PCIe, SATA, etc. (there is phy h/w that will do PCS, 
> PCIe, SATA). 'phy-handle' is for the ethernet PHY.

That might be confusing in some cases - I'm thinking of the Cadence GEM
controllers on the Xilinx MPSoC devices, where there is a PCS internal to the
controller, as well as a generic PHY device which is used to control the actual
low-level I/O transceiver on the chip (which can work in SGMII mode but also in
USB, PCIe, SATA modes). Calling them both just a "PHY" makes that distinction
rather unclear. In the case of that driver I believe its PCS is just "known
about" by the MAC when it is present and not configured in the device tree like
this driver does, but possibly it could be DT configurable someday (since I
think it's potentially possible to use a different PCS implemented in the
programmable logic if one wanted).

The way this PCS is accessed, both it and the potential external PHY are both
"Ethernet PHYs" in that they support standard Ethernet PHY registers etc., it's
just that generally the external one is the one the outside Linux networking
infrastructure cares about, the PCS PHY is more of an implementation detail
internal to the driver.

> 
> Rob

Re: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Andrew Lunn]

> > The use case is generic i.e. require separate handle to internal SGMII
> > and external Phy so would prefer this new DT convention is 
> > standardized or we discuss possible approaches on how to handle
> > both phys and not add it as vendor specific property in the first 
> > place.
> 
> IMO, you should use 'phys' for the internal PCS phy. That's aligned with 
> other uses like PCIe, SATA, etc. (there is phy h/w that will do PCS, 
> PCIe, SATA). 'phy-handle' is for the ethernet PHY.

We need to be careful here, because the PCS can have a well defined
set of registers accessible over MDIO. Generic PHY has no
infrastructure for that, it is all inside phylink which implements the
pcs registers which are part of 802.3.

I also wonder if a PCS might actually have a generic PHY embedded in
it to provide its lower interface?

   Andrew

Re: [PATCH v4 3/4] dt-bindings: net: xilinx_axienet: add pcs-handle attribute

[Rob Herring]

On Tue, Mar 22, 2022 at 01:21:05AM +0100, Andrew Lunn wrote:
> > > The use case is generic i.e. require separate handle to internal SGMII
> > > and external Phy so would prefer this new DT convention is 
> > > standardized or we discuss possible approaches on how to handle
> > > both phys and not add it as vendor specific property in the first 
> > > place.
> > 
> > IMO, you should use 'phys' for the internal PCS phy. That's aligned with 
> > other uses like PCIe, SATA, etc. (there is phy h/w that will do PCS, 
> > PCIe, SATA). 'phy-handle' is for the ethernet PHY.
> 
> We need to be careful here, because the PCS can have a well defined
> set of registers accessible over MDIO. Generic PHY has no
> infrastructure for that, it is all inside phylink which implements the
> pcs registers which are part of 802.3.

Using the phy binding doesn't mean you have to use the kernel's 'generic 
PHY' subsytem.

But if there's a need to do something different then propose something 
that handles the complex cases.

> 
> I also wonder if a PCS might actually have a generic PHY embedded in
> it to provide its lower interface?

That's just looking at a single PCS/PHY block the other way around. 
PCS is part of the PHY or the PHY is part of PCS? I don't think that 
matters too much. I think the 2 cases would be it's all 1 block or 2 
blocks.

Rob

[PATCH v4 4/4] net: axiemac: use a phandle to reference pcs_phy

[Andy Chiu]

In some SGMII use cases where both a fixed link external PHY and the
internal PCS/PMA PHY need to be configured, we should explicitly use a
phandle "pcs-phy" to get the reference to the PCS/PMA PHY. Otherwise, the
driver would use "phy-handle" in the DT as the reference to both the
external and the internal PCS/PMA PHY.

In other cases where the core is connected to a SFP cage, we could still
point phy-handle to the intenal PCS/PMA PHY, and let the driver connect
to the SFP module, if exist, via phylink.

Fixes: 1a02556086fc (net: axienet: Properly handle PCS/PMA PHY for 1000BaseX mode)
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 496a9227e760..163753508464 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2071,9 +2071,16 @@ static int axienet_probe(struct platform_device *pdev)
 
 	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
 	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
-		np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
+		np = of_parse_phandle(pdev->dev.of_node, "pcs-handle", 0);
 		if (!np) {
-			dev_err(&pdev->dev, "phy-handle required for 1000BaseX/SGMII\n");
+			/* Deprecated: Always use "pcs-handle" for pcs_phy.
+			 * Falling back to "phy-handle" here is only for
+			 * backward compatibility with old device trees.
+			 */
+			np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
+		}
+		if (!np) {
+			dev_err(&pdev->dev, "pcs-handle (preferred) or phy-handle required for 1000BaseX/SGMII\n");
 			ret = -EINVAL;
 			goto cleanup_mdio;
 		}
-- 
2.34.1

Re: [PATCH v4 4/4] net: axiemac: use a phandle to reference pcs_phy

[Robert Hancock]

On Mon, 2022-03-21 at 23:25 +0800, Andy Chiu wrote:
> In some SGMII use cases where both a fixed link external PHY and the
> internal PCS/PMA PHY need to be configured, we should explicitly use a
> phandle "pcs-phy" to get the reference to the PCS/PMA PHY. Otherwise, the
> driver would use "phy-handle" in the DT as the reference to both the
> external and the internal PCS/PMA PHY.
> 
> In other cases where the core is connected to a SFP cage, we could still
> point phy-handle to the intenal PCS/PMA PHY, and let the driver connect
> to the SFP module, if exist, via phylink.
> 
> Fixes: 1a02556086fc (net: axienet: Properly handle PCS/PMA PHY for 1000BaseX
> mode)
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 496a9227e760..163753508464 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2071,9 +2071,16 @@ static int axienet_probe(struct platform_device *pdev)
>  
>  	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
>  	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
> -		np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> +		np = of_parse_phandle(pdev->dev.of_node, "pcs-handle", 0);
>  		if (!np) {
> -			dev_err(&pdev->dev, "phy-handle required for
> 1000BaseX/SGMII\n");
> +			/* Deprecated: Always use "pcs-handle" for pcs_phy.
> +			 * Falling back to "phy-handle" here is only for
> +			 * backward compatibility with old device trees.
> +			 */
> +			np = of_parse_phandle(pdev->dev.of_node, "phy-handle",
> 0);
> +		}
> +		if (!np) {
> +			dev_err(&pdev->dev, "pcs-handle (preferred) or phy-
> handle required for 1000BaseX/SGMII\n");
>  			ret = -EINVAL;
>  			goto cleanup_mdio;
>  		}

Reviewed-by: Robert Hancock <robert.hancock@calian.com>

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH v4 1/4] net: axienet: setup mdio unconditionally

[Robert Hancock]

On Mon, 2022-03-21 at 23:25 +0800, Andy Chiu wrote:
> The call to axienet_mdio_setup should not depend on whether "phy-node"
> pressents on the DT. Besides, since `lp->phy_node` is used if PHY is in
> SGMII or 100Base-X modes, move it into the if statement. And the next patch
> will remove `lp->phy_node` from driver's private structure and do an
> of_node_put on it right away after use since it is not used elsewhere.
> 
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Reviewed-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 6fd5157f0a6d..5d41b8de840a 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2064,15 +2064,14 @@ static int axienet_probe(struct platform_device
> *pdev)
>  	if (ret)
>  		goto cleanup_clk;
>  
> -	lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> -	if (lp->phy_node) {
> -		ret = axienet_mdio_setup(lp);
> -		if (ret)
> -			dev_warn(&pdev->dev,
> -				 "error registering MDIO bus: %d\n", ret);
> -	}
> +	ret = axienet_mdio_setup(lp);
> +	if (ret)
> +		dev_warn(&pdev->dev,
> +			 "error registering MDIO bus: %d\n", ret);
> +
>  	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
>  	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
> +		lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-
> handle", 0);
>  		if (!lp->phy_node) {
>  			dev_err(&pdev->dev, "phy-handle required for
> 1000BaseX/SGMII\n");
>  			ret = -EINVAL;

Reviewed-by: Robert Hancock <robert.hancock@calian.com>

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH v4 1/4] net: axienet: setup mdio unconditionally

[Andrew Lunn]

Hi Andy

A patch series should always have a patch 0/X which explains the
overall purpose of the series. That then helps people understand how
the individual patches fit together.

Please also read the netdev FAQ. You are missing an indication of
which tree these patches are for.

      Andrew

Re: [PATCH v4 1/4] net: axienet: setup mdio unconditionally

[Andy Chiu]

Thanks for pointing that out.

The patchset I submitted is based on the net-next tree. But I think it
should be on the net tree after reading the FAQ since it is a bug fix
on DT handling. I will rework the patches and follow the format in a
v5 patch.

Andy