[PATCH net 2/2] net: stmmac: move fixed-link support fixup code

[Sit, Michael Wei Hong]

> -----Original Message-----
> From: Raczynski, Piotr <piotr.raczynski at intel.com>
> Sent: Tuesday, March 14, 2023 5:02 PM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit at intel.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro at st.com>; Alexandre
> Torgue <alexandre.torgue at foss.st.com>; Jose Abreu
> <joabreu at synopsys.com>; David S . Miller <davem at davemloft.net>;
> Eric Dumazet <edumazet at google.com>; Jakub Kicinski
> <kuba at kernel.org>; Paolo Abeni <pabeni at redhat.com>; Maxime
> Coquelin <mcoquelin.stm32 at gmail.com>; Ong, Boon Leong
> <boon.leong.ong at intel.com>; netdev at vger.kernel.org; linux-
> stm32 at st-md-mailman.stormreply.com; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Looi,
> Hong Aun <hong.aun.looi at intel.com>; Voon, Weifeng
> <weifeng.voon at intel.com>; Lai, Peter Jun Ann
> <peter.jun.ann.lai at intel.com>
> Subject: Re: [PATCH net 2/2] net: stmmac: move fixed-link support
> fixup code
> 
> On Mon, Mar 13, 2023 at 04:01:35PM +0800, Michael Sit Wei Hong
> wrote:
> > xpcs_an_inband value is updated in the speed_mode_2500
> function which
> > turns on the xpcs_an_inband mode.
> >
> > Moving the fixed-link fixup code to right before phylink setup to
> > ensure no more fixup will affect the fixed-link mode configurations.
> >
> > Fixes: 72edaf39fc65 ("stmmac: intel: add phy-mode and fixed-link
> ACPI
> > _DSD setting support")
> > Signed-off-by: Michael Sit Wei Hong
> <michael.wei.hong.sit at intel.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 11 ---------
> --
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15
> +++++++++++++++
> >  2 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > index 7deb1f817dac..d02db2b529b9 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > @@ -592,17 +592,6 @@ static int
> intel_mgbe_common_data(struct pci_dev *pdev,
> >  		plat->mdio_bus_data->xpcs_an_inband = true;
> >  	}
> >
> > -	/* For fixed-link setup, we clear xpcs_an_inband */
> > -	if (fwnode) {
> > -		struct fwnode_handle *fixed_node;
> > -
> > -		fixed_node =
> fwnode_get_named_child_node(fwnode, "fixed-link");
> > -		if (fixed_node)
> > -			plat->mdio_bus_data->xpcs_an_inband =
> false;
> > -
> > -		fwnode_handle_put(fixed_node);
> > -	}
> > -
> >  	/* Ensure mdio bus scan skips intel serdes and pcs-xpcs */
> >  	plat->mdio_bus_data->phy_mask = 1 <<
> INTEL_MGBE_ADHOC_ADDR;
> >  	plat->mdio_bus_data->phy_mask |= 1 <<
> INTEL_MGBE_XPCS_ADDR; diff
> > --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 398adcd68ee8..5a9abafba490 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7064,6 +7064,7 @@ int stmmac_dvr_probe(struct device
> *device,
> >  		     struct stmmac_resources *res)
> >  {
> >  	struct net_device *ndev = NULL;
> > +	struct fwnode_handle *fwnode;
> >  	struct stmmac_priv *priv;
> >  	u32 rxq;
> >  	int i, ret = 0;
> > @@ -7306,6 +7307,20 @@ int stmmac_dvr_probe(struct device
> *device,
> >  			goto error_xpcs_setup;
> >  	}
> >
> > +	/* For fixed-link setup, we clear xpcs_an_inband */
> > +	if (!fwnode)
> > +		fwnode = dev_fwnode(priv->device);
> > +
> > +	if (fwnode) {
> > +		struct fwnode_handle *fixed_node;
> > +
> > +		fixed_node =
> fwnode_get_named_child_node(fwnode, "fixed-link");
> > +		if (fixed_node)
> > +			priv->plat->mdio_bus_data-
> >xpcs_an_inband = false;
> > +
> > +		fwnode_handle_put(fixed_node);
> > +	}
> > +
> 
> Now you're doing similar checks here and inside stmmac_init_phy.
> Maybe you could combined this to some function?
> 
> Piotr
In stmmac_dvr_probe, the check for fixed-link  is to fixup the xpcs_an_inband
data. Whereas in stmmac_init_phy, we store the fixed_node info to
determine if we need to setup the PHY handle manually in the later code,
and will run phylink_fwnode_phy_connect which uses the fwnode variable.
The NULL check on fwnode is to ensure the functions that uses the fwnode
does not access NULL pointers. It might be difficult to combine it into a
function due to the way fixed_node is used.
> 
> >  	ret = stmmac_phy_setup(priv);
> >  	if (ret) {
> >  		netdev_err(ndev, "failed to setup phy (%d)\n", ret);
> > --
> > 2.34.1
> >