Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
@ 2019-10-10 19:46 Marek Vasut
  2019-10-10 19:46 ` [PATCH V2 2/2] net: phy: micrel: Update KSZ87xx PHY name Marek Vasut
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Marek Vasut @ 2019-10-10 19:46 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Heiner Kallweit, Tristram Ha, Woojung Huh

The KSZ8051 PHY and the KSZ8794/KSZ8795/KSZ8765 switch share exactly the
same PHY ID. Since KSZ8051 is higher in the ksphy_driver[] list of PHYs
in the micrel PHY driver, it is used even with the KSZ87xx switch. This
is wrong, since the KSZ8051 configures registers of the PHY which are
not present on the simplified KSZ87xx switch PHYs and misconfigures
other registers of the KSZ87xx switch PHYs.

Fortunatelly, it is possible to tell apart the KSZ8051 PHY from the
KSZ87xx switch by checking the Basic Status register Bit 0, which is
read-only and indicates presence of the Extended Capability Registers.
The KSZ8051 PHY has those registers while the KSZ87xx switch does not.

This patch implements simple check for the presence of this bit for
both the KSZ8051 PHY and KSZ87xx switch, to let both use the correct
PHY driver instance.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David S. Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: George McCollister <george.mccollister@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
---
NOTE: It was also suggested to populate phydev->dev_flags to discern
      the PHY from the switch, this does not work for setups where
      the switch is used as a PHY without a DSA driver. Checking the
      BMSR Bit 0 for Extended Capability Register works for both DSA
      and non-DSA usecase.
V2: Move phy_id check into ksz8051_match_phy_device() and
    ksz8795_match_phy_device() and drop phy_id{,_mask} from the
    ksphy_driver[] list to avoid matching on other PHY IDs.
---
 drivers/net/phy/micrel.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 2fea5541c35a..028a4a177790 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -341,6 +341,25 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
 	return genphy_config_aneg(phydev);
 }
 
+static int ksz8051_match_phy_device(struct phy_device *phydev)
+{
+	int ret;
+
+	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
+		return 0;
+
+	ret = phy_read(phydev, MII_BMSR);
+	if (ret < 0)
+		return ret;
+
+	/* KSZ8051 PHY and KSZ8794/KSZ8795/KSZ8765 switch share the same
+	 * exact PHY ID. However, they can be told apart by the extended
+	 * capability registers presence. The KSZ8051 PHY has them while
+	 * the switch does not.
+	 */
+	return ret & BMSR_ERCAP;
+}
+
 static int ksz8081_config_init(struct phy_device *phydev)
 {
 	/* KSZPHY_OMSO_FACTORY_TEST is set at de-assertion of the reset line
@@ -364,6 +383,21 @@ static int ksz8061_config_init(struct phy_device *phydev)
 	return kszphy_config_init(phydev);
 }
 
+static int ksz8795_match_phy_device(struct phy_device *phydev)
+{
+	int ret;
+
+	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
+		return 0;
+
+	ret = phy_read(phydev, MII_BMSR);
+	if (ret < 0)
+		return ret;
+
+	/* See comment in ksz8051_match_phy_device() for details. */
+	return !(ret & BMSR_ERCAP);
+}
+
 static int ksz9021_load_values_from_of(struct phy_device *phydev,
 				       const struct device_node *of_node,
 				       u16 reg,
@@ -1017,8 +1051,6 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	.phy_id		= PHY_ID_KSZ8051,
-	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8051",
 	/* PHY_BASIC_FEATURES */
 	.driver_data	= &ksz8051_type,
@@ -1029,6 +1061,7 @@ static struct phy_driver ksphy_driver[] = {
 	.get_sset_count = kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
+	.match_phy_device = ksz8051_match_phy_device,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -1141,13 +1174,12 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	.phy_id		= PHY_ID_KSZ8795,
-	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8795",
 	/* 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.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH V2 2/2] net: phy: micrel: Update KSZ87xx PHY name
  2019-10-10 19:46 [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Marek Vasut
@ 2019-10-10 19:46 ` Marek Vasut
  2019-10-11  5:57 ` [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Simon Horman
  2019-10-12 20:58 ` Heiner Kallweit
  2 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2019-10-10 19:46 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Heiner Kallweit, Tristram Ha, Woojung Huh

The KSZ8795 PHY ID is in fact used by KSZ8794/KSZ8795/KSZ8765 switches.
Update the PHY ID and name to reflect that, as this family of switches
is commonly refered to as KSZ87xx

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David S. Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: George McCollister <george.mccollister@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
---
V2: Rebase on top of 1/2, no functional change
---
 drivers/net/phy/micrel.c   | 4 ++--
 include/linux/micrel_phy.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 028a4a177790..2c5b0f81d4f3 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -387,7 +387,7 @@ static int ksz8795_match_phy_device(struct phy_device *phydev)
 {
 	int ret;
 
-	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
+	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ87XX)
 		return 0;
 
 	ret = phy_read(phydev, MII_BMSR);
@@ -1174,7 +1174,7 @@ static struct phy_driver ksphy_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	.name		= "Micrel KSZ8795",
+	.name		= "Micrel KSZ87XX Switch",
 	/* PHY_BASIC_FEATURES */
 	.config_init	= kszphy_config_init,
 	.config_aneg	= ksz8873mll_config_aneg,
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index ad24554f11f9..75f880c25bb8 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -31,7 +31,7 @@
 #define PHY_ID_KSZ886X		0x00221430
 #define PHY_ID_KSZ8863		0x00221435
 
-#define PHY_ID_KSZ8795		0x00221550
+#define PHY_ID_KSZ87XX		0x00221550
 
 #define	PHY_ID_KSZ9477		0x00221631
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
  2019-10-10 19:46 [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Marek Vasut
  2019-10-10 19:46 ` [PATCH V2 2/2] net: phy: micrel: Update KSZ87xx PHY name Marek Vasut
@ 2019-10-11  5:57 ` Simon Horman
  2019-10-11  6:52   ` Marek Vasut
  2019-10-12 20:58 ` Heiner Kallweit
  2 siblings, 1 reply; 8+ messages in thread
From: Simon Horman @ 2019-10-11  5:57 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Heiner Kallweit, Tristram Ha, Woojung Huh

On Thu, Oct 10, 2019 at 09:46:21PM +0200, Marek Vasut wrote:
> The KSZ8051 PHY and the KSZ8794/KSZ8795/KSZ8765 switch share exactly the
> same PHY ID. Since KSZ8051 is higher in the ksphy_driver[] list of PHYs
> in the micrel PHY driver, it is used even with the KSZ87xx switch. This
> is wrong, since the KSZ8051 configures registers of the PHY which are
> not present on the simplified KSZ87xx switch PHYs and misconfigures
> other registers of the KSZ87xx switch PHYs.
> 
> Fortunatelly, it is possible to tell apart the KSZ8051 PHY from the
> KSZ87xx switch by checking the Basic Status register Bit 0, which is
> read-only and indicates presence of the Extended Capability Registers.
> The KSZ8051 PHY has those registers while the KSZ87xx switch does not.
> 
> This patch implements simple check for the presence of this bit for
> both the KSZ8051 PHY and KSZ87xx switch, to let both use the correct
> PHY driver instance.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: George McCollister <george.mccollister@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> Cc: Woojung Huh <woojung.huh@microchip.com>
> ---
> NOTE: It was also suggested to populate phydev->dev_flags to discern
>       the PHY from the switch, this does not work for setups where
>       the switch is used as a PHY without a DSA driver. Checking the
>       BMSR Bit 0 for Extended Capability Register works for both DSA
>       and non-DSA usecase.
> V2: Move phy_id check into ksz8051_match_phy_device() and
>     ksz8795_match_phy_device() and drop phy_id{,_mask} from the
>     ksphy_driver[] list to avoid matching on other PHY IDs.
> ---
>  drivers/net/phy/micrel.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2fea5541c35a..028a4a177790 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -341,6 +341,25 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
>  	return genphy_config_aneg(phydev);
>  }
>  
> +static int ksz8051_match_phy_device(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
> +		return 0;
> +
> +	ret = phy_read(phydev, MII_BMSR);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* KSZ8051 PHY and KSZ8794/KSZ8795/KSZ8765 switch share the same
> +	 * exact PHY ID. However, they can be told apart by the extended
> +	 * capability registers presence. The KSZ8051 PHY has them while
> +	 * the switch does not.
> +	 */
> +	return ret & BMSR_ERCAP;
> +}
> +
>  static int ksz8081_config_init(struct phy_device *phydev)
>  {
>  	/* KSZPHY_OMSO_FACTORY_TEST is set at de-assertion of the reset line
> @@ -364,6 +383,21 @@ static int ksz8061_config_init(struct phy_device *phydev)
>  	return kszphy_config_init(phydev);
>  }
>  
> +static int ksz8795_match_phy_device(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
> +		return 0;
> +
> +	ret = phy_read(phydev, MII_BMSR);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* See comment in ksz8051_match_phy_device() for details. */
> +	return !(ret & BMSR_ERCAP);
> +}
> +

Hi Marek,

given the similarity between ksz8051_match_phy_device() and
ksz8795_match_phy_device() I wonder if a common helper is appropriate.

>  static int ksz9021_load_values_from_of(struct phy_device *phydev,
>  				       const struct device_node *of_node,
>  				       u16 reg,
> @@ -1017,8 +1051,6 @@ static struct phy_driver ksphy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> -	.phy_id		= PHY_ID_KSZ8051,
> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>  	.name		= "Micrel KSZ8051",
>  	/* PHY_BASIC_FEATURES */
>  	.driver_data	= &ksz8051_type,
> @@ -1029,6 +1061,7 @@ static struct phy_driver ksphy_driver[] = {
>  	.get_sset_count = kszphy_get_sset_count,
>  	.get_strings	= kszphy_get_strings,
>  	.get_stats	= kszphy_get_stats,
> +	.match_phy_device = ksz8051_match_phy_device,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> @@ -1141,13 +1174,12 @@ static struct phy_driver ksphy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> -	.phy_id		= PHY_ID_KSZ8795,
> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>  	.name		= "Micrel KSZ8795",
>  	/* 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.23.0
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
  2019-10-11  5:57 ` [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Simon Horman
@ 2019-10-11  6:52   ` Marek Vasut
  2019-10-11  9:47     ` Simon Horman
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2019-10-11  6:52 UTC (permalink / raw)
  To: Simon Horman
  Cc: netdev, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Heiner Kallweit, Tristram Ha, Woojung Huh

On 10/11/19 7:57 AM, Simon Horman wrote:
[...]
>> +static int ksz8795_match_phy_device(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
>> +		return 0;
>> +
>> +	ret = phy_read(phydev, MII_BMSR);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* See comment in ksz8051_match_phy_device() for details. */
>> +	return !(ret & BMSR_ERCAP);
>> +}
>> +
> 
> Hi Marek,
> 
> given the similarity between ksz8051_match_phy_device() and
> ksz8795_match_phy_device() I wonder if a common helper is appropriate.

Then one (or both) of them look like this:

static int ksz8795_match_phy_device(struct phy_device *phydev)
{
        int ret;

        /* See comment in ksz8051_match_phy_device() for details. */
        ret = ksz8051_match_phy_device(phydev);
        if (ret < 0)
                return ret;

        return !ret;
}

It's not that much better.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
  2019-10-11  6:52   ` Marek Vasut
@ 2019-10-11  9:47     ` Simon Horman
  0 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2019-10-11  9:47 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Heiner Kallweit, Tristram Ha, Woojung Huh

On Fri, Oct 11, 2019 at 08:52:13AM +0200, Marek Vasut wrote:
> On 10/11/19 7:57 AM, Simon Horman wrote:
> [...]
> >> +static int ksz8795_match_phy_device(struct phy_device *phydev)
> >> +{
> >> +	int ret;
> >> +
> >> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
> >> +		return 0;
> >> +
> >> +	ret = phy_read(phydev, MII_BMSR);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	/* See comment in ksz8051_match_phy_device() for details. */
> >> +	return !(ret & BMSR_ERCAP);
> >> +}
> >> +
> > 
> > Hi Marek,
> > 
> > given the similarity between ksz8051_match_phy_device() and
> > ksz8795_match_phy_device() I wonder if a common helper is appropriate.
> 
> Then one (or both) of them look like this:
> 
> static int ksz8795_match_phy_device(struct phy_device *phydev)
> {
>         int ret;
> 
>         /* See comment in ksz8051_match_phy_device() for details. */
>         ret = ksz8051_match_phy_device(phydev);
>         if (ret < 0)
>                 return ret;
> 
>         return !ret;
> }
> 
> It's not that much better.

Hi Marek,

I think I slightly prefer this but I do see your point
and I have no objections to leaving the patch as-is.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
  2019-10-10 19:46 [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Marek Vasut
  2019-10-10 19:46 ` [PATCH V2 2/2] net: phy: micrel: Update KSZ87xx PHY name Marek Vasut
  2019-10-11  5:57 ` [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Simon Horman
@ 2019-10-12 20:58 ` Heiner Kallweit
  2019-10-12 21:22   ` Heiner Kallweit
  2 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2019-10-12 20:58 UTC (permalink / raw)
  To: Marek Vasut, netdev
  Cc: Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Tristram Ha, Woojung Huh

On 10.10.2019 21:46, Marek Vasut wrote:
> The KSZ8051 PHY and the KSZ8794/KSZ8795/KSZ8765 switch share exactly the
> same PHY ID. Since KSZ8051 is higher in the ksphy_driver[] list of PHYs
> in the micrel PHY driver, it is used even with the KSZ87xx switch. This
> is wrong, since the KSZ8051 configures registers of the PHY which are
> not present on the simplified KSZ87xx switch PHYs and misconfigures
> other registers of the KSZ87xx switch PHYs.
> 
> Fortunatelly, it is possible to tell apart the KSZ8051 PHY from the
> KSZ87xx switch by checking the Basic Status register Bit 0, which is
> read-only and indicates presence of the Extended Capability Registers.
> The KSZ8051 PHY has those registers while the KSZ87xx switch does not.
> 
> This patch implements simple check for the presence of this bit for
> both the KSZ8051 PHY and KSZ87xx switch, to let both use the correct
> PHY driver instance.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: George McCollister <george.mccollister@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> Cc: Woojung Huh <woojung.huh@microchip.com>
> ---
> NOTE: It was also suggested to populate phydev->dev_flags to discern
>       the PHY from the switch, this does not work for setups where
>       the switch is used as a PHY without a DSA driver. Checking the
>       BMSR Bit 0 for Extended Capability Register works for both DSA
>       and non-DSA usecase.
> V2: Move phy_id check into ksz8051_match_phy_device() and
>     ksz8795_match_phy_device() and drop phy_id{,_mask} from the
>     ksphy_driver[] list to avoid matching on other PHY IDs.
> ---
>  drivers/net/phy/micrel.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2fea5541c35a..028a4a177790 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -341,6 +341,25 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
>  	return genphy_config_aneg(phydev);
>  }
>  
> +static int ksz8051_match_phy_device(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
> +		return 0;
> +
> +	ret = phy_read(phydev, MII_BMSR);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* KSZ8051 PHY and KSZ8794/KSZ8795/KSZ8765 switch share the same
> +	 * exact PHY ID. However, they can be told apart by the extended
> +	 * capability registers presence. The KSZ8051 PHY has them while
> +	 * the switch does not.
> +	 */
> +	return ret & BMSR_ERCAP;
> +}
> +
>  static int ksz8081_config_init(struct phy_device *phydev)
>  {
>  	/* KSZPHY_OMSO_FACTORY_TEST is set at de-assertion of the reset line
> @@ -364,6 +383,21 @@ static int ksz8061_config_init(struct phy_device *phydev)
>  	return kszphy_config_init(phydev);
>  }
>  
> +static int ksz8795_match_phy_device(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
> +		return 0;
> +
> +	ret = phy_read(phydev, MII_BMSR);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* See comment in ksz8051_match_phy_device() for details. */
> +	return !(ret & BMSR_ERCAP);
> +}
> +
>  static int ksz9021_load_values_from_of(struct phy_device *phydev,
>  				       const struct device_node *of_node,
>  				       u16 reg,
> @@ -1017,8 +1051,6 @@ static struct phy_driver ksphy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> -	.phy_id		= PHY_ID_KSZ8051,
> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>  	.name		= "Micrel KSZ8051",
>  	/* PHY_BASIC_FEATURES */
>  	.driver_data	= &ksz8051_type,
> @@ -1029,6 +1061,7 @@ static struct phy_driver ksphy_driver[] = {
>  	.get_sset_count = kszphy_get_sset_count,
>  	.get_strings	= kszphy_get_strings,
>  	.get_stats	= kszphy_get_stats,
> +	.match_phy_device = ksz8051_match_phy_device,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> @@ -1141,13 +1174,12 @@ static struct phy_driver ksphy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> -	.phy_id		= PHY_ID_KSZ8795,
> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>  	.name		= "Micrel KSZ8795",
>  	/* 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,
>  }, {
> 

Patch needs to be annotated as "net-next".
See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

Apart from that:
Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
  2019-10-12 20:58 ` Heiner Kallweit
@ 2019-10-12 21:22   ` Heiner Kallweit
  2019-10-13 10:53     ` Marek Vasut
  0 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2019-10-12 21:22 UTC (permalink / raw)
  To: Marek Vasut, netdev
  Cc: Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Tristram Ha, Woojung Huh

On 12.10.2019 22:58, Heiner Kallweit wrote:
> On 10.10.2019 21:46, Marek Vasut wrote:
>> The KSZ8051 PHY and the KSZ8794/KSZ8795/KSZ8765 switch share exactly the
>> same PHY ID. Since KSZ8051 is higher in the ksphy_driver[] list of PHYs
>> in the micrel PHY driver, it is used even with the KSZ87xx switch. This
>> is wrong, since the KSZ8051 configures registers of the PHY which are
>> not present on the simplified KSZ87xx switch PHYs and misconfigures
>> other registers of the KSZ87xx switch PHYs.
>>
>> Fortunatelly, it is possible to tell apart the KSZ8051 PHY from the
>> KSZ87xx switch by checking the Basic Status register Bit 0, which is
>> read-only and indicates presence of the Extended Capability Registers.
>> The KSZ8051 PHY has those registers while the KSZ87xx switch does not.
>>
>> This patch implements simple check for the presence of this bit for
>> both the KSZ8051 PHY and KSZ87xx switch, to let both use the correct
>> PHY driver instance.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: George McCollister <george.mccollister@gmail.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
>> Cc: Woojung Huh <woojung.huh@microchip.com>
>> ---
>> NOTE: It was also suggested to populate phydev->dev_flags to discern
>>       the PHY from the switch, this does not work for setups where
>>       the switch is used as a PHY without a DSA driver. Checking the
>>       BMSR Bit 0 for Extended Capability Register works for both DSA
>>       and non-DSA usecase.
>> V2: Move phy_id check into ksz8051_match_phy_device() and
>>     ksz8795_match_phy_device() and drop phy_id{,_mask} from the
>>     ksphy_driver[] list to avoid matching on other PHY IDs.
>> ---
>>  drivers/net/phy/micrel.c | 40 ++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 2fea5541c35a..028a4a177790 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -341,6 +341,25 @@ static int ksz8041_config_aneg(struct phy_device *phydev)
>>  	return genphy_config_aneg(phydev);
>>  }
>>  
>> +static int ksz8051_match_phy_device(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
>> +		return 0;
>> +
>> +	ret = phy_read(phydev, MII_BMSR);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* KSZ8051 PHY and KSZ8794/KSZ8795/KSZ8765 switch share the same
>> +	 * exact PHY ID. However, they can be told apart by the extended
>> +	 * capability registers presence. The KSZ8051 PHY has them while
>> +	 * the switch does not.
>> +	 */
>> +	return ret & BMSR_ERCAP;
>> +}
>> +
>>  static int ksz8081_config_init(struct phy_device *phydev)
>>  {
>>  	/* KSZPHY_OMSO_FACTORY_TEST is set at de-assertion of the reset line
>> @@ -364,6 +383,21 @@ static int ksz8061_config_init(struct phy_device *phydev)
>>  	return kszphy_config_init(phydev);
>>  }
>>  
>> +static int ksz8795_match_phy_device(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8795)
>> +		return 0;
>> +
>> +	ret = phy_read(phydev, MII_BMSR);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* See comment in ksz8051_match_phy_device() for details. */
>> +	return !(ret & BMSR_ERCAP);
>> +}
>> +
>>  static int ksz9021_load_values_from_of(struct phy_device *phydev,
>>  				       const struct device_node *of_node,
>>  				       u16 reg,
>> @@ -1017,8 +1051,6 @@ static struct phy_driver ksphy_driver[] = {
>>  	.suspend	= genphy_suspend,
>>  	.resume		= genphy_resume,
>>  }, {
>> -	.phy_id		= PHY_ID_KSZ8051,
>> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>>  	.name		= "Micrel KSZ8051",
>>  	/* PHY_BASIC_FEATURES */
>>  	.driver_data	= &ksz8051_type,
>> @@ -1029,6 +1061,7 @@ static struct phy_driver ksphy_driver[] = {
>>  	.get_sset_count = kszphy_get_sset_count,
>>  	.get_strings	= kszphy_get_strings,
>>  	.get_stats	= kszphy_get_stats,
>> +	.match_phy_device = ksz8051_match_phy_device,
>>  	.suspend	= genphy_suspend,
>>  	.resume		= genphy_resume,
>>  }, {
>> @@ -1141,13 +1174,12 @@ static struct phy_driver ksphy_driver[] = {
>>  	.suspend	= genphy_suspend,
>>  	.resume		= genphy_resume,
>>  }, {
>> -	.phy_id		= PHY_ID_KSZ8795,
>> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>>  	.name		= "Micrel KSZ8795",
>>  	/* 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,
>>  }, {
>>
> 
> Patch needs to be annotated as "net-next".
> See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
> 
Except you consider this a fix, then it would require a Fixes tag and
should be annotated "net". The question is:
Do KSZ87xx switches misbehave currently?

> Apart from that:
> Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs
  2019-10-12 21:22   ` Heiner Kallweit
@ 2019-10-13 10:53     ` Marek Vasut
  0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2019-10-13 10:53 UTC (permalink / raw)
  To: Heiner Kallweit, netdev
  Cc: Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Tristram Ha, Woojung Huh

On 10/12/19 11:22 PM, Heiner Kallweit wrote:

[...]

>>> @@ -1141,13 +1174,12 @@ static struct phy_driver ksphy_driver[] = {
>>>  	.suspend	= genphy_suspend,
>>>  	.resume		= genphy_resume,
>>>  }, {
>>> -	.phy_id		= PHY_ID_KSZ8795,
>>> -	.phy_id_mask	= MICREL_PHY_ID_MASK,
>>>  	.name		= "Micrel KSZ8795",
>>>  	/* 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,
>>>  }, {
>>>
>>
>> Patch needs to be annotated as "net-next".
>> See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
>>
> Except you consider this a fix, then it would require a Fixes tag and
> should be annotated "net". The question is:
> Do KSZ87xx switches misbehave currently?

Well yes they do, otherwise I won't be sending this fix.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 19:46 [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Marek Vasut
2019-10-10 19:46 ` [PATCH V2 2/2] net: phy: micrel: Update KSZ87xx PHY name Marek Vasut
2019-10-11  5:57 ` [PATCH V2 1/2] net: phy: micrel: Discern KSZ8051 and KSZ8795 PHYs Simon Horman
2019-10-11  6:52   ` Marek Vasut
2019-10-11  9:47     ` Simon Horman
2019-10-12 20:58 ` Heiner Kallweit
2019-10-12 21:22   ` Heiner Kallweit
2019-10-13 10:53     ` Marek Vasut

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git