Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

Russell King (Oracle)
2025-01-08T12:43:42Z

On Wed, Jan 08, 2025 at 01:13:35PM +0100, Oleksij Rempel wrote:
> @@ -2508,6 +2369,207 @@ static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
> dev->domain_data.irqdomain = NULL;
> }
>
> +static void lan78xx_mac_config(struct phylink_config *config, unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct net_device *net = to_net_dev(config->dev);
> + struct lan78xx_net *dev = netdev_priv(net);
> + u32 rgmii_id = 0;
> + u32 mac_cr = 0;
> + int ret;
> +
> + /* Check if the mode is supported */
> + if (mode != MLO_AN_FIXED && mode != MLO_AN_PHY) {
> + netdev_err(net, "Unsupported negotiation mode: %u\n", mode);
> + return;
> + }
> +
> + switch (state->interface) {
> + case PHY_INTERFACE_MODE_INTERNAL:
> + case PHY_INTERFACE_MODE_GMII:
> + mac_cr |= MAC_CR_GMII_EN_;
> + break;

The indentation has gone a bit weird here.

> + case PHY_INTERFACE_MODE_RGMII_ID:
> + break;

Normally, a MAC should support all RGMII interface modes, because these
define the RGMII delays at the PHY and have little dependence on the
MAC.

> +static int lan78xx_phylink_setup(struct lan78xx_net *dev)
> +{
> + phy_interface_t link_interface;
> + struct phylink *phylink;
> +
> + dev->phylink_config.dev = &dev->net->dev;
> + dev->phylink_config.type = PHYLINK_NETDEV;
> + dev->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000FD;
> + dev->phylink_config.mac_managed_pm = true;
> +
> + if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> + __set_bit(PHY_INTERFACE_MODE_RGMII,
> + dev->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_RGMII_ID,
> + dev->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_RGMII_RXID,
> + dev->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_RGMII_TXID,
> + dev->phylink_config.supported_interfaces);

The mac_config implementation conflicts with this.

> + link_interface = PHY_INTERFACE_MODE_NA;

This is supposed to be for DSA, not for general use. Is there any reason
you can't pass the real mode that is being used here?

> @@ -2576,7 +2673,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> return -EIO;
> }
> phydev->is_internal = true;
> - dev->interface = PHY_INTERFACE_MODE_GMII;
> + phydev->interface = PHY_INTERFACE_MODE_INTERNAL;

This needs to be explained.

As for continuing to use fixed-phy, please instead use
phylink_set_fixed_link() instead.

Thanks.

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