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

[Sit, Michael Wei Hong]

> -----Original Message-----
> From: Raczynski, Piotr <piotr.raczynski@xxxxxxxxx>
> Sent: Tuesday, March 14, 2023 5:02 PM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit@xxxxxxxxx>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@xxxxxx>; Alexandre
> Torgue <alexandre.torgue@xxxxxxxxxxx>; Jose Abreu
> <joabreu@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski
> <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Maxime
> Coquelin <mcoquelin.stm32@xxxxxxxxx>; Ong, Boon Leong
> <boon.leong.ong@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Looi,
> Hong Aun <hong.aun.looi@xxxxxxxxx>; Voon, Weifeng
> <weifeng.voon@xxxxxxxxx>; Lai, Peter Jun Ann
> <peter.jun.ann.lai@xxxxxxxxx>
> 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@xxxxxxxxx>
> > ---
> >  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
> >