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

[Rami Rosen]

Hi,
One minor comment:

The return type of xge_init_hw() should be changed to be void, as
the method xge_port_reset() always returns 0;  and also the return type
of xge_port_reset() should be changed to be void, it never fails; see
in [PATCH v4 net-next 3/6] drivers: net: xgene-v2: Add ethernet
hardware configuration.


+static int xge_init_hw(struct net_device *ndev)
+{
+   struct xge_pdata *pdata = netdev_priv(ndev);
+   int ret;
+
+   ret = xge_port_reset(ndev);
+   if (ret)
+   return ret;
+
+   xge_port_init(ndev);
+   pdata->nbufs = NUM_BUFS;
+
+   return 0;
+}

Regards,
Rami Rosen

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

[Florian Fainelli]

On 03/07/2017 05:08 PM, 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 
> ---

> + 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 does not take into account all other PHY_INTERFACE_MODE_RGMII
variants, is that really intentional here?

> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret <= 0) {

0 can be a valid interrupt on some platforms AFAIR, so you may want to
just check < 0.

> + dev_err(dev, "Unable to get ENET IRQ\n");
> + ret = ret ? : -ENXIO;
> + return ret;
> + }
> + pdata->resources.irq = ret;
> +

> +static int xge_request_irq(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct device *dev = &pdata->pdev->dev;
> + int ret;
> +
> + snprintf(pdata->irq_name, IRQ_ID_SIZE, "%s", ndev->name);
> +
> + ret = devm_request_irq(dev, pdata->resources.irq, xge_irq,
> +0, pdata->irq_name, pdata);
> + if (ret)
> + netdev_err(ndev, "Failed to request irq %s\n", pdata->irq_name);

The preference for network driver is to manage the request_irq() in the
ndo_open() callback and free_irq() in the ndo_close() which kind of
defeats the purpose of using devm_* functions for that purpose.


> +static int xge_open(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + int ret;
> +
> + ret = xge_create_desc_rings(ndev);
> + if (ret)
> + return ret;
> +
> + napi_enable(&pdata->napi);
> + ret = xge_request_irq(ndev);
> + if (ret)
> + return ret;
> +
> + xge_intr_enable(pdata);
> + xge_wr_csr(pdata, DMARXCTRL, 1);
> + xge_mac_enable(pdata);
> + netif_start_queue(ndev);
> + netif_carrier_on(ndev);

Can't you use PHYLIB to get the link indication and not manage the link
state manually here? Setting netif_carrier_on() without checking the
actualy physical medium is just plain wrong/


> +static void xge_timeout(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> +
> + rtnl_lock();
> +
> + if (netif_running(ndev)) {

Reduce indention here.

> + netif_carrier_off(ndev);
> + netif_stop_queue(ndev);
> + xge_intr_disable(pdata);
> + napi_disable(&pdata->napi);
> +

> +static int xge_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct net_device *ndev;
> + struct xge_pdata *pdata;
> + int ret;
> +
> + ndev = alloc_etherdev(sizeof(struct xge_pdata));

sizeof(*pdata).
-- 
Florian

[PATCH v4 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 | 510 +++
 1 file changed, 510 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..c96b4cc
--- /dev/null
+++ b/drivers/net/ethernet/apm/xgene-v2/main.c
@@ -0,0 +1,510 @@
+/*
+ * 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 = &pdev->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 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 = &pdata->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 = &ring->raw_desc[tail];
+
+   len = XGENE_ENET_STD_MTU;
+   skb = netdev_alloc_skb(ndev, len);
+   if (unlikely(!skb))
+   return -ENOMEM;
+
+   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;
+   }
+
+   ring->pkt_info[tail].skb = skb;
+   ring->pkt_info[tail].dma_addr = dma_addr;
+
+   addr_hi = GET_BITS(NEXT_DESC_ADDRH, le64_to_cpu(raw_desc->m1));
+   addr_lo = GET_BITS(NEXT_DESC_ADDRL, le64_to_cpu(raw_desc->m1));
+   raw_desc->m1 = cpu_to_le64(SET_BITS(NEXT_DESC_ADDRL, addr_lo) |
+  SET_BITS(NEXT_DESC_ADDRH, addr_hi) |
+  SET_BITS(PKT_ADDRH,
+   dma_addr >> PKT_ADDRL_LEN));
+
+   dma_wmb();
+   raw_desc->m0 = cpu_to_le64(SET_BITS(PKT_ADDRL, dma_addr) |
+  SET_BITS(E, 1));
+   tail = (tail + 1) &