From: Grygorii Strashko <[email protected]>
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.
Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
V1 patch at:
https://lore.kernel.org/r/[email protected]
Changes since v1 patch:
- Wrap the line invoking phy_write_mmd(), limiting it to 80 characters.
- Replace 0X0E81 with 0x0e81 in the call to phy_write_mmd().
- Replace 0X012C with 0x012c in the new define for DP83867_DSP_FFE_CFG.
RFC patch at:
https://lore.kernel.org/r/[email protected]/
Changes since RFC patch:
- Change patch subject to PATCH net.
- Add Fixes tag.
- Check return value of phy_write_mmd().
drivers/net/phy/dp83867.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index d75f526a20a4..bbdcc595715d 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -44,6 +44,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
@@ -941,8 +942,23 @@ 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;
+
+ err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
+ 0x0e81);
+ if (err < 0)
+ return err;
+
+ 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
On 09.05.2023 07:21, Siddharth Vadapalli wrote:
> From: Grygorii Strashko <[email protected]>
>
> 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.
>
> Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
> Signed-off-by: Grygorii Strashko <[email protected]>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
>
> V1 patch at:
> https://lore.kernel.org/r/[email protected]
>
> Changes since v1 patch:
> - Wrap the line invoking phy_write_mmd(), limiting it to 80 characters.
> - Replace 0X0E81 with 0x0e81 in the call to phy_write_mmd().
> - Replace 0X012C with 0x012c in the new define for DP83867_DSP_FFE_CFG.
>
> RFC patch at:
> https://lore.kernel.org/r/[email protected]/
>
> Changes since RFC patch:
> - Change patch subject to PATCH net.
> - Add Fixes tag.
> - Check return value of phy_write_mmd().
>
> drivers/net/phy/dp83867.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index d75f526a20a4..bbdcc595715d 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -44,6 +44,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
> @@ -941,8 +942,23 @@ 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;
> +
Would be good to add a comment here explaining what this magic write does.
> + err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
> + 0x0e81);
> + if (err < 0)
> + return err;
> +
> + 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)
On 09/05/23 14:29, Heiner Kallweit wrote:
> On 09.05.2023 07:21, Siddharth Vadapalli wrote:
>> From: Grygorii Strashko <[email protected]>
>>
>> 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.
>>
>> Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>> ---
>>
>> V1 patch at:
>> https://lore.kernel.org/r/[email protected]
>>
>> Changes since v1 patch:
>> - Wrap the line invoking phy_write_mmd(), limiting it to 80 characters.
>> - Replace 0X0E81 with 0x0e81 in the call to phy_write_mmd().
>> - Replace 0X012C with 0x012c in the new define for DP83867_DSP_FFE_CFG.
>>
>> RFC patch at:
>> https://lore.kernel.org/r/[email protected]/
>>
>> Changes since RFC patch:
>> - Change patch subject to PATCH net.
>> - Add Fixes tag.
>> - Check return value of phy_write_mmd().
>>
>> drivers/net/phy/dp83867.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>> index d75f526a20a4..bbdcc595715d 100644
>> --- a/drivers/net/phy/dp83867.c
>> +++ b/drivers/net/phy/dp83867.c
>> @@ -44,6 +44,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
>> @@ -941,8 +942,23 @@ 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;
>> +
>
> Would be good to add a comment here explaining what this magic write does.
Sure. Is the following comment acceptable?
"Configure the DSP Feedforward Equalizer Configuration register to improve short
cable (< 1 meter) performance. This will not affect long cable performance."
>
>> + err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
>> + 0x0e81);
>> + if (err < 0)
>> + return err;
>> +
>> + 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)
>
--
Regards,
Siddharth.
On 09.05.2023 12:27, Siddharth Vadapalli wrote:
>
>
> On 09/05/23 14:29, Heiner Kallweit wrote:
>> On 09.05.2023 07:21, Siddharth Vadapalli wrote:
>>> From: Grygorii Strashko <[email protected]>
>>>
>>> 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.
>>>
>>> Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>>> ---
>>>
>>> V1 patch at:
>>> https://lore.kernel.org/r/[email protected]
>>>
>>> Changes since v1 patch:
>>> - Wrap the line invoking phy_write_mmd(), limiting it to 80 characters.
>>> - Replace 0X0E81 with 0x0e81 in the call to phy_write_mmd().
>>> - Replace 0X012C with 0x012c in the new define for DP83867_DSP_FFE_CFG.
>>>
>>> RFC patch at:
>>> https://lore.kernel.org/r/[email protected]/
>>>
>>> Changes since RFC patch:
>>> - Change patch subject to PATCH net.
>>> - Add Fixes tag.
>>> - Check return value of phy_write_mmd().
>>>
>>> drivers/net/phy/dp83867.c | 18 +++++++++++++++++-
>>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>> index d75f526a20a4..bbdcc595715d 100644
>>> --- a/drivers/net/phy/dp83867.c
>>> +++ b/drivers/net/phy/dp83867.c
>>> @@ -44,6 +44,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
>>> @@ -941,8 +942,23 @@ 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;
>>> +
>>
>> Would be good to add a comment here explaining what this magic write does.
>
> Sure. Is the following comment acceptable?
>
> "Configure the DSP Feedforward Equalizer Configuration register to improve short
> cable (< 1 meter) performance. This will not affect long cable performance."
>
Sounds good. Important is just that the magic value write is explained, so that
readers of the source code don't have to scroll through the commit history.
>>
>>> + err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
>>> + 0x0e81);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + 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)
>>
>
On 09-05-2023 16:10, Heiner Kallweit wrote:
> On 09.05.2023 12:27, Siddharth Vadapalli wrote:
>>
>>
>> On 09/05/23 14:29, Heiner Kallweit wrote:
>>> On 09.05.2023 07:21, Siddharth Vadapalli wrote:
>>>> From: Grygorii Strashko <[email protected]>
>>>>
>>>> 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.
>>>>
>>>> Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
>>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>>>> ---
>>>>
>>>> V1 patch at:
>>>> https://lore.kernel.org/r/[email protected]
>>>>
>>>> Changes since v1 patch:
>>>> - Wrap the line invoking phy_write_mmd(), limiting it to 80 characters.
>>>> - Replace 0X0E81 with 0x0e81 in the call to phy_write_mmd().
>>>> - Replace 0X012C with 0x012c in the new define for DP83867_DSP_FFE_CFG.
>>>>
>>>> RFC patch at:
>>>> https://lore.kernel.org/r/[email protected]/
>>>>
>>>> Changes since RFC patch:
>>>> - Change patch subject to PATCH net.
>>>> - Add Fixes tag.
>>>> - Check return value of phy_write_mmd().
>>>>
>>>> drivers/net/phy/dp83867.c | 18 +++++++++++++++++-
>>>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>> index d75f526a20a4..bbdcc595715d 100644
>>>> --- a/drivers/net/phy/dp83867.c
>>>> +++ b/drivers/net/phy/dp83867.c
>>>> @@ -44,6 +44,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
>>>> @@ -941,8 +942,23 @@ 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;
>>>> +
>>>
>>> Would be good to add a comment here explaining what this magic write does.
>>
>> Sure. Is the following comment acceptable?
>>
>> "Configure the DSP Feedforward Equalizer Configuration register to improve short
>> cable (< 1 meter) performance. This will not affect long cable performance."
>>
> Sounds good. Important is just that the magic value write is explained, so that
> readers of the source code don't have to scroll through the commit history.
Thank you for letting me know. I will post the v3 patch with the comment.
>
>>>
>>>> + err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
>>>> + 0x0e81);
>>>> + if (err < 0)
>>>> + return err;
>>>> +
>>>> + 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)
>>>
>>
>
--
Regards,
Siddharth.