[PATCH] net: phy: micrel: Fix detection of ksz87xx switch

[PATCH] net: phy: micrel: Fix detection of ksz87xx switch

[Steve Bennett]

The previous logic was wrong such that the ksz87xx
switch was not identified correctly.

Signed-off-by: Steve Bennett <steveb@workware.net.au>
---
 drivers/net/phy/micrel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 4d53886f7d51..a4acec02c8cb 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -401,11 +401,11 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
 }
 
 static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
-					    const u32 ksz_phy_id)
+					    const u32 ksz_8051)
 {
 	int ret;
 
-	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != ksz_phy_id)
+	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
 		return 0;
 
 	ret = phy_read(phydev, MII_BMSR);
@@ -418,7 +418,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
 	 * the switch does not.
 	 */
 	ret &= BMSR_ERCAP;
-	if (ksz_phy_id == PHY_ID_KSZ8051)
+	if (ksz_8051)
 		return ret;
 	else
 		return !ret;
@@ -426,7 +426,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
 
 static int ksz8051_match_phy_device(struct phy_device *phydev)
 {
-	return ksz8051_ksz8795_match_phy_device(phydev, PHY_ID_KSZ8051);
+	return ksz8051_ksz8795_match_phy_device(phydev, 1);
 }
 
 static int ksz8081_config_init(struct phy_device *phydev)
@@ -535,7 +535,7 @@ static int ksz8061_config_init(struct phy_device *phydev)
 
 static int ksz8795_match_phy_device(struct phy_device *phydev)
 {
-	return ksz8051_ksz8795_match_phy_device(phydev, PHY_ID_KSZ87XX);
+	return ksz8051_ksz8795_match_phy_device(phydev, 0);
 }
 
 static int ksz9021_load_values_from_of(struct phy_device *phydev,
-- 
2.24.2 (Apple Git-127)

Re: [PATCH] net: phy: micrel: Fix detection of ksz87xx switch

[Jakub Kicinski]

Please extend the CC list to the maintainers, and people who
worked on this driver in the past, especially Marek.

On Fri, 30 Jul 2021 20:51:20 +1000 Steve Bennett wrote:
> The previous logic was wrong such that the ksz87xx
> switch was not identified correctly.

Any more details of what is happening? Which extact device do you see
this problem on?

I presume ksz87xx devices used to work and gotten broken - would you
mind clarifying and adding a Fixes tag to help backporting to the
correct stable branches?

> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 4d53886f7d51..a4acec02c8cb 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -401,11 +401,11 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
>  }
>  
>  static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
> -					    const u32 ksz_phy_id)
> +					    const u32 ksz_8051)

bool and use true/false in the callers?

Re: [PATCH] net: phy: micrel: Fix detection of ksz87xx switch

[Steve Bennett]

> On 31 Jul 2021, at 2:59 am, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> Please extend the CC list to the maintainers, and people who
> worked on this driver in the past, especially Marek.

Sure, I can do that in a v2 of the patch along with the more detailed
explanation below.

> 
> On Fri, 30 Jul 2021 20:51:20 +1000 Steve Bennett wrote:
>> The previous logic was wrong such that the ksz87xx
>> switch was not identified correctly.
> 
> Any more details of what is happening? Which extact device do you see
> this problem on?

I have a ksz8795 switch.

Without the patch:

ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Generic PHY]
ksz8795-switch spi3.1 ade2 (uninitialized): PHY [dsa-0.1:04] driver [Generic PHY]

With the patch:

ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Micrel KSZ87XX Switch]
ksz8795-switch spi3.1 ade2 (uninitialized): PHY [dsa-0.1:04] driver [Micrel KSZ87XX Switch]

> 
> I presume ksz87xx devices used to work and gotten broken - would you
> mind clarifying and adding a Fixes tag to help backporting to the
> correct stable branches?

I looked at the original commit 8b95599c55ed24b36cf44a4720067cfe67edbcb4, but
it couldn't ever have worked.

ksz8051_ksz8795_match_phy_device() uses the parameter ksz_phy_id to discriminate
whether it was called from ksz8051_match_phy_device() or from ksz8795_match_phy_device()
but since PHY_ID_KSZ87XX is the same value as PHY_ID_KSZ8051, this doesn't do anything.

Need to pass a different value to discriminate.

> 
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 4d53886f7d51..a4acec02c8cb 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -401,11 +401,11 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
>> }
>> 
>> static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
>> -					    const u32 ksz_phy_id)
>> +					    const u32 ksz_8051)
> 
> bool and use true/false in the callers?

Sure.

[PATCH v2] net: phy: micrel: Fix detection of ksz87xx switch

[Steve Bennett]

The logic for discerning between KSZ8051 and KSZ87XX PHYs is incorrect
such that the that KSZ87XX switch is not identified correctly.

ksz8051_ksz8795_match_phy_device() uses the parameter ksz_phy_id
to discriminate whether it was called from ksz8051_match_phy_device()
or from ksz8795_match_phy_device() but since PHY_ID_KSZ87XX is the
same value as PHY_ID_KSZ8051, this doesn't work.

Instead use a bool to discriminate the caller.

Without this patch, the KSZ8795 switch port identifies as:

ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Generic PHY]

With the patch, it identifies correctly:

ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Micrel KSZ87XX Switch]

Fixes: 8b95599c55ed24b36cf4 ("net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs")
Signed-off-by: Steve Bennett <steveb@workware.net.au>
---
 drivers/net/phy/micrel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 4d53886f7d51..53bdd673ae56 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -401,11 +401,11 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
 }
 
 static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
-					    const u32 ksz_phy_id)
+					    const bool ksz_8051)
 {
 	int ret;
 
-	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != ksz_phy_id)
+	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
 		return 0;
 
 	ret = phy_read(phydev, MII_BMSR);
@@ -418,7 +418,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
 	 * the switch does not.
 	 */
 	ret &= BMSR_ERCAP;
-	if (ksz_phy_id == PHY_ID_KSZ8051)
+	if (ksz_8051)
 		return ret;
 	else
 		return !ret;
@@ -426,7 +426,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
 
 static int ksz8051_match_phy_device(struct phy_device *phydev)
 {
-	return ksz8051_ksz8795_match_phy_device(phydev, PHY_ID_KSZ8051);
+	return ksz8051_ksz8795_match_phy_device(phydev, true);
 }
 
 static int ksz8081_config_init(struct phy_device *phydev)
@@ -535,7 +535,7 @@ static int ksz8061_config_init(struct phy_device *phydev)
 
 static int ksz8795_match_phy_device(struct phy_device *phydev)
 {
-	return ksz8051_ksz8795_match_phy_device(phydev, PHY_ID_KSZ87XX);
+	return ksz8051_ksz8795_match_phy_device(phydev, false);
 }
 
 static int ksz9021_load_values_from_of(struct phy_device *phydev,
-- 
2.24.2 (Apple Git-127)

Re: [PATCH v2] net: phy: micrel: Fix detection of ksz87xx switch

[Marek Vasut]

On 7/31/21 12:57 AM, Steve Bennett wrote:
> The logic for discerning between KSZ8051 and KSZ87XX PHYs is incorrect
> such that the that KSZ87XX switch is not identified correctly.
> 
> ksz8051_ksz8795_match_phy_device() uses the parameter ksz_phy_id
> to discriminate whether it was called from ksz8051_match_phy_device()
> or from ksz8795_match_phy_device() but since PHY_ID_KSZ87XX is the
> same value as PHY_ID_KSZ8051, this doesn't work.
> 
> Instead use a bool to discriminate the caller.
> 
> Without this patch, the KSZ8795 switch port identifies as:
> 
> ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Generic PHY]
> 
> With the patch, it identifies correctly:
> 
> ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Micrel KSZ87XX Switch]
> 
> Fixes: 8b95599c55ed24b36cf4 ("net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs")
> Signed-off-by: Steve Bennett <steveb@workware.net.au>

Looks obviously correct to me, and yes, this reinstates behavior before 
V3 of the original patch. Thanks!

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH v2] net: phy: micrel: Fix detection of ksz87xx switch

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sat, 31 Jul 2021 08:57:50 +1000 you wrote:
> The logic for discerning between KSZ8051 and KSZ87XX PHYs is incorrect
> such that the that KSZ87XX switch is not identified correctly.
> 
> ksz8051_ksz8795_match_phy_device() uses the parameter ksz_phy_id
> to discriminate whether it was called from ksz8051_match_phy_device()
> or from ksz8795_match_phy_device() but since PHY_ID_KSZ87XX is the
> same value as PHY_ID_KSZ8051, this doesn't work.
> 
> [...]

Here is the summary with links:
  - [v2] net: phy: micrel: Fix detection of ksz87xx switch
    https://git.kernel.org/netdev/net/c/a5e63c7d38d5

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net: phy: micrel: Fix detection of ksz87xx switch

[Ben Hutchings]

On Sat, Jul 31, 2021 at 08:19:17AM +1000, Steve Bennett wrote:
> > On 31 Jul 2021, at 2:59 am, Jakub Kicinski <kuba@kernel.org> wrote:
> > 
> > Please extend the CC list to the maintainers, and people who
> > worked on this driver in the past, especially Marek.
> 
> Sure, I can do that in a v2 of the patch along with the more detailed
> explanation below.
> 
> > 
> > On Fri, 30 Jul 2021 20:51:20 +1000 Steve Bennett wrote:
> >> The previous logic was wrong such that the ksz87xx
> >> switch was not identified correctly.
> > 
> > Any more details of what is happening? Which extact device do you see
> > this problem on?
> 
> I have a ksz8795 switch.
> 
> Without the patch:
> 
> ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Generic PHY]
> ksz8795-switch spi3.1 ade2 (uninitialized): PHY [dsa-0.1:04] driver [Generic PHY]
> 
> With the patch:
> 
> ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Micrel KSZ87XX Switch]
> ksz8795-switch spi3.1 ade2 (uninitialized): PHY [dsa-0.1:04] driver [Micrel KSZ87XX Switch]
[...]

And do the external ports work for you after this?

I have a development board with a KSZ8795.  All ports worked before
this patch.  After this patch, when I bring up the external ports they
are reported as having link up at 10M half duplex, when the link is
actually down.

The ksz8873mll_read_status() function is trying to read a non-standard
MDIO register that is not handled by the ksz8795 driver's MDIO
emulation (and is not documented as existing on the KSZ8873MLL,
either!).  It also also reports link up, which is obviously not
correct for an external port.

I'll post a patch as a reply to this.

Ben.

-- 
Ben Hutchings · Senior Embedded Software Engineer, Essensium-Mind · mind.be

[PATCH net] net: phy: micrel: Fix link detection on ksz87xx switch"

[Ben Hutchings]

Commit a5e63c7d38d5 "net: phy: micrel: Fix detection of ksz87xx
switch" broke link detection on the external ports of the KSZ8795.

The previously unused phy_driver structure for these devices specifies
config_aneg and read_status functions that appear to be designed for a
fixed link and do not work with the embedded PHYs in the KSZ8795.

Delete the use of these functions in favour of the generic PHY
implementations which were used previously.

Fixes: a5e63c7d38d5 ("net: phy: micrel: Fix detection of ksz87xx switch")
Signed-off-by: Ben Hutchings <ben.hutchings@mind.be>
---
 drivers/net/phy/micrel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 53bdd673ae56..5c928f827173 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1760,8 +1760,6 @@ static struct phy_driver ksphy_driver[] = {
 	.name		= "Micrel KSZ87XX Switch",
 	/* PHY_BASIC_FEATURES */
 	.config_init	= kszphy_config_init,
-	.config_aneg	= ksz8873mll_config_aneg,
-	.read_status	= ksz8873mll_read_status,
 	.match_phy_device = ksz8795_match_phy_device,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.20.1

Re: [PATCH net] net: phy: micrel: Fix link detection on ksz87xx switch"

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sat, 7 Aug 2021 02:06:18 +0200 you wrote:
> Commit a5e63c7d38d5 "net: phy: micrel: Fix detection of ksz87xx
> switch" broke link detection on the external ports of the KSZ8795.
> 
> The previously unused phy_driver structure for these devices specifies
> config_aneg and read_status functions that appear to be designed for a
> fixed link and do not work with the embedded PHYs in the KSZ8795.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: micrel: Fix link detection on ksz87xx switch"
    https://git.kernel.org/netdev/net/c/2383cb9497d1

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net: phy: micrel: Fix detection of ksz87xx switch

[Steve Bennett]

> On 7 Aug 2021, at 10:01 am, Ben Hutchings <ben.hutchings@essensium.com> wrote:
> 
> On Sat, Jul 31, 2021 at 08:19:17AM +1000, Steve Bennett wrote:
>>> On 31 Jul 2021, at 2:59 am, Jakub Kicinski <kuba@kernel.org> wrote:
>>> 
>>> Please extend the CC list to the maintainers, and people who
>>> worked on this driver in the past, especially Marek.
>> 
>> Sure, I can do that in a v2 of the patch along with the more detailed
>> explanation below.
>> 
>>> 
>>> On Fri, 30 Jul 2021 20:51:20 +1000 Steve Bennett wrote:
>>>> The previous logic was wrong such that the ksz87xx
>>>> switch was not identified correctly.
>>> 
>>> Any more details of what is happening? Which extact device do you see
>>> this problem on?
>> 
>> I have a ksz8795 switch.
>> 
>> Without the patch:
>> 
>> ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Generic PHY]
>> ksz8795-switch spi3.1 ade2 (uninitialized): PHY [dsa-0.1:04] driver [Generic PHY]
>> 
>> With the patch:
>> 
>> ksz8795-switch spi3.1 ade1 (uninitialized): PHY [dsa-0.1:03] driver [Micrel KSZ87XX Switch]
>> ksz8795-switch spi3.1 ade2 (uninitialized): PHY [dsa-0.1:04] driver [Micrel KSZ87XX Switch]
> [...]
> 
> And do the external ports work for you after this?
> 
> I have a development board with a KSZ8795.  All ports worked before
> this patch.  After this patch, when I bring up the external ports they
> are reported as having link up at 10M half duplex, when the link is
> actually down.
> 
> The ksz8873mll_read_status() function is trying to read a non-standard
> MDIO register that is not handled by the ksz8795 driver's MDIO
> emulation (and is not documented as existing on the KSZ8873MLL,
> either!).  It also also reports link up, which is obviously not
> correct for an external port.
> 
> I'll post a patch as a reply to this.
> 
> Ben.

Thanks Ben,

That looks reasonable to me. My board is running the external ports at 10HD so
I didn't pick this up, but your patch looks correct.

Cheers,
Steve