Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Florian Fainelli]

On 09/04/2018 05:15 PM, Moritz Fischer wrote:
> Add basic PHYLINK support to driver.
> 
> Suggested-by: Andrew Lunn <andrew@xxxxxxx>
> Signed-off-by: Moritz Fischer <mdf@xxxxxxxxxx>
> ---
> 
> Hi all,
> 
> as Andrew suggested in order to enable SFP as
> well as fixed-link support add PHYLINK support.
> 
> A couple of questions are still open (hence the RFC):
> 
> 1) It seems odd to implement PHYLINK callbacks that
>    are all empty? If so, should we have generic empty
>    ones in drivers/net/phy/phylink.c like we have for
>    genphys?

Yes it is odd, the validate callback most certainly should not be empty,
neither should the mac_config and mac_link_{up,down}, but, with some
luck, you can get things to just work, typically with MDIO PHYs, since a
large amount of what they can do is discoverable.

If you had an existing adjust_link callback from PHYLIB, it's really
about breaking it down such that the MAC configuration of
speed/duplex/pause happens in mac_config, and the link setting (if
necessary), happens in mac_link_{up,down}, and that's pretty much it for
MLO_AN_PHY cases.

> 
> 2) If this is ok, then I'll go ahead rework this with
>    a DT binding update to deprecate the non-'mdio'-subnode
>    case (since there are no in-tree users we might just
>    change the binding)?
> 
> 3) I'm again not sure about the 'select PHYLINK', wouldn't
>    wanna break the build again...
> 
> Thanks again for your time!
> 
> Moritz
> 
> ---
>  drivers/net/ethernet/ni/Kconfig |   1 +
>  drivers/net/ethernet/ni/nixge.c | 115 +++++++++++++++++++++++---------
>  2 files changed, 83 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ni/Kconfig b/drivers/net/ethernet/ni/Kconfig
> index c73978474c4b..80cd72948551 100644
> --- a/drivers/net/ethernet/ni/Kconfig
> +++ b/drivers/net/ethernet/ni/Kconfig
> @@ -21,6 +21,7 @@ config NI_XGE_MANAGEMENT_ENET
>  	depends on HAS_IOMEM && HAS_DMA
>  	select PHYLIB
>  	select OF_MDIO if OF
> +	select PHYLINK
>  	help
>  	  Simple LAN device for debug or management purposes. Can
>  	  support either 10G or 1G PHYs via SFP+ ports.
> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 74cf52e3fb09..a0e790d07b1c 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -11,6 +11,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/of_platform.h>
> +#include <linux/phylink.h>
>  #include <linux/of_irq.h>
>  #include <linux/skbuff.h>
>  #include <linux/phy.h>
> @@ -165,7 +166,7 @@ struct nixge_priv {
>  	struct device *dev;
>  
>  	/* Connection to PHY device */
> -	struct device_node *phy_node;
> +	struct phylink *phylink;
>  	phy_interface_t		phy_mode;
>  
>  	int link;
> @@ -416,20 +417,6 @@ static void nixge_device_reset(struct net_device *ndev)
>  	netif_trans_update(ndev);
>  }
>  
> -static void nixge_handle_link_change(struct net_device *ndev)
> -{
> -	struct nixge_priv *priv = netdev_priv(ndev);
> -	struct phy_device *phydev = ndev->phydev;
> -
> -	if (phydev->link != priv->link || phydev->speed != priv->speed ||
> -	    phydev->duplex != priv->duplex) {
> -		priv->link = phydev->link;
> -		priv->speed = phydev->speed;
> -		priv->duplex = phydev->duplex;
> -		phy_print_status(phydev);
> -	}
> -}
> -
>  static void nixge_tx_skb_unmap(struct nixge_priv *priv,
>  			       struct nixge_tx_skb *tx_skb)
>  {
> @@ -859,17 +846,15 @@ static void nixge_dma_err_handler(unsigned long data)
>  static int nixge_open(struct net_device *ndev)
>  {
>  	struct nixge_priv *priv = netdev_priv(ndev);
> -	struct phy_device *phy;
>  	int ret;
>  
>  	nixge_device_reset(ndev);
>  
> -	phy = of_phy_connect(ndev, priv->phy_node,
> -			     &nixge_handle_link_change, 0, priv->phy_mode);
> -	if (!phy)
> -		return -ENODEV;
> +	ret = phylink_of_phy_connect(priv->phylink, priv->dev->of_node, 0);
> +	if (ret < 0)
> +		return ret;
>  
> -	phy_start(phy);
> +	phylink_start(priv->phylink);
>  
>  	/* Enable tasklets for Axi DMA error handling */
>  	tasklet_init(&priv->dma_err_tasklet, nixge_dma_err_handler,
> @@ -893,8 +878,7 @@ static int nixge_open(struct net_device *ndev)
>  err_rx_irq:
>  	free_irq(priv->tx_irq, ndev);
>  err_tx_irq:
> -	phy_stop(phy);
> -	phy_disconnect(phy);
> +	phylink_disconnect_phy(priv->phylink);
>  	tasklet_kill(&priv->dma_err_tasklet);
>  	netdev_err(ndev, "request_irq() failed\n");
>  	return ret;
> @@ -908,9 +892,9 @@ static int nixge_stop(struct net_device *ndev)
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
>  
> -	if (ndev->phydev) {
> -		phy_stop(ndev->phydev);
> -		phy_disconnect(ndev->phydev);
> +	if (priv->phylink) {
> +		phylink_stop(priv->phylink);
> +		phylink_disconnect_phy(priv->phylink);
>  	}
>  
>  	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> @@ -1076,13 +1060,31 @@ static int nixge_ethtools_set_phys_id(struct net_device *ndev,
>  	return 0;
>  }
>  
> +static int
> +nixge_ethtool_set_link_ksettings(struct net_device *ndev,
> +				 const struct ethtool_link_ksettings *cmd)
> +{
> +	struct nixge_priv *priv = netdev_priv(ndev);
> +
> +	return phylink_ethtool_ksettings_set(priv->phylink, cmd);
> +}
> +
> +static int
> +nixge_ethtool_get_link_ksettings(struct net_device *ndev,
> +				 struct ethtool_link_ksettings *cmd)
> +{
> +	struct nixge_priv *priv = netdev_priv(ndev);
> +
> +	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
> +}
> +
>  static const struct ethtool_ops nixge_ethtool_ops = {
>  	.get_drvinfo    = nixge_ethtools_get_drvinfo,
>  	.get_coalesce   = nixge_ethtools_get_coalesce,
>  	.set_coalesce   = nixge_ethtools_set_coalesce,
>  	.set_phys_id    = nixge_ethtools_set_phys_id,
> -	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
> -	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
> +	.get_link_ksettings     = nixge_ethtool_get_link_ksettings,
> +	.set_link_ksettings     = nixge_ethtool_set_link_ksettings,
>  	.get_link		= ethtool_op_get_link,
>  };
>  
> @@ -1225,11 +1227,52 @@ static void *nixge_get_nvmem_address(struct device *dev)
>  	return mac;
>  }
>  
> +static void nixge_validate(struct net_device *ndev, unsigned long *supported,
> +			   struct phylink_link_state *state)
> +{
> +}
> +
> +static int nixge_mac_link_state(struct net_device *ndev,
> +				struct phylink_link_state *state)
> +{
> +	return 0;
> +}
> +
> +static void nixge_mac_config(struct net_device *ndev, unsigned int mode,
> +			     const struct phylink_link_state *state)
> +{
> +}
> +
> +static void nixge_mac_an_restart(struct net_device *ndev)
> +{
> +}
> +
> +static void nixge_mac_link_down(struct net_device *ndev, unsigned int mode,
> +				phy_interface_t interface)
> +{
> +}
> +
> +static void nixge_mac_link_up(struct net_device *ndev, unsigned int mode,
> +			      phy_interface_t interface,
> +			      struct phy_device *phy)
> +{
> +}
> +
> +static const struct phylink_mac_ops nixge_phylink_ops = {
> +	.validate = nixge_validate,
> +	.mac_link_state = nixge_mac_link_state,
> +	.mac_an_restart = nixge_mac_an_restart,
> +	.mac_config = nixge_mac_config,
> +	.mac_link_down = nixge_mac_link_down,
> +	.mac_link_up = nixge_mac_link_up,
> +};
> +
>  static int nixge_probe(struct platform_device *pdev)
>  {
>  	struct nixge_priv *priv;
>  	struct net_device *ndev;
>  	struct resource *dmares;
> +	struct device_node *mn;
>  	const u8 *mac_addr;
>  	int err;
>  
> @@ -1286,7 +1329,13 @@ static int nixge_probe(struct platform_device *pdev)
>  	priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
>  	priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
>  
> -	err = nixge_mdio_setup(priv, pdev->dev.of_node);
> +	mn = of_get_child_by_name(pdev->dev.of_node, "mdio");
> +	if (!mn) {
> +		dev_warn(&pdev->dev, "No \"mdio\" subnode found, defaulting to legacy\n");
> +		mn = pdev->dev.of_node;
> +	}
> +
> +	err = nixge_mdio_setup(priv, mn);
>  	if (err) {
>  		netdev_err(ndev, "error registering mdio bus");
>  		goto free_netdev;
> @@ -1299,10 +1348,10 @@ static int nixge_probe(struct platform_device *pdev)
>  		goto unregister_mdio;
>  	}
>  
> -	priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> -	if (!priv->phy_node) {
> -		netdev_err(ndev, "not find \"phy-handle\" property\n");
> -		err = -EINVAL;
> +	priv->phylink = phylink_create(ndev, pdev->dev.fwnode, priv->phy_mode,
> +				       &nixge_phylink_ops);
> +	if (IS_ERR(priv->phylink)) {
> +		err = PTR_ERR(priv->phylink);
>  		goto unregister_mdio;
>  	}
>  
> 


-- 
Florian