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@xxxxxxxxx>
>> ---
>> Â 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