Re: [PATCH RFC 5/6] net/faraday: Enable NCSI interface

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2015-11-09 at 11:10 +1100, Gavin Shan wrote:
> 
> -	if (likely(netif_running(netdev))) {
> +	/* When running in NCSI mode, the interface should be
> +	 * ready to receive or transmit NCSI packet before it's
> +	 * opened.
> +	 */

No, that's not right. open/close is when a driver allocates it's data
structures, DMA ring descriptors, packet buffers etc... and request
it's interrupts.

You cannot expect packets to flow on a closed interface and you
shouldn't be getting any interrupts on a closed interface either.

> +	if (likely(priv->use_ncsi || netif_running(netdev))) {
>  		/* Disable interrupts for polling */
>  		iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
>  		napi_schedule(&priv->napi);
> @@ -1162,8 +1168,18 @@ static int ftgmac100_open(struct net_device
> *netdev)
>  
>  	/* enable all interrupts */
>  	iowrite32(INT_MASK_ALL_ENABLED, priv->base +
> FTGMAC100_OFFSET_IER);
> +	/* Start the NCSI device */
> +	if (priv->use_ncsi){
> +		err = ncsi_start_dev(priv->ndev);
> +		if (err)
> +			goto err_ncsi;
> +	}
>  	return 0;
>  
> +err_ncsi:
> +	napi_disable(&priv->napi);
> +	netif_stop_queue(netdev);
> +	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
>  err_hw:
>  	free_irq(priv->irq, netdev);
>  err_irq:
> @@ -1172,7 +1188,7 @@ err_alloc:
>  	return err;
>  }
>  
> -static int ftgmac100_stop(struct net_device *netdev)
> +static int ftgmac100_stop_dev(struct net_device *netdev)
>  {
>  	struct ftgmac100 *priv = netdev_priv(netdev);
>  
> @@ -1191,6 +1207,18 @@ static int ftgmac100_stop(struct net_device
> *netdev)
>  	return 0;
>  }
>  
> +static int ftgmac100_stop(struct net_device *netdev)
> +{
> +	struct ftgmac100 *priv = netdev_priv(netdev);
> +
> +	/* Stop NCSI device */
> +	if (priv->use_ncsi) {
> +		ncsi_stop_dev(priv->ndev);
> +		return 0;
> +	}
> +
> +	return ftgmac100_stop_dev(netdev);
> +}
>  static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
>  				     struct net_device *netdev)
>  {
> @@ -1291,6 +1319,21 @@ static const struct net_device_ops
> ftgmac100_netdev_ops = {
>  	.ndo_do_ioctl		= ftgmac100_do_ioctl,
>  };
>  
> +static void ftgmac100_ncsi_handler(struct ncsi_dev *nd)
> +{
> +	struct net_device *netdev = nd->nd_dev;
> +
> +	if (nd->nd_state != ncsi_dev_state_functional)
> +		return;
> +
> +	if (nd->nd_link_up) {
> +		pr_info("NCSI dev is up\n");
> +		netif_start_queue(netdev);
> +	} else {
> +		pr_info("NCSI dev is down\n");
> +		ftgmac100_stop_dev(netdev);
> +	}
> +}
>  /*******************************************************************
> ***********
>   * struct platform_driver functions
>  
> *********************************************************************
> ********/
> @@ -1300,7 +1343,7 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
>  	int irq;
>  	struct net_device *netdev;
>  	struct ftgmac100 *priv;
> -	int err;
> +	int err = 0;
>  
>  	if (!pdev)
>  		return -ENODEV;
> @@ -1320,7 +1363,17 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
>  		goto err_alloc_etherdev;
>  	}
>  
> +	/* Check for NCSI mode */
> +	priv = netdev_priv(netdev);
>  	SET_NETDEV_DEV(netdev, &pdev->dev);
> +	if (pdev->dev.of_node &&
> +	    of_get_property(pdev->dev.of_node, "use-nc-si", NULL)) {
> +		dev_info(&pdev->dev, "Using NCSI interface\n");
> +		priv->phydev = NULL;
> +		priv->use_ncsi = true;
> +	} else {
> +		priv->use_ncsi = false;
> +	}
>  
>  	netdev->ethtool_ops = &ftgmac100_ethtool_ops;
>  	netdev->netdev_ops = &ftgmac100_netdev_ops;
> @@ -1329,7 +1382,6 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
>  	platform_set_drvdata(pdev, netdev);
>  
>  	/* setup private data */
> -	priv = netdev_priv(netdev);
>  	priv->netdev = netdev;
>  	priv->dev = &pdev->dev;
>  
> @@ -1356,24 +1408,14 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
>  
>  	priv->irq = irq;
>  
> -	/* Check for NC-SI mode */
> -	if (pdev->dev.of_node &&
> -	    of_get_property(pdev->dev.of_node, "use-nc-si", NULL))
> -		priv->use_ncsi = true;
> -	else
> -		priv->use_ncsi = false;
> +	/* Read MAC address or setup a new one */
> +	ftgmac100_setup_mac(priv);
>  
> -	/* If we use NC-SI, we need to set that up, which isn't
> implemented yet
> -	 * so we pray things were setup by the bootloader and just
> mark our link
> -	 * as up (otherwise we can't get packets through).
> -	 *
> -	 * Eventually, we'll have a proper NC-SI stack as a helper
> we can
> -	 * instanciate
> -	 */
> +	/* Register NCSI device */
>  	if (priv->use_ncsi) {
> -		/* XXX */
> -		priv->phydev = NULL;
> -		dev_info(&pdev->dev, "Using NC-SI interface\n");
> +		priv->ndev = ncsi_register_dev(netdev,
> ftgmac100_ncsi_handler);
> +		if (!priv->ndev)
> +			goto err_ncsi_dev;
>  	} else {
>  		err = ftgmac100_setup_mdio(priv);
>  
> @@ -1384,9 +1426,6 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
>  			dev_warn(&pdev->dev, "Error %d setting up
> MDIO\n", err);
>  	}
>  
> -	/* Read MAC address or setup a new one */
> -	ftgmac100_setup_mac(priv);
> -
>  	/* Register network device */
>  	err = register_netdev(netdev);
>  	if (err) {
> @@ -1399,7 +1438,11 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
>  	return 0;
>  
>  err_register_netdev:
> -	ftgmac100_destroy_mdio(priv);
> +	if (!priv->use_ncsi)
> +		ftgmac100_destroy_mdio(priv);
> +	else
> +		ncsi_unregister_dev(priv->ndev);
> +err_ncsi_dev:
>  	iounmap(priv->base);
>  err_ioremap:
>  	release_resource(priv->res);