[PATCH] net: gem: Fix setting PCS auto-negotiation state
The code was trying to disable PCS auto-negotiation when a fixed-link node is present and enable it otherwise. However, the PCS registers were being written before the PCSSEL bit was set in the network configuration register, and it appears that in this state, PCS register writes are ignored. The result is that the intended change only took effect on the second network operation that was performed, since at that time PCSSEL is already enabled.
Fix the order of register writes so that PCS registers are only written to after the PCS is enabled.
Fixes: 26e62cc971 ("net: gem: Disable PCS autonegotiation in case of fixed-link")
Signed-off-by: Robert Hancock robert.hancock@calian.com --- drivers/net/zynq_gem.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index baf06a2ad8..ff59982267 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -454,14 +454,6 @@ static int zynq_gem_init(struct udevice *dev) priv->int_pcs) { nwconfig |= ZYNQ_GEM_NWCFG_SGMII_ENBL | ZYNQ_GEM_NWCFG_PCS_SEL; -#ifdef CONFIG_ARM64 - if (priv->phydev->phy_id != PHY_FIXED_ID) - writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL, - ®s->pcscntrl); - else - writel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL, - ®s->pcscntrl); -#endif }
switch (priv->phydev->speed) { @@ -480,6 +472,23 @@ static int zynq_gem_init(struct udevice *dev) break; }
+#ifdef CONFIG_ARM64 + if (priv->interface == PHY_INTERFACE_MODE_SGMII && + priv->int_pcs) { + /* + * Disable AN for fixed link configuration, enable otherwise. + * Must be written after PCS_SEL is set in nwconfig, + * otherwise writes will not take effect. + */ + if (priv->phydev->phy_id != PHY_FIXED_ID) + writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL, + ®s->pcscntrl); + else + writel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL, + ®s->pcscntrl); + } +#endif + ret = clk_set_rate(&priv->tx_clk, clk_rate); if (IS_ERR_VALUE(ret)) { dev_err(dev, "failed to set tx clock rate\n");
On Fri, Mar 12, 2021 at 12:56 AM Robert Hancock robert.hancock@calian.com wrote:
The code was trying to disable PCS auto-negotiation when a fixed-link node is present and enable it otherwise. However, the PCS registers were being written before the PCSSEL bit was set in the network configuration register, and it appears that in this state, PCS register writes are ignored. The result is that the intended change only took effect on the second network operation that was performed, since at that time PCSSEL is already enabled.
Fix the order of register writes so that PCS registers are only written to after the PCS is enabled.
Fixes: 26e62cc971 ("net: gem: Disable PCS autonegotiation in case of fixed-link")
Signed-off-by: Robert Hancock robert.hancock@calian.com
drivers/net/zynq_gem.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index baf06a2ad8..ff59982267 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -454,14 +454,6 @@ static int zynq_gem_init(struct udevice *dev) priv->int_pcs) { nwconfig |= ZYNQ_GEM_NWCFG_SGMII_ENBL | ZYNQ_GEM_NWCFG_PCS_SEL; -#ifdef CONFIG_ARM64
if (priv->phydev->phy_id != PHY_FIXED_ID)writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);elsewritel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);-#endif }
switch (priv->phydev->speed) {@@ -480,6 +472,23 @@ static int zynq_gem_init(struct udevice *dev) break; }
+#ifdef CONFIG_ARM64
if (priv->interface == PHY_INTERFACE_MODE_SGMII &&priv->int_pcs) {/** Disable AN for fixed link configuration, enable otherwise.* Must be written after PCS_SEL is set in nwconfig,* otherwise writes will not take effect.*/if (priv->phydev->phy_id != PHY_FIXED_ID)writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);elsewritel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);}+#endif
ret = clk_set_rate(&priv->tx_clk, clk_rate); if (IS_ERR_VALUE(ret)) { dev_err(dev, "failed to set tx clock rate\n");-- 2.27.0
Reviewed-by: Ramon Fried rfried.dev@gmail.com
On 3/11/21 11:55 PM, Robert Hancock wrote:
The code was trying to disable PCS auto-negotiation when a fixed-link node is present and enable it otherwise. However, the PCS registers were being written before the PCSSEL bit was set in the network configuration register, and it appears that in this state, PCS register writes are ignored. The result is that the intended change only took effect on the second network operation that was performed, since at that time PCSSEL is already enabled.
Fix the order of register writes so that PCS registers are only written to after the PCS is enabled.
Fixes: 26e62cc971 ("net: gem: Disable PCS autonegotiation in case of fixed-link")
Signed-off-by: Robert Hancock robert.hancock@calian.com
drivers/net/zynq_gem.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index baf06a2ad8..ff59982267 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -454,14 +454,6 @@ static int zynq_gem_init(struct udevice *dev) priv->int_pcs) { nwconfig |= ZYNQ_GEM_NWCFG_SGMII_ENBL | ZYNQ_GEM_NWCFG_PCS_SEL; -#ifdef CONFIG_ARM64
- if (priv->phydev->phy_id != PHY_FIXED_ID)
writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);- else
writel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);-#endif }
switch (priv->phydev->speed) { @@ -480,6 +472,23 @@ static int zynq_gem_init(struct udevice *dev) break; }
+#ifdef CONFIG_ARM64
- if (priv->interface == PHY_INTERFACE_MODE_SGMII &&
priv->int_pcs) {/** Disable AN for fixed link configuration, enable otherwise.* Must be written after PCS_SEL is set in nwconfig,* otherwise writes will not take effect.*/if (priv->phydev->phy_id != PHY_FIXED_ID)writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);elsewritel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);- }
+#endif
- ret = clk_set_rate(&priv->tx_clk, clk_rate); if (IS_ERR_VALUE(ret)) { dev_err(dev, "failed to set tx clock rate\n");
Karthik/Ashok: Please retest it and reply.
Thanks, Michal
Reviewed-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: Monday, March 15, 2021 2:38 PM To: Robert Hancock robert.hancock@calian.com; Michal Simek michals@xilinx.com; T Karthik Reddy tkarthik@xilinx.com; Ashok Reddy Soma ashokred@xilinx.com Cc: joe.hershberger@ni.com; rfried.dev@gmail.com; u-boot@lists.denx.de Subject: Re: [PATCH] net: gem: Fix setting PCS auto-negotiation statee
On 3/11/21 11:55 PM, Robert Hancock wrote:
The code was trying to disable PCS auto-negotiation when a fixed-link node is present and enable it otherwise. However, the PCS registers were being written before the PCSSEL bit was set in the network configuration register, and it appears that in this state, PCS register writes are ignored. The result is that the intended change only took effect on the second network operation that was performed, since at that time PCSSEL is already enabled.
Fix the order of register writes so that PCS registers are only written to after the PCS is enabled.
Fixes: 26e62cc971 ("net: gem: Disable PCS autonegotiation in case of fixed-link")
Signed-off-by: Robert Hancock robert.hancock@calian.com
drivers/net/zynq_gem.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index baf06a2ad8..ff59982267 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -454,14 +454,6 @@ static int zynq_gem_init(struct udevice *dev) priv->int_pcs) { nwconfig |= ZYNQ_GEM_NWCFG_SGMII_ENBL | ZYNQ_GEM_NWCFG_PCS_SEL; -#ifdef CONFIG_ARM64
- if (priv->phydev->phy_id != PHY_FIXED_ID)
writel(readl(®s->pcscntrl) |ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
®s->pcscntrl);- else
writel(readl(®s->pcscntrl) &~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
®s->pcscntrl);-#endif }
switch (priv->phydev->speed) { @@ -480,6 +472,23 @@ static int zynq_gem_init(struct udevice *dev) break; }
+#ifdef CONFIG_ARM64
- if (priv->interface == PHY_INTERFACE_MODE_SGMII &&
priv->int_pcs) {/** Disable AN for fixed link configuration, enable otherwise.* Must be written after PCS_SEL is set in nwconfig,* otherwise writes will not take effect.*/if (priv->phydev->phy_id != PHY_FIXED_ID)writel(readl(®s->pcscntrl) |ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
®s->pcscntrl);elsewritel(readl(®s->pcscntrl) &~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
®s->pcscntrl);- }
+#endif
- ret = clk_set_rate(&priv->tx_clk, clk_rate); if (IS_ERR_VALUE(ret)) { dev_err(dev, "failed to set tx clock rate\n");
Karthik/Ashok: Please retest it and reply.
Looks good.
Thanks, Michal
On 3/11/21 11:55 PM, Robert Hancock wrote:
The code was trying to disable PCS auto-negotiation when a fixed-link node is present and enable it otherwise. However, the PCS registers were being written before the PCSSEL bit was set in the network configuration register, and it appears that in this state, PCS register writes are ignored. The result is that the intended change only took effect on the second network operation that was performed, since at that time PCSSEL is already enabled.
Fix the order of register writes so that PCS registers are only written to after the PCS is enabled.
Fixes: 26e62cc971 ("net: gem: Disable PCS autonegotiation in case of fixed-link")
Signed-off-by: Robert Hancock robert.hancock@calian.com
drivers/net/zynq_gem.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index baf06a2ad8..ff59982267 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -454,14 +454,6 @@ static int zynq_gem_init(struct udevice *dev) priv->int_pcs) { nwconfig |= ZYNQ_GEM_NWCFG_SGMII_ENBL | ZYNQ_GEM_NWCFG_PCS_SEL; -#ifdef CONFIG_ARM64
- if (priv->phydev->phy_id != PHY_FIXED_ID)
writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);- else
writel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);-#endif }
switch (priv->phydev->speed) { @@ -480,6 +472,23 @@ static int zynq_gem_init(struct udevice *dev) break; }
+#ifdef CONFIG_ARM64
- if (priv->interface == PHY_INTERFACE_MODE_SGMII &&
priv->int_pcs) {/** Disable AN for fixed link configuration, enable otherwise.* Must be written after PCS_SEL is set in nwconfig,* otherwise writes will not take effect.*/if (priv->phydev->phy_id != PHY_FIXED_ID)writel(readl(®s->pcscntrl) | ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);elsewritel(readl(®s->pcscntrl) & ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,®s->pcscntrl);- }
+#endif
- ret = clk_set_rate(&priv->tx_clk, clk_rate); if (IS_ERR_VALUE(ret)) { dev_err(dev, "failed to set tx clock rate\n");
Applied. M
participants (4)
-
Ashok Reddy Soma -
Michal Simek -
Ramon Fried -
Robert Hancock