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

[Russell King - ARM Linux admin]

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@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.
> > 
> > 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@xxxxxxxxx/
> (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!