Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

[Sean Anderson <sean.anderson@seco.com>]

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