On 13/04/16 10:59, Timur Tabi wrote:
> From: Gilad Avidov <gavi...@codeaurora.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@feb20000 {
> + 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@fd510000 {
> + 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
