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

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

[Ken Sloat]

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)

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 */
+	reg = (GENMASK(31, 4) & 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);
 	if (reg != mode) {
 		dev_err(dev,
 			"gmii_sel PHY mode NOT SET!: requested: %08x, gmii_sel: %08x\n",
-- 
2.34.1

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

[Roger Quadros]

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@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

Re: [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@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