Messages in this thread Patch in this message | | | Date | Thu, 3 Jun 2021 19:14:07 +0300 | From | Vladimir Oltean <> | Subject | Re: [RESEND PATCH net-next v4 1/3] net: stmmac: split xPCS setup from mdio register |
| |
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?
| |