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

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> Sent: Monday, January 10, 2022 6:17 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>;
> Andrew Lunn <andrew@xxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> Jakub Kicinski <kuba@xxxxxxxxxx>; Oleksij Rempel <linux@rempel-
> privat.de>; Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> >> Sent: Monday, January 10, 2022 4:34 PM
> >> To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>;
> >> Andrew Lunn <andrew@xxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>;
> >> Jakub Kicinski <kuba@xxxxxxxxxx>; Oleksij Rempel <linux@rempel-
> >> privat.de>; Russell King <linux@xxxxxxxxxxxxxxx>
> >> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> stable@xxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxxxxxxx> # 5.15.x
> >>> Signed-off-by: Mohammad Athari Bin Ismail
> >>> <mohammad.athari.ismail@xxxxxxxxx>
> >>> ---
> >>>  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,
> >