Re: [PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

[Iyappan Subramanian]

Hi Andrew,

On Tue, Jan 31, 2017 at 12:01 PM, Andrew Lunn  wrote:
>> + phy_mode = device_get_phy_mode(dev);
>> + if (phy_mode < 0) {
>> + dev_err(dev, "Unable to get phy-connection-type\n");
>> + return phy_mode;
>> + }
>> + pdata->resources.phy_mode = phy_mode;
>> +
>> + if (pdata->resources.phy_mode != PHY_INTERFACE_MODE_RGMII) {
>> + dev_err(dev, "Incorrect phy-connection-type specified\n");
>> + return -ENODEV;
>> + }
>
> This seems a bit limiting. What if you need to use:
>
> PHY_INTERFACE_MODE_RGMII_ID,
> PHY_INTERFACE_MODE_RGMII_RXID,
> PHY_INTERFACE_MODE_RGMII_TXID,
>
> in order to set the RGMII delays.

This version of the driver doesn't support setting delays.  The delay
support will be added in the future.

>
>Andrew
>

Re: [PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

[Iyappan Subramanian]

Hi Florian,

On Tue, Jan 31, 2017 at 12:31 PM, Florian Fainelli  wrote:
> On 01/31/2017 11:03 AM, Iyappan Subramanian wrote:
>> This patch adds,
>>
>>  - probe, remove, shutdown
>>  - open, close and stats
>>  - create and delete ring
>>  - request and delete irq
>>
>> Signed-off-by: Iyappan Subramanian 
>> Signed-off-by: Keyur Chudgar 
>> ---
>
>> +static void xge_delete_desc_rings(struct net_device *ndev)
>> +{
>> + struct xge_pdata *pdata = netdev_priv(ndev);
>> + struct device *dev = >pdev->dev;
>> + struct xge_desc_ring *ring;
>> +
>> + ring = pdata->tx_ring;
>> + if (ring) {
>> + if (ring->skbs)
>> + devm_kfree(dev, ring->skbs);
>> + if (ring->pkt_bufs)
>> + devm_kfree(dev, ring->pkt_bufs);
>> + devm_kfree(dev, ring);
>> + }
>
> The very fact that you have to do the devm_kfree suggests that the way
> you manage the lifetime of the ring is not appropriate, and in fact, if
> we look at how xge_create_desc_ring() is called, in the driver's probe
> function indicates that if the network interface is never openeded, we
> are just wasting memory sitting there and doing nothing. You should
> consider moving this to the ndo_open(), resp. ndo_close() functions to
> optimize memory consumption wrt. the network interface state.

I will move these to open and close functions and will use dma_zalloc() APIs.

>
>> +
>> + ring = pdata->rx_ring;
>> + if (ring) {
>> + if (ring->skbs)
>> + devm_kfree(dev, ring->skbs);
>> + devm_kfree(dev, ring);
>> + }
>> +}
>> +
>> +static struct xge_desc_ring *xge_create_desc_ring(struct net_device *ndev)
>> +{
>> + struct xge_pdata *pdata = netdev_priv(ndev);
>> + struct device *dev = >pdev->dev;
>> + struct xge_desc_ring *ring;
>> + u16 size;
>> +
>> + ring = devm_kzalloc(dev, sizeof(struct xge_desc_ring), GFP_KERNEL);
>> + if (!ring)
>> + return NULL;
>> +
>> + ring->ndev = ndev;
>> +
>> + size = XGENE_ENET_DESC_SIZE * XGENE_ENET_NUM_DESC;
>> + ring->desc_addr = dmam_alloc_coherent(dev, size, >dma_addr,
>> +   GFP_KERNEL | __GFP_ZERO);
>
> There is no dmam_zalloc_coherent()? Then again, that seems to be a
> candidate for dma_zalloc_coherent() and moving this to the ndo_open()
> function.
>
>> + if (!ring->desc_addr) {
>> + devm_kfree(dev, ring);
>> + return NULL;
>> + }
>> +
>> + xge_setup_desc(ring);
>> +
>> + return ring;
>> +}
>> +
>> +static int xge_refill_buffers(struct net_device *ndev, u32 nbuf)
>> +{
>> + struct xge_pdata *pdata = netdev_priv(ndev);
>> + struct xge_desc_ring *ring = pdata->rx_ring;
>> + const u8 slots = XGENE_ENET_NUM_DESC - 1;
>> + struct device *dev = >pdev->dev;
>> + struct xge_raw_desc *raw_desc;
>> + u64 addr_lo, addr_hi;
>> + u8 tail = ring->tail;
>> + struct sk_buff *skb;
>> + dma_addr_t dma_addr;
>> + u16 len;
>> + int i;
>> +
>> + for (i = 0; i < nbuf; i++) {
>> + raw_desc = >raw_desc[tail];
>> +
>> + len = XGENE_ENET_STD_MTU;
>> + skb = netdev_alloc_skb(ndev, len);
>> + if (unlikely(!skb))
>> + return -ENOMEM;
>
> Are not you leaving holes in your RX ring if you do that?

No.  The probe will fail and clean up the unused buffers.

>
>> +
>> + dma_addr = dma_map_single(dev, skb->data, len, 
>> DMA_FROM_DEVICE);
>> + if (dma_mapping_error(dev, dma_addr)) {
>> + netdev_err(ndev, "DMA mapping error\n");
>> + dev_kfree_skb_any(skb);
>> + return -EINVAL;
>> + }
>
>
>> +static void xge_timeout(struct net_device *ndev)
>> +{
>> + struct xge_pdata *pdata = netdev_priv(ndev);
>> + struct netdev_queue *txq;
>> +
>> + xge_mac_reset(pdata);
>> +
>> + txq = netdev_get_tx_queue(ndev, 0);
>> + txq->trans_start = jiffies;
>> + netif_tx_start_queue(txq);
>
> It most likely is not that simple, don't you want to walk the list of
> pending transmissed SKBs and free them all?

I'll add more exhaustive clean up and restart of Tx hardware.

>
>> +}
>> +
>> +static void xge_get_stats64(struct net_device *ndev,
>> + struct rtnl_link_stats64 *storage)
>> +{
>> + struct xge_pdata *pdata = netdev_priv(ndev);
>> + struct xge_stats *stats = >stats;
>> +
>> + storage->tx_packets += stats->tx_packets;
>> + storage->tx_bytes += stats->tx_bytes;
>> +
>> + storage->rx_packets += stats->rx_packets;
>> + storage->rx_bytes += stats->rx_bytes;
>
> Pretty sure you need some synchronization primitives here for non 64-bit
> architectures (maybe this driver is not used outside of 64-bit, but still).

Synchronization primitives are not required for this 

Re: [PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

[Florian Fainelli]

On 01/31/2017 11:03 AM, Iyappan Subramanian wrote:
> This patch adds,
> 
>  - probe, remove, shutdown
>  - open, close and stats
>  - create and delete ring
>  - request and delete irq
> 
> Signed-off-by: Iyappan Subramanian 
> Signed-off-by: Keyur Chudgar 
> ---

> +static void xge_delete_desc_rings(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct device *dev = >pdev->dev;
> + struct xge_desc_ring *ring;
> +
> + ring = pdata->tx_ring;
> + if (ring) {
> + if (ring->skbs)
> + devm_kfree(dev, ring->skbs);
> + if (ring->pkt_bufs)
> + devm_kfree(dev, ring->pkt_bufs);
> + devm_kfree(dev, ring);
> + }

The very fact that you have to do the devm_kfree suggests that the way
you manage the lifetime of the ring is not appropriate, and in fact, if
we look at how xge_create_desc_ring() is called, in the driver's probe
function indicates that if the network interface is never openeded, we
are just wasting memory sitting there and doing nothing. You should
consider moving this to the ndo_open(), resp. ndo_close() functions to
optimize memory consumption wrt. the network interface state.

> +
> + ring = pdata->rx_ring;
> + if (ring) {
> + if (ring->skbs)
> + devm_kfree(dev, ring->skbs);
> + devm_kfree(dev, ring);
> + }
> +}
> +
> +static struct xge_desc_ring *xge_create_desc_ring(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct device *dev = >pdev->dev;
> + struct xge_desc_ring *ring;
> + u16 size;
> +
> + ring = devm_kzalloc(dev, sizeof(struct xge_desc_ring), GFP_KERNEL);
> + if (!ring)
> + return NULL;
> +
> + ring->ndev = ndev;
> +
> + size = XGENE_ENET_DESC_SIZE * XGENE_ENET_NUM_DESC;
> + ring->desc_addr = dmam_alloc_coherent(dev, size, >dma_addr,
> +   GFP_KERNEL | __GFP_ZERO);

There is no dmam_zalloc_coherent()? Then again, that seems to be a
candidate for dma_zalloc_coherent() and moving this to the ndo_open()
function.

> + if (!ring->desc_addr) {
> + devm_kfree(dev, ring);
> + return NULL;
> + }
> +
> + xge_setup_desc(ring);
> +
> + return ring;
> +}
> +
> +static int xge_refill_buffers(struct net_device *ndev, u32 nbuf)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct xge_desc_ring *ring = pdata->rx_ring;
> + const u8 slots = XGENE_ENET_NUM_DESC - 1;
> + struct device *dev = >pdev->dev;
> + struct xge_raw_desc *raw_desc;
> + u64 addr_lo, addr_hi;
> + u8 tail = ring->tail;
> + struct sk_buff *skb;
> + dma_addr_t dma_addr;
> + u16 len;
> + int i;
> +
> + for (i = 0; i < nbuf; i++) {
> + raw_desc = >raw_desc[tail];
> +
> + len = XGENE_ENET_STD_MTU;
> + skb = netdev_alloc_skb(ndev, len);
> + if (unlikely(!skb))
> + return -ENOMEM;

Are not you leaving holes in your RX ring if you do that?

> +
> + dma_addr = dma_map_single(dev, skb->data, len, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, dma_addr)) {
> + netdev_err(ndev, "DMA mapping error\n");
> + dev_kfree_skb_any(skb);
> + return -EINVAL;
> + }


> +static void xge_timeout(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct netdev_queue *txq;
> +
> + xge_mac_reset(pdata);
> +
> + txq = netdev_get_tx_queue(ndev, 0);
> + txq->trans_start = jiffies;
> + netif_tx_start_queue(txq);

It most likely is not that simple, don't you want to walk the list of
pending transmissed SKBs and free them all?

> +}
> +
> +static void xge_get_stats64(struct net_device *ndev,
> + struct rtnl_link_stats64 *storage)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct xge_stats *stats = >stats;
> +
> + storage->tx_packets += stats->tx_packets;
> + storage->tx_bytes += stats->tx_bytes;
> +
> + storage->rx_packets += stats->rx_packets;
> + storage->rx_bytes += stats->rx_bytes;

Pretty sure you need some synchronization primitives here for non 64-bit
architectures (maybe this driver is not used outside of 64-bit, but still).


> +
> + ndev->hw_features = ndev->features;
> +
> + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
> + if (ret) {
> + netdev_err(ndev, "No usable DMA configuration\n");
> + goto err;
> + }
> +
> + ret = xge_init_hw(ndev);
> + if (ret)
> + goto err;

Missing netif_carrier_off() right before the register_netdev().

> +
> + ret = register_netdev(ndev);
> + if (ret) {
> + netdev_err(ndev, "Failed to register 

Re: [PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

[Andrew Lunn]

> + phy_mode = device_get_phy_mode(dev);
> + if (phy_mode < 0) {
> + dev_err(dev, "Unable to get phy-connection-type\n");
> + return phy_mode;
> + }
> + pdata->resources.phy_mode = phy_mode;
> +
> + if (pdata->resources.phy_mode != PHY_INTERFACE_MODE_RGMII) {
> + dev_err(dev, "Incorrect phy-connection-type specified\n");
> + return -ENODEV;
> + }

This seems a bit limiting. What if you need to use:

PHY_INTERFACE_MODE_RGMII_ID,
PHY_INTERFACE_MODE_RGMII_RXID,
PHY_INTERFACE_MODE_RGMII_TXID,

in order to set the RGMII delays.

   Andrew

[PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

[Iyappan Subramanian]

This patch adds,

 - probe, remove, shutdown
 - open, close and stats
 - create and delete ring
 - request and delete irq

Signed-off-by: Iyappan Subramanian 
Signed-off-by: Keyur Chudgar 
---
 drivers/net/ethernet/apm/xgene-v2/main.c | 459 +++
 1 file changed, 459 insertions(+)
 create mode 100644 drivers/net/ethernet/apm/xgene-v2/main.c

diff --git a/drivers/net/ethernet/apm/xgene-v2/main.c 
b/drivers/net/ethernet/apm/xgene-v2/main.c
new file mode 100644
index 000..3881f27
--- /dev/null
+++ b/drivers/net/ethernet/apm/xgene-v2/main.c
@@ -0,0 +1,459 @@
+/*
+ * Applied Micro X-Gene SoC Ethernet v2 Driver
+ *
+ * Copyright (c) 2017, Applied Micro Circuits Corporation
+ * Author(s): Iyappan Subramanian 
+ *   Keyur Chudgar 
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include "main.h"
+
+static const struct acpi_device_id xge_acpi_match[];
+
+static int xge_get_resources(struct xge_pdata *pdata)
+{
+   struct platform_device *pdev;
+   struct net_device *ndev;
+   struct device *dev;
+   struct resource *res;
+   int phy_mode, ret = 0;
+
+   pdev = pdata->pdev;
+   dev = >dev;
+   ndev = pdata->ndev;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   if (!res) {
+   dev_err(dev, "Resource enet_csr not defined\n");
+   return -ENODEV;
+   }
+
+   pdata->resources.base_addr = devm_ioremap(dev, res->start,
+ resource_size(res));
+   if (!pdata->resources.base_addr) {
+   dev_err(dev, "Unable to retrieve ENET Port CSR region\n");
+   return -ENOMEM;
+   }
+
+   if (!device_get_mac_address(dev, ndev->dev_addr, ETH_ALEN))
+   eth_hw_addr_random(ndev);
+
+   memcpy(ndev->perm_addr, ndev->dev_addr, ndev->addr_len);
+
+   phy_mode = device_get_phy_mode(dev);
+   if (phy_mode < 0) {
+   dev_err(dev, "Unable to get phy-connection-type\n");
+   return phy_mode;
+   }
+   pdata->resources.phy_mode = phy_mode;
+
+   if (pdata->resources.phy_mode != PHY_INTERFACE_MODE_RGMII) {
+   dev_err(dev, "Incorrect phy-connection-type specified\n");
+   return -ENODEV;
+   }
+
+   ret = platform_get_irq(pdev, 0);
+   if (ret <= 0) {
+   dev_err(dev, "Unable to get ENET IRQ\n");
+   ret = ret ? : -ENXIO;
+   return ret;
+   }
+   pdata->resources.irq = ret;
+
+   return 0;
+}
+
+static void xge_delete_desc_rings(struct net_device *ndev)
+{
+   struct xge_pdata *pdata = netdev_priv(ndev);
+   struct device *dev = >pdev->dev;
+   struct xge_desc_ring *ring;
+
+   ring = pdata->tx_ring;
+   if (ring) {
+   if (ring->skbs)
+   devm_kfree(dev, ring->skbs);
+   if (ring->pkt_bufs)
+   devm_kfree(dev, ring->pkt_bufs);
+   devm_kfree(dev, ring);
+   }
+
+   ring = pdata->rx_ring;
+   if (ring) {
+   if (ring->skbs)
+   devm_kfree(dev, ring->skbs);
+   devm_kfree(dev, ring);
+   }
+}
+
+static struct xge_desc_ring *xge_create_desc_ring(struct net_device *ndev)
+{
+   struct xge_pdata *pdata = netdev_priv(ndev);
+   struct device *dev = >pdev->dev;
+   struct xge_desc_ring *ring;
+   u16 size;
+
+   ring = devm_kzalloc(dev, sizeof(struct xge_desc_ring), GFP_KERNEL);
+   if (!ring)
+   return NULL;
+
+   ring->ndev = ndev;
+
+   size = XGENE_ENET_DESC_SIZE * XGENE_ENET_NUM_DESC;
+   ring->desc_addr = dmam_alloc_coherent(dev, size, >dma_addr,
+ GFP_KERNEL | __GFP_ZERO);
+   if (!ring->desc_addr) {
+   devm_kfree(dev, ring);
+   return NULL;
+   }
+
+   xge_setup_desc(ring);
+
+   return ring;
+}
+
+static int xge_refill_buffers(struct net_device *ndev, u32 nbuf)
+{
+   struct xge_pdata *pdata = netdev_priv(ndev);
+   struct xge_desc_ring *ring = pdata->rx_ring;
+   const u8 slots = XGENE_ENET_NUM_DESC - 1;
+   struct device *dev = >pdev->dev;
+   struct xge_raw_desc *raw_desc;
+