Re: [PATCH net 1/1] net: phy: marvell: add Marvell specific PHY loopback

[Heiner Kallweit <hkallweit1@gmail.com>]

On 10.01.2022 10:36, Ismail, Mohammad Athari wrote:
> 
> 
>> -----Original Message-----
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Sent: Monday, January 10, 2022 4:34 PM
>> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>;
>> Andrew Lunn <andrew@lunn.ch>; David S . Miller <davem@davemloft.net>;
>> Jakub Kicinski <kuba@kernel.org>; Oleksij Rempel <linux@rempel-
>> privat.de>; Russell King <linux@armlinux.org.uk>
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> stable@vger.kernel.org
>> Subject: Re: [PATCH net 1/1] net: phy: marvell: add Marvell specific PHY
>> loopback
>>
>> On 10.01.2022 07:21, Mohammad Athari Bin Ismail wrote:
>>> Existing genphy_loopback() is not applicable for Marvell PHY. So,
>>> adding Marvell specific PHY loopback operation by only setting(enable)
>>> or
>>> clearing(disable) BMCR_LOOPBACK bit.
>>>
>>> Tested working on Marvell 88E1510.
>>>
>> With this change you'd basically revert the original change and loose its
>> functionality. Did you check the Marvell datasheets?
>> At least for few versions I found that you may have to configure bits 0..2 in
>> MAC Specific Control Register 2 (page 2, register 21) instead of BMCR.
> 
> May I know what datasheet version that has the bits 2:0's detail explanation? The version that I have, bits 2:0 in MAC Specific Control Register 2 shows as Reserved.
> The datasheet I have is "Marvell Alaska 88E1510/88E1518/88E1512/88E1514 Integrated 10/100/1000 Mbps Energy Efficient Ethernet Transceiver Rev. G December 17, 2021"
> 
I checked the 88E6352 switch chip datasheet. The part covering the integrated PHY's lists the mentioned bits
in MAC Specific Control Register 2.

Table 75 in the 88E1510 datasheet says: Loopback speed is determined by Registers 21_2.6,13.
So Marvell PHY's seem to use different bits (although same register) for loopback speed configuration.

> Really appreciate if you could advice on PHY loopback enabling for Marvell 88E1510 because the existing genphy_loopback() function doesn't work for the PHY.
> 
> Thank you.
> 
> -Athari-
> 
>>
>>
>>> Fixes: 014068dcb5b1 ("net: phy: genphy_loopback: add link speed
>>> configuration")
>>> Cc: <stable@vger.kernel.org> # 5.15.x
>>> Signed-off-by: Mohammad Athari Bin Ismail
>>> <mohammad.athari.ismail@intel.com>
>>> ---
>>>  drivers/net/phy/marvell.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>>> index 4fcfca4e1702..2a73a959b48b 100644
>>> --- a/drivers/net/phy/marvell.c
>>> +++ b/drivers/net/phy/marvell.c
>>> @@ -1932,6 +1932,12 @@ static void marvell_get_stats(struct phy_device
>> *phydev,
>>>  		data[i] = marvell_get_stat(phydev, i);  }
>>>
>>> +static int marvell_loopback(struct phy_device *phydev, bool enable) {
>>> +	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>>> +			  enable ? BMCR_LOOPBACK : 0);
>>> +}
>>> +
>>>  static int marvell_vct5_wait_complete(struct phy_device *phydev)  {
>>>  	int i;
>>> @@ -3078,7 +3084,7 @@ static struct phy_driver marvell_drivers[] = {
>>>  		.get_sset_count = marvell_get_sset_count,
>>>  		.get_strings = marvell_get_strings,
>>>  		.get_stats = marvell_get_stats,
>>> -		.set_loopback = genphy_loopback,
>>> +		.set_loopback = marvell_loopback,
>>>  		.get_tunable = m88e1011_get_tunable,
>>>  		.set_tunable = m88e1011_set_tunable,
>>>  		.cable_test_start = marvell_vct7_cable_test_start,
>