| Message ID | 20240208111714.11456-1-quic_snehshah@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v4] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII | expand |
On Thu, 8 Feb 2024 16:47:14 +0530 Sneh Shah wrote: > Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000 > mode for 1G/100M/10M speed. > Added changes to configure serdes phy and mac based on link speed. > Changing serdes phy speed involves multiple register writes for > serdes block. To avoid redundant write operations only update serdes > phy when new speed is different. Sounds like 2 separate changes in one patch, please split the optimization of not writing the registers multiple times and the 2.5G support. > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > index 31631e3f89d0..6bbdbb7bef44 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > @@ -106,6 +106,7 @@ struct qcom_ethqos { > struct clk *link_clk; > struct phy *serdes_phy; > unsigned int speed; > + int serdes_speed; Why signed if speed itself is unsigned? > /* Enable and restart the Auto-Negotiation */ > if (ane) > value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN; > + else > + value &= ~GMAC_AN_CTRL_ANE; That looks unrelated. Either a separate patch or please explain in the commit msg why.
On 2/13/2024 7:00 AM, Jakub Kicinski wrote: > On Thu, 8 Feb 2024 16:47:14 +0530 Sneh Shah wrote: >> Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000 >> mode for 1G/100M/10M speed. >> Added changes to configure serdes phy and mac based on link speed. >> Changing serdes phy speed involves multiple register writes for >> serdes block. To avoid redundant write operations only update serdes >> phy when new speed is different. > > Sounds like 2 separate changes in one patch, please split the > optimization of not writing the registers multiple times and > the 2.5G support. > Optimization is part of 2.5G support change only. with introduction of 2.5G speed support we need to update reconfigure serdes phy. there are 2 different serdes configs. 1. config for 2.5G 2. common config 1G/100M/10M The change here is not to reconfigure serdes phy among 1G/100M/10M speeds and only reconfigure if it switches to 2.5G. >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >> index 31631e3f89d0..6bbdbb7bef44 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >> @@ -106,6 +106,7 @@ struct qcom_ethqos { >> struct clk *link_clk; >> struct phy *serdes_phy; >> unsigned int speed; >> + int serdes_speed; > > Why signed if speed itself is unsigned? In stmmac speed can be assigned as SPEED_UNKNOWN which will be -1. I will fix speed being unsigned int in a separate patch. > >> /* Enable and restart the Auto-Negotiation */ >> if (ane) >> value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN; >> + else >> + value &= ~GMAC_AN_CTRL_ANE; > > That looks unrelated. Either a separate patch or please explain in the > commit msg why. For 2.5G speed support MAC PCS autoneg needs to be disabled. This adds the change to disable PCS autoneg. I will update commit message to add more dtails
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c index 31631e3f89d0..6bbdbb7bef44 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c @@ -106,6 +106,7 @@ struct qcom_ethqos { struct clk *link_clk; struct phy *serdes_phy; unsigned int speed; + int serdes_speed; phy_interface_t phy_mode; const struct ethqos_emac_por *por; @@ -606,19 +607,39 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos) */ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos) { + struct net_device *dev = platform_get_drvdata(ethqos->pdev); + struct stmmac_priv *priv = netdev_priv(dev); int val; val = readl(ethqos->mac_base + MAC_CTRL_REG); switch (ethqos->speed) { + case SPEED_2500: + val &= ~ETHQOS_MAC_CTRL_PORT_SEL; + rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG, + RGMII_CONFIG2_RGMII_CLK_SEL_CFG, + RGMII_IO_MACRO_CONFIG2); + if (ethqos->serdes_speed != SPEED_2500) + phy_set_speed(ethqos->serdes_phy, SPEED_2500); + ethqos->serdes_speed = SPEED_2500; + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 0, 0, 0); + break; case SPEED_1000: val &= ~ETHQOS_MAC_CTRL_PORT_SEL; rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG, RGMII_CONFIG2_RGMII_CLK_SEL_CFG, RGMII_IO_MACRO_CONFIG2); + if (ethqos->serdes_speed != SPEED_1000) + phy_set_speed(ethqos->serdes_phy, SPEED_1000); + ethqos->serdes_speed = SPEED_1000; + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); break; case SPEED_100: val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE; + if (ethqos->serdes_speed != SPEED_1000) + phy_set_speed(ethqos->serdes_phy, SPEED_1000); + ethqos->serdes_speed = SPEED_1000; + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); break; case SPEED_10: val |= ETHQOS_MAC_CTRL_PORT_SEL; @@ -627,6 +648,10 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos) FIELD_PREP(RGMII_CONFIG_SGMII_CLK_DVDR, SGMII_10M_RX_CLK_DVDR), RGMII_IO_MACRO_CONFIG); + if (ethqos->serdes_speed != SPEED_1000) + phy_set_speed(ethqos->serdes_phy, ethqos->speed); + ethqos->serdes_speed = SPEED_1000; + stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0); break; } @@ -799,6 +824,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev) "Failed to get serdes phy\n"); ethqos->speed = SPEED_1000; + ethqos->serdes_speed = SPEED_1000; ethqos_update_link_clk(ethqos, SPEED_1000); ethqos_set_func_clk_en(ethqos); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h index aefc121464b5..13a30e6df4c1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h @@ -110,6 +110,8 @@ static inline void dwmac_ctrl_ane(void __iomem *ioaddr, u32 reg, bool ane, /* Enable and restart the Auto-Negotiation */ if (ane) value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN; + else + value &= ~GMAC_AN_CTRL_ANE; /* In case of MAC-2-MAC connection, block is configured to operate * according to MAC conf register.
Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000 mode for 1G/100M/10M speed. Added changes to configure serdes phy and mac based on link speed. Changing serdes phy speed involves multiple register writes for serdes block. To avoid redundant write operations only update serdes phy when new speed is different. Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com> --- v4 changelog: - Made cosmetic changes v3 changelog: - updated commit message --- v2 changelog: - updated stmmac_pcs_ane to support autoneg disable - Update serdes speed to 1000 for 100M and 10M also--- .../stmicro/stmmac/dwmac-qcom-ethqos.c | 26 +++++++++++++++++++ .../net/ethernet/stmicro/stmmac/stmmac_pcs.h | 2 ++ 2 files changed, 28 insertions(+)