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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, Nov 09, 2015 at 11:32:07AM +1100, Benjamin Herrenschmidt wrote:
>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.
>

Yeah, It's something that I hilighed in the cover letter. I was
thinking we might need a better way to enable Tx/Rx before the
interrupt is up, but couldn't figure out one way. So I need some
advice here.

Thanks,
Gavin

>> +	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);
>