Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

["Marek Behún" <marek.behun@nic.cz>]

On Wed, 12 Aug 2020 16:54:41 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Wed, Aug 12, 2020 at 05:44:36PM +0200, Andrew Lunn wrote:
> > > I'm aware of that problem.  I have some experimental patches
> > > which add PHY interface mode bitmaps to the MAC, PHY, and SFP
> > > module parsing functions.  I have stumbled on some problems
> > > though - it's going to be another API change (and people are
> > > already whinging about the phylink API changing "too quickly",
> > > were too quickly seems to be defined as once in three years), and
> > > in some cases, DSA, it's extremely hard to work out how to
> > > properly set such a bitmap due to DSA's layered approach.  
> > 
> > Hi Russell
> > 
> > If DSAs layering is causing real problems, we could rip it out, and
> > let the driver directly interact with phylink. I'm not opposed to
> > that.  
> 
> The reason I mentioned it is because I have this unpublished patch
> (beware, whitespace damaged due to copy-n-pasted):
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index c1967e08b017..ba32492f3ec0 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1629,6 +1629,12 @@ static int dsa_slave_phy_setup(struct
> net_device *slave_dev)
> 
>         dp->pl_config.dev = &slave_dev->dev;
>         dp->pl_config.type = PHYLINK_NETDEV;
> +       __set_bit(PHY_INTERFACE_MODE_SGMII,
> +                 dp->pl_config.supported_interfaces);
> +       __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +                 dp->pl_config.supported_interfaces);
> +       __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +                 dp->pl_config.supported_interfaces);
> 
>         /* The get_fixed_state callback takes precedence over polling
> the
>          * link GPIO in PHYLINK (see phylink_get_fixed_state).  Only
> set
> 
> Which clearly is a gross hack - this code certainly has no idea about
> what interfaces the port itself supports.  How do we get around that
> with DSA layering?
> 
> We could add yet-another-driver-call down into the DSA driver for it
> to fill in that information and keep the current structure.  However,
> is that really the best solution - to have lots of fine grained driver
> calls?
> 
> DSA feels to be very cumbersome and awkward to modify at least to me.
> Almost everything seems to be "add another driver call" at the DSA
> layer, followed by "add another sub-driver call" at the mv88e6xxx
> layer.
> 

So I am encountering this now when testing my SFP + marvell10g patches
on a board with SFP cage on a DSA port.

I get what you find cumbersome, but I don't think there is other
reasonable solution here. We already have .phylink_validate, which
works with ethtool mode mask. So maybe a .phylink_config_init ?

Marek