[PATCH RFT] ravb: Mask PHY mode to avoid inserting delays twice

[PATCH RFT] ravb: Mask PHY mode to avoid inserting delays twice

[Geert Uytterhoeven]

Until recently, the Micrel KSZ9031 PHY driver ignored any PHY mode
("RGMII-*ID") settings, but used the hardware defaults, augmented by
explicit configuration of individual skew values using the "*-skew-ps"
DT properties.  The lack of PHY mode support was compensated by the
EtherAVB MAC driver, which configures TX and/or RX internal delay
itself, based on the PHY mode.

However, now the KSZ9031 driver has gained PHY mode support, delays may
be configured twice, causing regressions.  E.g. on the Renesas
Salvator-X board with R-Car M3-W ES1.0, TX performance dropped from ca.
400 Mbps to 0.1-0.3 Mbps, as measured by nuttcp.

As internal delay configuration supported by the KSZ9031 PHY is too
limited for some use cases, the ability to configure MAC internal delay
is deemed useful and necessary.  Hence a proper fix would involve
splitting internal delay configuration in two parts, one for the PHY,
and one for the MAC.  However, this would require adding new DT
properties, thus breaking DTB backwards-compatibility.

Hence fix the regression in a backwards-compatibility way, by letting
the EtherAVB driver mask the PHY mode when it has inserted a delay, to
avoid the PHY driver adding a second delay.  This also fixes messages
like:

    Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: *-skew-ps values should be used only with phy-mode = "rgmii"

as the PHY no longer sees the original RGMII-*ID mode.

Solving the issue by splitting configuration in two parts can be handled
in future patches, and would require retaining a backwards-compatibility
mode anyway.

Fixes: bcf3440c6dd78bfe ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested on:
  - Salvator-X with R-Car H3 ES1.0 (limited too 100M, hardware erratum),
  - Salvator-X with R-Car M3-W ES1.0,
  - Salvator-XS with R-Car H3 ES2.0,
  - Salvator-XS with R-Car M3-N ES1.0,
  - Ebisu-4D with R-Car E3 ES1.0 (limited to 100M, no MAC TX delay).

Needs testing on:
  - ULCB with various R-Car H3, M3-W, and M3-N SoCs and revisions,
  - HiHope RZ/G2M sub board, using RGMII-TXID,
  - Eagle and V3MSK with R-Car V3M, using RGMII-ID,

Not affected by this patch, but may still be impacted by the micrel
patch, as it changed skew values for all RGMII* modes, not just for
RGMII-*ID modes, so needs testing:
  - Condor with R-Car V3H, using GEther MAC (support for TX/RX internal
    delay not yet implemented) and RGMII-ID.
  - V3HSK with R-Car V3H, using GEther MAC and RGMII,
  - Draak with R-Car D3 using EtherAVB MAC and RGMII (limited to 100M,
    no MAC TX delay).

Reference:
  "Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for
  the KSZ9031 PHY"
  (https://lore.kernel.org/r/CAMuHMdU1ZmSm_tjtWxoFNako2fzmranGVz5qqD2YRNEFRjX0Sw@mail.gmail.com/)
---
 drivers/net/ethernet/renesas/ravb_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 067ad25553b92e43..a442bcf64b9cd875 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1014,6 +1014,7 @@ static int ravb_phy_init(struct net_device *ndev)
 	struct ravb_private *priv = netdev_priv(ndev);
 	struct phy_device *phydev;
 	struct device_node *pn;
+	phy_interface_t iface;
 	int err;
 
 	priv->link = 0;
@@ -1032,8 +1033,13 @@ static int ravb_phy_init(struct net_device *ndev)
 		}
 		pn = of_node_get(np);
 	}
-	phydev = of_phy_connect(ndev, pn, ravb_adjust_link, 0,
-				priv->phy_interface);
+
+	iface = priv->phy_interface;
+	if (priv->chip_id != RCAR_GEN2 && phy_interface_mode_is_rgmii(iface)) {
+		/* ravb_set_delay_mode() takes care of internal delay mode */
+		iface = PHY_INTERFACE_MODE_RGMII;
+	}
+	phydev = of_phy_connect(ndev, pn, ravb_adjust_link, 0, iface);
 	of_node_put(pn);
 	if (!phydev) {
 		netdev_err(ndev, "failed to connect PHY\n");
-- 
2.17.1

Re: [PATCH RFT] ravb: Mask PHY mode to avoid inserting delays twice

[Andrew Lunn]

On Fri, May 29, 2020 at 02:25:40PM +0200, Geert Uytterhoeven wrote:
> Until recently, the Micrel KSZ9031 PHY driver ignored any PHY mode
> ("RGMII-*ID") settings, but used the hardware defaults, augmented by
> explicit configuration of individual skew values using the "*-skew-ps"
> DT properties.  The lack of PHY mode support was compensated by the
> EtherAVB MAC driver, which configures TX and/or RX internal delay
> itself, based on the PHY mode.
> 
> However, now the KSZ9031 driver has gained PHY mode support, delays may
> be configured twice, causing regressions.  E.g. on the Renesas
> Salvator-X board with R-Car M3-W ES1.0, TX performance dropped from ca.
> 400 Mbps to 0.1-0.3 Mbps, as measured by nuttcp.
> 
> As internal delay configuration supported by the KSZ9031 PHY is too
> limited for some use cases, the ability to configure MAC internal delay
> is deemed useful and necessary.  Hence a proper fix would involve
> splitting internal delay configuration in two parts, one for the PHY,
> and one for the MAC.  However, this would require adding new DT
> properties, thus breaking DTB backwards-compatibility.
> 
> Hence fix the regression in a backwards-compatibility way, by letting
> the EtherAVB driver mask the PHY mode when it has inserted a delay, to
> avoid the PHY driver adding a second delay.  This also fixes messages
> like:
> 
>     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: *-skew-ps values should be used only with phy-mode = "rgmii"
> 
> as the PHY no longer sees the original RGMII-*ID mode.
> 
> Solving the issue by splitting configuration in two parts can be handled
> in future patches, and would require retaining a backwards-compatibility
> mode anyway.
> 
> Fixes: bcf3440c6dd78bfe ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFT] ravb: Mask PHY mode to avoid inserting delays twice

[Florian Fainelli]

On 5/29/2020 5:25 AM, Geert Uytterhoeven wrote:
> Until recently, the Micrel KSZ9031 PHY driver ignored any PHY mode
> ("RGMII-*ID") settings, but used the hardware defaults, augmented by
> explicit configuration of individual skew values using the "*-skew-ps"
> DT properties.  The lack of PHY mode support was compensated by the
> EtherAVB MAC driver, which configures TX and/or RX internal delay
> itself, based on the PHY mode.
> 
> However, now the KSZ9031 driver has gained PHY mode support, delays may
> be configured twice, causing regressions.  E.g. on the Renesas
> Salvator-X board with R-Car M3-W ES1.0, TX performance dropped from ca.
> 400 Mbps to 0.1-0.3 Mbps, as measured by nuttcp.
> 
> As internal delay configuration supported by the KSZ9031 PHY is too
> limited for some use cases, the ability to configure MAC internal delay
> is deemed useful and necessary.  Hence a proper fix would involve
> splitting internal delay configuration in two parts, one for the PHY,
> and one for the MAC.  However, this would require adding new DT
> properties, thus breaking DTB backwards-compatibility.
> 
> Hence fix the regression in a backwards-compatibility way, by letting
> the EtherAVB driver mask the PHY mode when it has inserted a delay, to
> avoid the PHY driver adding a second delay.  This also fixes messages
> like:
> 
>     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: *-skew-ps values should be used only with phy-mode = "rgmii"
> 
> as the PHY no longer sees the original RGMII-*ID mode.
> 
> Solving the issue by splitting configuration in two parts can be handled
> in future patches, and would require retaining a backwards-compatibility
> mode anyway.
> 
> Fixes: bcf3440c6dd78bfe ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH RFT] ravb: Mask PHY mode to avoid inserting delays twice

[David Miller]

From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Fri, 29 May 2020 14:25:40 +0200

> Until recently, the Micrel KSZ9031 PHY driver ignored any PHY mode
> ("RGMII-*ID") settings, but used the hardware defaults, augmented by
> explicit configuration of individual skew values using the "*-skew-ps"
> DT properties.  The lack of PHY mode support was compensated by the
> EtherAVB MAC driver, which configures TX and/or RX internal delay
> itself, based on the PHY mode.
> 
> However, now the KSZ9031 driver has gained PHY mode support, delays may
> be configured twice, causing regressions.  E.g. on the Renesas
> Salvator-X board with R-Car M3-W ES1.0, TX performance dropped from ca.
> 400 Mbps to 0.1-0.3 Mbps, as measured by nuttcp.
> 
> As internal delay configuration supported by the KSZ9031 PHY is too
> limited for some use cases, the ability to configure MAC internal delay
> is deemed useful and necessary.  Hence a proper fix would involve
> splitting internal delay configuration in two parts, one for the PHY,
> and one for the MAC.  However, this would require adding new DT
> properties, thus breaking DTB backwards-compatibility.
> 
> Hence fix the regression in a backwards-compatibility way, by letting
> the EtherAVB driver mask the PHY mode when it has inserted a delay, to
> avoid the PHY driver adding a second delay.  This also fixes messages
> like:
> 
>     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: *-skew-ps values should be used only with phy-mode = "rgmii"
> 
> as the PHY no longer sees the original RGMII-*ID mode.
> 
> Solving the issue by splitting configuration in two parts can be handled
> in future patches, and would require retaining a backwards-compatibility
> mode anyway.
> 
> Fixes: bcf3440c6dd78bfe ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied to net-next, thank you.

Re: [PATCH RFT] ravb: Mask PHY mode to avoid inserting delays twice

[Sergei Shtylyov]

Hello!

On 31.05.2020 7:51, David Miller wrote:

>> Until recently, the Micrel KSZ9031 PHY driver ignored any PHY mode
>> ("RGMII-*ID") settings, but used the hardware defaults, augmented by
>> explicit configuration of individual skew values using the "*-skew-ps"
>> DT properties.  The lack of PHY mode support was compensated by the
>> EtherAVB MAC driver, which configures TX and/or RX internal delay
>> itself, based on the PHY mode.
>>
>> However, now the KSZ9031 driver has gained PHY mode support, delays may
>> be configured twice, causing regressions.  E.g. on the Renesas
>> Salvator-X board with R-Car M3-W ES1.0, TX performance dropped from ca.
>> 400 Mbps to 0.1-0.3 Mbps, as measured by nuttcp.
>>
>> As internal delay configuration supported by the KSZ9031 PHY is too
>> limited for some use cases, the ability to configure MAC internal delay
>> is deemed useful and necessary.  Hence a proper fix would involve
>> splitting internal delay configuration in two parts, one for the PHY,
>> and one for the MAC.  However, this would require adding new DT
>> properties, thus breaking DTB backwards-compatibility.
>>
>> Hence fix the regression in a backwards-compatibility way, by letting
>> the EtherAVB driver mask the PHY mode when it has inserted a delay, to
>> avoid the PHY driver adding a second delay.  This also fixes messages
>> like:
>>
>>      Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: *-skew-ps values should be used only with phy-mode = "rgmii"
>>
>> as the PHY no longer sees the original RGMII-*ID mode.
>>
>> Solving the issue by splitting configuration in two parts can be handled
>> in future patches, and would require retaining a backwards-compatibility
>> mode anyway.
>>
>> Fixes: bcf3440c6dd78bfe ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Applied to net-next, thank you.

    Why not to net.git? It's a fix after all...

MBR, Sergei