Re: [PATCH v2 1/2] net: macb: Clean up macb_validate
From: Sean Anderson <hidden>
Date: 2021-10-22 17:37:45
Subsystem:
atmel macb ethernet driver, networking drivers, sff/sfp/sfp+ module support, the rest · Maintainers:
Théo Lebrun, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, Linus Torvalds
Hi Russell, On 10/19/21 11:02 AM, Russell King (Oracle) wrote:
On Fri, Oct 15, 2021 at 11:47:30PM +0100, Russell King (Oracle) wrote:quoted
I have been working on it but haven't finished the patches yet. There's a few issues that came up with e.g. DSA and mvneta being able to switch between different speeds with some SFP modules that have needed other tweaks.Okay, have a look at: http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue and the patches from "net: enetc: remove interface checks in enetc_pl_mac_validate ()" down to the "net-merged" branch label. That set of patches add the supported_interfaces bitmap, uses it for validation purposes, converts all but one of the ethernet drivers over to using it, and then simplifies the validate() implementations.
For "net: phy: add phy_interface_t bitmap support", phylink_or would be
nice as well. I use it when implementing NA support for PCSs.
For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
drivers/net/phy/marvell.c also needs to be converted. This is due to
b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
added to net-next/master.
(I think you have fixed this in your latest revision)
"net: phylink: use supported_interfaces for phylink validation" looks
good. Though the documentation should be updated. Perhaps something
like
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index bc4b866cd99b..a911872c12d8 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h@@ -134,11 +134,14 @@ struct phylink_mac_ops { * based on @state->advertising and/or @state->speed and update * @state->interface accordingly. See phylink_helper_basex_speed(). * - * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects the - * MAC driver to return all supported link modes. + * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects the MAC + * driver to return all supported link modes. If @config->supported_interfaces + * is populated, phylink will handle this, and it is not necessary for + * validate() to support %PHY_INTERFACE_MODE_NA. * - * If the @state->interface mode is not supported, then the @supported - * mask must be cleared. + * If the @state->interface mode is not supported, then the @supported mask + * must be cleared. If @config->supported_interfaces is populated, validate() + * will only be called with values of @state->interfaces present in the bitmap. */ void validate(struct phylink_config *config, unsigned long *supported, struct phylink_link_state *state); --
I think "net: macb: populate supported_interfaces member" is wrong. Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE. I know you leave the check in validate(), but this is the sort of thing which should be put in supported interfaces. Additionally, SGMII should depend on PCS. So something like
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c1f976a79a44..02eff23adcfb 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c@@ -880,6 +880,7 @@ static void macb_get_pcs_fixed_state(struct phylink_config *config, static int macb_mii_probe(struct net_device *dev) { struct macb *bp = netdev_priv(dev); + unsigned long *supported = bp->phylink_config.supported_interfaces; bp->phylink_config.dev = &dev->dev; bp->phylink_config.type = PHYLINK_NETDEV;
@@ -889,6 +890,21 @@ static int macb_mii_probe(struct net_device *dev) bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state; } + if (bp->caps & MACB_CAPS_HIGH_SPEED && + bp->caps & MACB_CAPS_PCS) + __set_bit(PHY_INTERFACE_MODE_10GBASER, supported); + if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { + __set_bit(PHY_INTERFACE_MODE_GMII, supported); + phy_interface_set_rgmii(supported); + if (bp->caps & MACB_CAPS_PCS) + __set_bit(PHY_INTERFACE_MODE_SGMII, supported); + } + __set_bit(PHY_INTERFACE_MODE_MII, supported); + __set_bit(PHY_INTERFACE_MODE_RMII, supported); + bp->phylink = phylink_create(&bp->phylink_config, bp->pdev->dev.fwnode, bp->phy_interface, &macb_phylink_ops); if (IS_ERR(bp->phylink)) { --
Other than that, the commits in the range you mentioned above looks good to me. For reference, my working branch with the above changes applied is [1]. [1] https://github.com/sean-anderson-seco/linux/tree/rking --Sean