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/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) >
Thanks for the patch. Some comments below. > Signed-off-by: Ken Sloat <ke...@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; 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); 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