[PATCH net 0/2] net: bcmgenet: revisit MAC reset

[PATCH net 0/2] net: bcmgenet: revisit MAC reset

[Doug Berger]

Commit 3a55402c9387 ("net: bcmgenet: use RGMII loopback for MAC 
reset") was intended to resolve issues with reseting the UniMAC
core within the GENET block by providing better control over the
clocks used by the UniMAC core. Unfortunately, it is not
compatible with all of the supported system configurations so an
alternative method must be applied.

This commit set provides such an alternative. The first commit
reverts the previous change and the second commit provides the
alternative reset sequence that addresses the concerns observed
with the previous implementation.

This replacement implementation should be applied to the stable
branches wherever commit 3a55402c9387 ("net: bcmgenet: use RGMII 
loopback for MAC reset") has been applied.

Unfortunately, reverting that commit may conflict with some
restructuring changes introduced by commit 4f8d81b77e66 ("net: 
bcmgenet: Refactor register access in bcmgenet_mii_config").
The first commit in this set has been manually edited to
resolve the conflict on net/master. I would be happy to help
stable maintainers with resolving any such conflicts if they
occur. However, I do not expect that commit to have been
backported to stable branch so hopefully the revert can be
applied cleanly.

Doug Berger (2):
  Revert "net: bcmgenet: use RGMII loopback for MAC reset"
  net: bcmgenet: keep MAC in reset until PHY is up

 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 10 +++---
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c |  6 +++-
 drivers/net/ethernet/broadcom/genet/bcmmii.c       | 40 ++++------------------
 3 files changed, 16 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH net 1/2] Revert "net: bcmgenet: use RGMII loopback for MAC reset"

[Doug Berger]

This reverts commit 3a55402c93877d291b0a612d25edb03d1b4b93ac.

This is not a good solution when connecting to an external switch
that may not support the isolation of the TXC signal resulting in
output driver contention on the pin.

A different solution is necessary.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  2 ++
 drivers/net/ethernet/broadcom/genet/bcmmii.c   | 34 --------------------------
 2 files changed, 2 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index e50a15397e11..c8ac2d83208f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1989,6 +1989,8 @@ static void reset_umac(struct bcmgenet_priv *priv)
 
 	/* issue soft reset with (rg)mii loopback to ensure a stable rxclk */
 	bcmgenet_umac_writel(priv, CMD_SW_RESET | CMD_LCL_LOOP_EN, UMAC_CMD);
+	udelay(2);
+	bcmgenet_umac_writel(priv, 0, UMAC_CMD);
 }
 
 static void bcmgenet_intr_disable(struct bcmgenet_priv *priv)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 10244941a7a6..69e80fb6e039 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -181,38 +181,8 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
 	const char *phy_name = NULL;
 	u32 id_mode_dis = 0;
 	u32 port_ctrl;
-	int bmcr = -1;
-	int ret;
 	u32 reg;
 
-	/* MAC clocking workaround during reset of umac state machines */
-	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
-	if (reg & CMD_SW_RESET) {
-		/* An MII PHY must be isolated to prevent TXC contention */
-		if (priv->phy_interface == PHY_INTERFACE_MODE_MII) {
-			ret = phy_read(phydev, MII_BMCR);
-			if (ret >= 0) {
-				bmcr = ret;
-				ret = phy_write(phydev, MII_BMCR,
-						bmcr | BMCR_ISOLATE);
-			}
-			if (ret) {
-				netdev_err(dev, "failed to isolate PHY\n");
-				return ret;
-			}
-		}
-		/* Switch MAC clocking to RGMII generated clock */
-		bcmgenet_sys_writel(priv, PORT_MODE_EXT_GPHY, SYS_PORT_CTRL);
-		/* Ensure 5 clks with Rx disabled
-		 * followed by 5 clks with Reset asserted
-		 */
-		udelay(4);
-		reg &= ~(CMD_SW_RESET | CMD_LCL_LOOP_EN);
-		bcmgenet_umac_writel(priv, reg, UMAC_CMD);
-		/* Ensure 5 more clocks before Rx is enabled */
-		udelay(2);
-	}
-
 	switch (priv->phy_interface) {
 	case PHY_INTERFACE_MODE_INTERNAL:
 		phy_name = "internal PHY";
@@ -282,10 +252,6 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
 
 	bcmgenet_sys_writel(priv, port_ctrl, SYS_PORT_CTRL);
 
-	/* Restore the MII PHY after isolation */
-	if (bmcr >= 0)
-		phy_write(phydev, MII_BMCR, bmcr);
-
 	priv->ext_phy = !priv->internal_phy &&
 			(priv->phy_interface != PHY_INTERFACE_MODE_MOCA);
 
-- 
2.7.4

Re: [PATCH net 1/2] Revert "net: bcmgenet: use RGMII loopback for MAC reset"

[Florian Fainelli]

On 3/16/2020 2:44 PM, Doug Berger wrote:
> This reverts commit 3a55402c93877d291b0a612d25edb03d1b4b93ac.
> 
> This is not a good solution when connecting to an external switch
> that may not support the isolation of the TXC signal resulting in
> output driver contention on the pin.
> 
> A different solution is necessary.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Did you want this to be tagged with:

Fixes: 3a55402c9387 ("net: bcmgenet: use RGMII loopback for MAC reset")

so as to make it more explicit how the two commits relate to each other?

Thanks!
--
Florian

Re: [PATCH net 1/2] Revert "net: bcmgenet: use RGMII loopback for MAC reset"

[Doug Berger]

On 3/16/2020 7:21 PM, Florian Fainelli wrote:
> 
> 
> On 3/16/2020 2:44 PM, Doug Berger wrote:
>> This reverts commit 3a55402c93877d291b0a612d25edb03d1b4b93ac.
>>
>> This is not a good solution when connecting to an external switch
>> that may not support the isolation of the TXC signal resulting in
>> output driver contention on the pin.
>>
>> A different solution is necessary.
>>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
> 
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Did you want this to be tagged with:
> 
> Fixes: 3a55402c9387 ("net: bcmgenet: use RGMII loopback for MAC reset")
> 
> so as to make it more explicit how the two commits relate to each other?
I wasn't sure how best to tag this commit.

It seems odd to indicate that the reversion of a commit fixes the commit
that is reverted, but maybe that is the best way. It is more accurate to
say that the reversion of the commit reintroduces the problem that it
was intended to address, and therefore doesn't fix anything but rather
trades one problem for another.

It is clearer that the second commit fixes the original issue and so I
tagged it accordingly, but it is far less clear how a reversion like
this should be tagged.

If you think it is clearer to tag this as you describe I have no objection.

> 
> Thanks!
> --
> Florian
> 
Thank you for taking a look at this and for the feedback,
    Doug

[PATCH net 2/2] net: bcmgenet: keep MAC in reset until PHY is up

[Doug Berger]

As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
during UniMAC sw_reset") the UniMAC must be clocked at least 5
cycles while the sw_reset is asserted to ensure a clean reset.

That commit enabled local loopback to provide an Rx clock from the
GENET sourced Tx clk. However, when connected in MII mode the Tx
clk is sourced by the PHY so if an EPHY is not supplying clocks
(e.g. when the link is down) the UniMAC does not receive the
necessary clocks.

This commit extends the sw_reset window until the PHY reports that
the link is up thereby ensuring that the clocks are being provided
to the MAC to produce a clean reset.

One consequence is that if the system attempts to enter a Wake on
LAN suspend state when the PHY link has not been active the MAC
may not have had a chance to initialize cleanly. In this case, we
remove the sw_reset and enable the WoL reception path as normal
with the hope that the PHY will provide the necessary clocks to
drive the WoL blocks if the link becomes active after the system
has entered suspend.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 10 ++++------
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c |  6 +++++-
 drivers/net/ethernet/broadcom/genet/bcmmii.c       |  6 ++++++
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index c8ac2d83208f..ce64b4e47feb 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1965,6 +1965,8 @@ static void umac_enable_set(struct bcmgenet_priv *priv, u32 mask, bool enable)
 	u32 reg;
 
 	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
+	if (reg & CMD_SW_RESET)
+		return;
 	if (enable)
 		reg |= mask;
 	else
@@ -1984,13 +1986,9 @@ static void reset_umac(struct bcmgenet_priv *priv)
 	bcmgenet_rbuf_ctrl_set(priv, 0);
 	udelay(10);
 
-	/* disable MAC while updating its registers */
-	bcmgenet_umac_writel(priv, 0, UMAC_CMD);
-
-	/* issue soft reset with (rg)mii loopback to ensure a stable rxclk */
-	bcmgenet_umac_writel(priv, CMD_SW_RESET | CMD_LCL_LOOP_EN, UMAC_CMD);
+	/* issue soft reset and disable MAC while updating its registers */
+	bcmgenet_umac_writel(priv, CMD_SW_RESET, UMAC_CMD);
 	udelay(2);
-	bcmgenet_umac_writel(priv, 0, UMAC_CMD);
 }
 
 static void bcmgenet_intr_disable(struct bcmgenet_priv *priv)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
index ea20d94bd050..c9a43695b182 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
@@ -132,8 +132,12 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 		return -EINVAL;
 	}
 
-	/* disable RX */
+	/* Can't suspend with WoL if MAC is still in reset */
 	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
+	if (reg & CMD_SW_RESET)
+		reg &= ~CMD_SW_RESET;
+
+	/* disable RX */
 	reg &= ~CMD_RX_EN;
 	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
 	mdelay(10);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 69e80fb6e039..b5930f80039d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -95,6 +95,12 @@ void bcmgenet_mii_setup(struct net_device *dev)
 			       CMD_HD_EN |
 			       CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE);
 		reg |= cmd_bits;
+		if (reg & CMD_SW_RESET) {
+			reg &= ~CMD_SW_RESET;
+			bcmgenet_umac_writel(priv, reg, UMAC_CMD);
+			udelay(2);
+			reg |= CMD_TX_EN | CMD_RX_EN;
+		}
 		bcmgenet_umac_writel(priv, reg, UMAC_CMD);
 	} else {
 		/* done if nothing has changed */
-- 
2.7.4

Re: [PATCH net 2/2] net: bcmgenet: keep MAC in reset until PHY is up

[Florian Fainelli]

On 3/16/2020 2:44 PM, Doug Berger wrote:
> As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
> during UniMAC sw_reset") the UniMAC must be clocked at least 5
> cycles while the sw_reset is asserted to ensure a clean reset.
> 
> That commit enabled local loopback to provide an Rx clock from the
> GENET sourced Tx clk. However, when connected in MII mode the Tx
> clk is sourced by the PHY so if an EPHY is not supplying clocks
> (e.g. when the link is down) the UniMAC does not receive the
> necessary clocks.
> 
> This commit extends the sw_reset window until the PHY reports that
> the link is up thereby ensuring that the clocks are being provided
> to the MAC to produce a clean reset.
> 
> One consequence is that if the system attempts to enter a Wake on
> LAN suspend state when the PHY link has not been active the MAC
> may not have had a chance to initialize cleanly. In this case, we
> remove the sw_reset and enable the WoL reception path as normal
> with the hope that the PHY will provide the necessary clocks to
> drive the WoL blocks if the link becomes active after the system
> has entered suspend.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net 0/2] net: bcmgenet: revisit MAC reset

[David Miller]

From: Doug Berger <opendmb@gmail.com>
Date: Mon, 16 Mar 2020 14:44:54 -0700

> Commit 3a55402c9387 ("net: bcmgenet: use RGMII loopback for MAC 
> reset") was intended to resolve issues with reseting the UniMAC
> core within the GENET block by providing better control over the
> clocks used by the UniMAC core. Unfortunately, it is not
> compatible with all of the supported system configurations so an
> alternative method must be applied.
> 
> This commit set provides such an alternative. The first commit
> reverts the previous change and the second commit provides the
> alternative reset sequence that addresses the concerns observed
> with the previous implementation.
> 
> This replacement implementation should be applied to the stable
> branches wherever commit 3a55402c9387 ("net: bcmgenet: use RGMII 
> loopback for MAC reset") has been applied.
> 
> Unfortunately, reverting that commit may conflict with some
> restructuring changes introduced by commit 4f8d81b77e66 ("net: 
> bcmgenet: Refactor register access in bcmgenet_mii_config").
> The first commit in this set has been manually edited to
> resolve the conflict on net/master. I would be happy to help
> stable maintainers with resolving any such conflicts if they
> occur. However, I do not expect that commit to have been
> backported to stable branch so hopefully the revert can be
> applied cleanly.

Series applied and queued up for -stable, thank you.