diff mbox series

[v2,1/1] net: ti: am65-cpsw-nuss: Remove incorrect RGMII_ID bit functionality

Message ID AS8PR08MB79915B184330438FBFE664DAEC432@AS8PR08MB7991.eurprd08.prod.outlook.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series [v2,1/1] net: ti: am65-cpsw-nuss: Remove incorrect RGMII_ID bit functionality | expand

Commit Message

Ken Sloat Feb. 1, 2024, 8:17 p.m. UTC 
The CPSW implementations on the AM6x platforms do not support the
selectable RGMII TX clk delay functionality via the RGMII_ID_MODE bit as
the earlier platforms did. While it is possible to write the bit,
according to various TI AM6x datasheets, reference manuals, hardware
design guides and TI forum posts from TI, this bit is "not timed, tested
or characterized. RGMII_ID is enabled by default."

The driver implementation today however, will incorrectly set this bit
whenever the interface mode is in RGMII_ID or RGMII_TXID. Since
disabling the delay (bit=1) is not supported by TI, this bit should
always be written as 0.

See:
https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1211306/am625-enet1_ctrl_rgmii_id-_mode
https://www.ti.com/lit/pdf/spruiv7 (Rev. B Figure 14-1717)
https://www.ti.com/lit/gpn/am625 (Rev. B Figure 7-31 Note A)
https://www.ti.com/lit/an/sprad05b/sprad05b.pdf (Rev. B Section 7.4)

Signed-off-by: Ken Sloat <ken.s@variscite.com>
---

Notes:
    Changes in v2:
    -Explicitly clear RGMII_ID bit in all cases
    -Clarify commit message based on items in v1 review

 drivers/net/ti/am65-cpsw-nuss.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.34.1

Comments

Roger Quadros Feb. 2, 2024, 12:29 p.m. UTC | #1 
On 01/02/2024 22:17, Ken Sloat wrote:
> The CPSW implementations on the AM6x platforms do not support the
> selectable RGMII TX clk delay functionality via the RGMII_ID_MODE bit as
> the earlier platforms did. While it is possible to write the bit,
> according to various TI AM6x datasheets, reference manuals, hardware
> design guides and TI forum posts from TI, this bit is "not timed, tested
> or characterized. RGMII_ID is enabled by default."
> 
> The driver implementation today however, will incorrectly set this bit
> whenever the interface mode is in RGMII_ID or RGMII_TXID. Since
> disabling the delay (bit=1) is not supported by TI, this bit should
> always be written as 0.
> 
> See:
> https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1211306/am625-enet1_ctrl_rgmii_id-_mode
> https://www.ti.com/lit/pdf/spruiv7 (Rev. B Figure 14-1717)
> https://www.ti.com/lit/gpn/am625 (Rev. B Figure 7-31 Note A)
> https://www.ti.com/lit/an/sprad05b/sprad05b.pdf (Rev. B Section 7.4)
> 
> Signed-off-by: Ken Sloat <ken.s@variscite.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>
Tom Rini March 1, 2024, 2:17 p.m. UTC | #2 
On Thu, Feb 01, 2024 at 08:17:05PM +0000, Ken Sloat wrote:

> The CPSW implementations on the AM6x platforms do not support the
> selectable RGMII TX clk delay functionality via the RGMII_ID_MODE bit as
> the earlier platforms did. While it is possible to write the bit,
> according to various TI AM6x datasheets, reference manuals, hardware
> design guides and TI forum posts from TI, this bit is "not timed, tested
> or characterized. RGMII_ID is enabled by default."
> 
> The driver implementation today however, will incorrectly set this bit
> whenever the interface mode is in RGMII_ID or RGMII_TXID. Since
> disabling the delay (bit=1) is not supported by TI, this bit should
> always be written as 0.
> 
> See:
> https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1211306/am625-enet1_ctrl_rgmii_id-_mode
> https://www.ti.com/lit/pdf/spruiv7 (Rev. B Figure 14-1717)
> https://www.ti.com/lit/gpn/am625 (Rev. B Figure 7-31 Note A)
> https://www.ti.com/lit/an/sprad05b/sprad05b.pdf (Rev. B Section 7.4)
> 
> Signed-off-by: Ken Sloat <ken.s@variscite.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>

The whitespace in this version was totally destroyed, please re-send,
thanks.
diff mbox series

Patch

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 6da018c0f9..0836da1583 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -250,13 +250,13 @@  out:
 #define AM65_GMII_SEL_MODE_SGMII   3
 
 #define AM65_GMII_SEL_RGMII_IDMODE BIT(4)
+#define AM6X_GMII_SEL_MODE_SEL_MASK      GENMASK(2, 0)
 
 static int am65_cpsw_gmii_sel_k3(struct am65_cpsw_priv *priv,
                         phy_interface_t phy_mode)
 {
      struct udevice *dev = priv->dev;
      u32 offset, reg, phandle;
-     bool rgmii_id = false;
      fdt_addr_t gmii_sel;
      u32 mode = 0;
      ofnode node;
@@ -296,7 +296,6 @@  static int am65_cpsw_gmii_sel_k3(struct am65_cpsw_priv *priv,
      case PHY_INTERFACE_MODE_RGMII_ID:
      case PHY_INTERFACE_MODE_RGMII_TXID:
            mode = AM65_GMII_SEL_MODE_RGMII;
-           rgmii_id = true;
            break;
 
      case PHY_INTERFACE_MODE_SGMII:
@@ -313,15 +312,16 @@  static int am65_cpsw_gmii_sel_k3(struct am65_cpsw_priv *priv,
            break;
      };
 
-     if (rgmii_id)
-           mode |= AM65_GMII_SEL_RGMII_IDMODE;
+     /* RGMII_ID_MODE = 1 is not supported */
+     reg &= ~AM65_GMII_SEL_RGMII_IDMODE;
+     reg &= ~AM6X_GMII_SEL_MODE_SEL_MASK;
+     reg |= mode;
 
-     reg = mode;
      dev_dbg(dev, "gmii_sel PHY mode: %u, new gmii_sel: %08x\n",
            phy_mode, reg);
      writel(reg, gmii_sel);
 
-     reg = readl(gmii_sel);
+     reg = AM6X_GMII_SEL_MODE_SEL_MASK & readl(gmii_sel);
      if (reg != mode) {
            dev_err(dev,
                  "gmii_sel PHY mode NOT SET!: requested: %08x, gmii_sel: %08x\n",