1. Patchew
  2. linux
  3. View series

[PATCH] phy: ti: gmii-sel: Force RGMII TX delay

Michael Walle posted 1 patch 2 months ago 
drivers/phy/ti/phy-gmii-sel.c | 52 +++++++++++++++++++++++++++++------
1 file changed, 44 insertions(+), 8 deletions(-)
[PATCH] phy: ti: gmii-sel: Force RGMII TX delay
Posted by Michael Walle 2 months ago 
Some SoCs are just validated with the TX delay enabled. With commit
ca13b249f291 ("net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed
RGMII TX delay"), the network driver will patch the delay setting on the
fly assuming that the TX delay is fixed. In reality, the TX delay is
configurable and just skipped in the documentation. There are
bootloaders, which will disable the TX delay and this will lead to a
transmit path which doesn't add any delays at all. Fix that by always
forcing the TX delay to be enabled.

Fixes: ca13b249f291 ("net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay")
Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/phy/ti/phy-gmii-sel.c | 52 +++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index ff5d5e29629f..a0c19d00ff3a 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -34,6 +34,7 @@ enum {
 	PHY_GMII_SEL_PORT_MODE = 0,
 	PHY_GMII_SEL_RGMII_ID_MODE,
 	PHY_GMII_SEL_RMII_IO_CLK_EN,
+	PHY_GMII_SEL_FIXED_TX_DELAY,
 	PHY_GMII_SEL_LAST,
 };
 
@@ -127,6 +128,16 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
 		goto unsupported;
 	}
 
+	/*
+	 * Some SoCs only support fixed MAC side TX delays. According to the
+	 * datasheet, they are always enabled, but that turns out not to be the
+	 * case and the delay is configurable. But according to the vendor that
+	 * mode is not validated and might not work. Some bootloaders disable
+	 * that bit. To work around that enable it again.
+	 */
+	if (soc_data->features & BIT(PHY_GMII_SEL_FIXED_TX_DELAY))
+		rgmii_id = 0;
+
 	if_phy->phy_if_mode = submode;
 
 	dev_dbg(dev, "%s id:%u mode:%u rgmii_id:%d rmii_clk_ext:%d\n",
@@ -210,25 +221,46 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_dm814 = {
 
 static const
 struct reg_field phy_gmii_sel_fields_am654[][PHY_GMII_SEL_LAST] = {
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x0, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x4, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x8, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0xC, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x10, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x14, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x18, 0, 2), },
-	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x1C, 0, 2), },
+	{
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x0, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x0, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x4, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x4, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x8, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x8, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0xC, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0xC, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x10, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x10, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x14, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x14, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x18, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x18, 4, 4),
+	}, {
+		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x1C, 0, 2),
+		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x1C, 4, 4),
+	},
 };
 
 static const
 struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
 	.use_of_data = true,
+	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
+		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
 	.regfields = phy_gmii_sel_fields_am654,
 };
 
 static const
 struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
 	.use_of_data = true,
+	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
+		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
 	.regfields = phy_gmii_sel_fields_am654,
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII) |
 		       BIT(PHY_INTERFACE_MODE_USXGMII),
@@ -239,6 +271,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
 static const
 struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = {
 	.use_of_data = true,
+	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
+		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
 	.regfields = phy_gmii_sel_fields_am654,
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
 	.num_ports = 8,
@@ -248,6 +282,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = {
 static const
 struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j784s4 = {
 	.use_of_data = true,
+	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
+		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
 	.regfields = phy_gmii_sel_fields_am654,
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII) |
 		       BIT(PHY_INTERFACE_MODE_USXGMII),
-- 
2.39.5
Re: [PATCH] phy: ti: gmii-sel: Force RGMII TX delay
Posted by Matthias Schiffer 2 months ago 
On Mon, 2025-08-04 at 16:06 +0200, Michael Walle wrote:
> Some SoCs are just validated with the TX delay enabled. With commit
> ca13b249f291 ("net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed
> RGMII TX delay"), the network driver will patch the delay setting on the
> fly assuming that the TX delay is fixed. In reality, the TX delay is
> configurable and just skipped in the documentation. There are
> bootloaders, which will disable the TX delay and this will lead to a
> transmit path which doesn't add any delays at all. Fix that by always
> forcing the TX delay to be enabled.
> 
> Fixes: ca13b249f291 ("net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed RGMII TX delay")
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 52 +++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index ff5d5e29629f..a0c19d00ff3a 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -34,6 +34,7 @@ enum {
>  	PHY_GMII_SEL_PORT_MODE = 0,
>  	PHY_GMII_SEL_RGMII_ID_MODE,
>  	PHY_GMII_SEL_RMII_IO_CLK_EN,
> +	PHY_GMII_SEL_FIXED_TX_DELAY,
>  	PHY_GMII_SEL_LAST,
>  };
>  
> @@ -127,6 +128,16 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>  		goto unsupported;
>  	}
>  
> +	/*
> +	 * Some SoCs only support fixed MAC side TX delays. According to the
> +	 * datasheet, they are always enabled, but that turns out not to be the
> +	 * case and the delay is configurable. But according to the vendor that
> +	 * mode is not validated and might not work. Some bootloaders disable
> +	 * that bit. To work around that enable it again.
> +	 */
> +	if (soc_data->features & BIT(PHY_GMII_SEL_FIXED_TX_DELAY))
> +		rgmii_id = 0;

The mode passed to phy_gmii_sel_mode by am65-cpsw is the same value that is
passed to the Ethernet PHY driver, so a case where rgmii_id == 1 and would be
reset to 0 is unreachable (PHY_INTERFACE_MODE_RGMII_ID and
PHY_INTERFACE_MODE_RGMII_TXID can't occur with the fixup in am65-cpsw).
Therefore we could return EINVAL here if PHY_GMII_SEL_FIXED_TX_DELAY is set and
rgmii_id == 1 instead of effectively ignoring the mode, which seems a bit more
robust to me.

Best,
Matthias


> +
>  	if_phy->phy_if_mode = submode;
>  
>  	dev_dbg(dev, "%s id:%u mode:%u rgmii_id:%d rmii_clk_ext:%d\n",
> @@ -210,25 +221,46 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_dm814 = {
>  
>  static const
>  struct reg_field phy_gmii_sel_fields_am654[][PHY_GMII_SEL_LAST] = {
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x0, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x4, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x8, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0xC, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x10, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x14, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x18, 0, 2), },
> -	{ [PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x1C, 0, 2), },
> +	{
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x0, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x0, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x4, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x4, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x8, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x8, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0xC, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0xC, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x10, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x10, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x14, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x14, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x18, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x18, 4, 4),
> +	}, {
> +		[PHY_GMII_SEL_PORT_MODE] = REG_FIELD(0x1C, 0, 2),
> +		[PHY_GMII_SEL_RGMII_ID_MODE] = REG_FIELD(0x1C, 4, 4),
> +	},
>  };
>  
>  static const
>  struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
>  	.use_of_data = true,
> +	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
> +		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
>  	.regfields = phy_gmii_sel_fields_am654,
>  };
>  
>  static const
>  struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>  	.use_of_data = true,
> +	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
> +		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
>  	.regfields = phy_gmii_sel_fields_am654,
>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII) |
>  		       BIT(PHY_INTERFACE_MODE_USXGMII),
> @@ -239,6 +271,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>  static const
>  struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = {
>  	.use_of_data = true,
> +	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
> +		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
>  	.regfields = phy_gmii_sel_fields_am654,
>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
>  	.num_ports = 8,
> @@ -248,6 +282,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = {
>  static const
>  struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j784s4 = {
>  	.use_of_data = true,
> +	.features = BIT(PHY_GMII_SEL_RGMII_ID_MODE) |
> +		    BIT(PHY_GMII_SEL_FIXED_TX_DELAY),
>  	.regfields = phy_gmii_sel_fields_am654,
>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII) |
>  		       BIT(PHY_INTERFACE_MODE_USXGMII),

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
Re: [PATCH] phy: ti: gmii-sel: Force RGMII TX delay
Posted by Andrew Lunn 2 months ago 
On Mon, Aug 04, 2025 at 04:06:52PM +0200, Michael Walle wrote:
> Some SoCs are just validated with the TX delay enabled. With commit
> ca13b249f291 ("net: ethernet: ti: am65-cpsw: fixup PHY mode for fixed
> RGMII TX delay"), the network driver will patch the delay setting on the
> fly assuming that the TX delay is fixed. In reality, the TX delay is
> configurable and just skipped in the documentation. There are
> bootloaders, which will disable the TX delay and this will lead to a
> transmit path which doesn't add any delays at all. Fix that by always
> forcing the TX delay to be enabled.

Please could you add a paragraph:

    This is safe to do, and will not break any existing boards
    supported in mainline because...

We have to be careful of regressions, and such a paragraph makes it
clear you have thought it through, and what your assumptions are. If
something does break, listing your assumptions will help finding what
went wrong.

   Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.