Re: [PATCH net-next v2 1/7] net: ngbe: implement phylink to handle PHY device

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

On Mon, Dec 04, 2023 at 05:18:59PM +0800, Jiawen Wu wrote:
> Add phylink support for Wangxun 1Gb Ethernet controller.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  |   1 +
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  21 ++-
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 127 +++++++++++-------
>  3 files changed, 88 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 165e82de772e..4088637440c6 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -940,6 +940,7 @@ struct wx {
>  	int speed;
>  	int duplex;
>  	struct phy_device *phydev;

If you also embed struct phylink_config() in struct wx, then you can get
to it in the MAC operations using an inline function in wx_type.h:

static inline struct wx *phylink_to_wx(struct phylink_config *config)
{
	return container_of(config, struct wx, phylink_config);
}

which will be more efficient than doing the
netdev_priv(to_net_dev(config->dev)) dance. It's also what other network
drivers that use phylink do, so it brings consistency of implementation.

...
> -static void ngbe_handle_link_change(struct net_device *dev)
> +static void ngbe_phy_fixup(struct wx *wx)
>  {
> -	struct wx *wx = netdev_priv(dev);
> -	struct phy_device *phydev;
> -	u32 lan_speed, reg;
> +	struct phy_device *phydev = wx->phydev;
> +	struct ethtool_eee eee;
>  
> -	phydev = wx->phydev;
> -	if (!(wx->link != phydev->link ||
> -	      wx->speed != phydev->speed ||
> -	      wx->duplex != phydev->duplex))
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);

Phylink restricts the advertisement according to the capabilities
provided, do you should not need to do this. Please remove.

> +
> +	phydev->mac_managed_pm = true;

Please don't bypass phylink's management of this, instead set the
"mac_managed_pm" boolean in struct phylink_config.

...
> +static int ngbe_phylink_init(struct wx *wx)
>  {
> -	int ret;
> +	struct phylink_config *config;
> +	phy_interface_t phy_mode;
> +	struct phylink *phylink;
>  
> -	ret = phy_connect_direct(wx->netdev,
> -				 wx->phydev,
> -				 ngbe_handle_link_change,
> -				 PHY_INTERFACE_MODE_RGMII_ID);
> -	if (ret) {
> -		wx_err(wx, "PHY connect failed.\n");
> -		return ret;
> -	}
> +	config = devm_kzalloc(&wx->pdev->dev, sizeof(*config), GFP_KERNEL);
> +	if (!config)
> +		return -ENOMEM;
>  
> -	return 0;
> -}
> +	config->dev = &wx->netdev->dev;
> +	config->type = PHYLINK_NETDEV;
> +	config->mac_capabilities = MAC_1000FD | MAC_100FD |
> +				   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
>  
> -static void ngbe_phy_fixup(struct wx *wx)
> -{
> -	struct phy_device *phydev = wx->phydev;
> -	struct ethtool_eee eee;
> +	phy_mode = PHY_INTERFACE_MODE_RGMII_ID;
> +	__set_bit(PHY_INTERFACE_MODE_RGMII_ID, config->supported_interfaces);

As mentioned above, also set config->mac_managed_pm.

Thanks.

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