[v2] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII
Commit Message
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.
Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com>
---
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 | 27 +++++++++++++++++++
.../net/ethernet/stmicro/stmmac/stmmac_pcs.h | 2 ++
2 files changed, 29 insertions(+)
Comments
On Mon, Jan 08, 2024 at 05:41:28PM +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.
Please take a look at:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
The Subject is missing which tree this is for. Also, net-next is
closed at the moment.
> 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);
Is calling phy_set_speed() expensive? Why not just unconditionally
call it?
Andrew
On 1/9/2024 1:00 AM, Andrew Lunn wrote:
> On Mon, Jan 08, 2024 at 05:41:28PM +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.
>
> Please take a look at:
>
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
>
> The Subject is missing which tree this is for. Also, net-next is
> closed at the moment.
It was supposed to be net-next. Missed updating in subject.
Sorry for that!
If net-next is closed at the moment, how to proceed further?
Should I wait until it gets reopened?
>
>> 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);
>
> Is calling phy_set_speed() expensive? Why not just unconditionally
> call it?
>
It reconfigures whole serdes phy block, with lots of register read/writes.
So I feel it is better to avoid doing this unconditionally
> Andrew
On Tue, Jan 09, 2024 at 08:22:40PM +0530, Sneh Shah wrote:
>
>
> On 1/9/2024 1:00 AM, Andrew Lunn wrote:
> > On Mon, Jan 08, 2024 at 05:41:28PM +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.
> >
> > Please take a look at:
> >
> > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> >
> > The Subject is missing which tree this is for. Also, net-next is
> > closed at the moment.
>
> It was supposed to be net-next. Missed updating in subject.
> Sorry for that!
> If net-next is closed at the moment, how to proceed further?
> Should I wait until it gets reopened?
Yes, please repost in two weeks time.
> >> 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);
> >
> > Is calling phy_set_speed() expensive? Why not just unconditionally
> > call it?
> >
> It reconfigures whole serdes phy block, with lots of register read/writes.
> So I feel it is better to avoid doing this unconditionally
O.K, please add this to the commit message.
Part of the purpose of the commit message is to try to answer
questions the reviewers are going to ask when they look at the
code. Its better to put more in the commit message than less, it helps
get your code merged faster, and reduces the load on reviewers.
Andrew
@@ -103,6 +103,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;
@@ -602,21 +603,46 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
{
int val;
+ struct platform_device *pdev = ethqos->pdev;
+ struct net_device *dev = platform_get_drvdata(pdev);
+ struct stmmac_priv *priv = netdev_priv(dev);
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;
val &= ~ETHQOS_MAC_CTRL_SPEED_MODE;
+ 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;
}
@@ -789,6 +815,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);
@@ -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.