[PATCH v8 0/3] add hisilicon hip04 ethernet driver

[PATCH v8 0/3] add hisilicon hip04 ethernet driver

[Zhangfei Gao]

v8:
Use poll to reclaim xmitted buffer as workaround since no tx done interrupt 

v7:
Remove select NET_CORE in 0002

v6:
Suggest by Russell: Use netdev_sent_queue & netdev_completed_queue to solve latency issue 
Also shorten the period of timer, which is used to wakeup the queue since no
tx completed interrupt.

v5:
no big change, fix typo

v4:
Modify accoringly to the suggetion from Arnd, Florian, Eric, David
Use of_parse_phandle_with_fixed_args & syscon_node_to_regmap get ppe info
Add skb_orphan() and tx_timer for reclaim since no tx_finished interrupt
Update timeout, and move of_phy_connect to probe to reuse open/stop

v3:
Suggest from Arnd, use syscon & regmap_write/read to replace static void __iomem *ppebase.
Modify hisilicon-hip04-net.txt accrordingly to suggestion from Florian and Sergei.

v2:
Got many suggestions from Russell, Arnd, Florian, Mark and Sergei
Remove memcpy, use dma_map/unmap_single, use dma_alloc_coherent rather than dma_pool, etc.
Refer property in ethernet.txt, change ppe description, etc.

Zhangfei Gao (3):
  Documentation: add Device tree bindings for Hisilicon hip04 ethernet
  net: hisilicon: new hip04 MDIO driver
  net: hisilicon: new hip04 ethernet driver

 .../bindings/net/hisilicon-hip04-net.txt           |   88 ++
 drivers/net/ethernet/Kconfig                       |    1 +
 drivers/net/ethernet/Makefile                      |    1 +
 drivers/net/ethernet/hisilicon/Kconfig             |   31 +
 drivers/net/ethernet/hisilicon/Makefile            |    5 +
 drivers/net/ethernet/hisilicon/hip04_eth.c         |  845 ++++++++++++++++++++
 drivers/net/ethernet/hisilicon/hip04_mdio.c        |  185 +++++
 7 files changed, 1156 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-hip04-net.txt
 create mode 100644 drivers/net/ethernet/hisilicon/Kconfig
 create mode 100644 drivers/net/ethernet/hisilicon/Makefile
 create mode 100644 drivers/net/ethernet/hisilicon/hip04_eth.c
 create mode 100644 drivers/net/ethernet/hisilicon/hip04_mdio.c

-- 
1.7.9.5

[PATCH v8 1/3] Documentation: add Device tree bindings for Hisilicon hip04 ethernet

[Zhangfei Gao]

This patch adds the Device Tree bindings for the Hisilicon hip04
Ethernet controller, including 100M / 1000M controller.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 .../bindings/net/hisilicon-hip04-net.txt           |   88 ++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-hip04-net.txt

diff --git a/Documentation/devicetree/bindings/net/hisilicon-hip04-net.txt b/Documentation/devicetree/bindings/net/hisilicon-hip04-net.txt
new file mode 100644
index 0000000..988fc69
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon-hip04-net.txt
@@ -0,0 +1,88 @@
+Hisilicon hip04 Ethernet Controller
+
+* Ethernet controller node
+
+Required properties:
+- compatible: should be "hisilicon,hip04-mac".
+- reg: address and length of the register set for the device.
+- interrupts: interrupt for the device.
+- port-handle: <phandle port channel>
+	phandle, specifies a reference to the syscon ppe node
+	port, port number connected to the controller
+	channel, recv channel start from channel * number (RX_DESC_NUM)
+- phy-mode: see ethernet.txt [1].
+
+Optional properties:
+- phy-handle: see ethernet.txt [1].
+
+[1] Documentation/devicetree/bindings/net/ethernet.txt
+
+
+* Ethernet ppe node:
+Control rx & tx fifos of all ethernet controllers.
+Have 2048 recv channels shared by all ethernet controllers, only if no overlap.
+Each controller's recv channel start from channel * number (RX_DESC_NUM).
+
+Required properties:
+- compatible: "hisilicon,hip04-ppe", "syscon".
+- reg: address and length of the register set for the device.
+
+
+* MDIO bus node:
+
+Required properties:
+
+- compatible: should be "hisilicon,hip04-mdio".
+- Inherits from MDIO bus node binding [2]
+[2] Documentation/devicetree/bindings/net/phy.txt
+
+Example:
+	mdio {
+		compatible = "hisilicon,hip04-mdio";
+		reg = <0x28f1000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		phy0: ethernet-phy@0 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			reg = <0>;
+			marvell,reg-init = <18 0x14 0 0x8001>;
+		};
+
+		phy1: ethernet-phy@1 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			reg = <1>;
+			marvell,reg-init = <18 0x14 0 0x8001>;
+		};
+	};
+
+	ppe: ppe@28c0000 {
+		compatible = "hisilicon,hip04-ppe", "syscon";
+		reg = <0x28c0000 0x10000>;
+	};
+
+	fe: ethernet@28b0000 {
+		compatible = "hisilicon,hip04-mac";
+		reg = <0x28b0000 0x10000>;
+		interrupts = <0 413 4>;
+		phy-mode = "mii";
+		port-handle = <&ppe 31 0>;
+	};
+
+	ge0: ethernet@2800000 {
+		compatible = "hisilicon,hip04-mac";
+		reg = <0x2800000 0x10000>;
+		interrupts = <0 402 4>;
+		phy-mode = "sgmii";
+		port-handle = <&ppe 0 1>;
+		phy-handle = <&phy0>;
+	};
+
+	ge8: ethernet@2880000 {
+		compatible = "hisilicon,hip04-mac";
+		reg = <0x2880000 0x10000>;
+		interrupts = <0 410 4>;
+		phy-mode = "sgmii";
+		port-handle = <&ppe 8 2>;
+		phy-handle = <&phy1>;
+	};
-- 
1.7.9.5

[PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Zhangfei Gao]

Hisilicon hip04 platform mdio driver
Reuse Marvell phy drivers/net/phy/marvell.c

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/net/ethernet/Kconfig                |    1 +
 drivers/net/ethernet/Makefile               |    1 +
 drivers/net/ethernet/hisilicon/Kconfig      |   31 +++++
 drivers/net/ethernet/hisilicon/Makefile     |    5 +
 drivers/net/ethernet/hisilicon/hip04_mdio.c |  185 +++++++++++++++++++++++++++
 5 files changed, 223 insertions(+)
 create mode 100644 drivers/net/ethernet/hisilicon/Kconfig
 create mode 100644 drivers/net/ethernet/hisilicon/Makefile
 create mode 100644 drivers/net/ethernet/hisilicon/hip04_mdio.c

diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 506b024..1c8dc3d 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -54,6 +54,7 @@ source "drivers/net/ethernet/neterion/Kconfig"
 source "drivers/net/ethernet/faraday/Kconfig"
 source "drivers/net/ethernet/freescale/Kconfig"
 source "drivers/net/ethernet/fujitsu/Kconfig"
+source "drivers/net/ethernet/hisilicon/Kconfig"
 source "drivers/net/ethernet/hp/Kconfig"
 source "drivers/net/ethernet/ibm/Kconfig"
 source "drivers/net/ethernet/intel/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index c0b8789..da1a435 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_NET_VENDOR_EXAR) += neterion/
 obj-$(CONFIG_NET_VENDOR_FARADAY) += faraday/
 obj-$(CONFIG_NET_VENDOR_FREESCALE) += freescale/
 obj-$(CONFIG_NET_VENDOR_FUJITSU) += fujitsu/
+obj-$(CONFIG_NET_VENDOR_HISILICON) += hisilicon/
 obj-$(CONFIG_NET_VENDOR_HP) += hp/
 obj-$(CONFIG_NET_VENDOR_IBM) += ibm/
 obj-$(CONFIG_NET_VENDOR_INTEL) += intel/
diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
new file mode 100644
index 0000000..628537f
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/Kconfig
@@ -0,0 +1,31 @@
+#
+# HISILICON device configuration
+#
+
+config NET_VENDOR_HISILICON
+	bool "Hisilicon devices"
+	default y
+	depends on ARM
+	---help---
+	  If you have a network (Ethernet) card belonging to this class, say Y
+	  and read the Ethernet-HOWTO, available from
+	  <http://www.tldp.org/docs.html#howto>.
+
+	  Note that the answer to this question doesn't directly affect the
+	  kernel: saying N will just cause the configurator to skip all
+	  the questions about Hisilicon devices. If you say Y, you will be asked
+	  for your specific card in the following questions.
+
+if NET_VENDOR_HISILICON
+
+config HIP04_ETH
+	tristate "HISILICON P04 Ethernet support"
+	select PHYLIB
+	select MARVELL_PHY
+	select MFD_SYSCON
+	---help---
+	  If you wish to compile a kernel for a hardware with hisilicon p04 SoC and
+	  want to use the internal ethernet then you should answer Y to this.
+
+
+endif # NET_VENDOR_HISILICON
diff --git a/drivers/net/ethernet/hisilicon/Makefile b/drivers/net/ethernet/hisilicon/Makefile
new file mode 100644
index 0000000..1d6eb6e
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the HISILICON network device drivers.
+#
+
+obj-$(CONFIG_HIP04_ETH) += hip04_mdio.o
diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c b/drivers/net/ethernet/hisilicon/hip04_mdio.c
new file mode 100644
index 0000000..19826a3
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
@@ -0,0 +1,185 @@
+
+/* Copyright (c) 2014 Linaro Ltd.
+ * Copyright (c) 2014 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of_mdio.h>
+#include <linux/delay.h>
+
+#define MDIO_CMD_REG		0x0
+#define MDIO_ADDR_REG		0x4
+#define MDIO_WDATA_REG		0x8
+#define MDIO_RDATA_REG		0xc
+#define MDIO_STA_REG		0x10
+
+#define MDIO_START		BIT(14)
+#define MDIO_R_VALID		BIT(1)
+#define MDIO_READ	        (BIT(12) | BIT(11) | MDIO_START)
+#define MDIO_WRITE	        (BIT(12) | BIT(10) | MDIO_START)
+
+struct hip04_mdio_priv {
+	void __iomem *base;
+};
+
+#define WAIT_TIMEOUT 10
+static int hip04_mdio_wait_ready(struct mii_bus *bus)
+{
+	struct hip04_mdio_priv *priv = bus->priv;
+	int i;
+
+	for (i = 0; readl_relaxed(priv->base + MDIO_CMD_REG) & MDIO_START; i++) {
+		if (i == WAIT_TIMEOUT)
+			return -ETIMEDOUT;
+		msleep(20);
+	}
+
+	return 0;
+}
+
+static int hip04_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct hip04_mdio_priv *priv = bus->priv;
+	u32 val;
+	int ret;
+
+	ret = hip04_mdio_wait_ready(bus);
+	if (ret < 0)
+		goto out;
+
+	val = regnum | (mii_id << 5) | MDIO_READ;
+	writel_relaxed(val, priv->base + MDIO_CMD_REG);
+
+	ret = hip04_mdio_wait_ready(bus);
+	if (ret < 0)
+		goto out;
+
+	val = readl_relaxed(priv->base + MDIO_STA_REG);
+	if (val & MDIO_R_VALID) {
+		dev_err(bus->parent, "SMI bus read not valid\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	val = readl_relaxed(priv->base + MDIO_RDATA_REG);
+	ret = val & 0xFFFF;
+out:
+	return ret;
+}
+
+static int hip04_mdio_write(struct mii_bus *bus, int mii_id,
+			    int regnum, u16 value)
+{
+	struct hip04_mdio_priv *priv = bus->priv;
+	u32 val;
+	int ret;
+
+	ret = hip04_mdio_wait_ready(bus);
+	if (ret < 0)
+		goto out;
+
+	writel_relaxed(value, priv->base + MDIO_WDATA_REG);
+	val = regnum | (mii_id << 5) | MDIO_WRITE;
+	writel_relaxed(val, priv->base + MDIO_CMD_REG);
+out:
+	return ret;
+}
+
+static int hip04_mdio_reset(struct mii_bus *bus)
+{
+	int temp, err, i;
+
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
+		hip04_mdio_write(bus, i, 22, 0);
+		temp = hip04_mdio_read(bus, i, MII_BMCR);
+		temp |= BMCR_RESET;
+		err = hip04_mdio_write(bus, i, MII_BMCR, temp);
+		if (err < 0)
+			return err;
+	}
+
+	mdelay(500);
+	return 0;
+}
+
+static int hip04_mdio_probe(struct platform_device *pdev)
+{
+	struct resource *r;
+	struct mii_bus *bus;
+	struct hip04_mdio_priv *priv;
+	int ret;
+
+	bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
+	if (!bus) {
+		dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
+		return -ENOMEM;
+	}
+
+	bus->name = "hip04_mdio_bus";
+	bus->read = hip04_mdio_read;
+	bus->write = hip04_mdio_write;
+	bus->reset = hip04_mdio_reset;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
+	bus->parent = &pdev->dev;
+	priv = bus->priv;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto out_mdio;
+	}
+
+	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
+		goto out_mdio;
+	}
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+
+out_mdio:
+	mdiobus_free(bus);
+	return ret;
+}
+
+static int hip04_mdio_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+
+	mdiobus_unregister(bus);
+	mdiobus_free(bus);
+
+	return 0;
+}
+
+static const struct of_device_id hip04_mdio_match[] = {
+	{ .compatible = "hisilicon,hip04-mdio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hip04_mdio_match);
+
+static struct platform_driver hip04_mdio_driver = {
+	.probe = hip04_mdio_probe,
+	.remove = hip04_mdio_remove,
+	.driver = {
+		.name = "hip04-mdio",
+		.owner = THIS_MODULE,
+		.of_match_table = hip04_mdio_match,
+	},
+};
+
+module_platform_driver(hip04_mdio_driver);
+
+MODULE_DESCRIPTION("HISILICON P04 MDIO interface driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hip04-mdio");
-- 
1.7.9.5

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Sergei Shtylyov]

Hello.

On 04/19/2014 05:12 AM, Zhangfei Gao wrote:

> Hisilicon hip04 platform mdio driver
> Reuse Marvell phy drivers/net/phy/marvell.c

> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
[...]

> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> new file mode 100644
> index 0000000..19826a3
> --- /dev/null
> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> @@ -0,0 +1,185 @@
> +

    Empty line not needed here.

> +/* Copyright (c) 2014 Linaro Ltd.
> + * Copyright (c) 2014 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

[...]

> +static int hip04_mdio_reset(struct mii_bus *bus)
> +{
> +	int temp, err, i;
> +
> +	for (i = 0; i < PHY_MAX_ADDR; i++) {
> +		hip04_mdio_write(bus, i, 22, 0);

    Why? What kind of a register this is? <uapi/linux/mii.h> tells me it's 
MII_SREVISION...

> +		temp = hip04_mdio_read(bus, i, MII_BMCR);

    You're not checking for error...

> +		temp |= BMCR_RESET;
> +		err = hip04_mdio_write(bus, i, MII_BMCR, temp);

    Hmm, why you're open coding BMCR reset? There's phy_init_hw() doing this 
correctly...

> +		if (err < 0)
> +			return err;
> +	}
> +
> +	mdelay(500);
> +	return 0;
> +}
> +
> +static int hip04_mdio_probe(struct platform_device *pdev)
> +{
> +	struct resource *r;
> +	struct mii_bus *bus;
> +	struct hip04_mdio_priv *priv;
> +	int ret;
> +
> +	bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
> +	if (!bus) {
> +		dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
> +		return -ENOMEM;
> +	}
> +
> +	bus->name = "hip04_mdio_bus";
> +	bus->read = hip04_mdio_read;
> +	bus->write = hip04_mdio_write;
> +	bus->reset = hip04_mdio_reset;

    Ah... However I don't think it a good implementation of that bus method...

[...]

WBR, Sergei

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Florian Fainelli]

2014-04-21 10:58 GMT-07:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
>
> On 04/19/2014 05:12 AM, Zhangfei Gao wrote:
>
>> Hisilicon hip04 platform mdio driver
>> Reuse Marvell phy drivers/net/phy/marvell.c
>
>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>
> [...]
>
>
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>> new file mode 100644
>> index 0000000..19826a3
>> --- /dev/null
>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>> @@ -0,0 +1,185 @@
>> +
>
>
>    Empty line not needed here.
>
>
>> +/* Copyright (c) 2014 Linaro Ltd.
>> + * Copyright (c) 2014 Hisilicon Limited.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>
>
> [...]
>
>
>> +static int hip04_mdio_reset(struct mii_bus *bus)
>> +{
>> +       int temp, err, i;
>> +
>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>> +               hip04_mdio_write(bus, i, 22, 0);
>
>
>    Why? What kind of a register this is? <uapi/linux/mii.h> tells me it's
> MII_SREVISION...

I think this rather means clause 22 as opposed to clause 45.

>
>
>> +               temp = hip04_mdio_read(bus, i, MII_BMCR);
>
>
>    You're not checking for error...
>
>
>> +               temp |= BMCR_RESET;
>> +               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
>
>
>    Hmm, why you're open coding BMCR reset? There's phy_init_hw() doing this
> correctly...

Except that this runs way before we have created the PHY driver, so we
can't use that function just yet. I already asked about this, and he
explained that this was because the PHY devices he uses are not
responding correcty to MII_PHYSID1/2 reads.

>
>
>> +               if (err < 0)
>> +                       return err;
>> +       }
>> +
>> +       mdelay(500);
>> +       return 0;
>> +}
>> +
>> +static int hip04_mdio_probe(struct platform_device *pdev)
>> +{
>> +       struct resource *r;
>> +       struct mii_bus *bus;
>> +       struct hip04_mdio_priv *priv;
>> +       int ret;
>> +
>> +       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
>> +       if (!bus) {
>> +               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       bus->name = "hip04_mdio_bus";
>> +       bus->read = hip04_mdio_read;
>> +       bus->write = hip04_mdio_write;
>> +       bus->reset = hip04_mdio_reset;
>
>
>    Ah... However I don't think it a good implementation of that bus
> method...
>
> [...]
>
> WBR, Sergei
>



-- 
Florian

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Sergei Shtylyov]

On 04/21/2014 10:03 PM, Florian Fainelli wrote:

>>> Hisilicon hip04 platform mdio driver
>>> Reuse Marvell phy drivers/net/phy/marvell.c

>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> [...]

>>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>> new file mode 100644
>>> index 0000000..19826a3
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>> @@ -0,0 +1,185 @@

[...]

>>> +static int hip04_mdio_reset(struct mii_bus *bus)
>>> +{
>>> +       int temp, err, i;
>>> +
>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>>> +               hip04_mdio_write(bus, i, 22, 0);

>>     Why? What kind of a register this is? <uapi/linux/mii.h> tells me it's
>> MII_SREVISION...

> I think this rather means clause 22 as opposed to clause 45.

    No, the corresponding hip04_mdio_write()'s parameter is a register #, so 
this is a write of 0 to register #22. A comment certainly wouldn't hurt here...

>>> +               temp = hip04_mdio_read(bus, i, MII_BMCR);

>>     You're not checking for error...

>>> +               temp |= BMCR_RESET;
>>> +               err = hip04_mdio_write(bus, i, MII_BMCR, temp);

>>     Hmm, why you're open coding BMCR reset? There's phy_init_hw() doing this
>> correctly...

> Except that this runs way before we have created the PHY driver, so we
> can't use that function just yet.

    Ah, you're right.

> I already asked about this, and he
> explained that this was because the PHY devices he uses are not
> responding correcty to MII_PHYSID1/2 reads.

    So, this manual reset loop helps with reading the ID registers? A comment 
wouldn't hurt either...

>>> +               if (err < 0)
>>> +                       return err;

    I'm not at all sure we want to leave the reset loop on a first write error.

>>> +       }
>>> +
>>> +       mdelay(500);

    I'm not sure this is enough, given that in phy_init_hw() we poll for 600 
ms + 1 ms.

>>> +       return 0;
>>> +}
>>> +
>>> +static int hip04_mdio_probe(struct platform_device *pdev)
>>> +{
>>> +       struct resource *r;
>>> +       struct mii_bus *bus;
>>> +       struct hip04_mdio_priv *priv;
>>> +       int ret;
>>> +
>>> +       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
>>> +       if (!bus) {
>>> +               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       bus->name = "hip04_mdio_bus";
>>> +       bus->read = hip04_mdio_read;
>>> +       bus->write = hip04_mdio_write;
>>> +       bus->reset = hip04_mdio_reset;

>>     Ah... However I don't think it a good implementation of that bus
>> method...

    I assumed this method exists to do a hardware reset of the whole bus, not 
to do a loop of soft-resetting all PHYs...

WBR, Sergei

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[zhangfei]

On 04/22/2014 02:21 AM, Sergei Shtylyov wrote:
> On 04/21/2014 10:03 PM, Florian Fainelli wrote:
>
>>>> Hisilicon hip04 platform mdio driver
>>>> Reuse Marvell phy drivers/net/phy/marvell.c
>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>> [...]
>
>>>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>>> new file mode 100644
>>>> index 0000000..19826a3
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>>> @@ -0,0 +1,185 @@
>
> [...]
>
>>>> +static int hip04_mdio_reset(struct mii_bus *bus)
>>>> +{
>>>> +       int temp, err, i;
>>>> +
>>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>>>> +               hip04_mdio_write(bus, i, 22, 0);
>
>>>     Why? What kind of a register this is? <uapi/linux/mii.h> tells me
>>> it's
>>> MII_SREVISION...
>
>> I think this rather means clause 22 as opposed to clause 45.
>
>     No, the corresponding hip04_mdio_write()'s parameter is a register
> #, so this is a write of 0 to register #22. A comment certainly wouldn't
> hurt here...

It's private register of the phy marvell 88e1512.
To make it clearer using define instead.
#define MII_MARVELL_PHY_PAGE    22

The registers has been grouped into several pages, access register need 
choose which page first.
>
>>>> +               temp = hip04_mdio_read(bus, i, MII_BMCR);
>
>>>     You're not checking for error...
>
>>>> +               temp |= BMCR_RESET;
>>>> +               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
>
>>>     Hmm, why you're open coding BMCR reset? There's phy_init_hw()
>>> doing this
>>> correctly...
>
>> Except that this runs way before we have created the PHY driver, so we
>> can't use that function just yet.
>
>     Ah, you're right.
>
>> I already asked about this, and he
>> explained that this was because the PHY devices he uses are not
>> responding correcty to MII_PHYSID1/2 reads.
>
>     So, this manual reset loop helps with reading the ID registers? A
> comment wouldn't hurt either...
Yes, it required for get_phy_id, will add comments.

>
>>>> +               if (err < 0)
>>>> +                       return err;
>
>     I'm not at all sure we want to leave the reset loop on a first write
> error.
Yes, continue can be used instead.

>
>>>> +       }
>>>> +
>>>> +       mdelay(500);
>
>     I'm not sure this is enough, given that in phy_init_hw() we poll for
> 600 ms + 1 ms.
Though not find clear delay time required from the phy spec, the 
experiment shows OK.
Also here can not verify the BMCR_RESET bit, 0xffff will returned if the 
phy is not exists.
>
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int hip04_mdio_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct resource *r;
>>>> +       struct mii_bus *bus;
>>>> +       struct hip04_mdio_priv *priv;
>>>> +       int ret;
>>>> +
>>>> +       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
>>>> +       if (!bus) {
>>>> +               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       bus->name = "hip04_mdio_bus";
>>>> +       bus->read = hip04_mdio_read;
>>>> +       bus->write = hip04_mdio_write;
>>>> +       bus->reset = hip04_mdio_reset;
>
>>>     Ah... However I don't think it a good implementation of that bus
>>> method...
>
>     I assumed this method exists to do a hardware reset of the whole
> bus, not to do a loop of soft-resetting all PHYs...
>

It is required for each phy, if no such reset, the specific phy can NOT be
detected and get_phy_id returns 0.

Thanks

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Arnd Bergmann]

On Tuesday 22 April 2014 14:03:40 zhangfei wrote:
> On 04/22/2014 02:21 AM, Sergei Shtylyov wrote:
> > On 04/21/2014 10:03 PM, Florian Fainelli wrote:
> >
> >>>> Hisilicon hip04 platform mdio driver
> >>>> Reuse Marvell phy drivers/net/phy/marvell.c
> >
> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>> [...]
> >
> >>>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
> >>>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> >>>> new file mode 100644
> >>>> index 0000000..19826a3
> >>>> --- /dev/null
> >>>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> >>>> @@ -0,0 +1,185 @@
> >>>> +static int hip04_mdio_reset(struct mii_bus *bus)
> >>>> +{
> >>>> +       int temp, err, i;
> >>>> +
> >>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
> >>>> +               hip04_mdio_write(bus, i, 22, 0);
> >
> >>>     Why? What kind of a register this is? <uapi/linux/mii.h> tells me
> >>> it's
> >>> MII_SREVISION...
> >
> >> I think this rather means clause 22 as opposed to clause 45.
> >
> >     No, the corresponding hip04_mdio_write()'s parameter is a register
> > #, so this is a write of 0 to register #22. A comment certainly wouldn't
> > hurt here...
> 
> It's private register of the phy marvell 88e1512.
> To make it clearer using define instead.
> #define MII_MARVELL_PHY_PAGE    22
> 
> The registers has been grouped into several pages, access register need 
> choose which page first.

You shouldn't touch the PHY private registers in the main driver though,
this should be purely handled by drivers/net/phy/marvell.c.

I don't see support for 88e1512 there, only 88e1510 and lots of older
ones, but I assume it isn't hard to add.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[zhangfei]

On 04/22/2014 04:22 PM, Arnd Bergmann wrote:

>> It's private register of the phy marvell 88e1512.
>> To make it clearer using define instead.
>> #define MII_MARVELL_PHY_PAGE    22
>>
>> The registers has been grouped into several pages, access register need
>> choose which page first.
>
> You shouldn't touch the PHY private registers in the main driver though,
> this should be purely handled by drivers/net/phy/marvell.c.
>
> I don't see support for 88e1512 there, only 88e1510 and lots of older
> ones, but I assume it isn't hard to add.
>

88e1512 driver is already supported, same as 88e1510.
#define MARVELL_PHY_ID_MASK             0xfffffff0
So it should support 88e151x.

Reset is required here for get_phy_id, otherwise only 0 can be get.
phy_device_create will not be called, and can not match any driver.

However in the experiment, it is found BMCR_RESET is not required in fact.
Only hip04_mdio_write(bus, i, MII_MARVELL_PHY_PAGE, 0) has to be set.
88e151x registers are divided into pages.
Generic MII registers is in page 0, including MII_PHYSID1 and MII_PHYSID2.
Unfortunately the default page is not 0, so get_phy_id will fail.

So bus->reset still required to set the page to 0, prepared for get_phy_id.

Thanks

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Arnd Bergmann]

On Tuesday 22 April 2014, zhangfei wrote:
> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:
> 
> >> It's private register of the phy marvell 88e1512.
> >> To make it clearer using define instead.
> >> #define MII_MARVELL_PHY_PAGE    22
> >>
> >> The registers has been grouped into several pages, access register need
> >> choose which page first.
> >
> > You shouldn't touch the PHY private registers in the main driver though,
> > this should be purely handled by drivers/net/phy/marvell.c.
> >
> > I don't see support for 88e1512 there, only 88e1510 and lots of older
> > ones, but I assume it isn't hard to add.
> >
> 
> 88e1512 driver is already supported, same as 88e1510.
> #define MARVELL_PHY_ID_MASK             0xfffffff0
> So it should support 88e151x.
> 
> Reset is required here for get_phy_id, otherwise only 0 can be get.
> phy_device_create will not be called, and can not match any driver.
> 
> However in the experiment, it is found BMCR_RESET is not required in fact.
> Only hip04_mdio_write(bus, i, MII_MARVELL_PHY_PAGE, 0) has to be set.
> 88e151x registers are divided into pages.
> Generic MII registers is in page 0, including MII_PHYSID1 and MII_PHYSID2.
> Unfortunately the default page is not 0, so get_phy_id will fail.
> 
> So bus->reset still required to set the page to 0, prepared for get_phy_id.

But it means that the hip04_mdio driver potentially won't work if connected
to something other than a Marvell PHY.

I noticed that the marvell_of_reg_init() does this at init time:

saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
... /* perform init */
if (page_changed)
	phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);

Is this a bug? Maybe it should always set page 0 when leaving
this function.

	Arnd

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[zhangfei]

On 04/22/2014 10:30 PM, Arnd Bergmann wrote:
> On Tuesday 22 April 2014, zhangfei wrote:
>> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:
>>
>>>> It's private register of the phy marvell 88e1512.
>>>> To make it clearer using define instead.
>>>> #define MII_MARVELL_PHY_PAGE    22
>>>>
>>>> The registers has been grouped into several pages, access register need
>>>> choose which page first.
>>>
>>> You shouldn't touch the PHY private registers in the main driver though,
>>> this should be purely handled by drivers/net/phy/marvell.c.
>>>
>>> I don't see support for 88e1512 there, only 88e1510 and lots of older
>>> ones, but I assume it isn't hard to add.
>>>
>>
>> 88e1512 driver is already supported, same as 88e1510.
>> #define MARVELL_PHY_ID_MASK             0xfffffff0
>> So it should support 88e151x.
>>
>> Reset is required here for get_phy_id, otherwise only 0 can be get.
>> phy_device_create will not be called, and can not match any driver.
>>
>> However in the experiment, it is found BMCR_RESET is not required in fact.
>> Only hip04_mdio_write(bus, i, MII_MARVELL_PHY_PAGE, 0) has to be set.
>> 88e151x registers are divided into pages.
>> Generic MII registers is in page 0, including MII_PHYSID1 and MII_PHYSID2.
>> Unfortunately the default page is not 0, so get_phy_id will fail.
>>
>> So bus->reset still required to set the page to 0, prepared for get_phy_id.
>
> But it means that the hip04_mdio driver potentially won't work if connected
> to something other than a Marvell PHY.
>
> I noticed that the marvell_of_reg_init() does this at init time:
>
> saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> ... /* perform init */
> if (page_changed)
> 	phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);
>
> Is this a bug? Maybe it should always set page 0 when leaving
> this function.
>
It is correct behavior return to the original page.

First get_phy_id, then match driver according to the id, then operation 
in the specific driver have the chance to run, including 
marvell_of_reg_init etc.

Just unlucky the default value of PHY_PAGE after power on is not 0.

Thanks

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[Arnd Bergmann]

On Tuesday 22 April 2014, zhangfei wrote:
> On 04/22/2014 10:30 PM, Arnd Bergmann wrote:
> > On Tuesday 22 April 2014, zhangfei wrote:
> >> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:

> > But it means that the hip04_mdio driver potentially won't work if connected
> > to something other than a Marvell PHY.
> >
> > I noticed that the marvell_of_reg_init() does this at init time:
> >
> > saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> > ... /* perform init */
> > if (page_changed)
> >       phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);
> >
> > Is this a bug? Maybe it should always set page 0 when leaving
> > this function.
> >
> It is correct behavior return to the original page.
> 
> First get_phy_id, then match driver according to the id, then operation 
> in the specific driver have the chance to run, including 
> marvell_of_reg_init etc.
> 
> Just unlucky the default value of PHY_PAGE after power on is not 0.

Ok, so it's actually not possible to detect this case prior to having
to work around it.

How about adding a DT property as a flag to tell the driver whether
to set the page to zero or not? That would let the driver work
with different types of PHY implementations, or skip the code if
there is firmware that can already set it up to page zero.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/3] net: hisilicon: new hip04 MDIO driver

[zhangfei]

On 04/24/2014 08:28 PM, Arnd Bergmann wrote:
> On Tuesday 22 April 2014, zhangfei wrote:
>> On 04/22/2014 10:30 PM, Arnd Bergmann wrote:
>>> On Tuesday 22 April 2014, zhangfei wrote:
>>>> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:
>
>>> But it means that the hip04_mdio driver potentially won't work if connected
>>> to something other than a Marvell PHY.
>>>
>>> I noticed that the marvell_of_reg_init() does this at init time:
>>>
>>> saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
>>> ... /* perform init */
>>> if (page_changed)
>>>        phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);
>>>
>>> Is this a bug? Maybe it should always set page 0 when leaving
>>> this function.
>>>
>> It is correct behavior return to the original page.
>>
>> First get_phy_id, then match driver according to the id, then operation
>> in the specific driver have the chance to run, including
>> marvell_of_reg_init etc.
>>
>> Just unlucky the default value of PHY_PAGE after power on is not 0.
>
> Ok, so it's actually not possible to detect this case prior to having
> to work around it.
>
> How about adding a DT property as a flag to tell the driver whether
> to set the page to zero or not? That would let the driver work
> with different types of PHY implementations, or skip the code if
> there is firmware that can already set it up to page zero.
>

Currently we suspect it maybe caused by bootloader (uefi) has accessed 
page register and forgot to recover, still in double confirm.
Even it not caused by uefi, we can set to page 0 in uefi, and let mdio 
driver common to other phy.
So we may remove the bus->reset, and let uefi do the preparation.

Thanks

[PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Zhangfei Gao]

Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
The controller has no tx done interrupt, reclaim xmitted buffer in the poll.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/net/ethernet/hisilicon/Makefile    |    2 +-
 drivers/net/ethernet/hisilicon/hip04_eth.c |  846 ++++++++++++++++++++++++++++
 2 files changed, 847 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/hisilicon/hip04_eth.c

diff --git a/drivers/net/ethernet/hisilicon/Makefile b/drivers/net/ethernet/hisilicon/Makefile
index 1d6eb6e..17dec03 100644
--- a/drivers/net/ethernet/hisilicon/Makefile
+++ b/drivers/net/ethernet/hisilicon/Makefile
@@ -2,4 +2,4 @@
 # Makefile for the HISILICON network device drivers.
 #
 
-obj-$(CONFIG_HIP04_ETH) += hip04_mdio.o
+obj-$(CONFIG_HIP04_ETH) += hip04_mdio.o hip04_eth.o
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
new file mode 100644
index 0000000..3a93ec3
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -0,0 +1,846 @@
+
+/* Copyright (c) 2014 Linaro Ltd.
+ * Copyright (c) 2014 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/etherdevice.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/phy.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define PPE_CFG_RX_ADDR			0x100
+#define PPE_CFG_POOL_GRP		0x300
+#define PPE_CFG_RX_BUF_SIZE		0x400
+#define PPE_CFG_RX_FIFO_SIZE		0x500
+#define PPE_CURR_BUF_CNT		0xa200
+
+#define GE_DUPLEX_TYPE			0x08
+#define GE_MAX_FRM_SIZE_REG		0x3c
+#define GE_PORT_MODE			0x40
+#define GE_PORT_EN			0x44
+#define GE_SHORT_RUNTS_THR_REG		0x50
+#define GE_TX_LOCAL_PAGE_REG		0x5c
+#define GE_TRANSMIT_CONTROL_REG		0x60
+#define GE_CF_CRC_STRIP_REG		0x1b0
+#define GE_MODE_CHANGE_REG		0x1b4
+#define GE_RECV_CONTROL_REG		0x1e0
+#define GE_STATION_MAC_ADDRESS		0x210
+#define PPE_CFG_CPU_ADD_ADDR		0x580
+#define PPE_CFG_MAX_FRAME_LEN_REG	0x408
+#define PPE_CFG_BUS_CTRL_REG		0x424
+#define PPE_CFG_RX_CTRL_REG		0x428
+#define PPE_CFG_RX_PKT_MODE_REG		0x438
+#define PPE_CFG_QOS_VMID_GEN		0x500
+#define PPE_CFG_RX_PKT_INT		0x538
+#define PPE_INTEN			0x600
+#define PPE_INTSTS			0x608
+#define PPE_RINT			0x604
+#define PPE_CFG_STS_MODE		0x700
+#define PPE_HIS_RX_PKT_CNT		0x804
+
+/* REG_INTERRUPT */
+#define RCV_INT				BIT(10)
+#define RCV_NOBUF			BIT(8)
+#define RCV_DROP			BIT(7)
+#define TX_DROP				BIT(6)
+#define DEF_INT_ERR			(RCV_NOBUF | RCV_DROP | TX_DROP)
+#define DEF_INT_MASK			(RCV_INT | DEF_INT_ERR)
+
+/* TX descriptor config */
+#define TX_FREE_MEM			BIT(0)
+#define TX_READ_ALLOC_L3		BIT(1)
+#define TX_FINISH_CACHE_INV		BIT(2)
+#define TX_CLEAR_WB			BIT(4)
+#define TX_L3_CHECKSUM			BIT(5)
+#define TX_LOOP_BACK			BIT(11)
+
+/* RX error */
+#define RX_PKT_DROP			BIT(0)
+#define RX_L2_ERR			BIT(1)
+#define RX_PKT_ERR			(RX_PKT_DROP | RX_L2_ERR)
+
+#define SGMII_SPEED_1000		0x08
+#define SGMII_SPEED_100			0x07
+#define SGMII_SPEED_10			0x06
+#define MII_SPEED_100			0x01
+#define MII_SPEED_10			0x00
+
+#define GE_DUPLEX_FULL			BIT(0)
+#define GE_DUPLEX_HALF			0x00
+#define GE_MODE_CHANGE_EN		BIT(0)
+
+#define GE_TX_AUTO_NEG			BIT(5)
+#define GE_TX_ADD_CRC			BIT(6)
+#define GE_TX_SHORT_PAD_THROUGH		BIT(7)
+
+#define GE_RX_STRIP_CRC			BIT(0)
+#define GE_RX_STRIP_PAD			BIT(3)
+#define GE_RX_PAD_EN			BIT(4)
+
+#define GE_AUTO_NEG_CTL			BIT(0)
+
+#define GE_RX_INT_THRESHOLD		BIT(6)
+#define GE_RX_TIMEOUT			0x04
+
+#define GE_RX_PORT_EN			BIT(1)
+#define GE_TX_PORT_EN			BIT(2)
+
+#define PPE_CFG_STS_RX_PKT_CNT_RC	BIT(12)
+
+#define PPE_CFG_RX_PKT_ALIGN		BIT(18)
+#define PPE_CFG_QOS_VMID_MODE		BIT(14)
+#define PPE_CFG_QOS_VMID_GRP_SHIFT	8
+
+#define PPE_CFG_RX_FIFO_FSFU		BIT(11)
+#define PPE_CFG_RX_DEPTH_SHIFT		16
+#define PPE_CFG_RX_START_SHIFT		0
+#define PPE_CFG_RX_CTRL_ALIGN_SHIFT	11
+
+#define PPE_CFG_BUS_LOCAL_REL		BIT(14)
+#define PPE_CFG_BUS_BIG_ENDIEN		BIT(0)
+
+#define RX_DESC_NUM			128
+#define TX_DESC_NUM			256
+#define TX_NEXT(N)			(((N) + 1) & (TX_DESC_NUM-1))
+#define RX_NEXT(N)			(((N) + 1) & (RX_DESC_NUM-1))
+
+#define GMAC_PPE_RX_PKT_MAX_LEN		379
+#define GMAC_MAX_PKT_LEN		1516
+#define GMAC_MIN_PKT_LEN		31
+#define RX_BUF_SIZE			1600
+#define RESET_TIMEOUT			1000
+#define TX_TIMEOUT			(6 * HZ)
+
+#define DRV_NAME			"hip04-ether"
+
+struct tx_desc {
+	u32 send_addr;
+	u32 send_size;
+	u32 next_addr;
+	u32 cfg;
+	u32 wb_addr;
+} __aligned(64);
+
+struct rx_desc {
+	u16 reserved_16;
+	u16 pkt_len;
+	u32 reserve1[3];
+	u32 pkt_err;
+	u32 reserve2[4];
+};
+
+struct hip04_priv {
+	void __iomem *base;
+	int phy_mode;
+	int chan;
+	unsigned int port;
+	unsigned int speed;
+	unsigned int duplex;
+	unsigned int reg_inten;
+
+	struct napi_struct napi;
+	struct net_device *ndev;
+
+	struct tx_desc *tx_desc;
+	dma_addr_t tx_desc_dma;
+	struct sk_buff *tx_skb[TX_DESC_NUM];
+	dma_addr_t tx_phys[TX_DESC_NUM];
+	unsigned int tx_head;
+	unsigned int tx_tail;
+	int tx_count;
+
+	unsigned char *rx_buf[RX_DESC_NUM];
+	dma_addr_t rx_phys[RX_DESC_NUM];
+	unsigned int rx_head;
+	unsigned int rx_buf_size;
+
+	struct device_node *phy_node;
+	struct phy_device *phy;
+	struct regmap *map;
+	struct work_struct tx_timeout_task;
+};
+
+static void hip04_config_port(struct net_device *ndev, u32 speed, u32 duplex)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	u32 val;
+
+	priv->speed = speed;
+	priv->duplex = duplex;
+
+	switch (priv->phy_mode) {
+	case PHY_INTERFACE_MODE_SGMII:
+		if (speed == SPEED_1000)
+			val = SGMII_SPEED_1000;
+		else if (speed == SPEED_100)
+			val = SGMII_SPEED_100;
+		else
+			val = SGMII_SPEED_10;
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		if (speed == SPEED_100)
+			val = MII_SPEED_100;
+		else
+			val = MII_SPEED_10;
+		break;
+	default:
+		netdev_warn(ndev, "not supported mode\n");
+		val = MII_SPEED_10;
+		break;
+	}
+	writel_relaxed(val, priv->base + GE_PORT_MODE);
+
+	val = duplex ? GE_DUPLEX_FULL : GE_DUPLEX_HALF;
+	writel_relaxed(val, priv->base + GE_DUPLEX_TYPE);
+
+	val = GE_MODE_CHANGE_EN;
+	writel_relaxed(val, priv->base + GE_MODE_CHANGE_REG);
+}
+
+static void hip04_reset_ppe(struct hip04_priv *priv)
+{
+	u32 val, tmp, timeout = 0;
+
+	do {
+		regmap_read(priv->map, priv->port * 4 + PPE_CURR_BUF_CNT, &val);
+		regmap_read(priv->map, priv->port * 4 + PPE_CFG_RX_ADDR, &tmp);
+		if (timeout++ > RESET_TIMEOUT)
+			break;
+	} while (val & 0xfff);
+}
+
+static void hip04_config_fifo(struct hip04_priv *priv)
+{
+	u32 val;
+
+	val = readl_relaxed(priv->base + PPE_CFG_STS_MODE);
+	val |= PPE_CFG_STS_RX_PKT_CNT_RC;
+	writel_relaxed(val, priv->base + PPE_CFG_STS_MODE);
+
+	val = BIT(priv->port);
+	regmap_write(priv->map, priv->port * 4 + PPE_CFG_POOL_GRP, val);
+
+	val = priv->port << PPE_CFG_QOS_VMID_GRP_SHIFT;
+	val |= PPE_CFG_QOS_VMID_MODE;
+	writel_relaxed(val, priv->base + PPE_CFG_QOS_VMID_GEN);
+
+	val = RX_BUF_SIZE;
+	regmap_write(priv->map, priv->port * 4 + PPE_CFG_RX_BUF_SIZE, val);
+
+	val = RX_DESC_NUM << PPE_CFG_RX_DEPTH_SHIFT;
+	val |= PPE_CFG_RX_FIFO_FSFU;
+	val |= priv->chan << PPE_CFG_RX_START_SHIFT;
+	regmap_write(priv->map, priv->port * 4 + PPE_CFG_RX_FIFO_SIZE, val);
+
+	val = NET_IP_ALIGN << PPE_CFG_RX_CTRL_ALIGN_SHIFT;
+	writel_relaxed(val, priv->base + PPE_CFG_RX_CTRL_REG);
+
+	val = PPE_CFG_RX_PKT_ALIGN;
+	writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_MODE_REG);
+
+	val = PPE_CFG_BUS_LOCAL_REL | PPE_CFG_BUS_BIG_ENDIEN;
+	writel_relaxed(val, priv->base + PPE_CFG_BUS_CTRL_REG);
+
+	val = GMAC_PPE_RX_PKT_MAX_LEN;
+	writel_relaxed(val, priv->base + PPE_CFG_MAX_FRAME_LEN_REG);
+
+	val = GMAC_MAX_PKT_LEN;
+	writel_relaxed(val, priv->base + GE_MAX_FRM_SIZE_REG);
+
+	val = GMAC_MIN_PKT_LEN;
+	writel_relaxed(val, priv->base + GE_SHORT_RUNTS_THR_REG);
+
+	val = readl_relaxed(priv->base + GE_TRANSMIT_CONTROL_REG);
+	val |= GE_TX_AUTO_NEG | GE_TX_ADD_CRC | GE_TX_SHORT_PAD_THROUGH;
+	writel_relaxed(val, priv->base + GE_TRANSMIT_CONTROL_REG);
+
+	val = GE_RX_STRIP_CRC;
+	writel_relaxed(val, priv->base + GE_CF_CRC_STRIP_REG);
+
+	val = readl_relaxed(priv->base + GE_RECV_CONTROL_REG);
+	val |= GE_RX_STRIP_PAD | GE_RX_PAD_EN;
+	writel_relaxed(val, priv->base + GE_RECV_CONTROL_REG);
+
+	val = GE_AUTO_NEG_CTL;
+	writel_relaxed(val, priv->base + GE_TX_LOCAL_PAGE_REG);
+}
+
+static void hip04_mac_enable(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	u32 val;
+
+	/* enable tx & rx */
+	val = readl_relaxed(priv->base + GE_PORT_EN);
+	val |= GE_RX_PORT_EN | GE_TX_PORT_EN;
+	writel_relaxed(val, priv->base + GE_PORT_EN);
+
+	/* clear rx int */
+	val = RCV_INT;
+	writel_relaxed(val, priv->base + PPE_RINT);
+
+	/* config recv int */
+	val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT;
+	writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT);
+
+	/* enable interrupt */
+	priv->reg_inten = DEF_INT_MASK;
+	writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+}
+
+static void hip04_mac_disable(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	u32 val;
+
+	/* disable int */
+	priv->reg_inten &= ~(DEF_INT_MASK);
+	writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+
+	/* disable tx & rx */
+	val = readl_relaxed(priv->base + GE_PORT_EN);
+	val &= ~(GE_RX_PORT_EN | GE_TX_PORT_EN);
+	writel_relaxed(val, priv->base + GE_PORT_EN);
+}
+
+static void hip04_set_xmit_desc(struct hip04_priv *priv, dma_addr_t phys)
+{
+	writel(phys, priv->base + PPE_CFG_CPU_ADD_ADDR);
+}
+
+static void hip04_set_recv_desc(struct hip04_priv *priv, dma_addr_t phys)
+{
+	regmap_write(priv->map, priv->port * 4 + PPE_CFG_RX_ADDR, phys);
+}
+
+static u32 hip04_recv_cnt(struct hip04_priv *priv)
+{
+	return readl(priv->base + PPE_HIS_RX_PKT_CNT);
+}
+
+static void hip04_update_mac_address(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+
+	writel_relaxed(((ndev->dev_addr[0] << 8) | (ndev->dev_addr[1])),
+			priv->base + GE_STATION_MAC_ADDRESS);
+	writel_relaxed(((ndev->dev_addr[2] << 24) | (ndev->dev_addr[3] << 16) |
+			(ndev->dev_addr[4] << 8) | (ndev->dev_addr[5])),
+			priv->base + GE_STATION_MAC_ADDRESS + 4);
+}
+
+static int hip04_set_mac_address(struct net_device *ndev, void *addr)
+{
+	eth_mac_addr(ndev, addr);
+	hip04_update_mac_address(ndev);
+	return 0;
+}
+
+static void hip04_tx_reclaim(struct net_device *ndev, bool force)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	unsigned tx_tail = priv->tx_tail;
+	struct tx_desc *desc;
+	unsigned int bytes_compl = 0, pkts_compl = 0;
+
+	netif_tx_lock(ndev);
+	if (priv->tx_count == 0)
+		goto out;
+
+	while ((tx_tail != priv->tx_head) || (priv->tx_count == TX_DESC_NUM)) {
+		desc = &priv->tx_desc[priv->tx_tail];
+		if (desc->send_addr != 0) {
+			if (force)
+				desc->send_addr = 0;
+			else
+				break;
+		}
+
+		if (priv->tx_phys[tx_tail]) {
+			dma_unmap_single(&ndev->dev, priv->tx_phys[tx_tail],
+				priv->tx_skb[tx_tail]->len, DMA_TO_DEVICE);
+			priv->tx_phys[tx_tail] = 0;
+		}
+		pkts_compl++;
+		bytes_compl += priv->tx_skb[tx_tail]->len;
+		dev_kfree_skb(priv->tx_skb[tx_tail]);
+		priv->tx_skb[tx_tail] = NULL;
+		tx_tail = TX_NEXT(tx_tail);
+		priv->tx_count--;
+
+		if (priv->tx_count <= 0)
+			break;
+	}
+
+	priv->tx_tail = tx_tail;
+
+	/* Ensure tx_tail & tx_count visible to xmit */
+	smp_mb();
+out:
+	netif_tx_unlock(ndev);
+
+	if (pkts_compl || bytes_compl)
+		netdev_completed_queue(ndev, pkts_compl, bytes_compl);
+
+	if (unlikely(netif_queue_stopped(ndev)) &&
+		(priv->tx_count < TX_DESC_NUM))
+		netif_wake_queue(ndev);
+}
+
+static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	unsigned int tx_head = priv->tx_head;
+	struct tx_desc *desc = &priv->tx_desc[tx_head];
+	dma_addr_t phys;
+
+	if (priv->tx_count >= TX_DESC_NUM) {
+		netif_stop_queue(ndev);
+		return NETDEV_TX_BUSY;
+	}
+
+	phys = dma_map_single(&ndev->dev, skb->data, skb->len, DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, phys)) {
+		dev_kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
+	priv->tx_skb[tx_head] = skb;
+	priv->tx_phys[tx_head] = phys;
+	desc->send_addr = cpu_to_be32(phys);
+	desc->send_size = cpu_to_be32(skb->len);
+	desc->cfg = cpu_to_be32(TX_CLEAR_WB | TX_FINISH_CACHE_INV);
+	phys = priv->tx_desc_dma + tx_head * sizeof(struct tx_desc);
+	desc->wb_addr = cpu_to_be32(phys);
+	skb_tx_timestamp(skb);
+
+	/* Don't wait up for transmitted skbs to be freed. */
+	skb_orphan(skb);
+
+	hip04_set_xmit_desc(priv, phys);
+	priv->tx_head = TX_NEXT(tx_head);
+	netdev_sent_queue(ndev, skb->len);
+
+	stats->tx_bytes += skb->len;
+	stats->tx_packets++;
+	priv->tx_count++;
+
+	/* Ensure tx_head & tx_count update visible to tx reclaim */
+	smp_mb();
+	napi_schedule(&priv->napi);
+
+	return NETDEV_TX_OK;
+}
+
+static int hip04_rx_poll(struct napi_struct *napi, int budget)
+{
+	struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
+	struct net_device *ndev = priv->ndev;
+	struct net_device_stats *stats = &ndev->stats;
+	unsigned int cnt = hip04_recv_cnt(priv);
+	struct rx_desc *desc;
+	struct sk_buff *skb;
+	unsigned char *buf;
+	bool last = false;
+	dma_addr_t phys;
+	int rx = 0;
+	u16 len;
+	u32 err;
+
+	while (cnt && !last) {
+		buf = priv->rx_buf[priv->rx_head];
+		skb = build_skb(buf, priv->rx_buf_size);
+		if (unlikely(!skb))
+			net_dbg_ratelimited("build_skb failed\n");
+
+		dma_unmap_single(&ndev->dev, priv->rx_phys[priv->rx_head],
+				RX_BUF_SIZE, DMA_FROM_DEVICE);
+		priv->rx_phys[priv->rx_head] = 0;
+
+		desc = (struct rx_desc *)skb->data;
+		len = be16_to_cpu(desc->pkt_len);
+		err = be32_to_cpu(desc->pkt_err);
+
+		if (0 == len) {
+			dev_kfree_skb_any(skb);
+			last = true;
+		} else if ((err & RX_PKT_ERR) || (len >= GMAC_MAX_PKT_LEN)) {
+			dev_kfree_skb_any(skb);
+			stats->rx_dropped++;
+			stats->rx_errors++;
+		} else {
+			skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
+			skb_put(skb, len);
+			skb->protocol = eth_type_trans(skb, ndev);
+			napi_gro_receive(&priv->napi, skb);
+			stats->rx_packets++;
+			stats->rx_bytes += len;
+			rx++;
+		}
+
+		buf = netdev_alloc_frag(priv->rx_buf_size);
+		if (!buf)
+			return -ENOMEM;
+		phys = dma_map_single(&ndev->dev, buf,
+				RX_BUF_SIZE, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, phys))
+			return -EIO;
+		priv->rx_buf[priv->rx_head] = buf;
+		priv->rx_phys[priv->rx_head] = phys;
+		hip04_set_recv_desc(priv, phys);
+
+		priv->rx_head = RX_NEXT(priv->rx_head);
+		if (rx >= budget)
+			goto done;
+
+		if (--cnt == 0)
+			cnt = hip04_recv_cnt(priv);
+	}
+
+	/* reclaim in the poll since no tx done interrupt */
+	hip04_tx_reclaim(ndev, false);
+
+	if (!(priv->reg_inten & RCV_INT)) {
+		/* enable rx interrupt */
+		priv->reg_inten |= RCV_INT;
+		writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+	}
+	napi_complete(napi);
+done:
+	return rx;
+}
+
+static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
+{
+	struct net_device *ndev = (struct net_device *) dev_id;
+	struct hip04_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	u32 ists = readl_relaxed(priv->base + PPE_INTSTS);
+
+	writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
+
+	if (unlikely(ists & DEF_INT_ERR)) {
+		if (ists & (RCV_NOBUF | RCV_DROP))
+			stats->rx_errors++;
+			stats->rx_dropped++;
+			netdev_err(ndev, "rx drop\n");
+		if (ists & TX_DROP) {
+			stats->tx_dropped++;
+			netdev_err(ndev, "tx drop\n");
+		}
+	}
+
+	if (ists & RCV_INT) {
+		/* disable rx interrupt */
+		priv->reg_inten &= ~(RCV_INT);
+		writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+		napi_schedule(&priv->napi);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void hip04_adjust_link(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	struct phy_device *phy = priv->phy;
+
+	if ((priv->speed != phy->speed) || (priv->duplex != phy->duplex)) {
+		hip04_config_port(ndev, phy->speed, phy->duplex);
+		phy_print_status(phy);
+	}
+}
+
+static int hip04_mac_open(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	int i;
+
+	priv->rx_head = 0;
+	priv->tx_head = 0;
+	priv->tx_tail = 0;
+	priv->tx_count = 0;
+	hip04_reset_ppe(priv);
+
+	for (i = 0; i < RX_DESC_NUM; i++) {
+		dma_addr_t phys;
+
+		phys = dma_map_single(&ndev->dev, priv->rx_buf[i],
+				RX_BUF_SIZE, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, phys))
+			return -EIO;
+
+		priv->rx_phys[i] = phys;
+		hip04_set_recv_desc(priv, phys);
+	}
+
+	if (priv->phy)
+		phy_start(priv->phy);
+
+	netdev_reset_queue(ndev);
+	netif_start_queue(ndev);
+	hip04_mac_enable(ndev);
+	napi_enable(&priv->napi);
+
+	return 0;
+}
+
+static int hip04_mac_stop(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	int i;
+
+	napi_disable(&priv->napi);
+	netif_stop_queue(ndev);
+	hip04_mac_disable(ndev);
+	hip04_tx_reclaim(ndev, true);
+	hip04_reset_ppe(priv);
+
+	if (priv->phy)
+		phy_stop(priv->phy);
+
+	for (i = 0; i < RX_DESC_NUM; i++) {
+		if (priv->rx_phys[i]) {
+			dma_unmap_single(&ndev->dev, priv->rx_phys[i],
+					RX_BUF_SIZE, DMA_FROM_DEVICE);
+			priv->rx_phys[i] = 0;
+		}
+	}
+
+	return 0;
+}
+
+static void hip04_timeout(struct net_device *ndev)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+
+	schedule_work(&priv->tx_timeout_task);
+}
+
+static void hip04_tx_timeout_task(struct work_struct *work)
+{
+	struct hip04_priv *priv;
+
+	priv = container_of(work, struct hip04_priv, tx_timeout_task);
+	hip04_mac_stop(priv->ndev);
+	hip04_mac_open(priv->ndev);
+	return;
+}
+
+static struct net_device_stats *hip04_get_stats(struct net_device *ndev)
+{
+	return &ndev->stats;
+}
+
+static struct net_device_ops hip04_netdev_ops = {
+	.ndo_open		= hip04_mac_open,
+	.ndo_stop		= hip04_mac_stop,
+	.ndo_get_stats		= hip04_get_stats,
+	.ndo_start_xmit		= hip04_mac_start_xmit,
+	.ndo_set_mac_address	= hip04_set_mac_address,
+	.ndo_tx_timeout         = hip04_timeout,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_change_mtu		= eth_change_mtu,
+};
+
+static int hip04_alloc_ring(struct net_device *ndev, struct device *d)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	int i;
+
+	priv->tx_desc = dma_alloc_coherent(d,
+			TX_DESC_NUM * sizeof(struct tx_desc),
+			&priv->tx_desc_dma, GFP_KERNEL);
+	if (!priv->tx_desc)
+		return -ENOMEM;
+
+	priv->rx_buf_size = RX_BUF_SIZE +
+			    SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	for (i = 0; i < RX_DESC_NUM; i++) {
+		priv->rx_buf[i] = netdev_alloc_frag(priv->rx_buf_size);
+		if (!priv->rx_buf[i])
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void hip04_free_ring(struct net_device *ndev, struct device *d)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	int i;
+
+	for (i = 0; i < RX_DESC_NUM; i++)
+		if (priv->rx_buf[i])
+			put_page(virt_to_head_page(priv->rx_buf[i]));
+
+	for (i = 0; i < TX_DESC_NUM; i++)
+		if (priv->tx_skb[i])
+			dev_kfree_skb_any(priv->tx_skb[i]);
+
+	dma_free_coherent(d, TX_DESC_NUM * sizeof(struct tx_desc),
+			priv->tx_desc, priv->tx_desc_dma);
+}
+
+static int hip04_mac_probe(struct platform_device *pdev)
+{
+	struct device *d = &pdev->dev;
+	struct device_node *node = d->of_node;
+	struct of_phandle_args arg;
+	struct net_device *ndev;
+	struct hip04_priv *priv;
+	struct resource *res;
+	unsigned int irq;
+	int ret;
+
+	ndev = alloc_etherdev(sizeof(struct hip04_priv));
+	if (!ndev)
+		return -ENOMEM;
+
+	priv = netdev_priv(ndev);
+	priv->ndev = ndev;
+	platform_set_drvdata(pdev, ndev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(d, res);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto init_fail;
+	}
+
+	ret = of_parse_phandle_with_fixed_args(node, "port-handle", 2, 0, &arg);
+	if (ret < 0) {
+		dev_warn(d, "no port-handle\n");
+		goto init_fail;
+	}
+
+	priv->port = arg.args[0];
+	priv->chan = arg.args[1] * RX_DESC_NUM;
+
+	priv->map = syscon_node_to_regmap(arg.np);
+	if (IS_ERR(priv->map)) {
+		dev_warn(d, "no syscon hisilicon,hip04-ppe\n");
+		ret = PTR_ERR(priv->map);
+		goto init_fail;
+	}
+
+	priv->phy_mode = of_get_phy_mode(node);
+	if (priv->phy_mode < 0) {
+		dev_warn(d, "not find phy-mode\n");
+		ret = -EINVAL;
+		goto init_fail;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		ret = -EINVAL;
+		goto init_fail;
+	}
+
+	ret = devm_request_irq(d, irq, hip04_mac_interrupt,
+				0, pdev->name, ndev);
+	if (ret) {
+		netdev_err(ndev, "devm_request_irq failed\n");
+		goto init_fail;
+	}
+
+	priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
+	if (priv->phy_node) {
+		priv->phy = of_phy_connect(ndev, priv->phy_node,
+			&hip04_adjust_link, 0, priv->phy_mode);
+		if (!priv->phy) {
+			ret = -EPROBE_DEFER;
+			goto init_fail;
+		}
+	}
+
+	INIT_WORK(&priv->tx_timeout_task, hip04_tx_timeout_task);
+
+	ether_setup(ndev);
+	ndev->netdev_ops = &hip04_netdev_ops;
+	ndev->watchdog_timeo = TX_TIMEOUT;
+	ndev->priv_flags |= IFF_UNICAST_FLT;
+	ndev->irq = irq;
+	netif_napi_add(ndev, &priv->napi, hip04_rx_poll, NAPI_POLL_WEIGHT);
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	hip04_reset_ppe(priv);
+	if (priv->phy_mode == PHY_INTERFACE_MODE_MII)
+		hip04_config_port(ndev, SPEED_100, DUPLEX_FULL);
+
+	hip04_config_fifo(priv);
+	random_ether_addr(ndev->dev_addr);
+	hip04_update_mac_address(ndev);
+
+	ret = hip04_alloc_ring(ndev, d);
+	if (ret) {
+		netdev_err(ndev, "alloc ring fail\n");
+		goto alloc_fail;
+	}
+
+	ret = register_netdev(ndev);
+	if (ret) {
+		free_netdev(ndev);
+		goto alloc_fail;
+	}
+
+	return 0;
+
+alloc_fail:
+	hip04_free_ring(ndev, d);
+init_fail:
+	of_node_put(priv->phy_node);
+	free_netdev(ndev);
+	return ret;
+}
+
+static int hip04_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct hip04_priv *priv = netdev_priv(ndev);
+	struct device *d = &pdev->dev;
+
+	if (priv->phy)
+		phy_disconnect(priv->phy);
+
+	hip04_free_ring(ndev, d);
+	unregister_netdev(ndev);
+	free_irq(ndev->irq, ndev);
+	of_node_put(priv->phy_node);
+	cancel_work_sync(&priv->tx_timeout_task);
+	free_netdev(ndev);
+
+	return 0;
+}
+
+static const struct of_device_id hip04_mac_match[] = {
+	{ .compatible = "hisilicon,hip04-mac" },
+	{ }
+};
+
+static struct platform_driver hip04_mac_driver = {
+	.probe	= hip04_mac_probe,
+	.remove	= hip04_remove,
+	.driver	= {
+		.name		= DRV_NAME,
+		.owner		= THIS_MODULE,
+		.of_match_table	= hip04_mac_match,
+	},
+};
+module_platform_driver(hip04_mac_driver);
+
+MODULE_DESCRIPTION("HISILICON P04 Ethernet driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hip04-ether");
-- 
1.7.9.5

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Alexander Graf]

On 19.04.14 03:13, Zhangfei Gao wrote:
> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>

Is this driver still supposed to go upstream? I presume this was the
last submission and it's been quite some time ago :)

> ---
>  drivers/net/ethernet/hisilicon/Makefile    |    2 +-
>  drivers/net/ethernet/hisilicon/hip04_eth.c |  846 ++++++++++++++++++++++++++++
>  2 files changed, 847 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/hisilicon/hip04_eth.c
> 
> diff --git a/drivers/net/ethernet/hisilicon/Makefile b/drivers/net/ethernet/hisilicon/Makefile
> index 1d6eb6e..17dec03 100644

[...]

> +static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = (struct net_device *) dev_id;
> +	struct hip04_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	u32 ists = readl_relaxed(priv->base + PPE_INTSTS);
> +
> +	writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
> +
> +	if (unlikely(ists & DEF_INT_ERR)) {
> +		if (ists & (RCV_NOBUF | RCV_DROP))
> +			stats->rx_errors++;
> +			stats->rx_dropped++;
> +			netdev_err(ndev, "rx drop\n");

Why would a user want to see this as a dmesg error? We already have
stats for packet drops, no?

> +		if (ists & TX_DROP) {
> +			stats->tx_dropped++;
> +			netdev_err(ndev, "tx drop\n");

Same here.

> +		}
> +	}
> +
> +	if (ists & RCV_INT) {
> +		/* disable rx interrupt */
> +		priv->reg_inten &= ~(RCV_INT);
> +		writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +		napi_schedule(&priv->napi);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[...]

> +
> +static const struct of_device_id hip04_mac_match[] = {
> +	{ .compatible = "hisilicon,hip04-mac" },
> +	{ }
> +};

This is missing the magic macro that actually would make module aliases
work.


Alex

> +
> +static struct platform_driver hip04_mac_driver = {
> +	.probe	= hip04_mac_probe,
> +	.remove	= hip04_remove,
> +	.driver	= {
> +		.name		= DRV_NAME,
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= hip04_mac_match,
> +	},
> +};
> +module_platform_driver(hip04_mac_driver);
> +
> +MODULE_DESCRIPTION("HISILICON P04 Ethernet driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:hip04-ether");
> 

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Ding Tianhong]

On 2014/12/7 8:42, Alexander Graf wrote:
> On 19.04.14 03:13, Zhangfei Gao wrote:
>> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
>> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> 
> Is this driver still supposed to go upstream? I presume this was the
> last submission and it's been quite some time ago :)
> 

yes, it is really a long time, but The hip04 did not support tx irq, 
we couldn't get any better idea to fix this defect, do you have any suggestion?

Regards
Ding

>> ---
>>  drivers/net/ethernet/hisilicon/Makefile    |    2 +-
>>  drivers/net/ethernet/hisilicon/hip04_eth.c |  846 ++++++++++++++++++++++++++++
>>  2 files changed, 847 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/net/ethernet/hisilicon/hip04_eth.c
>>
>> diff --git a/drivers/net/ethernet/hisilicon/Makefile b/drivers/net/ethernet/hisilicon/Makefile
>> index 1d6eb6e..17dec03 100644
> 
> [...]
> 
>> +static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
>> +{
>> +	struct net_device *ndev = (struct net_device *) dev_id;
>> +	struct hip04_priv *priv = netdev_priv(ndev);
>> +	struct net_device_stats *stats = &ndev->stats;
>> +	u32 ists = readl_relaxed(priv->base + PPE_INTSTS);
>> +
>> +	writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
>> +
>> +	if (unlikely(ists & DEF_INT_ERR)) {
>> +		if (ists & (RCV_NOBUF | RCV_DROP))
>> +			stats->rx_errors++;
>> +			stats->rx_dropped++;
>> +			netdev_err(ndev, "rx drop\n");
> 
> Why would a user want to see this as a dmesg error? We already have
> stats for packet drops, no?
> 
>> +		if (ists & TX_DROP) {
>> +			stats->tx_dropped++;
>> +			netdev_err(ndev, "tx drop\n");
> 
> Same here.
> 
>> +		}
>> +	}
>> +
>> +	if (ists & RCV_INT) {
>> +		/* disable rx interrupt */
>> +		priv->reg_inten &= ~(RCV_INT);
>> +		writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
>> +		napi_schedule(&priv->napi);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> [...]
> 
>> +
>> +static const struct of_device_id hip04_mac_match[] = {
>> +	{ .compatible = "hisilicon,hip04-mac" },
>> +	{ }
>> +};
> 
> This is missing the magic macro that actually would make module aliases
> work.
> 
> 
> Alex
> 
>> +
>> +static struct platform_driver hip04_mac_driver = {
>> +	.probe	= hip04_mac_probe,
>> +	.remove	= hip04_remove,
>> +	.driver	= {
>> +		.name		= DRV_NAME,
>> +		.owner		= THIS_MODULE,
>> +		.of_match_table	= hip04_mac_match,
>> +	},
>> +};
>> +module_platform_driver(hip04_mac_driver);
>> +
>> +MODULE_DESCRIPTION("HISILICON P04 Ethernet driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:hip04-ether");
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Alexander Graf]

On 07.12.14 04:28, Ding Tianhong wrote:
> On 2014/12/7 8:42, Alexander Graf wrote:
>> On 19.04.14 03:13, Zhangfei Gao wrote:
>>> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
>>> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
>>>
>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>
>> Is this driver still supposed to go upstream? I presume this was the
>> last submission and it's been quite some time ago :)
>>
> 
> yes, it is really a long time, but The hip04 did not support tx irq, 
> we couldn't get any better idea to fix this defect, do you have any suggestion?

Well, if hardware doesn't have a TX irq I don't see there's anything we
can do to fix that ;).

Dave, what's your take here? Should we keep a driver from going upstream
just because the hardware is partly broken? I'd really prefer to have an
upstream driver on that SoC rather than some random (eventually even
more broken) downstream code.


Alex

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Arnd Bergmann]

On Sunday 07 December 2014 10:49:12 Alexander Graf wrote:
> On 07.12.14 04:28, Ding Tianhong wrote:
> > On 2014/12/7 8:42, Alexander Graf wrote:
> >> On 19.04.14 03:13, Zhangfei Gao wrote:
> >>> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
> >>> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
> >>>
> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>
> >> Is this driver still supposed to go upstream? I presume this was the
> >> last submission and it's been quite some time ago 
> >>
> > 
> > yes, it is really a long time, but The hip04 did not support tx irq, 
> > we couldn't get any better idea to fix this defect, do you have any suggestion?
> 
> Well, if hardware doesn't have a TX irq I don't see there's anything we
> can do to fix that ;).

I don't know if it's related to the ethernet on hip01, but I would assume
it is, and that platform is currently being submitted for inclusion, so
I'd definitely hope to see this driver get merged too eventually.

IIRC, the last revision of the patch set had basically fixed the problem,
except for a race that would still allow the napi poll function to exit
with poll_complete() but a full queue of TX descriptors and no fallback
to clean them up. There was also still an open question about whether or
not the driver should use skb_orphan, but I may be misremembering that part.
 
> Dave, what's your take here? Should we keep a driver from going upstream
> just because the hardware is partly broken? I'd really prefer to have an
> upstream driver on that SoC rather than some random (eventually even
> more broken) downstream code.

We can certainly have a slow driver for this hardware, and I'd much
prefer slow over broken. I'd guess that some of the performance impact
of the missing interrupts can now be offset with the xmit_more	 logic.

	Arnd

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Ding Tianhong]

On 2014/12/8 4:09, Arnd Bergmann wrote:
> On Sunday 07 December 2014 10:49:12 Alexander Graf wrote:
>> On 07.12.14 04:28, Ding Tianhong wrote:
>>> On 2014/12/7 8:42, Alexander Graf wrote:
>>>> On 19.04.14 03:13, Zhangfei Gao wrote:
>>>>> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
>>>>> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
>>>>>
>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>
>>>> Is this driver still supposed to go upstream? I presume this was the
>>>> last submission and it's been quite some time ago 
>>>>
>>>
>>> yes, it is really a long time, but The hip04 did not support tx irq, 
>>> we couldn't get any better idea to fix this defect, do you have any suggestion?
>>
>> Well, if hardware doesn't have a TX irq I don't see there's anything we
>> can do to fix that ;).
> 
> I don't know if it's related to the ethernet on hip01, but I would assume
> it is, and that platform is currently being submitted for inclusion, so
> I'd definitely hope to see this driver get merged too eventually.
> 
> IIRC, the last revision of the patch set had basically fixed the problem,
> except for a race that would still allow the napi poll function to exit
> with poll_complete() but a full queue of TX descriptors and no fallback
> to clean them up. There was also still an open question about whether or
> not the driver should use skb_orphan, but I may be misremembering that part.
>  
>> Dave, what's your take here? Should we keep a driver from going upstream
>> just because the hardware is partly broken? I'd really prefer to have an
>> upstream driver on that SoC rather than some random (eventually even
>> more broken) downstream code.
> 
> We can certainly have a slow driver for this hardware, and I'd much
> prefer slow over broken. I'd guess that some of the performance impact
> of the missing interrupts can now be offset with the xmit_more	 logic.
> 
> 	Arnd

Ok, if so, I can modify this patch set and send them again base on zhangfei's last version
just for reviewing.

Regards
Ding

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Ding Tianhong]

On 2014/12/8 4:09, Arnd Bergmann wrote:
> On Sunday 07 December 2014 10:49:12 Alexander Graf wrote:
>> On 07.12.14 04:28, Ding Tianhong wrote:
>>> On 2014/12/7 8:42, Alexander Graf wrote:
>>>> On 19.04.14 03:13, Zhangfei Gao wrote:
>>>>> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
>>>>> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
>>>>>
>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>
>>>> Is this driver still supposed to go upstream? I presume this was the
>>>> last submission and it's been quite some time ago 
>>>>
>>>
>>> yes, it is really a long time, but The hip04 did not support tx irq, 
>>> we couldn't get any better idea to fix this defect, do you have any suggestion?
>>
>> Well, if hardware doesn't have a TX irq I don't see there's anything we
>> can do to fix that ;).
> 
> I don't know if it's related to the ethernet on hip01, but I would assume
> it is, and that platform is currently being submitted for inclusion, so
> I'd definitely hope to see this driver get merged too eventually.
> 
> IIRC, the last revision of the patch set had basically fixed the problem,
> except for a race that would still allow the napi poll function to exit
> with poll_complete() but a full queue of TX descriptors and no fallback
> to clean them up. There was also still an open question about whether or
> not the driver should use skb_orphan, but I may be misremembering that part.
>  

Hi Arnd:

what about use a state machine to check the tx queue and free the skb, just like:

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 8593658..71faca8 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -396,9 +396,25 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
        stats->tx_packets++;
        priv->tx_count++;

+
+       queue_delayed_work(priv->wq, &priv->tx_queue, delay);
+
        return NETDEV_TX_OK;
 }

+static void hip04_tx_queue_monitor(struct work_struct *work)
+{
+       struct hip04_priv *priv = container_of(work, struct hip04_priv,
+                                              queue_work.work);
+       struct net_device *dev = priv->ndev;
+       hip04_tx_reclain(ndev, false);
+
+       if (TX_QUEUE_IS_EMPRY(ndev))
+               return;
+
+       queue_delayed_work(priv->wq, &priv->tx_queue, delay);
+}
+
 static int hip04_rx_poll(struct napi_struct *napi, int budget)
 {
        struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
@@ -736,6 +752,8 @@ static int hip04_mac_probe(struct platform_device *pdev)
                goto alloc_fail;
        }

+       INIT_DELAYED_WORK(&priv->tx_queue, hip04_tx_queue_monitor);
+
        return 0;



what do you think of this solution?

Regards
Ding


>> Dave, what's your take here? Should we keep a driver from going upstream
>> just because the hardware is partly broken? I'd really prefer to have an
>> upstream driver on that SoC rather than some random (eventually even
>> more broken) downstream code.
> 
> We can certainly have a slow driver for this hardware, and I'd much
> prefer slow over broken. I'd guess that some of the performance impact
> of the missing interrupts can now be offset with the xmit_more	 logic.
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Ding Tianhong]

On 2014/12/10 11:51, Ding Tianhong wrote:
> On 2014/12/8 4:09, Arnd Bergmann wrote:
>> On Sunday 07 December 2014 10:49:12 Alexander Graf wrote:
>>> On 07.12.14 04:28, Ding Tianhong wrote:
>>>> On 2014/12/7 8:42, Alexander Graf wrote:
>>>>> On 19.04.14 03:13, Zhangfei Gao wrote:
>>>>>> Support Hisilicon hip04 ethernet driver, including 100M / 1000M controller.
>>>>>> The controller has no tx done interrupt, reclaim xmitted buffer in the poll.
>>>>>>
>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>>
>>>>> Is this driver still supposed to go upstream? I presume this was the
>>>>> last submission and it's been quite some time ago 
>>>>>
>>>>
>>>> yes, it is really a long time, but The hip04 did not support tx irq, 
>>>> we couldn't get any better idea to fix this defect, do you have any suggestion?
>>>
>>> Well, if hardware doesn't have a TX irq I don't see there's anything we
>>> can do to fix that ;).
>>
>> I don't know if it's related to the ethernet on hip01, but I would assume
>> it is, and that platform is currently being submitted for inclusion, so
>> I'd definitely hope to see this driver get merged too eventually.
>>
>> IIRC, the last revision of the patch set had basically fixed the problem,
>> except for a race that would still allow the napi poll function to exit
>> with poll_complete() but a full queue of TX descriptors and no fallback
>> to clean them up. There was also still an open question about whether or
>> not the driver should use skb_orphan, but I may be misremembering that part.
>>  
> 
> Hi Arnd:
> 
> what about use a state machine to check the tx queue and free the skb, just like:
> 
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index 8593658..71faca8 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -396,9 +396,25 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>         stats->tx_packets++;
>         priv->tx_count++;
> 
> +
> +       queue_delayed_work(priv->wq, &priv->tx_queue, delay);
> +
>         return NETDEV_TX_OK;
>  }
> 
> +static void hip04_tx_queue_monitor(struct work_struct *work)
> +{
> +       struct hip04_priv *priv = container_of(work, struct hip04_priv,
> +                                              queue_work.work);
> +       struct net_device *dev = priv->ndev;
> +       hip04_tx_reclain(ndev, false);
> +
> +       if (TX_QUEUE_IS_EMPRY(ndev))
> +               return;
> +
> +       queue_delayed_work(priv->wq, &priv->tx_queue, delay);
> +}
> +
>  static int hip04_rx_poll(struct napi_struct *napi, int budget)
>  {
>         struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
> @@ -736,6 +752,8 @@ static int hip04_mac_probe(struct platform_device *pdev)
>                 goto alloc_fail;
>         }
> 
> +       INIT_DELAYED_WORK(&priv->tx_queue, hip04_tx_queue_monitor);
> +
>         return 0;
> 
> 
> 
> what do you think of this solution?
> 
> Regards
> Ding
> 

Miss this code, I think the best way is skb_orphan(skb), just like the cxgb3 drivers, some hardware
didn't use the tx inq to free dmad Tx packages.

Ding

> 
>>> Dave, what's your take here? Should we keep a driver from going upstream
>>> just because the hardware is partly broken? I'd really prefer to have an
>>> upstream driver on that SoC rather than some random (eventually even
>>> more broken) downstream code.
>>
>> We can certainly have a slow driver for this hardware, and I'd much
>> prefer slow over broken. I'd guess that some of the performance impact
>> of the missing interrupts can now be offset with the xmit_more	 logic.
>>
>> 	Arnd
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> .
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Arnd Bergmann]

On Wednesday 10 December 2014 14:45:29 Ding Tianhong wrote:
> 
> Miss this code, I think the best way is skb_orphan(skb), just like the cxgb3 drivers, some hardware
> didn't use the tx inq to free dmad Tx packages.

The problem with skb_orphan is that you are telling the network stack that
the packet is now out of its reach and it will no longer be able to take
queued packets into account. This can work as long as you are guaranteed
to never fill up the tx queue and assume that the network link is always
faster than your CPUs, but then you have to do additional steps, at least:

- turn off flow control (pause frames)
- disable half-duplex, 10mbit and 100mbit connections
- remove the byte queue limit code from the driver
- never call netif_stop_queue or return NETDEV_TX_BUSY from the tx
  function, but instead drop the oldest packets from the tx queue
  to make room, and rely on higher-level protocols (TCP) to throttle
  the output
- for best throughput, don't bother looking at the status of the tx
  queue until it has filled up, then clean up all transmitted packets
  (if any) but at least enough so you can continue sending a bit longer.

The above can work because you know the device is only used in one SoC
that has a relatively slow CPU. I would describe it as "we don't know
how much data we can send, but in doubt we send more and do our best
to make that case as rare as possible". It's also management friendly
because you can reach higher-than-gigabit transmit rates this way
if you ignore the packet loss ;-). The price for this is that you
eventually have to retransmit packets that get dropped.

The alternative to this is to keep all the features of the network
stack in place and to keep retrying the tx cleanup in some way so
you don't have to orphan or drop any skbs. This could be as easy as

- drop the skb_orphan() call
- make hip04_tx_reclaim() return an error if the queue is still full
- do not call napi_complete or turn on the rx irq if hip04_tx_reclaim
  failed

	Arnd

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 10 Dec 2014 10:35:20 +0100

> On Wednesday 10 December 2014 14:45:29 Ding Tianhong wrote:
>> 
>> Miss this code, I think the best way is skb_orphan(skb), just like the cxgb3 drivers, some hardware
>> didn't use the tx inq to free dmad Tx packages.
> 
> The problem with skb_orphan is that you are telling the network stack that
> the packet is now out of its reach and it will no longer be able to take
> queued packets into account.

skb_orphan() also does not release netfilter resources attached to the
packet.

Really, all drivers must free TX SKBs in a small, finite, amount of
time after submission.

There is no way around this.

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[Arnd Bergmann]

On Wednesday 10 December 2014 11:07:32 David Miller wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed, 10 Dec 2014 10:35:20 +0100
> 
> > On Wednesday 10 December 2014 14:45:29 Ding Tianhong wrote:
> >> 
> >> Miss this code, I think the best way is skb_orphan(skb), just like the cxgb3 drivers, some hardware
> >> didn't use the tx inq to free dmad Tx packages.
> > 
> > The problem with skb_orphan is that you are telling the network stack that
> > the packet is now out of its reach and it will no longer be able to take
> > queued packets into account.
> 
> skb_orphan() also does not release netfilter resources attached to the
> packet.
> 
> Really, all drivers must free TX SKBs in a small, finite, amount of
> time after submission.
> 
> There is no way around this.

I see. Do you see a problem with returning early from napi->poll()
without calling napi_complete() when the TX queue remains full at
the end of the poll function?

Or is there any better alternative for broken hardware that lacks
a TX-complete interrupt?

	Arnd

Re: [PATCH v8 3/3] net: hisilicon: new hip04 ethernet driver

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 10 Dec 2014 17:36:46 +0100

> Or is there any better alternative for broken hardware that lacks
> a TX-complete interrupt?

The only option is an hrtimer or similar.