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

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

[Andy Chiu]

The `of_mdio_find_device()` would get a reference to `mdio_device` by
a given device tree node. Thus, getting a reference to the internal
PCS/PMA PHY by `lp->phy_node` is incorrect since "phy-handle" in the DT
sould point to the external PHY. This incorrect use of "phy-hanlde"
would cause a problem when the driver called `phylink_of_phy_connect()`,
where it would use "phy-handle" in the DT to connect the external PHY.
To fix it, we could add a phandle, "pcs-phy", in the DT so that it would
point to the internal PHY. And the driver would get the correct address
of PCS/PHY.

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 6fd5157f0a6d..293189aab4e6 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2073,12 +2073,15 @@ static int axienet_probe(struct platform_device *pdev)
 	}
 	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
 	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
-		if (!lp->phy_node) {
-			dev_err(&pdev->dev, "phy-handle required for 1000BaseX/SGMII\n");
+		np = of_parse_phandle(pdev->dev.of_node, "pcs-phy", 0);
+		if (!lp->phy_node || !np) {
+			dev_err(&pdev->dev, "phy-handle and pcs-phy are required for 1000BaseX/SGMII\n");
+			of_node_put(np);
 			ret = -EINVAL;
 			goto cleanup_mdio;
 		}
-		lp->pcs_phy = of_mdio_find_device(lp->phy_node);
+		lp->pcs_phy = of_mdio_find_device(np);
+		of_node_put(np);
 		if (!lp->pcs_phy) {
 			ret = -EPROBE_DEFER;
 			goto cleanup_mdio;
-- 
2.34.1

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

[Andrew Lunn]

On Wed, Mar 16, 2022 at 03:59:53PM +0800, Andy Chiu wrote:
> The `of_mdio_find_device()` would get a reference to `mdio_device` by
> a given device tree node. Thus, getting a reference to the internal
> PCS/PMA PHY by `lp->phy_node` is incorrect since "phy-handle" in the DT
> sould point to the external PHY. This incorrect use of "phy-hanlde"
> would cause a problem when the driver called `phylink_of_phy_connect()`,
> where it would use "phy-handle" in the DT to connect the external PHY.
> To fix it, we could add a phandle, "pcs-phy", in the DT so that it would
> point to the internal PHY. And the driver would get the correct address
> of PCS/PHY.
> 
> 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 | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 6fd5157f0a6d..293189aab4e6 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2073,12 +2073,15 @@ static int axienet_probe(struct platform_device *pdev)
>  	}
>  	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
>  	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
> -		if (!lp->phy_node) {
> -			dev_err(&pdev->dev, "phy-handle required for 1000BaseX/SGMII\n");
> +		np = of_parse_phandle(pdev->dev.of_node, "pcs-phy", 0);

Other drivers call this pcs-handle. Please be consistent with them.

You also should update the binding document with this new property.

	Andrew

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

[Jakub Kicinski]

On Wed, 16 Mar 2022 15:59:53 +0800 Andy Chiu wrote:
> Fixes: 1a02556086fc (net: axienet: Properly handle PCS/PMA PHY for 1000BaseX mode)

Please make sure to CC the maintainer of the driver. You also ate an
"m" at the end of Robert's address.

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

[Robert Hancock]

Re: https://lore.kernel.org/all/20220316075953.61398-1-andy.chiu@sifive.com/
(looks like I was CCed with the wrong email):

I think we likely need something similar to this for the use case (I assume)
you have, where there's an external SGMII PHY as well as the internal PCS/PMA
PHY which both need to be configured. However, we (and possibly others) already
have some cases where the core is connected to an SFP cage, phy-handle points
to the internal PCS/PMA PHY, and the external PHY - if one exists at all - is
not in the device tree because it would be on an SFP module.

Also, as Jakub mentioned, it looks like other drivers like dpaa2 use pcs-handle 
for this.

Possibly something like: in the 1000Base-X or SGMII modes, if pcs-handle is
set, then use that as the PCS node, otherwise fall back to assuming phy-handle
points to the PCS. It should not require that both are present, since even in
the pure SGMII case, the PHY could still be supplied by an SFP module
downstream.

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

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

[Andy Chiu]

Thanks for the kind help and suggestions.

I will submit the v2 patchset with an updated flow that supports both
cases, and include a binding document. And use the correct mail
address.

Regards,
Andy

On Thu, Mar 17, 2022 at 5:15 AM Robert Hancock
<robert.hancock@calian.com> wrote:
>
> Re: https://lore.kernel.org/all/20220316075953.61398-1-andy.chiu@sifive.com/
> (looks like I was CCed with the wrong email):
>
> I think we likely need something similar to this for the use case (I assume)
> you have, where there's an external SGMII PHY as well as the internal PCS/PMA
> PHY which both need to be configured. However, we (and possibly others) already
> have some cases where the core is connected to an SFP cage, phy-handle points
> to the internal PCS/PMA PHY, and the external PHY - if one exists at all - is
> not in the device tree because it would be on an SFP module.
>
> Also, as Jakub mentioned, it looks like other drivers like dpaa2 use pcs-handle
> for this.
>
> Possibly something like: in the 1000Base-X or SGMII modes, if pcs-handle is
> set, then use that as the PCS node, otherwise fall back to assuming phy-handle
> points to the PCS. It should not require that both are present, since even in
> the pure SGMII case, the PHY could still be supplied by an SFP module
> downstream.
>
> --
> Robert Hancock
> Senior Hardware Designer, Calian Advanced Technologies
> www.calian.com