[PATCH 0/3] Add Hisilicon MDIO bus driver and FEMAC driver

[PATCH 0/3] Add Hisilicon MDIO bus driver and FEMAC driver

[Dongpo Li]

This patch set adds a Hisilicon MDIO bus driver and
a Fast Ethernet MAC(FEMAC) driver.
We found that most ethernet mac drivers use the same ethtool
get_settings and set_settings, so this patch set also adds
common functions for get and set settings.

Dongpo Li (3):
  net: Add MDIO bus driver for the Hisilicon FEMAC
  ethtool: Add common functions for get and set settings
  net: hisilicon: Add Fast Ethernet MAC driver

 .../bindings/net/hisilicon-femac-mdio.txt          |   22 +
 .../devicetree/bindings/net/hisilicon-femac.txt    |   40 +
 drivers/net/ethernet/hisilicon/Kconfig             |   12 +
 drivers/net/ethernet/hisilicon/Makefile            |    1 +
 drivers/net/ethernet/hisilicon/hisi_femac.c        | 1015 ++++++++++++++++++++
 drivers/net/phy/Kconfig                            |    8 +
 drivers/net/phy/Makefile                           |    1 +
 drivers/net/phy/mdio-hisi-femac.c                  |  165 ++++
 include/linux/ethtool.h                            |    2 +
 net/core/ethtool.c                                 |   21 +
 10 files changed, 1287 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac.txt
 create mode 100644 drivers/net/ethernet/hisilicon/hisi_femac.c
 create mode 100644 drivers/net/phy/mdio-hisi-femac.c

-- 
2.8.2

[PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

[Dongpo Li]

This patch adds a separate driver for the MDIO interface of the
Hisilicon Fast Ethernet MAC.

Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
 .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++
 drivers/net/phy/Kconfig                            |   8 +
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
 create mode 100644 drivers/net/phy/mdio-hisi-femac.c

diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
new file mode 100644
index 0000000..6f46337
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
@@ -0,0 +1,22 @@
+Hisilicon Fast Ethernet MDIO Controller interface
+
+Required properties:
+- compatible: should be "hisilicon,hisi-femac-mdio".
+- reg: address and length of the register set for the device.
+- clocks: A phandle to the reference clock for this device.
+
+- PHY subnode: inherits from phy binding [1]
+[1] Documentation/devicetree/bindings/net/phy.txt
+
+Example:
+mdio: mdio@10091100 {
+	compatible = "hisilicon,hisi-femac-mdio";
+	reg = <0x10091100 0x10>;
+	clocks = <&crg HI3518EV200_MDIO_CLK>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	phy0: phy@1 {
+		reg = <1>;
+	};
+};
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 6dad9a9..e257322 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -271,6 +271,14 @@ config MDIO_BCM_IPROC
 	  This module provides a driver for the MDIO busses found in the
 	  Broadcom iProc SoC's.
 
+config MDIO_HISI_FEMAC
+	tristate "Hisilicon FEMAC MDIO bus controller"
+	depends on ARCH_HISI
+	depends on HAS_IOMEM && OF_MDIO
+	help
+	  This module provides a driver for the MDIO busses found in the
+	  Hisilicon SoC that have an Fast Ethernet MAC.
+
 endif # PHYLIB
 
 config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index fcdbb92..039c4be 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
+obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
diff --git a/drivers/net/phy/mdio-hisi-femac.c b/drivers/net/phy/mdio-hisi-femac.c
new file mode 100644
index 0000000..a9eea61
--- /dev/null
+++ b/drivers/net/phy/mdio-hisi-femac.c
@@ -0,0 +1,165 @@
+/*
+ * Hisilicon Fast Ethernet MDIO Bus Driver
+ *
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_mdio.h>
+#include <linux/platform_device.h>
+
+#define MDIO_RWCTRL		0x00
+#define MDIO_RO_DATA		0x04
+#define MDIO_WRITE		BIT(13)
+#define MDIO_RW_FINISH		BIT(15)
+#define BIT_PHY_ADDR_OFFSET	8
+#define BIT_WR_DATA_OFFSET	16
+
+struct hisi_femac_mdio_data {
+	struct clk *clk;
+	void __iomem *membase;
+};
+
+static int hisi_femac_mdio_wait_ready(struct mii_bus *bus)
+{
+	struct hisi_femac_mdio_data *data = bus->priv;
+	u32 val;
+
+	return readl_poll_timeout(data->membase + MDIO_RWCTRL,
+				  val, val & MDIO_RW_FINISH, 20, 10000);
+}
+
+static int hisi_femac_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct hisi_femac_mdio_data *data = bus->priv;
+	int ret;
+
+	ret = hisi_femac_mdio_wait_ready(bus);
+	if (ret)
+		return ret;
+
+	writel((mii_id << BIT_PHY_ADDR_OFFSET) | regnum,
+	       data->membase + MDIO_RWCTRL);
+
+	ret = hisi_femac_mdio_wait_ready(bus);
+	if (ret)
+		return ret;
+
+	return readl(data->membase + MDIO_RO_DATA) & 0xFFFF;
+}
+
+static int hisi_femac_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
+				 u16 value)
+{
+	struct hisi_femac_mdio_data *data = bus->priv;
+	int ret;
+
+	ret = hisi_femac_mdio_wait_ready(bus);
+	if (ret)
+		return ret;
+
+	writel(MDIO_WRITE | (value << BIT_WR_DATA_OFFSET) |
+	       (mii_id << BIT_PHY_ADDR_OFFSET) | regnum,
+	       data->membase + MDIO_RWCTRL);
+
+	return hisi_femac_mdio_wait_ready(bus);
+}
+
+static int hisi_femac_mdio_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mii_bus *bus;
+	struct hisi_femac_mdio_data *data;
+	struct resource *res;
+	int ret;
+
+	bus = mdiobus_alloc_size(sizeof(*data));
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "hisi_femac_mii_bus";
+	bus->read = &hisi_femac_mdio_read;
+	bus->write = &hisi_femac_mdio_write;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", pdev->name);
+	bus->parent = &pdev->dev;
+
+	data = bus->priv;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->membase)) {
+		ret = PTR_ERR(data->membase);
+		goto err_out_free_mdiobus;
+	}
+
+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk)) {
+		ret = -ENODEV;
+		goto err_out_free_mdiobus;
+	}
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		goto err_out_free_mdiobus;
+
+	ret = of_mdiobus_register(bus, np);
+	if (ret)
+		goto err_out_free_mdiobus;
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+
+err_out_free_mdiobus:
+	mdiobus_free(bus);
+	return ret;
+}
+
+static int hisi_femac_mdio_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+	struct hisi_femac_mdio_data *data = bus->priv;
+
+	mdiobus_unregister(bus);
+	clk_disable_unprepare(data->clk);
+	mdiobus_free(bus);
+
+	return 0;
+}
+
+static const struct of_device_id hisi_femac_mdio_dt_ids[] = {
+	{ .compatible = "hisilicon,hisi-femac-mdio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hisi_femac_mdio_dt_ids);
+
+static struct platform_driver hisi_femac_mdio_driver = {
+	.probe = hisi_femac_mdio_probe,
+	.remove = hisi_femac_mdio_remove,
+	.driver = {
+		.name = "hisi-femac-mdio",
+		.of_match_table = hisi_femac_mdio_dt_ids,
+	},
+};
+
+module_platform_driver(hisi_femac_mdio_driver);
+
+MODULE_DESCRIPTION("Hisilicon Fast Ethernet MAC MDIO interface driver");
+MODULE_AUTHOR("Dongpo Li <lidongpo@hisilicon.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.8.2

[PATCH 2/3] ethtool: Add common functions for get and set settings

[Dongpo Li]

Currently, most drivers only support get and set PHY settings
by PHY ethtool API. For those drivers, it's better to
supply common functions for get_settings and set_settings.
This patch adds common functions implementation.

Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
 include/linux/ethtool.h |  2 ++
 net/core/ethtool.c      | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9ded8c6..e114db9 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -82,6 +82,8 @@ struct net_device;
 /* Some generic methods drivers may use in their ethtool_ops */
 u32 ethtool_op_get_link(struct net_device *dev);
 int ethtool_op_get_ts_info(struct net_device *dev, struct ethtool_ts_info *eti);
+int ethtool_op_get_settings(struct net_device *dev, struct ethtool_cmd *cmd);
+int ethtool_op_set_settings(struct net_device *dev, struct ethtool_cmd *cmd);
 
 /**
  * ethtool_rxfh_indir_default - get default value for RX flow hash indirection
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f403481..270b6d1 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -50,6 +50,27 @@ int ethtool_op_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 }
 EXPORT_SYMBOL(ethtool_op_get_ts_info);
 
+int ethtool_op_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	if (!dev->phydev)
+		return -ENODEV;
+
+	return phy_ethtool_gset(dev->phydev, cmd);
+}
+EXPORT_SYMBOL(ethtool_op_get_settings);
+
+int ethtool_op_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (!dev->phydev)
+		return -ENODEV;
+
+	return phy_ethtool_sset(dev->phydev, cmd);
+}
+EXPORT_SYMBOL(ethtool_op_set_settings);
+
 /* Handlers for each ethtool command */
 
 #define ETHTOOL_DEV_FEATURE_WORDS	((NETDEV_FEATURE_COUNT + 31) / 32)
-- 
2.8.2

[PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Dongpo Li]

This patch adds the Hisilicon Fast Ethernet MAC(FEMAC) driver.
The FEMAC supports max speed 100Mbps and has been used in many
Hisilicon SoC.

Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
---
 .../devicetree/bindings/net/hisilicon-femac.txt    |   40 +
 drivers/net/ethernet/hisilicon/Kconfig             |   12 +
 drivers/net/ethernet/hisilicon/Makefile            |    1 +
 drivers/net/ethernet/hisilicon/hisi_femac.c        | 1015 ++++++++++++++++++++
 4 files changed, 1068 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac.txt
 create mode 100644 drivers/net/ethernet/hisilicon/hisi_femac.c

diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
new file mode 100644
index 0000000..b953a56
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
@@ -0,0 +1,40 @@
+Hisilicon Fast Ethernet MAC controller
+
+Required properties:
+- compatible: should be "hisilicon,hisi-femac" and one of the following:
+	* "hisilicon,hisi-femac-v1"
+	* "hisilicon,hisi-femac-v2"
+- reg: specifies base physical address(s) and size of the device registers.
+  The first region is the MAC core register base and size.
+  The second region is the global MAC control register.
+- interrupts: should contain the MAC interrupt.
+- clocks: clock phandle and specifier pair.
+- resets: should contain the phandle to the MAC reset signal(required) and
+	the PHY reset signal(optional).
+- reset-names: should contain the reset signal name "mac_reset"(required)
+	and "phy_reset"(optional).
+- mac-address: see ethernet.txt [1].
+- phy-mode: see ethernet.txt [1].
+- phy-handle: see ethernet.txt [1].
+- hisilicon,phy-reset-delays: triplet of delays if PHY reset signal given.
+	The 1st cell is reset pre-delay in micro seconds.
+	The 2nd cell is reset pulse in micro seconds.
+	The 3rd cell is reset post-delay in micro seconds.
+
+[1] Documentation/devicetree/bindings/net/ethernet.txt
+
+Example:
+	hisi_femac: ethernet@10090000 {
+		compatible = "hisilicon,hisi-femac-v2", "hisilicon,hisi-femac";
+		reg = <0x10090000 0x1000>,<0x10091300 0x200>;
+		interrupts = <12>;
+		clocks = <&crg HI3518EV200_ETH_CLK>;
+		resets = <&crg 0xec 0>,
+				<&crg 0xec 3>;
+		reset-names = "mac_reset",
+				"phy_reset";
+		mac-address = [00 00 00 00 00 00];
+		phy-mode = "mii";
+		phy-handle = <&phy0>;
+		hisilicon,phy-reset-delays = <10000 20000 20000>;
+	};
diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
index 4ccc032..49ab550 100644
--- a/drivers/net/ethernet/hisilicon/Kconfig
+++ b/drivers/net/ethernet/hisilicon/Kconfig
@@ -23,6 +23,18 @@ config HIX5HD2_GMAC
 	help
 	  This selects the hix5hd2 mac family network device.
 
+config HISI_FEMAC
+	tristate "Hisilicon Fast Ethernet MAC device support"
+	select PHYLIB
+	select RESET_CONTROLLER
+	select MDIO_HISI_FEMAC
+	help
+	  This selects the Hisilicon Fast Ethernet MAC device(FEMAC).
+	  The FEMAC receives and transmits data over Ethernet
+	  ports at 10/100 Mbps in full-duplex or half-duplex mode.
+	  The FEMAC exchanges data with the CPU, and supports
+	  the energy efficient Ethernet (EEE).
+
 config HIP04_ETH
 	tristate "HISILICON P04 Ethernet support"
 	depends on HAS_IOMEM	# For MFD_SYSCON
diff --git a/drivers/net/ethernet/hisilicon/Makefile b/drivers/net/ethernet/hisilicon/Makefile
index 390b71f..8661695 100644
--- a/drivers/net/ethernet/hisilicon/Makefile
+++ b/drivers/net/ethernet/hisilicon/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_HIX5HD2_GMAC) += hix5hd2_gmac.o
 obj-$(CONFIG_HIP04_ETH) += hip04_eth.o
 obj-$(CONFIG_HNS_MDIO) += hns_mdio.o
 obj-$(CONFIG_HNS) += hns/
+obj-$(CONFIG_HISI_FEMAC) += hisi_femac.o
diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
new file mode 100644
index 0000000..0c85ac7
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -0,0 +1,1015 @@
+/*
+ * Hisilicon Fast Ethernet MAC Driver
+ *
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/circ_buf.h>
+#include <linux/clk.h>
+#include <linux/etherdevice.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+/* MAC control register list */
+#define MAC_PORTSEL			0x0200
+#define MAC_PORTSEL_STAT_CPU		BIT(0)
+#define MAC_PORTSEL_RMII		BIT(1)
+#define MAC_PORTSET			0x0208
+#define MAC_PORTSET_DUPLEX_FULL		BIT(0)
+#define MAC_PORTSET_LINKED		BIT(1)
+#define MAC_PORTSET_SPEED_100M		BIT(2)
+#define MAC_SET				0x0210
+#define MAX_FRAME_SIZE			1600
+#define MAX_FRAME_SIZE_MASK		GENMASK(10, 0)
+#define BIT_PAUSE_EN			BIT(18)
+#define QLEN_SET			0x0344
+#define RX_DEPTH_OFFSET			8
+#define MAX_HW_FIFO_DEPTH		64
+#define HW_TX_FIFO_DEPTH		12
+#define HW_RX_FIFO_DEPTH		(MAX_HW_FIFO_DEPTH - HW_TX_FIFO_DEPTH)
+#define IQFRM_DES			0x0354
+#define RX_FRAME_LEN_MASK		GENMASK(11, 0)
+#define IQ_ADDR				0x0358
+#define EQ_ADDR				0x0360
+#define EQFRM_LEN			0x0364
+#define ADDRQ_STAT			0x036C
+#define TX_CNT_INUSE_MASK		GENMASK(5, 0)
+#define BIT_TX_READY			BIT(24)
+#define BIT_RX_READY			BIT(25)
+/* global control register list */
+#define GLB_HOSTMAC_L32			0x0000
+#define GLB_HOSTMAC_H16			0x0004
+#define GLB_SOFT_RESET			0x0008
+#define SOFT_RESET_ALL			BIT(0)
+#define GLB_FWCTRL			0x0010
+#define FWCTRL_VLAN_ENABLE		BIT(0)
+#define FWCTRL_FW2CPU_ENA		BIT(5)
+#define FWCTRL_FWALL2CPU		BIT(7)
+#define GLB_MACTCTRL			0x0014
+#define MACTCTRL_UNI2CPU		BIT(1)
+#define MACTCTRL_MULTI2CPU		BIT(3)
+#define MACTCTRL_BROAD2CPU		BIT(5)
+#define MACTCTRL_MACT_ENA		BIT(7)
+#define GLB_IRQ_STAT			0x0030
+#define GLB_IRQ_ENA			0x0034
+#define IRQ_ENA_PORT0_MASK		GENMASK(7, 0)
+#define IRQ_ENA_PORT0			BIT(18)
+#define IRQ_ENA_ALL			BIT(19)
+#define GLB_IRQ_RAW			0x0038
+#define IRQ_INT_RX_RDY			BIT(0)
+#define IRQ_INT_TX_FIFO_EMPTY		BIT(6)
+#define IRQ_INT_MULTI_RXRDY		BIT(7)
+#define DEF_INT_MASK			(IRQ_INT_MULTI_RXRDY | \
+					IRQ_INT_TX_FIFO_EMPTY)
+#define GLB_MAC_L32_BASE		0x0100
+#define GLB_MAC_H16_BASE		0x0104
+#define MACFLT_HI16_MASK		GENMASK(15, 0)
+#define BIT_MACFLT_ENA			BIT(17)
+#define BIT_MACFLT_FW2CPU		BIT(21)
+#define GLB_MAC_H16(reg)		(GLB_MAC_H16_BASE + ((reg) * 0x8))
+#define GLB_MAC_L32(reg)		(GLB_MAC_L32_BASE + ((reg) * 0x8))
+#define MAX_MAC_FILTER_NUM		8
+#define MAX_UNICAST_ADDRESSES		2
+#define MAX_MULTICAST_ADDRESSES		(MAX_MAC_FILTER_NUM - \
+					MAX_UNICAST_ADDRESSES)
+/* software tx and rx queue number, should be power of 2 */
+#define TXQ_NUM				64
+#define RXQ_NUM				128
+
+#define MAC_RST_NAME    "mac_reset"
+#define PHY_RST_NAME    "phy_reset"
+
+enum phy_reset_delays {
+	PRE_DELAY,
+	PULSE,
+	POST_DELAY,
+	DELAYS_NUM,
+};
+
+struct hisi_femac_queue {
+	struct sk_buff **skb;
+	dma_addr_t *dma_phys;
+	int num;
+	unsigned int head;
+	unsigned int tail;
+};
+
+struct hisi_femac_priv {
+	void __iomem *port_base;
+	void __iomem *glb_base;
+	struct clk *clk;
+	struct reset_control *mac_rst;
+	struct reset_control *phy_rst;
+	u32 phy_reset_delays[DELAYS_NUM];
+
+	struct device *dev;
+	struct net_device *ndev;
+
+	struct hisi_femac_queue txq;
+	struct hisi_femac_queue rxq;
+	u32 tx_fifo_used_cnt;
+	struct napi_struct napi;
+
+	struct device_node *phy_node;
+	phy_interface_t	phy_mode;
+	struct phy_device *phy;
+	u32 link_status;
+};
+
+static void hisi_femac_irq_enable(struct hisi_femac_priv *priv, int irqs)
+{
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_IRQ_ENA);
+	writel(val | irqs, priv->glb_base + GLB_IRQ_ENA);
+}
+
+static void hisi_femac_irq_disable(struct hisi_femac_priv *priv, int irqs)
+{
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_IRQ_ENA);
+	writel(val & (~irqs), priv->glb_base + GLB_IRQ_ENA);
+}
+
+static void hisi_femac_tx_dma_unmap(struct hisi_femac_priv *priv,
+				    struct sk_buff *skb, unsigned int pos)
+{
+	dma_addr_t dma_addr;
+
+	dma_addr = priv->txq.dma_phys[pos];
+	dma_unmap_single(priv->dev, dma_addr, skb->len, DMA_TO_DEVICE);
+}
+
+static void hisi_femac_xmit_reclaim(struct net_device *dev)
+{
+	struct sk_buff *skb;
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+	struct hisi_femac_queue *txq = &priv->txq;
+	unsigned int bytes_compl = 0, pkts_compl = 0;
+	u32 val;
+
+	netif_tx_lock(dev);
+
+	val = readl(priv->port_base + ADDRQ_STAT) & TX_CNT_INUSE_MASK;
+	while (val < priv->tx_fifo_used_cnt) {
+		skb = txq->skb[txq->tail];
+		if (unlikely(!skb)) {
+			netdev_err(dev, "xmitq_cnt_inuse=%d, tx_fifo_used=%d\n",
+				   val, priv->tx_fifo_used_cnt);
+			break;
+		}
+		hisi_femac_tx_dma_unmap(priv, skb, txq->tail);
+		pkts_compl++;
+		bytes_compl += skb->len;
+		dev_kfree_skb_any(skb);
+
+		priv->tx_fifo_used_cnt--;
+
+		val = readl(priv->port_base + ADDRQ_STAT) & TX_CNT_INUSE_MASK;
+		txq->skb[txq->tail] = NULL;
+		txq->tail = (txq->tail + 1) % txq->num;
+	}
+
+	netdev_completed_queue(dev, pkts_compl, bytes_compl);
+
+	if (unlikely(netif_queue_stopped(dev)) && pkts_compl)
+		netif_wake_queue(dev);
+
+	netif_tx_unlock(dev);
+}
+
+static void hisi_femac_adjust_link(struct net_device *dev)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+	struct phy_device *phy = priv->phy;
+	u32 status = 0;
+
+	if (phy->link)
+		status |= MAC_PORTSET_LINKED;
+	if (phy->duplex == DUPLEX_FULL)
+		status |= MAC_PORTSET_DUPLEX_FULL;
+	if (phy->speed == SPEED_100)
+		status |= MAC_PORTSET_SPEED_100M;
+
+	if ((status != priv->link_status) &&
+	    ((status | priv->link_status) & MAC_PORTSET_LINKED)) {
+		writel(status, priv->port_base + MAC_PORTSET);
+		priv->link_status = status;
+		phy_print_status(phy);
+	}
+}
+
+static void hisi_femac_rx_refill(struct hisi_femac_priv *priv)
+{
+	struct hisi_femac_queue *rxq = &priv->rxq;
+	struct sk_buff *skb;
+	u32 pos;
+	u32 len = MAX_FRAME_SIZE;
+	dma_addr_t addr;
+
+	pos = rxq->head;
+	while (readl(priv->port_base + ADDRQ_STAT) & BIT_RX_READY) {
+		if (!CIRC_SPACE(pos, rxq->tail, rxq->num))
+			break;
+		if (unlikely(rxq->skb[pos])) {
+			netdev_err(priv->ndev, "err skb[%d]=%p\n",
+				   pos, rxq->skb[pos]);
+			break;
+		}
+		skb = netdev_alloc_skb_ip_align(priv->ndev, len);
+		if (unlikely(!skb))
+			break;
+
+		addr = dma_map_single(priv->dev, skb->data, len,
+				      DMA_FROM_DEVICE);
+		if (dma_mapping_error(priv->dev, addr)) {
+			dev_kfree_skb_any(skb);
+			break;
+		}
+		rxq->dma_phys[pos] = addr;
+		rxq->skb[pos] = skb;
+		writel(addr, priv->port_base + IQ_ADDR);
+		pos = (pos + 1) % rxq->num;
+	}
+	rxq->head = pos;
+}
+
+static int hisi_femac_rx(struct net_device *dev, int limit)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+	struct hisi_femac_queue *rxq = &priv->rxq;
+	struct sk_buff *skb;
+	dma_addr_t addr;
+	u32 rx_pkt_info, pos, len, rx_pkts_num = 0;
+
+	pos = rxq->tail;
+	while (readl(priv->glb_base + GLB_IRQ_RAW) & IRQ_INT_RX_RDY) {
+		rx_pkt_info = readl(priv->port_base + IQFRM_DES);
+		len = rx_pkt_info & RX_FRAME_LEN_MASK;
+		len -= ETH_FCS_LEN;
+
+		/* tell hardware we will deal with this packet */
+		writel(IRQ_INT_RX_RDY, priv->glb_base + GLB_IRQ_RAW);
+
+		rx_pkts_num++;
+
+		skb = rxq->skb[pos];
+		if (unlikely(!skb)) {
+			netdev_err(dev, "rx skb NULL. pos=%d\n", pos);
+			break;
+		}
+		rxq->skb[pos] = NULL;
+
+		addr = rxq->dma_phys[pos];
+		dma_unmap_single(priv->dev, addr, MAX_FRAME_SIZE,
+				 DMA_FROM_DEVICE);
+		skb_put(skb, len);
+		if (unlikely(skb->len > MAX_FRAME_SIZE)) {
+			netdev_err(dev, "rcv len err, len = %d\n", skb->len);
+			dev->stats.rx_errors++;
+			dev->stats.rx_length_errors++;
+			dev_kfree_skb_any(skb);
+			goto next;
+		}
+
+		skb->protocol = eth_type_trans(skb, dev);
+		napi_gro_receive(&priv->napi, skb);
+		dev->stats.rx_packets++;
+		dev->stats.rx_bytes += skb->len;
+next:
+		pos = (pos + 1) % rxq->num;
+		if (rx_pkts_num >= limit)
+			break;
+	}
+	rxq->tail = pos;
+
+	hisi_femac_rx_refill(priv);
+
+	return rx_pkts_num;
+}
+
+static int hisi_femac_poll(struct napi_struct *napi, int budget)
+{
+	struct hisi_femac_priv *priv = container_of(napi,
+					struct hisi_femac_priv, napi);
+	struct net_device *dev = priv->ndev;
+	int work_done = 0, task = budget;
+	int ints, num;
+
+	do {
+		hisi_femac_xmit_reclaim(dev);
+		num = hisi_femac_rx(dev, task);
+		work_done += num;
+		task -= num;
+		if ((work_done >= budget) || (num == 0))
+			break;
+
+		ints = readl(priv->glb_base + GLB_IRQ_STAT);
+		writel(ints & DEF_INT_MASK,
+		       priv->glb_base + GLB_IRQ_RAW);
+	} while (ints & DEF_INT_MASK);
+
+	if (work_done < budget) {
+		napi_complete(napi);
+		hisi_femac_irq_enable(priv, DEF_INT_MASK);
+	}
+
+	return work_done;
+}
+
+static irqreturn_t hisi_femac_interrupt(int irq, void *dev_id)
+{
+	int ints;
+	struct net_device *dev = (struct net_device *)dev_id;
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+
+	ints = readl(priv->glb_base + GLB_IRQ_STAT);
+
+	if (likely(ints & DEF_INT_MASK)) {
+		writel(ints & DEF_INT_MASK,
+		       priv->glb_base + GLB_IRQ_RAW);
+		hisi_femac_irq_disable(priv, DEF_INT_MASK);
+		napi_schedule(&priv->napi);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_femac_init_queue(struct device *dev,
+				 struct hisi_femac_queue *queue,
+				 unsigned int num)
+{
+	queue->skb = devm_kcalloc(dev, num, sizeof(struct sk_buff *),
+				  GFP_KERNEL);
+	if (!queue->skb)
+		return -ENOMEM;
+
+	queue->dma_phys = devm_kcalloc(dev, num, sizeof(dma_addr_t),
+				       GFP_KERNEL);
+	if (!queue->dma_phys)
+		return -ENOMEM;
+
+	queue->num = num;
+	queue->head = 0;
+	queue->tail = 0;
+
+	return 0;
+}
+
+static int hisi_femac_init_tx_and_rx_queues(struct hisi_femac_priv *priv)
+{
+	int ret;
+
+	ret = hisi_femac_init_queue(priv->dev, &priv->txq, TXQ_NUM);
+	if (ret)
+		return ret;
+
+	ret = hisi_femac_init_queue(priv->dev, &priv->rxq, RXQ_NUM);
+	if (ret)
+		return ret;
+
+	priv->tx_fifo_used_cnt = 0;
+
+	return 0;
+}
+
+static void hisi_femac_free_skb_rings(struct hisi_femac_priv *priv)
+{
+	struct hisi_femac_queue *txq = &priv->txq;
+	struct hisi_femac_queue *rxq = &priv->rxq;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
+	u32 pos;
+
+	pos = rxq->tail;
+	while (pos != rxq->head) {
+		skb = rxq->skb[pos];
+		if (unlikely(!skb)) {
+			netdev_err(priv->ndev, "NULL rx skb. pos=%d, head=%d\n",
+				   pos, rxq->head);
+			continue;
+		}
+
+		dma_addr = rxq->dma_phys[pos];
+		dma_unmap_single(priv->dev, dma_addr, MAX_FRAME_SIZE,
+				 DMA_FROM_DEVICE);
+
+		dev_kfree_skb_any(skb);
+		rxq->skb[pos] = NULL;
+		pos = (pos + 1) % rxq->num;
+	}
+	rxq->tail = pos;
+
+	pos = txq->tail;
+	while (pos != txq->head) {
+		skb = txq->skb[pos];
+		if (unlikely(!skb)) {
+			netdev_err(priv->ndev, "NULL tx skb. pos=%d, head=%d\n",
+				   pos, txq->head);
+			continue;
+		}
+		hisi_femac_tx_dma_unmap(priv, skb, pos);
+		dev_kfree_skb_any(skb);
+		txq->skb[pos] = NULL;
+		pos = (pos + 1) % txq->num;
+	}
+	txq->tail = pos;
+	priv->tx_fifo_used_cnt = 0;
+}
+
+static int hisi_femac_set_hw_mac_addr(struct hisi_femac_priv *priv,
+				      unsigned char *mac)
+{
+	u32 reg;
+
+	reg = mac[1] | (mac[0] << 8);
+	writel(reg, priv->glb_base + GLB_HOSTMAC_H16);
+
+	reg = mac[5] | (mac[4] << 8) | (mac[3] << 16) | (mac[2] << 24);
+	writel(reg, priv->glb_base + GLB_HOSTMAC_L32);
+
+	return 0;
+}
+
+static int hisi_femac_port_reset(struct hisi_femac_priv *priv)
+{
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_SOFT_RESET);
+	val |= SOFT_RESET_ALL;
+	writel(val, priv->glb_base + GLB_SOFT_RESET);
+
+	usleep_range(500, 800);
+
+	val &= ~SOFT_RESET_ALL;
+	writel(val, priv->glb_base + GLB_SOFT_RESET);
+
+	return 0;
+}
+
+static int hisi_femac_net_open(struct net_device *dev)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+
+	hisi_femac_port_reset(priv);
+	hisi_femac_set_hw_mac_addr(priv, dev->dev_addr);
+	hisi_femac_rx_refill(priv);
+
+	netif_carrier_off(dev);
+	netdev_reset_queue(dev);
+	netif_start_queue(dev);
+	napi_enable(&priv->napi);
+
+	priv->link_status = 0;
+	if (priv->phy)
+		phy_start(priv->phy);
+
+	writel(IRQ_ENA_PORT0_MASK, priv->glb_base + GLB_IRQ_RAW);
+	hisi_femac_irq_enable(priv, IRQ_ENA_ALL | IRQ_ENA_PORT0 | DEF_INT_MASK);
+
+	return 0;
+}
+
+static int hisi_femac_net_close(struct net_device *dev)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+
+	hisi_femac_irq_disable(priv, IRQ_ENA_PORT0);
+
+	if (priv->phy)
+		phy_stop(priv->phy);
+
+	netif_stop_queue(dev);
+	napi_disable(&priv->napi);
+
+	hisi_femac_free_skb_rings(priv);
+
+	return 0;
+}
+
+static netdev_tx_t hisi_femac_net_xmit(struct sk_buff *skb,
+				       struct net_device *dev)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+	struct hisi_femac_queue *txq = &priv->txq;
+	dma_addr_t addr;
+	u32 val;
+
+	val = readl(priv->port_base + ADDRQ_STAT);
+	val &= BIT_TX_READY;
+	if (!val) {
+		dev->stats.tx_dropped++;
+		dev->stats.tx_fifo_errors++;
+		netif_stop_queue(dev);
+		return NETDEV_TX_BUSY;
+	}
+
+	if (unlikely(!CIRC_SPACE(txq->head, txq->tail,
+				 txq->num))) {
+		dev->stats.tx_dropped++;
+		dev->stats.tx_fifo_errors++;
+		netif_stop_queue(dev);
+		return NETDEV_TX_BUSY;
+	}
+
+	addr = dma_map_single(priv->dev, skb->data,
+			      skb->len, DMA_TO_DEVICE);
+	if (unlikely(dma_mapping_error(priv->dev, addr))) {
+		dev_kfree_skb_any(skb);
+		dev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
+	}
+	txq->dma_phys[txq->head] = addr;
+
+	txq->skb[txq->head] = skb;
+	txq->head = (txq->head + 1) % txq->num;
+
+	writel(addr, priv->port_base + EQ_ADDR);
+	writel(skb->len + ETH_FCS_LEN, priv->port_base + EQFRM_LEN);
+
+	priv->tx_fifo_used_cnt++;
+
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+	netdev_sent_queue(dev, skb->len);
+
+	return NETDEV_TX_OK;
+}
+
+static int hisi_femac_set_mac_address(struct net_device *dev, void *p)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+	struct sockaddr *skaddr = p;
+
+	if (!is_valid_ether_addr(skaddr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(dev->dev_addr, skaddr->sa_data, dev->addr_len);
+	dev->addr_assign_type &= ~NET_ADDR_RANDOM;
+
+	hisi_femac_set_hw_mac_addr(priv, dev->dev_addr);
+
+	return 0;
+}
+
+static void hisi_femac_enable_hw_addr_filter(struct hisi_femac_priv *priv,
+					     unsigned int reg_n, bool enable)
+{
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_MAC_H16(reg_n));
+	if (enable)
+		val |= BIT_MACFLT_ENA;
+	else
+		val &= ~BIT_MACFLT_ENA;
+	writel(val, priv->glb_base + GLB_MAC_H16(reg_n));
+}
+
+static void hisi_femac_set_hw_addr_filter(struct hisi_femac_priv *priv,
+					  unsigned char *addr,
+					  unsigned int reg_n)
+{
+	unsigned int high, low;
+	u32 val;
+
+	high = GLB_MAC_H16(reg_n);
+	low = GLB_MAC_L32(reg_n);
+
+	val = (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
+	writel(val, priv->glb_base + low);
+
+	val = readl(priv->glb_base + high);
+	val &= ~MACFLT_HI16_MASK;
+	val |= ((addr[0] << 8) | addr[1]);
+	val |= (BIT_MACFLT_ENA | BIT_MACFLT_FW2CPU);
+	writel(val, priv->glb_base + high);
+}
+
+static void hisi_femac_set_promisc_mode(struct hisi_femac_priv *priv,
+					bool promisc_mode)
+{
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_FWCTRL);
+	if (promisc_mode)
+		val |= FWCTRL_FWALL2CPU;
+	else
+		val &= ~FWCTRL_FWALL2CPU;
+	writel(val, priv->glb_base + GLB_FWCTRL);
+}
+
+/* Handle multiple multicast addresses (perfect filtering)*/
+static void hisi_femac_set_mc_addr_filter(struct hisi_femac_priv *priv)
+{
+	struct net_device *dev = priv->ndev;
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_MACTCTRL);
+	if ((netdev_mc_count(dev) > MAX_MULTICAST_ADDRESSES) ||
+	    (dev->flags & IFF_ALLMULTI)) {
+		val |= MACTCTRL_MULTI2CPU;
+	} else {
+		int reg = MAX_UNICAST_ADDRESSES;
+		int i;
+		struct netdev_hw_addr *ha;
+
+		for (i = reg; i < MAX_MAC_FILTER_NUM; i++)
+			hisi_femac_enable_hw_addr_filter(priv, i, false);
+
+		netdev_for_each_mc_addr(ha, dev) {
+			hisi_femac_set_hw_addr_filter(priv, ha->addr, reg);
+			reg++;
+		}
+		val &= ~MACTCTRL_MULTI2CPU;
+	}
+	writel(val, priv->glb_base + GLB_MACTCTRL);
+}
+
+/* Handle multiple unicast addresses (perfect filtering)*/
+static void hisi_femac_set_uc_addr_filter(struct hisi_femac_priv *priv)
+{
+	struct net_device *dev = priv->ndev;
+	u32 val;
+
+	val = readl(priv->glb_base + GLB_MACTCTRL);
+	if (netdev_uc_count(dev) > MAX_UNICAST_ADDRESSES) {
+		val |= MACTCTRL_UNI2CPU;
+	} else {
+		int reg = 0;
+		int i;
+		struct netdev_hw_addr *ha;
+
+		for (i = reg; i < MAX_UNICAST_ADDRESSES; i++)
+			hisi_femac_enable_hw_addr_filter(priv, i, false);
+
+		netdev_for_each_uc_addr(ha, dev) {
+			hisi_femac_set_hw_addr_filter(priv, ha->addr, reg);
+			reg++;
+		}
+		val &= ~MACTCTRL_UNI2CPU;
+	}
+	writel(val, priv->glb_base + GLB_MACTCTRL);
+}
+
+static void hisi_femac_net_set_rx_mode(struct net_device *dev)
+{
+	struct hisi_femac_priv *priv = netdev_priv(dev);
+
+	if (dev->flags & IFF_PROMISC) {
+		hisi_femac_set_promisc_mode(priv, true);
+	} else {
+		hisi_femac_set_promisc_mode(priv, false);
+		hisi_femac_set_mc_addr_filter(priv);
+		hisi_femac_set_uc_addr_filter(priv);
+	}
+}
+
+static int hisi_femac_net_ioctl(struct net_device *dev,
+				struct ifreq *ifreq, int cmd)
+{
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	if (!dev->phydev)
+		return -EINVAL;
+
+	return phy_mii_ioctl(dev->phydev, ifreq, cmd);
+}
+
+static struct ethtool_ops hisi_femac_ethtools_ops = {
+	.get_link		= ethtool_op_get_link,
+	.get_settings		= ethtool_op_get_settings,
+	.set_settings		= ethtool_op_set_settings,
+};
+
+static const struct net_device_ops hisi_femac_netdev_ops = {
+	.ndo_open		= hisi_femac_net_open,
+	.ndo_stop		= hisi_femac_net_close,
+	.ndo_start_xmit		= hisi_femac_net_xmit,
+	.ndo_do_ioctl		= hisi_femac_net_ioctl,
+	.ndo_set_mac_address	= hisi_femac_set_mac_address,
+	.ndo_set_rx_mode	= hisi_femac_net_set_rx_mode,
+	.ndo_change_mtu		= eth_change_mtu,
+};
+
+static void hisi_femac_core_reset(struct hisi_femac_priv *priv)
+{
+	reset_control_assert(priv->mac_rst);
+	reset_control_deassert(priv->mac_rst);
+}
+
+static void hisi_femac_sleep_us(u32 time_us)
+{
+	u32 time_ms;
+
+	if (!time_us)
+		return;
+
+	time_ms = DIV_ROUND_UP(time_us, 1000);
+	if (time_ms < 20)
+		usleep_range(time_us, time_us + 500);
+	else
+		msleep(time_ms);
+}
+
+static void hisi_femac_phy_reset(struct hisi_femac_priv *priv)
+{
+	/* To make sure PHY hardware reset success,
+	 * we must keep PHY in deassert state first and
+	 * then complete the hardware reset operation
+	 */
+	reset_control_deassert(priv->phy_rst);
+	hisi_femac_sleep_us(priv->phy_reset_delays[PRE_DELAY]);
+
+	reset_control_assert(priv->phy_rst);
+	/* delay some time to ensure reset ok,
+	 * this depends on PHY hardware feature
+	 */
+	hisi_femac_sleep_us(priv->phy_reset_delays[PULSE]);
+	reset_control_deassert(priv->phy_rst);
+	/* delay some time to ensure later MDIO access */
+	hisi_femac_sleep_us(priv->phy_reset_delays[POST_DELAY]);
+}
+
+static void hisi_femac_port_init(struct hisi_femac_priv *priv)
+{
+	u32 val;
+
+	/* MAC gets link status info and phy mode by software config */
+	val = MAC_PORTSEL_STAT_CPU;
+	if (priv->phy_mode == PHY_INTERFACE_MODE_RMII)
+		val |= MAC_PORTSEL_RMII;
+	writel(val, priv->port_base + MAC_PORTSEL);
+
+	/*clear all interrupt status */
+	writel(IRQ_ENA_PORT0_MASK, priv->glb_base + GLB_IRQ_RAW);
+	hisi_femac_irq_disable(priv, IRQ_ENA_PORT0_MASK | IRQ_ENA_PORT0);
+
+	val = readl(priv->glb_base + GLB_FWCTRL);
+	val &= ~(FWCTRL_VLAN_ENABLE | FWCTRL_FWALL2CPU);
+	val |= FWCTRL_FW2CPU_ENA;
+	writel(val, priv->glb_base + GLB_FWCTRL);
+
+	val = readl(priv->glb_base + GLB_MACTCTRL);
+	val |= (MACTCTRL_BROAD2CPU | MACTCTRL_MACT_ENA);
+	writel(val, priv->glb_base + GLB_MACTCTRL);
+
+	val = readl(priv->port_base + MAC_SET);
+	val &= ~MAX_FRAME_SIZE_MASK;
+	val |= MAX_FRAME_SIZE;
+	writel(val, priv->port_base + MAC_SET);
+
+	val = (HW_RX_FIFO_DEPTH << RX_DEPTH_OFFSET) | HW_TX_FIFO_DEPTH;
+	writel(val, priv->port_base + QLEN_SET);
+}
+
+static int hisi_femac_drv_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct resource *res;
+	struct net_device *ndev;
+	struct hisi_femac_priv *priv;
+	const char *mac_addr;
+	int ret;
+
+	ndev = alloc_etherdev(sizeof(*priv));
+	if (!ndev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ndev);
+
+	priv = netdev_priv(ndev);
+	priv->dev = dev;
+	priv->ndev = ndev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->port_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->port_base)) {
+		ret = PTR_ERR(priv->port_base);
+		goto out_free_netdev;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	priv->glb_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->glb_base)) {
+		ret = PTR_ERR(priv->glb_base);
+		goto out_free_netdev;
+	}
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "failed to get clk\n");
+		ret = -ENODEV;
+		goto out_free_netdev;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clk %d\n", ret);
+		goto out_free_netdev;
+	}
+
+	priv->mac_rst = devm_reset_control_get(dev, MAC_RST_NAME);
+	if (IS_ERR(priv->mac_rst)) {
+		ret = PTR_ERR(priv->mac_rst);
+		goto out_disable_clk;
+	}
+	hisi_femac_core_reset(priv);
+
+	priv->phy_rst = devm_reset_control_get(dev, PHY_RST_NAME);
+	if (IS_ERR(priv->phy_rst)) {
+		priv->phy_rst = NULL;
+	} else {
+		ret = of_property_read_u32_array(node,
+						 "hisilicon,phy-reset-delays",
+						 priv->phy_reset_delays,
+						 DELAYS_NUM);
+		if (ret)
+			goto out_disable_clk;
+		hisi_femac_phy_reset(priv);
+	}
+
+	priv->phy_mode = of_get_phy_mode(node);
+	if (priv->phy_mode < 0) {
+		dev_err(dev, "not find phy-mode\n");
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
+	if (!priv->phy_node) {
+		dev_err(dev, "not find phy-handle\n");
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	priv->phy = of_phy_connect(ndev, priv->phy_node,
+				   hisi_femac_adjust_link, 0, priv->phy_mode);
+	if (!(priv->phy) || IS_ERR(priv->phy)) {
+		dev_err(dev, "connect to PHY failed!\n");
+		ret = -ENODEV;
+		goto out_phy_node;
+	}
+
+	phy_attached_print(priv->phy, "phy_id=0x%.8lx, phy_mode=%s\n",
+			   (unsigned long)priv->phy->phy_id,
+			   phy_modes(priv->phy_mode));
+
+	mac_addr = of_get_mac_address(node);
+	if (mac_addr)
+		ether_addr_copy(ndev->dev_addr, mac_addr);
+	if (!is_valid_ether_addr(ndev->dev_addr)) {
+		eth_hw_addr_random(ndev);
+		dev_warn(dev, "using random MAC address %pM\n",
+			 ndev->dev_addr);
+	}
+
+	ndev->watchdog_timeo = 6 * HZ;
+	ndev->priv_flags |= IFF_UNICAST_FLT;
+	ndev->netdev_ops = &hisi_femac_netdev_ops;
+	ndev->ethtool_ops = &hisi_femac_ethtools_ops;
+	netif_napi_add(ndev, &priv->napi, hisi_femac_poll, NAPI_POLL_WEIGHT);
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	hisi_femac_port_init(priv);
+
+	ret = hisi_femac_init_tx_and_rx_queues(priv);
+	if (ret)
+		goto out_disconnect_phy;
+
+	ndev->irq = platform_get_irq(pdev, 0);
+	if (ndev->irq <= 0) {
+		dev_err(dev, "No irq resource\n");
+		ret = -ENODEV;
+		goto out_disconnect_phy;
+	}
+
+	ret = devm_request_irq(dev, ndev->irq, hisi_femac_interrupt,
+			       IRQF_SHARED, pdev->name, ndev);
+	if (ret) {
+		dev_err(dev, "devm_request_irq %d failed!\n", ndev->irq);
+		goto out_disconnect_phy;
+	}
+
+	ret = register_netdev(ndev);
+	if (ret) {
+		dev_err(dev, "register_netdev failed!\n");
+		goto out_disconnect_phy;
+	}
+
+	return ret;
+
+out_disconnect_phy:
+	netif_napi_del(&priv->napi);
+	phy_disconnect(priv->phy);
+out_phy_node:
+	of_node_put(priv->phy_node);
+out_disable_clk:
+	clk_disable_unprepare(priv->clk);
+out_free_netdev:
+	free_netdev(ndev);
+
+	return ret;
+}
+
+static int hisi_femac_drv_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct hisi_femac_priv *priv = netdev_priv(ndev);
+
+	netif_napi_del(&priv->napi);
+	unregister_netdev(ndev);
+
+	phy_disconnect(priv->phy);
+	of_node_put(priv->phy_node);
+	clk_disable_unprepare(priv->clk);
+	free_netdev(ndev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+int hisi_femac_drv_suspend(struct platform_device *pdev,
+			   pm_message_t state)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct hisi_femac_priv *priv = netdev_priv(ndev);
+
+	disable_irq(ndev->irq);
+	if (netif_running(ndev)) {
+		hisi_femac_net_close(ndev);
+		netif_device_detach(ndev);
+	}
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+int hisi_femac_drv_resume(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct hisi_femac_priv *priv = netdev_priv(ndev);
+
+	clk_prepare_enable(priv->clk);
+	if (priv->phy_rst)
+		hisi_femac_phy_reset(priv);
+
+	if (netif_running(ndev)) {
+		hisi_femac_port_init(priv);
+		hisi_femac_net_open(ndev);
+		netif_device_attach(ndev);
+	}
+	enable_irq(ndev->irq);
+
+	return 0;
+}
+#endif
+
+static const struct of_device_id hisi_femac_match[] = {
+	{.compatible = "hisilicon,hisi-femac",},
+	{.compatible = "hisilicon,hisi-femac-v1",},
+	{.compatible = "hisilicon,hisi-femac-v2",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, hisi_femac_match);
+
+static struct platform_driver hisi_femac_driver = {
+	.driver = {
+		.name = "hisi-femac",
+		.of_match_table = hisi_femac_match,
+	},
+	.probe = hisi_femac_drv_probe,
+	.remove = hisi_femac_drv_remove,
+#ifdef CONFIG_PM
+	.suspend = hisi_femac_drv_suspend,
+	.resume = hisi_femac_drv_resume,
+#endif
+};
+
+module_platform_driver(hisi_femac_driver);
+
+MODULE_DESCRIPTION("Hisilicon Fast Ethernet MAC driver");
+MODULE_AUTHOR("Dongpo Li <lidongpo@hisilicon.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hisi-femac");
-- 
2.8.2

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

Hi,

[auto build test WARNING on net/master]
[also build test WARNING on v4.7-rc3 next-20160609]
[cannot apply to net-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dongpo-Li/Add-Hisilicon-MDIO-bus-driver-and-FEMAC-driver/20160613-141459
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

warning: (HISI_FEMAC) selects MDIO_HISI_FEMAC which has unmet direct dependencies (NETDEVICES && PHYLIB && ARCH_HISI && HAS_IOMEM && OF_MDIO)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46357 bytes --]

Re: [PATCH 2/3] ethtool: Add common functions for get and set settings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

Hi,

[auto build test ERROR on net/master]
[also build test ERROR on v4.7-rc3 next-20160609]
[cannot apply to net-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dongpo-Li/Add-Hisilicon-MDIO-bus-driver-and-FEMAC-driver/20160613-141459
config: i386-randconfig-i0-201624 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/built-in.o: In function `ethtool_op_get_settings':
>> (.text+0x18f07): undefined reference to `phy_ethtool_gset'
   net/built-in.o: In function `ethtool_op_set_settings':
>> (.text+0x18f40): undefined reference to `phy_ethtool_sset'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24021 bytes --]

Re: [PATCH 2/3] ethtool: Add common functions for get and set settings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

Hi,

[auto build test ERROR on net/master]
[also build test ERROR on v4.7-rc3 next-20160609]
[cannot apply to net-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dongpo-Li/Add-Hisilicon-MDIO-bus-driver-and-FEMAC-driver/20160613-141459
config: sh-sh7785lcr_32bit_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   net/built-in.o: In function `ethtool_op_get_settings':
>> net/core/ethtool.c:56: undefined reference to `phy_ethtool_gset'
   net/built-in.o: In function `ethtool_op_set_settings':
>> net/core/ethtool.c:68: undefined reference to `phy_ethtool_sset'

vim +56 net/core/ethtool.c

    50	}
    51	EXPORT_SYMBOL(ethtool_op_get_ts_info);
    52	
    53	int ethtool_op_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
    54	{
    55		if (!dev->phydev)
  > 56			return -ENODEV;
    57	
    58		return phy_ethtool_gset(dev->phydev, cmd);
    59	}
    60	EXPORT_SYMBOL(ethtool_op_get_settings);
    61	
    62	int ethtool_op_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
    63	{
    64		if (!capable(CAP_NET_ADMIN))
    65			return -EPERM;
    66	
    67		if (!dev->phydev)
  > 68			return -ENODEV;
    69	
    70		return phy_ethtool_sset(dev->phydev, cmd);
    71	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 16936 bytes --]

Re: [PATCH 2/3] ethtool: Add common functions for get and set settings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

Hi,

[auto build test ERROR on net/master]
[also build test ERROR on v4.7-rc3 next-20160609]
[cannot apply to net-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dongpo-Li/Add-Hisilicon-MDIO-bus-driver-and-FEMAC-driver/20160613-141459
config: arm-magician_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   net/built-in.o: In function `ethtool_op_get_settings':
>> sysctl_net.c:(.text+0x1dba0): undefined reference to `phy_ethtool_gset'
   net/built-in.o: In function `ethtool_op_set_settings':
>> sysctl_net.c:(.text+0x1dbdc): undefined reference to `phy_ethtool_sset'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 17379 bytes --]

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Arnd Bergmann]

On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:

> +- reset-names: should contain the reset signal name "mac_reset"(required)
> +	and "phy_reset"(optional).

Maybe just name the resets 'mac' and 'phy'? The '_reset' part is
implied by the property.

I gave the driver a brief review and basically everything looks
great, very nice work!

There are two small things that I noticed:

> +
> +       do {
> +               hisi_femac_xmit_reclaim(dev);
> +               num = hisi_femac_rx(dev, task);
> +               work_done += num;
> +               task -= num;
> +               if ((work_done >= budget) || (num == 0))
> +                       break;
> +
> +               ints = readl(priv->glb_base + GLB_IRQ_STAT);
> +               writel(ints & DEF_INT_MASK,
> +                      priv->glb_base + GLB_IRQ_RAW);
> +       } while (ints & DEF_INT_MASK);
> +
> +       if (work_done < budget) {
> +               napi_complete(napi);
> +               hisi_femac_irq_enable(priv, DEF_INT_MASK);
> +       }

You tx function uses BQL to optimize the queue length, and that
is great. You also check xmit reclaim for rx interrupts, so
as long as you have both rx and tx traffic, this should work
great.

However, I notice that you only have a 'tx fifo empty'
interrupt triggering the napi poll, so I guess on a tx-only
workload you will always end up pushing packets into the
queue until BQL throttles tx, and then get the interrupt
after all packets have been sent, which will cause BQL to
make the queue longer up to the maximum queue size, and that
negates the effect of BQL.

Is there any way you can get a tx interrupt earlier than
this in order to get a more balanced queue, or is it ok
to just rely on rx packets to come in occasionally, and
just use the tx fifo empty interrupt as a fallback?

> +	priv->phy_mode = of_get_phy_mode(node);
> +	if (priv->phy_mode < 0) {
> +		dev_err(dev, "not find phy-mode\n");
> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
> +	if (!priv->phy_node) {
> +		dev_err(dev, "not find phy-handle\n");
> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	priv->phy = of_phy_connect(ndev, priv->phy_node,
> +				   hisi_femac_adjust_link, 0, priv->phy_mode);
> +	if (!(priv->phy) || IS_ERR(priv->phy)) {
> +		dev_err(dev, "connect to PHY failed!\n");
> +		ret = -ENODEV;
> +		goto out_phy_node;
> +	}

I wonder if we could generalize this set of three calls, I
get the impression that we duplicate this across several
drivers that shouldn't need to bother with the specific
phy-handle and phy-mode properties.

	Arnd

Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

[Andrew Lunn]

On Mon, Jun 13, 2016 at 02:07:54PM +0800, Dongpo Li wrote:
> This patch adds a separate driver for the MDIO interface of the
> Hisilicon Fast Ethernet MAC.
> 
> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
> ---
>  .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++
>  drivers/net/phy/Kconfig                            |   8 +
>  drivers/net/phy/Makefile                           |   1 +
>  drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
>  4 files changed, 196 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
>  create mode 100644 drivers/net/phy/mdio-hisi-femac.c
> 
> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
> new file mode 100644
> index 0000000..6f46337
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
> @@ -0,0 +1,22 @@
> +Hisilicon Fast Ethernet MDIO Controller interface
> +
> +Required properties:
> +- compatible: should be "hisilicon,hisi-femac-mdio".
> +- reg: address and length of the register set for the device.
> +- clocks: A phandle to the reference clock for this device.
> +
> +- PHY subnode: inherits from phy binding [1]
> +[1] Documentation/devicetree/bindings/net/phy.txt
> +
> +Example:
> +mdio: mdio@10091100 {
> +	compatible = "hisilicon,hisi-femac-mdio";
> +	reg = <0x10091100 0x10>;
> +	clocks = <&crg HI3518EV200_MDIO_CLK>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	phy0: phy@1 {
> +		reg = <1>;
> +	};
> +};
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 6dad9a9..e257322 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -271,6 +271,14 @@ config MDIO_BCM_IPROC
>  	  This module provides a driver for the MDIO busses found in the
>  	  Broadcom iProc SoC's.
>  
> +config MDIO_HISI_FEMAC
> +	tristate "Hisilicon FEMAC MDIO bus controller"
> +	depends on ARCH_HISI
> +	depends on HAS_IOMEM && OF_MDIO
> +	help
> +	  This module provides a driver for the MDIO busses found in the
> +	  Hisilicon SoC that have an Fast Ethernet MAC.
> +
>  endif # PHYLIB
>  
>  config MICREL_KS8995MA
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index fcdbb92..039c4be 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -44,3 +44,4 @@ obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
>  obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
>  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>  obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
> +obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
> diff --git a/drivers/net/phy/mdio-hisi-femac.c b/drivers/net/phy/mdio-hisi-femac.c
> new file mode 100644
> index 0000000..a9eea61
> --- /dev/null
> +++ b/drivers/net/phy/mdio-hisi-femac.c
> @@ -0,0 +1,165 @@
> +/*
> + * Hisilicon Fast Ethernet MDIO Bus Driver
> + *
> + * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_mdio.h>
> +#include <linux/platform_device.h>
> +
> +#define MDIO_RWCTRL		0x00
> +#define MDIO_RO_DATA		0x04
> +#define MDIO_WRITE		BIT(13)
> +#define MDIO_RW_FINISH		BIT(15)
> +#define BIT_PHY_ADDR_OFFSET	8
> +#define BIT_WR_DATA_OFFSET	16
> +
> +struct hisi_femac_mdio_data {
> +	struct clk *clk;
> +	void __iomem *membase;
> +};
> +

Hi Dongpo

Overall this looks good. Just some minor comments

> +static int hisi_femac_mdio_wait_ready(struct mii_bus *bus)
> +{
> +	struct hisi_femac_mdio_data *data = bus->priv;

You could just pass data here. Your read and write functions already
have it.

> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk)) {
> +		ret = -ENODEV;
> +		goto err_out_free_mdiobus;
> +	}

Return the error which devm_clk_get() gives you.

> +
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		goto err_out_free_mdiobus;
> +
> +	ret = of_mdiobus_register(bus, np);
> +	if (ret)
> +		goto err_out_free_mdiobus;

You leave the clock prepared and enabled on error.

    Andrew

Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

[Li Dongpo]

On 2016/6/13 21:32, Andrew Lunn wrote:
> On Mon, Jun 13, 2016 at 02:07:54PM +0800, Dongpo Li wrote:
>> This patch adds a separate driver for the MDIO interface of the
>> Hisilicon Fast Ethernet MAC.
>>
>> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
>> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
>> ---
>>  .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++
>>  drivers/net/phy/Kconfig                            |   8 +
>>  drivers/net/phy/Makefile                           |   1 +
>>  drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
>>  4 files changed, 196 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
>>  create mode 100644 drivers/net/phy/mdio-hisi-femac.c

[...]
>> +
> 
> Hi Dongpo
> 
> Overall this looks good. Just some minor comments
> 
>> +static int hisi_femac_mdio_wait_ready(struct mii_bus *bus)
>> +{
>> +	struct hisi_femac_mdio_data *data = bus->priv;
> 
> You could just pass data here. Your read and write functions already
> have it.
> 
Thank you, I will fix it in next patch version.

>> +	data->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(data->clk)) {
>> +		ret = -ENODEV;
>> +		goto err_out_free_mdiobus;
>> +	}
> 
> Return the error which devm_clk_get() gives you.
> 
ok, I will fix it.

>> +
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret)
>> +		goto err_out_free_mdiobus;
>> +
>> +	ret = of_mdiobus_register(bus, np);
>> +	if (ret)
>> +		goto err_out_free_mdiobus;
> 
> You leave the clock prepared and enabled on error.
> 
ok, I will fix it.

>     Andrew
> 
> .
> 

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Li Dongpo]

On 2016/6/13 17:06, Arnd Bergmann wrote:
> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
> 
>> +- reset-names: should contain the reset signal name "mac_reset"(required)
>> +	and "phy_reset"(optional).
> 
> Maybe just name the resets 'mac' and 'phy'? The '_reset' part is
> implied by the property.
> 
Hi, Arnd,
Thank you for your advice. It's ok to omit the '_reset', I'll fix it in next version.

> I gave the driver a brief review and basically everything looks
> great, very nice work!
> 
> There are two small things that I noticed:
> 
>> +
>> +       do {
>> +               hisi_femac_xmit_reclaim(dev);
>> +               num = hisi_femac_rx(dev, task);
>> +               work_done += num;
>> +               task -= num;
>> +               if ((work_done >= budget) || (num == 0))
>> +                       break;
>> +
>> +               ints = readl(priv->glb_base + GLB_IRQ_STAT);
>> +               writel(ints & DEF_INT_MASK,
>> +                      priv->glb_base + GLB_IRQ_RAW);
>> +       } while (ints & DEF_INT_MASK);
>> +
>> +       if (work_done < budget) {
>> +               napi_complete(napi);
>> +               hisi_femac_irq_enable(priv, DEF_INT_MASK);
>> +       }
> 
> You tx function uses BQL to optimize the queue length, and that
> is great. You also check xmit reclaim for rx interrupts, so
> as long as you have both rx and tx traffic, this should work
> great.
> 
> However, I notice that you only have a 'tx fifo empty'
> interrupt triggering the napi poll, so I guess on a tx-only
> workload you will always end up pushing packets into the
> queue until BQL throttles tx, and then get the interrupt
> after all packets have been sent, which will cause BQL to
> make the queue longer up to the maximum queue size, and that
> negates the effect of BQL.
> 
> Is there any way you can get a tx interrupt earlier than
> this in order to get a more balanced queue, or is it ok
> to just rely on rx packets to come in occasionally, and
> just use the tx fifo empty interrupt as a fallback?
> 
In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
and 'tx one packet finish'. I didn't use 'tx one packet finish' because
it would lead to high hardware interrupts rate. This has been verified in
our chips. It's ok to just use tx fifo empty interrupt.

>> +	priv->phy_mode = of_get_phy_mode(node);
>> +	if (priv->phy_mode < 0) {
>> +		dev_err(dev, "not find phy-mode\n");
>> +		ret = -EINVAL;
>> +		goto out_disable_clk;
>> +	}
>> +
>> +	priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
>> +	if (!priv->phy_node) {
>> +		dev_err(dev, "not find phy-handle\n");
>> +		ret = -EINVAL;
>> +		goto out_disable_clk;
>> +	}
>> +
>> +	priv->phy = of_phy_connect(ndev, priv->phy_node,
>> +				   hisi_femac_adjust_link, 0, priv->phy_mode);
>> +	if (!(priv->phy) || IS_ERR(priv->phy)) {
>> +		dev_err(dev, "connect to PHY failed!\n");
>> +		ret = -ENODEV;
>> +		goto out_phy_node;
>> +	}
> 
> I wonder if we could generalize this set of three calls, I
> get the impression that we duplicate this across several
> drivers that shouldn't need to bother with the specific
> phy-handle and phy-mode properties.
> 
Some drivers only call 'of_phy_connect' when ndo_open called,
some call when driver probed. But 'phy_mode' and 'phy_node' are
usually initialized when driver probed.
So I think it's not suitable to combine 'of_phy_connect' with
'of_get_phy_mode' and 'of_parse_phandle'.
Do you have any more suggestions ?

> 	Arnd
> 
> 
> .
> 
	Regards,
	Dongpo


.

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Arnd Bergmann]

On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
> On 2016/6/13 17:06, Arnd Bergmann wrote:
> > On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
> > You tx function uses BQL to optimize the queue length, and that
> > is great. You also check xmit reclaim for rx interrupts, so
> > as long as you have both rx and tx traffic, this should work
> > great.
> > 
> > However, I notice that you only have a 'tx fifo empty'
> > interrupt triggering the napi poll, so I guess on a tx-only
> > workload you will always end up pushing packets into the
> > queue until BQL throttles tx, and then get the interrupt
> > after all packets have been sent, which will cause BQL to
> > make the queue longer up to the maximum queue size, and that
> > negates the effect of BQL.
> > 
> > Is there any way you can get a tx interrupt earlier than
> > this in order to get a more balanced queue, or is it ok
> > to just rely on rx packets to come in occasionally, and
> > just use the tx fifo empty interrupt as a fallback?
> > 
> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
> it would lead to high hardware interrupts rate. This has been verified in
> our chips. It's ok to just use tx fifo empty interrupt.

I'm not convinced by the explanation, I don't think that has anything
to do with the hardware design, but instead is about the correctness
of the BQL logic with your driver.

Maybe your xmit function can do something like

	if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
		enable per-packet interrupt
	else
		use only fifo-empty interrupt

That way, you don't get a lot of interrupts when the system is
in a state of packets being received and sent continuously,
but if you get to the point where your tx queue fills up
and no rx interrupts arrive, you don't have to wait for it
to become completely empty before adding new packets, and
BQL won't keep growing the queue.

> >> +    priv->phy_mode = of_get_phy_mode(node);
> >> +    if (priv->phy_mode < 0) {
> >> +            dev_err(dev, "not find phy-mode\n");
> >> +            ret = -EINVAL;
> >> +            goto out_disable_clk;
> >> +    }
> >> +
> >> +    priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
> >> +    if (!priv->phy_node) {
> >> +            dev_err(dev, "not find phy-handle\n");
> >> +            ret = -EINVAL;
> >> +            goto out_disable_clk;
> >> +    }
> >> +
> >> +    priv->phy = of_phy_connect(ndev, priv->phy_node,
> >> +                               hisi_femac_adjust_link, 0, priv->phy_mode);
> >> +    if (!(priv->phy) || IS_ERR(priv->phy)) {
> >> +            dev_err(dev, "connect to PHY failed!\n");
> >> +            ret = -ENODEV;
> >> +            goto out_phy_node;
> >> +    }
> > 
> > I wonder if we could generalize this set of three calls, I
> > get the impression that we duplicate this across several
> > drivers that shouldn't need to bother with the specific
> > phy-handle and phy-mode properties.
> > 
> Some drivers only call 'of_phy_connect' when ndo_open called,
> some call when driver probed. But 'phy_mode' and 'phy_node' are
> usually initialized when driver probed.
> So I think it's not suitable to combine 'of_phy_connect' with
> 'of_get_phy_mode' and 'of_parse_phandle'.
> Do you have any more suggestions ?

My idea was to add another interface that drivers could optionally
call if they use the logic that you have here, but other drivers
could keep using the plain of_phy_connect.

Anyway, this was just an idea, it's not important.

	Arnd

Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

[Rob Herring]

On Mon, Jun 13, 2016 at 02:07:54PM +0800, Dongpo Li wrote:
> This patch adds a separate driver for the MDIO interface of the
> Hisilicon Fast Ethernet MAC.
> 
> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
> ---
>  .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/net/phy/Kconfig                            |   8 +
>  drivers/net/phy/Makefile                           |   1 +
>  drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
>  4 files changed, 196 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
>  create mode 100644 drivers/net/phy/mdio-hisi-femac.c

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Rob Herring]

On Mon, Jun 13, 2016 at 02:07:56PM +0800, Dongpo Li wrote:
> This patch adds the Hisilicon Fast Ethernet MAC(FEMAC) driver.
> The FEMAC supports max speed 100Mbps and has been used in many
> Hisilicon SoC.
> 
> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
> ---
>  .../devicetree/bindings/net/hisilicon-femac.txt    |   40 +
>  drivers/net/ethernet/hisilicon/Kconfig             |   12 +
>  drivers/net/ethernet/hisilicon/Makefile            |    1 +
>  drivers/net/ethernet/hisilicon/hisi_femac.c        | 1015 ++++++++++++++++++++
>  4 files changed, 1068 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac.txt
>  create mode 100644 drivers/net/ethernet/hisilicon/hisi_femac.c
> 
> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
> new file mode 100644
> index 0000000..b953a56
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
> @@ -0,0 +1,40 @@
> +Hisilicon Fast Ethernet MAC controller
> +
> +Required properties:
> +- compatible: should be "hisilicon,hisi-femac" and one of the following:

This compatible seems a bit pointless. The following 2 are generic 
enough.

> +	* "hisilicon,hisi-femac-v1"
> +	* "hisilicon,hisi-femac-v2"

SoC specific compatible strings in addition to these please.

> +- reg: specifies base physical address(s) and size of the device registers.
> +  The first region is the MAC core register base and size.
> +  The second region is the global MAC control register.
> +- interrupts: should contain the MAC interrupt.
> +- clocks: clock phandle and specifier pair.

How many clocks?

> +- resets: should contain the phandle to the MAC reset signal(required) and
> +	the PHY reset signal(optional).
> +- reset-names: should contain the reset signal name "mac_reset"(required)
> +	and "phy_reset"(optional).
> +- mac-address: see ethernet.txt [1].
> +- phy-mode: see ethernet.txt [1].
> +- phy-handle: see ethernet.txt [1].
> +- hisilicon,phy-reset-delays: triplet of delays if PHY reset signal given.
> +	The 1st cell is reset pre-delay in micro seconds.
> +	The 2nd cell is reset pulse in micro seconds.
> +	The 3rd cell is reset post-delay in micro seconds.

Add standard unit suffixes.

> +
> +[1] Documentation/devicetree/bindings/net/ethernet.txt
> +
> +Example:
> +	hisi_femac: ethernet@10090000 {
> +		compatible = "hisilicon,hisi-femac-v2", "hisilicon,hisi-femac";
> +		reg = <0x10090000 0x1000>,<0x10091300 0x200>;
> +		interrupts = <12>;
> +		clocks = <&crg HI3518EV200_ETH_CLK>;
> +		resets = <&crg 0xec 0>,
> +				<&crg 0xec 3>;
> +		reset-names = "mac_reset",
> +				"phy_reset";
> +		mac-address = [00 00 00 00 00 00];
> +		phy-mode = "mii";
> +		phy-handle = <&phy0>;
> +		hisilicon,phy-reset-delays = <10000 20000 20000>;
> +	};

Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

[Li Dongpo]

On 2016/6/15 6:27, Rob Herring wrote:
> On Mon, Jun 13, 2016 at 02:07:54PM +0800, Dongpo Li wrote:
>> This patch adds a separate driver for the MDIO interface of the
>> Hisilicon Fast Ethernet MAC.
>>
>> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
>> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
>> ---
>>  .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
Hi Rob,
Thanks for your review.
>>  drivers/net/phy/Kconfig                            |   8 +
>>  drivers/net/phy/Makefile                           |   1 +
>>  drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
>>  4 files changed, 196 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
>>  create mode 100644 drivers/net/phy/mdio-hisi-femac.c
> 
> .
> 

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Dongpo Li]

On 2016/6/15 5:20, Arnd Bergmann wrote:
> On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
>> On 2016/6/13 17:06, Arnd Bergmann wrote:
>>> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
>>> You tx function uses BQL to optimize the queue length, and that
>>> is great. You also check xmit reclaim for rx interrupts, so
>>> as long as you have both rx and tx traffic, this should work
>>> great.
>>>
>>> However, I notice that you only have a 'tx fifo empty'
>>> interrupt triggering the napi poll, so I guess on a tx-only
>>> workload you will always end up pushing packets into the
>>> queue until BQL throttles tx, and then get the interrupt
>>> after all packets have been sent, which will cause BQL to
>>> make the queue longer up to the maximum queue size, and that
>>> negates the effect of BQL.
>>>
>>> Is there any way you can get a tx interrupt earlier than
>>> this in order to get a more balanced queue, or is it ok
>>> to just rely on rx packets to come in occasionally, and
>>> just use the tx fifo empty interrupt as a fallback?
>>>
>> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
>> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
>> it would lead to high hardware interrupts rate. This has been verified in
>> our chips. It's ok to just use tx fifo empty interrupt.
> 
> I'm not convinced by the explanation, I don't think that has anything
> to do with the hardware design, but instead is about the correctness
> of the BQL logic with your driver.
> 
> Maybe your xmit function can do something like
> 
> 	if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
> 		enable per-packet interrupt
> 	else
> 		use only fifo-empty interrupt
> 
> That way, you don't get a lot of interrupts when the system is
> in a state of packets being received and sent continuously,
> but if you get to the point where your tx queue fills up
> and no rx interrupts arrive, you don't have to wait for it
> to become completely empty before adding new packets, and
> BQL won't keep growing the queue.
> 
Hi Arnd,
Thanks for your advice. It's a good advice and I will try to fix it and
test on our chip.

>>>> +    priv->phy_mode = of_get_phy_mode(node);
>>>> +    if (priv->phy_mode < 0) {
>>>> +            dev_err(dev, "not find phy-mode\n");
>>>> +            ret = -EINVAL;
>>>> +            goto out_disable_clk;
>>>> +    }
>>>> +
>>>> +    priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
>>>> +    if (!priv->phy_node) {
>>>> +            dev_err(dev, "not find phy-handle\n");
>>>> +            ret = -EINVAL;
>>>> +            goto out_disable_clk;
>>>> +    }
>>>> +
>>>> +    priv->phy = of_phy_connect(ndev, priv->phy_node,
>>>> +                               hisi_femac_adjust_link, 0, priv->phy_mode);
>>>> +    if (!(priv->phy) || IS_ERR(priv->phy)) {
>>>> +            dev_err(dev, "connect to PHY failed!\n");
>>>> +            ret = -ENODEV;
>>>> +            goto out_phy_node;
>>>> +    }
>>>
>>> I wonder if we could generalize this set of three calls, I
>>> get the impression that we duplicate this across several
>>> drivers that shouldn't need to bother with the specific
>>> phy-handle and phy-mode properties.
>>>
>> Some drivers only call 'of_phy_connect' when ndo_open called,
>> some call when driver probed. But 'phy_mode' and 'phy_node' are
>> usually initialized when driver probed.
>> So I think it's not suitable to combine 'of_phy_connect' with
>> 'of_get_phy_mode' and 'of_parse_phandle'.
>> Do you have any more suggestions ?
> 
> My idea was to add another interface that drivers could optionally
> call if they use the logic that you have here, but other drivers
> could keep using the plain of_phy_connect.
> 
> Anyway, this was just an idea, it's not important.
> 
ok, I get your point. I will try to figure out the general interface.
If there is a solution, I'd like to get more review.
> 	Arnd
> 
> .
> 

    Regards,
    Dongpo

.

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Dongpo Li]

On 2016/6/15 6:31, Rob Herring wrote:
> On Mon, Jun 13, 2016 at 02:07:56PM +0800, Dongpo Li wrote:
>> This patch adds the Hisilicon Fast Ethernet MAC(FEMAC) driver.
>> The FEMAC supports max speed 100Mbps and has been used in many
>> Hisilicon SoC.
>>
>> Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
>> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
>> ---
>>  .../devicetree/bindings/net/hisilicon-femac.txt    |   40 +
>>  drivers/net/ethernet/hisilicon/Kconfig             |   12 +
>>  drivers/net/ethernet/hisilicon/Makefile            |    1 +
>>  drivers/net/ethernet/hisilicon/hisi_femac.c        | 1015 ++++++++++++++++++++
>>  4 files changed, 1068 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac.txt
>>  create mode 100644 drivers/net/ethernet/hisilicon/hisi_femac.c
>>
>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
>> new file mode 100644
>> index 0000000..b953a56
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
>> @@ -0,0 +1,40 @@
>> +Hisilicon Fast Ethernet MAC controller
>> +
>> +Required properties:
>> +- compatible: should be "hisilicon,hisi-femac" and one of the following:
> 
> This compatible seems a bit pointless. The following 2 are generic 
> enough.
> 
ok, I will remove this compatible.

>> +	* "hisilicon,hisi-femac-v1"
>> +	* "hisilicon,hisi-femac-v2"
> 
> SoC specific compatible strings in addition to these please.
> 
ok.

>> +- reg: specifies base physical address(s) and size of the device registers.
>> +  The first region is the MAC core register base and size.
>> +  The second region is the global MAC control register.
>> +- interrupts: should contain the MAC interrupt.
>> +- clocks: clock phandle and specifier pair.
> 
> How many clocks?
> 
Only one clock, the following description is ok?
- clocks: phandle reference to the MAC main clock

>> +- resets: should contain the phandle to the MAC reset signal(required) and
>> +	the PHY reset signal(optional).
>> +- reset-names: should contain the reset signal name "mac_reset"(required)
>> +	and "phy_reset"(optional).
>> +- mac-address: see ethernet.txt [1].
>> +- phy-mode: see ethernet.txt [1].
>> +- phy-handle: see ethernet.txt [1].
>> +- hisilicon,phy-reset-delays: triplet of delays if PHY reset signal given.
>> +	The 1st cell is reset pre-delay in micro seconds.
>> +	The 2nd cell is reset pulse in micro seconds.
>> +	The 3rd cell is reset post-delay in micro seconds.
> 
> Add standard unit suffixes.
> 
ok.

>> +
>> +[1] Documentation/devicetree/bindings/net/ethernet.txt
>> +
>> +Example:
>> +	hisi_femac: ethernet@10090000 {
>> +		compatible = "hisilicon,hisi-femac-v2", "hisilicon,hisi-femac";
>> +		reg = <0x10090000 0x1000>,<0x10091300 0x200>;
>> +		interrupts = <12>;
>> +		clocks = <&crg HI3518EV200_ETH_CLK>;
>> +		resets = <&crg 0xec 0>,
>> +				<&crg 0xec 3>;
>> +		reset-names = "mac_reset",
>> +				"phy_reset";
>> +		mac-address = [00 00 00 00 00 00];
>> +		phy-mode = "mii";
>> +		phy-handle = <&phy0>;
>> +		hisilicon,phy-reset-delays = <10000 20000 20000>;
>> +	};
> 
> .
> 

    Regards,
    Dongpo

.

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Dongpo Li]

On 2016/6/15 5:20, Arnd Bergmann wrote:
> On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
>> On 2016/6/13 17:06, Arnd Bergmann wrote:
>>> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
>>> You tx function uses BQL to optimize the queue length, and that
>>> is great. You also check xmit reclaim for rx interrupts, so
>>> as long as you have both rx and tx traffic, this should work
>>> great.
>>>
>>> However, I notice that you only have a 'tx fifo empty'
>>> interrupt triggering the napi poll, so I guess on a tx-only
>>> workload you will always end up pushing packets into the
>>> queue until BQL throttles tx, and then get the interrupt
>>> after all packets have been sent, which will cause BQL to
>>> make the queue longer up to the maximum queue size, and that
>>> negates the effect of BQL.
>>>
>>> Is there any way you can get a tx interrupt earlier than
>>> this in order to get a more balanced queue, or is it ok
>>> to just rely on rx packets to come in occasionally, and
>>> just use the tx fifo empty interrupt as a fallback?
>>>
>> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
>> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
>> it would lead to high hardware interrupts rate. This has been verified in
>> our chips. It's ok to just use tx fifo empty interrupt.
> 
> I'm not convinced by the explanation, I don't think that has anything
> to do with the hardware design, but instead is about the correctness
> of the BQL logic with your driver.
> 
> Maybe your xmit function can do something like
> 
> 	if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
> 		enable per-packet interrupt
> 	else
> 		use only fifo-empty interrupt
> 
> That way, you don't get a lot of interrupts when the system is
> in a state of packets being received and sent continuously,
> but if you get to the point where your tx queue fills up
> and no rx interrupts arrive, you don't have to wait for it
> to become completely empty before adding new packets, and
> BQL won't keep growing the queue.
> 
Hi, Arnd
I tried enable per-packet interrupt when tx queue full in xmit function
and disable it in NAPI poll. But the number of interrupts are a little
bigger than only using fifo-empty interrupt.
The other hand, this is a fast ethernet MAC. Its maximum speed is 100Mbps.
This speed is very easily achived and the efficiency of the BQL is not
so important. What we focus on is the lower cpu utilization.
So I think it is okay to just use the tx fifo empty interrupt.

>>>> +    priv->phy_mode = of_get_phy_mode(node);
>>>> +    if (priv->phy_mode < 0) {
>>>> +            dev_err(dev, "not find phy-mode\n");
>>>> +            ret = -EINVAL;
>>>> +            goto out_disable_clk;
>>>> +    }
>>>> +
>>>> +    priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
>>>> +    if (!priv->phy_node) {
>>>> +            dev_err(dev, "not find phy-handle\n");
>>>> +            ret = -EINVAL;
>>>> +            goto out_disable_clk;
>>>> +    }
>>>> +
>>>> +    priv->phy = of_phy_connect(ndev, priv->phy_node,
>>>> +                               hisi_femac_adjust_link, 0, priv->phy_mode);
>>>> +    if (!(priv->phy) || IS_ERR(priv->phy)) {
>>>> +            dev_err(dev, "connect to PHY failed!\n");
>>>> +            ret = -ENODEV;
>>>> +            goto out_phy_node;
>>>> +    }
>>>
>>> I wonder if we could generalize this set of three calls, I
>>> get the impression that we duplicate this across several
>>> drivers that shouldn't need to bother with the specific
>>> phy-handle and phy-mode properties.
>>>
>> Some drivers only call 'of_phy_connect' when ndo_open called,
>> some call when driver probed. But 'phy_mode' and 'phy_node' are
>> usually initialized when driver probed.
>> So I think it's not suitable to combine 'of_phy_connect' with
>> 'of_get_phy_mode' and 'of_parse_phandle'.
>> Do you have any more suggestions ?
> 
> My idea was to add another interface that drivers could optionally
> call if they use the logic that you have here, but other drivers
> could keep using the plain of_phy_connect.
> 
> Anyway, this was just an idea, it's not important.
> 
> 	Arnd
> 
> .
> 

    Regards,
    Dongpo

.

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Arnd Bergmann]

On Tuesday, June 28, 2016 5:21:19 PM CEST Dongpo Li wrote:
> On 2016/6/15 5:20, Arnd Bergmann wrote:
> > On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
> >> On 2016/6/13 17:06, Arnd Bergmann wrote:
> >>> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
> >>> You tx function uses BQL to optimize the queue length, and that
> >>> is great. You also check xmit reclaim for rx interrupts, so
> >>> as long as you have both rx and tx traffic, this should work
> >>> great.
> >>>
> >>> However, I notice that you only have a 'tx fifo empty'
> >>> interrupt triggering the napi poll, so I guess on a tx-only
> >>> workload you will always end up pushing packets into the
> >>> queue until BQL throttles tx, and then get the interrupt
> >>> after all packets have been sent, which will cause BQL to
> >>> make the queue longer up to the maximum queue size, and that
> >>> negates the effect of BQL.
> >>>
> >>> Is there any way you can get a tx interrupt earlier than
> >>> this in order to get a more balanced queue, or is it ok
> >>> to just rely on rx packets to come in occasionally, and
> >>> just use the tx fifo empty interrupt as a fallback?
> >>>
> >> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
> >> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
> >> it would lead to high hardware interrupts rate. This has been verified in
> >> our chips. It's ok to just use tx fifo empty interrupt.
> > 
> > I'm not convinced by the explanation, I don't think that has anything
> > to do with the hardware design, but instead is about the correctness
> > of the BQL logic with your driver.
> > 
> > Maybe your xmit function can do something like
> > 
> >       if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
> >               enable per-packet interrupt
> >       else
> >               use only fifo-empty interrupt
> > 
> > That way, you don't get a lot of interrupts when the system is
> > in a state of packets being received and sent continuously,
> > but if you get to the point where your tx queue fills up
> > and no rx interrupts arrive, you don't have to wait for it
> > to become completely empty before adding new packets, and
> > BQL won't keep growing the queue.
> > 
> Hi, Arnd
> I tried enable per-packet interrupt when tx queue full in xmit function
> and disable it in NAPI poll. But the number of interrupts are a little
> bigger than only using fifo-empty interrupt.

Right, I'd expect that to be the case, it basically means that the
algorithm works as expected.

Just to be sure you didn't have extra interrupts: you only enable the
per-packet interrupts if interrupts are currently enabled, not in
NAPI polling mode, right?

> The other hand, this is a fast ethernet MAC. Its maximum speed is 100Mbps.
> This speed is very easily achived and the efficiency of the BQL is not
> so important. What we focus on is the lower cpu utilization.
> So I think it is okay to just use the tx fifo empty interrupt.

BQL is not about efficiency, it's about keeping the latency down, which
is at least as important for low-throughput devices as it is for faster
ones. I don't think that disabling BQL here would be the right answer,
you'd just end up with the maximum TX queue length all the time.

Your queue length is 12 packets of 1500 bytes, meaning that you have 1.4ms
of latency at 100mbit/s rate, or 14ms for 10mbit/s. This is much less
than most, but it's probably still worth using BQL on it.

	Arnd

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Dongpo Li]

Hi Arnd,

On 2016/6/28 17:34, Arnd Bergmann wrote:
> On Tuesday, June 28, 2016 5:21:19 PM CEST Dongpo Li wrote:
>> On 2016/6/15 5:20, Arnd Bergmann wrote:
>>> On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
>>>> On 2016/6/13 17:06, Arnd Bergmann wrote:
>>>>> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
>>>>> You tx function uses BQL to optimize the queue length, and that
>>>>> is great. You also check xmit reclaim for rx interrupts, so
>>>>> as long as you have both rx and tx traffic, this should work
>>>>> great.
>>>>>
>>>>> However, I notice that you only have a 'tx fifo empty'
>>>>> interrupt triggering the napi poll, so I guess on a tx-only
>>>>> workload you will always end up pushing packets into the
>>>>> queue until BQL throttles tx, and then get the interrupt
>>>>> after all packets have been sent, which will cause BQL to
>>>>> make the queue longer up to the maximum queue size, and that
>>>>> negates the effect of BQL.
>>>>>
>>>>> Is there any way you can get a tx interrupt earlier than
>>>>> this in order to get a more balanced queue, or is it ok
>>>>> to just rely on rx packets to come in occasionally, and
>>>>> just use the tx fifo empty interrupt as a fallback?
>>>>>
>>>> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
>>>> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
>>>> it would lead to high hardware interrupts rate. This has been verified in
>>>> our chips. It's ok to just use tx fifo empty interrupt.
>>>
>>> I'm not convinced by the explanation, I don't think that has anything
>>> to do with the hardware design, but instead is about the correctness
>>> of the BQL logic with your driver.
>>>
>>> Maybe your xmit function can do something like
>>>
>>>       if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
>>>               enable per-packet interrupt
>>>       else
>>>               use only fifo-empty interrupt
>>>
>>> That way, you don't get a lot of interrupts when the system is
>>> in a state of packets being received and sent continuously,
>>> but if you get to the point where your tx queue fills up
>>> and no rx interrupts arrive, you don't have to wait for it
>>> to become completely empty before adding new packets, and
>>> BQL won't keep growing the queue.
>>>
>> Hi, Arnd
>> I tried enable per-packet interrupt when tx queue full in xmit function
>> and disable it in NAPI poll. But the number of interrupts are a little
>> bigger than only using fifo-empty interrupt.
> 
> Right, I'd expect that to be the case, it basically means that the
> algorithm works as expected.
> 
> Just to be sure you didn't have extra interrupts: you only enable the
> per-packet interrupts if interrupts are currently enabled, not in
> NAPI polling mode, right?
> 
Sorry so long to reply to you. I use the per-packet interrupt like this:
In my xmit function,
	if (hardware tx fifo is full) {
		enable tx per-packet interrupt;
		netif_stop_queue(dev);
		return NETDEV_TX_BUSY;
	}

In interrupt handle function,
	if (interrupt is tx per-packet or tx fifo-empty or rx) {
		disable tx per-packet interrupt;
		napi_schedule(&priv->napi);
	}
We disable tx per-packet interrupt anyway because the NAPI poll will reclaim
the tx fifo.
When the NAPI poll completed, it will only enable the tx fifo-empty interrupt
and rx interrupt except the tx per-packet interrupt.

Is this solution okay?

>> The other hand, this is a fast ethernet MAC. Its maximum speed is 100Mbps.
>> This speed is very easily achived and the efficiency of the BQL is not
>> so important. What we focus on is the lower cpu utilization.
>> So I think it is okay to just use the tx fifo empty interrupt.
> 
> BQL is not about efficiency, it's about keeping the latency down, which
> is at least as important for low-throughput devices as it is for faster
> ones. I don't think that disabling BQL here would be the right answer,
> you'd just end up with the maximum TX queue length all the time.
> 
> Your queue length is 12 packets of 1500 bytes, meaning that you have 1.4ms
> of latency at 100mbit/s rate, or 14ms for 10mbit/s. This is much less
> than most, but it's probably still worth using BQL on it.
> 
I spent some time reading some articles and the goal of BQL is more clear to me.
BQL is designed to get the minimum buffer size that will not make hardware under starvation.
The goal is to reduce latency without the side effect of throughput.
Thanks for your explanation.

> 	Arnd
> 
> .
> 

    Regards,
    Dongpo

.

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Arnd Bergmann]

On Monday, July 11, 2016 11:44:23 AM CEST Dongpo Li wrote:
> Hi Arnd,
> 
> On 2016/6/28 17:34, Arnd Bergmann wrote:
> > On Tuesday, June 28, 2016 5:21:19 PM CEST Dongpo Li wrote:
> >> On 2016/6/15 5:20, Arnd Bergmann wrote:
> >>> On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
> >>>> On 2016/6/13 17:06, Arnd Bergmann wrote:
> >>>>> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
> >>>>> You tx function uses BQL to optimize the queue length, and that
> >>>>> is great. You also check xmit reclaim for rx interrupts, so
> >>>>> as long as you have both rx and tx traffic, this should work
> >>>>> great.
> >>>>>
> >>>>> However, I notice that you only have a 'tx fifo empty'
> >>>>> interrupt triggering the napi poll, so I guess on a tx-only
> >>>>> workload you will always end up pushing packets into the
> >>>>> queue until BQL throttles tx, and then get the interrupt
> >>>>> after all packets have been sent, which will cause BQL to
> >>>>> make the queue longer up to the maximum queue size, and that
> >>>>> negates the effect of BQL.
> >>>>>
> >>>>> Is there any way you can get a tx interrupt earlier than
> >>>>> this in order to get a more balanced queue, or is it ok
> >>>>> to just rely on rx packets to come in occasionally, and
> >>>>> just use the tx fifo empty interrupt as a fallback?
> >>>>>
> >>>> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
> >>>> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
> >>>> it would lead to high hardware interrupts rate. This has been verified in
> >>>> our chips. It's ok to just use tx fifo empty interrupt.
> >>>
> >>> I'm not convinced by the explanation, I don't think that has anything
> >>> to do with the hardware design, but instead is about the correctness
> >>> of the BQL logic with your driver.
> >>>
> >>> Maybe your xmit function can do something like
> >>>
> >>>       if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
> >>>               enable per-packet interrupt
> >>>       else
> >>>               use only fifo-empty interrupt
> >>>
> >>> That way, you don't get a lot of interrupts when the system is
> >>> in a state of packets being received and sent continuously,
> >>> but if you get to the point where your tx queue fills up
> >>> and no rx interrupts arrive, you don't have to wait for it
> >>> to become completely empty before adding new packets, and
> >>> BQL won't keep growing the queue.
> >>>
> >> Hi, Arnd
> >> I tried enable per-packet interrupt when tx queue full in xmit function
> >> and disable it in NAPI poll. But the number of interrupts are a little
> >> bigger than only using fifo-empty interrupt.
> > 
> > Right, I'd expect that to be the case, it basically means that the
> > algorithm works as expected.
> > 
> > Just to be sure you didn't have extra interrupts: you only enable the
> > per-packet interrupts if interrupts are currently enabled, not in
> > NAPI polling mode, right?
> > 
> Sorry so long to reply to you. I use the per-packet interrupt like this:
> In my xmit function,
> 	if (hardware tx fifo is full) {
> 		enable tx per-packet interrupt;
> 		netif_stop_queue(dev);
> 		return NETDEV_TX_BUSY;
> 	}
> 
> In interrupt handle function,
> 	if (interrupt is tx per-packet or tx fifo-empty or rx) {
> 		disable tx per-packet interrupt;
> 		napi_schedule(&priv->napi);
> 	}
> We disable tx per-packet interrupt anyway because the NAPI poll will reclaim
> the tx fifo.
> When the NAPI poll completed, it will only enable the tx fifo-empty interrupt
> and rx interrupt except the tx per-packet interrupt.
> 
> Is this solution okay?

Yes, this looks good to me.

	Arnd

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

[Dongpo Li]

On 2016/7/11 16:16, Arnd Bergmann wrote:
> On Monday, July 11, 2016 11:44:23 AM CEST Dongpo Li wrote:
>> Hi Arnd,
>>
>> On 2016/6/28 17:34, Arnd Bergmann wrote:
>>> On Tuesday, June 28, 2016 5:21:19 PM CEST Dongpo Li wrote:
>>>> On 2016/6/15 5:20, Arnd Bergmann wrote:
>>>>> On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote:
>>>>>> On 2016/6/13 17:06, Arnd Bergmann wrote:
>>>>>>> On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:
>>>>>>> You tx function uses BQL to optimize the queue length, and that
>>>>>>> is great. You also check xmit reclaim for rx interrupts, so
>>>>>>> as long as you have both rx and tx traffic, this should work
>>>>>>> great.
>>>>>>>
>>>>>>> However, I notice that you only have a 'tx fifo empty'
>>>>>>> interrupt triggering the napi poll, so I guess on a tx-only
>>>>>>> workload you will always end up pushing packets into the
>>>>>>> queue until BQL throttles tx, and then get the interrupt
>>>>>>> after all packets have been sent, which will cause BQL to
>>>>>>> make the queue longer up to the maximum queue size, and that
>>>>>>> negates the effect of BQL.
>>>>>>>
>>>>>>> Is there any way you can get a tx interrupt earlier than
>>>>>>> this in order to get a more balanced queue, or is it ok
>>>>>>> to just rely on rx packets to come in occasionally, and
>>>>>>> just use the tx fifo empty interrupt as a fallback?
>>>>>>>
>>>>>> In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
>>>>>> and 'tx one packet finish'. I didn't use 'tx one packet finish' because
>>>>>> it would lead to high hardware interrupts rate. This has been verified in
>>>>>> our chips. It's ok to just use tx fifo empty interrupt.
>>>>>
>>>>> I'm not convinced by the explanation, I don't think that has anything
>>>>> to do with the hardware design, but instead is about the correctness
>>>>> of the BQL logic with your driver.
>>>>>
>>>>> Maybe your xmit function can do something like
>>>>>
>>>>>       if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0)
>>>>>               enable per-packet interrupt
>>>>>       else
>>>>>               use only fifo-empty interrupt
>>>>>
>>>>> That way, you don't get a lot of interrupts when the system is
>>>>> in a state of packets being received and sent continuously,
>>>>> but if you get to the point where your tx queue fills up
>>>>> and no rx interrupts arrive, you don't have to wait for it
>>>>> to become completely empty before adding new packets, and
>>>>> BQL won't keep growing the queue.
>>>>>
>>>> Hi, Arnd
>>>> I tried enable per-packet interrupt when tx queue full in xmit function
>>>> and disable it in NAPI poll. But the number of interrupts are a little
>>>> bigger than only using fifo-empty interrupt.
>>>
>>> Right, I'd expect that to be the case, it basically means that the
>>> algorithm works as expected.
>>>
>>> Just to be sure you didn't have extra interrupts: you only enable the
>>> per-packet interrupts if interrupts are currently enabled, not in
>>> NAPI polling mode, right?
>>>
>> Sorry so long to reply to you. I use the per-packet interrupt like this:
>> In my xmit function,
>> 	if (hardware tx fifo is full) {
>> 		enable tx per-packet interrupt;
>> 		netif_stop_queue(dev);
>> 		return NETDEV_TX_BUSY;
>> 	}
>>
>> In interrupt handle function,
>> 	if (interrupt is tx per-packet or tx fifo-empty or rx) {
>> 		disable tx per-packet interrupt;
>> 		napi_schedule(&priv->napi);
>> 	}
>> We disable tx per-packet interrupt anyway because the NAPI poll will reclaim
>> the tx fifo.
>> When the NAPI poll completed, it will only enable the tx fifo-empty interrupt
>> and rx interrupt except the tx per-packet interrupt.
>>
>> Is this solution okay?
> 
> Yes, this looks good to me.
> 
Okay, many thanks for your review.

> 	Arnd
> 
> .
> 

    Regards,
    Dongpo

.