2022-01-10 06:22:11

by Ismail, Mohammad Athari

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

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



2022-01-10 06:22:12

by Ismail, Mohammad Athari

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

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: <[email protected]> # 5.15.x
Signed-off-by: Mohammad Athari Bin Ismail <[email protected]>
---
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


2022-01-10 08:35:25

by Heiner Kallweit

[permalink] [raw]
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.


> Fixes: 014068dcb5b1 ("net: phy: genphy_loopback: add link speed configuration")
> Cc: <[email protected]> # 5.15.x
> Signed-off-by: Mohammad Athari Bin Ismail <[email protected]>
> ---
> 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,


2022-01-10 09:35:48

by Russell King (Oracle)

[permalink] [raw]
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.

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

2022-01-10 09:38:40

by Ismail, Mohammad Athari

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



> -----Original Message-----
> From: Heiner Kallweit <[email protected]>
> Sent: Monday, January 10, 2022 4:34 PM
> To: Ismail, Mohammad Athari <[email protected]>;
> Andrew Lunn <[email protected]>; David S . Miller <[email protected]>;
> Jakub Kicinski <[email protected]>; Oleksij Rempel <linux@rempel-
> privat.de>; Russell King <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> 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: <[email protected]> # 5.15.x
> > Signed-off-by: Mohammad Athari Bin Ismail
> > <[email protected]>
> > ---
> > 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,

2022-01-10 09:48:00

by Ismail, Mohammad Athari

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



> -----Original Message-----
> From: Russell King <[email protected]>
> Sent: Monday, January 10, 2022 5:36 PM
> To: Ismail, Mohammad Athari <[email protected]>
> Cc: Andrew Lunn <[email protected]>; David S . Miller
> <[email protected]>; Jakub Kicinski <[email protected]>; Oleksij
> Rempel <[email protected]>; Heiner Kallweit
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> 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!

2022-01-10 10:17:27

by Heiner Kallweit

[permalink] [raw]
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 <[email protected]>
>> Sent: Monday, January 10, 2022 4:34 PM
>> To: Ismail, Mohammad Athari <[email protected]>;
>> Andrew Lunn <[email protected]>; David S . Miller <[email protected]>;
>> Jakub Kicinski <[email protected]>; Oleksij Rempel <linux@rempel-
>> privat.de>; Russell King <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]
>> 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: <[email protected]> # 5.15.x
>>> Signed-off-by: Mohammad Athari Bin Ismail
>>> <[email protected]>
>>> ---
>>> 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,
>


2022-01-12 09:30:33

by Ismail, Mohammad Athari

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



> -----Original Message-----
> From: Heiner Kallweit <[email protected]>
> Sent: Monday, January 10, 2022 6:17 PM
> To: Ismail, Mohammad Athari <[email protected]>;
> Andrew Lunn <[email protected]>; David S . Miller <[email protected]>;
> Jakub Kicinski <[email protected]>; Oleksij Rempel <linux@rempel-
> privat.de>; Russell King <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> >> Sent: Monday, January 10, 2022 4:34 PM
> >> To: Ismail, Mohammad Athari <[email protected]>;
> >> Andrew Lunn <[email protected]>; David S . Miller
> <[email protected]>;
> >> Jakub Kicinski <[email protected]>; Oleksij Rempel <linux@rempel-
> >> privat.de>; Russell King <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> [email protected]
> >> 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: <[email protected]> # 5.15.x
> >>> Signed-off-by: Mohammad Athari Bin Ismail
> >>> <[email protected]>
> >>> ---
> >>> 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,
> >