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

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

[Mohammad Athari Bin Ismail]

This patch to implement Marvell PHY specific loopback callback function.
Verified working on Marvell 88E1510 at 1Gbps speed only. For 100Mbps and
10Mbps, found that the PHY loopback not able to function properly.
Possible due to limitation in Marvell 88E1510 PHY.

Tested on Intel Elkhart Lake platform (Synopsys Designware QoS MAC and
Marvell 88E1510 PHY).

Mohammad Athari Bin Ismail (1):
  net: phy: marvell: add Marvell specific PHY loopback

 drivers/net/phy/marvell.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[Mohammad Athari Bin Ismail]

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.

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,
-- 
2.17.1

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

[Heiner Kallweit]

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.


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

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

[Russell King (Oracle)]

On Mon, Jan 10, 2022 at 02:21:16PM +0800, Mohammad Athari Bin Ismail wrote:
> This patch to implement Marvell PHY specific loopback callback function.
> Verified working on Marvell 88E1510 at 1Gbps speed only. For 100Mbps and
> 10Mbps, found that the PHY loopback not able to function properly.
> Possible due to limitation in Marvell 88E1510 PHY.

This is valuable information that should be in the commit message for
the patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Ismail, Mohammad Athari]

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

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,

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

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Monday, January 10, 2022 5:36 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; David S . Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Oleksij
> Rempel <linux@rempel-privat.de>; Heiner Kallweit
> <hkallweit1@gmail.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH net 0/1] net: phy: marvell: add Marvell specific PHY
> loopback
> 
> On Mon, Jan 10, 2022 at 02:21:16PM +0800, Mohammad Athari Bin Ismail
> wrote:
> > This patch to implement Marvell PHY specific loopback callback function.
> > Verified working on Marvell 88E1510 at 1Gbps speed only. For 100Mbps
> > and 10Mbps, found that the PHY loopback not able to function properly.
> > Possible due to limitation in Marvell 88E1510 PHY.
> 
> This is valuable information that should be in the commit message for the
> patch.

Sure. I will add this information in commit message for next version patch.

-Athari-

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Heiner Kallweit]

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

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

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Sent: Monday, January 10, 2022 6:17 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 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.

I get your point. Registers 21_2.6,13 also need to be configured for loopback to work for all speeds.
I'll send the v2 patch later.

-Athari-

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