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

[Ansuel Smith]

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@xxxxxxxxx>
> > > > ---
> > > >  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!