Hi Roger,

Thanks for your review.

Hi Ken,

On 16/01/2024 22:01, 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. According to various TI 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 and the register bit reserved."

The driver implementation today however, will incorrectly set this bit whenever the interface mode is in RGMII_ID or RGMII_TXID.

To fix this, remove any statement setting this bit, and moreover, mask bits 31:4 as these are all reserved.

See: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/12... 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)

Thanks for the patch. Some comments below.

...

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

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

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 18a33c4c0e..51c432408f 100644 --- a/drivers/net/ti/am65-cpsw-nuss.c +++ b/drivers/net/ti/am65-cpsw-nuss.c @@ -256,7 +256,6 @@ static int am65_cpsw_gmii_sel_k3(struct am65_cpsw_priv *priv, { 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 +295,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 +311,13 @@ static int am65_cpsw_gmii_sel_k3(struct am65_cpsw_priv *priv, break; };

• if (rgmii_id)
  

•         mode |= AM65_GMII_SEL_RGMII_IDMODE;
  

• reg = mode;
  

• /* bits 31:4 are reserved */
  

Updated documentation is still wrong. Reserved bits should be Read only and should always read 0. In updated TRM it doesn't show reserved bits as read-only and states read value is 1.

Bit 4 is clearly read/write so it cannot be treated as reserved. But setting it to 1 is not supported. If bootROM or some other software set it to 1 then it will remain 1 and we cannot rely on it to be always 0. That's why it shouldn't be treated as reserved.

On AM65 bits 31:5 and 3:2 are reserved (read only 0). Bit 4 is still r/w but setting 1 is not supported.

...

• reg = (GENMASK(31, 4) & reg) | mode;
  

As reserved bits will always read 0 this mask is not required. What we do need to do is always clear bit 4.

So #define AM6X_GMII_SEL_MODE_SEL_MASK GENMASK(2:0)

     /* RGMII_ID_MODE = 1 is not supported */
     reg &= ~AM65_GMII_SEL_RGMII_IDMODE;
     reg &= ~AM6X_GMII_SEL_MODE_SEL_MASK;
     reg |= mode;

I like this first approach, as it is clear what is happening and could account for future use of other currently reserved bits in this register.

Or simply this would have worked as we don't really set bit 4 in 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 = GENMASK(3, 0) & readl(gmii_sel);
  

should be GENMASK(2, 0);

You are right thanks

but now we can use AM6X_GMII_SEL_MODE_SEL_MASK here instead.

...

   if (reg != mode) {
           dev_err(dev,
                   "gmii_sel PHY mode NOT SET!: requested: %08x, gmii_sel: %08x\n",

-- cheers, -roger

I will work on a v2, thanks again.

Sincerely, Ken Sloat