[PATCH net 0/3] net: bcmgenet: restore internal EPHY support (part 2)

[PATCH net 0/3] net: bcmgenet: restore internal EPHY support (part 2)

[Doug Berger]

This is a follow up to my previous submission (see [1]).

The first commit provides what is intended to be a complete solution
for the issues that can result from insufficient clocking of the MAC
during reset of its state machines. It should be backported to the
stable releases.

It is intended to replace the partial solution of commit 1f515486275a 
("net: bcmgenet: soft reset 40nm EPHYs before MAC init") which is
reverted by the second commit of this series and should not be back-
ported as noted in [2].

The third commit corrects a timing hazard with a polled PHY that can
occur when the MAC resumes and also when a v3 internal EPHY is reset
by the change in commit 25382b991d25 ("net: bcmgenet: reset 40nm EPHY 
on energy detect"). It is expected that commit 25382b991d25 be back-
ported to stable first before backporting this commit.

[1] https://lkml.org/lkml/2019/10/16/1706
[2] https://lkml.org/lkml/2019/10/31/749

Doug Berger (3):
  net: bcmgenet: use RGMII loopback for MAC reset
  Revert "net: bcmgenet: soft reset 40nm EPHYs before MAC init"
  net: bcmgenet: reapply manual settings to the PHY

 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  35 +++---
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   2 +-
 drivers/net/ethernet/broadcom/genet/bcmmii.c   | 145 ++++++++++++++++---------
 3 files changed, 110 insertions(+), 72 deletions(-)

-- 
2.7.4

[PATCH net 1/3] net: bcmgenet: use RGMII loopback for MAC reset

[Doug Berger]

As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
during UniMAC sw_reset") the UniMAC must be clocked while sw_reset
is asserted for its state machines to reset cleanly.

The transmit and receive clocks used by the UniMAC are derived from
the signals used on its PHY interface. The bcmgenet MAC can be
configured to work with different PHY interfaces including MII,
GMII, RGMII, and Reverse MII on internal and external interfaces.
Unfortunately for the UniMAC, when configured for MII the Tx clock
is always driven from the PHY which places it outside of the direct
control of the MAC.

The earlier commit enabled a local loopback mode within the UniMAC
so that the receive clock would be derived from the transmit clock
which addressed the observed issue with an external GPHY disabling
it's Rx clock. However, when a Tx clock is not available this
loopback is insufficient.

This commit implements a workaround that leverages the fact that
the MAC can reliably generate all of its necessary clocking by
enterring the external GPHY RGMII interface mode with the UniMAC in
local loopback during the sw_reset interval. Unfortunately, this
has the undesirable side efect of the RGMII GTXCLK signal being
driven during the same window.

In most configurations this is a benign side effect as the signal
is either not routed to a pin or is already expected to drive the
pin. The one exception is when an external MII PHY is expected to
drive the same pin with its TX_CLK output creating output driver
contention.

This commit exploits the IEEE 802.3 clause 22 standard defined
isolate mode to force an external MII PHY to present a high
impedance on its TX_CLK output during the window to prevent any
contention at the pin.

The MII interface is used internally with the 40nm internal EPHY
which agressively disables its clocks for power savings leading to
incomplete resets of the UniMAC and many instabilities observed
over the years. The workaround of this commit is expected to put
an end to those problems.

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

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0f138280315a..a1776ed8d7a1 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1996,8 +1996,6 @@ 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 17bb8d60a157..fcd181ae3a7d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -221,8 +221,38 @@ 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);
+	}
+
 	priv->ext_phy = !priv->internal_phy &&
 			(priv->phy_interface != PHY_INTERFACE_MODE_MOCA);
 
@@ -254,6 +284,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
 		phy_set_max_speed(phydev, SPEED_100);
 		bcmgenet_sys_writel(priv,
 				    PORT_MODE_EXT_EPHY, SYS_PORT_CTRL);
+		/* Restore the MII PHY after isolation */
+		if (bmcr >= 0)
+			phy_write(phydev, MII_BMCR, bmcr);
 		break;
 
 	case PHY_INTERFACE_MODE_REVMII:
-- 
2.7.4

[PATCH net 2/3] Revert "net: bcmgenet: soft reset 40nm EPHYs before MAC init"

[Doug Berger]

This reverts commit 1f515486275a08a17a2c806b844cca18f7de5b34.

This commit improved the chances of the umac resetting cleanly by
ensuring that the PHY was restored to its normal operation prior
to resetting the umac. However, there were still cases when the
PHY might not be driving a Tx clock to the umac during this window
(e.g. when the PHY detects no link).

The previous commit now ensures that the unimac receives clocks
from the MAC during its reset window so this commit is no longer
needed. This commit also has an unintended negative impact on the
MDIO performance of the UniMAC MDIO interface because it is used
before the MDIO interrupts are reenabled, so it should be removed.

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

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index a1776ed8d7a1..b5255dd08265 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2877,12 +2877,6 @@ static int bcmgenet_open(struct net_device *dev)
 	if (priv->internal_phy)
 		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
 
-	ret = bcmgenet_mii_connect(dev);
-	if (ret) {
-		netdev_err(dev, "failed to connect to PHY\n");
-		goto err_clk_disable;
-	}
-
 	/* take MAC out of reset */
 	bcmgenet_umac_reset(priv);
 
@@ -2892,12 +2886,6 @@ static int bcmgenet_open(struct net_device *dev)
 	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
 	priv->crc_fwd_en = !!(reg & CMD_CRC_FWD);
 
-	ret = bcmgenet_mii_config(dev, true);
-	if (ret) {
-		netdev_err(dev, "unsupported PHY\n");
-		goto err_disconnect_phy;
-	}
-
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
 
 	if (priv->internal_phy) {
@@ -2913,7 +2901,7 @@ static int bcmgenet_open(struct net_device *dev)
 	ret = bcmgenet_init_dma(priv);
 	if (ret) {
 		netdev_err(dev, "failed to initialize DMA\n");
-		goto err_disconnect_phy;
+		goto err_clk_disable;
 	}
 
 	/* Always enable ring 16 - descriptor ring */
@@ -2936,19 +2924,25 @@ static int bcmgenet_open(struct net_device *dev)
 		goto err_irq0;
 	}
 
+	ret = bcmgenet_mii_probe(dev);
+	if (ret) {
+		netdev_err(dev, "failed to connect to PHY\n");
+		goto err_irq1;
+	}
+
 	bcmgenet_netif_start(dev);
 
 	netif_tx_start_all_queues(dev);
 
 	return 0;
 
+err_irq1:
+	free_irq(priv->irq1, priv);
 err_irq0:
 	free_irq(priv->irq0, priv);
 err_fini_dma:
 	bcmgenet_dma_teardown(priv);
 	bcmgenet_fini_dma(priv);
-err_disconnect_phy:
-	phy_disconnect(dev->phydev);
 err_clk_disable:
 	if (priv->internal_phy)
 		bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
@@ -3629,8 +3623,6 @@ static int bcmgenet_resume(struct device *d)
 	if (priv->internal_phy)
 		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
 
-	phy_init_hw(dev->phydev);
-
 	bcmgenet_umac_reset(priv);
 
 	init_umac(priv);
@@ -3639,6 +3631,8 @@ static int bcmgenet_resume(struct device *d)
 	if (priv->wolopts)
 		clk_disable_unprepare(priv->clk_wol);
 
+	phy_init_hw(dev->phydev);
+
 	/* Speed settings must be restored */
 	bcmgenet_mii_config(priv->dev, false);
 
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 7fbf573d8d52..dbc69d8fa05f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -720,8 +720,8 @@ GENET_IO_MACRO(rbuf, GENET_RBUF_OFF);
 
 /* MDIO routines */
 int bcmgenet_mii_init(struct net_device *dev);
-int bcmgenet_mii_connect(struct net_device *dev);
 int bcmgenet_mii_config(struct net_device *dev, bool init);
+int bcmgenet_mii_probe(struct net_device *dev);
 void bcmgenet_mii_exit(struct net_device *dev);
 void bcmgenet_phy_power_set(struct net_device *dev, bool enable);
 void bcmgenet_mii_setup(struct net_device *dev);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index fcd181ae3a7d..dbe18cdf6c1b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -173,46 +173,6 @@ static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
 					  bcmgenet_fixed_phy_link_update);
 }
 
-int bcmgenet_mii_connect(struct net_device *dev)
-{
-	struct bcmgenet_priv *priv = netdev_priv(dev);
-	struct device_node *dn = priv->pdev->dev.of_node;
-	struct phy_device *phydev;
-	u32 phy_flags = 0;
-	int ret;
-
-	/* Communicate the integrated PHY revision */
-	if (priv->internal_phy)
-		phy_flags = priv->gphy_rev;
-
-	/* Initialize link state variables that bcmgenet_mii_setup() uses */
-	priv->old_link = -1;
-	priv->old_speed = -1;
-	priv->old_duplex = -1;
-	priv->old_pause = -1;
-
-	if (dn) {
-		phydev = of_phy_connect(dev, priv->phy_dn, bcmgenet_mii_setup,
-					phy_flags, priv->phy_interface);
-		if (!phydev) {
-			pr_err("could not attach to PHY\n");
-			return -ENODEV;
-		}
-	} else {
-		phydev = dev->phydev;
-		phydev->dev_flags = phy_flags;
-
-		ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup,
-					 priv->phy_interface);
-		if (ret) {
-			pr_err("could not attach to PHY\n");
-			return -ENODEV;
-		}
-	}
-
-	return 0;
-}
-
 int bcmgenet_mii_config(struct net_device *dev, bool init)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
@@ -339,21 +299,71 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
 		bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
 	}
 
-	if (init) {
-		linkmode_copy(phydev->advertising, phydev->supported);
+	if (init)
+		dev_info(kdev, "configuring instance for %s\n", phy_name);
 
-		/* The internal PHY has its link interrupts routed to the
-		 * Ethernet MAC ISRs. On GENETv5 there is a hardware issue
-		 * that prevents the signaling of link UP interrupts when
-		 * the link operates at 10Mbps, so fallback to polling for
-		 * those versions of GENET.
-		 */
-		if (priv->internal_phy && !GENET_IS_V5(priv))
-			phydev->irq = PHY_IGNORE_INTERRUPT;
+	return 0;
+}
 
-		dev_info(kdev, "configuring instance for %s\n", phy_name);
+int bcmgenet_mii_probe(struct net_device *dev)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	struct device_node *dn = priv->pdev->dev.of_node;
+	struct phy_device *phydev;
+	u32 phy_flags = 0;
+	int ret;
+
+	/* Communicate the integrated PHY revision */
+	if (priv->internal_phy)
+		phy_flags = priv->gphy_rev;
+
+	/* Initialize link state variables that bcmgenet_mii_setup() uses */
+	priv->old_link = -1;
+	priv->old_speed = -1;
+	priv->old_duplex = -1;
+	priv->old_pause = -1;
+
+	if (dn) {
+		phydev = of_phy_connect(dev, priv->phy_dn, bcmgenet_mii_setup,
+					phy_flags, priv->phy_interface);
+		if (!phydev) {
+			pr_err("could not attach to PHY\n");
+			return -ENODEV;
+		}
+	} else {
+		phydev = dev->phydev;
+		phydev->dev_flags = phy_flags;
+
+		ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup,
+					 priv->phy_interface);
+		if (ret) {
+			pr_err("could not attach to PHY\n");
+			return -ENODEV;
+		}
+	}
+
+	/* Configure port multiplexer based on what the probed PHY device since
+	 * reading the 'max-speed' property determines the maximum supported
+	 * PHY speed which is needed for bcmgenet_mii_config() to configure
+	 * things appropriately.
+	 */
+	ret = bcmgenet_mii_config(dev, true);
+	if (ret) {
+		phy_disconnect(dev->phydev);
+		return ret;
 	}
 
+	linkmode_copy(phydev->advertising, phydev->supported);
+
+	/* The internal PHY has its link interrupts routed to the
+	 * Ethernet MAC ISRs. On GENETv5 there is a hardware issue
+	 * that prevents the signaling of link UP interrupts when
+	 * the link operates at 10Mbps, so fallback to polling for
+	 * those versions of GENET.
+	 */
+	if (priv->internal_phy && !GENET_IS_V5(priv))
+		dev->phydev->irq = PHY_IGNORE_INTERRUPT;
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH net 3/3] net: bcmgenet: reapply manual settings to the PHY

[Doug Berger]

The phy_init_hw() function may reset the PHY to a configuration
that does not match manual network settings stored in the phydev
structure. If the phy state machine is polled rather than event
driven this can create a timing hazard where the phy state machine
might alter the settings stored in the phydev structure from the
value read from the BMCR.

This commit follows invocations of phy_init_hw() by the bcmgenet
driver with invocations of the genphy_config_aneg() function to
ensure that the BMCR is written to match the settings held in the
phydev structure. This prevents the risk of manual settings being
accidentally altered.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index b5255dd08265..1de51811fcb4 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2612,8 +2612,10 @@ static void bcmgenet_irq_task(struct work_struct *work)
 	spin_unlock_irq(&priv->lock);
 
 	if (status & UMAC_IRQ_PHY_DET_R &&
-	    priv->dev->phydev->autoneg != AUTONEG_ENABLE)
+	    priv->dev->phydev->autoneg != AUTONEG_ENABLE) {
 		phy_init_hw(priv->dev->phydev);
+		genphy_config_aneg(priv->dev->phydev);
+	}
 
 	/* Link UP/DOWN event */
 	if (status & UMAC_IRQ_LINK_EVENT)
@@ -3634,6 +3636,7 @@ static int bcmgenet_resume(struct device *d)
 	phy_init_hw(dev->phydev);
 
 	/* Speed settings must be restored */
+	genphy_config_aneg(dev->phydev);
 	bcmgenet_mii_config(priv->dev, false);
 
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
-- 
2.7.4

Re: [PATCH net 1/3] net: bcmgenet: use RGMII loopback for MAC reset

[Scott Branden]

Hi Doug,

On 2019-11-05 11:07 a.m., Doug Berger wrote:
> As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
> during UniMAC sw_reset") the UniMAC must be clocked while sw_reset
> is asserted for its state machines to reset cleanly.
>
> The transmit and receive clocks used by the UniMAC are derived from
> the signals used on its PHY interface. The bcmgenet MAC can be
> configured to work with different PHY interfaces including MII,
> GMII, RGMII, and Reverse MII on internal and external interfaces.
> Unfortunately for the UniMAC, when configured for MII the Tx clock
> is always driven from the PHY which places it outside of the direct
> control of the MAC.
>
> The earlier commit enabled a local loopback mode within the UniMAC
> so that the receive clock would be derived from the transmit clock
> which addressed the observed issue with an external GPHY disabling
> it's Rx clock. However, when a Tx clock is not available this
> loopback is insufficient.
>
> This commit implements a workaround that leverages the fact that
> the MAC can reliably generate all of its necessary clocking by
> enterring the external GPHY RGMII interface mode with the UniMAC in
> local loopback during the sw_reset interval. Unfortunately, this
> has the undesirable side efect of the RGMII GTXCLK signal being
> driven during the same window.
>
> In most configurations this is a benign side effect as the signal
> is either not routed to a pin or is already expected to drive the
> pin. The one exception is when an external MII PHY is expected to
> drive the same pin with its TX_CLK output creating output driver
> contention.
>
> This commit exploits the IEEE 802.3 clause 22 standard defined
> isolate mode to force an external MII PHY to present a high
> impedance on its TX_CLK output during the window to prevent any
> contention at the pin.
>
> The MII interface is used internally with the 40nm internal EPHY
> which agressively disables its clocks for power savings leading to
> incomplete resets of the UniMAC and many instabilities observed
> over the years. The workaround of this commit is expected to put
> an end to those problems.
>
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c |  2 --
>   drivers/net/ethernet/broadcom/genet/bcmmii.c   | 33 ++++++++++++++++++++++++++
>   2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 0f138280315a..a1776ed8d7a1 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1996,8 +1996,6 @@ 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 17bb8d60a157..fcd181ae3a7d 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -221,8 +221,38 @@ 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);
How do these magic delays work, they are different values?
In one case you have a udelay(4) to ensure rx disabled for 5 clks.
Yet below you have a udelay(2) to ensure 4 more clocks?
> +		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);
> +	}
> +
>   	priv->ext_phy = !priv->internal_phy &&
>   			(priv->phy_interface != PHY_INTERFACE_MODE_MOCA);
>   
> @@ -254,6 +284,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
>   		phy_set_max_speed(phydev, SPEED_100);
>   		bcmgenet_sys_writel(priv,
>   				    PORT_MODE_EXT_EPHY, SYS_PORT_CTRL);
> +		/* Restore the MII PHY after isolation */
> +		if (bmcr >= 0)
> +			phy_write(phydev, MII_BMCR, bmcr);
>   		break;
>   
>   	case PHY_INTERFACE_MODE_REVMII:

Re: [PATCH net 1/3] net: bcmgenet: use RGMII loopback for MAC reset

[Doug Berger]

On 11/5/19 11:14 AM, Scott Branden wrote:
> Hi Doug,
> 
> On 2019-11-05 11:07 a.m., Doug Berger wrote:
>> As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
>> during UniMAC sw_reset") the UniMAC must be clocked while sw_reset
>> is asserted for its state machines to reset cleanly.
>>
>> The transmit and receive clocks used by the UniMAC are derived from
>> the signals used on its PHY interface. The bcmgenet MAC can be
>> configured to work with different PHY interfaces including MII,
>> GMII, RGMII, and Reverse MII on internal and external interfaces.
>> Unfortunately for the UniMAC, when configured for MII the Tx clock
>> is always driven from the PHY which places it outside of the direct
>> control of the MAC.
>>
>> The earlier commit enabled a local loopback mode within the UniMAC
>> so that the receive clock would be derived from the transmit clock
>> which addressed the observed issue with an external GPHY disabling
>> it's Rx clock. However, when a Tx clock is not available this
>> loopback is insufficient.
>>
>> This commit implements a workaround that leverages the fact that
>> the MAC can reliably generate all of its necessary clocking by
>> enterring the external GPHY RGMII interface mode with the UniMAC in
>> local loopback during the sw_reset interval. Unfortunately, this
>> has the undesirable side efect of the RGMII GTXCLK signal being
>> driven during the same window.
>>
>> In most configurations this is a benign side effect as the signal
>> is either not routed to a pin or is already expected to drive the
>> pin. The one exception is when an external MII PHY is expected to
>> drive the same pin with its TX_CLK output creating output driver
>> contention.
>>
>> This commit exploits the IEEE 802.3 clause 22 standard defined
>> isolate mode to force an external MII PHY to present a high
>> impedance on its TX_CLK output during the window to prevent any
>> contention at the pin.
>>
>> The MII interface is used internally with the 40nm internal EPHY
>> which agressively disables its clocks for power savings leading to
>> incomplete resets of the UniMAC and many instabilities observed
>> over the years. The workaround of this commit is expected to put
>> an end to those problems.
>>
>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>> ---
>>   drivers/net/ethernet/broadcom/genet/bcmgenet.c |  2 --
>>   drivers/net/ethernet/broadcom/genet/bcmmii.c   | 33
>> ++++++++++++++++++++++++++
>>   2 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 0f138280315a..a1776ed8d7a1 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -1996,8 +1996,6 @@ 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 17bb8d60a157..fcd181ae3a7d 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -221,8 +221,38 @@ 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);
> How do these magic delays work, they are different values?
> In one case you have a udelay(4) to ensure rx disabled for 5 clks.
> Yet below you have a udelay(2) to ensure 4 more clocks?
The delays are based on 2.5MHz clock cycles (the clock used for 10Mbps).
5 clocks is 2us.

The udelay(4) is for 10 clocks: rx is disabled for 5 and then 5 more
clocks with reset held. The requirement is poorly specified and this is
a conservative interpretation.

The udelay(2) allows at least 5 more clocks without reset before rx can
be enabled.

>> +        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);
>> +    }
>> +
>>       priv->ext_phy = !priv->internal_phy &&
>>               (priv->phy_interface != PHY_INTERFACE_MODE_MOCA);
>>   @@ -254,6 +284,9 @@ int bcmgenet_mii_config(struct net_device *dev,
>> bool init)
>>           phy_set_max_speed(phydev, SPEED_100);
>>           bcmgenet_sys_writel(priv,
>>                       PORT_MODE_EXT_EPHY, SYS_PORT_CTRL);
>> +        /* Restore the MII PHY after isolation */
>> +        if (bmcr >= 0)
>> +            phy_write(phydev, MII_BMCR, bmcr);
>>           break;
>>         case PHY_INTERFACE_MODE_REVMII:
> 

Regards,
    Doug

Re: [PATCH net 1/3] net: bcmgenet: use RGMII loopback for MAC reset

[Scott Branden]

On 2019-11-05 11:27 a.m., Doug Berger wrote:
> On 11/5/19 11:14 AM, Scott Branden wrote:
>> Hi Doug,
>>
>> On 2019-11-05 11:07 a.m., Doug Berger wrote:
>>> As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
>>> during UniMAC sw_reset") the UniMAC must be clocked while sw_reset
>>> is asserted for its state machines to reset cleanly.
>>>
>>> The transmit and receive clocks used by the UniMAC are derived from
>>> the signals used on its PHY interface. The bcmgenet MAC can be
>>> configured to work with different PHY interfaces including MII,
>>> GMII, RGMII, and Reverse MII on internal and external interfaces.
>>> Unfortunately for the UniMAC, when configured for MII the Tx clock
>>> is always driven from the PHY which places it outside of the direct
>>> control of the MAC.
-- SNIP
>>> +        /* 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);
>> How do these magic delays work, they are different values?
>> In one case you have a udelay(4) to ensure rx disabled for 5 clks.
>> Yet below you have a udelay(2) to ensure 4 more clocks?
> The delays are based on 2.5MHz clock cycles (the clock used for 10Mbps).
> 5 clocks is 2us.
>
> The udelay(4) is for 10 clocks: rx is disabled for 5 and then 5 more
> clocks with reset held. The requirement is poorly specified and this is
> a conservative interpretation.
>
> The udelay(2) allows at least 5 more clocks without reset before rx can
> be enabled.
>
Thanks, the part I was missing was "2.5MHz clock cycles (the clock used 
for 10Mbps)".
If that was added to the comment it would help those unfamiliar with in 
understanding.
>>> +        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);
>>> +    }
>>> +
>>>        priv->ext_phy = !priv->internal_phy &&
>>>                (priv->phy_interface != PHY_INTERFACE_MODE_MOCA);
>>>    @@ -254,6 +284,9 @@ int bcmgenet_mii_config(struct net_device *dev,
>>> bool init)
>>>            phy_set_max_speed(phydev, SPEED_100);
>>>            bcmgenet_sys_writel(priv,
>>>                        PORT_MODE_EXT_EPHY, SYS_PORT_CTRL);
>>> +        /* Restore the MII PHY after isolation */
>>> +        if (bmcr >= 0)
>>> +            phy_write(phydev, MII_BMCR, bmcr);
>>>            break;
>>>          case PHY_INTERFACE_MODE_REVMII:
> Regards,
>      Doug

Re: [PATCH net 1/3] net: bcmgenet: use RGMII loopback for MAC reset

[Florian Fainelli]

On 11/5/2019 11:07 AM, Doug Berger wrote:
> As noted in commit 28c2d1a7a0bf ("net: bcmgenet: enable loopback
> during UniMAC sw_reset") the UniMAC must be clocked while sw_reset
> is asserted for its state machines to reset cleanly.
> 
> The transmit and receive clocks used by the UniMAC are derived from
> the signals used on its PHY interface. The bcmgenet MAC can be
> configured to work with different PHY interfaces including MII,
> GMII, RGMII, and Reverse MII on internal and external interfaces.
> Unfortunately for the UniMAC, when configured for MII the Tx clock
> is always driven from the PHY which places it outside of the direct
> control of the MAC.
> 
> The earlier commit enabled a local loopback mode within the UniMAC
> so that the receive clock would be derived from the transmit clock
> which addressed the observed issue with an external GPHY disabling
> it's Rx clock. However, when a Tx clock is not available this
> loopback is insufficient.
> 
> This commit implements a workaround that leverages the fact that
> the MAC can reliably generate all of its necessary clocking by
> enterring the external GPHY RGMII interface mode with the UniMAC in
> local loopback during the sw_reset interval. Unfortunately, this
> has the undesirable side efect of the RGMII GTXCLK signal being
> driven during the same window.
> 
> In most configurations this is a benign side effect as the signal
> is either not routed to a pin or is already expected to drive the
> pin. The one exception is when an external MII PHY is expected to
> drive the same pin with its TX_CLK output creating output driver
> contention.
> 
> This commit exploits the IEEE 802.3 clause 22 standard defined
> isolate mode to force an external MII PHY to present a high
> impedance on its TX_CLK output during the window to prevent any
> contention at the pin.
> 
> The MII interface is used internally with the 40nm internal EPHY
> which agressively disables its clocks for power savings leading to
> incomplete resets of the UniMAC and many instabilities observed
> over the years. The workaround of this commit is expected to put
> an end to those problems.
> 
> 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 2/3] Revert "net: bcmgenet: soft reset 40nm EPHYs before MAC init"

[Florian Fainelli]

On 11/5/2019 11:07 AM, Doug Berger wrote:
> This reverts commit 1f515486275a08a17a2c806b844cca18f7de5b34.
> 
> This commit improved the chances of the umac resetting cleanly by
> ensuring that the PHY was restored to its normal operation prior
> to resetting the umac. However, there were still cases when the
> PHY might not be driving a Tx clock to the umac during this window
> (e.g. when the PHY detects no link).
> 
> The previous commit now ensures that the unimac receives clocks
> from the MAC during its reset window so this commit is no longer
> needed. This commit also has an unintended negative impact on the
> MDIO performance of the UniMAC MDIO interface because it is used
> before the MDIO interrupts are reenabled, so it should be removed.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net 3/3] net: bcmgenet: reapply manual settings to the PHY

[Florian Fainelli]

On 11/5/2019 11:07 AM, Doug Berger wrote:
> The phy_init_hw() function may reset the PHY to a configuration
> that does not match manual network settings stored in the phydev
> structure. If the phy state machine is polled rather than event
> driven this can create a timing hazard where the phy state machine
> might alter the settings stored in the phydev structure from the
> value read from the BMCR.
> 
> This commit follows invocations of phy_init_hw() by the bcmgenet
> driver with invocations of the genphy_config_aneg() function to
> ensure that the BMCR is written to match the settings held in the
> phydev structure. This prevents the risk of manual settings being
> accidentally altered.
> 
> 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/3] net: bcmgenet: restore internal EPHY support (part 2)

[David Miller]

From: Doug Berger <opendmb@gmail.com>
Date: Tue,  5 Nov 2019 11:07:23 -0800

> This is a follow up to my previous submission (see [1]).

Series applied.