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

[Doug Berger <opendmb@gmail.com>]

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