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

[Ken Sloat]

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/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 <ken.s at 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