Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 3 Jun 2021 19:14:07 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     "Sit, Michael Wei Hong" <michael.wei.hong.sit@intel.com>
Cc:     "Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>,
        "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
        "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "peppe.cavallaro@st.com" <peppe.cavallaro@st.com>,
        "alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
        "Voon, Weifeng" <weifeng.voon@intel.com>,
        "Ong, Boon Leong" <boon.leong.ong@intel.com>,
        "Tan, Tee Min" <tee.min.tan@intel.com>,
        "vee.khee.wong@linux.intel.com" <vee.khee.wong@linux.intel.com>,
        "Wong, Vee Khee" <vee.khee.wong@intel.com>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH net-next v4 1/3] net: stmmac: split xPCS setup
 from mdio register
Message-ID: <20210603161407.457olvjmia3zoj6w@skbuf>
References: <20210603115032.2470-1-michael.wei.hong.sit@intel.com>
 <20210603115032.2470-2-michael.wei.hong.sit@intel.com>
 <20210603132056.zklgtbsslbkgqtsn@skbuf>
 <SA2PR11MB50513D751429D3D456A5A9409D3C9@SA2PR11MB5051.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SA2PR11MB50513D751429D3D456A5A9409D3C9@SA2PR11MB5051.namprd11.prod.outlook.com>
Precedence: bulk

On Thu, Jun 03, 2021 at 01:49:20PM +0000, Sit, Michael Wei Hong wrote:
> Hi Vladimir,
> 
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: Thursday, June 3, 2021 9:21 PM
> > To: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>
> > Cc: Jose.Abreu@synopsys.com; andrew@lunn.ch;
> > hkallweit1@gmail.com; linux@armlinux.org.uk; kuba@kernel.org;
> > netdev@vger.kernel.org; peppe.cavallaro@st.com;
> > alexandre.torgue@foss.st.com; davem@davemloft.net;
> > mcoquelin.stm32@gmail.com; Voon, Weifeng
> > <weifeng.voon@intel.com>; Ong, Boon Leong
> > <boon.leong.ong@intel.com>; Tan, Tee Min
> > <tee.min.tan@intel.com>; vee.khee.wong@linux.intel.com;
> > Wong, Vee Khee <vee.khee.wong@intel.com>; linux-stm32@st-
> > md-mailman.stormreply.com; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [RESEND PATCH net-next v4 1/3] net: stmmac: split
> > xPCS setup from mdio register
> > 
> > Hi Michael,
> > 
> > On Thu, Jun 03, 2021 at 07:50:30PM +0800, Michael Sit Wei Hong wrote:
> > > From: Voon Weifeng <weifeng.voon@intel.com>
> > >
> > > This patch is a preparation patch for the enabling of Intel mGbE
> > > 2.5Gbps link speed. The Intel mGbR link speed configuration (1G/2.5G)
> > > is depends on a mdio ADHOC register which can be configured in the bios menu.
> > > As PHY interface might be different for 1G and 2.5G, the mdio bus need
> > > be ready to check the link speed and select the PHY interface before
> > > probing the xPCS.
> > >
> > > Signed-off-by: Voon Weifeng <weifeng.voon@intel.com>
> > > Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
> > > .../net/ethernet/stmicro/stmmac/stmmac_main.c |  7 ++
> > > .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 73 ++++++++++---------
> > >  3 files changed, 46 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > > b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > > index b6cd43eda7ac..fd7212afc543 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > > @@ -311,6 +311,7 @@ enum stmmac_state {  int
> > > stmmac_mdio_unregister(struct net_device *ndev);  int
> > > stmmac_mdio_register(struct net_device *ndev);  int
> > > stmmac_mdio_reset(struct mii_bus *mii);
> > > +int stmmac_xpcs_setup(struct mii_bus *mii);
> > >  void stmmac_set_ethtool_ops(struct net_device *netdev);
> > >
> > >  void stmmac_ptp_register(struct stmmac_priv *priv); diff --git
> > > a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index 13720bf6f6ff..eb81baeb13b0 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -7002,6 +7002,12 @@ int stmmac_dvr_probe(struct device
> > *device,
> > >  		}
> > >  	}
> > >
> > > +	if (priv->plat->mdio_bus_data->has_xpcs) {
> > > +		ret = stmmac_xpcs_setup(priv->mii);
> > > +		if (ret)
> > > +			goto error_xpcs_setup;
> > > +	}
> > > +
> > 
> > I don't understand why this change is necessary?
> > 
> > The XPCS probing code was at the end of stmmac_mdio_register().
> > You moved the code right _after_ stmmac_mdio_register().
> > So the code flow is exactly the same.
> > 
> Yes, the code flow may look the same, but for intel platforms,
> we need to read the mdio ADHOC register to determine the link speed
> that is set in the BIOS, after reading the mdio ADHOC register value,
> we can determine the link speed and set the appropriate phy_interface
> for 1G/2.5G, where 2.5G uses the PHY_INTERFACE_MODE_2500BASEX
> and 1G uses the PHY_INTERFACE_MODE_SGMII.
> 
> The register reading function is added in between the mdio_register and
> xpcs_setup in patch 3 of the series

Ah, ok, I did not notice this bit:

@@ -7002,6 +7006,9 @@ int stmmac_dvr_probe(struct device *device,
 		}
 	}
 
+	if (priv->plat->speed_mode_2500)
+		priv->plat->speed_mode_2500(ndev, priv->plat->bsp_priv);
+
 	if (priv->plat->mdio_bus_data->has_xpcs) {
 		ret = stmmac_xpcs_setup(priv->mii);
 		if (ret)

With the current placement, there seems to be indeed no way for the
platform-level code to set plat->phy_interface after the MDIO bus has
probed but before the XPCS has probed.

I wonder whether it might be possible to probe the XPCS completely
outside of stmmac_dvr_probe(); once that function ends you should have
all knowledge necessary to set plat->phy_interface all within the Intel
platform code. An additional benefit if you do this is that you no
longer need the has_xpcs variable - Intel is the only one setting it
right now, as far as I can see. What do you think?