Message-ID: <570EC541.6080603@gmail.com>
Date:	Wed, 13 Apr 2016 15:16:33 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Timur Tabi <timur@...eaurora.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, sdharia@...eaurora.org,
	Shanker Donthineni <shankerd@...eaurora.org>,
	Greg Kroah-Hartman <greg@...ah.com>, vikrams@...eaurora.org,
	cov@...eaurora.org, gavidov@...eaurora.org,
	Rob Herring <robh+dt@...nel.org>, andrew@...n.ch,
	bjorn.andersson@...aro.org, Mark Langsdorf <mlangsdo@...hat.com>,
	Jon Masters <jcm@...hat.com>,
	Andy Gross <agross@...eaurora.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller
 driver

On 13/04/16 10:59, Timur Tabi wrote:
> From: Gilad Avidov <gavidov@...eaurora.org>
> 
> Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
> This driver supports the following features:
> 1) Checksum offload.
> 2) Runtime power management support.
> 3) Interrupt coalescing support.
> 4) SGMII phy.
> 5) SGMII direct connection without external phy.

I think you should shoot for more simple for an initial submission:

- no offload
- no timestamping

get that accepted, and then add features one by one, it sure is more
work, but it helps with the review, and makes you work off a solid base.

You will see below, but a pet peeve of mine is authors reimplementing
code that exists in PHYLIB.

[snip]

> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt b/Documentation/devicetree/bindings/net/qcom-emac.txt
> new file mode 100644
> index 0000000..df5e7c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
> @@ -0,0 +1,65 @@
> +Qualcomm EMAC Gigabit Ethernet Controller
> +
> +Required properties:
> +- compatible : Should be "qcom,emac".
> +- reg : Offset and length of the register regions for the device
> +- reg-names : Register region names referenced in 'reg' above.
> +	Required register resource entries are:
> +	"base"   : EMAC controller base register block.
> +	"csr"    : EMAC wrapper register block.
> +	Optional register resource entries are:
> +	"ptp"    : EMAC PTP (1588) register block.
> +		   Required if 'qcom,emac-tstamp-en' is present.
> +	"sgmii"  : EMAC SGMII PHY register block.
> +- interrupts : Interrupt numbers used by this controller
> +- interrupt-names : Interrupt resource names referenced in 'interrupts' above.
> +	Required interrupt resource entries are:
> +	"emac_core0"   : EMAC core0 interrupt.
> +	"sgmii_irq"   : EMAC SGMII interrupt.
> +- phy-addr            : Specifies phy address on MDIO bus.
> +			Required if the optional property "qcom,no-external-phy"
> +			is not specified.

This is not the standard way to represent an Ethernet PHY hanging off a
MDIO bus see ethernet.txt and phy.txt in D/dt/bindings/net/

> +
> +Optional properties:
> +- qcom,emac-tstamp-en       : Enables the PTP (1588) timestamping feature.
> +			      Include this only if PTP (1588) timestamping
> +			      feature is needed. If included, "ptp" register
> +			      base should be specified.

If the "ptp" register range is not specified, then PTP gets disabled, so
is a boolean really required here, considering that this looks like a
policy decision more than anything.

> +- mac-address               : The 6-byte MAC address. If present, it is the
> +			      default MAC address.

This property is pretty much mandatory

> +- qcom,no-external-phy      : Indicates there is no external PHY connected to
> +			      EMAC. Include this only if the EMAC is directly
> +			      connected to the peer end without EPHY.

How is the internal PHY accessed, is it responding on the MDIO bus at a
particular address? If so, standard MDIO scanning/probing works, and you
can have your PHY driver flag this device has internal. Worst case, you
can do what BCMGENET does, and have a special "phy-mode" value set to
"internal" when this knowledge needs to exist prior to MDIO bus scanning
(e.g: to power on the PHY).

> +Example:
> +	emac0: qcom,emac@...20000 {
> +		compatible = "qcom,fsm9900-emac";
> +		reg-names = "base", "csr", "ptp", "sgmii";
> +		reg =   <0xfeb20000 0x10000>,
> +			<0xfeb36000 0x1000>,
> +			<0xfeb3c000 0x4000>,
> +			<0xfeb38000 0x400>;
> +		#address-cells = <0>;
> +		interrupt-parent = <&emac0>;
> +		#interrupt-cells = <1>;
> +		interrupts = <0 1>;
> +		interrupt-map-mask = <0xffffffff>;
> +		interrupt-map = <0 &intc 0 76 0
> +				 1 &intc 0 80 0>;
> +		interrupt-names = "emac_core0", "sgmii_irq";
> +		qcom,emac-tstamp-en;
> +		phy-addr = <0>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&mdio_pins_a>;
> +	};
> +
> +	tlmm: pinctrl@...10000 {
> +		compatible = "qcom,fsm9900-pinctrl";
> +
> +		mdio_pins_a: mdio {
> +			state {
> +				pins = "gpio123", "gpio124";
> +				function = "mdio";
> +			};
> +		};
> +	};
> diff --git a/drivers/net/ethernet/qualcomm/Kconfig b/drivers/net/ethernet/qualcomm/Kconfig
> index a76e380..85b599f 100644
> --- a/drivers/net/ethernet/qualcomm/Kconfig
> +++ b/drivers/net/ethernet/qualcomm/Kconfig
> @@ -24,4 +24,15 @@ config QCA7000
>  	  To compile this driver as a module, choose M here. The module
>  	  will be called qcaspi.
>  
> +config QCOM_EMAC
> +	tristate "Qualcomm Technologies, Inc. EMAC Gigabit Ethernet support"
> +	select CRC32
> +	---help---
> +	  This driver supports the Qualcomm Technologies, Inc. Gigabit
> +	  Ethernet Media Access Controller (EMAC). The controller
> +	  supports IEEE 802.3-2002, half-duplex mode at 10/100 Mb/s,
> +	  full-duplex mode at 10/100/1000Mb/s, Wake On LAN (WOL) for
> +	  low power, Receive-Side Scaling (RSS), and IEEE 1588-2008
> +	  Precision Clock Synchronization Protocol.
> +
>  endif # NET_VENDOR_QUALCOMM

[snip]

> +/* Config MAC modes */
> +void emac_mac_mode_config(struct emac_adapter *adpt)
> +{
> +	u32 mac;
> +
> +	mac = readl(adpt->base + EMAC_MAC_CTRL);
> +
> +	if (test_bit(EMAC_STATUS_VLANSTRIP_EN, &adpt->status))
> +		mac |= VLAN_STRIP;
> +	else
> +		mac &= ~VLAN_STRIP;
> +
> +	if (test_bit(EMAC_STATUS_PROMISC_EN, &adpt->status))
> +		mac |= PROM_MODE;
> +	else
> +		mac &= ~PROM_MODE;
> +
> +	if (test_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status))
> +		mac |= MULTI_ALL;
> +	else
> +		mac &= ~MULTI_ALL;
> +
> +	if (test_bit(EMAC_STATUS_LOOPBACK_EN, &adpt->status))
> +		mac |= MAC_LP_EN;
> +	else
> +		mac &= ~MAC_LP_EN;

Do you need to maintain these flags when most, if not all of them
already exist in dev->flags or dev->features?

[snip]

> +	/* setup link speed */
> +	mac &= ~SPEED_BMSK;
> +	switch (phy->link_speed) {
> +	case EMAC_LINK_SPEED_1GB_FULL:
> +		mac |= ((emac_mac_speed_1000 << SPEED_SHFT) & SPEED_BMSK);
> +		csr1 |= FREQ_MODE;
> +		break;
> +	default:
> +		mac |= ((emac_mac_speed_10_100 << SPEED_SHFT) & SPEED_BMSK);
> +		csr1 &= ~FREQ_MODE;
> +		break;
> +	}

If you implement the driver using PHYLIB, which you should in order to
support arbitrary or internal PHYs, then this function gets invoked
whenever there is a link parameter change (auto-neg, forcing,
duplex/speed/no link etc.).

[snip]

> +	napi_enable(&adpt->rx_q.napi);
> +
> +	/* enable mac irq */
> +	writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
> +	writel(adpt->irq.mask, adpt->base + EMAC_INT_MASK);
> +
> +	netif_start_queue(netdev);

Starting the TX queue is typically the last ting you want to do, to
avoid a transient state where the TX queue is enabled, and the link is
not (which is okay if your driver is properly implemented and reflects
carrier changes anyway).

> +	clear_bit(EMAC_STATUS_DOWN, &adpt->status);
> +
> +	/* check link status */
> +	set_bit(EMAC_STATUS_TASK_LSC_REQ, &adpt->status);
> +	adpt->link_chk_timeout = jiffies + EMAC_TRY_LINK_TIMEOUT;
> +	mod_timer(&adpt->timers, jiffies);

Please implement a PHYLIB driver and use phy_start() here.

> +
> +	return 0;
> +}
> +
> +/* Bring down the interface/HW */
> +void emac_mac_down(struct emac_adapter *adpt, bool reset)
> +{
> +	struct net_device *netdev = adpt->netdev;
> +	struct emac_phy *phy = &adpt->phy;
> +	unsigned long flags;
> +
> +	set_bit(EMAC_STATUS_DOWN, &adpt->status);

Do you need to maintain that? Would not netif_running() tell you what
you want if you reflect the carrier state properly?

> +
> +	netif_stop_queue(netdev);
> +	netif_carrier_off(netdev);

phy_stop() would take care of the latter.

[snip]

> +/* Process transmit event */
> +void emac_mac_tx_process(struct emac_adapter *adpt, struct emac_tx_queue *tx_q)
> +{
> +	struct emac_buffer *tpbuf;
> +	u32 hw_consume_idx;
> +	u32 pkts_compl = 0, bytes_compl = 0;
> +	u32 reg = readl_relaxed(adpt->base + tx_q->consume_reg);
> +
> +	hw_consume_idx = (reg & tx_q->consume_mask) >> tx_q->consume_shift;
> +
> +	while (tx_q->tpd.consume_idx != hw_consume_idx) {
> +		tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.consume_idx);
> +		if (tpbuf->dma_addr) {
> +			dma_unmap_single(adpt->netdev->dev.parent,
> +					 tpbuf->dma_addr, tpbuf->length,
> +					 DMA_TO_DEVICE);
> +			tpbuf->dma_addr = 0;
> +		}
> +
> +		if (tpbuf->skb) {
> +			pkts_compl++;
> +			bytes_compl += tpbuf->skb->len;
> +			dev_kfree_skb_irq(tpbuf->skb);
> +			tpbuf->skb = NULL;
> +		}
> +
> +		if (++tx_q->tpd.consume_idx == tx_q->tpd.count)
> +			tx_q->tpd.consume_idx = 0;
> +	}
> +
> +	if (pkts_compl || bytes_compl)
> +		netdev_completed_queue(adpt->netdev, pkts_compl, bytes_compl);

The condition can be eliminated.

[snip]

> +	if (skb_network_offset(skb) != ETH_HLEN)
> +		TPD_TYP_SET(&tpd, 1);
> +
> +	emac_tx_fill_tpd(adpt, tx_q, skb, &tpd);
> +
> +	netdev_sent_queue(adpt->netdev, skb->len);
> +
> +	/* update produce idx */
> +	prod_idx = (tx_q->tpd.produce_idx << tx_q->produce_shift) &
> +		    tx_q->produce_mask;
> +	emac_reg_update32(adpt->base + tx_q->produce_reg,
> +			  tx_q->produce_mask, prod_idx);

Since you have a producer index, you should consider checking
skb->xmit_more to know whether you can update the register now, or
later, which could save some expensive operation and batch TX.

[snip]

> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
> new file mode 100644
> index 0000000..7d18de3
> --- /dev/null
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c

This file is really really ugly, and duplicates a lot of functionality
provided by PHYLIB, you really need to implement a PHYLIB MDIO driver
and eventually a small PHY driver for your internal PHY if it needs some
baby sitting.
[snip]

> diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
> new file mode 100644
> index 0000000..ce328f5
> --- /dev/null
> +++ b/drivers/net/ethernet/qualcomm/emac/emac.c
> @@ -0,0 +1,1206 @@
> +/* Copyright (c) 2013-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +/* Qualcomm Technologies, Inc. EMAC Gigabit Ethernet Driver
> + * The EMAC driver supports following features:
> + * 1) Receive Side Scaling (RSS).
> + * 2) Checksum offload.
> + * 3) Multiple PHY support on MDIO bus.
> + * 4) Runtime power management support.
> + * 5) Interrupt coalescing support.
> + * 6) SGMII phy.
> + * 7) SGMII direct connection (without external phy).
> + */
> +
> +#include <linux/if_ether.h>
> +#include <linux/if_vlan.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_net.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include "emac.h"
> +#include "emac-mac.h"
> +#include "emac-phy.h"
> +#include "emac-sgmii.h"
> +
> +#define DRV_VERSION "1.3.0.0"
> +
> +static int debug = -1;
> +module_param(debug, int, S_IRUGO | S_IWUSR | S_IWGRP);

ethtool -s <iface> msglvl provides you with that already.

> +
> +static int emac_irq_use_extended;
> +module_param(emac_irq_use_extended, int, S_IRUGO | S_IWUSR | S_IWGRP);

What is that module parameter used for?

> +
> +const char emac_drv_name[] = "qcom-emac";
> +const char emac_drv_description[] =
> +			"Qualcomm Technologies, Inc. EMAC Ethernet Driver";
> +const char emac_drv_version[] = DRV_VERSION;

Static all other the place?

[snip]

> +
> +/* NAPI */
> +static int emac_napi_rtx(struct napi_struct *napi, int budget)
> +{
> +	struct emac_rx_queue *rx_q = container_of(napi, struct emac_rx_queue,
> +						   napi);
> +	struct emac_adapter *adpt = netdev_priv(rx_q->netdev);
> +	struct emac_irq *irq = rx_q->irq;
> +
> +	int work_done = 0;
> +
> +	/* Keep link state information with original netdev */
> +	if (!netif_carrier_ok(adpt->netdev))
> +		goto quit_polling;

I do not think this is a condition that could occur?

> +
> +	emac_mac_rx_process(adpt, rx_q, &work_done, budget);
> +
> +	if (work_done < budget) {
> +quit_polling:
> +		napi_complete(napi);
> +
> +		irq->mask |= rx_q->intr;
> +		writel(irq->mask, adpt->base + EMAC_INT_MASK);
> +	}
> +
> +	return work_done;
> +}
> +
> +/* Transmit the packet */
> +static int emac_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +
> +	return emac_mac_tx_buf_send(adpt, &adpt->tx_q, skb);

I would inline emac_mac_tx_buf_send()'s body here to make it much easier
to read and audit...

> +}
> +
> +irqreturn_t emac_isr(int _irq, void *data)
> +{
> +	struct emac_irq *irq = data;
> +	struct emac_adapter *adpt = container_of(irq, struct emac_adapter, irq);
> +	struct emac_rx_queue *rx_q = &adpt->rx_q;
> +
> +	int max_ints = 1;
> +	u32 isr, status;
> +
> +	/* disable the interrupt */
> +	writel(0, adpt->base + EMAC_INT_MASK);
> +
> +	do {

With max_ints = 1, this is essentially the same as no loop, so just
inline it to reduce the indentation.

> +		isr = readl_relaxed(adpt->base + EMAC_INT_STATUS);
> +		status = isr & irq->mask;
> +
> +		if (status == 0)
> +			break;
> +
> +		if (status & ISR_ERROR) {
> +			netif_warn(adpt,  intr, adpt->netdev,
> +				   "warning: error irq status 0x%lx\n",
> +				   status & ISR_ERROR);
> +			/* reset MAC */
> +			set_bit(EMAC_STATUS_TASK_REINIT_REQ, &adpt->status);
> +			emac_work_thread_reschedule(adpt);
> +		}
> +
> +		/* Schedule the napi for receive queue with interrupt
> +		 * status bit set
> +		 */
> +		if ((status & rx_q->intr)) {
> +			if (napi_schedule_prep(&rx_q->napi)) {
> +				irq->mask &= ~rx_q->intr;
> +				__napi_schedule(&rx_q->napi);
> +			}
> +		}
> +
> +		if (status & TX_PKT_INT)
> +			emac_mac_tx_process(adpt, &adpt->tx_q);

You should consider using a NAPI instance for reclaiming TX buffers as well.

> +
> +		if (status & ISR_OVER)
> +			netif_warn(adpt, intr, adpt->netdev,
> +				   "warning: TX/RX overflow status 0x%lx\n",
> +				   status & ISR_OVER);

This should be ratelimited presumably

> +
> +		/* link event */
> +		if (status & (ISR_GPHY_LINK | SW_MAN_INT)) {
> +			emac_lsc_schedule_check(adpt);
> +			break;
> +		}
> +	} while (--max_ints > 0);
> +
> +	/* enable the interrupt */
> +	writel(irq->mask, adpt->base + EMAC_INT_MASK);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Configure VLAN tag strip/insert feature */
> +static int emac_set_features(struct net_device *netdev,
> +			     netdev_features_t features)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +
> +	netdev_features_t changed = features ^ netdev->features;
> +
> +	if (!(changed & (NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX)))
> +		return 0;
> +
> +	netdev->features = features;
> +	if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
> +		set_bit(EMAC_STATUS_VLANSTRIP_EN, &adpt->status);
> +	else
> +		clear_bit(EMAC_STATUS_VLANSTRIP_EN, &adpt->status);

What about TX vlan offload?

[snip]

> +
> +/* Called when the network interface is made active */
> +static int emac_open(struct net_device *netdev)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +	int ret;
> +
> +	netif_carrier_off(netdev);

That seems unnecessary here because your close/down function does that,
and with PHYLIB you would get it set correctly anyway.

[snip]

> +/* PHY related IOCTLs */
> +static int emac_mii_ioctl(struct net_device *netdev,
> +			  struct ifreq *ifr, int cmd)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +	struct emac_phy *phy = &adpt->phy;
> +	struct mii_ioctl_data *data = if_mii(ifr);
> +
> +	switch (cmd) {
> +	case SIOCGMIIPHY:
> +		data->phy_id = phy->addr;
> +		return 0;
> +
> +	case SIOCGMIIREG:
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		if (data->reg_num & ~(0x1F))
> +			return -EFAULT;
> +
> +		if (data->phy_id >= PHY_MAX_ADDR)
> +			return -EFAULT;
> +
> +		if (phy->external && data->phy_id != phy->addr)
> +			return -EFAULT;
> +
> +		return emac_phy_read(adpt, data->phy_id, data->reg_num,
> +				     &data->val_out);
> +
> +	case SIOCSMIIREG:
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		if (data->reg_num & ~(0x1F))
> +			return -EFAULT;
> +
> +		if (data->phy_id >= PHY_MAX_ADDR)
> +			return -EFAULT;
> +
> +		if (phy->external && data->phy_id != phy->addr)
> +			return -EFAULT;
> +
> +		return emac_phy_write(adpt, data->phy_id, data->reg_num,
> +				      data->val_in);
> +	default:
> +		return -EFAULT;
> +	}

All of that can be eliminated with a PHYLIB implementation too.

[snip]

> +/* Provide network statistics info for the interface */
> +struct rtnl_link_stats64 *emac_get_stats64(struct net_device *netdev,
> +					   struct rtnl_link_stats64 *net_stats)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +	struct emac_stats *stats = &adpt->stats;
> +	u16 addr = REG_MAC_RX_STATUS_BIN;
> +	u64 *stats_itr = &adpt->stats.rx_ok;
> +	u32 val;
> +
> +	while (addr <= REG_MAC_RX_STATUS_END) {
> +		val = readl_relaxed(adpt->base + addr);
> +		*stats_itr += val;
> +		++stats_itr;
> +		addr += sizeof(u32);
> +	}

There is no reader locking here, what happens if two applications read
the statistics at the same time?

[snip]

> +/* Get the resources */
> +static int emac_probe_resources(struct platform_device *pdev,
> +				struct emac_adapter *adpt)
> +{
> +	struct net_device *netdev = adpt->netdev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct resource *res;
> +	const void *maddr;
> +	int ret = 0;
> +	int i;
> +
> +	/* get time stamp enable flag */
> +	adpt->timestamp_en = of_property_read_bool(node, "qcom,emac-tstamp-en");
> +
> +	/* get mac address */
> +	maddr = of_get_mac_address(node);
> +	if (!maddr)
> +		return -ENODEV;

No, generate a random one, continue, but warn,

> +
> +	memcpy(adpt->mac_perm_addr, maddr, netdev->addr_len);
> +
> +	ret = platform_get_irq_byname(pdev, EMAC_MAC_IRQ_RES);
> +	if (ret < 0) {
> +		netdev_err(adpt->netdev,
> +			   "error: missing %s resource\n", EMAC_MAC_IRQ_RES);
> +		return ret;
> +	}
> +	adpt->irq.irq = ret;
> +
> +	ret = emac_clks_get(pdev, adpt);
> +	if (ret)
> +		return ret;
> +
> +	/* get register addresses */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base");
> +	if (!res) {
> +		netdev_err(adpt->netdev, "error: missing 'base' resource\n");
> +		ret = -ENXIO;
> +		goto err_reg_res;
> +	}
> +
> +	adpt->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!adpt->base) {
> +		ret = -ENOMEM;
> +		goto err_reg_res;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
> +	if (!res) {
> +		netdev_err(adpt->netdev, "error: missing 'csr' resource\n");
> +		ret = -ENXIO;
> +		goto err_reg_res;
> +	}

No need to check that, devm_ioremap_resource() does it too.

> +
> +	adpt->csr = devm_ioremap_resource(&pdev->dev, res);
> +	if (!adpt->csr) {
> +		ret = -ENOMEM;
> +		goto err_reg_res;
> +	}
> +
> +	netdev->base_addr = (unsigned long)adpt->base;
> +	return 0;
> +
> +err_reg_res:
> +	for (i = 0; i < EMAC_CLK_CNT; i++) {
> +		if (adpt->clk[i]) {
> +			clk_put(adpt->clk[i]);
> +			adpt->clk[i] = NULL;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/* Release resources */
> +static void emac_release_resources(struct emac_adapter *adpt)
> +{
> +	int i;
> +
> +	for (i = 0; i < EMAC_CLK_CNT; i++)
> +		if (adpt->clk[i]) {
> +			clk_put(adpt->clk[i]);
> +			adpt->clk[i] = NULL;
> +		}
> +}
> +
> +/* Probe function */
> +static int emac_probe(struct platform_device *pdev)
> +{
> +	struct net_device *netdev;
> +	struct emac_adapter *adpt;
> +	struct emac_phy *phy;
> +	int ret = 0;
> +	u32 hw_ver;
> +	u32 extended_irq_mask = emac_irq_use_extended ? IMR_EXTENDED_MASK :
> +							IMR_NORMAL_MASK;
> +
> +	netdev = alloc_etherdev(sizeof(struct emac_adapter));
> +	if (!netdev)
> +		return -ENOMEM;

There are references to multiple queues in the code, so why not
alloc_etherdev_mq() here with the correct number of queues?

> +
> +	dev_set_drvdata(&pdev->dev, netdev);
> +	SET_NETDEV_DEV(netdev, &pdev->dev);
> +
> +	adpt = netdev_priv(netdev);
> +	adpt->netdev = netdev;
> +	phy = &adpt->phy;
> +	adpt->msg_enable = netif_msg_init(debug, EMAC_MSG_DEFAULT);
> +
> +	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

Really, is not that supposed to run on ARM64 servers?
-- 
Florian

Powered by blists - more mailing lists