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

["Ismail, Mohammad Athari" <mohammad.athari.ismail@intel.com>]

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