[RFC PATCH 0/2] DP83867/DP83869 Ethernet PHY workaround/fix

[RFC PATCH 0/2] DP83867/DP83869 Ethernet PHY workaround/fix

[Siddharth Vadapalli]

Hello,

This series adds a workaround for the DP83867 Ethernet PHY to fix packet
errors observed with short cables, when both ends of the link use the
DP83867 Ethernet PHY. This issue is described in Section 3.8 at [0].

Also, for the DP83869 Ethernet PHY which supports both RGMII and MII
modes of operation, support is added to allow switching to MII mode by
configuring the OP_MODE_DECODE Register in Section 9.6.1.65 at [1].

Regards,
Siddharth.

---
[0]: https://www.ti.com/lit/an/snla246b/snla246b.pdf
[1]: https://www.ti.com/lit/ds/symlink/dp83869hm.pdf

Grygorii Strashko (2):
  net: phy: dp83867: add w/a for packet errors seen with short cables
  net: phy: dp83869: fix mii mode when rgmii strap cfg is used

 drivers/net/phy/dp83867.c | 15 ++++++++++++++-
 drivers/net/phy/dp83869.c |  5 ++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.25.1

[RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen with short cables

[Siddharth Vadapalli]

From: Grygorii Strashko <grygorii.strashko@ti.com>

Introduce the W/A for packet errors seen with short cables (<1m) between
two DP83867 PHYs.

The W/A recommended by DM requires FFE Equalizer Configuration tuning by
writing value 0x0E81 to DSP_FFE_CFG register (0x012C), surrounded by hard
and soft resets as follows:

write_reg(0x001F, 0x8000); //hard reset
write_reg(DSP_FFE_CFG, 0x0E81);
write_reg(0x001F, 0x4000); //soft reset

Since  DP83867 PHY DM says "Changing this register to 0x0E81, will not
affect Long Cable performance.", enable the W/A by default.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/phy/dp83867.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 5821f04c69dc..ba60cf35872e 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -42,6 +42,7 @@
 #define DP83867_STRAP_STS1	0x006E
 #define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
+#define DP83867_DSP_FFE_CFG	0X012C
 #define DP83867_RXFCFG		0x0134
 #define DP83867_RXFPMD1	0x0136
 #define DP83867_RXFPMD2	0x0137
@@ -934,8 +935,20 @@ static int dp83867_phy_reset(struct phy_device *phydev)
 
 	usleep_range(10, 20);
 
-	return phy_modify(phydev, MII_DP83867_PHYCTRL,
+	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
 			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
+	if (err < 0)
+		return err;
+
+	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG, 0X0E81);
+
+	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESTART);
+	if (err < 0)
+		return err;
+
+	usleep_range(10, 20);
+
+	return 0;
 }
 
 static void dp83867_link_change_notify(struct phy_device *phydev)
-- 
2.25.1

Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen with short cables

[Andrew Lunn]

On Tue, Apr 25, 2023 at 11:14:28AM +0530, Siddharth Vadapalli wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Introduce the W/A for packet errors seen with short cables (<1m) between
> two DP83867 PHYs.
> 
> The W/A recommended by DM requires FFE Equalizer Configuration tuning by
> writing value 0x0E81 to DSP_FFE_CFG register (0x012C), surrounded by hard
> and soft resets as follows:
> 
> write_reg(0x001F, 0x8000); //hard reset
> write_reg(DSP_FFE_CFG, 0x0E81);
> write_reg(0x001F, 0x4000); //soft reset
> 
> Since  DP83867 PHY DM says "Changing this register to 0x0E81, will not
> affect Long Cable performance.", enable the W/A by default.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Please set the tree in the Subject line to be net.
Please also add a Fixes: tag, probably for the patch which added this driver.




> ---
>  drivers/net/phy/dp83867.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 5821f04c69dc..ba60cf35872e 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -42,6 +42,7 @@
>  #define DP83867_STRAP_STS1	0x006E
>  #define DP83867_STRAP_STS2	0x006f
>  #define DP83867_RGMIIDCTL	0x0086
> +#define DP83867_DSP_FFE_CFG	0X012C
>  #define DP83867_RXFCFG		0x0134
>  #define DP83867_RXFPMD1	0x0136
>  #define DP83867_RXFPMD2	0x0137
> @@ -934,8 +935,20 @@ static int dp83867_phy_reset(struct phy_device *phydev)
>  
>  	usleep_range(10, 20);
>  
> -	return phy_modify(phydev, MII_DP83867_PHYCTRL,
> +	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
>  			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
> +	if (err < 0)
> +		return err;
> +
> +	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG, 0X0E81);

Maybe check the return code for errors?

      Andrew

Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen with short cables

[Siddharth Vadapalli]

On 25/04/23 17:35, Andrew Lunn wrote:
> On Tue, Apr 25, 2023 at 11:14:28AM +0530, Siddharth Vadapalli wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Introduce the W/A for packet errors seen with short cables (<1m) between
>> two DP83867 PHYs.
>>
>> The W/A recommended by DM requires FFE Equalizer Configuration tuning by
>> writing value 0x0E81 to DSP_FFE_CFG register (0x012C), surrounded by hard
>> and soft resets as follows:
>>
>> write_reg(0x001F, 0x8000); //hard reset
>> write_reg(DSP_FFE_CFG, 0x0E81);
>> write_reg(0x001F, 0x4000); //soft reset
>>
>> Since  DP83867 PHY DM says "Changing this register to 0x0E81, will not
>> affect Long Cable performance.", enable the W/A by default.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> Please set the tree in the Subject line to be net.
> Please also add a Fixes: tag, probably for the patch which added this driver.

I will update the subject to "RFC PATCH net" and also add the "Fixes" tag in the
v2 series.

> 
> 
> 
> 
>> ---
>>  drivers/net/phy/dp83867.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>> index 5821f04c69dc..ba60cf35872e 100644
>> --- a/drivers/net/phy/dp83867.c
>> +++ b/drivers/net/phy/dp83867.c
>> @@ -42,6 +42,7 @@
>>  #define DP83867_STRAP_STS1	0x006E
>>  #define DP83867_STRAP_STS2	0x006f
>>  #define DP83867_RGMIIDCTL	0x0086
>> +#define DP83867_DSP_FFE_CFG	0X012C
>>  #define DP83867_RXFCFG		0x0134
>>  #define DP83867_RXFPMD1	0x0136
>>  #define DP83867_RXFPMD2	0x0137
>> @@ -934,8 +935,20 @@ static int dp83867_phy_reset(struct phy_device *phydev)
>>  
>>  	usleep_range(10, 20);
>>  
>> -	return phy_modify(phydev, MII_DP83867_PHYCTRL,
>> +	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
>>  			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG, 0X0E81);
> 
> Maybe check the return code for errors?

The return value of phy_write_mmd() doesn't have to be checked since it will be
zero for the following reasons:
The dp83867 driver does not have a custom .write_mmd method. Also, the dp83867
phy does not support clause 45. Due to this, within __phy_write_mmd(), the ELSE
statement will be executed, which results in the return value being zero.

> 
>       Andrew

-- 
Regards,
Siddharth.

Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen with short cables

[Andrew Lunn]

> >> @@ -934,8 +935,20 @@ static int dp83867_phy_reset(struct phy_device *phydev)
> >>  
> >>  	usleep_range(10, 20);
> >>  
> >> -	return phy_modify(phydev, MII_DP83867_PHYCTRL,
> >> +	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
> >>  			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
> >> +	if (err < 0)
> >> +		return err;
> >> +
> >> +	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG, 0X0E81);
> > 
> > Maybe check the return code for errors?
> 
> The return value of phy_write_mmd() doesn't have to be checked since it will be
> zero for the following reasons:
> The dp83867 driver does not have a custom .write_mmd method. Also, the dp83867
> phy does not support clause 45. Due to this, within __phy_write_mmd(), the ELSE
> statement will be executed, which results in the return value being zero.

Interesting.

I would actually say __phy_write_mmd() is broken, and should be
returning what __mdiobus_write() returns.

You should assume it will get fixed, and check the return value. And
it does no harm to check the return value.

    Andrew

Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen with short cables

[Siddharth Vadapalli]

On 26/04/23 18:06, Andrew Lunn wrote:
>>>> @@ -934,8 +935,20 @@ static int dp83867_phy_reset(struct phy_device *phydev)
>>>>  
>>>>  	usleep_range(10, 20);
>>>>  
>>>> -	return phy_modify(phydev, MII_DP83867_PHYCTRL,
>>>> +	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
>>>>  			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
>>>> +	if (err < 0)
>>>> +		return err;
>>>> +
>>>> +	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG, 0X0E81);
>>>
>>> Maybe check the return code for errors?
>>
>> The return value of phy_write_mmd() doesn't have to be checked since it will be
>> zero for the following reasons:
>> The dp83867 driver does not have a custom .write_mmd method. Also, the dp83867
>> phy does not support clause 45. Due to this, within __phy_write_mmd(), the ELSE
>> statement will be executed, which results in the return value being zero.
> 
> Interesting.
> 
> I would actually say __phy_write_mmd() is broken, and should be
> returning what __mdiobus_write() returns.
> 
> You should assume it will get fixed, and check the return value. And
> it does no harm to check the return value.

Thank you for clarifying. The reasoning behind the initial patch not checking
the return value was:
At all invocations of phy_write_mmd() in the dp83867 driver, at no place is the
return value checked, which led me to analyze why that was the case. I noticed
that it was due to the return value being guaranteed to be zero for this
particular driver.

Since the existing __phy_write_mmd() implementation is broken as pointed out by
you, I will update this patch to check the return value. Also, I will probably
add a cleanup patch as well, to fix this at all other invocations of
phy_write_mmd() in the driver.

-- 
Regards,
Siddharth.

[RFC PATCH 2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

[Siddharth Vadapalli]

From: Grygorii Strashko <grygorii.strashko@ti.com>

The DP83869 PHY on TI's k3-am642-evm supports both MII and RGMII
interfaces and is configured by default to use RGMII interface (strap).
However, the board design allows switching dynamically to MII interface
for testing purposes by applying different set of pinmuxes.

To support switching to MII interface, update the DP83869 PHY driver to
configure OP_MODE_DECODE.RGMII_MII_SEL(bit 5) properly when MII PHY
interface mode is requested.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/phy/dp83869.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 9ab5eff502b7..8dbc502bcd9e 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 	/* Below init sequence for each operational mode is defined in
 	 * section 9.4.8 of the datasheet.
 	 */
+	phy_ctrl_val = dp83869->mode;
+	if (phydev->interface == PHY_INTERFACE_MODE_MII)
+		phy_ctrl_val |= DP83869_OP_MODE_MII;
 	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
-			    dp83869->mode);
+			    phy_ctrl_val);
 	if (ret)
 		return ret;
 
-- 
2.25.1

Re: [RFC PATCH 2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

[Andrew Lunn]

On Tue, Apr 25, 2023 at 11:14:29AM +0530, Siddharth Vadapalli wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> The DP83869 PHY on TI's k3-am642-evm supports both MII and RGMII
> interfaces and is configured by default to use RGMII interface (strap).
> However, the board design allows switching dynamically to MII interface
> for testing purposes by applying different set of pinmuxes.

Only for testing? So nobody should actually design a board to use MII
and use software to change the interface from RGMII to MII?

This does not seem to be a fix, it is a new feature. So please submit
to net-next, in two weeks time when it opens again.

> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>  	/* Below init sequence for each operational mode is defined in
>  	 * section 9.4.8 of the datasheet.
>  	 */
> +	phy_ctrl_val = dp83869->mode;
> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
> +		phy_ctrl_val |= DP83869_OP_MODE_MII;

Should there be some validation here with dp83869->mode?

DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
DP83869_RGMII_100_BASE seem to be the only valid modes with MII?

	Andrew

Re: [RFC PATCH 2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

[Siddharth Vadapalli]

On 25/04/23 17:48, Andrew Lunn wrote:
> On Tue, Apr 25, 2023 at 11:14:29AM +0530, Siddharth Vadapalli wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> The DP83869 PHY on TI's k3-am642-evm supports both MII and RGMII
>> interfaces and is configured by default to use RGMII interface (strap).
>> However, the board design allows switching dynamically to MII interface
>> for testing purposes by applying different set of pinmuxes.
> 
> Only for testing? So nobody should actually design a board to use MII
> and use software to change the interface from RGMII to MII?
> 
> This does not seem to be a fix, it is a new feature. So please submit
> to net-next, in two weeks time when it opens again.

Sure. I will split this patch from the series and post the v2 of this patch with
the subject "RFC PATCH net-next" for requesting further feedback on this patch
if needed. Following that, I will post it to net-next as a new patch.

> 
>> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>  	/* Below init sequence for each operational mode is defined in
>>  	 * section 9.4.8 of the datasheet.
>>  	 */
>> +	phy_ctrl_val = dp83869->mode;
>> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
>> +		phy_ctrl_val |= DP83869_OP_MODE_MII;
> 
> Should there be some validation here with dp83869->mode?
> 
> DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
> make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
> DP83869_RGMII_100_BASE seem to be the only valid modes with MII?

The DP83869_OP_MODE_MII macro corresponds to BIT(5) which is the RGMII_MII_SEL
bit in the OP_MODE_DECODE register. If the RGMII_MII_SEL bit is set, MII mode is
selected. If the bit is cleared, which is the default value, RGMII mode is
selected. As pointed out by you, there are modes which aren't valid with MII
mode. However, a mode which isn't valid with RGMII mode (default value of the
RGMII_MII_SEL bit) also exists: DP83869_SGMII_COPPER_ETHERNET. For this reason,
I believe that setting the bit when MII mode is requested shouldn't cause any
issues.

> 
> 	Andrew

-- 
Regards,
Siddharth.

Re: [RFC PATCH 2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

[Andrew Lunn]

> >> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
> >>  	/* Below init sequence for each operational mode is defined in
> >>  	 * section 9.4.8 of the datasheet.
> >>  	 */
> >> +	phy_ctrl_val = dp83869->mode;
> >> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
> >> +		phy_ctrl_val |= DP83869_OP_MODE_MII;
> > 
> > Should there be some validation here with dp83869->mode?
> > 
> > DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
> > make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
> > DP83869_RGMII_100_BASE seem to be the only valid modes with MII?
> 
> The DP83869_OP_MODE_MII macro corresponds to BIT(5) which is the RGMII_MII_SEL
> bit in the OP_MODE_DECODE register. If the RGMII_MII_SEL bit is set, MII mode is
> selected. If the bit is cleared, which is the default value, RGMII mode is
> selected. As pointed out by you, there are modes which aren't valid with MII
> mode. However, a mode which isn't valid with RGMII mode (default value of the
> RGMII_MII_SEL bit) also exists: DP83869_SGMII_COPPER_ETHERNET. For this reason,
> I believe that setting the bit when MII mode is requested shouldn't cause any
> issues.

If you say so. I was just thinking you could give the poor software
engineer a hint the hardware engineer has put on strapping resistors
which means the PHY is not going to work.

      Andrew

Re: [RFC PATCH 2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

[Siddharth Vadapalli]

On 26/04/23 18:11, Andrew Lunn wrote:
>>>> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>>>  	/* Below init sequence for each operational mode is defined in
>>>>  	 * section 9.4.8 of the datasheet.
>>>>  	 */
>>>> +	phy_ctrl_val = dp83869->mode;
>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
>>>> +		phy_ctrl_val |= DP83869_OP_MODE_MII;
>>>
>>> Should there be some validation here with dp83869->mode?
>>>
>>> DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
>>> make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
>>> DP83869_RGMII_100_BASE seem to be the only valid modes with MII?
>>
>> The DP83869_OP_MODE_MII macro corresponds to BIT(5) which is the RGMII_MII_SEL
>> bit in the OP_MODE_DECODE register. If the RGMII_MII_SEL bit is set, MII mode is
>> selected. If the bit is cleared, which is the default value, RGMII mode is
>> selected. As pointed out by you, there are modes which aren't valid with MII
>> mode. However, a mode which isn't valid with RGMII mode (default value of the
>> RGMII_MII_SEL bit) also exists: DP83869_SGMII_COPPER_ETHERNET. For this reason,
>> I believe that setting the bit when MII mode is requested shouldn't cause any
>> issues.
> 
> If you say so. I was just thinking you could give the poor software
> engineer a hint the hardware engineer has put on strapping resistors
> which means the PHY is not going to work.

I understand now. I will update this patch to add a print if the MII mode is not
valid with the configured "dp83869->mode". Would you suggest using a dev_err()
or a dev_dbg()?

Thank you for the feedback on this series.

-- 
Regards,
Siddharth.

Re: [RFC PATCH 2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

[Andrew Lunn]

> > If you say so. I was just thinking you could give the poor software
> > engineer a hint the hardware engineer has put on strapping resistors
> > which means the PHY is not going to work.
> 
> I understand now. I will update this patch to add a print if the MII mode is not
> valid with the configured "dp83869->mode". Would you suggest using a dev_err()
> or a dev_dbg()?

dev_err(). And you can return -EINVAL when asked to set the interface
mode.

	Andrew