Date: Thu, 10 Aug 2023 18:16:17 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
	Linus Walleij <linus.walleij@...aro.org>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy
 switch drivers

Hi Russell,

On Tue, Aug 08, 2023 at 03:19:20PM +0100, Russell King (Oracle) wrote:
> On Tue, Aug 08, 2023 at 04:52:15PM +0300, Vladimir Oltean wrote:
> > On Tue, Aug 08, 2023 at 01:57:43PM +0100, Russell King (Oracle) wrote:
> > > Thanks for the r-b.
> > > 
> > > At risk of delaying this patch through further discussion... so I'll
> > > say now that we're going off into discussions about future changes.
> > > 
> > > I believe all DSA drivers that provide .phylink_get_caps fill in the
> > > .mac_capabilities member, which leaves just a few drivers that do not,
> > > which are:
> > > 
> > > $ git grep -l dsa_switch_ops.*= drivers/net/dsa/ | xargs grep -L '\.phylink_get_caps'
> > > drivers/net/dsa/dsa_loop.c
> > > drivers/net/dsa/mv88e6060.c
> > > drivers/net/dsa/realtek/rtl8366rb.c
> > > drivers/net/dsa/vitesse-vsc73xx-core.c
> > > 
> > > I've floated the idea to Linus W and Arinc about setting
> > > .mac_capabilities in the non-phylink_get_caps path as well, suggesting:
> > 
> > Not sure what you mean by "in the non-phylink_get_caps path" (what is
> > that other path). Don't you mean that we should implement phylink_get_caps()
> > for these drivers, to have a unified code flow for everyone?
> 
> I meant this:
> 
>                 /* For legacy drivers */
>                 if (mode != PHY_INTERFACE_MODE_NA) {
>                         __set_bit(mode, dp->pl_config.supported_interfaces);
>                 } else {
>                         __set_bit(PHY_INTERFACE_MODE_INTERNAL,
>                                   dp->pl_config.supported_interfaces);
>                         __set_bit(PHY_INTERFACE_MODE_GMII,
>                                   dp->pl_config.supported_interfaces);
>                 }

Ah, ok, you'd like a built-in assumption of the mac_capabilities in
dsa_port_phylink_create().

> but ultimately yes, having the DSA phylink_get_caps method mandatory
> would be excellent, but I don't think we have sufficient information
> to do that.
> 
> For example, what interface modes does the Vitesse DSA switch support?
> What speeds? Does it support pause? Does it vary depending on port?

I only have a VSC7395 datasheet which was shared with me by Linus (and
that link is no longer functional).

This switch supports MII/REV-MII/GMII/RGMII on MAC 6, and MACs 0-4 are
connected to internal PHYs (yes, there is no port 5, also see the
comment in vsc73xx_probe()).

Based on vsc73xx_init_port() and vsc73xx_adjust_enable_port(), I guess
all ports support flow control, and thus, PHYs should advertise it.

I don't have a datasheet for the other switches supported by the driver:

 * Vitesse VSC7385 SparX-G5 5+1-port Integrated Gigabit Ethernet Switch
 * Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
 * Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
 * Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch

but based on the common treatment in vsc73xx_init_port(), I'd say that
on all models, port 6 (CPU_PORT) is the xMII port and all the others are
internal PHY ports, and all support the same configuration. So a
phylink_get_caps() implementation could probably also do one of two
things, based on "if (port == CPU_PORT)".

> > > 	MAC_1000 | MAC_100 | MAC_10 | MAC_SYM_PAUSE | MAC_ASYM_PAUSE
> > > 
> > > support more than 1G speeds. I think the only exception to that may
> > > be dsa_loop, but as I think that makes use of the old fixed-link
> > > software emulated PHYs, I believe that would be limited to max. 1G
> > > as well.
> > 
> > I don't believe that dsa_loop makes use of fixed-link at all. Its user
> > ports use phy/gmii mode through the non-OF-based dsa_slave_phy_connect()
> > to the ds->slave_mii_bus, and the CPU port goes through the non-OF code
> > path ("else" block) here (because dsa_loop_bdinfo.c _is_ non-OF-based):
> 
> Sorry, I meant fixed-phy not fixed-link.
> 
> > 
> > dsa_port_setup:
> > 	case DSA_PORT_TYPE_CPU:
> > 		if (dp->dn) {
> > 			err = dsa_shared_port_link_register_of(dp);
> > 			if (err)
> > 				break;
> > 			dsa_port_link_registered = true;
> > 		} else {
> > 			dev_warn(ds->dev,
> > 				 "skipping link registration for CPU port %d\n",
> > 				 dp->index);
> > 		}
> 
> What made me believe that it uses the old fixed-phy stuff is:
> 
> static int __init dsa_loop_init(void)
> ...
>         for (i = 0; i < NUM_FIXED_PHYS; i++)
>                 phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL);
> 
> These PHYs end up on the "fixed-0" virtual MDIO bus, which also has a
> MDIO device created for the dsa-loop driver at address 31. Thus, in
> dsa_loop_drv_probe():
> 
> 	ps->bus = mdiodev->bus;
> 
> is the fixed-0 bus with these fixed-PHYs on, and dsa_loop_phy_read()
> and dsa_loop_phy_write() access these fixed PHYs.
> 
> These fixed PHYs are clause-22 PHYs, which only support up to 1G
> speeds. Therefore, it is my understanding that dsa-loop will only
> support up to 1G speeds.

Clear now. Yes, this is correct.

> > > If we did set .mac_capabilities, then dsa_port_phylink_validate() would
> > > always call phylink_generic_validate() for all DSA drivers, and at that
> > > point, we don't need dsa_port_phylink_validate() anymore as it provides
> > > nothing that isn't already done inside phylink.
> > > 
> > > Once dsa_port_phylink_validate() is gone, then I believe there are no
> > > drivers populating the .validate method in phylink_mac_ops, which
> > > then means there is the possibility to remove that method.
> > 
> > Assuming I understand correctly, I agree it would be beneficial for
> > mv88e6060, rtl8366rb and vsc73xx to populate mac_capabilities and
> > supported_interfaces.
> 
> ... which we can only do if someone can furnish information on what
> these support. Short of that, we would need something in the core
> DSA code (like we're doing for the supported_interfaces) that would
> allow them to continue working until .phylink_get_caps could be
> reasonably implemented for them.
> 
> Providing a legacy .phylink_get_caps would also be a possibility.
> Maybe something like this:
> 
> void legacy_dsa_phylink_get_caps(struct dsa_switch *ds, int port,
> 				 struct phylink_config *config)
> {
> 	struct dsa_port *dp = dsa_to_port(ds, port);
> 	phy_interface_t mode;
> 	int err;
> 
> 	err = of_get_phy_mode(dp->dn, &mode);
> 	if (!err) {
> 		__set_bit(mode, config->supported_interfaces);
> 	} else {
> 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> 			  config->supported_interfaces);
> 		__set_bit(PHY_INTERFACE_MODE_GMII,
> 			  config->supported_interfaces);
> 	}
> 
> 	config->mac_capabilities = MAC_1000 | MAC_100 | MAC_10 |
> 				   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
> }
> 
> and then dsa_port_phylink_create() always calls phylink_get_caps:
> 
> -	if (ds->ops->phylink_get_caps) {
> -		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
> -	} else {
> -	...
> -	}
> +	ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);

That could be an option, but I think the volume of switches is low
enough that we could just consider converting them all.

I see you've sent a mv88e6060 patch, I'll go review that now.

Powered by blists - more mailing lists