| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f68345e-c58d-3d99-189b-b4be39c4b899@gmail.com>
Date: Tue, 5 Nov 2019 11:27:20 -0800
From: Doug Berger <opendmb@...il.com>
To: Scott Branden <scott.branden@...adcom.com>,
"David S. Miller" <davem@...emloft.net>
Cc: Florian Fainelli <f.fainelli@...il.com>,
bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/3] net: bcmgenet: use RGMII loopback for MAC reset
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@...il.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
Powered by blists - more mailing lists