| Message ID | 20220316075953.61398-1-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: axiemac: use a phandle to reference pcs_phy | expand |
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
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: 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.
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
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;