Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy switch drivers

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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);
                }

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?

> > 
> > 	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.

> > 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);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!