Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1378315 > unrolled thread

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Florian Fainelli <f.fainelli@gmail.com> - 2016-04-14 00:20 +0200
    Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-14 22:30 +0200
      Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Florian Fainelli <f.fainelli@gmail.com> - 2016-04-14 23:30 +0200
        Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Vikram Sethi <vikrams@codeaurora.org> - 2016-04-15 00:10 +0200
          Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-15 01:40 +0200
            Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver Rob Herring <robh+dt@kernel.org> - 2016-04-15 14:40 +0200
              Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-15 17:50 +0200
                Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver Rob Herring <robh+dt@kernel.org> - 2016-04-15 18:00 +0200
                  Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-15 19:30 +0200
            Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-15 18:50 +0200
              Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-15 19:10 +0200
                Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-15 19:40 +0200
                  Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-15 20:30 +0200
        Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-21 20:10 +0200
          Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-04-22 21:50 +0200
            Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Florian Fainelli <f.fainelli@gmail.com> - 2016-04-22 22:00 +0200
          Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Andrew Lunn <andrew@lunn.ch> - 2016-04-25 15:20 +0200

#1378315 — 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@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

[toc] | [next] | [standalone]

#1379288

Florian Fainelli wrote:
> On 13/04/16 10:59, Timur Tabi wrote:
>> From: Gilad Avidov <gavidov@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.

Unfortunately, I didn't write this driver initially, so I'm not sure how 
to remove these features from it.  Variants of this driver have been 
bouncing around Qualcomm for years, and even the author of this patch 
(Gilad) is no longer around.

So although I have a lot of experience upstreaming code, I have little 
experience and knowledge with network drivers.  I'm going to need a lot 
of hand-holding.  I hope you will be patient with me.

Timestamping support seems to be just a few lines of code, so I can 
probably remove that.  I don't know where offloading is in the driver, 
however.  I don't know how offloading in netdev drivers works.

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

I can understand that, but the PHYs on these SOCs are non-standard.  The 
"internal PHY" (for lack of a better name) is part of the EMAC itself, 
and it acts as a middle-man for the external PHY.  There is an MDIO bus, 
but it's hard-wired to the EMAC, and most of the time you don't touch it 
directly.  Instead you let the EMAC and/or the internal PHY send/receive 
commands/data to the external PHY on your behalf.  The internal phy 
talks to the external phy via SGMII only.  Only the EMAC uses the mdio bus.

I will look at PHYLIB, but I can't tell you whether it will work with 
this hardware (Gilad previously claim that it wouldn't work well).

>> 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/

The MDIO bus on these chips is not accessible as a separate entity.  It 
is melded (for lack of a better word) into the EMAC itself.  That's why 
there is a "qcom,no-external-phy" property.  You could, in theory, wire 
the internal phy of one SOC directly to the internal phy of another SOC, 
and use that as in interconnect between SOCs.  I don't know of any such 
use-cases however.

>> +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.

It is, and I forget to remove it, since this is apparently handled via 
ethtool (which the driver does not currently support).

>> +- mac-address               : The 6-byte MAC address. If present, it is the
>> +			      default MAC address.
>
> This property is pretty much mandatory

Ok.

>> +- 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?

There is a set of memory-mapped registers.  It's not connected via MDIO 
at all.  It's mapped via the "sgmii" addresses in the device tree (see 
function emac_sgmii_config).

 > 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).

So the internal phy is not a real phy.  It's not capable of driving an 
RJ45 port (there's no analog part).  It's an SGMII-like device that is 
hard-wired to the EMAC itself.

In theory, the internal PHY is optional.  You could design an SOC that 
has just the EMAC connected via normal MDIO to an external phy.  I 
really wish our hardware designers has done that.  But unfortunately, 
there are no SOCs like that, and so we have to treat the internal phy as 
an extension of the EMAC.

My preference would be to get rid of the "qcom,no-external-phy" property 
and have an external phy be required, at least until Qualcomm creates an 
SOC without the internal phy (which may never happen, for all I know).

>> +/* 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?

So you're saying that, for example, in emac_set_features() I should 
remove this:

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

and then in emac_mac_mode_config(), I should do this instead:

void emac_mac_mode_config(struct emac_adapter *adpt)
{
	struct net_device *netdev = adpt->netdev;

	if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
		mac |= VLAN_STRIP;
	else
		mac &= ~VLAN_STRIP;


If so, then what do I do in emac_rx_mode_set()?  Should I delete this 
entire block:

	/* Check for Promiscuous and All Multicast modes */
	if (netdev->flags & IFF_PROMISC) {
		set_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
	} else if (netdev->flags & IFF_ALLMULTI) {
		set_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status);
		clear_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
	} else {
		clear_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status);
		clear_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
	}

It does look like Gilad is just mirroring the flags/features variable 
into adpt->status.  What I can't figure out is why.  It seems completely 
redundant, but I have a nagging feeling that there is a good reason.

>> +	/* 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.).

Ok, I'll probably understand this better once I figure out how to 
implement phylib.

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

So I should move the netif_start_queue() to the end of this function? 
Sorry if that's a stupid question, but I know little about the MAC side 
of network drivers.

>> +	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.

Ok, I'll try it.

>
>> +
>> +	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?

I think that emac_work_thread_link_check() handles this.  It's a timer 
thread that polls the link state and calls netif_carrier_off() if the 
link is down.  Is that sufficient?

>> +
>> +	netif_stop_queue(netdev);
>> +	netif_carrier_off(netdev);
>
> phy_stop() would take care of the latter.

I'm beginning to see how phylib support would be useful.

>> +/* 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.

Ok.

>> +	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.

I'll have to figure out what means and get back to you.  When would 
"later" be?

>> 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.

I'll try.

>> 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.

I'll remove it.  There's no ethtool support in this driver anyway, but 
there's no code that uses this parameter.

>
>> +
>> +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?

Good question.  Apparently it's some IRQ mask.  I'll have to study the 
documentation and get back to you.  We don't ever set the parameter, so 
I think I'll just drop it.

>> +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?

Thanks for catching that.  I'll fix it.

>
> [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?

I don't know what this code is trying to do.  I'll have to study it and 
get back to you.

>
>> +
>> +	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...

Ok.

>
>> +}
>> +
>> +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.

In another internal version of this driver, max_ints is set to 5.  Could 
this be some way of processing multiple packets in one interrupt?  Isn't 
that something that NAPI already takes care of, anyway?

>> +		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.

I'll have to figure out what means and get back to you.

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

Ok.

>
>> +
>> +		/* 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?

I don't know what that is.

>> +/* 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.

Ok.  I'll see what I can do about it.

>> +/* 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.

Ok.

>
> [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?

Ah, even though the readl is atomic, it's reading a bunch of them in a 
row.  I'll add a lock or something.

>> +/* 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,

Ok.

>
>> +
>> +	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.

Ok.

>> +/* 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?

That support was removed from the driver, and on our SOC, we hard-code 
the number of queues to 1 anyway.  I'm planning on adding multiple queue 
support (much) later.

>> +	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?

Well, this version of the driver isn't, which is why it supports DT and 
not ACPI.  I'm planning on adding that support in a later patch. 
However, I'll add support for 64-bit masks in the next version of this 
patch.

Would this be okay:

	retval = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
	if (retval) {
		dev_err(&pdev->dev, "failed to set DMA mask err %d\n", retval);
		goto err_res;
	}

I've seen code like this in other drivers:

         ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
         if (ret) {
                 ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
                 if (ret) {
                         dev_err(dev, "failed to set dma mask\n");
                         return ret;
                 }
         }

and I've never understood why it's necessary to fall back to 32-bits if 
64 bits fails.  Isn't 64 bits a superset of 32 bits?  The driver is 
saying that the hardware supports all of DDR.  How could fail, and how 
could 32-bit succeed if 64-bits fails?

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1379312

On 14/04/16 13:19, Timur Tabi wrote:
> Florian Fainelli wrote:
>> On 13/04/16 10:59, Timur Tabi wrote:
>>> From: Gilad Avidov <gavidov@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.
> 
> Unfortunately, I didn't write this driver initially, so I'm not sure how
> to remove these features from it.  Variants of this driver have been
> bouncing around Qualcomm for years, and even the author of this patch
> (Gilad) is no longer around.

Well, good luck :)

> 
> So although I have a lot of experience upstreaming code, I have little
> experience and knowledge with network drivers.  I'm going to need a lot
> of hand-holding.  I hope you will be patient with me.
> 
> Timestamping support seems to be just a few lines of code, so I can
> probably remove that.  I don't know where offloading is in the driver,
> however.  I don't know how offloading in netdev drivers works.

Based on what the driver seems to do right now, it would be located in
the transmit and receive paths, and would have to access
mac/network/transport offsets and deal with checksums, so anything that
deals with checksums, provided that the HW does not require that to
transmit/receive packets, could be eliminated entirely for now and be
added later.

It is not the biggest part that needs to be slightly re-architected
though, the SGMII/PHY/MDIO stuff is more important as it impacts the
Device Tree binding, see below.

> 
>> You will see below, but a pet peeve of mine is authors reimplementing
>> code that exists in PHYLIB.
> 
> I can understand that, but the PHYs on these SOCs are non-standard.  The
> "internal PHY" (for lack of a better name) is part of the EMAC itself,
> and it acts as a middle-man for the external PHY.  There is an MDIO bus,
> but it's hard-wired to the EMAC, and most of the time you don't touch it
> directly.  Instead you let the EMAC and/or the internal PHY send/receive
> commands/data to the external PHY on your behalf.  The internal phy
> talks to the external phy via SGMII only.  Only the EMAC uses the mdio bus.

Humm OK, this PHY proxy, provided that this is really how it works,
seems a bit unusual, but, is not necessarily a roadblock to having a
proper MDIO implementation here which is standard and will allow you to
utilize re-usable drivers and facilities that are already there.

> 
> I will look at PHYLIB, but I can't tell you whether it will work with
> this hardware (Gilad previously claim that it wouldn't work well).

Well, PHYLIB does prefer using MDIO accesses to "speak" to PHYs,
built-in or external, but there is always the option of investing into
some custom development with the subsystem to make it play nicely with
your HW.

> 
>>> 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/
> 
> The MDIO bus on these chips is not accessible as a separate entity.  It
> is melded (for lack of a better word) into the EMAC itself.  That's why
> there is a "qcom,no-external-phy" property.  You could, in theory, wire
> the internal phy of one SOC directly to the internal phy of another SOC,
> and use that as in interconnect between SOCs.  I don't know of any such
> use-cases however.

The fact the MDIO bus is built-into the MAC is really not a problem
here, there are tons of drivers that deal with that just fine, yet, the
DT binding needs to reflect that properly by having a sub-node of the
Ethernet MAC which is a MDIO bus controller node. If external or
internal PHYs are accessible through that MDIO bus, they also need to
appear as child-nodes of that MDIO bus controller node.

BTW, wiring two PHYs internally is a waste of HW resource at best, if
not just asking for trouble, you can do an Ethernet MAC to MAC
connection, tons of HW do that too.

[snip]

>>> +- 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?
> 
> There is a set of memory-mapped registers.  It's not connected via MDIO
> at all.  It's mapped via the "sgmii" addresses in the device tree (see
> function emac_sgmii_config).
> 
>> 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).
> 
> So the internal phy is not a real phy.  It's not capable of driving an
> RJ45 port (there's no analog part).  It's an SGMII-like device that is
> hard-wired to the EMAC itself.

OK, that explains things a bit, thanks, this is quite a bit of important
detail actually.

> 
> In theory, the internal PHY is optional.  You could design an SOC that
> has just the EMAC connected via normal MDIO to an external phy.  I
> really wish our hardware designers has done that.  But unfortunately,
> there are no SOCs like that, and so we have to treat the internal phy as
> an extension of the EMAC.
> 
> My preference would be to get rid of the "qcom,no-external-phy" property
> and have an external phy be required, at least until Qualcomm creates an
> SOC without the internal phy (which may never happen, for all I know).
> 

Can we just say that, an absence of PHY specified in the Device Tree (no
phy-handle property and PHY not a child node of the MDIO bus), means
that there is no external PHY?

[snip]

>> Do you need to maintain these flags when most, if not all of them
>> already exist in dev->flags or dev->features?
> 
> So you're saying that, for example, in emac_set_features() I should
> remove this:
> 
>     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);
> 
> and then in emac_mac_mode_config(), I should do this instead:
> 
> void emac_mac_mode_config(struct emac_adapter *adpt)
> {
>     struct net_device *netdev = adpt->netdev;
> 
>     if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
>         mac |= VLAN_STRIP;
>     else
>         mac &= ~VLAN_STRIP;
> 
> 
> If so, then what do I do in emac_rx_mode_set()?  Should I delete this
> entire block:
> 
>     /* Check for Promiscuous and All Multicast modes */
>     if (netdev->flags & IFF_PROMISC) {
>         set_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
>     } else if (netdev->flags & IFF_ALLMULTI) {
>         set_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status);
>         clear_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
>     } else {
>         clear_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status);
>         clear_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
>     }
> 
> It does look like Gilad is just mirroring the flags/features variable
> into adpt->status.  What I can't figure out is why.  It seems completely
> redundant, but I have a nagging feeling that there is a good reason.

Yes, I think your set_features and set_rx_mode functions would be
greatly simplified, if each of them did take care of programming the HW
immediately based on function arguments/flags. Unless absolutely
required (e.g: suspend/resume, outside of the scope of the function
etc..) having bookeeping variables is always something that can be out
of sync, so better avoid them as much as possible.

[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).
> 
> So I should move the netif_start_queue() to the end of this function?
> Sorry if that's a stupid question, but I know little about the MAC side
> of network drivers.

That's fine, yes moving netif_start_queue() at the far end of the
function is a good change.

[snip]

>>
>>> +
>>> +    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?
> 
> I think that emac_work_thread_link_check() handles this.  It's a timer
> thread that polls the link state and calls netif_carrier_off() if the
> link is down.  Is that sufficient?
> 

Probably, then again, with PHYLIB you have the option of either
switching the PHY to interrupt mode (thsus saving the polling_), or it
polls the PHY for link statuses every HZ.

[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.
> 
> I'll have to figure out what means and get back to you.  When would
> "later" be?

After the driver gets accepted mainline for instance would seem fine.
Considering how this seems to work, something like this is usally all
that is needed:

if (!skb->xmit_more || netif_xmit_stopped(txq)
	/* write producer index to get HW to transmit */

[snip]

>>> +static int debug = -1;
>>> +module_param(debug, int, S_IRUGO | S_IWUSR | S_IWGRP);
>>
>> ethtool -s <iface> msglvl provides you with that already.
> 
> I'll remove it.  There's no ethtool support in this driver anyway, but
> there's no code that uses this parameter.

Adding support for changing message levels is really trivial, and will
probably help you while developing this driver.

[snip]

>>
>>> +}
>>> +
>>> +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.
> 
> In another internal version of this driver, max_ints is set to 5.  Could
> this be some way of processing multiple packets in one interrupt?  Isn't
> that something that NAPI already takes care of, anyway?

Yes, NAPI is going to mitigate the cost of taking an interrupt and
scheduling your bottom-half/soft IRQ for actual packet processing, it is
the recommended way to mitigate the number of interrupts in the receive
path (and transmit for that matter).

> 
>>> +        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.
> 
> I'll have to figure out what means and get back to you.

drivers/net/ethernet/broadcom/bcmsysport.c is an example driver that
reclaims transmitted buffers in NAPI. What that means is, take the TX
completion interrupt, schedule a NAPI instance to run, and this NAPI
instance cleans up the entire TX queue (it is not bounded, like the RX
NAPI instance). It is really just moving the freeing of SKBs into
softIRQ context vs. hardIRQ.

[snip]

>>> +/* 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?
> 
> I don't know what that is.

TX VLAN offload would be that you can specify the VLAN id somewhere in a
packet's descriptor and have the HW automatically build an Ethernet
frame with the correct VLAN id, and all the Ethernet frame payload
appropriately placed at the correct offsets, with no cost for the CPU
but indicating that information (and not having to do a memmove() to
insert the 802.1Q tag).

[snip]

>>> +/* 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?
> 
> That support was removed from the driver, and on our SOC, we hard-code
> the number of queues to 1 anyway.  I'm planning on adding multiple queue
> support (much) later.

Sounds like a good thing to do later, yes.

> 
>>> +    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?
> 
> Well, this version of the driver isn't, which is why it supports DT and
> not ACPI.  I'm planning on adding that support in a later patch.
> However, I'll add support for 64-bit masks in the next version of this
> patch.
> 
> Would this be okay:
> 
>     retval = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>     if (retval) {
>         dev_err(&pdev->dev, "failed to set DMA mask err %d\n", retval);
>         goto err_res;
>     }
> 
> I've seen code like this in other drivers:
> 
>         ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>         if (ret) {
>                 ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>                 if (ret) {
>                         dev_err(dev, "failed to set dma mask\n");
>                         return ret;
>                 }
>         }
> 
> and I've never understood why it's necessary to fall back to 32-bits if
> 64 bits fails.  Isn't 64 bits a superset of 32 bits?  The driver is
> saying that the hardware supports all of DDR.  How could fail, and how
> could 32-bit succeed if 64-bits fails?

I believe there could be cases where the HW is capable of addressing
more physical memory than the CPU itself (usually unlikely, but it
could), there could be cases where the HW is behind an IOMMMU which only
has a window into the DDR, and that could prevent a higher DMA_BIT_MASK
from being successfully configured.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1379322

A couple of clarifications on the SGMII internal PHY and the DMA capability of the EMAC inline.

On 04/14/2016 04:19 PM, Florian Fainelli wrote:
> On 14/04/16 13:19, Timur Tabi wrote:
>> Florian Fainelli wrote:
>>> On 13/04/16 10:59, Timur Tabi wrote:
>>>> From: Gilad Avidov <gavidov@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.
>>>
>>>
>>> [snip]
>>>
>>>> +- 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?
>> There is a set of memory-mapped registers.  It's not connected via MDIO
>> at all.  It's mapped via the "sgmii" addresses in the device tree (see
>> function emac_sgmii_config).
>>
>>> 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).
>> So the internal phy is not a real phy.  It's not capable of driving an
>> RJ45 port (there's no analog part).  It's an SGMII-like device that is
>> hard-wired to the EMAC itself.
There *is* an analog part to the internal SGMII PHY. Please check the SGMII specification. The only non-standard part is that it's not on MDIO.

> OK, that explains things a bit, thanks, this is quite a bit of important
> detail actually.
>
>> In theory, the internal PHY is optional.  You could design an SOC that
>> has just the EMAC connected via normal MDIO to an external phy.  I
>> really wish our hardware designers has done that.  But unfortunately,
>> there are no SOCs like that, and so we have to treat the internal phy as
>> an extension of the EMAC.
>>
>> My preference would be to get rid of the "qcom,no-external-phy" property
>> and have an external phy be required, at least until Qualcomm creates an
>> SOC without the internal phy (which may never happen, for all I know).
>>
> Can we just say that, an absence of PHY specified in the Device Tree (no
> phy-handle property and PHY not a child node of the MDIO bus), means
> that there is no external PHY?
>
> [snip]
>
>
[snip]
>>>> +    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?
>> Well, this version of the driver isn't, which is why it supports DT and
>> not ACPI.  I'm planning on adding that support in a later patch.
>> However, I'll add support for 64-bit masks in the next version of this
>> patch.
>>
>> Would this be okay:
>>
>>     retval = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>>     if (retval) {
>>         dev_err(&pdev->dev, "failed to set DMA mask err %d\n", retval);
>>         goto err_res;
>>     }

How can you set the mask to 64 bits when the EMAC IP on FSM9900 and QDF2432 can only do 32 bit DMA?
The mask in that API is a bit mask describing which bits of an address your device supports.

>> I've seen code like this in other drivers:
>>
>>         ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>         if (ret) {
>>                 ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>>                 if (ret) {
>>                         dev_err(dev, "failed to set dma mask\n");
>>                         return ret;
>>                 }
>>         }
>>
>> and I've never understood why it's necessary to fall back to 32-bits if
>> 64 bits fails.  Isn't 64 bits a superset of 32 bits?  The driver is
>> saying that the hardware supports all of DDR.  How could fail, and how
>> could 32-bit succeed if 64-bits fails?
> I believe there could be cases where the HW is capable of addressing
> more physical memory than the CPU itself (usually unlikely, but it
> could), there could be cases where the HW is behind an IOMMMU which only
> has a window into the DDR, and that could prevent a higher DMA_BIT_MASK
> from being successfully configured.


-- 
Vikram Sethi
Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1379367

Vikram Sethi wrote:
>>> >>     retval = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>>> >>     if (retval) {
>>> >>         dev_err(&pdev->dev, "failed to set DMA mask err %d\n", retval);
>>> >>         goto err_res;
>>> >>     }
> How can you set the mask to 64 bits when the EMAC IP on FSM9900 and QDF2432 can only do 32 bit DMA?
> The mask in that API is a bit mask describing which bits of an address your device supports.

Vikram, Shanker, and I discussed this offline, and came to a consensus.

The FSM9900 is a 32-bit platform, so the kernel will never create a DMA 
address above 4GB. Even if the driver sets the mask to 64 bits, it will 
technically work.  However, the mask should be set to 32 because all 
address buses are 32 bits.

The QDF2432 is different.  Although it's an ARM64 platform, we have the 
unfortunate situation that only 32 bits of that address is wired to the 
rest of the chip.  So even though the Emac can handle 64-bit bus 
addresses, if it actually attempts to DMA above 4GB, the address will 
get truncated and corrupt memory.  The mask needs to be set to 32.

There may or may not be other ARM64 chips from us that won't have this 
problem in the future, so these hypothetical chips would have a mask of 64.

So I think the solution is to create a device tree (and ACPI) property 
that holds the mask.

	dma-mask = <0 0xffffffff>;

or

	dma-mask = <0xffffffff 0xffffffff>;

The driver will then do this:

	u64 dma_mask;
	device_property_read_u64(&pdev->dev, "dma-mask", &dma_mask);
	dma_coerce_mask_and_coherent(&pdev->dev, dma_mask);

What I'm not sure yet is whether I should call 
dma_coerce_mask_and_coherent() or dma_set_coherent_mask().

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1379791 — Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

On Thu, Apr 14, 2016 at 6:34 PM, Timur Tabi <timur@codeaurora.org> wrote:
> Vikram Sethi wrote:
>>>>
>>>> >>     retval = dma_coerce_mask_and_coherent(&pdev->dev,
>>>> >> DMA_BIT_MASK(64));
>>>> >>     if (retval) {
>>>> >>         dev_err(&pdev->dev, "failed to set DMA mask err %d\n",
>>>> >> retval);
>>>> >>         goto err_res;
>>>> >>     }
>>
>> How can you set the mask to 64 bits when the EMAC IP on FSM9900 and
>> QDF2432 can only do 32 bit DMA?
>> The mask in that API is a bit mask describing which bits of an address
>> your device supports.
>
>
> Vikram, Shanker, and I discussed this offline, and came to a consensus.
>
> The FSM9900 is a 32-bit platform, so the kernel will never create a DMA
> address above 4GB. Even if the driver sets the mask to 64 bits, it will
> technically work.  However, the mask should be set to 32 because all address
> buses are 32 bits.
>
> The QDF2432 is different.  Although it's an ARM64 platform, we have the
> unfortunate situation that only 32 bits of that address is wired to the rest
> of the chip.  So even though the Emac can handle 64-bit bus addresses, if it
> actually attempts to DMA above 4GB, the address will get truncated and
> corrupt memory.  The mask needs to be set to 32.
>
> There may or may not be other ARM64 chips from us that won't have this
> problem in the future, so these hypothetical chips would have a mask of 64.
>
> So I think the solution is to create a device tree (and ACPI) property that
> holds the mask.
>
>         dma-mask = <0 0xffffffff>;
>
> or
>
>         dma-mask = <0xffffffff 0xffffffff>;

No. See dma-ranges.

Rob

>
> The driver will then do this:
>
>         u64 dma_mask;
>         device_property_read_u64(&pdev->dev, "dma-mask", &dma_mask);
>         dma_coerce_mask_and_coherent(&pdev->dev, dma_mask);
>
> What I'm not sure yet is whether I should call
> dma_coerce_mask_and_coherent() or dma_set_coherent_mask().
>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1379936

Rob Herring wrote:
>> >
>> >         dma-mask = <0 0xffffffff>;
>> >
>> >or
>> >
>> >         dma-mask = <0xffffffff 0xffffffff>;
> No. See dma-ranges.

How exactly should I use dma-ranges?  I can't find any other drivers 
that queries that property and uses the result to call dma_set_mask.  I 
thought the dma-ranges property is intended to specify address 
translation.  I don't need to translate any address, I just need to know 
a single number.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1379946 — Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

On Fri, Apr 15, 2016 at 10:44 AM, Timur Tabi <timur@codeaurora.org> wrote:
> Rob Herring wrote:
>>>
>>> >
>>> >         dma-mask = <0 0xffffffff>;
>>> >
>>> >or
>>> >
>>> >         dma-mask = <0xffffffff 0xffffffff>;
>>
>> No. See dma-ranges.
>
>
> How exactly should I use dma-ranges?  I can't find any other drivers that
> queries that property and uses the result to call dma_set_mask.  I thought
> the dma-ranges property is intended to specify address translation.  I don't
> need to translate any address, I just need to know a single number.

You may only care about the size, but the binding has to handle the
more complex case. Here's an example

<0x0 0x2 0x0 0x1 0x0>

dma address 0 (cell 0) maps to cpu (parent) address 0x2_00000000 (cell
1-2) and the range/size is 4G (cell 3-4).

If you have the same base address, then use the same address. The core
will calculate the mask based on the size. IIRC, we also handle ~0 as
a special case to support 4G for #size-cell=1.

Rob

[toc] | [prev] | [next] | [standalone]

#1380053

Rob Herring wrote:
> You may only care about the size, but the binding has to handle the
> more complex case. Here's an example
>
> <0x0 0x2 0x0 0x1 0x0>
>
> dma address 0 (cell 0) maps to cpu (parent) address 0x2_00000000 (cell
> 1-2) and the range/size is 4G (cell 3-4).
>
> If you have the same base address, then use the same address. The core
> will calculate the mask based on the size. IIRC, we also handle ~0 as
> a special case to support 4G for #size-cell=1.

So the first thing I noticed is that Gilad had this:

	reg =   <0xfeb20000 0x10000>,
		<0xfeb36000 0x1000>,
		<0xfeb3c000 0x4000>,
		<0xfeb38000 0x400>;
	#address-cells = <0>;

Shouldn't address-cells have been 1 instead?

Ok, let me see if I get this right:

32-bit:

soc {
	#address-cells = <1>;
	#size-cells = <1>;

	emac0: qcom,emac@feb20000 {
		compatible = "qcom,fsm9900-emac";
		#address-cells = <1>;
		#size-cells = <1>;
		reg-names = "base", "csr", "ptp", "sgmii";
		reg =   <0xfeb20000 0x10000>,
			<0xfeb36000 0x1000>,
			<0xfeb3c000 0x4000>,
			<0xfeb38000 0x400>;
		dma-ranges = <0 0 0xffffffff>;
		interrupt-parent = <&emac0>;

64-bit

soc {
	#address-cells = <2>;
	#size-cells = <2>;

	emac0: qcom,emac@feb20000 {
		compatible = "qcom,fsm9900-emac";
		#address-cells = <2>;
		#size-cells = <2>;
		reg-names = "base", "csr", "ptp", "sgmii";
		reg =   <0 0xfeb20000 0 0x10000>,
			<0 0xfeb36000 0 0x1000>,
			<0 0xfeb3c000 0 0x4000>,
			<0 0xfeb38000 0 0x400>;
		dma-ranges = <0 0 0 0 0xffffffff 0xffffffff>;

This seems inelegant, though.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1380005

On Thu 14 Apr 16:34 PDT 2016, Timur Tabi wrote:

[..]
> So I think the solution is to create a device tree (and ACPI) property that
> holds the mask.
> 
> 	dma-mask = <0 0xffffffff>;
> 
> or
> 
> 	dma-mask = <0xffffffff 0xffffffff>;
> 
> The driver will then do this:
> 
> 	u64 dma_mask;
> 	device_property_read_u64(&pdev->dev, "dma-mask", &dma_mask);
> 	dma_coerce_mask_and_coherent(&pdev->dev, dma_mask);
> 
> What I'm not sure yet is whether I should call
> dma_coerce_mask_and_coherent() or dma_set_coherent_mask().
> 

For platform devices being populated via from DT you will pass:
of_platform_bus_create()
  of_platform_device_create_pdata()
    of_dma_configure()

Which calls of_dma_get_range() to acquire this information from the
dma-ranges property and set up the dma ops and properties.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1380035

Bjorn Andersson wrote:
> For platform devices being populated via from DT you will pass:
> of_platform_bus_create()
>    of_platform_device_create_pdata()
>      of_dma_configure()
>
> Which calls of_dma_get_range() to acquire this information from the
> dma-ranges property and set up the dma ops and properties.

This seems excessive.  I have to create a platform bus just to configure 
the DMA mask?  Most drivers just call dma_set_mask and give it a number, 
and that's not device-tree specific.  I also need to come up with a way 
to get this to work on ACPI.

I just seems like a lot of work only because I need to determine at 
runtime what my DMA mask is.  I also don't see any drivers that call 
of_dma_configure().

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1380058

On Fri 15 Apr 10:00 PDT 2016, Timur Tabi wrote:

> Bjorn Andersson wrote:
> >For platform devices being populated via from DT you will pass:
> >of_platform_bus_create()
> >   of_platform_device_create_pdata()
> >     of_dma_configure()
> >
> >Which calls of_dma_get_range() to acquire this information from the
> >dma-ranges property and set up the dma ops and properties.
> 
> This seems excessive.  I have to create a platform bus just to configure the
> DMA mask?  Most drivers just call dma_set_mask and give it a number, and
> that's not device-tree specific.  I also need to come up with a way to get
> this to work on ACPI.
> 
> I just seems like a lot of work only because I need to determine at runtime
> what my DMA mask is.  I also don't see any drivers that call
> of_dma_configure().
> 

Your driver is a platform driver and it's being probed from DT. As such
all this is already taken care of for you, by the core.

The listing is for your reference to know why the dma-ranges property
would affect your device.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1380102

Bjorn Andersson wrote:
> Your driver is a platform driver and it's being probed from DT. As such
> all this is already taken care of for you, by the core.
>
> The listing is for your reference to know why the dma-ranges property
> would affect your device.

Ah, sorry. I misunderstood what you meant by "you will pass".

Coincidentally, it looks like Lorenzo Pieralisi has posted patches the 
ACPI equivalent just yesterday: https://lkml.org/lkml/2016/4/14/694

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1384470

Florian Fainelli wrote:

> Well, PHYLIB does prefer using MDIO accesses to "speak" to PHYs,
> built-in or external, but there is always the option of investing into
> some custom development with the subsystem to make it play nicely with
> your HW.

So I've done some more research, and I believe that the internal phy is 
not a candidate for phylib, but the external phy (which is a real phy) 
might be.  There's no MDIO bus to the internal phy.

Does this mean that I will need to enable a PHY driver, and that driver 
will control the external phy?  If so, then does that mean that I would 
delete all to code in my driver that calls emac_phy_read() and 
emac_phy_write()?  For example, I wouldn't need emac_phy_link_check() 
any more?

>> The MDIO bus on these chips is not accessible as a separate entity.  It
>> is melded (for lack of a better word) into the EMAC itself.  That's why
>> there is a "qcom,no-external-phy" property.  You could, in theory, wire
>> the internal phy of one SOC directly to the internal phy of another SOC,
>> and use that as in interconnect between SOCs.  I don't know of any such
>> use-cases however.
>
> The fact the MDIO bus is built-into the MAC is really not a problem
> here, there are tons of drivers that deal with that just fine, yet, the
> DT binding needs to reflect that properly by having a sub-node of the
> Ethernet MAC which is a MDIO bus controller node. If external or
> internal PHYs are accessible through that MDIO bus, they also need to
> appear as child-nodes of that MDIO bus controller node.

Does the compatible property of the phy node (for the external phy) need 
to list the actual external phy?  That is, should it look like this:

	phy0: ethernet-phy@0 {
		compatible = "qcom,fsm9900-emac-phy";
		reg = <0>;
	}

or this:

	phy0: ethernet-phy@0 {
		compatible = "athr,whatever-phy";
		reg = <0>;
	}



> Can we just say that, an absence of PHY specified in the Device Tree (no
> phy-handle property and PHY not a child node of the MDIO bus), means
> that there is no external PHY?

Yes, that works.

>
> [snip]
>
>>> Do you need to maintain these flags when most, if not all of them
>>> already exist in dev->flags or dev->features?
>>
>> So you're saying that, for example, in emac_set_features() I should
>> remove this:
>>
>>      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);
>>
>> and then in emac_mac_mode_config(), I should do this instead:
>>
>> void emac_mac_mode_config(struct emac_adapter *adpt)
>> {
>>      struct net_device *netdev = adpt->netdev;
>>
>>      if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
>>          mac |= VLAN_STRIP;
>>      else
>>          mac &= ~VLAN_STRIP;
>>
>>
>> If so, then what do I do in emac_rx_mode_set()?  Should I delete this
>> entire block:
>>
>>      /* Check for Promiscuous and All Multicast modes */
>>      if (netdev->flags & IFF_PROMISC) {
>>          set_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
>>      } else if (netdev->flags & IFF_ALLMULTI) {
>>          set_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status);
>>          clear_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
>>      } else {
>>          clear_bit(EMAC_STATUS_MULTIALL_EN, &adpt->status);
>>          clear_bit(EMAC_STATUS_PROMISC_EN, &adpt->status);
>>      }
>>
>> It does look like Gilad is just mirroring the flags/features variable
>> into adpt->status.  What I can't figure out is why.  It seems completely
>> redundant, but I have a nagging feeling that there is a good reason.
>
> Yes, I think your set_features and set_rx_mode functions would be
> greatly simplified, if each of them did take care of programming the HW
> immediately based on function arguments/flags. Unless absolutely
> required (e.g: suspend/resume, outside of the scope of the function
> etc..) having bookeeping variables is always something that can be out
> of sync, so better avoid them as much as possible.

Ok, I'll try to clean this up.

>> So I should move the netif_start_queue() to the end of this function?
>> Sorry if that's a stupid question, but I know little about the MAC side
>> of network drivers.
>
> That's fine, yes moving netif_start_queue() at the far end of the
> function is a good change.

Ok.

>>>> +/* 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?
>>
>> I think that emac_work_thread_link_check() handles this.  It's a timer
>> thread that polls the link state and calls netif_carrier_off() if the
>> link is down.  Is that sufficient?
>>
>
> Probably, then again, with PHYLIB you have the option of either
> switching the PHY to interrupt mode (thsus saving the polling_), or it
> polls the PHY for link statuses every HZ.

I'll have to check and see if interrupt mode is even an option.  So 
phylib can do the polling for me?

>>> 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.
>>
>> I'll have to figure out what means and get back to you.  When would
>> "later" be?
>
> After the driver gets accepted mainline for instance would seem fine.
> Considering how this seems to work, something like this is usally all
> that is needed:
>
> if (!skb->xmit_more || netif_xmit_stopped(txq)
> 	/* write producer index to get HW to transmit */

Oh, I thought you meant later in the code somewhere.  At a later date 
with another patch sounds great to me, though.

>>>> +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.
>>
>> In another internal version of this driver, max_ints is set to 5.  Could
>> this be some way of processing multiple packets in one interrupt?  Isn't
>> that something that NAPI already takes care of, anyway?
>
> Yes, NAPI is going to mitigate the cost of taking an interrupt and
> scheduling your bottom-half/soft IRQ for actual packet processing, it is
> the recommended way to mitigate the number of interrupts in the receive
> path (and transmit for that matter).

I'll clean up the code and remove max_ints.

>
>>
>>>> +        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.
>>
>> I'll have to figure out what means and get back to you.
>
> drivers/net/ethernet/broadcom/bcmsysport.c is an example driver that
> reclaims transmitted buffers in NAPI. What that means is, take the TX
> completion interrupt, schedule a NAPI instance to run, and this NAPI
> instance cleans up the entire TX queue (it is not bounded, like the RX
> NAPI instance). It is really just moving the freeing of SKBs into
> softIRQ context vs. hardIRQ.

Thanks.  I don't think I'll get to any of the NAPI fixes in v5 of this 
driver.  I want to make sure I get the phylib conversion correct first.

>>>> +/* 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?
>>
>> I don't know what that is.
>
> TX VLAN offload would be that you can specify the VLAN id somewhere in a
> packet's descriptor and have the HW automatically build an Ethernet
> frame with the correct VLAN id, and all the Ethernet frame payload
> appropriately placed at the correct offsets, with no cost for the CPU
> but indicating that information (and not having to do a memmove() to
> insert the 802.1Q tag).

I have no idea if our hardware supports that.  I'll make a note of TX 
VLAN offload and submit a separate patch if I can make it work.

>> and I've never understood why it's necessary to fall back to 32-bits if
>> 64 bits fails.  Isn't 64 bits a superset of 32 bits?  The driver is
>> saying that the hardware supports all of DDR.  How could fail, and how
>> could 32-bit succeed if 64-bits fails?
>
> I believe there could be cases where the HW is capable of addressing
> more physical memory than the CPU itself (usually unlikely, but it
> could),there could be cases where the HW is behind an IOMMMU which only
> has a window into the DDR, and that could prevent a higher DMA_BIT_MASK
> from being successfully configured.

So, so I'm going to add dma-ranges support (I posted another patch asked 
for feedback, but I haven't gotten it yet).

For ACPI, we're going to depend on IORT to set the DMA mask for us.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1385463

Timur Tabi wrote:
>>
>
> So I've done some more research, and I believe that the internal phy is
> not a candidate for phylib, but the external phy (which is a real phy)
> might be.  There's no MDIO bus to the internal phy.
>
> Does this mean that I will need to enable a PHY driver, and that driver
> will control the external phy?  If so, then does that mean that I would
> delete all to code in my driver that calls emac_phy_read() and
> emac_phy_write()?  For example, I wouldn't need emac_phy_link_check()
> any more?

So I think I have it partially working, but I'm not sure if I'm doing 
things correctly, and I'd like some help.

The external phy is an Atheros 8031, so I load the at803x driver.  I 
added this code to my driver:

	mii_bus = devm_mdiobus_alloc(&pdev->dev);
	mii_bus->phy_mask = ~(1 << adpt->hw.phy_addr);
	mii_bus->read = emac_mdio_read;
	mii_bus->write = emac_mdio_write;
	mii_bus->reset = emac_mdio_reset;
	mii_bus->parent = &pdev->dev;
	mii_bus->priv = hw;

	mdiobus_register(mii_bus);

When I call mdiobus_register, I can see that the at803x_probe() probe 
function is called, so a connection is made.

The problem is that after that point, it appears that the at803x driver 
is never called again.  I tried bring the interface up and down, and 
connecting and disconnecting an Ethernet cable, but that didn't trigger 
anything.  I would expect the PHY driver to do more than just probe.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1385465

On 22/04/16 12:45, Timur Tabi wrote:
> Timur Tabi wrote:
>>>
>>
>> So I've done some more research, and I believe that the internal phy is
>> not a candidate for phylib, but the external phy (which is a real phy)
>> might be.  There's no MDIO bus to the internal phy.
>>
>> Does this mean that I will need to enable a PHY driver, and that driver
>> will control the external phy?  If so, then does that mean that I would
>> delete all to code in my driver that calls emac_phy_read() and
>> emac_phy_write()?  For example, I wouldn't need emac_phy_link_check()
>> any more?
> 
> So I think I have it partially working, but I'm not sure if I'm doing
> things correctly, and I'd like some help.
> 
> The external phy is an Atheros 8031, so I load the at803x driver.  I
> added this code to my driver:
> 
>     mii_bus = devm_mdiobus_alloc(&pdev->dev);
>     mii_bus->phy_mask = ~(1 << adpt->hw.phy_addr);
>     mii_bus->read = emac_mdio_read;
>     mii_bus->write = emac_mdio_write;
>     mii_bus->reset = emac_mdio_reset;
>     mii_bus->parent = &pdev->dev;
>     mii_bus->priv = hw;
> 
>     mdiobus_register(mii_bus);
> 
> When I call mdiobus_register, I can see that the at803x_probe() probe
> function is called, so a connection is made.
> 
> The problem is that after that point, it appears that the at803x driver
> is never called again.  I tried bring the interface up and down, and
> connecting and disconnecting an Ethernet cable, but that didn't trigger
> anything.  I would expect the PHY driver to do more than just probe.

Are you utilizing the PHYLIB APIs properly? You need at least a
phy_start() to start the PHY state machine, and an adjust_link callback
to be provided to phy_connect() (or of_phy_connect()) to manage link
state changes. And that's the very basic minimum here, there could be
additional APIs that you may end up using.

There are tons of example in tree of drivers doing this, bcmgenet,
bcmsysport, tg3 etc.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1386404

> Does the compatible property of the phy node (for the external phy)
> need to list the actual external phy?  That is, should it look like
> this:
> 
> 	phy0: ethernet-phy@0 {
> 		compatible = "qcom,fsm9900-emac-phy";
> 		reg = <0>;
> 	}
> 
> or this:
> 
> 	phy0: ethernet-phy@0 {
> 		compatible = "athr,whatever-phy";
> 		reg = <0>;
> 	}
> 

Documentation/devicetree/bindings/net/phy.txt says:

Optional Properties:

- compatible: Compatible list, may contain
  "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for
  PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45
  specifications. If neither of these are specified, the default is to
  assume clause 22.

  If the phy's identifier is known then the list may contain an entry
  of the form: "ethernet-phy-idAAAA.BBBB" where
     AAAA - The value of the 16 bit Phy Identifier 1 register as
            4 hex digits. This is the chip vendor OUI bits 3:18
     BBBB - The value of the 16 bit Phy Identifier 2 register as
            4 hex digits. This is the chip vendor OUI bits 19:24,
            followed by 10 bits of a vendor specific ID.

  The compatible list should not contain other values than those
  listed here.

  Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web