[RFC,net-next,v3,19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

Message ID 20210504222915.17206-19-ansuelsmth@gmail.com
State New
Headers show
Series
  • Multiple improvement to qca8k stability
Related show

Commit Message

Ansuel Smith May 4, 2021, 10:29 p.m.
Define get_phy_flags to pass switch_Revision needed to tweak the
internal PHY with debug values based on the revision.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Vladimir Oltean May 6, 2021, 11:24 a.m. | #1
On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:
> Define get_phy_flags to pass switch_Revision needed to tweak the

> internal PHY with debug values based on the revision.

> 

> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> ---

>  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++

>  1 file changed, 19 insertions(+)

> 

> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> index b4cd891ad35d..237e09bb1425 100644

> --- a/drivers/net/dsa/qca8k.c

> +++ b/drivers/net/dsa/qca8k.c

> @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,

>  	return ret;

>  }

>  

> +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> +{

> +	struct qca8k_priv *priv = ds->priv;

> +

> +	pr_info("revision from phy %d", priv->switch_revision);


Log spam.

> +	/* Communicate to the phy internal driver the switch revision.

> +	 * Based on the switch revision different values needs to be

> +	 * set to the dbg and mmd reg on the phy.

> +	 * The first 2 bit are used to communicate the switch revision

> +	 * to the phy driver.

> +	 */

> +	if (port > 0 && port < 6)

> +		return priv->switch_revision;

> +

> +	return 0;

> +}

> +

>  static enum dsa_tag_protocol

>  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,

>  		       enum dsa_tag_protocol mp)

> @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {

>  	.phylink_mac_config	= qca8k_phylink_mac_config,

>  	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,

>  	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,

> +	.get_phy_flags		= qca8k_get_phy_flags,

>  };

>  

>  static int qca8k_read_switch_id(struct qca8k_priv *priv)

> -- 

> 2.30.2

> 


Florian, I think at one point you said that a correct user of
phydev->dev_flags should first check the PHY revision and not apply
dev_flags in blind, since they are namespaced to each PHY driver?
It sounds a bit circular to pass the PHY revision to the PHY through
phydev->dev_flags, either that or I'm missing some piece.
Russell King (Oracle) May 7, 2021, 9:44 a.m. | #2
On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:
> +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> +{

> +	struct qca8k_priv *priv = ds->priv;

> +

> +	pr_info("revision from phy %d", priv->switch_revision);


Should this be a "pr_info" ?

> +

> +	/* Communicate to the phy internal driver the switch revision.

> +	 * Based on the switch revision different values needs to be

> +	 * set to the dbg and mmd reg on the phy.

> +	 * The first 2 bit are used to communicate the switch revision

> +	 * to the phy driver.

> +	 */

> +	if (port > 0 && port < 6)

> +		return priv->switch_revision;


We had some discussion back in February about the PHY flags argument
("Rework of phydev->dev_flags") as there is a need to generically
identify whether a PHY is on a SFP module or not. This discussion
hasn't progressed to any changes (yet) but some of the points remain
valid: if we do go down the route of needing to have generic PHY flags
in dev_flags, then we need vendor specific stuff to avoid those bits.
So, rather than introduce a new case of passing some undefined data
through the flags argument, can we come up with some sort of proposal
for this.

It may also be a good idea if we document it. Maybe something like
"low 16 bits are free for driver use, upper 16 bits are reserved
for generic use"?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Ansuel Smith May 7, 2021, 11:26 p.m. | #3
On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:
> On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:

> > Define get_phy_flags to pass switch_Revision needed to tweak the

> > internal PHY with debug values based on the revision.

> > 

> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> > ---

> >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++

> >  1 file changed, 19 insertions(+)

> > 

> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> > index b4cd891ad35d..237e09bb1425 100644

> > --- a/drivers/net/dsa/qca8k.c

> > +++ b/drivers/net/dsa/qca8k.c

> > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,

> >  	return ret;

> >  }

> >  

> > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> > +{

> > +	struct qca8k_priv *priv = ds->priv;

> > +

> > +	pr_info("revision from phy %d", priv->switch_revision);

> 

> Log spam.

> 

> > +	/* Communicate to the phy internal driver the switch revision.

> > +	 * Based on the switch revision different values needs to be

> > +	 * set to the dbg and mmd reg on the phy.

> > +	 * The first 2 bit are used to communicate the switch revision

> > +	 * to the phy driver.

> > +	 */

> > +	if (port > 0 && port < 6)

> > +		return priv->switch_revision;

> > +

> > +	return 0;

> > +}

> > +

> >  static enum dsa_tag_protocol

> >  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,

> >  		       enum dsa_tag_protocol mp)

> > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {

> >  	.phylink_mac_config	= qca8k_phylink_mac_config,

> >  	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,

> >  	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,

> > +	.get_phy_flags		= qca8k_get_phy_flags,

> >  };

> >  

> >  static int qca8k_read_switch_id(struct qca8k_priv *priv)

> > -- 

> > 2.30.2

> > 

> 

> Florian, I think at one point you said that a correct user of

> phydev->dev_flags should first check the PHY revision and not apply

> dev_flags in blind, since they are namespaced to each PHY driver?

> It sounds a bit circular to pass the PHY revision to the PHY through

> phydev->dev_flags, either that or I'm missing some piece.


Just to make sure. This is the SWITCH revision not the PHY revision. It
was pointed out in old version that I should get this value from the PHY
regs but they are different values. This is why the dsa driver needs to
use the dev_flags to pass the SWITCH revision to the phy driver. Am I
implementing this in the wrong way and I should declare something to
pass this value in a more standard way? (anyway i'm pushing v4 so i
don't know if we should continue that there)
Russell King (Oracle) May 7, 2021, 11:33 p.m. | #4
On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:
> On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:

> > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:

> > > Define get_phy_flags to pass switch_Revision needed to tweak the

> > > internal PHY with debug values based on the revision.

> > > 

> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> > > ---

> > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++

> > >  1 file changed, 19 insertions(+)

> > > 

> > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> > > index b4cd891ad35d..237e09bb1425 100644

> > > --- a/drivers/net/dsa/qca8k.c

> > > +++ b/drivers/net/dsa/qca8k.c

> > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,

> > >  	return ret;

> > >  }

> > >  

> > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> > > +{

> > > +	struct qca8k_priv *priv = ds->priv;

> > > +

> > > +	pr_info("revision from phy %d", priv->switch_revision);

> > 

> > Log spam.

> > 

> > > +	/* Communicate to the phy internal driver the switch revision.

> > > +	 * Based on the switch revision different values needs to be

> > > +	 * set to the dbg and mmd reg on the phy.

> > > +	 * The first 2 bit are used to communicate the switch revision

> > > +	 * to the phy driver.

> > > +	 */

> > > +	if (port > 0 && port < 6)

> > > +		return priv->switch_revision;

> > > +

> > > +	return 0;

> > > +}

> > > +

> > >  static enum dsa_tag_protocol

> > >  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,

> > >  		       enum dsa_tag_protocol mp)

> > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {

> > >  	.phylink_mac_config	= qca8k_phylink_mac_config,

> > >  	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,

> > >  	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,

> > > +	.get_phy_flags		= qca8k_get_phy_flags,

> > >  };

> > >  

> > >  static int qca8k_read_switch_id(struct qca8k_priv *priv)

> > > -- 

> > > 2.30.2

> > > 

> > 

> > Florian, I think at one point you said that a correct user of

> > phydev->dev_flags should first check the PHY revision and not apply

> > dev_flags in blind, since they are namespaced to each PHY driver?

> > It sounds a bit circular to pass the PHY revision to the PHY through

> > phydev->dev_flags, either that or I'm missing some piece.

> 

> Just to make sure. This is the SWITCH revision not the PHY revision. It

> was pointed out in old version that I should get this value from the PHY

> regs but they are different values. This is why the dsa driver needs to

> use the dev_flags to pass the SWITCH revision to the phy driver. Am I

> implementing this in the wrong way and I should declare something to

> pass this value in a more standard way? (anyway i'm pushing v4 so i

> don't know if we should continue that there)


Vladimir is confused - it is not PHY revision at all, but the PHY
identifiers.

What was actually suggested was checking the PHY identifiers before
passing PHY-driver specific flags, so that we didn't end up setting
driver private flags that are intending for one driver, but end up
actually binding a different driver, and mis-interpreting the flags.

This is one of the problems of the current scheme: it's just a
meaningless opaque u32 variable with no defined structure to it that
the various PHY drivers themselves use in whatever way they see fit.
That is only fine to use _if_ you know for certain which driver is
going to bind ahead of time.

As I mentioned in direct reply to your patch, there was discussions
about this back in February, but they seem to have stalled.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Ansuel Smith May 7, 2021, 11:51 p.m. | #5
On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote:
> On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:

> > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:

> > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:

> > > > Define get_phy_flags to pass switch_Revision needed to tweak the

> > > > internal PHY with debug values based on the revision.

> > > > 

> > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> > > > ---

> > > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++

> > > >  1 file changed, 19 insertions(+)

> > > > 

> > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> > > > index b4cd891ad35d..237e09bb1425 100644

> > > > --- a/drivers/net/dsa/qca8k.c

> > > > +++ b/drivers/net/dsa/qca8k.c

> > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,

> > > >  	return ret;

> > > >  }

> > > >  

> > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> > > > +{

> > > > +	struct qca8k_priv *priv = ds->priv;

> > > > +

> > > > +	pr_info("revision from phy %d", priv->switch_revision);

> > > 

> > > Log spam.

> > > 

> > > > +	/* Communicate to the phy internal driver the switch revision.

> > > > +	 * Based on the switch revision different values needs to be

> > > > +	 * set to the dbg and mmd reg on the phy.

> > > > +	 * The first 2 bit are used to communicate the switch revision

> > > > +	 * to the phy driver.

> > > > +	 */

> > > > +	if (port > 0 && port < 6)

> > > > +		return priv->switch_revision;

> > > > +

> > > > +	return 0;

> > > > +}

> > > > +

> > > >  static enum dsa_tag_protocol

> > > >  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,

> > > >  		       enum dsa_tag_protocol mp)

> > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {

> > > >  	.phylink_mac_config	= qca8k_phylink_mac_config,

> > > >  	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,

> > > >  	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,

> > > > +	.get_phy_flags		= qca8k_get_phy_flags,

> > > >  };

> > > >  

> > > >  static int qca8k_read_switch_id(struct qca8k_priv *priv)

> > > > -- 

> > > > 2.30.2

> > > > 

> > > 

> > > Florian, I think at one point you said that a correct user of

> > > phydev->dev_flags should first check the PHY revision and not apply

> > > dev_flags in blind, since they are namespaced to each PHY driver?

> > > It sounds a bit circular to pass the PHY revision to the PHY through

> > > phydev->dev_flags, either that or I'm missing some piece.

> > 

> > Just to make sure. This is the SWITCH revision not the PHY revision. It

> > was pointed out in old version that I should get this value from the PHY

> > regs but they are different values. This is why the dsa driver needs to

> > use the dev_flags to pass the SWITCH revision to the phy driver. Am I

> > implementing this in the wrong way and I should declare something to

> > pass this value in a more standard way? (anyway i'm pushing v4 so i

> > don't know if we should continue that there)

> 

> Vladimir is confused - it is not PHY revision at all, but the PHY

> identifiers.

> 

> What was actually suggested was checking the PHY identifiers before

> passing PHY-driver specific flags, so that we didn't end up setting

> driver private flags that are intending for one driver, but end up

> actually binding a different driver, and mis-interpreting the flags.

> 

> This is one of the problems of the current scheme: it's just a

> meaningless opaque u32 variable with no defined structure to it that

> the various PHY drivers themselves use in whatever way they see fit.

> That is only fine to use _if_ you know for certain which driver is

> going to bind ahead of time.

>


The problem here was find a way to pass data from the dsa driver to the
phy driver. In this specific case the phy driver is an internal phy
present in the switch so it won't appear on anything else. Aside from
this I agree that it seems wrong that random values are used without
some type of rules or definition but I think that try to address this
problem is too much for this already large series. In theory this should
be safe to use as this driver would only be used by qca8k dsa driver.

> As I mentioned in direct reply to your patch, there was discussions

> about this back in February, but they seem to have stalled.

> 

> -- 

> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/

> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Andrew Lunn May 8, 2021, 5:31 p.m. | #6
> The problem here was find a way to pass data from the dsa driver to the

> phy driver. In this specific case the phy driver is an internal phy

> present in the switch so it won't appear on anything else.


For internal PHYs, you are safe. But please keep in mind any RGMII
ports which the switch might have. Somebody could attach an external
PHY on such a port. So you should not return any flags for such ports.

    Andrew
Vladimir Oltean May 8, 2021, 6:26 p.m. | #7
On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote:
> On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:

> > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:

> > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:

> > > > Define get_phy_flags to pass switch_Revision needed to tweak the

> > > > internal PHY with debug values based on the revision.

> > > > 

> > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> > > > ---

> > > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++

> > > >  1 file changed, 19 insertions(+)

> > > > 

> > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> > > > index b4cd891ad35d..237e09bb1425 100644

> > > > --- a/drivers/net/dsa/qca8k.c

> > > > +++ b/drivers/net/dsa/qca8k.c

> > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,

> > > >  	return ret;

> > > >  }

> > > >  

> > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> > > > +{

> > > > +	struct qca8k_priv *priv = ds->priv;

> > > > +

> > > > +	pr_info("revision from phy %d", priv->switch_revision);

> > > 

> > > Log spam.

> > > 

> > > > +	/* Communicate to the phy internal driver the switch revision.

> > > > +	 * Based on the switch revision different values needs to be

> > > > +	 * set to the dbg and mmd reg on the phy.

> > > > +	 * The first 2 bit are used to communicate the switch revision

> > > > +	 * to the phy driver.

> > > > +	 */

> > > > +	if (port > 0 && port < 6)

> > > > +		return priv->switch_revision;

> > > > +

> > > > +	return 0;

> > > > +}

> > > > +

> > > >  static enum dsa_tag_protocol

> > > >  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,

> > > >  		       enum dsa_tag_protocol mp)

> > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {

> > > >  	.phylink_mac_config	= qca8k_phylink_mac_config,

> > > >  	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,

> > > >  	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,

> > > > +	.get_phy_flags		= qca8k_get_phy_flags,

> > > >  };

> > > >  

> > > >  static int qca8k_read_switch_id(struct qca8k_priv *priv)

> > > > -- 

> > > > 2.30.2

> > > > 

> > > 

> > > Florian, I think at one point you said that a correct user of

> > > phydev->dev_flags should first check the PHY revision and not apply

> > > dev_flags in blind, since they are namespaced to each PHY driver?

> > > It sounds a bit circular to pass the PHY revision to the PHY through

> > > phydev->dev_flags, either that or I'm missing some piece.

> > 

> > Just to make sure. This is the SWITCH revision not the PHY revision. It

> > was pointed out in old version that I should get this value from the PHY

> > regs but they are different values. This is why the dsa driver needs to

> > use the dev_flags to pass the SWITCH revision to the phy driver. Am I

> > implementing this in the wrong way and I should declare something to

> > pass this value in a more standard way? (anyway i'm pushing v4 so i

> > don't know if we should continue that there)

> 

> Vladimir is confused - it is not PHY revision at all, but the PHY

> identifiers.

> 

> What was actually suggested was checking the PHY identifiers before

> passing PHY-driver specific flags, so that we didn't end up setting

> driver private flags that are intending for one driver, but end up

> actually binding a different driver, and mis-interpreting the flags.

> 

> This is one of the problems of the current scheme: it's just a

> meaningless opaque u32 variable with no defined structure to it that

> the various PHY drivers themselves use in whatever way they see fit.

> That is only fine to use _if_ you know for certain which driver is

> going to bind ahead of time.

> 

> As I mentioned in direct reply to your patch, there was discussions

> about this back in February, but they seem to have stalled.


Yes, I was indeed confused. My problem was mixing up the PHY OUI/device ID
and revision concepts in one big fuzzy notion. I remembered Heiner's
suggestion to do something similar to mv88e6xxx_mdio_read from here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210423014741.11858-12-ansuelsmth@gmail.com/
(where the problem is that some internal PHYs are lacking a device
identifier) and thought that the problem here is the same.

Nonetheless, now it is clear to me that with care (don't set dev_flags
except for internal PHYs which are statically known), it is possible for
the PHY driver to have a larger identifier (PHY ID concatenated with
switch revision passed through dev_flags) based on which it can
configure the hardware.

Sorry.
Russell King (Oracle) May 8, 2021, 7:39 p.m. | #8
On Sat, May 08, 2021 at 09:26:20PM +0300, Vladimir Oltean wrote:
> On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote:

> > On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:

> > > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:

> > > > On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:

> > > > > Define get_phy_flags to pass switch_Revision needed to tweak the

> > > > > internal PHY with debug values based on the revision.

> > > > > 

> > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

> > > > > ---

> > > > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++

> > > > >  1 file changed, 19 insertions(+)

> > > > > 

> > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c

> > > > > index b4cd891ad35d..237e09bb1425 100644

> > > > > --- a/drivers/net/dsa/qca8k.c

> > > > > +++ b/drivers/net/dsa/qca8k.c

> > > > > @@ -1654,6 +1654,24 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,

> > > > >  	return ret;

> > > > >  }

> > > > >  

> > > > > +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)

> > > > > +{

> > > > > +	struct qca8k_priv *priv = ds->priv;

> > > > > +

> > > > > +	pr_info("revision from phy %d", priv->switch_revision);

> > > > 

> > > > Log spam.

> > > > 

> > > > > +	/* Communicate to the phy internal driver the switch revision.

> > > > > +	 * Based on the switch revision different values needs to be

> > > > > +	 * set to the dbg and mmd reg on the phy.

> > > > > +	 * The first 2 bit are used to communicate the switch revision

> > > > > +	 * to the phy driver.

> > > > > +	 */

> > > > > +	if (port > 0 && port < 6)

> > > > > +		return priv->switch_revision;

> > > > > +

> > > > > +	return 0;

> > > > > +}

> > > > > +

> > > > >  static enum dsa_tag_protocol

> > > > >  qca8k_get_tag_protocol(struct dsa_switch *ds, int port,

> > > > >  		       enum dsa_tag_protocol mp)

> > > > > @@ -1687,6 +1705,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {

> > > > >  	.phylink_mac_config	= qca8k_phylink_mac_config,

> > > > >  	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,

> > > > >  	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,

> > > > > +	.get_phy_flags		= qca8k_get_phy_flags,

> > > > >  };

> > > > >  

> > > > >  static int qca8k_read_switch_id(struct qca8k_priv *priv)

> > > > > -- 

> > > > > 2.30.2

> > > > > 

> > > > 

> > > > Florian, I think at one point you said that a correct user of

> > > > phydev->dev_flags should first check the PHY revision and not apply

> > > > dev_flags in blind, since they are namespaced to each PHY driver?

> > > > It sounds a bit circular to pass the PHY revision to the PHY through

> > > > phydev->dev_flags, either that or I'm missing some piece.

> > > 

> > > Just to make sure. This is the SWITCH revision not the PHY revision. It

> > > was pointed out in old version that I should get this value from the PHY

> > > regs but they are different values. This is why the dsa driver needs to

> > > use the dev_flags to pass the SWITCH revision to the phy driver. Am I

> > > implementing this in the wrong way and I should declare something to

> > > pass this value in a more standard way? (anyway i'm pushing v4 so i

> > > don't know if we should continue that there)

> > 

> > Vladimir is confused - it is not PHY revision at all, but the PHY

> > identifiers.

> > 

> > What was actually suggested was checking the PHY identifiers before

> > passing PHY-driver specific flags, so that we didn't end up setting

> > driver private flags that are intending for one driver, but end up

> > actually binding a different driver, and mis-interpreting the flags.

> > 

> > This is one of the problems of the current scheme: it's just a

> > meaningless opaque u32 variable with no defined structure to it that

> > the various PHY drivers themselves use in whatever way they see fit.

> > That is only fine to use _if_ you know for certain which driver is

> > going to bind ahead of time.

> > 

> > As I mentioned in direct reply to your patch, there was discussions

> > about this back in February, but they seem to have stalled.

> 

> Yes, I was indeed confused. My problem was mixing up the PHY OUI/device ID

> and revision concepts in one big fuzzy notion. I remembered Heiner's

> suggestion to do something similar to mv88e6xxx_mdio_read from here:

> https://patchwork.kernel.org/project/netdevbpf/patch/20210423014741.11858-12-ansuelsmth@gmail.com/

> (where the problem is that some internal PHYs are lacking a device

> identifier) and thought that the problem here is the same.

> 

> Nonetheless, now it is clear to me that with care (don't set dev_flags

> except for internal PHYs which are statically known), it is possible for

> the PHY driver to have a larger identifier (PHY ID concatenated with

> switch revision passed through dev_flags) based on which it can

> configure the hardware.


We do have the problem with Marvell DSA vs Marvell PHY setup in that
the Marvell DSA driver assumes that all integrated PHYs that do not
have an ID are all the same. They are most definitely not, and this
shows itself up when we register the hwmon stuff inappropriately, or
access the wrong registers to report hwmon values.

We really need to solve this problem properly rather than bodging
around it with driver specific usage of dev_flags.

We already have the ability for drivers to have custom match functions
(match_phy_device) that do not depend on the probed ID - maybe we
should have an additional u32 member in struct phy_device for the
switch_id that the PHY is a part of that PHY drivers can check in their
match_phy_device method if necessary, or otherwise use that to parse
the switch revision from. Or something like that?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Andrew Lunn May 8, 2021, 8:55 p.m. | #9
> We do have the problem with Marvell DSA vs Marvell PHY setup in that

> the Marvell DSA driver assumes that all integrated PHYs that do not

> have an ID are all the same. They are most definitely not, and this

> shows itself up when we register the hwmon stuff inappropriately, or

> access the wrong registers to report hwmon values.


Hi Russell

This was to some degree fixed recently. Rather than always use the
6390 ID for everything, the family ID is now used. This should avoid
the issue with the SERDES being incorrectly considered a PHY, since
that particular family ID is not listed in the PHY driver.

      Andrew

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b4cd891ad35d..237e09bb1425 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1654,6 +1654,24 @@  qca8k_port_vlan_del(struct dsa_switch *ds, int port,
 	return ret;
 }
 
+static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	pr_info("revision from phy %d", priv->switch_revision);
+
+	/* Communicate to the phy internal driver the switch revision.
+	 * Based on the switch revision different values needs to be
+	 * set to the dbg and mmd reg on the phy.
+	 * The first 2 bit are used to communicate the switch revision
+	 * to the phy driver.
+	 */
+	if (port > 0 && port < 6)
+		return priv->switch_revision;
+
+	return 0;
+}
+
 static enum dsa_tag_protocol
 qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 		       enum dsa_tag_protocol mp)
@@ -1687,6 +1705,7 @@  static const struct dsa_switch_ops qca8k_switch_ops = {
 	.phylink_mac_config	= qca8k_phylink_mac_config,
 	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
 	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
+	.get_phy_flags		= qca8k_get_phy_flags,
 };
 
 static int qca8k_read_switch_id(struct qca8k_priv *priv)