[RFC PATCH 0/6] Add the netdev support for Intel PAC N3000 FPGA

[RFC PATCH 0/6] Add the netdev support for Intel PAC N3000 FPGA

[Xu Yilun]

This patchset adds the driver for FPGA DFL (Device Feature List)
Ether Group private feature. It also adds the driver for the retimer
chips on the Intel MAX 10 BMC (Board Management Controller). These
devices are the networking components on Intel PAC N3000.

Patch #1 provides the document which gives a overview of the hardware
and basic driver design.

Patch #2 & #3 export some APIs to fetch necessary networking
information in DFL framework. These information will be used in the 
retimer driver and Ether Group driver.

Patch #4 implements the retimer driver.

Patch #5 implements the Ether Group driver for 25G.

Patch #6 adds 10G support for the Ether Group driver.


Xu Yilun (6):
  docs: networking: add the document for DFL Ether Group driver
  fpga: dfl: export network configuration info for DFL based FPGA
  fpga: dfl: add an API to get the base device for dfl device
  ethernet: m10-retimer: add support for retimers on Intel MAX 10 BMC
  ethernet: dfl-eth-group: add DFL eth group private feature driver
  ethernet: dfl-eth-group: add support for the 10G configurations

 .../ABI/testing/sysfs-class-net-dfl-eth-group      |  19 +
 .../networking/device_drivers/ethernet/index.rst   |   1 +
 .../ethernet/intel/dfl-eth-group.rst               | 102 ++++
 drivers/fpga/dfl-fme-main.c                        |  10 +-
 drivers/fpga/dfl-n3000-nios.c                      |  11 +-
 drivers/fpga/dfl.c                                 |  30 +
 drivers/fpga/dfl.h                                 |  12 +
 drivers/mfd/intel-m10-bmc.c                        |  18 +
 drivers/net/ethernet/intel/Kconfig                 |  30 +
 drivers/net/ethernet/intel/Makefile                |   4 +
 drivers/net/ethernet/intel/dfl-eth-group-10g.c     | 544 ++++++++++++++++++
 drivers/net/ethernet/intel/dfl-eth-group-25g.c     | 525 +++++++++++++++++
 drivers/net/ethernet/intel/dfl-eth-group-main.c    | 635 +++++++++++++++++++++
 drivers/net/ethernet/intel/dfl-eth-group.h         |  84 +++
 drivers/net/ethernet/intel/intel-m10-bmc-retimer.c | 231 ++++++++
 include/linux/dfl.h                                |   3 +
 include/linux/mfd/intel-m10-bmc.h                  |  16 +
 17 files changed, 2265 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
 create mode 100644 Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-10g.c
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-25g.c
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-main.c
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group.h
 create mode 100644 drivers/net/ethernet/intel/intel-m10-bmc-retimer.c

-- 
2.7.4

[RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Xu Yilun]

This patch adds the document for DFL Ether Group driver.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 .../networking/device_drivers/ethernet/index.rst   |   1 +
 .../ethernet/intel/dfl-eth-group.rst               | 102 +++++++++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst

diff --git a/Documentation/networking/device_drivers/ethernet/index.rst b/Documentation/networking/device_drivers/ethernet/index.rst
index cbb75a18..eb7c443 100644
--- a/Documentation/networking/device_drivers/ethernet/index.rst
+++ b/Documentation/networking/device_drivers/ethernet/index.rst
@@ -26,6 +26,7 @@ Contents:
    freescale/gianfar
    google/gve
    huawei/hinic
+   intel/dfl-eth-group
    intel/e100
    intel/e1000
    intel/e1000e
diff --git a/Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst b/Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst
new file mode 100644
index 0000000..525807e
--- /dev/null
+++ b/Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst
@@ -0,0 +1,102 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+=======================================================================
+DFL device driver for Ether Group private feature on Intel(R) PAC N3000
+=======================================================================
+
+This is the driver for Ether Group private feature on Intel(R)
+PAC (Programmable Acceleration Card) N3000.
+
+The Intel(R) PAC N3000 is a FPGA based SmartNIC platform for multi-workload
+networking application acceleration. A simple diagram below to for the board:
+
+                     +----------------------------------------+
+                     |                  FPGA                  |
++----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
+|QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
++----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
+                     |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
+                     +-----------+  |offloading|  +-----------+   +----------+
+                     |              +----------+              |
+                     |                                        |
+                     +----------------------------------------+
+
+The FPGA is composed of FPGA Interface Module (FIM) and Accelerated Function
+Unit (AFU). The FIM implements the basic functionalities for FPGA access,
+management and reprograming, while the AFU is the FPGA reprogramable region for
+users.
+
+The Line Side & Host Side Ether Groups are soft IP blocks embedded in FIM. They
+are internally wire connected to AFU and communicate with AFU with MAC packets.
+The user logic is developed by the FPGA users and re-programmed to AFU,
+providing the user defined wire connections between line side & host side data
+interfaces, as well as the MAC layer offloading.
+
+There are 2 types of interfaces for the Ether Groups:
+
+1. The data interfaces connects the Ether Groups and the AFU, host has no
+ability to control the data stream . So the FPGA is like a pipe between the
+host ethernet controller and the retimer chip.
+
+2. The management interfaces connects the Ether Groups to the host, so host
+could access the Ether Group registers for configuration and statistics
+reading.
+
+The Intel(R) PAC N3000 could be programmed to various configurations (with
+different link numbers and speeds, e.g. 8x10G, 4x25G ...). It is done by
+programing different variants of the Ether Group IP blocks, and doing
+corresponding configuration to the retimer chips.
+
+The DFL Ether Group driver registers netdev for each line side link. Users
+could use standard commands (ethtool, ip, ifconfig) for configuration and
+link state/statistics reading. For host side links, they are always connected
+to the host ethernet controller, so they should always have same features as
+the host ethernet controller. There is no need to register netdevs for them.
+The driver just enables these links on probe.
+
+The retimer chips are managed by onboard BMC (Board Management Controller)
+firmware, host driver is not capable to access them directly. So it is mostly
+like an external fixed PHY. However the link states detected by the retimer
+chips can not be propagated to the Ether Groups for hardware limitation, in
+order to manage the link state, a PHY driver (intel-m10-bmc-retimer) is
+introduced to query the BMC for the retimer's link state. The Ether Group
+driver would connect to the PHY devices and get the link states. The
+intel-m10-bmc-retimer driver creates a peseudo MDIO bus for each board, so
+that the Ether Group driver could find the PHY devices by their peseudo PHY
+addresses.
+
+
+2. Features supported
+=====================
+
+Data Path
+---------
+Since the driver can't control the data stream, the Ether Group driver
+doesn't implement the valid tx/rx functions. Any transmit attempt on these
+links from host will be dropped, and no data could be received to host from
+these links. Users should operate on the netdev of host ethernet controller
+for networking data traffic.
+
+
+Speed/Duplex
+------------
+The Ether Group doesn't support auto-negotiation. The link speed is fixed to
+10G, 25G or 40G full duplex according to which Ether Group IP is programmed.
+
+Statistics
+----------
+The Ether Group IP has the statistics counters for ethernet traffic and errors.
+The user can obtain these MAC-level statistics using "ethtool -S" option.
+
+MTU
+---
+The Ether Group IP is capable of detecting oversized packets. It will not drop
+the packet but pass it up and increment the tx/rx oversize counters. The MTU
+could be changed via ip or ifconfig commands.
+
+Flow Control
+------------
+Ethernet Flow Control (IEEE 802.3x) can be configured with ethtool to enable
+transmitting pause frames. Receiving pause request from outside to Ether Group
+MAC is not supported. The flow control auto-negotiation is not supported. The
+user can enable or disable Tx Flow Control using "ethtool -A eth? tx <on|off>"
-- 
2.7.4

[RFC PATCH 2/6] fpga: dfl: export network configuration info for DFL based FPGA

[Xu Yilun]

This patch makes preparation for supporting DFL Ether Group private
feature driver, which reads bitstream_id.vendor_net_cfg field to
determin the interconnection of network components on FPGA device.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/fpga/dfl-fme-main.c | 10 ++--------
 drivers/fpga/dfl.c          | 21 +++++++++++++++++++++
 drivers/fpga/dfl.h          | 12 ++++++++++++
 include/linux/dfl.h         |  2 ++
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 77ea04d..a2b8ba0 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -46,14 +46,8 @@ static DEVICE_ATTR_RO(ports_num);
 static ssize_t bitstream_id_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
-	void __iomem *base;
-	u64 v;
-
-	base = dfl_get_feature_ioaddr_by_id(dev, FME_FEATURE_ID_HEADER);
-
-	v = readq(base + FME_HDR_BITSTREAM_ID);
-
-	return scnprintf(buf, PAGE_SIZE, "0x%llx\n", (unsigned long long)v);
+	return scnprintf(buf, PAGE_SIZE, "0x%llx\n",
+			 (unsigned long long)dfl_get_bitstream_id(dev));
 }
 static DEVICE_ATTR_RO(bitstream_id);
 
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index bc35750..ca3c678 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -537,6 +537,27 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv)
 }
 EXPORT_SYMBOL(dfl_driver_unregister);
 
+int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev)
+{
+	struct device *fme_dev;
+	u64 v;
+
+	if (!dfl_dev)
+		return -EINVAL;
+
+	if (dfl_dev->type == FME_ID)
+		fme_dev = dfl_dev->dev.parent;
+	else
+		fme_dev = dfl_dev->cdev->fme_dev;
+
+	if (!fme_dev)
+		return -EINVAL;
+
+	v = dfl_get_bitstream_id(fme_dev);
+	return (int)FIELD_GET(FME_BID_VENDOR_NET_CFG, v);
+}
+EXPORT_SYMBOL_GPL(dfl_dev_get_vendor_net_cfg);
+
 #define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
 
 /**
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 2b82c96..6c7a6961 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -104,6 +104,9 @@
 #define FME_CAP_CACHE_SIZE	GENMASK_ULL(43, 32)	/* cache size in KB */
 #define FME_CAP_CACHE_ASSOC	GENMASK_ULL(47, 44)	/* Associativity */
 
+/* FME BITSTREAM_ID Register Bitfield */
+#define FME_BID_VENDOR_NET_CFG	GENMASK_ULL(35, 32)     /* vendor net cfg */
+
 /* FME Port Offset Register Bitfield */
 /* Offset to port device feature header */
 #define FME_PORT_OFST_DFH_OFST	GENMASK_ULL(23, 0)
@@ -397,6 +400,15 @@ static inline bool is_dfl_feature_present(struct device *dev, u16 id)
 	return !!dfl_get_feature_ioaddr_by_id(dev, id);
 }
 
+static inline u64 dfl_get_bitstream_id(struct device *dev)
+{
+	void __iomem *base;
+
+	base = dfl_get_feature_ioaddr_by_id(dev, FME_FEATURE_ID_HEADER);
+
+	return readq(base + FME_HDR_BITSTREAM_ID);
+}
+
 static inline
 struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
 {
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index e1b2471..5ee2b1e 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -67,6 +67,8 @@ struct dfl_driver {
 #define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
 #define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
 
+int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev);
+
 /*
  * use a macro to avoid include chaining to get THIS_MODULE.
  */
-- 
2.7.4

[RFC PATCH 3/6] fpga: dfl: add an API to get the base device for dfl device

[Xu Yilun]

This patch adds an API for dfl devices to find which physical device
owns the DFL.

This patch makes preparation for supporting DFL Ether Group private
feature driver. It uses this information to determine which retimer
device physically connects to which ether group.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/fpga/dfl.c  | 9 +++++++++
 include/linux/dfl.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index ca3c678..52d18e6 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -558,6 +558,15 @@ int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev)
 }
 EXPORT_SYMBOL_GPL(dfl_dev_get_vendor_net_cfg);
 
+struct device *dfl_dev_get_base_dev(struct dfl_device *dfl_dev)
+{
+	if (!dfl_dev || !dfl_dev->cdev)
+		return NULL;
+
+	return dfl_dev->cdev->parent;
+}
+EXPORT_SYMBOL_GPL(dfl_dev_get_base_dev);
+
 #define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
 
 /**
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 5ee2b1e..dd313f2 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -68,6 +68,7 @@ struct dfl_driver {
 #define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
 
 int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev);
+struct device *dfl_dev_get_base_dev(struct dfl_device *dfl_dev);
 
 /*
  * use a macro to avoid include chaining to get THIS_MODULE.
-- 
2.7.4

[RFC PATCH 4/6] ethernet: m10-retimer: add support for retimers on Intel MAX 10 BMC

[Xu Yilun]

This driver supports the ethernet retimers (Parkvale) for the Intel PAC
(Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.

Parkvale is an Intel(R) Ethernet serdes transceiver chip that supports up
to 100G transfer. On Intel PAC N3000 there are 2 Parkvale chips managed
by the Intel MAX 10 BMC firmware. They are configured in 4 ports 10G/25G
retimer mode. Host could query their link status via retimer interfaces
(Shared registers) on Intel MAX 10 BMC.

The driver adds the phy device for each port of the 2 retimer chips.
Since the phys are not accessed by the real MDIO bus, it creates a virtual
mdio bus for each NIC device instance, and a dedicated phy driver which
only provides the supported features and link state.

A DFL Ether Group driver will create net devices and connect to these
phys.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/fpga/dfl-n3000-nios.c                      |  11 +-
 drivers/mfd/intel-m10-bmc.c                        |  18 ++
 drivers/net/ethernet/intel/Kconfig                 |  12 ++
 drivers/net/ethernet/intel/Makefile                |   2 +
 drivers/net/ethernet/intel/intel-m10-bmc-retimer.c | 229 +++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h                  |  16 ++
 6 files changed, 286 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/intel-m10-bmc-retimer.c

diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
index 4983a2b..096931a 100644
--- a/drivers/fpga/dfl-n3000-nios.c
+++ b/drivers/fpga/dfl-n3000-nios.c
@@ -16,6 +16,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mfd/intel-m10-bmc.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/stddef.h>
@@ -159,6 +160,8 @@ struct n3000_nios {
 	struct regmap *regmap;
 	struct device *dev;
 	struct platform_device *altera_spi;
+	struct intel_m10bmc_platdata m10bmc_pdata;
+	struct intel_m10bmc_retimer_pdata m10bmc_retimer_pdata;
 };
 
 static ssize_t nios_fw_version_show(struct device *dev,
@@ -412,7 +415,8 @@ static struct spi_board_info m10_n3000_info = {
 	.chip_select = 0,
 };
 
-static int create_altera_spi_controller(struct n3000_nios *nn)
+static int create_altera_spi_controller(struct n3000_nios *nn,
+					struct device *retimer_master)
 {
 	struct altera_spi_platform_data pdata = { 0 };
 	struct platform_device_info pdevinfo = { 0 };
@@ -431,6 +435,9 @@ static int create_altera_spi_controller(struct n3000_nios *nn)
 	pdata.bits_per_word_mask =
 		SPI_BPW_RANGE_MASK(1, FIELD_GET(N3000_NS_PARAM_DATA_WIDTH, v));
 
+	nn->m10bmc_retimer_pdata.retimer_master = retimer_master;
+	nn->m10bmc_pdata.retimer = &nn->m10bmc_retimer_pdata;
+	m10_n3000_info.platform_data = &nn->m10bmc_pdata;
 	pdata.num_devices = 1;
 	pdata.devices = &m10_n3000_info;
 
@@ -549,7 +556,7 @@ static int n3000_nios_probe(struct dfl_device *ddev)
 	if (ret)
 		return ret;
 
-	ret = create_altera_spi_controller(nn);
+	ret = create_altera_spi_controller(nn, dfl_dev_get_base_dev(ddev));
 	if (ret)
 		dev_err(dev, "altera spi controller create failed: %d\n", ret);
 
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index b84579b..adbfb177 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -23,6 +23,21 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
 	{ .name = "n3000bmc-secure" },
 };
 
+static void
+m10bmc_init_cells_platdata(struct intel_m10bmc_platdata *pdata,
+			   struct mfd_cell *cells, int n_cell)
+{
+	int i;
+
+	for (i = 0; i < n_cell; i++) {
+		if (!strcmp(cells[i].name, "n3000bmc-retimer")) {
+			cells[i].platform_data = pdata->retimer;
+			cells[i].pdata_size =
+				pdata->retimer ? sizeof(*pdata->retimer) : 0;
+		}
+	}
+}
+
 static struct regmap_config intel_m10bmc_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -97,6 +112,7 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
 
 static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 {
+	struct intel_m10bmc_platdata *pdata = dev_get_platdata(&spi->dev);
 	const struct spi_device_id *id = spi_get_device_id(spi);
 	struct device *dev = &spi->dev;
 	struct mfd_cell *cells;
@@ -134,6 +150,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 		return -ENODEV;
 	}
 
+	m10bmc_init_cells_platdata(pdata, cells, n_cell);
+
 	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
 				   NULL, 0, NULL);
 	if (ret)
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 5aa8631..81c43d4 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -343,4 +343,16 @@ config IGC
 	  To compile this driver as a module, choose M here. The module
 	  will be called igc.
 
+config INTEL_M10_BMC_RETIMER
+	tristate "Intel(R) MAX 10 BMC ethernet retimer support"
+	depends on MFD_INTEL_M10_BMC
+	select PHYLIB
+        help
+          This driver supports the ethernet retimer (Parkvale) on
+	  Intel(R) MAX 10 BMC, which is used by Intel PAC N3000 FPGA based
+	  Smart NIC.
+
+	  To compile this driver as a module, choose M here. The module
+	  will be called intel-m10-bmc-retimer.
+
 endif # NET_VENDOR_INTEL
diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
index 3075290..5965447 100644
--- a/drivers/net/ethernet/intel/Makefile
+++ b/drivers/net/ethernet/intel/Makefile
@@ -16,3 +16,5 @@ obj-$(CONFIG_IXGB) += ixgb/
 obj-$(CONFIG_IAVF) += iavf/
 obj-$(CONFIG_FM10K) += fm10k/
 obj-$(CONFIG_ICE) += ice/
+
+obj-$(CONFIG_INTEL_M10_BMC_RETIMER) += intel-m10-bmc-retimer.o
diff --git a/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
new file mode 100644
index 0000000..c7b0558
--- /dev/null
+++ b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Intel Max10 BMC Retimer Interface Driver
+ *
+ * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ *
+ */
+#include <linux/device.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+
+#define NUM_CHIP	2
+#define MAX_LINK	4
+
+#define BITS_MASK(nbits)	((1 << (nbits)) - 1)
+
+#define N3000BMC_RETIMER_DEV_NAME "n3000bmc-retimer"
+#define M10BMC_RETIMER_MII_NAME "m10bmc retimer mii"
+
+struct m10bmc_retimer {
+	struct device *dev;
+	struct intel_m10bmc *m10bmc;
+	int num_devs;
+	struct device *base_dev;
+	struct mii_bus *retimer_mii_bus;
+};
+
+#define RETIMER_LINK_STAT_BIT(retimer_id, link_id) \
+	BIT(((retimer_id) << 2) + (link_id))
+
+static u32 retimer_get_link(struct m10bmc_retimer *retimer, int index)
+{
+	unsigned int val;
+
+	if (m10bmc_sys_read(retimer->m10bmc, PKVL_LINK_STATUS, &val)) {
+		dev_err(retimer->dev, "fail to read PKVL_LINK_STATUS\n");
+		return 0;
+	}
+
+	if (val & BIT(index))
+		return 1;
+
+	return 0;
+}
+
+static int m10bmc_retimer_phy_match(struct phy_device *phydev)
+{
+	if (phydev->mdio.bus->name &&
+	    !strcmp(phydev->mdio.bus->name, M10BMC_RETIMER_MII_NAME)) {
+		return 1;
+	}
+
+	return 0;
+}
+
+static int m10bmc_retimer_phy_probe(struct phy_device *phydev)
+{
+	struct m10bmc_retimer *retimer = phydev->mdio.bus->priv;
+
+	phydev->priv = retimer;
+
+	return 0;
+}
+
+static void m10bmc_retimer_phy_remove(struct phy_device *phydev)
+{
+	if (phydev->attached_dev)
+		phy_disconnect(phydev);
+}
+
+static int m10bmc_retimer_read_status(struct phy_device *phydev)
+{
+	struct m10bmc_retimer *retimer = phydev->priv;
+
+	phydev->link = retimer_get_link(retimer, phydev->mdio.addr);
+
+	phydev->duplex = DUPLEX_FULL;
+
+	return 0;
+}
+
+static int m10bmc_retimer_get_features(struct phy_device *phydev)
+{
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+			 phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
+			 phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
+			 phydev->supported);
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
+			 phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseSR_Full_BIT,
+			 phydev->supported);
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
+			 phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
+			 phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
+			 phydev->supported);
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
+
+	return 0;
+}
+
+static struct phy_driver m10bmc_retimer_phy_driver = {
+	.phy_id			= 0xffffffff,
+	.phy_id_mask		= 0xffffffff,
+	.name			= "m10bmc retimer PHY",
+	.match_phy_device	= m10bmc_retimer_phy_match,
+	.probe			= m10bmc_retimer_phy_probe,
+	.remove			= m10bmc_retimer_phy_remove,
+	.read_status		= m10bmc_retimer_read_status,
+	.get_features		= m10bmc_retimer_get_features,
+	.read_mmd		= genphy_read_mmd_unsupported,
+	.write_mmd		= genphy_write_mmd_unsupported,
+};
+
+static int m10bmc_retimer_read(struct mii_bus *bus, int addr, int regnum)
+{
+	struct m10bmc_retimer *retimer = bus->priv;
+
+	if (addr < retimer->num_devs &&
+	    (regnum == MII_PHYSID1 || regnum == MII_PHYSID2))
+		return 0;
+
+	return 0xffff;
+}
+
+static int m10bmc_retimer_write(struct mii_bus *bus, int addr, int regnum, u16 val)
+{
+	return 0;
+}
+
+static int m10bmc_retimer_mii_bus_init(struct m10bmc_retimer *retimer)
+{
+	struct mii_bus *bus;
+	int ret;
+
+	bus = devm_mdiobus_alloc(retimer->dev);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->priv = (void *)retimer;
+	bus->name = M10BMC_RETIMER_MII_NAME;
+	bus->read = m10bmc_retimer_read;
+	bus->write = m10bmc_retimer_write;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
+		 dev_name(retimer->base_dev));
+	bus->parent = retimer->dev;
+	bus->phy_mask = ~(BITS_MASK(retimer->num_devs));
+
+	ret = mdiobus_register(bus);
+	if (ret)
+		return ret;
+
+	retimer->retimer_mii_bus = bus;
+
+	return 0;
+}
+
+static void m10bmc_retimer_mii_bus_uinit(struct m10bmc_retimer *retimer)
+{
+	mdiobus_unregister(retimer->retimer_mii_bus);
+}
+
+static int intel_m10bmc_retimer_probe(struct platform_device *pdev)
+{
+	struct intel_m10bmc_retimer_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct intel_m10bmc *m10bmc = dev_get_drvdata(pdev->dev.parent);
+	struct m10bmc_retimer *retimer;
+
+	retimer = devm_kzalloc(&pdev->dev, sizeof(*retimer), GFP_KERNEL);
+	if (!retimer)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, retimer);
+
+	retimer->dev = &pdev->dev;
+	retimer->m10bmc = m10bmc;
+	retimer->base_dev = pdata->retimer_master;
+	retimer->num_devs = NUM_CHIP * MAX_LINK;
+
+	return m10bmc_retimer_mii_bus_init(retimer);
+}
+
+static int intel_m10bmc_retimer_remove(struct platform_device *pdev)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(&pdev->dev);
+
+	m10bmc_retimer_mii_bus_uinit(retimer);
+
+	return 0;
+}
+
+static struct platform_driver intel_m10bmc_retimer_driver = {
+	.probe = intel_m10bmc_retimer_probe,
+	.remove = intel_m10bmc_retimer_remove,
+	.driver = {
+		.name = N3000BMC_RETIMER_DEV_NAME,
+	},
+};
+
+static int __init intel_m10bmc_retimer_init(void)
+{
+	int ret;
+
+	ret = phy_driver_register(&m10bmc_retimer_phy_driver, THIS_MODULE);
+	if (ret)
+		return ret;
+
+	return platform_driver_register(&intel_m10bmc_retimer_driver);
+}
+module_init(intel_m10bmc_retimer_init);
+
+static void __exit intel_m10bmc_retimer_exit(void)
+{
+	platform_driver_unregister(&intel_m10bmc_retimer_driver);
+	phy_driver_unregister(&m10bmc_retimer_phy_driver);
+}
+module_exit(intel_m10bmc_retimer_exit);
+
+MODULE_ALIAS("platform:" N3000BMC_RETIMER_DEV_NAME);
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel MAX 10 BMC retimer driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index c8ef2f1..3d9d22c 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -21,6 +21,22 @@
 #define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
 #define M10BMC_VER_LEGACY_INVALID	0xffffffff
 
+/* PKVL related registers, in system register region */
+#define PKVL_LINK_STATUS		0x164
+
+/**
+ * struct intel_m10bmc_retimer_pdata - subdev retimer platform data
+ *
+ * @retimer_master: the NIC device which connects to the retimers on m10bmc
+ */
+struct intel_m10bmc_retimer_pdata {
+	struct device *retimer_master;
+};
+
+struct intel_m10bmc_platdata {
+	struct intel_m10bmc_retimer_pdata *retimer;
+};
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
-- 
2.7.4

[RFC PATCH 5/6] ethernet: dfl-eth-group: add DFL eth group private feature driver

[Xu Yilun]

This driver supports the DFL Ether Group private feature for the Intel(R)
PAC N3000 FPGA Smart NIC.

The DFL Ether Group private feature contains an Ether Wrapper and several
ports (phy-mac pair). There are two types of Ether Groups, line side &
host side. These 2 types of Ether Groups, together with FPGA internal
logic, act as several independent pipes between the host Ethernet
Controller and the front-panel cages. The FPGA logic between the 2 Ether
Groups implements user defined mac layer offloading.

The line side ether interfaces connect to the Parkvale serdes transceiver
chips, which are working in 4 ports 10G/25G retimer mode.

There are several configurations (8x10G, 2x1x25G, 2x2x25G ...) for
different connections between line side and host side. Not all links are
active in some configurations.

For the line side link, the driver connects to the Parkvale phy device
and then register net device for each active link.

For the host side link, its link state is always on. The host side link
always has same features as host side ether controller, so there is no
need to register a netdev for it. The driver just enables the link on
probe.

This patch supports the 25G configurations. Support for 10G will be in
other patches.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../ABI/testing/sysfs-class-net-dfl-eth-group      |  19 +
 drivers/net/ethernet/intel/Kconfig                 |  18 +
 drivers/net/ethernet/intel/Makefile                |   2 +
 drivers/net/ethernet/intel/dfl-eth-group-25g.c     | 525 +++++++++++++++++
 drivers/net/ethernet/intel/dfl-eth-group-main.c    | 632 +++++++++++++++++++++
 drivers/net/ethernet/intel/dfl-eth-group.h         |  83 +++
 drivers/net/ethernet/intel/intel-m10-bmc-retimer.c |   4 +-
 7 files changed, 1282 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-25g.c
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-main.c
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group.h

diff --git a/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group b/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
new file mode 100644
index 0000000..ad528f2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
@@ -0,0 +1,19 @@
+What:		/sys/class/net/<iface>/tx_pause_frame_quanta
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:
+		Read-Write. Value representing the tx pause quanta of Pause
+		flow control frames to be sent to remote partner. Read the file
+		for the actual tx pause quanta value. Write the file to set
+		value of the tx pause quanta.
+
+What:		/sys/class/net/<iface>/tx_pause_frame_holdoff
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:
+		Read-Write. Value representing the separation between 2
+		consecutive XOFF flow control frames. Read the file for the
+		actual separation value. Write the file to set value of the
+		separation.
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 81c43d4..61b5d91 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -343,6 +343,24 @@ config IGC
 	  To compile this driver as a module, choose M here. The module
 	  will be called igc.
 
+config FPGA_DFL_ETH_GROUP
+        tristate "DFL Ether Group private feature support"
+        depends on FPGA_DFL && HAS_IOMEM
+        help
+          This driver supports DFL Ether Group private feature for
+	  Intel(R) PAC N3000 FPGA Smart NIC.
+
+	  The DFL Ether Group private feature contains several ether
+	  interfaces, each of them contains an Ether Wrapper and several
+	  ports (phy-mac pairs). There are two types of interfaces, Line
+	  side & CPU side. These 2 types of interfaces, together with FPGA
+	  internal logic, act as several independent pipes between the
+	  host Ethernet Controller and the front-panel cages. The FPGA
+	  logic between 2 interfaces implements user defined mac layer
+	  offloading.
+
+	  This driver implements each active port as a netdev.
+
 config INTEL_M10_BMC_RETIMER
 	tristate "Intel(R) MAX 10 BMC ethernet retimer support"
 	depends on MFD_INTEL_M10_BMC
diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
index 5965447..1624c26 100644
--- a/drivers/net/ethernet/intel/Makefile
+++ b/drivers/net/ethernet/intel/Makefile
@@ -17,4 +17,6 @@ obj-$(CONFIG_IAVF) += iavf/
 obj-$(CONFIG_FM10K) += fm10k/
 obj-$(CONFIG_ICE) += ice/
 
+dfl-eth-group-objs := dfl-eth-group-main.o dfl-eth-group-25g.o
+obj-$(CONFIG_FPGA_DFL_ETH_GROUP) += dfl-eth-group.o
 obj-$(CONFIG_INTEL_M10_BMC_RETIMER) += intel-m10-bmc-retimer.o
diff --git a/drivers/net/ethernet/intel/dfl-eth-group-25g.c b/drivers/net/ethernet/intel/dfl-eth-group-25g.c
new file mode 100644
index 0000000..a690364
--- /dev/null
+++ b/drivers/net/ethernet/intel/dfl-eth-group-25g.c
@@ -0,0 +1,525 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for 25G Ether Group private feature on Intel PAC (Programmable
+ * Acceleration Card) N3000
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Wu Hao <hao.wu@intel.com>
+ *   Xu Yilun <yilun.xu@intel.com>
+ */
+#include <linux/netdevice.h>
+
+#include "dfl-eth-group.h"
+
+/* 25G PHY/MAC Register */
+#define PHY_CONFIG	0x310
+#define PHY_MAC_RESET_MASK	GENMASK(2, 0)
+#define PHY_PMA_SLOOP		0x313
+#define MAX_TX_SIZE_CONFIG	0x407
+#define MAX_RX_SIZE_CONFIG	0x506
+#define TX_FLOW_CTRL_EN		0x605
+#define TX_FLOW_CTRL_EN_PAUSE	BIT(0)
+#define TX_FLOW_CTRL_QUANTA	0x620
+#define TX_FLOW_CTRL_HOLDOFF	0x628
+#define TX_FLOW_CTRL_SEL	0x640
+#define TX_FLOW_CTRL_SEL_PAUSE	0x0
+#define TX_FLOW_CTRL_SEL_PFC	0x1
+
+static int edev25g40g_reset(struct eth_dev *edev, bool en)
+{
+	struct eth_com *mac = edev->mac;
+	struct device *dev = edev->dev;
+	u32 val;
+	int ret;
+
+	ret = eth_com_read_reg(mac, PHY_CONFIG, &val);
+	if (ret) {
+		dev_err(dev, "fail to read PHY_CONFIG: %d\n", ret);
+		return ret;
+	}
+
+	/* skip if config is in expected state already */
+	if ((((val & PHY_MAC_RESET_MASK) == PHY_MAC_RESET_MASK) && en) ||
+	    (((val & PHY_MAC_RESET_MASK) == 0) && !en))
+		return 0;
+
+	if (en)
+		val |= PHY_MAC_RESET_MASK;
+	else
+		val &= ~PHY_MAC_RESET_MASK;
+
+	ret = eth_com_write_reg(mac, PHY_CONFIG, val);
+	if (ret)
+		dev_err(dev, "fail to write PHY_CONFIG: %d\n", ret);
+
+	return ret;
+}
+
+static ssize_t tx_pause_frame_quanta_show(struct device *d,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
+	u32 data;
+	int ret;
+
+	ret = eth_com_read_reg(edev->mac, TX_FLOW_CTRL_QUANTA, &data);
+
+	return ret ? : sprintf(buf, "0x%x\n", data);
+}
+
+static ssize_t tx_pause_frame_quanta_store(struct device *d,
+					   struct device_attribute *attr,
+					   const char *buf, size_t len)
+{
+	struct net_device *netdev = to_net_dev(d);
+	struct eth_dev *edev;
+	u32 data;
+	int ret;
+
+	if (kstrtou32(buf, 0, &data))
+		return -EINVAL;
+
+	edev = net_device_to_eth_dev(netdev);
+
+	rtnl_lock();
+
+	if (netif_running(netdev)) {
+		netdev_err(netdev, "must be stopped to change pause param\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = eth_com_write_reg(edev->mac, TX_FLOW_CTRL_QUANTA, data);
+
+out:
+	rtnl_unlock();
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_RW(tx_pause_frame_quanta);
+
+static ssize_t tx_pause_frame_holdoff_show(struct device *d,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
+	u32 data;
+	int ret;
+
+	ret = eth_com_read_reg(edev->mac, TX_FLOW_CTRL_HOLDOFF, &data);
+
+	return ret ? : sprintf(buf, "0x%x\n", data);
+}
+
+static ssize_t tx_pause_frame_holdoff_store(struct device *d,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	struct net_device *netdev = to_net_dev(d);
+	struct eth_dev *edev;
+	u32 data;
+	int ret;
+
+	if (kstrtou32(buf, 0, &data))
+		return -EINVAL;
+
+	edev = net_device_to_eth_dev(netdev);
+
+	rtnl_lock();
+
+	if (netif_running(netdev)) {
+		netdev_err(netdev, "must be stopped to change pause param\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = eth_com_write_reg(edev->mac, TX_FLOW_CTRL_HOLDOFF, data);
+
+out:
+	rtnl_unlock();
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_RW(tx_pause_frame_holdoff);
+
+static struct attribute *edev25g_dev_attrs[] = {
+	&dev_attr_tx_pause_frame_quanta.attr,
+	&dev_attr_tx_pause_frame_holdoff.attr,
+	NULL
+};
+
+/* device attributes */
+static const struct attribute_group edev25g_attr_group = {
+	.attrs = edev25g_dev_attrs,
+};
+
+/* ethtool ops */
+static struct stat_info stats_25g[] = {
+	/* TX Statistics */
+	{STAT_INFO(0x800, "tx_fragments")},
+	{STAT_INFO(0x802, "tx_jabbers")},
+	{STAT_INFO(0x804, "tx_fcs")},
+	{STAT_INFO(0x806, "tx_crc_err")},
+	{STAT_INFO(0x808, "tx_mcast_data_err")},
+	{STAT_INFO(0x80a, "tx_bcast_data_err")},
+	{STAT_INFO(0x80c, "tx_ucast_data_err")},
+	{STAT_INFO(0x80e, "tx_mcast_ctrl_err")},
+	{STAT_INFO(0x810, "tx_bcast_ctrl_err")},
+	{STAT_INFO(0x812, "tx_ucast_ctrl_err")},
+	{STAT_INFO(0x814, "tx_pause_err")},
+	{STAT_INFO(0x816, "tx_64_byte")},
+	{STAT_INFO(0x818, "tx_65_127_byte")},
+	{STAT_INFO(0x81a, "tx_128_255_byte")},
+	{STAT_INFO(0x81c, "tx_256_511_byte")},
+	{STAT_INFO(0x81e, "tx_512_1023_byte")},
+	{STAT_INFO(0x820, "tx_1024_1518_byte")},
+	{STAT_INFO(0x822, "tx_1519_max_byte")},
+	{STAT_INFO(0x824, "tx_oversize")},
+	{STAT_INFO(0x826, "tx_mcast_data_ok")},
+	{STAT_INFO(0x828, "tx_bcast_data_ok")},
+	{STAT_INFO(0x82a, "tx_ucast_data_ok")},
+	{STAT_INFO(0x82c, "tx_mcast_ctrl_ok")},
+	{STAT_INFO(0x82e, "tx_bcast_ctrl_ok")},
+	{STAT_INFO(0x830, "tx_ucast_ctrl_ok")},
+	{STAT_INFO(0x832, "tx_pause")},
+	{STAT_INFO(0x834, "tx_runt")},
+	{STAT_INFO(0x860, "tx_payload_octets_ok")},
+	{STAT_INFO(0x862, "tx_frame_octets_ok")},
+
+	/* RX Statistics */
+	{STAT_INFO(0x900, "rx_fragments")},
+	{STAT_INFO(0x902, "rx_jabbers")},
+	{STAT_INFO(0x904, "rx_fcs")},
+	{STAT_INFO(0x906, "rx_crc_err")},
+	{STAT_INFO(0x908, "rx_mcast_data_err")},
+	{STAT_INFO(0x90a, "rx_bcast_data_err")},
+	{STAT_INFO(0x90c, "rx_ucast_data_err")},
+	{STAT_INFO(0x90e, "rx_mcast_ctrl_err")},
+	{STAT_INFO(0x910, "rx_bcast_ctrl_err")},
+	{STAT_INFO(0x912, "rx_ucast_ctrl_err")},
+	{STAT_INFO(0x914, "rx_pause_err")},
+	{STAT_INFO(0x916, "rx_64_byte")},
+	{STAT_INFO(0x918, "rx_65_127_byte")},
+	{STAT_INFO(0x91a, "rx_128_255_byte")},
+	{STAT_INFO(0x91c, "rx_256_511_byte")},
+	{STAT_INFO(0x91e, "rx_512_1023_byte")},
+	{STAT_INFO(0x920, "rx_1024_1518_byte")},
+	{STAT_INFO(0x922, "rx_1519_max_byte")},
+	{STAT_INFO(0x924, "rx_oversize")},
+	{STAT_INFO(0x926, "rx_mcast_data_ok")},
+	{STAT_INFO(0x928, "rx_bcast_data_ok")},
+	{STAT_INFO(0x92a, "rx_ucast_data_ok")},
+	{STAT_INFO(0x92c, "rx_mcast_ctrl_ok")},
+	{STAT_INFO(0x92e, "rx_bcast_ctrl_ok")},
+	{STAT_INFO(0x930, "rx_ucast_ctrl_ok")},
+	{STAT_INFO(0x932, "rx_pause")},
+	{STAT_INFO(0x934, "rx_runt")},
+	{STAT_INFO(0x960, "rx_payload_octets_ok")},
+	{STAT_INFO(0x962, "rx_frame_octets_ok")},
+};
+
+static void edev25g_get_strings(struct net_device *netdev, u32 stringset, u8 *s)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	unsigned int i;
+
+	if (stringset != ETH_SS_STATS || edev->lw_mac)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(stats_25g); i++, s += ETH_GSTRING_LEN)
+		memcpy(s, stats_25g[i].string, ETH_GSTRING_LEN);
+}
+
+static int edev25g_get_sset_count(struct net_device *netdev, int stringset)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+
+	if (stringset != ETH_SS_STATS || edev->lw_mac)
+		return -EOPNOTSUPP;
+
+	return (int)ARRAY_SIZE(stats_25g);
+}
+
+static void edev25g_get_stats(struct net_device *netdev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	unsigned int i;
+
+	if (edev->lw_mac || !netif_running(netdev))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(stats_25g); i++)
+		data[i] = read_mac_stats(edev->mac, stats_25g[i].addr);
+}
+
+static int edev25g_get_link_ksettings(struct net_device *netdev,
+				      struct ethtool_link_ksettings *cmd)
+{
+	if (!netdev->phydev)
+		return -ENODEV;
+
+	phy_ethtool_ksettings_get(netdev->phydev, cmd);
+
+	return 0;
+}
+
+static int edev25g_pause_init(struct net_device *netdev)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+
+	return eth_com_write_reg(edev->mac, TX_FLOW_CTRL_SEL,
+				 TX_FLOW_CTRL_SEL_PAUSE);
+}
+
+static void edev25g_get_pauseparam(struct net_device *netdev,
+				   struct ethtool_pauseparam *pause)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	u32 data;
+	int ret;
+
+	pause->autoneg = 0;
+	pause->rx_pause = 0;
+
+	ret = eth_com_read_reg(edev->mac, TX_FLOW_CTRL_EN, &data);
+	if (ret) {
+		pause->tx_pause = 0;
+		return;
+	}
+
+	pause->tx_pause = (data & TX_FLOW_CTRL_EN_PAUSE) ? 0x1 : 0;
+}
+
+static int edev25g_set_pauseparam(struct net_device *netdev,
+				  struct ethtool_pauseparam *pause)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	bool enable = pause->tx_pause;
+
+	if (pause->autoneg || pause->rx_pause)
+		return -EOPNOTSUPP;
+
+	return eth_com_write_reg(edev->mac, TX_FLOW_CTRL_EN,
+				 enable ? TX_FLOW_CTRL_EN_PAUSE : 0);
+}
+
+static const struct ethtool_ops edev25g_ethtool_ops = {
+	.get_link = ethtool_op_get_link,
+	.get_strings = edev25g_get_strings,
+	.get_sset_count = edev25g_get_sset_count,
+	.get_ethtool_stats = edev25g_get_stats,
+	.get_link_ksettings = edev25g_get_link_ksettings,
+	.get_pauseparam = edev25g_get_pauseparam,
+	.set_pauseparam = edev25g_set_pauseparam,
+};
+
+/* netdev ops */
+static int edev25g_netdev_open(struct net_device *netdev)
+{
+	struct n3000_net_priv *priv = netdev_priv(netdev);
+	struct eth_dev *edev = priv->edev;
+	int ret;
+
+	ret = edev25g40g_reset(edev, false);
+	if (ret)
+		return ret;
+
+	if (netdev->phydev)
+		phy_start(netdev->phydev);
+
+	return 0;
+}
+
+static int edev25g_netdev_stop(struct net_device *netdev)
+{
+	struct n3000_net_priv *priv = netdev_priv(netdev);
+	struct eth_dev *edev = priv->edev;
+	int ret;
+
+	ret = edev25g40g_reset(edev, true);
+	if (ret)
+		return ret;
+
+	if (netdev->phydev)
+		phy_stop(netdev->phydev);
+
+	return 0;
+}
+
+static int edev25g_mtu_init(struct net_device *netdev)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	struct eth_com *mac = edev->mac;
+	u32 tx = 0, rx = 0, mtu;
+	int ret;
+
+	ret = eth_com_read_reg(mac, MAX_TX_SIZE_CONFIG, &tx);
+	if (ret)
+		return ret;
+
+	ret = eth_com_read_reg(mac, MAX_RX_SIZE_CONFIG, &rx);
+	if (ret)
+		return ret;
+
+	mtu = min(min(tx, rx), netdev->max_mtu);
+
+	ret = eth_com_write_reg(mac, MAX_TX_SIZE_CONFIG, rx);
+	if (ret)
+		return ret;
+
+	ret = eth_com_write_reg(mac, MAX_RX_SIZE_CONFIG, tx);
+	if (ret)
+		return ret;
+
+	netdev->mtu = mtu;
+
+	return 0;
+}
+
+static int edev25g_change_mtu(struct net_device *netdev, int new_mtu)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	struct eth_com *mac = edev->mac;
+	int ret;
+
+	ret = eth_com_write_reg(mac, MAX_TX_SIZE_CONFIG, new_mtu);
+	if (ret)
+		return ret;
+
+	ret = eth_com_write_reg(mac, MAX_RX_SIZE_CONFIG, new_mtu);
+	if (ret)
+		return ret;
+
+	netdev->mtu = new_mtu;
+
+	return 0;
+}
+
+static int edev25g_set_loopback(struct net_device *netdev, bool en)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+
+	return eth_com_write_reg(edev->mac, PHY_PMA_SLOOP, en);
+}
+
+static int edev25g_set_features(struct net_device *netdev,
+				netdev_features_t features)
+{
+	netdev_features_t changed = netdev->features ^ features;
+
+	if (changed & NETIF_F_LOOPBACK)
+		return edev25g_set_loopback(netdev,
+					    !!(features & NETIF_F_LOOPBACK));
+
+	return 0;
+}
+
+static const struct net_device_ops edev25g_netdev_ops = {
+	.ndo_open = edev25g_netdev_open,
+	.ndo_stop = edev25g_netdev_stop,
+	.ndo_change_mtu = edev25g_change_mtu,
+	.ndo_set_features = edev25g_set_features,
+	.ndo_start_xmit = n3000_dummy_netdev_xmit,
+};
+
+static void edev25g_adjust_link(struct net_device *netdev)
+{}
+
+static int edev25g_netdev_init(struct net_device *netdev)
+{
+	int ret;
+
+	ret = edev25g_pause_init(netdev);
+	if (ret)
+		return ret;
+
+	ret = edev25g_mtu_init(netdev);
+	if (ret)
+		return ret;
+
+	return edev25g_set_loopback(netdev,
+				    !!(netdev->features & NETIF_F_LOOPBACK));
+}
+
+static int dfl_eth_dev_25g_init(struct eth_dev *edev)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	struct device *dev = edev->dev;
+	struct phy_device *phydev;
+	struct net_device *netdev;
+	int ret;
+
+	netdev = n3000_netdev_create(edev);
+	if (!netdev)
+		return -ENOMEM;
+
+	netdev->hw_features |= NETIF_F_LOOPBACK;
+	netdev->netdev_ops = &edev25g_netdev_ops;
+	netdev->ethtool_ops = &edev25g_ethtool_ops;
+
+	phydev = phy_connect(netdev, edev->phy_id, edev25g_adjust_link,
+			     PHY_INTERFACE_MODE_NA);
+	if (IS_ERR(phydev)) {
+		dev_err(dev, "PHY connection failed\n");
+		ret = PTR_ERR(phydev);
+		goto err_free_netdev;
+	}
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
+	linkmode_and(phydev->supported, phydev->supported, mask);
+	linkmode_copy(phydev->advertising, phydev->supported);
+
+	phy_attached_info(phydev);
+
+	ret = edev25g_netdev_init(netdev);
+	if (ret) {
+		dev_err(dev, "fail to init netdev %s\n", netdev->name);
+		goto err_phy_disconnect;
+	}
+
+	netdev->sysfs_groups[0] = &edev25g_attr_group;
+
+	netif_carrier_off(netdev);
+	ret = register_netdev(netdev);
+	if (ret) {
+		dev_err(dev, "fail to register netdev %s\n", netdev->name);
+		goto err_phy_disconnect;
+	}
+
+	edev->netdev = netdev;
+
+	return 0;
+
+err_phy_disconnect:
+	if (netdev->phydev)
+		phy_disconnect(phydev);
+err_free_netdev:
+	free_netdev(netdev);
+
+	return ret;
+}
+
+static void dfl_eth_dev_25g_remove(struct eth_dev *edev)
+{
+	struct net_device *netdev = edev->netdev;
+
+	if (netdev->phydev)
+		phy_disconnect(netdev->phydev);
+
+	unregister_netdev(netdev);
+}
+
+struct eth_dev_ops dfl_eth_dev_25g_ops = {
+	.lineside_init = dfl_eth_dev_25g_init,
+	.lineside_remove = dfl_eth_dev_25g_remove,
+	.reset = edev25g40g_reset,
+};
+
+struct eth_dev_ops dfl_eth_dev_40g_ops = {
+	.reset = edev25g40g_reset,
+};
diff --git a/drivers/net/ethernet/intel/dfl-eth-group-main.c b/drivers/net/ethernet/intel/dfl-eth-group-main.c
new file mode 100644
index 0000000..a29b8b1
--- /dev/null
+++ b/drivers/net/ethernet/intel/dfl-eth-group-main.c
@@ -0,0 +1,632 @@
+// SPDX-License-Identifier: GPL-2.0
+/* DFL device driver for Ether Group private feature on Intel PAC (Programmable
+ * Acceleration Card) N3000
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Wu Hao <hao.wu@intel.com>
+ *   Xu Yilun <yilun.xu@intel.com>
+ */
+#include <linux/bitfield.h>
+#include <linux/dfl.h>
+#include <linux/errno.h>
+#include <linux/ethtool.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+#include "dfl-eth-group.h"
+
+struct dfl_eth_group {
+	char name[32];
+	struct device *dev;
+	void __iomem *base;
+	/* lock to protect register access of the ether group */
+	struct mutex reg_lock;
+	struct dfl_device *dfl_dev;
+	unsigned int config;
+	unsigned int direction;
+	unsigned int group_id;
+	unsigned int speed;
+	unsigned int lw_mac;
+	unsigned int num_edevs;
+	struct eth_dev *edevs;
+	struct eth_dev_ops *ops;
+};
+
+u64 read_mac_stats(struct eth_com *ecom, unsigned int addr)
+{
+	u32 data_l, data_h;
+
+	if (eth_com_read_reg(ecom, addr, &data_l) ||
+	    eth_com_read_reg(ecom, addr + 1, &data_h))
+		return 0xffffffffffffffffULL;
+
+	return data_l + ((u64)data_h << 32);
+}
+
+netdev_tx_t n3000_dummy_netdev_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	kfree_skb(skb);
+	net_warn_ratelimited("%s(): Dropping skb.\n", __func__);
+	return NETDEV_TX_OK;
+}
+
+static void n3000_netdev_setup(struct net_device *netdev)
+{
+	netdev->features = 0;
+	netdev->hard_header_len = 0;
+	netdev->priv_flags |= IFF_NO_QUEUE;
+	netdev->needs_free_netdev = true;
+	netdev->min_mtu = 0;
+	netdev->max_mtu = ETH_MAX_MTU;
+}
+
+struct net_device *n3000_netdev_create(struct eth_dev *edev)
+{
+	struct dfl_eth_group *egroup = edev->egroup;
+	struct n3000_net_priv *priv;
+	struct net_device *netdev;
+	char name[IFNAMSIZ];
+
+	/* The name of n3000 network device is using this format "npacAgBlC"
+	 *
+	 * A is the unique ethdev index
+	 * B is the group id of this ETH Group.
+	 * C is the PHY/MAC link index for Line side ethernet group.
+	 */
+	snprintf(name, IFNAMSIZ, "npac%%dg%ul%u",
+		 egroup->group_id, edev->index);
+
+	netdev = alloc_netdev(sizeof(*priv), name, NET_NAME_UNKNOWN,
+			      n3000_netdev_setup);
+	if (!netdev)
+		return NULL;
+
+	priv = netdev_priv(netdev);
+	priv->edev = edev;
+	SET_NETDEV_DEV(netdev, egroup->dev);
+
+	return netdev;
+}
+
+enum n3000_eth_cfg {
+	ETH_CONFIG_8x10G,
+	ETH_CONFIG_4x25G,
+	ETH_CONFIG_2x1x25G,
+	ETH_CONFIG_4x25G_2x25G,
+	ETH_CONFIG_2x2x25G,
+	ETH_CONFIG_MAX
+};
+
+#define N3000_EDEV_MAX 8
+
+static int phy_addr_table[ETH_CONFIG_MAX][N3000_EDEV_MAX] = {
+	/* 8x10G configuration
+	 *
+	 *    [retimer_dev]   <------->   [eth_dev]
+	 *	  0			   0
+	 *	  1			   1
+	 *	  2			   2
+	 *	  3			   3
+	 *	  4			   4
+	 *	  5			   5
+	 *	  6			   6
+	 *	  7			   7
+	 */
+	[ETH_CONFIG_8x10G] = {0, 1, 2, 3, 4, 5, 6, 7},
+
+	/* 4x25G and 4x25G_2x25G configuration
+	 *
+	 *    [retimer_dev]   <------->   [eth_dev]
+	 *	  0			   0
+	 *	  1			   1
+	 *	  2			   2
+	 *	  3			   3
+	 *	  4
+	 *	  5
+	 *	  6
+	 *	  7
+	 */
+	[ETH_CONFIG_4x25G] = {0, 1, 2, 3, -1, -1, -1, -1},
+	[ETH_CONFIG_4x25G_2x25G] = {0, 1, 2, 3, -1, -1, -1, -1},
+
+	/* 2x1x25G configuration
+	 *
+	 *    [retimer_dev]   <------->   [eth_dev]
+	 *        0                      0
+	 *        1
+	 *        2
+	 *        3
+	 *        4                      1
+	 *        5
+	 *        6
+	 *        7
+	 */
+	[ETH_CONFIG_2x1x25G] = {0, 4, -1, -1, -1, -1, -1, -1},
+
+	/* 2x2x25G configuration
+	 *
+	 *    [retimer_dev]   <------->   [eth_dev]
+	 *	  0			   0
+	 *	  1			   1
+	 *	  2
+	 *	  3
+	 *	  4			   2
+	 *	  5			   3
+	 *	  6
+	 *	  7
+	 */
+	[ETH_CONFIG_2x2x25G] = {0, 1, 4, 5, -1, -1, -1, -1},
+};
+
+#define eth_group_for_each_dev(edev, egp) \
+	for ((edev) = (egp)->edevs; (edev) < (egp)->edevs + (egp)->num_edevs; \
+	     (edev)++)
+
+#define eth_group_reverse_each_dev(edev, egp) \
+	for ((edev)--; (edev) >= (egp)->edevs; (edev)--)
+
+static struct mii_bus *eth_group_get_phy_bus(struct dfl_eth_group *egroup)
+{
+	char mii_name[MII_BUS_ID_SIZE];
+	struct device *base_dev;
+	struct mii_bus *bus;
+
+	base_dev = dfl_dev_get_base_dev(egroup->dfl_dev);
+	if (!base_dev)
+		return ERR_PTR(-ENODEV);
+
+	snprintf(mii_name, MII_BUS_ID_SIZE, DFL_ETH_MII_ID_FMT,
+		 dev_name(base_dev));
+
+	bus = mdio_find_bus(mii_name);
+	if (!bus)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return bus;
+}
+
+static int eth_dev_get_phy_id(struct eth_dev *edev, struct mii_bus *bus)
+{
+	struct dfl_eth_group *egroup = edev->egroup;
+	struct phy_device *phydev;
+	int phyaddr;
+
+	phyaddr = phy_addr_table[egroup->config][edev->index];
+	if (phyaddr < 0)
+		return -ENODEV;
+
+	phydev = mdiobus_get_phy(bus, phyaddr);
+	if (!phydev) {
+		dev_err(egroup->dev, "fail to get phydev\n");
+		return -EPROBE_DEFER;
+	}
+
+	strncpy(edev->phy_id, phydev_name(phydev), MII_BUS_ID_SIZE + 3);
+	edev->phy_id[MII_BUS_ID_SIZE + 2] = '\0';
+
+	return 0;
+}
+
+static int init_lineside_eth_devs(struct dfl_eth_group *egroup,
+				  struct mii_bus *phy_bus)
+{
+	struct eth_dev *edev;
+	int ret = 0;
+
+	if (!egroup->ops->lineside_init)
+		return -ENODEV;
+
+	eth_group_for_each_dev(edev, egroup) {
+		ret = eth_dev_get_phy_id(edev, phy_bus);
+		if (ret)
+			break;
+
+		ret = egroup->ops->lineside_init(edev);
+		if (ret)
+			break;
+	}
+
+	if (!ret)
+		return 0;
+
+	dev_err(egroup->dev, "failed to init lineside edev %d", edev->index);
+
+	if (egroup->ops->lineside_remove)
+		eth_group_reverse_each_dev(edev, egroup)
+			egroup->ops->lineside_remove(edev);
+
+	return ret;
+}
+
+static void remove_lineside_eth_devs(struct dfl_eth_group *egroup)
+{
+	struct eth_dev *edev;
+
+	if (!egroup->ops->lineside_remove)
+		return;
+
+	eth_group_for_each_dev(edev, egroup)
+		egroup->ops->lineside_remove(edev);
+}
+
+#define ETH_GROUP_INFO		0x8
+#define LIGHT_WEIGHT_MAC	BIT_ULL(25)
+#define INFO_DIRECTION		BIT_ULL(24)
+#define INFO_SPEED		GENMASK_ULL(23, 16)
+#define INFO_PHY_NUM		GENMASK_ULL(15, 8)
+#define INFO_GROUP_ID		GENMASK_ULL(7, 0)
+
+#define ETH_GROUP_CTRL		0x10
+#define CTRL_CMD		GENMASK_ULL(63, 62)
+#define CMD_NOP			0
+#define CMD_RD			1
+#define CMD_WR			2
+#define CTRL_DEV_SELECT		GENMASK_ULL(53, 49)
+#define CTRL_FEAT_SELECT	BIT_ULL(48)
+#define SELECT_IP		0
+#define SELECT_FEAT		1
+#define CTRL_ADDR		GENMASK_ULL(47, 32)
+#define CTRL_WR_DATA		GENMASK_ULL(31, 0)
+
+#define ETH_GROUP_STAT		0x18
+#define STAT_RW_VAL		BIT_ULL(32)
+#define STAT_RD_DATA		GENMASK_ULL(31, 0)
+
+enum ecom_type {
+	ETH_GROUP_PHY	= 1,
+	ETH_GROUP_MAC,
+	ETH_GROUP_ETHER
+};
+
+struct eth_com {
+	struct dfl_eth_group *egroup;
+	unsigned int type;
+	u8 select;
+};
+
+static const char *eth_com_type_string(enum ecom_type type)
+{
+	switch (type) {
+	case ETH_GROUP_PHY:
+		return "phy";
+	case ETH_GROUP_MAC:
+		return "mac";
+	case ETH_GROUP_ETHER:
+		return "ethernet wrapper";
+	default:
+		return "unknown";
+	}
+}
+
+#define eth_com_base(com)	((com)->egroup->base)
+#define eth_com_dev(com)	((com)->egroup->dev)
+
+#define RW_VAL_INVL		1 /* us */
+#define RW_VAL_POLL_TIMEOUT	10 /* us */
+
+static int __do_eth_com_write_reg(struct eth_com *ecom, bool add_feature,
+				  u16 addr, u32 data)
+{
+	void __iomem *base = eth_com_base(ecom);
+	struct device *dev = eth_com_dev(ecom);
+	u64 v = 0;
+
+	dev_dbg(dev, "%s [%s] select 0x%x add_feat %d addr 0x%x data 0x%x\n",
+		__func__, eth_com_type_string(ecom->type),
+		ecom->select, add_feature, addr, data);
+
+	/* only PHY has additional feature registers */
+	if (add_feature && ecom->type != ETH_GROUP_PHY)
+		return -EINVAL;
+
+	v |= FIELD_PREP(CTRL_CMD, CMD_WR);
+	v |= FIELD_PREP(CTRL_DEV_SELECT, ecom->select);
+	v |= FIELD_PREP(CTRL_ADDR, addr);
+	v |= FIELD_PREP(CTRL_WR_DATA, data);
+	v |= FIELD_PREP(CTRL_FEAT_SELECT, !!add_feature);
+
+	writeq(v, base + ETH_GROUP_CTRL);
+
+	if (readq_poll_timeout(base + ETH_GROUP_STAT, v, v & STAT_RW_VAL,
+			       RW_VAL_INVL, RW_VAL_POLL_TIMEOUT))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int __do_eth_com_read_reg(struct eth_com *ecom, bool add_feature,
+				 u16 addr, u32 *data)
+{
+	void __iomem *base = eth_com_base(ecom);
+	struct device *dev = eth_com_dev(ecom);
+	u64 v = 0;
+
+	dev_dbg(dev, "%s [%s] select %x add_feat %d addr %x\n",
+		__func__, eth_com_type_string(ecom->type),
+		ecom->select, add_feature, addr);
+
+	/* only PHY has additional feature registers */
+	if (add_feature && ecom->type != ETH_GROUP_PHY)
+		return -EINVAL;
+
+	v |= FIELD_PREP(CTRL_CMD, CMD_RD);
+	v |= FIELD_PREP(CTRL_DEV_SELECT, ecom->select);
+	v |= FIELD_PREP(CTRL_ADDR, addr);
+	v |= FIELD_PREP(CTRL_FEAT_SELECT, !!add_feature);
+
+	writeq(v, base + ETH_GROUP_CTRL);
+
+	if (readq_poll_timeout(base + ETH_GROUP_STAT, v, v & STAT_RW_VAL,
+			       RW_VAL_INVL, RW_VAL_POLL_TIMEOUT))
+		return -ETIMEDOUT;
+
+	*data = FIELD_GET(STAT_RD_DATA, v);
+
+	return 0;
+}
+
+int do_eth_com_write_reg(struct eth_com *ecom, bool add_feature,
+			 u16 addr, u32 data)
+{
+	int ret;
+
+	mutex_lock(&ecom->egroup->reg_lock);
+	ret = __do_eth_com_write_reg(ecom, add_feature, addr, data);
+	mutex_unlock(&ecom->egroup->reg_lock);
+	return ret;
+}
+
+int do_eth_com_read_reg(struct eth_com *ecom, bool add_feature,
+			u16 addr, u32 *data)
+{
+	int ret;
+
+	mutex_lock(&ecom->egroup->reg_lock);
+	ret = __do_eth_com_read_reg(ecom, add_feature, addr, data);
+	mutex_unlock(&ecom->egroup->reg_lock);
+	return ret;
+}
+
+static struct eth_com *
+eth_com_create(struct dfl_eth_group *egroup, enum ecom_type type,
+	       unsigned int link_idx)
+{
+	struct eth_com *ecom;
+
+	ecom = devm_kzalloc(egroup->dev, sizeof(*ecom), GFP_KERNEL);
+	if (!ecom)
+		return ERR_PTR(-ENOMEM);
+
+	ecom->egroup = egroup;
+	ecom->type = type;
+
+	if (type == ETH_GROUP_PHY)
+		ecom->select = link_idx * 2 + 2;
+	else if (type == ETH_GROUP_MAC)
+		ecom->select = link_idx * 2 + 3;
+	else if (type == ETH_GROUP_ETHER)
+		ecom->select = 0;
+
+	return ecom;
+}
+
+static int init_eth_dev(struct eth_dev *edev, struct dfl_eth_group *egroup,
+			unsigned int link_idx)
+{
+	edev->egroup = egroup;
+	edev->dev = egroup->dev;
+	edev->index = link_idx;
+	edev->lw_mac = !!egroup->lw_mac;
+	edev->phy = eth_com_create(egroup, ETH_GROUP_PHY, link_idx);
+	if (IS_ERR(edev->phy))
+		return PTR_ERR(edev->phy);
+
+	edev->mac = eth_com_create(egroup, ETH_GROUP_MAC, link_idx);
+	if (IS_ERR(edev->mac))
+		return PTR_ERR(edev->mac);
+
+	return 0;
+}
+
+static int eth_devs_init(struct dfl_eth_group *egroup)
+{
+	int ret, i;
+
+	egroup->edevs = devm_kcalloc(egroup->dev, egroup->num_edevs,
+				     sizeof(*egroup->edevs), GFP_KERNEL);
+	if (!egroup->edevs)
+		return -ENOMEM;
+
+	for (i = 0; i < egroup->num_edevs; i++) {
+		ret = init_eth_dev(&egroup->edevs[i], egroup, i);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int eth_group_setup(struct dfl_eth_group *egroup)
+{
+	int net_cfg, ret;
+	u64 v;
+
+	/* read parameters of this ethernet components group */
+	v = readq(egroup->base + ETH_GROUP_INFO);
+
+	egroup->direction = FIELD_GET(INFO_DIRECTION, v);
+	egroup->speed = FIELD_GET(INFO_SPEED, v);
+	egroup->num_edevs = FIELD_GET(INFO_PHY_NUM, v);
+	egroup->group_id = FIELD_GET(INFO_GROUP_ID, v);
+	egroup->lw_mac = FIELD_GET(LIGHT_WEIGHT_MAC, v);
+
+	net_cfg = dfl_dev_get_vendor_net_cfg(egroup->dfl_dev);
+	if (net_cfg < 0)
+		return -EINVAL;
+
+	egroup->config = (unsigned int)net_cfg;
+
+	ret = eth_devs_init(egroup);
+	if (ret)
+		return ret;
+
+	switch (egroup->speed) {
+	case 25:
+		egroup->ops = &dfl_eth_dev_25g_ops;
+		break;
+	case 40:
+		egroup->ops = &dfl_eth_dev_40g_ops;
+		break;
+	}
+
+	mutex_init(&egroup->reg_lock);
+
+	return 0;
+}
+
+static void eth_group_destroy(struct dfl_eth_group *egroup)
+{
+	mutex_destroy(&egroup->reg_lock);
+}
+
+static void eth_group_devs_disable(struct dfl_eth_group *egroup)
+{
+	struct eth_dev *edev;
+
+	eth_group_for_each_dev(edev, egroup)
+		egroup->ops->reset(edev, true);
+}
+
+static int eth_group_devs_enable(struct dfl_eth_group *egroup)
+{
+	struct eth_dev *edev;
+	int ret;
+
+	eth_group_for_each_dev(edev, egroup) {
+		ret = egroup->ops->reset(edev, false);
+		if (ret) {
+			dev_err(egroup->dev, "fail to enable edev%d\n",
+				edev->index);
+			eth_group_devs_disable(egroup);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int dfl_eth_group_line_side_init(struct dfl_eth_group *egroup)
+{
+	struct mii_bus *phy_bus;
+	int ret;
+
+	if (!egroup->ops || !egroup->ops->reset ||
+	    !egroup->ops->lineside_init || !egroup->ops->lineside_remove)
+		return -EINVAL;
+
+	eth_group_devs_disable(egroup);
+
+	phy_bus = eth_group_get_phy_bus(egroup);
+	if (IS_ERR(phy_bus))
+		return PTR_ERR(phy_bus);
+
+	ret = init_lineside_eth_devs(egroup, phy_bus);
+	put_device(&phy_bus->dev);
+
+	return ret;
+}
+
+static void dfl_eth_group_line_side_uinit(struct dfl_eth_group *egroup)
+{
+	remove_lineside_eth_devs(egroup);
+}
+
+static int dfl_eth_group_host_side_init(struct dfl_eth_group *egroup)
+{
+	if (!egroup->ops || !egroup->ops->reset)
+		return -EINVAL;
+
+	return eth_group_devs_enable(egroup);
+}
+
+static int dfl_eth_group_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct dfl_eth_group *egroup;
+	int ret;
+
+	egroup = devm_kzalloc(dev, sizeof(*egroup), GFP_KERNEL);
+	if (!egroup)
+		return -ENOMEM;
+
+	dev_set_drvdata(&dfl_dev->dev, egroup);
+
+	egroup->dev = dev;
+	egroup->dfl_dev = dfl_dev;
+
+	egroup->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+	if (IS_ERR(egroup->base)) {
+		dev_err(dev, "get mem resource fail!\n");
+		return PTR_ERR(egroup->base);
+	}
+
+	ret = eth_group_setup(egroup);
+	if (ret)
+		return ret;
+
+	if (egroup->direction == 1)
+		ret = dfl_eth_group_line_side_init(egroup);
+	else
+		ret = dfl_eth_group_host_side_init(egroup);
+
+	if (!ret)
+		return 0;
+
+	eth_group_destroy(egroup);
+
+	return ret;
+}
+
+static void dfl_eth_group_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_eth_group *egroup = dev_get_drvdata(&dfl_dev->dev);
+
+	if (egroup->direction == 1)
+		dfl_eth_group_line_side_uinit(egroup);
+
+	eth_group_devs_disable(egroup);
+	eth_group_destroy(egroup);
+}
+
+#define FME_FEATURE_ID_ETH_GROUP	0x10
+
+static const struct dfl_device_id dfl_eth_group_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
+	{ }
+};
+
+static struct dfl_driver dfl_eth_group_driver = {
+	.drv	= {
+		.name       = "dfl-eth-group",
+	},
+	.id_table = dfl_eth_group_ids,
+	.probe   = dfl_eth_group_probe,
+	.remove  = dfl_eth_group_remove,
+};
+
+module_dfl_driver(dfl_eth_group_driver);
+
+MODULE_DEVICE_TABLE(dfl, dfl_eth_group_ids);
+MODULE_DESCRIPTION("DFL ether group driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/net/ethernet/intel/dfl-eth-group.h b/drivers/net/ethernet/intel/dfl-eth-group.h
new file mode 100644
index 0000000..2e90f86
--- /dev/null
+++ b/drivers/net/ethernet/intel/dfl-eth-group.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Internal header file for FPGA DFL Ether Group Driver
+ *
+ * Copyright (C) 2020 Intel Corporation. All rights reserved.
+ */
+
+#ifndef __DFL_ETH_GROUP_H__
+#define __DFL_ETH_GROUP_H__
+
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+#include <linux/rtnetlink.h>
+
+/* Used when trying to find a virtual mii bus on a specific dfl device.
+ * dev_name(dfl base device)-mii
+ */
+#define DFL_ETH_MII_ID_FMT "%s-mii"
+
+struct eth_dev {
+	struct dfl_eth_group *egroup;
+	struct device *dev;
+	int index;
+	bool lw_mac;
+	struct eth_com *phy;
+	struct eth_com *mac;
+	struct net_device *netdev;
+
+	char phy_id[MII_BUS_ID_SIZE + 3];
+};
+
+struct eth_dev_ops {
+	int (*lineside_init)(struct eth_dev *edev);
+	void (*lineside_remove)(struct eth_dev *edev);
+	int (*reset)(struct eth_dev *edev, bool en);
+};
+
+struct n3000_net_priv {
+	struct eth_dev *edev;
+};
+
+static inline struct eth_dev *net_device_to_eth_dev(struct net_device *netdev)
+{
+	struct n3000_net_priv *priv = netdev_priv(netdev);
+
+	return priv->edev;
+}
+
+struct stat_info {
+	unsigned int addr;
+	char string[ETH_GSTRING_LEN];
+};
+
+#define STAT_INFO(_addr, _string) \
+	.addr = _addr, .string = _string,
+
+int do_eth_com_write_reg(struct eth_com *ecom, bool add_feature,
+			 u16 addr, u32 data);
+int do_eth_com_read_reg(struct eth_com *ecom, bool add_feature,
+			u16 addr, u32 *data);
+
+#define eth_com_write_reg(ecom, addr, data)	\
+	do_eth_com_write_reg(ecom, false, addr, data)
+
+#define eth_com_read_reg(ecom, addr, data)	\
+	do_eth_com_read_reg(ecom, false, addr, data)
+
+#define eth_com_add_feat_write_reg(ecom, addr, data)	\
+	do_eth_com_write_reg(ecom, true, addr, data)
+
+#define eth_com_add_feat_read_reg(ecom, addr, data)	\
+	do_eth_com_read_reg(ecom, true, addr, data)
+
+u64 read_mac_stats(struct eth_com *ecom, unsigned int addr);
+
+struct net_device *n3000_netdev_create(struct eth_dev *edev);
+netdev_tx_t n3000_dummy_netdev_xmit(struct sk_buff *skb,
+				    struct net_device *dev);
+
+extern struct eth_dev_ops dfl_eth_dev_25g_ops;
+extern struct eth_dev_ops dfl_eth_dev_40g_ops;
+
+#endif /* __DFL_ETH_GROUP_H__ */
diff --git a/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
index c7b0558..3f686d2 100644
--- a/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
+++ b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
@@ -10,6 +10,8 @@
 #include <linux/phy.h>
 #include <linux/platform_device.h>
 
+#include "dfl-eth-group.h"
+
 #define NUM_CHIP	2
 #define MAX_LINK	4
 
@@ -148,7 +150,7 @@ static int m10bmc_retimer_mii_bus_init(struct m10bmc_retimer *retimer)
 	bus->name = M10BMC_RETIMER_MII_NAME;
 	bus->read = m10bmc_retimer_read;
 	bus->write = m10bmc_retimer_write;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
+	snprintf(bus->id, MII_BUS_ID_SIZE, DFL_ETH_MII_ID_FMT,
 		 dev_name(retimer->base_dev));
 	bus->parent = retimer->dev;
 	bus->phy_mask = ~(BITS_MASK(retimer->num_devs));
-- 
2.7.4

[RFC PATCH 6/6] ethernet: dfl-eth-group: add support for the 10G configurations

[Xu Yilun]

This patch adds 10G configurations support for dfl ether group private
feature.

10G configurations have different PHY & MAC IP blocks from 25G, so a
different set of HW operations is implemented, but the software arch is
quite similar with 25G.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/net/ethernet/intel/Makefile             |   2 +-
 drivers/net/ethernet/intel/dfl-eth-group-10g.c  | 544 ++++++++++++++++++++++++
 drivers/net/ethernet/intel/dfl-eth-group-main.c |   3 +
 drivers/net/ethernet/intel/dfl-eth-group.h      |   1 +
 4 files changed, 549 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-10g.c

diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
index 1624c26..be097656 100644
--- a/drivers/net/ethernet/intel/Makefile
+++ b/drivers/net/ethernet/intel/Makefile
@@ -17,6 +17,6 @@ obj-$(CONFIG_IAVF) += iavf/
 obj-$(CONFIG_FM10K) += fm10k/
 obj-$(CONFIG_ICE) += ice/
 
-dfl-eth-group-objs := dfl-eth-group-main.o dfl-eth-group-25g.o
+dfl-eth-group-objs := dfl-eth-group-main.o dfl-eth-group-10g.o dfl-eth-group-25g.o
 obj-$(CONFIG_FPGA_DFL_ETH_GROUP) += dfl-eth-group.o
 obj-$(CONFIG_INTEL_M10_BMC_RETIMER) += intel-m10-bmc-retimer.o
diff --git a/drivers/net/ethernet/intel/dfl-eth-group-10g.c b/drivers/net/ethernet/intel/dfl-eth-group-10g.c
new file mode 100644
index 0000000..36f9dfc
--- /dev/null
+++ b/drivers/net/ethernet/intel/dfl-eth-group-10g.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for 10G Ether Group private feature on Intel PAC (Programmable
+ * Acceleration Card) N3000
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Wu Hao <hao.wu@intel.com>
+ *   Xu Yilun <yilun.xu@intel.com>
+ */
+#include <linux/netdevice.h>
+
+#include "dfl-eth-group.h"
+
+/* 10G PHY Register */
+#define PHY_LOOPBACK		0x2e1
+#define PHY_LOOPBACK_SERIAL	BIT(0)
+
+/* 10G MAC Register */
+#define TX_FRAME_MAXLENGTH	0x2c
+#define TX_PAUSE_FRAME_QUANTA	0x42
+#define TX_PAUSE_FRAME_HOLDOFF	0x43
+#define TX_PAUSE_FRAME_EN	0x44
+#define TX_PAUSE_FRAME_EN_CFG	BIT(0)
+#define RX_FRAME_MAXLENGTH	0xae
+
+/* Additional Feature Register */
+#define ADD_PHY_CTRL		0x0
+#define PHY_RESET		BIT(0)
+
+static int edev10g_reset(struct eth_dev *edev, bool en)
+{
+	struct eth_com *phy = edev->phy;
+	struct device *dev = edev->dev;
+	u32 val;
+	int ret;
+
+	/* 10G eth group supports PHY reset. It uses external reset. */
+	ret = eth_com_add_feat_read_reg(phy, ADD_PHY_CTRL, &val);
+	if (ret) {
+		dev_err(dev, "fail to read ADD_PHY_CTRL reg: %d\n", ret);
+		return ret;
+	}
+
+	/* return if PHY is already in expected state */
+	if ((val & PHY_RESET && en) || (!(val & PHY_RESET) && !en))
+		return 0;
+
+	if (en)
+		val |= PHY_RESET;
+	else
+		val &= ~PHY_RESET;
+
+	ret = eth_com_add_feat_write_reg(phy, ADD_PHY_CTRL, val);
+	if (ret)
+		dev_err(dev, "fail to write ADD_PHY_CTRL reg: %d\n", ret);
+
+	return ret;
+}
+
+static ssize_t tx_pause_frame_quanta_show(struct device *d,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
+	u32 data;
+	int ret;
+
+	ret = eth_com_read_reg(edev->mac, TX_PAUSE_FRAME_QUANTA, &data);
+
+	return ret ? : sprintf(buf, "0x%x\n", data);
+}
+
+static ssize_t tx_pause_frame_quanta_store(struct device *d,
+					   struct device_attribute *attr,
+					   const char *buf, size_t len)
+{
+	struct net_device *netdev = to_net_dev(d);
+	struct eth_dev *edev;
+	u32 data;
+	int ret;
+
+	if (kstrtou32(buf, 0, &data))
+		return -EINVAL;
+
+	edev = net_device_to_eth_dev(netdev);
+
+	rtnl_lock();
+
+	if (netif_running(netdev)) {
+		netdev_err(netdev, "must be stopped to change pause param\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = eth_com_write_reg(edev->mac, TX_PAUSE_FRAME_QUANTA, data);
+
+out:
+	rtnl_unlock();
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_RW(tx_pause_frame_quanta);
+
+static ssize_t tx_pause_frame_holdoff_show(struct device *d,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
+	u32 data;
+	int ret;
+
+	ret = eth_com_read_reg(edev->mac, TX_PAUSE_FRAME_HOLDOFF, &data);
+
+	return ret ? : sprintf(buf, "0x%x\n", data);
+}
+
+static ssize_t tx_pause_frame_holdoff_store(struct device *d,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	struct net_device *netdev = to_net_dev(d);
+	struct eth_dev *edev;
+	u32 data;
+	int ret;
+
+	if (kstrtou32(buf, 0, &data))
+		return -EINVAL;
+
+	edev = net_device_to_eth_dev(netdev);
+
+	rtnl_lock();
+
+	if (netif_running(netdev)) {
+		netdev_err(netdev, "must be stopped to change pause param\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = eth_com_write_reg(edev->mac, TX_PAUSE_FRAME_HOLDOFF, data);
+
+out:
+	rtnl_unlock();
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_RW(tx_pause_frame_holdoff);
+
+static struct attribute *edev10g_dev_attrs[] = {
+	&dev_attr_tx_pause_frame_quanta.attr,
+	&dev_attr_tx_pause_frame_holdoff.attr,
+	NULL
+};
+
+/* device attributes */
+static const struct attribute_group edev10g_attr_group = {
+	.attrs = edev10g_dev_attrs,
+};
+
+/* ethtool ops */
+static struct stat_info stats_10g[] = {
+	/* TX Statistics */
+	{STAT_INFO(0x142, "tx_frame_ok")},
+	{STAT_INFO(0x144, "tx_frame_err")},
+	{STAT_INFO(0x146, "tx_frame_crc_err")},
+	{STAT_INFO(0x148, "tx_octets_ok")},
+	{STAT_INFO(0x14a, "tx_pause_mac_ctrl_frames")},
+	{STAT_INFO(0x14c, "tx_if_err")},
+	{STAT_INFO(0x14e, "tx_unicast_frame_ok")},
+	{STAT_INFO(0x150, "tx_unicast_frame_err")},
+	{STAT_INFO(0x152, "tx_multicast_frame_ok")},
+	{STAT_INFO(0x154, "tx_multicast_frame_err")},
+	{STAT_INFO(0x156, "tx_broadcast_frame_ok")},
+	{STAT_INFO(0x158, "tx_broadcast_frame_err")},
+	{STAT_INFO(0x15a, "tx_ether_octets")},
+	{STAT_INFO(0x15c, "tx_ether_pkts")},
+	{STAT_INFO(0x15e, "tx_ether_undersize_pkts")},
+	{STAT_INFO(0x160, "tx_ether_oversize_pkts")},
+	{STAT_INFO(0x162, "tx_ether_pkts_64_octets")},
+	{STAT_INFO(0x164, "tx_ether_pkts_65_127_octets")},
+	{STAT_INFO(0x166, "tx_ether_pkts_128_255_octets")},
+	{STAT_INFO(0x168, "tx_ether_pkts_256_511_octets")},
+	{STAT_INFO(0x16a, "tx_ether_pkts_512_1023_octets")},
+	{STAT_INFO(0x16c, "tx_ether_pkts_1024_1518_octets")},
+	{STAT_INFO(0x16e, "tx_ether_pkts_1519_x_octets")},
+	{STAT_INFO(0x176, "tx_unicast_mac_ctrl_frames")},
+	{STAT_INFO(0x178, "tx_multicast_mac_ctrl_frames")},
+	{STAT_INFO(0x17a, "tx_broadcast_mac_ctrl_frames")},
+	{STAT_INFO(0x17c, "tx_pfc_mac_ctrl_frames")},
+
+	/* RX Statistics */
+	{STAT_INFO(0x1c2, "rx_frame_ok")},
+	{STAT_INFO(0x1c4, "rx_frame_err")},
+	{STAT_INFO(0x1c6, "rx_frame_crc_err")},
+	{STAT_INFO(0x1c8, "rx_octets_ok")},
+	{STAT_INFO(0x1ca, "rx_pause_mac_ctrl_frames")},
+	{STAT_INFO(0x1cc, "rx_if_err")},
+	{STAT_INFO(0x1ce, "rx_unicast_frame_ok")},
+	{STAT_INFO(0x1d0, "rx_unicast_frame_err")},
+	{STAT_INFO(0x1d2, "rx_multicast_frame_ok")},
+	{STAT_INFO(0x1d4, "rx_multicast_frame_err")},
+	{STAT_INFO(0x1d6, "rx_broadcast_frame_ok")},
+	{STAT_INFO(0x1d8, "rx_broadcast_frame_err")},
+	{STAT_INFO(0x1da, "rx_ether_octets")},
+	{STAT_INFO(0x1dc, "rx_ether_pkts")},
+	{STAT_INFO(0x1de, "rx_ether_undersize_pkts")},
+	{STAT_INFO(0x1e0, "rx_ether_oversize_pkts")},
+	{STAT_INFO(0x1e2, "rx_ether_pkts_64_octets")},
+	{STAT_INFO(0x1e4, "rx_ether_pkts_65_127_octets")},
+	{STAT_INFO(0x1e6, "rx_ether_pkts_128_255_octets")},
+	{STAT_INFO(0x1e8, "rx_ether_pkts_256_511_octets")},
+	{STAT_INFO(0x1ea, "rx_ether_pkts_512_1023_octets")},
+	{STAT_INFO(0x1ec, "rx_ether_pkts_1024_1518_octets")},
+	{STAT_INFO(0x1ee, "rx_ether_pkts_1519_x_octets")},
+	{STAT_INFO(0x1f0, "rx_ether_fragments")},
+	{STAT_INFO(0x1f2, "rx_ether_jabbers")},
+	{STAT_INFO(0x1f4, "rx_ether_crc_err")},
+	{STAT_INFO(0x1f6, "rx_unicast_mac_ctrl_frames")},
+	{STAT_INFO(0x1f8, "rx_multicast_mac_ctrl_frames")},
+	{STAT_INFO(0x1fa, "rx_broadcast_mac_ctrl_frames")},
+	{STAT_INFO(0x1fc, "rx_pfc_mac_ctrl_frames")},
+};
+
+static void edev10g_get_strings(struct net_device *netdev, u32 stringset, u8 *s)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	unsigned int i;
+
+	if (stringset != ETH_SS_STATS || edev->lw_mac)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(stats_10g); i++, s += ETH_GSTRING_LEN)
+		memcpy(s, stats_10g[i].string, ETH_GSTRING_LEN);
+}
+
+static int edev10g_get_sset_count(struct net_device *netdev, int stringset)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+
+	if (stringset != ETH_SS_STATS || edev->lw_mac)
+		return -EOPNOTSUPP;
+
+	return (int)ARRAY_SIZE(stats_10g);
+}
+
+static void edev10g_get_stats(struct net_device *netdev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	unsigned int i;
+
+	if (edev->lw_mac || !netif_running(netdev))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(stats_10g); i++)
+		data[i] = read_mac_stats(edev->mac, stats_10g[i].addr);
+}
+
+static int edev10g_get_link_ksettings(struct net_device *netdev,
+				      struct ethtool_link_ksettings *cmd)
+{
+	if (!netdev->phydev)
+		return -ENODEV;
+
+	phy_ethtool_ksettings_get(netdev->phydev, cmd);
+
+	return 0;
+}
+
+static void edev10g_get_pauseparam(struct net_device *netdev,
+				   struct ethtool_pauseparam *pause)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	u32 data;
+	int ret;
+
+	pause->autoneg = 0;
+	pause->rx_pause = 0;
+
+	ret = eth_com_read_reg(edev->mac, TX_PAUSE_FRAME_EN, &data);
+	if (ret) {
+		pause->tx_pause = 0;
+		return;
+	}
+
+	pause->tx_pause = (data & TX_PAUSE_FRAME_EN_CFG) ? 0x1 : 0;
+}
+
+static int edev10g_set_pauseparam(struct net_device *netdev,
+				  struct ethtool_pauseparam *pause)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	struct eth_com *mac = edev->mac;
+	bool enable = pause->tx_pause;
+	u32 data;
+	int ret;
+
+	if (pause->autoneg || pause->rx_pause)
+		return -EOPNOTSUPP;
+
+	if (netif_running(netdev)) {
+		netdev_err(netdev, "must be stopped to change pause param\n");
+		return -EBUSY;
+	}
+
+	ret = eth_com_read_reg(mac, TX_PAUSE_FRAME_EN, &data);
+	if (ret)
+		return ret;
+
+	if (enable)
+		data |= TX_PAUSE_FRAME_EN_CFG;
+	else
+		data &= ~TX_PAUSE_FRAME_EN_CFG;
+
+	return eth_com_write_reg(mac, TX_PAUSE_FRAME_EN, data);
+}
+
+static const struct ethtool_ops edev10g_ethtool_ops = {
+	.get_link = ethtool_op_get_link,
+	.get_strings = edev10g_get_strings,
+	.get_sset_count = edev10g_get_sset_count,
+	.get_ethtool_stats = edev10g_get_stats,
+	.get_link_ksettings = edev10g_get_link_ksettings,
+	.get_pauseparam = edev10g_get_pauseparam,
+	.set_pauseparam = edev10g_set_pauseparam,
+};
+
+/* netdev ops */
+static int edev10g_netdev_open(struct net_device *netdev)
+{
+	struct n3000_net_priv *priv = netdev_priv(netdev);
+	struct eth_dev *edev = priv->edev;
+	int ret;
+
+	ret = edev10g_reset(edev, false);
+	if (ret)
+		return ret;
+
+	if (netdev->phydev)
+		phy_start(netdev->phydev);
+
+	return 0;
+}
+
+static int edev10g_netdev_stop(struct net_device *netdev)
+{
+	struct n3000_net_priv *priv = netdev_priv(netdev);
+	struct eth_dev *edev = priv->edev;
+	int ret;
+
+	ret = edev10g_reset(edev, true);
+	if (ret)
+		return ret;
+
+	if (netdev->phydev)
+		phy_stop(netdev->phydev);
+
+	return 0;
+}
+
+static int edev10g_mtu_init(struct net_device *netdev)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	struct eth_com *mac = edev->mac;
+	u32 tx = 0, rx = 0, mtu;
+	int ret;
+
+	ret = eth_com_read_reg(mac, TX_FRAME_MAXLENGTH, &tx);
+	if (ret)
+		return ret;
+
+	ret = eth_com_read_reg(mac, RX_FRAME_MAXLENGTH, &rx);
+	if (ret)
+		return ret;
+
+	mtu = min(min(tx, rx), netdev->max_mtu);
+
+	ret = eth_com_write_reg(mac, TX_FRAME_MAXLENGTH, rx);
+	if (ret)
+		return ret;
+
+	ret = eth_com_write_reg(mac, RX_FRAME_MAXLENGTH, tx);
+	if (ret)
+		return ret;
+
+	netdev->mtu = mtu;
+
+	return 0;
+}
+
+static int edev10g_change_mtu(struct net_device *netdev, int new_mtu)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	struct eth_com *mac = edev->mac;
+	int ret;
+
+	if (netif_running(netdev)) {
+		netdev_err(netdev, "must be stopped to change mtu\n");
+		return -EBUSY;
+	}
+
+	ret = eth_com_write_reg(mac, TX_FRAME_MAXLENGTH, new_mtu);
+	if (ret)
+		return ret;
+
+	ret = eth_com_write_reg(mac, RX_FRAME_MAXLENGTH, new_mtu);
+	if (ret)
+		return ret;
+
+	netdev->mtu = new_mtu;
+
+	return 0;
+}
+
+static int edev10g_set_loopback(struct net_device *netdev, bool en)
+{
+	struct eth_dev *edev = net_device_to_eth_dev(netdev);
+	struct eth_com *phy = edev->phy;
+	u32 data;
+	int ret;
+
+	ret = eth_com_read_reg(phy, PHY_LOOPBACK, &data);
+	if (ret)
+		return ret;
+
+	if (en)
+		data |= PHY_LOOPBACK_SERIAL;
+	else
+		data &= ~PHY_LOOPBACK_SERIAL;
+
+	return eth_com_write_reg(phy, PHY_LOOPBACK, data);
+}
+
+static int edev10g_set_features(struct net_device *netdev,
+				netdev_features_t features)
+{
+	netdev_features_t changed = netdev->features ^ features;
+
+	if (changed & NETIF_F_LOOPBACK)
+		return edev10g_set_loopback(netdev,
+					    !!(features & NETIF_F_LOOPBACK));
+
+	return 0;
+}
+
+static const struct net_device_ops edev10g_netdev_ops = {
+	.ndo_open = edev10g_netdev_open,
+	.ndo_stop = edev10g_netdev_stop,
+	.ndo_change_mtu = edev10g_change_mtu,
+	.ndo_set_features = edev10g_set_features,
+	.ndo_start_xmit = n3000_dummy_netdev_xmit,
+};
+
+static void edev10g_adjust_link(struct net_device *netdev)
+{}
+
+static int edev10g_netdev_init(struct net_device *netdev)
+{
+	int ret;
+
+	ret = edev10g_mtu_init(netdev);
+	if (ret)
+		return ret;
+
+	return edev10g_set_loopback(netdev,
+				   !!(netdev->features & NETIF_F_LOOPBACK));
+}
+
+static int dfl_eth_dev_10g_init(struct eth_dev *edev)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	struct device *dev = edev->dev;
+	struct phy_device *phydev;
+	struct net_device *netdev;
+	int ret;
+
+	netdev = n3000_netdev_create(edev);
+	if (!netdev)
+		return -ENOMEM;
+
+	netdev->hw_features |= NETIF_F_LOOPBACK;
+	netdev->netdev_ops = &edev10g_netdev_ops;
+	netdev->ethtool_ops = &edev10g_ethtool_ops;
+
+	phydev = phy_connect(netdev, edev->phy_id, edev10g_adjust_link,
+			     PHY_INTERFACE_MODE_NA);
+	if (IS_ERR(phydev)) {
+		dev_err(dev, "PHY connection failed\n");
+		ret = PTR_ERR(phydev);
+		goto err_free_netdev;
+	}
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
+	linkmode_and(phydev->supported, phydev->supported, mask);
+	linkmode_copy(phydev->advertising, phydev->supported);
+
+	phy_attached_info(phydev);
+
+	ret = edev10g_netdev_init(netdev);
+	if (ret) {
+		dev_err(dev, "fail to init netdev %s\n", netdev->name);
+		goto err_phy_disconnect;
+	}
+
+	netdev->sysfs_groups[0] = &edev10g_attr_group;
+
+	netif_carrier_off(netdev);
+	ret = register_netdev(netdev);
+	if (ret) {
+		dev_err(dev, "fail to register netdev %s\n", netdev->name);
+		goto err_phy_disconnect;
+	}
+
+	edev->netdev = netdev;
+
+	return 0;
+
+err_phy_disconnect:
+	if (netdev->phydev)
+		phy_disconnect(phydev);
+err_free_netdev:
+	free_netdev(netdev);
+
+	return ret;
+}
+
+static void dfl_eth_dev_10g_remove(struct eth_dev *edev)
+{
+	struct net_device *netdev = edev->netdev;
+
+	if (netdev->phydev)
+		phy_disconnect(netdev->phydev);
+
+	unregister_netdev(netdev);
+}
+
+struct eth_dev_ops dfl_eth_dev_10g_ops = {
+	.lineside_init = dfl_eth_dev_10g_init,
+	.lineside_remove = dfl_eth_dev_10g_remove,
+	.reset = edev10g_reset,
+};
diff --git a/drivers/net/ethernet/intel/dfl-eth-group-main.c b/drivers/net/ethernet/intel/dfl-eth-group-main.c
index a29b8b1..89b4450 100644
--- a/drivers/net/ethernet/intel/dfl-eth-group-main.c
+++ b/drivers/net/ethernet/intel/dfl-eth-group-main.c
@@ -481,6 +481,9 @@ static int eth_group_setup(struct dfl_eth_group *egroup)
 		return ret;
 
 	switch (egroup->speed) {
+	case 10:
+		egroup->ops = &dfl_eth_dev_10g_ops;
+		break;
 	case 25:
 		egroup->ops = &dfl_eth_dev_25g_ops;
 		break;
diff --git a/drivers/net/ethernet/intel/dfl-eth-group.h b/drivers/net/ethernet/intel/dfl-eth-group.h
index 2e90f86..63f49a0 100644
--- a/drivers/net/ethernet/intel/dfl-eth-group.h
+++ b/drivers/net/ethernet/intel/dfl-eth-group.h
@@ -77,6 +77,7 @@ struct net_device *n3000_netdev_create(struct eth_dev *edev);
 netdev_tx_t n3000_dummy_netdev_xmit(struct sk_buff *skb,
 				    struct net_device *dev);
 
+extern struct eth_dev_ops dfl_eth_dev_10g_ops;
 extern struct eth_dev_ops dfl_eth_dev_25g_ops;
 extern struct eth_dev_ops dfl_eth_dev_40g_ops;
 
-- 
2.7.4

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Andrew Lunn]

Hi Xu

Before i look at the other patches, i want to understand the
architecture properly.

> +=======================================================================
> +DFL device driver for Ether Group private feature on Intel(R) PAC N3000
> +=======================================================================
> +
> +This is the driver for Ether Group private feature on Intel(R)
> +PAC (Programmable Acceleration Card) N3000.

I assume this is just one implementation. The FPGA could be placed on
other boards. So some of the limitations you talk about with the BMC
artificial, and the overall architecture of the drivers is more
generic?

> +The Intel(R) PAC N3000 is a FPGA based SmartNIC platform for multi-workload
> +networking application acceleration. A simple diagram below to for the board:
> +
> +                     +----------------------------------------+
> +                     |                  FPGA                  |
> ++----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
> +|QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
> ++----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
> +                     |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
> +                     +-----------+  |offloading|  +-----------+   +----------+
> +                     |              +----------+              |
> +                     |                                        |
> +                     +----------------------------------------+

Is XL710 required? I assume any MAC with the correct MII interface
will work?

Do you really mean PHY? I actually expect it is PCS? 

> +The DFL Ether Group driver registers netdev for each line side link. Users
> +could use standard commands (ethtool, ip, ifconfig) for configuration and
> +link state/statistics reading. For host side links, they are always connected
> +to the host ethernet controller, so they should always have same features as
> +the host ethernet controller. There is no need to register netdevs for them.

So lets say the XL710 is eth0. The line side netif is eth1. Where do i
put the IP address? What interface do i add to quagga OSPF? 

> +The driver just enables these links on probe.
> +
> +The retimer chips are managed by onboard BMC (Board Management Controller)
> +firmware, host driver is not capable to access them directly.

What about the QSPF socket? Can the host get access to the I2C bus?
The pins for TX enable, etc. ethtool -m?

> +Speed/Duplex
> +------------
> +The Ether Group doesn't support auto-negotiation. The link speed is fixed to
> +10G, 25G or 40G full duplex according to which Ether Group IP is programmed.

So that means, if i pop out the SFP and put in a different one which
supports a different speed, it is expected to be broken until the FPGA
is reloaded?

     Andrew

Re: [RFC PATCH 2/6] fpga: dfl: export network configuration info for DFL based FPGA

[Tom Rix]

On 10/23/20 1:45 AM, Xu Yilun wrote:
> This patch makes preparation for supporting DFL Ether Group private
> feature driver, which reads bitstream_id.vendor_net_cfg field to
> determin the interconnection of network components on FPGA device.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/dfl-fme-main.c | 10 ++--------
>  drivers/fpga/dfl.c          | 21 +++++++++++++++++++++
>  drivers/fpga/dfl.h          | 12 ++++++++++++
>  include/linux/dfl.h         |  2 ++
>  4 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 77ea04d..a2b8ba0 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -46,14 +46,8 @@ static DEVICE_ATTR_RO(ports_num);
>  static ssize_t bitstream_id_show(struct device *dev,
>  				 struct device_attribute *attr, char *buf)
>  {
> -	void __iomem *base;
> -	u64 v;
> -
> -	base = dfl_get_feature_ioaddr_by_id(dev, FME_FEATURE_ID_HEADER);
> -
> -	v = readq(base + FME_HDR_BITSTREAM_ID);
> -
> -	return scnprintf(buf, PAGE_SIZE, "0x%llx\n", (unsigned long long)v);
> +	return scnprintf(buf, PAGE_SIZE, "0x%llx\n",
> +			 (unsigned long long)dfl_get_bitstream_id(dev));
should use sysfs_emit()
>  }
>  static DEVICE_ATTR_RO(bitstream_id);
>  
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index bc35750..ca3c678 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -537,6 +537,27 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv)
>  }
>  EXPORT_SYMBOL(dfl_driver_unregister);
>  
> +int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev)
> +{
> +	struct device *fme_dev;
> +	u64 v;
> +
> +	if (!dfl_dev)
> +		return -EINVAL;
> +
> +	if (dfl_dev->type == FME_ID)
> +		fme_dev = dfl_dev->dev.parent;
> +	else
> +		fme_dev = dfl_dev->cdev->fme_dev;
> +
> +	if (!fme_dev)
> +		return -EINVAL;
> +
> +	v = dfl_get_bitstream_id(fme_dev);
> +	return (int)FIELD_GET(FME_BID_VENDOR_NET_CFG, v);
> +}
> +EXPORT_SYMBOL_GPL(dfl_dev_get_vendor_net_cfg);
> +
>  #define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
>  
>  /**
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2b82c96..6c7a6961 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -104,6 +104,9 @@
>  #define FME_CAP_CACHE_SIZE	GENMASK_ULL(43, 32)	/* cache size in KB */
>  #define FME_CAP_CACHE_ASSOC	GENMASK_ULL(47, 44)	/* Associativity */
>  
> +/* FME BITSTREAM_ID Register Bitfield */
> +#define FME_BID_VENDOR_NET_CFG	GENMASK_ULL(35, 32)     /* vendor net cfg */

Are there any other similar #defines that could be added here for completeness?

> +
>  /* FME Port Offset Register Bitfield */
>  /* Offset to port device feature header */
>  #define FME_PORT_OFST_DFH_OFST	GENMASK_ULL(23, 0)
> @@ -397,6 +400,15 @@ static inline bool is_dfl_feature_present(struct device *dev, u16 id)
>  	return !!dfl_get_feature_ioaddr_by_id(dev, id);
>  }
>  
> +static inline u64 dfl_get_bitstream_id(struct device *dev)
> +{
> +	void __iomem *base;
> +
> +	base = dfl_get_feature_ioaddr_by_id(dev, FME_FEATURE_ID_HEADER);
> +
> +	return readq(base + FME_HDR_BITSTREAM_ID);
> +}

This is is a generic change and should be split out.

Tom

> +
>  static inline
>  struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
>  {
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index e1b2471..5ee2b1e 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -67,6 +67,8 @@ struct dfl_driver {
>  #define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
>  #define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
>  
> +int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev);
> +
>  /*
>   * use a macro to avoid include chaining to get THIS_MODULE.
>   */

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Tom Rix]

On 10/23/20 1:45 AM, Xu Yilun wrote:
> This patch adds the document for DFL Ether Group driver.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  .../networking/device_drivers/ethernet/index.rst   |   1 +
>  .../ethernet/intel/dfl-eth-group.rst               | 102 +++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>  create mode 100644 Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst
>
> diff --git a/Documentation/networking/device_drivers/ethernet/index.rst b/Documentation/networking/device_drivers/ethernet/index.rst
> index cbb75a18..eb7c443 100644
> --- a/Documentation/networking/device_drivers/ethernet/index.rst
> +++ b/Documentation/networking/device_drivers/ethernet/index.rst
> @@ -26,6 +26,7 @@ Contents:
>     freescale/gianfar
>     google/gve
>     huawei/hinic
> +   intel/dfl-eth-group
>     intel/e100
>     intel/e1000
>     intel/e1000e
> diff --git a/Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst b/Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst
> new file mode 100644
> index 0000000..525807e
> --- /dev/null
> +++ b/Documentation/networking/device_drivers/ethernet/intel/dfl-eth-group.rst
> @@ -0,0 +1,102 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +=======================================================================
> +DFL device driver for Ether Group private feature on Intel(R) PAC N3000
> +=======================================================================
> +
> +This is the driver for Ether Group private feature on Intel(R)
> +PAC (Programmable Acceleration Card) N3000.
> +
> +The Intel(R) PAC N3000 is a FPGA based SmartNIC platform for multi-workload
> +networking application acceleration. A simple diagram below to for the board:
> +
> +                     +----------------------------------------+
> +                     |                  FPGA                  |
> ++----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
> +|QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
> ++----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
> +                     |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
> +                     +-----------+  |offloading|  +-----------+   +----------+
> +                     |              +----------+              |
> +                     |                                        |
> +                     +----------------------------------------+
> +
> +The FPGA is composed of FPGA Interface Module (FIM) and Accelerated Function
> +Unit (AFU). The FIM implements the basic functionalities for FPGA access,
> +management and reprograming, while the AFU is the FPGA reprogramable region for
> +users.
> +
> +The Line Side & Host Side Ether Groups are soft IP blocks embedded in FIM. They
The Line Side and Host Side Ether Groups are soft IP blocks embedded in the FIM.
> +are internally wire connected to AFU and communicate with AFU with MAC packets.
are internally connected to the AFU and communicate with the AFU using MAC packets
> +The user logic is developed by the FPGA users and re-programmed to AFU,
The user logic is application dependent, supplied by the FPGA developer and used to reprogram the AFU.
> +providing the user defined wire connections between line side & host side data
between Line Side and Host Side
> +interfaces, as well as the MAC layer offloading.
> +
> +There are 2 types of interfaces for the Ether Groups:
> +
> +1. The data interfaces connects the Ether Groups and the AFU, host has no
The data interface which connects
> +ability to control the data stream . So the FPGA is like a pipe between the
> +host ethernet controller and the retimer chip.
> +
> +2. The management interfaces connects the Ether Groups to the host, so host
The management interface which connects
> +could access the Ether Group registers for configuration and statistics
> +reading.
> +
> +The Intel(R) PAC N3000 could be programmed to various configurations (with
N3000 can be
> +different link numbers and speeds, e.g. 8x10G, 4x25G ...). It is done by
This is done
> +programing different variants of the Ether Group IP blocks, and doing
> +corresponding configuration to the retimer chips.
programming different variants of the Ether Group IP blocks and retimer configuration.
> +
> +The DFL Ether Group driver registers netdev for each line side link. Users
registers a netdev
> +could use standard commands (ethtool, ip, ifconfig) for configuration and
> +link state/statistics reading. For host side links, they are always connected
> +to the host ethernet controller, so they should always have same features as
> +the host ethernet controller. There is no need to register netdevs for them.
> +The driver just enables these links on probe.
> +
> +The retimer chips are managed by onboard BMC (Board Management Controller)
> +firmware, host driver is not capable to access them directly. So it is mostly

firmware. The host driver

So it behaves like

> +like an external fixed PHY. However the link states detected by the retimer
> +chips can not be propagated to the Ether Groups for hardware limitation, in
Limitations should get there own section, this is going off on tangent.
> +order to manage the link state, a PHY driver (intel-m10-bmc-retimer) is
> +introduced to query the BMC for the retimer's link state. The Ether Group
> +driver would connect to the PHY devices and get the link states. The
> +intel-m10-bmc-retimer driver creates a peseudo MDIO bus for each board, so
> +that the Ether Group driver could find the PHY devices by their peseudo PHY
> +addresses.
> +
> +
> +2. Features supported
> +=====================
> +
> +Data Path
> +---------
> +Since the driver can't control the data stream, the Ether Group driver
> +doesn't implement the valid tx/rx functions. Any transmit attempt on these
> +links from host will be dropped, and no data could be received to host from
links from the host will be dropped.  (you can assume a dropped link will not have data and shorten the sentence)
> +these links. Users should operate on the netdev of host ethernet controller
> +for networking data traffic.
> +
> +
> +Speed/Duplex
> +------------
> +The Ether Group doesn't support auto-negotiation. The link speed is fixed to
does not
> +10G, 25G or 40G full duplex according to which Ether Group IP is programmed.
> +
> +Statistics
> +----------
> +The Ether Group IP has the statistics counters for ethernet traffic and errors.
> +The user can obtain these MAC-level statistics using "ethtool -S" option.
> +
> +MTU
> +---
> +The Ether Group IP is capable of detecting oversized packets. It will not drop
> +the packet but pass it up and increment the tx/rx oversize counters. The MTU
but will pass it and
> +could be changed via ip or ifconfig commands.
> +
> +Flow Control
> +------------
> +Ethernet Flow Control (IEEE 802.3x) can be configured with ethtool to enable
> +transmitting pause frames. Receiving pause request from outside to Ether Group

pausing tx frames. Receiving a pause

Tom

> +MAC is not supported. The flow control auto-negotiation is not supported. The
> +user can enable or disable Tx Flow Control using "ethtool -A eth? tx <on|off>"

Re: [RFC PATCH 5/6] ethernet: dfl-eth-group: add DFL eth group private feature driver

[Andrew Lunn]

> +++ b/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
> @@ -0,0 +1,19 @@
> +What:		/sys/class/net/<iface>/tx_pause_frame_quanta
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:
> +		Read-Write. Value representing the tx pause quanta of Pause
> +		flow control frames to be sent to remote partner. Read the file
> +		for the actual tx pause quanta value. Write the file to set
> +		value of the tx pause quanta.
> +

Is this really configuring the quanta? My very limited understanding
is that the quanta is defined as 512 bit times. For this to work, you
are going to have to modify the quanta on both ends of the link,
otherwise increasing the quanta is actually going to shorten the pause
time.

> +What:		/sys/class/net/<iface>/tx_pause_frame_holdoff
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:
> +		Read-Write. Value representing the separation between 2
> +		consecutive XOFF flow control frames. Read the file for the
> +		actual separation value. Write the file to set value of the
> +		separation.

This is the wrong API for this. Please extend the ethtool -A|--pause
API. Now that it is netlink, adding extra attributes should be simple.

	Andrew
 

Re: [RFC PATCH 3/6] fpga: dfl: add an API to get the base device for dfl device

[Tom Rix]

On 10/23/20 1:45 AM, Xu Yilun wrote:
> This patch adds an API for dfl devices to find which physical device
> owns the DFL.
>
> This patch makes preparation for supporting DFL Ether Group private
> feature driver. It uses this information to determine which retimer
> device physically connects to which ether group.
device is physically
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/dfl.c  | 9 +++++++++
>  include/linux/dfl.h | 1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index ca3c678..52d18e6 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -558,6 +558,15 @@ int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev)
>  }
>  EXPORT_SYMBOL_GPL(dfl_dev_get_vendor_net_cfg);
>  
> +struct device *dfl_dev_get_base_dev(struct dfl_device *dfl_dev)
> +{
> +	if (!dfl_dev || !dfl_dev->cdev)
> +		return NULL;
> +
> +	return dfl_dev->cdev->parent;
> +}
> +EXPORT_SYMBOL_GPL(dfl_dev_get_base_dev);

This name is awkward. maybe

dfl_dev_parent() or dfl_dev_base()


> +
>  #define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
>  
>  /**
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 5ee2b1e..dd313f2 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -68,6 +68,7 @@ struct dfl_driver {
>  #define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
>  
>  int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev);
> +struct device *dfl_dev_get_base_dev(struct dfl_device *dfl_dev);

Because this is an external interface these should have comments

This is another generic change and should be split out of the patchset.

I believe the generic changes would have an easier time being accepted and could be done in parallel as the harder part of the private features is worked out.

Tom

>  
>  /*
>   * use a macro to avoid include chaining to get THIS_MODULE.

Re: [RFC PATCH 4/6] ethernet: m10-retimer: add support for retimers on Intel MAX 10 BMC

[Tom Rix]

On 10/23/20 1:45 AM, Xu Yilun wrote:
> This driver supports the ethernet retimers (Parkvale) for the Intel PAC
> (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.

Parkvale is a code name, it would be better if the public name was used.

As this is a physical chip that could be used on other cards,

I think the generic parts should be split out of intel-m10-bmc-retimer.c

into a separate file, maybe retimer-c827.c

Tom

>
> Parkvale is an Intel(R) Ethernet serdes transceiver chip that supports up
> to 100G transfer. On Intel PAC N3000 there are 2 Parkvale chips managed
> by the Intel MAX 10 BMC firmware. They are configured in 4 ports 10G/25G
> retimer mode. Host could query their link status via retimer interfaces
> (Shared registers) on Intel MAX 10 BMC.
>
> The driver adds the phy device for each port of the 2 retimer chips.
> Since the phys are not accessed by the real MDIO bus, it creates a virtual
> mdio bus for each NIC device instance, and a dedicated phy driver which
> only provides the supported features and link state.
>
> A DFL Ether Group driver will create net devices and connect to these
> phys.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/dfl-n3000-nios.c                      |  11 +-
>  drivers/mfd/intel-m10-bmc.c                        |  18 ++
>  drivers/net/ethernet/intel/Kconfig                 |  12 ++
>  drivers/net/ethernet/intel/Makefile                |   2 +
>  drivers/net/ethernet/intel/intel-m10-bmc-retimer.c | 229 +++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h                  |  16 ++
>  6 files changed, 286 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
>
> diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
> index 4983a2b..096931a 100644
> --- a/drivers/fpga/dfl-n3000-nios.c
> +++ b/drivers/fpga/dfl-n3000-nios.c
> @@ -16,6 +16,7 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/stddef.h>
> @@ -159,6 +160,8 @@ struct n3000_nios {
>  	struct regmap *regmap;
>  	struct device *dev;
>  	struct platform_device *altera_spi;
> +	struct intel_m10bmc_platdata m10bmc_pdata;
> +	struct intel_m10bmc_retimer_pdata m10bmc_retimer_pdata;
>  };
>  
>  static ssize_t nios_fw_version_show(struct device *dev,
> @@ -412,7 +415,8 @@ static struct spi_board_info m10_n3000_info = {
>  	.chip_select = 0,
>  };
>  
> -static int create_altera_spi_controller(struct n3000_nios *nn)
> +static int create_altera_spi_controller(struct n3000_nios *nn,
> +					struct device *retimer_master)
>  {
>  	struct altera_spi_platform_data pdata = { 0 };
>  	struct platform_device_info pdevinfo = { 0 };
> @@ -431,6 +435,9 @@ static int create_altera_spi_controller(struct n3000_nios *nn)
>  	pdata.bits_per_word_mask =
>  		SPI_BPW_RANGE_MASK(1, FIELD_GET(N3000_NS_PARAM_DATA_WIDTH, v));
>  
> +	nn->m10bmc_retimer_pdata.retimer_master = retimer_master;
> +	nn->m10bmc_pdata.retimer = &nn->m10bmc_retimer_pdata;
> +	m10_n3000_info.platform_data = &nn->m10bmc_pdata;
>  	pdata.num_devices = 1;
>  	pdata.devices = &m10_n3000_info;
>  
> @@ -549,7 +556,7 @@ static int n3000_nios_probe(struct dfl_device *ddev)
>  	if (ret)
>  		return ret;
>  
> -	ret = create_altera_spi_controller(nn);
> +	ret = create_altera_spi_controller(nn, dfl_dev_get_base_dev(ddev));
>  	if (ret)
>  		dev_err(dev, "altera spi controller create failed: %d\n", ret);
>  
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> index b84579b..adbfb177 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -23,6 +23,21 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
>  	{ .name = "n3000bmc-secure" },
>  };
>  
> +static void
> +m10bmc_init_cells_platdata(struct intel_m10bmc_platdata *pdata,
> +			   struct mfd_cell *cells, int n_cell)
> +{
> +	int i;
> +
> +	for (i = 0; i < n_cell; i++) {
> +		if (!strcmp(cells[i].name, "n3000bmc-retimer")) {
> +			cells[i].platform_data = pdata->retimer;
> +			cells[i].pdata_size =
> +				pdata->retimer ? sizeof(*pdata->retimer) : 0;
> +		}
> +	}
> +}
> +
>  static struct regmap_config intel_m10bmc_regmap_config = {
>  	.reg_bits = 32,
>  	.val_bits = 32,
> @@ -97,6 +112,7 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
>  
>  static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  {
> +	struct intel_m10bmc_platdata *pdata = dev_get_platdata(&spi->dev);
>  	const struct spi_device_id *id = spi_get_device_id(spi);
>  	struct device *dev = &spi->dev;
>  	struct mfd_cell *cells;
> @@ -134,6 +150,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  		return -ENODEV;
>  	}
>  
> +	m10bmc_init_cells_platdata(pdata, cells, n_cell);
> +
>  	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
>  				   NULL, 0, NULL);
>  	if (ret)
> diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> index 5aa8631..81c43d4 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -343,4 +343,16 @@ config IGC
>  	  To compile this driver as a module, choose M here. The module
>  	  will be called igc.
>  
> +config INTEL_M10_BMC_RETIMER
> +	tristate "Intel(R) MAX 10 BMC ethernet retimer support"
> +	depends on MFD_INTEL_M10_BMC
> +	select PHYLIB
> +        help
> +          This driver supports the ethernet retimer (Parkvale) on
> +	  Intel(R) MAX 10 BMC, which is used by Intel PAC N3000 FPGA based
> +	  Smart NIC.
> +
> +	  To compile this driver as a module, choose M here. The module
> +	  will be called intel-m10-bmc-retimer.
> +
>  endif # NET_VENDOR_INTEL
> diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
> index 3075290..5965447 100644
> --- a/drivers/net/ethernet/intel/Makefile
> +++ b/drivers/net/ethernet/intel/Makefile
> @@ -16,3 +16,5 @@ obj-$(CONFIG_IXGB) += ixgb/
>  obj-$(CONFIG_IAVF) += iavf/
>  obj-$(CONFIG_FM10K) += fm10k/
>  obj-$(CONFIG_ICE) += ice/
> +
> +obj-$(CONFIG_INTEL_M10_BMC_RETIMER) += intel-m10-bmc-retimer.o
> diff --git a/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
> new file mode 100644
> index 0000000..c7b0558
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Intel Max10 BMC Retimer Interface Driver
> + *
> + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> + *
> + */
> +#include <linux/device.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +
> +#define NUM_CHIP	2
> +#define MAX_LINK	4
> +
> +#define BITS_MASK(nbits)	((1 << (nbits)) - 1)
> +
> +#define N3000BMC_RETIMER_DEV_NAME "n3000bmc-retimer"
> +#define M10BMC_RETIMER_MII_NAME "m10bmc retimer mii"
> +
> +struct m10bmc_retimer {
> +	struct device *dev;
> +	struct intel_m10bmc *m10bmc;
> +	int num_devs;
> +	struct device *base_dev;
> +	struct mii_bus *retimer_mii_bus;
> +};
> +
> +#define RETIMER_LINK_STAT_BIT(retimer_id, link_id) \
> +	BIT(((retimer_id) << 2) + (link_id))
> +
> +static u32 retimer_get_link(struct m10bmc_retimer *retimer, int index)
> +{
> +	unsigned int val;
> +
> +	if (m10bmc_sys_read(retimer->m10bmc, PKVL_LINK_STATUS, &val)) {
> +		dev_err(retimer->dev, "fail to read PKVL_LINK_STATUS\n");
> +		return 0;
> +	}
> +
> +	if (val & BIT(index))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int m10bmc_retimer_phy_match(struct phy_device *phydev)
> +{
> +	if (phydev->mdio.bus->name &&
> +	    !strcmp(phydev->mdio.bus->name, M10BMC_RETIMER_MII_NAME)) {
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int m10bmc_retimer_phy_probe(struct phy_device *phydev)
> +{
> +	struct m10bmc_retimer *retimer = phydev->mdio.bus->priv;
> +
> +	phydev->priv = retimer;
> +
> +	return 0;
> +}
> +
> +static void m10bmc_retimer_phy_remove(struct phy_device *phydev)
> +{
> +	if (phydev->attached_dev)
> +		phy_disconnect(phydev);
> +}
> +
> +static int m10bmc_retimer_read_status(struct phy_device *phydev)
> +{
> +	struct m10bmc_retimer *retimer = phydev->priv;
> +
> +	phydev->link = retimer_get_link(retimer, phydev->mdio.addr);
> +
> +	phydev->duplex = DUPLEX_FULL;
> +
> +	return 0;
> +}
> +
> +static int m10bmc_retimer_get_features(struct phy_device *phydev)
> +{
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
> +			 phydev->supported);
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseSR_Full_BIT,
> +			 phydev->supported);
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT,
> +			 phydev->supported);
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
> +
> +	return 0;
> +}
> +
> +static struct phy_driver m10bmc_retimer_phy_driver = {
> +	.phy_id			= 0xffffffff,
> +	.phy_id_mask		= 0xffffffff,
> +	.name			= "m10bmc retimer PHY",
> +	.match_phy_device	= m10bmc_retimer_phy_match,
> +	.probe			= m10bmc_retimer_phy_probe,
> +	.remove			= m10bmc_retimer_phy_remove,
> +	.read_status		= m10bmc_retimer_read_status,
> +	.get_features		= m10bmc_retimer_get_features,
> +	.read_mmd		= genphy_read_mmd_unsupported,
> +	.write_mmd		= genphy_write_mmd_unsupported,
> +};
> +
> +static int m10bmc_retimer_read(struct mii_bus *bus, int addr, int regnum)
> +{
> +	struct m10bmc_retimer *retimer = bus->priv;
> +
> +	if (addr < retimer->num_devs &&
> +	    (regnum == MII_PHYSID1 || regnum == MII_PHYSID2))
> +		return 0;
> +
> +	return 0xffff;
> +}
> +
> +static int m10bmc_retimer_write(struct mii_bus *bus, int addr, int regnum, u16 val)
> +{
> +	return 0;
> +}
> +
> +static int m10bmc_retimer_mii_bus_init(struct m10bmc_retimer *retimer)
> +{
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc(retimer->dev);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->priv = (void *)retimer;
> +	bus->name = M10BMC_RETIMER_MII_NAME;
> +	bus->read = m10bmc_retimer_read;
> +	bus->write = m10bmc_retimer_write;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
> +		 dev_name(retimer->base_dev));
> +	bus->parent = retimer->dev;
> +	bus->phy_mask = ~(BITS_MASK(retimer->num_devs));
> +
> +	ret = mdiobus_register(bus);
> +	if (ret)
> +		return ret;
> +
> +	retimer->retimer_mii_bus = bus;
> +
> +	return 0;
> +}
> +
> +static void m10bmc_retimer_mii_bus_uinit(struct m10bmc_retimer *retimer)
> +{
> +	mdiobus_unregister(retimer->retimer_mii_bus);
> +}
> +
> +static int intel_m10bmc_retimer_probe(struct platform_device *pdev)
> +{
> +	struct intel_m10bmc_retimer_pdata *pdata = dev_get_platdata(&pdev->dev);
> +	struct intel_m10bmc *m10bmc = dev_get_drvdata(pdev->dev.parent);
> +	struct m10bmc_retimer *retimer;
> +
> +	retimer = devm_kzalloc(&pdev->dev, sizeof(*retimer), GFP_KERNEL);
> +	if (!retimer)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, retimer);
> +
> +	retimer->dev = &pdev->dev;
> +	retimer->m10bmc = m10bmc;
> +	retimer->base_dev = pdata->retimer_master;
> +	retimer->num_devs = NUM_CHIP * MAX_LINK;
> +
> +	return m10bmc_retimer_mii_bus_init(retimer);
> +}
> +
> +static int intel_m10bmc_retimer_remove(struct platform_device *pdev)
> +{
> +	struct m10bmc_retimer *retimer = dev_get_drvdata(&pdev->dev);
> +
> +	m10bmc_retimer_mii_bus_uinit(retimer);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver intel_m10bmc_retimer_driver = {
> +	.probe = intel_m10bmc_retimer_probe,
> +	.remove = intel_m10bmc_retimer_remove,
> +	.driver = {
> +		.name = N3000BMC_RETIMER_DEV_NAME,
> +	},
> +};
> +
> +static int __init intel_m10bmc_retimer_init(void)
> +{
> +	int ret;
> +
> +	ret = phy_driver_register(&m10bmc_retimer_phy_driver, THIS_MODULE);
> +	if (ret)
> +		return ret;
> +
> +	return platform_driver_register(&intel_m10bmc_retimer_driver);
> +}
> +module_init(intel_m10bmc_retimer_init);
> +
> +static void __exit intel_m10bmc_retimer_exit(void)
> +{
> +	platform_driver_unregister(&intel_m10bmc_retimer_driver);
> +	phy_driver_unregister(&m10bmc_retimer_phy_driver);
> +}
> +module_exit(intel_m10bmc_retimer_exit);
> +
> +MODULE_ALIAS("platform:" N3000BMC_RETIMER_DEV_NAME);
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("Intel MAX 10 BMC retimer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index c8ef2f1..3d9d22c 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -21,6 +21,22 @@
>  #define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
>  #define M10BMC_VER_LEGACY_INVALID	0xffffffff
>  
> +/* PKVL related registers, in system register region */
> +#define PKVL_LINK_STATUS		0x164
> +
> +/**
> + * struct intel_m10bmc_retimer_pdata - subdev retimer platform data
> + *
> + * @retimer_master: the NIC device which connects to the retimers on m10bmc
> + */
> +struct intel_m10bmc_retimer_pdata {
> +	struct device *retimer_master;
> +};
> +
> +struct intel_m10bmc_platdata {
> +	struct intel_m10bmc_retimer_pdata *retimer;
> +};
> +
>  /**
>   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
>   * @dev: this device

Re: [RFC PATCH 4/6] ethernet: m10-retimer: add support for retimers on Intel MAX 10 BMC

[Andrew Lunn]

On Sat, Oct 24, 2020 at 08:03:51AM -0700, Tom Rix wrote:
> 
> On 10/23/20 1:45 AM, Xu Yilun wrote:
> > This driver supports the ethernet retimers (Parkvale) for the Intel PAC
> > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> 
> Parkvale is a code name, it would be better if the public name was used.
> 
> As this is a physical chip that could be used on other cards,
> 
> I think the generic parts should be split out of intel-m10-bmc-retimer.c
> 
> into a separate file, maybe retimer-c827.c

This driver is not really a driver for the Parkvale. That driver is
hidden away in the BMC. So we need to be a bit careful with the name,
leaving it available for when somebody writes a real Linux driver for
retimer.

	Andrew

Re: [RFC PATCH 5/6] ethernet: dfl-eth-group: add DFL eth group private feature driver

[Tom Rix]

On 10/23/20 1:45 AM, Xu Yilun wrote:
> This driver supports the DFL Ether Group private feature for the Intel(R)
> PAC N3000 FPGA Smart NIC.
>
> The DFL Ether Group private feature contains an Ether Wrapper and several
> ports (phy-mac pair). There are two types of Ether Groups, line side &

(phy-mac pairs)

line side and

> host side. These 2 types of Ether Groups, together with FPGA internal
> logic, act as several independent pipes between the host Ethernet
> Controller and the front-panel cages. The FPGA logic between the 2 Ether
> Groups implements user defined mac layer offloading.
>
> The line side ether interfaces connect to the Parkvale serdes transceiver
> chips, which are working in 4 ports 10G/25G retimer mode.
>
> There are several configurations (8x10G, 2x1x25G, 2x2x25G ...) for
> different connections between line side and host side. Not all links are
> active in some configurations.
>
> For the line side link, the driver connects to the Parkvale phy device
> and then register net device for each active link.
and then registers a
>
> For the host side link, its link state is always on. The host side link
, the link
> always has same features as host side ether controller, so there is no
has the same
> need to register a netdev for it. The driver just enables the link on
> probe.
>
> This patch supports the 25G configurations. Support for 10G will be in
> other patches.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  .../ABI/testing/sysfs-class-net-dfl-eth-group      |  19 +
>  drivers/net/ethernet/intel/Kconfig                 |  18 +
>  drivers/net/ethernet/intel/Makefile                |   2 +
>  drivers/net/ethernet/intel/dfl-eth-group-25g.c     | 525 +++++++++++++++++
>  drivers/net/ethernet/intel/dfl-eth-group-main.c    | 632 +++++++++++++++++++++

Is it necessary to have a separate dfl-eth-group-25g.c file ?

From below, it looks like adding a 25G prefix or suffix to the register #defines

would make them unique wrt the upcoming 10G.

since this patch is doing two big things, I wonder if the n3000_netdev part should be split out and added after 10G patch.

>  drivers/net/ethernet/intel/dfl-eth-group.h         |  83 +++
>  drivers/net/ethernet/intel/intel-m10-bmc-retimer.c |   4 +-
>  7 files changed, 1282 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
>  create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-25g.c
>  create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-main.c
>  create mode 100644 drivers/net/ethernet/intel/dfl-eth-group.h
>
> diff --git a/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group b/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
> new file mode 100644
> index 0000000..ad528f2
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-net-dfl-eth-group
> @@ -0,0 +1,19 @@
> +What:		/sys/class/net/<iface>/tx_pause_frame_quanta
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:
> +		Read-Write. Value representing the tx pause quanta of Pause
> +		flow control frames to be sent to remote partner. Read the file
> +		for the actual tx pause quanta value. Write the file to set
> +		value of the tx pause quanta.

units ?

What are the valid range of values ?

Similar below.

> +
> +What:		/sys/class/net/<iface>/tx_pause_frame_holdoff
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:
> +		Read-Write. Value representing the separation between 2
> +		consecutive XOFF flow control frames. Read the file for the
> +		actual separation value. Write the file to set value of the
> +		separation.
> diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> index 81c43d4..61b5d91 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -343,6 +343,24 @@ config IGC
>  	  To compile this driver as a module, choose M here. The module
>  	  will be called igc.
>  
> +config FPGA_DFL_ETH_GROUP
> +        tristate "DFL Ether Group private feature support"
> +        depends on FPGA_DFL && HAS_IOMEM
> +        help
> +          This driver supports DFL Ether Group private feature for
The last 4 lines have white space issues, spaces should be replaced by tabs
> +	  Intel(R) PAC N3000 FPGA Smart NIC.
> +
> +	  The DFL Ether Group private feature contains several ether
> +	  interfaces, each of them contains an Ether Wrapper and several
> +	  ports (phy-mac pairs). There are two types of interfaces, Line
> +	  side & CPU side. These 2 types of interfaces, together with FPGA
Side and Host Side (be consistent with Documentation in patch 1)
> +	  internal logic, act as several independent pipes between the
> +	  host Ethernet Controller and the front-panel cages. The FPGA
> +	  logic between 2 interfaces implements user defined mac layer
between these interfaces implements the user
> +	  offloading.
> +
> +	  This driver implements each active port as a netdev.
> +
>  config INTEL_M10_BMC_RETIMER
>  	tristate "Intel(R) MAX 10 BMC ethernet retimer support"
>  	depends on MFD_INTEL_M10_BMC
> diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
> index 5965447..1624c26 100644
> --- a/drivers/net/ethernet/intel/Makefile
> +++ b/drivers/net/ethernet/intel/Makefile
> @@ -17,4 +17,6 @@ obj-$(CONFIG_IAVF) += iavf/
>  obj-$(CONFIG_FM10K) += fm10k/
>  obj-$(CONFIG_ICE) += ice/
>  
> +dfl-eth-group-objs := dfl-eth-group-main.o dfl-eth-group-25g.o
> +obj-$(CONFIG_FPGA_DFL_ETH_GROUP) += dfl-eth-group.o
>  obj-$(CONFIG_INTEL_M10_BMC_RETIMER) += intel-m10-bmc-retimer.o
> diff --git a/drivers/net/ethernet/intel/dfl-eth-group-25g.c b/drivers/net/ethernet/intel/dfl-eth-group-25g.c
> new file mode 100644
> index 0000000..a690364
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/dfl-eth-group-25g.c
> @@ -0,0 +1,525 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for 25G Ether Group private feature on Intel PAC (Programmable
> + * Acceleration Card) N3000
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Wu Hao <hao.wu@intel.com>
> + *   Xu Yilun <yilun.xu@intel.com>
> + */
> +#include <linux/netdevice.h>
> +
> +#include "dfl-eth-group.h"
> +
> +/* 25G PHY/MAC Register */
> +#define PHY_CONFIG	0x310
> +#define PHY_MAC_RESET_MASK	GENMASK(2, 0)
> +#define PHY_PMA_SLOOP		0x313
> +#define MAX_TX_SIZE_CONFIG	0x407
> +#define MAX_RX_SIZE_CONFIG	0x506
> +#define TX_FLOW_CTRL_EN		0x605
> +#define TX_FLOW_CTRL_EN_PAUSE	BIT(0)
> +#define TX_FLOW_CTRL_QUANTA	0x620
> +#define TX_FLOW_CTRL_HOLDOFF	0x628
> +#define TX_FLOW_CTRL_SEL	0x640
> +#define TX_FLOW_CTRL_SEL_PAUSE	0x0
> +#define TX_FLOW_CTRL_SEL_PFC	0x1

Generally #defines should have a unique-ish prefix to avoid collisions.

Maybe

DFL_ETH_GROUP_25G_

> +
> +static int edev25g40g_reset(struct eth_dev *edev, bool en)

The name of this function should have prefix similar to its filename, maybe

dfl_eth_group_25g_

similar other places

> +{
> +	struct eth_com *mac = edev->mac;
> +	struct device *dev = edev->dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = eth_com_read_reg(mac, PHY_CONFIG, &val);
> +	if (ret) {
> +		dev_err(dev, "fail to read PHY_CONFIG: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* skip if config is in expected state already */
> +	if ((((val & PHY_MAC_RESET_MASK) == PHY_MAC_RESET_MASK) && en) ||
> +	    (((val & PHY_MAC_RESET_MASK) == 0) && !en))
> +		return 0;
> +
> +	if (en)
> +		val |= PHY_MAC_RESET_MASK;
> +	else
> +		val &= ~PHY_MAC_RESET_MASK;
> +
> +	ret = eth_com_write_reg(mac, PHY_CONFIG, val);
> +	if (ret)
> +		dev_err(dev, "fail to write PHY_CONFIG: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static ssize_t tx_pause_frame_quanta_show(struct device *d,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
> +	u32 data;
> +	int ret;
> +
> +	ret = eth_com_read_reg(edev->mac, TX_FLOW_CTRL_QUANTA, &data);

if (!ret)

  ret = sysfs_emit();

return ret;

similar other places

> +
> +	return ret ? : sprintf(buf, "0x%x\n", data);
> +}
> +
> +static ssize_t tx_pause_frame_quanta_store(struct device *d,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(d);
> +	struct eth_dev *edev;
> +	u32 data;
> +	int ret;
> +
> +	if (kstrtou32(buf, 0, &data))
> +		return -EINVAL;
> +
> +	edev = net_device_to_eth_dev(netdev);
> +
> +	rtnl_lock();
> +
> +	if (netif_running(netdev)) {
> +		netdev_err(netdev, "must be stopped to change pause param\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = eth_com_write_reg(edev->mac, TX_FLOW_CTRL_QUANTA, data);
> +
> +out:
> +	rtnl_unlock();
> +
> +	return ret ? : len;
> +}
> +static DEVICE_ATTR_RW(tx_pause_frame_quanta);
> +
> +static ssize_t tx_pause_frame_holdoff_show(struct device *d,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
> +	u32 data;
> +	int ret;
> +
> +	ret = eth_com_read_reg(edev->mac, TX_FLOW_CTRL_HOLDOFF, &data);
> +
> +	return ret ? : sprintf(buf, "0x%x\n", data);
> +}
> +
> +static ssize_t tx_pause_frame_holdoff_store(struct device *d,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(d);
> +	struct eth_dev *edev;
> +	u32 data;
> +	int ret;
> +
> +	if (kstrtou32(buf, 0, &data))
> +		return -EINVAL;
> +
> +	edev = net_device_to_eth_dev(netdev);
> +
> +	rtnl_lock();
> +
> +	if (netif_running(netdev)) {
> +		netdev_err(netdev, "must be stopped to change pause param\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = eth_com_write_reg(edev->mac, TX_FLOW_CTRL_HOLDOFF, data);
> +
> +out:
> +	rtnl_unlock();
> +
> +	return ret ? : len;
> +}
> +static DEVICE_ATTR_RW(tx_pause_frame_holdoff);
> +
> +static struct attribute *edev25g_dev_attrs[] = {
> +	&dev_attr_tx_pause_frame_quanta.attr,
> +	&dev_attr_tx_pause_frame_holdoff.attr,
> +	NULL
> +};
> +
> +/* device attributes */
> +static const struct attribute_group edev25g_attr_group = {
> +	.attrs = edev25g_dev_attrs,
> +};
> +
> +/* ethtool ops */
> +static struct stat_info stats_25g[] = {
> +	/* TX Statistics */
> +	{STAT_INFO(0x800, "tx_fragments")},

magic numbers should be #defines

why is STAT_INFO needed ? seem like unneeded layer of abstraction

> +	{STAT_INFO(0x802, "tx_jabbers")},
> +	{STAT_INFO(0x804, "tx_fcs")},
> +	{STAT_INFO(0x806, "tx_crc_err")},
> +	{STAT_INFO(0x808, "tx_mcast_data_err")},
> +	{STAT_INFO(0x80a, "tx_bcast_data_err")},
> +	{STAT_INFO(0x80c, "tx_ucast_data_err")},
> +	{STAT_INFO(0x80e, "tx_mcast_ctrl_err")},
> +	{STAT_INFO(0x810, "tx_bcast_ctrl_err")},
> +	{STAT_INFO(0x812, "tx_ucast_ctrl_err")},
> +	{STAT_INFO(0x814, "tx_pause_err")},
> +	{STAT_INFO(0x816, "tx_64_byte")},
> +	{STAT_INFO(0x818, "tx_65_127_byte")},
> +	{STAT_INFO(0x81a, "tx_128_255_byte")},
> +	{STAT_INFO(0x81c, "tx_256_511_byte")},
> +	{STAT_INFO(0x81e, "tx_512_1023_byte")},
> +	{STAT_INFO(0x820, "tx_1024_1518_byte")},
> +	{STAT_INFO(0x822, "tx_1519_max_byte")},
> +	{STAT_INFO(0x824, "tx_oversize")},
> +	{STAT_INFO(0x826, "tx_mcast_data_ok")},
> +	{STAT_INFO(0x828, "tx_bcast_data_ok")},
> +	{STAT_INFO(0x82a, "tx_ucast_data_ok")},
> +	{STAT_INFO(0x82c, "tx_mcast_ctrl_ok")},
> +	{STAT_INFO(0x82e, "tx_bcast_ctrl_ok")},
> +	{STAT_INFO(0x830, "tx_ucast_ctrl_ok")},
> +	{STAT_INFO(0x832, "tx_pause")},
> +	{STAT_INFO(0x834, "tx_runt")},
gap
> +	{STAT_INFO(0x860, "tx_payload_octets_ok")},
> +	{STAT_INFO(0x862, "tx_frame_octets_ok")},
> +
> +	/* RX Statistics */
> +	{STAT_INFO(0x900, "rx_fragments")},
> +	{STAT_INFO(0x902, "rx_jabbers")},
> +	{STAT_INFO(0x904, "rx_fcs")},
> +	{STAT_INFO(0x906, "rx_crc_err")},
> +	{STAT_INFO(0x908, "rx_mcast_data_err")},
> +	{STAT_INFO(0x90a, "rx_bcast_data_err")},
> +	{STAT_INFO(0x90c, "rx_ucast_data_err")},
> +	{STAT_INFO(0x90e, "rx_mcast_ctrl_err")},
> +	{STAT_INFO(0x910, "rx_bcast_ctrl_err")},
> +	{STAT_INFO(0x912, "rx_ucast_ctrl_err")},
> +	{STAT_INFO(0x914, "rx_pause_err")},
> +	{STAT_INFO(0x916, "rx_64_byte")},
> +	{STAT_INFO(0x918, "rx_65_127_byte")},
> +	{STAT_INFO(0x91a, "rx_128_255_byte")},
> +	{STAT_INFO(0x91c, "rx_256_511_byte")},
> +	{STAT_INFO(0x91e, "rx_512_1023_byte")},
> +	{STAT_INFO(0x920, "rx_1024_1518_byte")},
> +	{STAT_INFO(0x922, "rx_1519_max_byte")},
> +	{STAT_INFO(0x924, "rx_oversize")},
> +	{STAT_INFO(0x926, "rx_mcast_data_ok")},
> +	{STAT_INFO(0x928, "rx_bcast_data_ok")},
> +	{STAT_INFO(0x92a, "rx_ucast_data_ok")},
> +	{STAT_INFO(0x92c, "rx_mcast_ctrl_ok")},
> +	{STAT_INFO(0x92e, "rx_bcast_ctrl_ok")},
> +	{STAT_INFO(0x930, "rx_ucast_ctrl_ok")},
> +	{STAT_INFO(0x932, "rx_pause")},
> +	{STAT_INFO(0x934, "rx_runt")},
gap
> +	{STAT_INFO(0x960, "rx_payload_octets_ok")},
> +	{STAT_INFO(0x962, "rx_frame_octets_ok")},
> +};
> +
> +static void edev25g_get_strings(struct net_device *netdev, u32 stringset, u8 *s)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	unsigned int i;
> +
> +	if (stringset != ETH_SS_STATS || edev->lw_mac)
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(stats_25g); i++, s += ETH_GSTRING_LEN)
> +		memcpy(s, stats_25g[i].string, ETH_GSTRING_LEN);
> +}
> +
> +static int edev25g_get_sset_count(struct net_device *netdev, int stringset)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +
> +	if (stringset != ETH_SS_STATS || edev->lw_mac)
> +		return -EOPNOTSUPP;
> +
> +	return (int)ARRAY_SIZE(stats_25g);
> +}
> +
> +static void edev25g_get_stats(struct net_device *netdev,
> +			      struct ethtool_stats *stats, u64 *data)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	unsigned int i;
> +
> +	if (edev->lw_mac || !netif_running(netdev))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(stats_25g); i++)
> +		data[i] = read_mac_stats(edev->mac, stats_25g[i].addr);
> +}
> +
> +static int edev25g_get_link_ksettings(struct net_device *netdev,
> +				      struct ethtool_link_ksettings *cmd)
> +{
> +	if (!netdev->phydev)
> +		return -ENODEV;
> +
> +	phy_ethtool_ksettings_get(netdev->phydev, cmd);
> +
> +	return 0;
> +}
> +
> +static int edev25g_pause_init(struct net_device *netdev)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +
> +	return eth_com_write_reg(edev->mac, TX_FLOW_CTRL_SEL,
> +				 TX_FLOW_CTRL_SEL_PAUSE);
> +}
> +
> +static void edev25g_get_pauseparam(struct net_device *netdev,

> +				   struct ethtool_pauseparam *pause)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	u32 data;
> +	int ret;
> +
> +	pause->autoneg = 0;
> +	pause->rx_pause = 0;
> +
> +	ret = eth_com_read_reg(edev->mac, TX_FLOW_CTRL_EN, &data);
> +	if (ret) {
> +		pause->tx_pause = 0;
> +		return;
> +	}
> +
> +	pause->tx_pause = (data & TX_FLOW_CTRL_EN_PAUSE) ? 0x1 : 0;
> +}
> +
> +static int edev25g_set_pauseparam(struct net_device *netdev,
> +				  struct ethtool_pauseparam *pause)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	bool enable = pause->tx_pause;
> +
> +	if (pause->autoneg || pause->rx_pause)
> +		return -EOPNOTSUPP;
> +
> +	return eth_com_write_reg(edev->mac, TX_FLOW_CTRL_EN,
> +				 enable ? TX_FLOW_CTRL_EN_PAUSE : 0);
> +}
> +
> +static const struct ethtool_ops edev25g_ethtool_ops = {
> +	.get_link = ethtool_op_get_link,
> +	.get_strings = edev25g_get_strings,
> +	.get_sset_count = edev25g_get_sset_count,
> +	.get_ethtool_stats = edev25g_get_stats,
> +	.get_link_ksettings = edev25g_get_link_ksettings,
> +	.get_pauseparam = edev25g_get_pauseparam,
> +	.set_pauseparam = edev25g_set_pauseparam,
> +};
> +
> +/* netdev ops */
> +static int edev25g_netdev_open(struct net_device *netdev)
> +{
> +	struct n3000_net_priv *priv = netdev_priv(netdev);
> +	struct eth_dev *edev = priv->edev;
> +	int ret;
> +
> +	ret = edev25g40g_reset(edev, false);
> +	if (ret)
> +		return ret;
> +
> +	if (netdev->phydev)
> +		phy_start(netdev->phydev);
> +
> +	return 0;
> +}
> +
> +static int edev25g_netdev_stop(struct net_device *netdev)
> +{
> +	struct n3000_net_priv *priv = netdev_priv(netdev);
> +	struct eth_dev *edev = priv->edev;
> +	int ret;
> +
> +	ret = edev25g40g_reset(edev, true);
> +	if (ret)
> +		return ret;
> +
> +	if (netdev->phydev)
> +		phy_stop(netdev->phydev);
> +
> +	return 0;
> +}
> +
> +static int edev25g_mtu_init(struct net_device *netdev)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	struct eth_com *mac = edev->mac;
> +	u32 tx = 0, rx = 0, mtu;
> +	int ret;
> +
> +	ret = eth_com_read_reg(mac, MAX_TX_SIZE_CONFIG, &tx);
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_com_read_reg(mac, MAX_RX_SIZE_CONFIG, &rx);
> +	if (ret)
> +		return ret;
> +
> +	mtu = min(min(tx, rx), netdev->max_mtu);
> +
> +	ret = eth_com_write_reg(mac, MAX_TX_SIZE_CONFIG, rx);
This looks wrong. Did you mean to use 'mtu' ?
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_com_write_reg(mac, MAX_RX_SIZE_CONFIG, tx);
> +	if (ret)
> +		return ret;
> +
> +	netdev->mtu = mtu;
> +
> +	return 0;
> +}
> +
> +static int edev25g_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	struct eth_com *mac = edev->mac;
> +	int ret;
> +
> +	ret = eth_com_write_reg(mac, MAX_TX_SIZE_CONFIG, new_mtu);
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_com_write_reg(mac, MAX_RX_SIZE_CONFIG, new_mtu);
> +	if (ret)
Should the old mtu be restored to tx here ?
> +		return ret;
> +
> +	netdev->mtu = new_mtu;
> +
> +	return 0;
> +}
> +
> +static int edev25g_set_loopback(struct net_device *netdev, bool en)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +
> +	return eth_com_write_reg(edev->mac, PHY_PMA_SLOOP, en);
> +}
> +
> +static int edev25g_set_features(struct net_device *netdev,
> +				netdev_features_t features)
> +{
> +	netdev_features_t changed = netdev->features ^ features;
> +
> +	if (changed & NETIF_F_LOOPBACK)
> +		return edev25g_set_loopback(netdev,
> +					    !!(features & NETIF_F_LOOPBACK));
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops edev25g_netdev_ops = {
> +	.ndo_open = edev25g_netdev_open,
> +	.ndo_stop = edev25g_netdev_stop,
> +	.ndo_change_mtu = edev25g_change_mtu,
> +	.ndo_set_features = edev25g_set_features,
> +	.ndo_start_xmit = n3000_dummy_netdev_xmit,
> +};
> +
> +static void edev25g_adjust_link(struct net_device *netdev)
> +{}
> +
> +static int edev25g_netdev_init(struct net_device *netdev)
> +{
> +	int ret;
> +
> +	ret = edev25g_pause_init(netdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = edev25g_mtu_init(netdev);
> +	if (ret)
> +		return ret;
> +
> +	return edev25g_set_loopback(netdev,
> +				    !!(netdev->features & NETIF_F_LOOPBACK));

The enable param calculated the same a couple of places.

Maybe drop the 'bool en' and move the calculation inside edev25g_set_loopback

> +}
> +
> +static int dfl_eth_dev_25g_init(struct eth_dev *edev)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +	struct device *dev = edev->dev;
> +	struct phy_device *phydev;
> +	struct net_device *netdev;
> +	int ret;
> +
> +	netdev = n3000_netdev_create(edev);
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	netdev->hw_features |= NETIF_F_LOOPBACK;
> +	netdev->netdev_ops = &edev25g_netdev_ops;
> +	netdev->ethtool_ops = &edev25g_ethtool_ops;
> +
> +	phydev = phy_connect(netdev, edev->phy_id, edev25g_adjust_link,
> +			     PHY_INTERFACE_MODE_NA);
> +	if (IS_ERR(phydev)) {
> +		dev_err(dev, "PHY connection failed\n");
> +		ret = PTR_ERR(phydev);
> +		goto err_free_netdev;
> +	}
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, mask);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, mask);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> +	linkmode_and(phydev->supported, phydev->supported, mask);
> +	linkmode_copy(phydev->advertising, phydev->supported);
> +
> +	phy_attached_info(phydev);
> +
> +	ret = edev25g_netdev_init(netdev);
> +	if (ret) {
> +		dev_err(dev, "fail to init netdev %s\n", netdev->name);
> +		goto err_phy_disconnect;
> +	}
> +
> +	netdev->sysfs_groups[0] = &edev25g_attr_group;
> +
> +	netif_carrier_off(netdev);
> +	ret = register_netdev(netdev);
> +	if (ret) {
> +		dev_err(dev, "fail to register netdev %s\n", netdev->name);
> +		goto err_phy_disconnect;
> +	}
> +
> +	edev->netdev = netdev;
> +
> +	return 0;
> +
> +err_phy_disconnect:
> +	if (netdev->phydev)
> +		phy_disconnect(phydev);
> +err_free_netdev:
> +	free_netdev(netdev);
> +
> +	return ret;
> +}
> +
> +static void dfl_eth_dev_25g_remove(struct eth_dev *edev)
> +{
> +	struct net_device *netdev = edev->netdev;
> +
> +	if (netdev->phydev)
> +		phy_disconnect(netdev->phydev);
> +
> +	unregister_netdev(netdev);
> +}
> +
> +struct eth_dev_ops dfl_eth_dev_25g_ops = {
> +	.lineside_init = dfl_eth_dev_25g_init,
> +	.lineside_remove = dfl_eth_dev_25g_remove,
> +	.reset = edev25g40g_reset,
> +};
> +
> +struct eth_dev_ops dfl_eth_dev_40g_ops = {
Change only references 25g. The 40g parts should be split.
> +	.reset = edev25g40g_reset,
> +};
> diff --git a/drivers/net/ethernet/intel/dfl-eth-group-main.c b/drivers/net/ethernet/intel/dfl-eth-group-main.c
> new file mode 100644
> index 0000000..a29b8b1
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/dfl-eth-group-main.c
> @@ -0,0 +1,632 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* DFL device driver for Ether Group private feature on Intel PAC (Programmable
> + * Acceleration Card) N3000
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Wu Hao <hao.wu@intel.com>
> + *   Xu Yilun <yilun.xu@intel.com>
> + */
> +#include <linux/bitfield.h>
> +#include <linux/dfl.h>
> +#include <linux/errno.h>
> +#include <linux/ethtool.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +
> +#include "dfl-eth-group.h"
> +
> +struct dfl_eth_group {
> +	char name[32];
> +	struct device *dev;
> +	void __iomem *base;
> +	/* lock to protect register access of the ether group */
> +	struct mutex reg_lock;
> +	struct dfl_device *dfl_dev;
> +	unsigned int config;
> +	unsigned int direction;
> +	unsigned int group_id;
> +	unsigned int speed;
> +	unsigned int lw_mac;
> +	unsigned int num_edevs;
> +	struct eth_dev *edevs;
> +	struct eth_dev_ops *ops;
> +};
> +
> +u64 read_mac_stats(struct eth_com *ecom, unsigned int addr)
> +{
> +	u32 data_l, data_h;
> +
> +	if (eth_com_read_reg(ecom, addr, &data_l) ||
> +	    eth_com_read_reg(ecom, addr + 1, &data_h))
> +		return 0xffffffffffffffffULL;
return -1; ?
> +
> +	return data_l + ((u64)data_h << 32);
> +}
> +
> +netdev_tx_t n3000_dummy_netdev_xmit(struct sk_buff *skb,
> +				    struct net_device *dev)
> +{
> +	kfree_skb(skb);
> +	net_warn_ratelimited("%s(): Dropping skb.\n", __func__);
> +	return NETDEV_TX_OK;
> +}
> +
> +static void n3000_netdev_setup(struct net_device *netdev)
> +{
> +	netdev->features = 0;
> +	netdev->hard_header_len = 0;
> +	netdev->priv_flags |= IFF_NO_QUEUE;
> +	netdev->needs_free_netdev = true;
> +	netdev->min_mtu = 0;
> +	netdev->max_mtu = ETH_MAX_MTU;
> +}
> +
> +struct net_device *n3000_netdev_create(struct eth_dev *edev)
> +{
> +	struct dfl_eth_group *egroup = edev->egroup;
> +	struct n3000_net_priv *priv;
> +	struct net_device *netdev;
> +	char name[IFNAMSIZ];
> +
> +	/* The name of n3000 network device is using this format "npacAgBlC"
> +	 *
> +	 * A is the unique ethdev index
> +	 * B is the group id of this ETH Group.
> +	 * C is the PHY/MAC link index for Line side ethernet group.
> +	 */
> +	snprintf(name, IFNAMSIZ, "npac%%dg%ul%u",
> +		 egroup->group_id, edev->index);
> +
> +	netdev = alloc_netdev(sizeof(*priv), name, NET_NAME_UNKNOWN,
> +			      n3000_netdev_setup);
> +	if (!netdev)
> +		return NULL;
> +
> +	priv = netdev_priv(netdev);
> +	priv->edev = edev;
> +	SET_NETDEV_DEV(netdev, egroup->dev);
> +
> +	return netdev;
> +}
> +
> +enum n3000_eth_cfg {
> +	ETH_CONFIG_8x10G,
Since these are important, should explicitly set.
> +	ETH_CONFIG_4x25G,
> +	ETH_CONFIG_2x1x25G,
> +	ETH_CONFIG_4x25G_2x25G,
> +	ETH_CONFIG_2x2x25G,
> +	ETH_CONFIG_MAX
> +};
> +
> +#define N3000_EDEV_MAX 8
> +
> +static int phy_addr_table[ETH_CONFIG_MAX][N3000_EDEV_MAX] = {
> +	/* 8x10G configuration
> +	 *
> +	 *    [retimer_dev]   <------->   [eth_dev]
> +	 *	  0			   0
> +	 *	  1			   1
> +	 *	  2			   2
> +	 *	  3			   3
> +	 *	  4			   4
> +	 *	  5			   5
> +	 *	  6			   6
> +	 *	  7			   7
> +	 */
> +	[ETH_CONFIG_8x10G] = {0, 1, 2, 3, 4, 5, 6, 7},
> +
> +	/* 4x25G and 4x25G_2x25G configuration
> +	 *
> +	 *    [retimer_dev]   <------->   [eth_dev]
> +	 *	  0			   0
> +	 *	  1			   1
> +	 *	  2			   2
> +	 *	  3			   3
> +	 *	  4
> +	 *	  5
> +	 *	  6
> +	 *	  7
> +	 */
> +	[ETH_CONFIG_4x25G] = {0, 1, 2, 3, -1, -1, -1, -1},
> +	[ETH_CONFIG_4x25G_2x25G] = {0, 1, 2, 3, -1, -1, -1, -1},
> +
> +	/* 2x1x25G configuration
> +	 *
> +	 *    [retimer_dev]   <------->   [eth_dev]
> +	 *        0                      0
> +	 *        1
> +	 *        2
> +	 *        3
> +	 *        4                      1
> +	 *        5
> +	 *        6
> +	 *        7
> +	 */
> +	[ETH_CONFIG_2x1x25G] = {0, 4, -1, -1, -1, -1, -1, -1},
> +
> +	/* 2x2x25G configuration
> +	 *
> +	 *    [retimer_dev]   <------->   [eth_dev]
> +	 *	  0			   0
> +	 *	  1			   1
> +	 *	  2
> +	 *	  3
> +	 *	  4			   2
> +	 *	  5			   3
> +	 *	  6
> +	 *	  7
> +	 */
> +	[ETH_CONFIG_2x2x25G] = {0, 1, 4, 5, -1, -1, -1, -1},
> +};
> +
> +#define eth_group_for_each_dev(edev, egp) \
> +	for ((edev) = (egp)->edevs; (edev) < (egp)->edevs + (egp)->num_edevs; \
> +	     (edev)++)
> +
> +#define eth_group_reverse_each_dev(edev, egp) \
> +	for ((edev)--; (edev) >= (egp)->edevs; (edev)--)

Continuing to ask about another layer of abstraction,

Are these really needed ?

> +
> +static struct mii_bus *eth_group_get_phy_bus(struct dfl_eth_group *egroup)
> +{
> +	char mii_name[MII_BUS_ID_SIZE];
> +	struct device *base_dev;
> +	struct mii_bus *bus;
> +
> +	base_dev = dfl_dev_get_base_dev(egroup->dfl_dev);
> +	if (!base_dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	snprintf(mii_name, MII_BUS_ID_SIZE, DFL_ETH_MII_ID_FMT,
> +		 dev_name(base_dev));
> +
> +	bus = mdio_find_bus(mii_name);
> +	if (!bus)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return bus;
> +}
> +
> +static int eth_dev_get_phy_id(struct eth_dev *edev, struct mii_bus *bus)
> +{
> +	struct dfl_eth_group *egroup = edev->egroup;
> +	struct phy_device *phydev;
> +	int phyaddr;
> +
> +	phyaddr = phy_addr_table[egroup->config][edev->index];
> +	if (phyaddr < 0)
> +		return -ENODEV;
> +
> +	phydev = mdiobus_get_phy(bus, phyaddr);
> +	if (!phydev) {
> +		dev_err(egroup->dev, "fail to get phydev\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	strncpy(edev->phy_id, phydev_name(phydev), MII_BUS_ID_SIZE + 3);

+ 2

Since with + 3, the next statement will overwrite.

> +	edev->phy_id[MII_BUS_ID_SIZE + 2] = '\0';
> +
> +	return 0;
> +}
> +
> +static int init_lineside_eth_devs(struct dfl_eth_group *egroup,
> +				  struct mii_bus *phy_bus)
> +{
> +	struct eth_dev *edev;
> +	int ret = 0;
> +
> +	if (!egroup->ops->lineside_init)
> +		return -ENODEV;
> +
> +	eth_group_for_each_dev(edev, egroup) {
> +		ret = eth_dev_get_phy_id(edev, phy_bus);
> +		if (ret)
> +			break;
> +
> +		ret = egroup->ops->lineside_init(edev);
> +		if (ret)
> +			break;

maybe change to

goto err;

and skip the if-check.

> +	}
> +
> +	if (!ret)
> +		return 0;
> +

return 0;


err:
> +	dev_err(egroup->dev, "failed to init lineside edev %d", edev->index);
> +
> +	if (egroup->ops->lineside_remove)
> +		eth_group_reverse_each_dev(edev, egroup)
> +			egroup->ops->lineside_remove(edev);
> +
> +	return ret;
> +}
> +
> +static void remove_lineside_eth_devs(struct dfl_eth_group *egroup)
> +{
> +	struct eth_dev *edev;
> +
> +	if (!egroup->ops->lineside_remove)
> +		return;
> +
> +	eth_group_for_each_dev(edev, egroup)
> +		egroup->ops->lineside_remove(edev);
> +}
> +
> +#define ETH_GROUP_INFO		0x8
> +#define LIGHT_WEIGHT_MAC	BIT_ULL(25)
> +#define INFO_DIRECTION		BIT_ULL(24)
> +#define INFO_SPEED		GENMASK_ULL(23, 16)
> +#define INFO_PHY_NUM		GENMASK_ULL(15, 8)
> +#define INFO_GROUP_ID		GENMASK_ULL(7, 0)
> +
> +#define ETH_GROUP_CTRL		0x10
> +#define CTRL_CMD		GENMASK_ULL(63, 62)
> +#define CMD_NOP			0
> +#define CMD_RD			1
> +#define CMD_WR			2
> +#define CTRL_DEV_SELECT		GENMASK_ULL(53, 49)
> +#define CTRL_FEAT_SELECT	BIT_ULL(48)
> +#define SELECT_IP		0
> +#define SELECT_FEAT		1
> +#define CTRL_ADDR		GENMASK_ULL(47, 32)
> +#define CTRL_WR_DATA		GENMASK_ULL(31, 0)
> +
> +#define ETH_GROUP_STAT		0x18
> +#define STAT_RW_VAL		BIT_ULL(32)
> +#define STAT_RD_DATA		GENMASK_ULL(31, 0)
> +
> +enum ecom_type {
> +	ETH_GROUP_PHY	= 1,
> +	ETH_GROUP_MAC,
> +	ETH_GROUP_ETHER
> +};
> +
> +struct eth_com {
> +	struct dfl_eth_group *egroup;
> +	unsigned int type;
> +	u8 select;
> +};

Should have better prefixes and be closer to the top of the files.

similar problem below.

> +
> +static const char *eth_com_type_string(enum ecom_type type)
> +{
> +	switch (type) {
> +	case ETH_GROUP_PHY:
> +		return "phy";
> +	case ETH_GROUP_MAC:
> +		return "mac";
> +	case ETH_GROUP_ETHER:
> +		return "ethernet wrapper";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +#define eth_com_base(com)	((com)->egroup->base)
> +#define eth_com_dev(com)	((com)->egroup->dev)
> +
> +#define RW_VAL_INVL		1 /* us */
> +#define RW_VAL_POLL_TIMEOUT	10 /* us */
> +
> +static int __do_eth_com_write_reg(struct eth_com *ecom, bool add_feature,
> +				  u16 addr, u32 data)
> +{
This is only called do_eth_com_write_reg, so should be just be part of the caller.
> +	void __iomem *base = eth_com_base(ecom);
> +	struct device *dev = eth_com_dev(ecom);
> +	u64 v = 0;
> +
> +	dev_dbg(dev, "%s [%s] select 0x%x add_feat %d addr 0x%x data 0x%x\n",
> +		__func__, eth_com_type_string(ecom->type),
> +		ecom->select, add_feature, addr, data);
> +
> +	/* only PHY has additional feature registers */
> +	if (add_feature && ecom->type != ETH_GROUP_PHY)
> +		return -EINVAL;
> +
> +	v |= FIELD_PREP(CTRL_CMD, CMD_WR);
> +	v |= FIELD_PREP(CTRL_DEV_SELECT, ecom->select);
> +	v |= FIELD_PREP(CTRL_ADDR, addr);
> +	v |= FIELD_PREP(CTRL_WR_DATA, data);
> +	v |= FIELD_PREP(CTRL_FEAT_SELECT, !!add_feature);
> +
> +	writeq(v, base + ETH_GROUP_CTRL);
> +
> +	if (readq_poll_timeout(base + ETH_GROUP_STAT, v, v & STAT_RW_VAL,
> +			       RW_VAL_INVL, RW_VAL_POLL_TIMEOUT))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int __do_eth_com_read_reg(struct eth_com *ecom, bool add_feature,
> +				 u16 addr, u32 *data)
> +{
> +	void __iomem *base = eth_com_base(ecom);
> +	struct device *dev = eth_com_dev(ecom);
> +	u64 v = 0;
> +
> +	dev_dbg(dev, "%s [%s] select %x add_feat %d addr %x\n",
> +		__func__, eth_com_type_string(ecom->type),
> +		ecom->select, add_feature, addr);
> +
> +	/* only PHY has additional feature registers */
> +	if (add_feature && ecom->type != ETH_GROUP_PHY)
> +		return -EINVAL;
> +
> +	v |= FIELD_PREP(CTRL_CMD, CMD_RD);
> +	v |= FIELD_PREP(CTRL_DEV_SELECT, ecom->select);
> +	v |= FIELD_PREP(CTRL_ADDR, addr);
> +	v |= FIELD_PREP(CTRL_FEAT_SELECT, !!add_feature);
> +
> +	writeq(v, base + ETH_GROUP_CTRL);
> +
> +	if (readq_poll_timeout(base + ETH_GROUP_STAT, v, v & STAT_RW_VAL,
> +			       RW_VAL_INVL, RW_VAL_POLL_TIMEOUT))
> +		return -ETIMEDOUT;
> +
> +	*data = FIELD_GET(STAT_RD_DATA, v);
> +
> +	return 0;
> +}
> +
> +int do_eth_com_write_reg(struct eth_com *ecom, bool add_feature,
> +			 u16 addr, u32 data)
> +{
> +	int ret;
> +
> +	mutex_lock(&ecom->egroup->reg_lock);
> +	ret = __do_eth_com_write_reg(ecom, add_feature, addr, data);
> +	mutex_unlock(&ecom->egroup->reg_lock);
> +	return ret;
> +}
> +
> +int do_eth_com_read_reg(struct eth_com *ecom, bool add_feature,
> +			u16 addr, u32 *data)
> +{
> +	int ret;
> +
> +	mutex_lock(&ecom->egroup->reg_lock);
> +	ret = __do_eth_com_read_reg(ecom, add_feature, addr, data);
> +	mutex_unlock(&ecom->egroup->reg_lock);
> +	return ret;
> +}
> +
> +static struct eth_com *
> +eth_com_create(struct dfl_eth_group *egroup, enum ecom_type type,
> +	       unsigned int link_idx)
> +{
> +	struct eth_com *ecom;
> +
> +	ecom = devm_kzalloc(egroup->dev, sizeof(*ecom), GFP_KERNEL);
> +	if (!ecom)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ecom->egroup = egroup;
> +	ecom->type = type;
> +
> +	if (type == ETH_GROUP_PHY)
> +		ecom->select = link_idx * 2 + 2;
> +	else if (type == ETH_GROUP_MAC)
> +		ecom->select = link_idx * 2 + 3;
> +	else if (type == ETH_GROUP_ETHER)
should change the else-if to else or catch the invalid input earlier.
> +		ecom->select = 0;
> +
> +	return ecom;
> +}
> +
> +static int init_eth_dev(struct eth_dev *edev, struct dfl_eth_group *egroup,
> +			unsigned int link_idx)
> +{
> +	edev->egroup = egroup;
> +	edev->dev = egroup->dev;
> +	edev->index = link_idx;
> +	edev->lw_mac = !!egroup->lw_mac;
> +	edev->phy = eth_com_create(egroup, ETH_GROUP_PHY, link_idx);
> +	if (IS_ERR(edev->phy))
> +		return PTR_ERR(edev->phy);
> +
> +	edev->mac = eth_com_create(egroup, ETH_GROUP_MAC, link_idx);
> +	if (IS_ERR(edev->mac))
kfree(edev->phy)
> +		return PTR_ERR(edev->mac);
> +
> +	return 0;
> +}
> +
> +static int eth_devs_init(struct dfl_eth_group *egroup)
> +{
> +	int ret, i;
> +
> +	egroup->edevs = devm_kcalloc(egroup->dev, egroup->num_edevs,
> +				     sizeof(*egroup->edevs), GFP_KERNEL);
> +	if (!egroup->edevs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < egroup->num_edevs; i++) {
> +		ret = init_eth_dev(&egroup->edevs[i], egroup, i);
> +		if (ret)
free the early successes
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int eth_group_setup(struct dfl_eth_group *egroup)
> +{
> +	int net_cfg, ret;
> +	u64 v;
> +
> +	/* read parameters of this ethernet components group */
> +	v = readq(egroup->base + ETH_GROUP_INFO);
> +
> +	egroup->direction = FIELD_GET(INFO_DIRECTION, v);
> +	egroup->speed = FIELD_GET(INFO_SPEED, v);
> +	egroup->num_edevs = FIELD_GET(INFO_PHY_NUM, v);
> +	egroup->group_id = FIELD_GET(INFO_GROUP_ID, v);
> +	egroup->lw_mac = FIELD_GET(LIGHT_WEIGHT_MAC, v);
> +
> +	net_cfg = dfl_dev_get_vendor_net_cfg(egroup->dfl_dev);
> +	if (net_cfg < 0)
> +		return -EINVAL;
> +
> +	egroup->config = (unsigned int)net_cfg;
should check bounds or will overflow phy_addr_table
> +
> +	ret = eth_devs_init(egroup);
> +	if (ret)
> +		return ret;
> +
> +	switch (egroup->speed) {
> +	case 25:
> +		egroup->ops = &dfl_eth_dev_25g_ops;
> +		break;
> +	case 40:
> +		egroup->ops = &dfl_eth_dev_40g_ops;
> +		break;

default:

  fail();

> +	}
> +
> +	mutex_init(&egroup->reg_lock);
> +
> +	return 0;
> +}
> +
> +static void eth_group_destroy(struct dfl_eth_group *egroup)
> +{
> +	mutex_destroy(&egroup->reg_lock);
> +}
> +
> +static void eth_group_devs_disable(struct dfl_eth_group *egroup)
> +{
> +	struct eth_dev *edev;
> +
> +	eth_group_for_each_dev(edev, egroup)
> +		egroup->ops->reset(edev, true);
> +}
> +
> +static int eth_group_devs_enable(struct dfl_eth_group *egroup)
> +{
> +	struct eth_dev *edev;
> +	int ret;
> +
> +	eth_group_for_each_dev(edev, egroup) {
> +		ret = egroup->ops->reset(edev, false);
> +		if (ret) {
> +			dev_err(egroup->dev, "fail to enable edev%d\n",
> +				edev->index);
> +			eth_group_devs_disable(egroup);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int dfl_eth_group_line_side_init(struct dfl_eth_group *egroup)
earlier function has 'lineside'  use should be consistent.
> +{
> +	struct mii_bus *phy_bus;
> +	int ret;
> +
> +	if (!egroup->ops || !egroup->ops->reset ||
> +	    !egroup->ops->lineside_init || !egroup->ops->lineside_remove)
> +		return -EINVAL;
> +
> +	eth_group_devs_disable(egroup);
> +
> +	phy_bus = eth_group_get_phy_bus(egroup);
> +	if (IS_ERR(phy_bus))
> +		return PTR_ERR(phy_bus);
> +
> +	ret = init_lineside_eth_devs(egroup, phy_bus);
> +	put_device(&phy_bus->dev);
> +
> +	return ret;
> +}
> +
> +static void dfl_eth_group_line_side_uinit(struct dfl_eth_group *egroup)
> +{
> +	remove_lineside_eth_devs(egroup);
remove_lineside_eth_devs only called here, so move function contents to here.
> +}
> +
> +static int dfl_eth_group_host_side_init(struct dfl_eth_group *egroup)
> +{
> +	if (!egroup->ops || !egroup->ops->reset)
> +		return -EINVAL;
> +
> +	return eth_group_devs_enable(egroup);
> +}
> +
> +static int dfl_eth_group_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct dfl_eth_group *egroup;
> +	int ret;
> +
> +	egroup = devm_kzalloc(dev, sizeof(*egroup), GFP_KERNEL);
> +	if (!egroup)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&dfl_dev->dev, egroup);
> +
> +	egroup->dev = dev;
> +	egroup->dfl_dev = dfl_dev;
> +
> +	egroup->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(egroup->base)) {
> +		dev_err(dev, "get mem resource fail!\n");
> +		return PTR_ERR(egroup->base);
> +	}
> +
> +	ret = eth_group_setup(egroup);
> +	if (ret)
> +		return ret;
> +
> +	if (egroup->direction == 1)
1 is magic, should have #define DFL_ETH_GROUP_DIRECTION_LINE/HOST 1/0 or similar
> +		ret = dfl_eth_group_line_side_init(egroup);
> +	else
> +		ret = dfl_eth_group_host_side_init(egroup);
> +
> +	if (!ret)
> +		return 0;
> +
> +	eth_group_destroy(egroup);
> +
> +	return ret;
> +}
> +
> +static void dfl_eth_group_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_eth_group *egroup = dev_get_drvdata(&dfl_dev->dev);
> +
> +	if (egroup->direction == 1)
> +		dfl_eth_group_line_side_uinit(egroup);
> +
> +	eth_group_devs_disable(egroup);
> +	eth_group_destroy(egroup);
> +}
> +
> +#define FME_FEATURE_ID_ETH_GROUP	0x10
> +
> +static const struct dfl_device_id dfl_eth_group_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> +	{ }
> +};
> +
> +static struct dfl_driver dfl_eth_group_driver = {
> +	.drv	= {
> +		.name       = "dfl-eth-group",
> +	},
> +	.id_table = dfl_eth_group_ids,
> +	.probe   = dfl_eth_group_probe,
> +	.remove  = dfl_eth_group_remove,
> +};
> +
> +module_dfl_driver(dfl_eth_group_driver);
> +
> +MODULE_DEVICE_TABLE(dfl, dfl_eth_group_ids);
> +MODULE_DESCRIPTION("DFL ether group driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/net/ethernet/intel/dfl-eth-group.h b/drivers/net/ethernet/intel/dfl-eth-group.h
> new file mode 100644
> index 0000000..2e90f86
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/dfl-eth-group.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/* Internal header file for FPGA DFL Ether Group Driver
> + *
> + * Copyright (C) 2020 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __DFL_ETH_GROUP_H__
> +#define __DFL_ETH_GROUP_H__
This file would not be needed if there was single dfl-eth-group.c ?
> +
> +#include <linux/netdevice.h>
> +#include <linux/phy.h>
> +#include <linux/rtnetlink.h>
> +
> +/* Used when trying to find a virtual mii bus on a specific dfl device.
> + * dev_name(dfl base device)-mii
> + */
> +#define DFL_ETH_MII_ID_FMT "%s-mii"
> +
> +struct eth_dev {
> +	struct dfl_eth_group *egroup;
> +	struct device *dev;
> +	int index;
> +	bool lw_mac;
> +	struct eth_com *phy;
> +	struct eth_com *mac;
> +	struct net_device *netdev;
> +
> +	char phy_id[MII_BUS_ID_SIZE + 3];
This +3 may become problematic, maybe use ARRAY_SIZE() when accessing phy_id
> +};
> +
> +struct eth_dev_ops {
> +	int (*lineside_init)(struct eth_dev *edev);
> +	void (*lineside_remove)(struct eth_dev *edev);
> +	int (*reset)(struct eth_dev *edev, bool en);
> +};
> +
> +struct n3000_net_priv {
> +	struct eth_dev *edev;
> +};
> +
> +static inline struct eth_dev *net_device_to_eth_dev(struct net_device *netdev)
> +{
> +	struct n3000_net_priv *priv = netdev_priv(netdev);
> +
> +	return priv->edev;
> +}
> +
> +struct stat_info {
> +	unsigned int addr;
> +	char string[ETH_GSTRING_LEN];
> +};
> +
> +#define STAT_INFO(_addr, _string) \
> +	.addr = _addr, .string = _string,
> +
> +int do_eth_com_write_reg(struct eth_com *ecom, bool add_feature,
> +			 u16 addr, u32 data);
> +int do_eth_com_read_reg(struct eth_com *ecom, bool add_feature,
> +			u16 addr, u32 *data);
> +
> +#define eth_com_write_reg(ecom, addr, data)	\
> +	do_eth_com_write_reg(ecom, false, addr, data)
> +
> +#define eth_com_read_reg(ecom, addr, data)	\
> +	do_eth_com_read_reg(ecom, false, addr, data)
> +
> +#define eth_com_add_feat_write_reg(ecom, addr, data)	\
> +	do_eth_com_write_reg(ecom, true, addr, data)
> +
> +#define eth_com_add_feat_read_reg(ecom, addr, data)	\
> +	do_eth_com_read_reg(ecom, true, addr, data)
> +
> +

This seems like an unneeded layer of abstraction.

And functions' name maybe too verbose.

Tom

> u64 read_mac_stats(struct eth_com *ecom, unsigned int addr);
> +
> +struct net_device *n3000_netdev_create(struct eth_dev *edev);
> +netdev_tx_t n3000_dummy_netdev_xmit(struct sk_buff *skb,
> +				    struct net_device *dev);
> +
> +extern struct eth_dev_ops dfl_eth_dev_25g_ops;
> +extern struct eth_dev_ops dfl_eth_dev_40g_ops;
> +
> +#endif /* __DFL_ETH_GROUP_H__ */
> diff --git a/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
> index c7b0558..3f686d2 100644
> --- a/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
> +++ b/drivers/net/ethernet/intel/intel-m10-bmc-retimer.c
> @@ -10,6 +10,8 @@
>  #include <linux/phy.h>
>  #include <linux/platform_device.h>
>  
> +#include "dfl-eth-group.h"
> +
>  #define NUM_CHIP	2
>  #define MAX_LINK	4
>  
> @@ -148,7 +150,7 @@ static int m10bmc_retimer_mii_bus_init(struct m10bmc_retimer *retimer)
>  	bus->name = M10BMC_RETIMER_MII_NAME;
>  	bus->read = m10bmc_retimer_read;
>  	bus->write = m10bmc_retimer_write;
> -	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
> +	snprintf(bus->id, MII_BUS_ID_SIZE, DFL_ETH_MII_ID_FMT,
>  		 dev_name(retimer->base_dev));
>  	bus->parent = retimer->dev;
>  	bus->phy_mask = ~(BITS_MASK(retimer->num_devs));

Re: [RFC PATCH 4/6] ethernet: m10-retimer: add support for retimers on Intel MAX 10 BMC

[Tom Rix]

On 10/24/20 9:39 AM, Andrew Lunn wrote:
> On Sat, Oct 24, 2020 at 08:03:51AM -0700, Tom Rix wrote:
>> On 10/23/20 1:45 AM, Xu Yilun wrote:
>>> This driver supports the ethernet retimers (Parkvale) for the Intel PAC
>>> (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
>> Parkvale is a code name, it would be better if the public name was used.
>>
>> As this is a physical chip that could be used on other cards,
>>
>> I think the generic parts should be split out of intel-m10-bmc-retimer.c
>>
>> into a separate file, maybe retimer-c827.c
> This driver is not really a driver for the Parkvale. That driver is
> hidden away in the BMC. So we need to be a bit careful with the name,
> leaving it available for when somebody writes a real Linux driver for
> retimer.

Then the parkvale verbage should be removed.

And the doc ascii diagram be updated with a

+---------+

|  BMC    |

| retimer |

+---------+

Tom

> 	Andrew
>

Re: [RFC PATCH 6/6] ethernet: dfl-eth-group: add support for the 10G configurations

[Tom Rix]

On 10/23/20 1:45 AM, Xu Yilun wrote:
> This patch adds 10G configurations support for dfl ether group private
> feature.
>
> 10G configurations have different PHY & MAC IP blocks from 25G, so a
> different set of HW operations is implemented, but the software arch is
> quite similar with 25G.

Yes, it looks like functions only differ by a #defined register.

So 25 & 10 should be refactored to have a common base functions.

Tom

>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  drivers/net/ethernet/intel/Makefile             |   2 +-
>  drivers/net/ethernet/intel/dfl-eth-group-10g.c  | 544 ++++++++++++++++++++++++
>  drivers/net/ethernet/intel/dfl-eth-group-main.c |   3 +
>  drivers/net/ethernet/intel/dfl-eth-group.h      |   1 +
>  4 files changed, 549 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/intel/dfl-eth-group-10g.c
>
> diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
> index 1624c26..be097656 100644
> --- a/drivers/net/ethernet/intel/Makefile
> +++ b/drivers/net/ethernet/intel/Makefile
> @@ -17,6 +17,6 @@ obj-$(CONFIG_IAVF) += iavf/
>  obj-$(CONFIG_FM10K) += fm10k/
>  obj-$(CONFIG_ICE) += ice/
>  
> -dfl-eth-group-objs := dfl-eth-group-main.o dfl-eth-group-25g.o
> +dfl-eth-group-objs := dfl-eth-group-main.o dfl-eth-group-10g.o dfl-eth-group-25g.o
>  obj-$(CONFIG_FPGA_DFL_ETH_GROUP) += dfl-eth-group.o
>  obj-$(CONFIG_INTEL_M10_BMC_RETIMER) += intel-m10-bmc-retimer.o
> diff --git a/drivers/net/ethernet/intel/dfl-eth-group-10g.c b/drivers/net/ethernet/intel/dfl-eth-group-10g.c
> new file mode 100644
> index 0000000..36f9dfc
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/dfl-eth-group-10g.c
> @@ -0,0 +1,544 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for 10G Ether Group private feature on Intel PAC (Programmable
> + * Acceleration Card) N3000
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Wu Hao <hao.wu@intel.com>
> + *   Xu Yilun <yilun.xu@intel.com>
> + */
> +#include <linux/netdevice.h>
> +
> +#include "dfl-eth-group.h"
> +
> +/* 10G PHY Register */
> +#define PHY_LOOPBACK		0x2e1
> +#define PHY_LOOPBACK_SERIAL	BIT(0)
> +
> +/* 10G MAC Register */
> +#define TX_FRAME_MAXLENGTH	0x2c
> +#define TX_PAUSE_FRAME_QUANTA	0x42
> +#define TX_PAUSE_FRAME_HOLDOFF	0x43
> +#define TX_PAUSE_FRAME_EN	0x44
> +#define TX_PAUSE_FRAME_EN_CFG	BIT(0)
> +#define RX_FRAME_MAXLENGTH	0xae
> +
> +/* Additional Feature Register */
> +#define ADD_PHY_CTRL		0x0
> +#define PHY_RESET		BIT(0)
> +
> +static int edev10g_reset(struct eth_dev *edev, bool en)
> +{
> +	struct eth_com *phy = edev->phy;
> +	struct device *dev = edev->dev;
> +	u32 val;
> +	int ret;
> +
> +	/* 10G eth group supports PHY reset. It uses external reset. */
> +	ret = eth_com_add_feat_read_reg(phy, ADD_PHY_CTRL, &val);
> +	if (ret) {
> +		dev_err(dev, "fail to read ADD_PHY_CTRL reg: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* return if PHY is already in expected state */
> +	if ((val & PHY_RESET && en) || (!(val & PHY_RESET) && !en))
> +		return 0;
> +
> +	if (en)
> +		val |= PHY_RESET;
> +	else
> +		val &= ~PHY_RESET;
> +
> +	ret = eth_com_add_feat_write_reg(phy, ADD_PHY_CTRL, val);
> +	if (ret)
> +		dev_err(dev, "fail to write ADD_PHY_CTRL reg: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static ssize_t tx_pause_frame_quanta_show(struct device *d,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
> +	u32 data;
> +	int ret;
> +
> +	ret = eth_com_read_reg(edev->mac, TX_PAUSE_FRAME_QUANTA, &data);
> +
> +	return ret ? : sprintf(buf, "0x%x\n", data);
> +}
> +
> +static ssize_t tx_pause_frame_quanta_store(struct device *d,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(d);
> +	struct eth_dev *edev;
> +	u32 data;
> +	int ret;
> +
> +	if (kstrtou32(buf, 0, &data))
> +		return -EINVAL;
> +
> +	edev = net_device_to_eth_dev(netdev);
> +
> +	rtnl_lock();
> +
> +	if (netif_running(netdev)) {
> +		netdev_err(netdev, "must be stopped to change pause param\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = eth_com_write_reg(edev->mac, TX_PAUSE_FRAME_QUANTA, data);
> +
> +out:
> +	rtnl_unlock();
> +
> +	return ret ? : len;
> +}
> +static DEVICE_ATTR_RW(tx_pause_frame_quanta);
> +
> +static ssize_t tx_pause_frame_holdoff_show(struct device *d,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(to_net_dev(d));
> +	u32 data;
> +	int ret;
> +
> +	ret = eth_com_read_reg(edev->mac, TX_PAUSE_FRAME_HOLDOFF, &data);
> +
> +	return ret ? : sprintf(buf, "0x%x\n", data);
> +}
> +
> +static ssize_t tx_pause_frame_holdoff_store(struct device *d,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(d);
> +	struct eth_dev *edev;
> +	u32 data;
> +	int ret;
> +
> +	if (kstrtou32(buf, 0, &data))
> +		return -EINVAL;
> +
> +	edev = net_device_to_eth_dev(netdev);
> +
> +	rtnl_lock();
> +
> +	if (netif_running(netdev)) {
> +		netdev_err(netdev, "must be stopped to change pause param\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = eth_com_write_reg(edev->mac, TX_PAUSE_FRAME_HOLDOFF, data);
> +
> +out:
> +	rtnl_unlock();
> +
> +	return ret ? : len;
> +}
> +static DEVICE_ATTR_RW(tx_pause_frame_holdoff);
> +
> +static struct attribute *edev10g_dev_attrs[] = {
> +	&dev_attr_tx_pause_frame_quanta.attr,
> +	&dev_attr_tx_pause_frame_holdoff.attr,
> +	NULL
> +};
> +
> +/* device attributes */
> +static const struct attribute_group edev10g_attr_group = {
> +	.attrs = edev10g_dev_attrs,
> +};
> +
> +/* ethtool ops */
> +static struct stat_info stats_10g[] = {
> +	/* TX Statistics */
> +	{STAT_INFO(0x142, "tx_frame_ok")},
> +	{STAT_INFO(0x144, "tx_frame_err")},
> +	{STAT_INFO(0x146, "tx_frame_crc_err")},
> +	{STAT_INFO(0x148, "tx_octets_ok")},
> +	{STAT_INFO(0x14a, "tx_pause_mac_ctrl_frames")},
> +	{STAT_INFO(0x14c, "tx_if_err")},
> +	{STAT_INFO(0x14e, "tx_unicast_frame_ok")},
> +	{STAT_INFO(0x150, "tx_unicast_frame_err")},
> +	{STAT_INFO(0x152, "tx_multicast_frame_ok")},
> +	{STAT_INFO(0x154, "tx_multicast_frame_err")},
> +	{STAT_INFO(0x156, "tx_broadcast_frame_ok")},
> +	{STAT_INFO(0x158, "tx_broadcast_frame_err")},
> +	{STAT_INFO(0x15a, "tx_ether_octets")},
> +	{STAT_INFO(0x15c, "tx_ether_pkts")},
> +	{STAT_INFO(0x15e, "tx_ether_undersize_pkts")},
> +	{STAT_INFO(0x160, "tx_ether_oversize_pkts")},
> +	{STAT_INFO(0x162, "tx_ether_pkts_64_octets")},
> +	{STAT_INFO(0x164, "tx_ether_pkts_65_127_octets")},
> +	{STAT_INFO(0x166, "tx_ether_pkts_128_255_octets")},
> +	{STAT_INFO(0x168, "tx_ether_pkts_256_511_octets")},
> +	{STAT_INFO(0x16a, "tx_ether_pkts_512_1023_octets")},
> +	{STAT_INFO(0x16c, "tx_ether_pkts_1024_1518_octets")},
> +	{STAT_INFO(0x16e, "tx_ether_pkts_1519_x_octets")},
> +	{STAT_INFO(0x176, "tx_unicast_mac_ctrl_frames")},
> +	{STAT_INFO(0x178, "tx_multicast_mac_ctrl_frames")},
> +	{STAT_INFO(0x17a, "tx_broadcast_mac_ctrl_frames")},
> +	{STAT_INFO(0x17c, "tx_pfc_mac_ctrl_frames")},
> +
> +	/* RX Statistics */
> +	{STAT_INFO(0x1c2, "rx_frame_ok")},
> +	{STAT_INFO(0x1c4, "rx_frame_err")},
> +	{STAT_INFO(0x1c6, "rx_frame_crc_err")},
> +	{STAT_INFO(0x1c8, "rx_octets_ok")},
> +	{STAT_INFO(0x1ca, "rx_pause_mac_ctrl_frames")},
> +	{STAT_INFO(0x1cc, "rx_if_err")},
> +	{STAT_INFO(0x1ce, "rx_unicast_frame_ok")},
> +	{STAT_INFO(0x1d0, "rx_unicast_frame_err")},
> +	{STAT_INFO(0x1d2, "rx_multicast_frame_ok")},
> +	{STAT_INFO(0x1d4, "rx_multicast_frame_err")},
> +	{STAT_INFO(0x1d6, "rx_broadcast_frame_ok")},
> +	{STAT_INFO(0x1d8, "rx_broadcast_frame_err")},
> +	{STAT_INFO(0x1da, "rx_ether_octets")},
> +	{STAT_INFO(0x1dc, "rx_ether_pkts")},
> +	{STAT_INFO(0x1de, "rx_ether_undersize_pkts")},
> +	{STAT_INFO(0x1e0, "rx_ether_oversize_pkts")},
> +	{STAT_INFO(0x1e2, "rx_ether_pkts_64_octets")},
> +	{STAT_INFO(0x1e4, "rx_ether_pkts_65_127_octets")},
> +	{STAT_INFO(0x1e6, "rx_ether_pkts_128_255_octets")},
> +	{STAT_INFO(0x1e8, "rx_ether_pkts_256_511_octets")},
> +	{STAT_INFO(0x1ea, "rx_ether_pkts_512_1023_octets")},
> +	{STAT_INFO(0x1ec, "rx_ether_pkts_1024_1518_octets")},
> +	{STAT_INFO(0x1ee, "rx_ether_pkts_1519_x_octets")},
> +	{STAT_INFO(0x1f0, "rx_ether_fragments")},
> +	{STAT_INFO(0x1f2, "rx_ether_jabbers")},
> +	{STAT_INFO(0x1f4, "rx_ether_crc_err")},
> +	{STAT_INFO(0x1f6, "rx_unicast_mac_ctrl_frames")},
> +	{STAT_INFO(0x1f8, "rx_multicast_mac_ctrl_frames")},
> +	{STAT_INFO(0x1fa, "rx_broadcast_mac_ctrl_frames")},
> +	{STAT_INFO(0x1fc, "rx_pfc_mac_ctrl_frames")},
> +};
> +
> +static void edev10g_get_strings(struct net_device *netdev, u32 stringset, u8 *s)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	unsigned int i;
> +
> +	if (stringset != ETH_SS_STATS || edev->lw_mac)
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(stats_10g); i++, s += ETH_GSTRING_LEN)
> +		memcpy(s, stats_10g[i].string, ETH_GSTRING_LEN);
> +}
> +
> +static int edev10g_get_sset_count(struct net_device *netdev, int stringset)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +
> +	if (stringset != ETH_SS_STATS || edev->lw_mac)
> +		return -EOPNOTSUPP;
> +
> +	return (int)ARRAY_SIZE(stats_10g);
> +}
> +
> +static void edev10g_get_stats(struct net_device *netdev,
> +			      struct ethtool_stats *stats, u64 *data)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	unsigned int i;
> +
> +	if (edev->lw_mac || !netif_running(netdev))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(stats_10g); i++)
> +		data[i] = read_mac_stats(edev->mac, stats_10g[i].addr);
> +}
> +
> +static int edev10g_get_link_ksettings(struct net_device *netdev,
> +				      struct ethtool_link_ksettings *cmd)
> +{
> +	if (!netdev->phydev)
> +		return -ENODEV;
> +
> +	phy_ethtool_ksettings_get(netdev->phydev, cmd);
> +
> +	return 0;
> +}
> +
> +static void edev10g_get_pauseparam(struct net_device *netdev,
> +				   struct ethtool_pauseparam *pause)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	u32 data;
> +	int ret;
> +
> +	pause->autoneg = 0;
> +	pause->rx_pause = 0;
> +
> +	ret = eth_com_read_reg(edev->mac, TX_PAUSE_FRAME_EN, &data);
> +	if (ret) {
> +		pause->tx_pause = 0;
> +		return;
> +	}
> +
> +	pause->tx_pause = (data & TX_PAUSE_FRAME_EN_CFG) ? 0x1 : 0;
> +}
> +
> +static int edev10g_set_pauseparam(struct net_device *netdev,
> +				  struct ethtool_pauseparam *pause)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	struct eth_com *mac = edev->mac;
> +	bool enable = pause->tx_pause;
> +	u32 data;
> +	int ret;
> +
> +	if (pause->autoneg || pause->rx_pause)
> +		return -EOPNOTSUPP;
> +
> +	if (netif_running(netdev)) {
> +		netdev_err(netdev, "must be stopped to change pause param\n");
> +		return -EBUSY;
> +	}
> +
> +	ret = eth_com_read_reg(mac, TX_PAUSE_FRAME_EN, &data);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		data |= TX_PAUSE_FRAME_EN_CFG;
> +	else
> +		data &= ~TX_PAUSE_FRAME_EN_CFG;
> +
> +	return eth_com_write_reg(mac, TX_PAUSE_FRAME_EN, data);
> +}
> +
> +static const struct ethtool_ops edev10g_ethtool_ops = {
> +	.get_link = ethtool_op_get_link,
> +	.get_strings = edev10g_get_strings,
> +	.get_sset_count = edev10g_get_sset_count,
> +	.get_ethtool_stats = edev10g_get_stats,
> +	.get_link_ksettings = edev10g_get_link_ksettings,
> +	.get_pauseparam = edev10g_get_pauseparam,
> +	.set_pauseparam = edev10g_set_pauseparam,
> +};
> +
> +/* netdev ops */
> +static int edev10g_netdev_open(struct net_device *netdev)
> +{
> +	struct n3000_net_priv *priv = netdev_priv(netdev);
> +	struct eth_dev *edev = priv->edev;
> +	int ret;
> +
> +	ret = edev10g_reset(edev, false);
> +	if (ret)
> +		return ret;
> +
> +	if (netdev->phydev)
> +		phy_start(netdev->phydev);
> +
> +	return 0;
> +}
> +
> +static int edev10g_netdev_stop(struct net_device *netdev)
> +{
> +	struct n3000_net_priv *priv = netdev_priv(netdev);
> +	struct eth_dev *edev = priv->edev;
> +	int ret;
> +
> +	ret = edev10g_reset(edev, true);
> +	if (ret)
> +		return ret;
> +
> +	if (netdev->phydev)
> +		phy_stop(netdev->phydev);
> +
> +	return 0;
> +}
> +
> +static int edev10g_mtu_init(struct net_device *netdev)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	struct eth_com *mac = edev->mac;
> +	u32 tx = 0, rx = 0, mtu;
> +	int ret;
> +
> +	ret = eth_com_read_reg(mac, TX_FRAME_MAXLENGTH, &tx);
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_com_read_reg(mac, RX_FRAME_MAXLENGTH, &rx);
> +	if (ret)
> +		return ret;
> +
> +	mtu = min(min(tx, rx), netdev->max_mtu);
> +
> +	ret = eth_com_write_reg(mac, TX_FRAME_MAXLENGTH, rx);
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_com_write_reg(mac, RX_FRAME_MAXLENGTH, tx);
> +	if (ret)
> +		return ret;
> +
> +	netdev->mtu = mtu;
> +
> +	return 0;
> +}
> +
> +static int edev10g_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	struct eth_com *mac = edev->mac;
> +	int ret;
> +
> +	if (netif_running(netdev)) {
> +		netdev_err(netdev, "must be stopped to change mtu\n");
> +		return -EBUSY;
> +	}
> +
> +	ret = eth_com_write_reg(mac, TX_FRAME_MAXLENGTH, new_mtu);
> +	if (ret)
> +		return ret;
> +
> +	ret = eth_com_write_reg(mac, RX_FRAME_MAXLENGTH, new_mtu);
> +	if (ret)
> +		return ret;
> +
> +	netdev->mtu = new_mtu;
> +
> +	return 0;
> +}
> +
> +static int edev10g_set_loopback(struct net_device *netdev, bool en)
> +{
> +	struct eth_dev *edev = net_device_to_eth_dev(netdev);
> +	struct eth_com *phy = edev->phy;
> +	u32 data;
> +	int ret;
> +
> +	ret = eth_com_read_reg(phy, PHY_LOOPBACK, &data);
> +	if (ret)
> +		return ret;
> +
> +	if (en)
> +		data |= PHY_LOOPBACK_SERIAL;
> +	else
> +		data &= ~PHY_LOOPBACK_SERIAL;
> +
> +	return eth_com_write_reg(phy, PHY_LOOPBACK, data);
> +}
> +
> +static int edev10g_set_features(struct net_device *netdev,
> +				netdev_features_t features)
> +{
> +	netdev_features_t changed = netdev->features ^ features;
> +
> +	if (changed & NETIF_F_LOOPBACK)
> +		return edev10g_set_loopback(netdev,
> +					    !!(features & NETIF_F_LOOPBACK));
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops edev10g_netdev_ops = {
> +	.ndo_open = edev10g_netdev_open,
> +	.ndo_stop = edev10g_netdev_stop,
> +	.ndo_change_mtu = edev10g_change_mtu,
> +	.ndo_set_features = edev10g_set_features,
> +	.ndo_start_xmit = n3000_dummy_netdev_xmit,
> +};
> +
> +static void edev10g_adjust_link(struct net_device *netdev)
> +{}
> +
> +static int edev10g_netdev_init(struct net_device *netdev)
> +{
> +	int ret;
> +
> +	ret = edev10g_mtu_init(netdev);
> +	if (ret)
> +		return ret;
> +
> +	return edev10g_set_loopback(netdev,
> +				   !!(netdev->features & NETIF_F_LOOPBACK));
> +}
> +
> +static int dfl_eth_dev_10g_init(struct eth_dev *edev)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +	struct device *dev = edev->dev;
> +	struct phy_device *phydev;
> +	struct net_device *netdev;
> +	int ret;
> +
> +	netdev = n3000_netdev_create(edev);
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	netdev->hw_features |= NETIF_F_LOOPBACK;
> +	netdev->netdev_ops = &edev10g_netdev_ops;
> +	netdev->ethtool_ops = &edev10g_ethtool_ops;
> +
> +	phydev = phy_connect(netdev, edev->phy_id, edev10g_adjust_link,
> +			     PHY_INTERFACE_MODE_NA);
> +	if (IS_ERR(phydev)) {
> +		dev_err(dev, "PHY connection failed\n");
> +		ret = PTR_ERR(phydev);
> +		goto err_free_netdev;
> +	}
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, mask);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, mask);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, mask);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> +	linkmode_and(phydev->supported, phydev->supported, mask);
> +	linkmode_copy(phydev->advertising, phydev->supported);
> +
> +	phy_attached_info(phydev);
> +
> +	ret = edev10g_netdev_init(netdev);
> +	if (ret) {
> +		dev_err(dev, "fail to init netdev %s\n", netdev->name);
> +		goto err_phy_disconnect;
> +	}
> +
> +	netdev->sysfs_groups[0] = &edev10g_attr_group;
> +
> +	netif_carrier_off(netdev);
> +	ret = register_netdev(netdev);
> +	if (ret) {
> +		dev_err(dev, "fail to register netdev %s\n", netdev->name);
> +		goto err_phy_disconnect;
> +	}
> +
> +	edev->netdev = netdev;
> +
> +	return 0;
> +
> +err_phy_disconnect:
> +	if (netdev->phydev)
> +		phy_disconnect(phydev);
> +err_free_netdev:
> +	free_netdev(netdev);
> +
> +	return ret;
> +}
> +
> +static void dfl_eth_dev_10g_remove(struct eth_dev *edev)
> +{
> +	struct net_device *netdev = edev->netdev;
> +
> +	if (netdev->phydev)
> +		phy_disconnect(netdev->phydev);
> +
> +	unregister_netdev(netdev);
> +}
> +
> +struct eth_dev_ops dfl_eth_dev_10g_ops = {
> +	.lineside_init = dfl_eth_dev_10g_init,
> +	.lineside_remove = dfl_eth_dev_10g_remove,
> +	.reset = edev10g_reset,
> +};
> diff --git a/drivers/net/ethernet/intel/dfl-eth-group-main.c b/drivers/net/ethernet/intel/dfl-eth-group-main.c
> index a29b8b1..89b4450 100644
> --- a/drivers/net/ethernet/intel/dfl-eth-group-main.c
> +++ b/drivers/net/ethernet/intel/dfl-eth-group-main.c
> @@ -481,6 +481,9 @@ static int eth_group_setup(struct dfl_eth_group *egroup)
>  		return ret;
>  
>  	switch (egroup->speed) {
> +	case 10:
> +		egroup->ops = &dfl_eth_dev_10g_ops;
> +		break;
>  	case 25:
>  		egroup->ops = &dfl_eth_dev_25g_ops;
>  		break;
> diff --git a/drivers/net/ethernet/intel/dfl-eth-group.h b/drivers/net/ethernet/intel/dfl-eth-group.h
> index 2e90f86..63f49a0 100644
> --- a/drivers/net/ethernet/intel/dfl-eth-group.h
> +++ b/drivers/net/ethernet/intel/dfl-eth-group.h
> @@ -77,6 +77,7 @@ struct net_device *n3000_netdev_create(struct eth_dev *edev);
>  netdev_tx_t n3000_dummy_netdev_xmit(struct sk_buff *skb,
>  				    struct net_device *dev);
>  
> +extern struct eth_dev_ops dfl_eth_dev_10g_ops;
>  extern struct eth_dev_ops dfl_eth_dev_25g_ops;
>  extern struct eth_dev_ops dfl_eth_dev_40g_ops;
>  

Re: [RFC PATCH 4/6] ethernet: m10-retimer: add support for retimers on Intel MAX 10 BMC

[Andrew Lunn]

On Sat, Oct 24, 2020 at 10:36:36AM -0700, Tom Rix wrote:
> 
> On 10/24/20 9:39 AM, Andrew Lunn wrote:
> > On Sat, Oct 24, 2020 at 08:03:51AM -0700, Tom Rix wrote:
> >> On 10/23/20 1:45 AM, Xu Yilun wrote:
> >>> This driver supports the ethernet retimers (Parkvale) for the Intel PAC
> >>> (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> >> Parkvale is a code name, it would be better if the public name was used.
> >>
> >> As this is a physical chip that could be used on other cards,
> >>
> >> I think the generic parts should be split out of intel-m10-bmc-retimer.c
> >>
> >> into a separate file, maybe retimer-c827.c
> > This driver is not really a driver for the Parkvale. That driver is
> > hidden away in the BMC. So we need to be a bit careful with the name,
> > leaving it available for when somebody writes a real Linux driver for
> > retimer.
> 
> Then the parkvale verbage should be removed.
> 
> And the doc ascii diagram be updated with a
> 
> +---------+
> 
> |  BMC    |
> 
> | retimer |
> 
> +---------+

Yes, i would like to get a better understanding of what the BMC is
actually doing, particularly the SFP. Given my current limited
understanding of the driver architecture, i'm not sure it is flexiable
enough to handle other cards where Linux is controlling the SFPs, the
retimer, the host side PCS of the Arria 10, etc. Once Linux is driving
all that, you need phylink, not phylib. The proposed phylib
driver/mdio bus driver actually looks like a hack.

   Andrew

Re: [RFC PATCH 5/6] ethernet: dfl-eth-group: add DFL eth group private feature driver

[Andrew Lunn]

> > +u64 read_mac_stats(struct eth_com *ecom, unsigned int addr)
> > +{
> > +	u32 data_l, data_h;
> > +
> > +	if (eth_com_read_reg(ecom, addr, &data_l) ||
> > +	    eth_com_read_reg(ecom, addr + 1, &data_h))
> > +		return 0xffffffffffffffffULL;

> return -1; ?

Since this is a u64 function, i expect you get a compiler
warning. Maybe only with W=1. It is better to use U64_MAX.

	 Andrew

RE: [RFC PATCH 2/6] fpga: dfl: export network configuration info for DFL based FPGA

[Wu, Hao]

> Subject: [RFC PATCH 2/6] fpga: dfl: export network configuration info for DFL
> based FPGA
> 
> This patch makes preparation for supporting DFL Ether Group private
> feature driver, which reads bitstream_id.vendor_net_cfg field to
> determin the interconnection of network components on FPGA device.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/dfl-fme-main.c | 10 ++--------
>  drivers/fpga/dfl.c          | 21 +++++++++++++++++++++
>  drivers/fpga/dfl.h          | 12 ++++++++++++
>  include/linux/dfl.h         |  2 ++
>  4 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 77ea04d..a2b8ba0 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -46,14 +46,8 @@ static DEVICE_ATTR_RO(ports_num);
>  static ssize_t bitstream_id_show(struct device *dev,
>  				 struct device_attribute *attr, char *buf)
>  {
> -	void __iomem *base;
> -	u64 v;
> -
> -	base = dfl_get_feature_ioaddr_by_id(dev,
> FME_FEATURE_ID_HEADER);
> -
> -	v = readq(base + FME_HDR_BITSTREAM_ID);
> -
> -	return scnprintf(buf, PAGE_SIZE, "0x%llx\n", (unsigned long long)v);
> +	return scnprintf(buf, PAGE_SIZE, "0x%llx\n",
> +			 (unsigned long long)dfl_get_bitstream_id(dev));
>  }
>  static DEVICE_ATTR_RO(bitstream_id);
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index bc35750..ca3c678 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -537,6 +537,27 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv)
>  }
>  EXPORT_SYMBOL(dfl_driver_unregister);
> 
> +int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev)
> +{
> +	struct device *fme_dev;
> +	u64 v;
> +
> +	if (!dfl_dev)
> +		return -EINVAL;
> +
> +	if (dfl_dev->type == FME_ID)
> +		fme_dev = dfl_dev->dev.parent;
> +	else
> +		fme_dev = dfl_dev->cdev->fme_dev;

All of them have cdev, is my understanding correct?
If so, why handle it differently here?

> +
> +	if (!fme_dev)
> +		return -EINVAL;

ENODEV?

> +
> +	v = dfl_get_bitstream_id(fme_dev);
> +	return (int)FIELD_GET(FME_BID_VENDOR_NET_CFG, v);
> +}
> +EXPORT_SYMBOL_GPL(dfl_dev_get_vendor_net_cfg);
> +
>  #define is_header_feature(feature) ((feature)->id ==
> FEATURE_ID_FIU_HEADER)
> 
>  /**
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2b82c96..6c7a6961 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -104,6 +104,9 @@
>  #define FME_CAP_CACHE_SIZE	GENMASK_ULL(43, 32)	/* cache size
> in KB */
>  #define FME_CAP_CACHE_ASSOC	GENMASK_ULL(47, 44)	/*
> Associativity */
> 
> +/* FME BITSTREAM_ID Register Bitfield */

Bitstream ID, same style as others.

> +#define FME_BID_VENDOR_NET_CFG	GENMASK_ULL(35, 32)     /* vendor
> net cfg */
> +
>  /* FME Port Offset Register Bitfield */
>  /* Offset to port device feature header */
>  #define FME_PORT_OFST_DFH_OFST	GENMASK_ULL(23, 0)
> @@ -397,6 +400,15 @@ static inline bool is_dfl_feature_present(struct
> device *dev, u16 id)
>  	return !!dfl_get_feature_ioaddr_by_id(dev, id);
>  }
> 
> +static inline u64 dfl_get_bitstream_id(struct device *dev)
> +{
> +	void __iomem *base;
> +
> +	base = dfl_get_feature_ioaddr_by_id(dev,
> FME_FEATURE_ID_HEADER);
> +
> +	return readq(base + FME_HDR_BITSTREAM_ID);
> +}
> +
>  static inline
>  struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data
> *pdata)
>  {
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index e1b2471..5ee2b1e 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -67,6 +67,8 @@ struct dfl_driver {
>  #define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
>  #define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
> 
> +int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev);

It seems the vendor net configuration can be provided by a
vendor specific method. So bid_vendor_net_cfg maybe a better name?

Thanks
Hao

> +
>  /*
>   * use a macro to avoid include chaining to get THIS_MODULE.
>   */
> --
> 2.7.4

RE: [RFC PATCH 3/6] fpga: dfl: add an API to get the base device for dfl device

[Wu, Hao]

> Subject: [RFC PATCH 3/6] fpga: dfl: add an API to get the base device for dfl
> device
> 
> This patch adds an API for dfl devices to find which physical device
> owns the DFL.
> 
> This patch makes preparation for supporting DFL Ether Group private
> feature driver. It uses this information to determine which retimer
> device physically connects to which ether group.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/dfl.c  | 9 +++++++++
>  include/linux/dfl.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index ca3c678..52d18e6 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -558,6 +558,15 @@ int dfl_dev_get_vendor_net_cfg(struct dfl_device
> *dfl_dev)
>  }
>  EXPORT_SYMBOL_GPL(dfl_dev_get_vendor_net_cfg);
> 
> +struct device *dfl_dev_get_base_dev(struct dfl_device *dfl_dev)
> +{
> +	if (!dfl_dev || !dfl_dev->cdev)
> +		return NULL;
> +
> +	return dfl_dev->cdev->parent;
> +}
> +EXPORT_SYMBOL_GPL(dfl_dev_get_base_dev);

It may confuse people that this get doesn't require a put operation on
the base device. could we avoid this by using a different name?

And why it needs to know the physical device here? DFL hides the
physical device for upper layer drivers, so this is not the same case?
or cdev can be used instead here?

Thanks
Hao

> +
>  #define is_header_feature(feature) ((feature)->id ==
> FEATURE_ID_FIU_HEADER)
> 
>  /**
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 5ee2b1e..dd313f2 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -68,6 +68,7 @@ struct dfl_driver {
>  #define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
> 
>  int dfl_dev_get_vendor_net_cfg(struct dfl_device *dfl_dev);
> +struct device *dfl_dev_get_base_dev(struct dfl_device *dfl_dev);
> 
>  /*
>   * use a macro to avoid include chaining to get THIS_MODULE.
> --
> 2.7.4

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Xu Yilun]

Hi Andrew

Thanks for your fast response, see comments inline.

On Fri, Oct 23, 2020 at 05:37:31PM +0200, Andrew Lunn wrote:
> Hi Xu
> 
> Before i look at the other patches, i want to understand the
> architecture properly.

I have a doc to describe the architecture:

https://www.intel.com/content/www/us/en/programmable/documentation/xgz1560360700260.html

The "Figure 1" is a more detailed figure for the arch. It should be
helpful.

> 
> > +=======================================================================
> > +DFL device driver for Ether Group private feature on Intel(R) PAC N3000
> > +=======================================================================
> > +
> > +This is the driver for Ether Group private feature on Intel(R)
> > +PAC (Programmable Acceleration Card) N3000.
> 
> I assume this is just one implementation. The FPGA could be placed on
> other boards. So some of the limitations you talk about with the BMC
> artificial, and the overall architecture of the drivers is more
> generic?

I could see if the retimer management is changed, e.g. access the retimer
through a host controlled MDIO, maybe I need a more generic way to find the
MDIO bus.

Do you have other suggestions?

> 
> > +The Intel(R) PAC N3000 is a FPGA based SmartNIC platform for multi-workload
> > +networking application acceleration. A simple diagram below to for the board:
> > +
> > +                     +----------------------------------------+
> > +                     |                  FPGA                  |
> > ++----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
> > +|QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
> > ++----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
> > +                     |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
> > +                     +-----------+  |offloading|  +-----------+   +----------+
> > +                     |              +----------+              |
> > +                     |                                        |
> > +                     +----------------------------------------+
> 
> Is XL710 required? I assume any MAC with the correct MII interface
> will work?

The XL710 is required for this implementation, in which we have the Host
Side Ether Group facing the host.  The Host Side Ether Group actually
contains the same IP blocks as Line Side. It contains the compacted MAC &
PHY functionalities for 25G/40G case. The 25G MAC-PHY soft IP SPEC can
be found at:

https://www.intel.com/content/www/us/en/programmable/documentation/ewo1447742896786.html

So raw serial data is output from Host Side FPGA, and XL710 is good to
handle this.

> 
> Do you really mean PHY? I actually expect it is PCS? 

For this implementation, yes.

I guess if you program another IP block on FPGA host side, e.g. a PCS interface,
and replace XL710 with another MAC, it may also work. But I think there should
be other drivers to handle this.

I may contact with our Hardware designer if there is some concern we
don't use MII for connection of FPGA & Host.

The FPGA User is mainly concerned about the user logic part. The Ether
Groups in FIU and Board components are not expected to be re-designed by
the user. So I think I should still focus on the driver for this
implementation.

> 
> > +The DFL Ether Group driver registers netdev for each line side link. Users
> > +could use standard commands (ethtool, ip, ifconfig) for configuration and
> > +link state/statistics reading. For host side links, they are always connected
> > +to the host ethernet controller, so they should always have same features as
> > +the host ethernet controller. There is no need to register netdevs for them.
> 
> So lets say the XL710 is eth0. The line side netif is eth1. Where do i
> put the IP address? What interface do i add to quagga OSPF? 

The IP address should be put in eth0. eth0 should always be used for the
tools.

The line/host side Ether Group is not the terminal of the network data stream.
Eth1 will not paticipate in the network data exchange to host.

The main purposes for eth1 are:
1. For users to monitor the network statistics on Line Side, and by comparing the
statistics between eth0 & eth1, users could get some knowledge of how the User
logic is taking function.

2. Get the link state of the front panel. The XL710 is now connected to
Host Side of the FPGA and the its link state would be always on. So to
check the link state of the front panel, we need to query eth1.

> 
> > +The driver just enables these links on probe.
> > +
> > +The retimer chips are managed by onboard BMC (Board Management Controller)
> > +firmware, host driver is not capable to access them directly.
> 
> What about the QSPF socket? Can the host get access to the I2C bus?
> The pins for TX enable, etc. ethtool -m?

No, the QSPF/I2C are also managed by the BMC firmware, and host doesn't
have interface to talk to BMC firmware about QSPF.

> 
> > +Speed/Duplex
> > +------------
> > +The Ether Group doesn't support auto-negotiation. The link speed is fixed to
> > +10G, 25G or 40G full duplex according to which Ether Group IP is programmed.
> 
> So that means, if i pop out the SFP and put in a different one which
> supports a different speed, it is expected to be broken until the FPGA
> is reloaded?

It is expected to be broken.

Now the line side is expected to be configured to 4x10G, 4x25G, 2x25G, 1x25G.
host side is expected to be 4x10G or 2x40G for XL710.

So 4 channel SFP is expected to be inserted to front panel. And we should use
4x25G SFP, which is compatible to 4x10G connection.

Thanks,
Yilun

> 
>      Andrew

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Andrew Lunn]

> > > +The Intel(R) PAC N3000 is a FPGA based SmartNIC platform for multi-workload
> > > +networking application acceleration. A simple diagram below to for the board:
> > > +
> > > +                     +----------------------------------------+
> > > +                     |                  FPGA                  |
> > > ++----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
> > > +|QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
> > > ++----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
> > > +                     |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
> > > +                     +-----------+  |offloading|  +-----------+   +----------+
> > > +                     |              +----------+              |
> > > +                     |                                        |
> > > +                     +----------------------------------------+
> > 
> > Is XL710 required? I assume any MAC with the correct MII interface
> > will work?
> 
> The XL710 is required for this implementation, in which we have the Host
> Side Ether Group facing the host.  The Host Side Ether Group actually
> contains the same IP blocks as Line Side. It contains the compacted MAC &
> PHY functionalities for 25G/40G case. The 25G MAC-PHY soft IP SPEC can
> be found at:
> 
> https://www.intel.com/content/www/us/en/programmable/documentation/ewo1447742896786.html
> 
> So raw serial data is output from Host Side FPGA, and XL710 is good to
> handle this.

What i have seen working with Marvell Ethernet switches, is that
Marvell normally recommends connecting them to the Ethernet interfaces
of Marvell SoCs. But the switch just needs a compatible MII interface,
and lots of boards make use of non-Marvell MAC chips. Freescale FEC is
very popular.

What i'm trying to say is that ideally we need a collection of generic
drivers for the different major components on the board, and a board
driver which glues it all together. That then allows somebody to build
other boards, or integrate the FPGA directly into an embedded system
directly connected to a SoC, etc.

> > Do you really mean PHY? I actually expect it is PCS? 
> 
> For this implementation, yes.

Yes, you have a PHY? Or Yes, it is PCS?

To me, the phylib maintainer, having a PHY means you have a base-T
interface, 25Gbase-T, 40Gbase-T?  That would be an odd and expensive
architecture when you should be able to just connect SERDES interfaces
together.

> > > +The DFL Ether Group driver registers netdev for each line side link. Users
> > > +could use standard commands (ethtool, ip, ifconfig) for configuration and
> > > +link state/statistics reading. For host side links, they are always connected
> > > +to the host ethernet controller, so they should always have same features as
> > > +the host ethernet controller. There is no need to register netdevs for them.
> > 
> > So lets say the XL710 is eth0. The line side netif is eth1. Where do i
> > put the IP address? What interface do i add to quagga OSPF? 
> 
> The IP address should be put in eth0. eth0 should always be used for the
> tools.

That was what i was afraid of :-)

> 
> The line/host side Ether Group is not the terminal of the network data stream.
> Eth1 will not paticipate in the network data exchange to host.
> 
> The main purposes for eth1 are:
> 1. For users to monitor the network statistics on Line Side, and by comparing the
> statistics between eth0 & eth1, users could get some knowledge of how the User
> logic is taking function.
> 
> 2. Get the link state of the front panel. The XL710 is now connected to
> Host Side of the FPGA and the its link state would be always on. So to
> check the link state of the front panel, we need to query eth1.

This is very non-intuitive. We try to avoid this in the kernel and the
API to userspace. Ethernet switches are always modelled as
accelerators for what the Linux network stack can already do. You
configure an Ethernet switch port in just the same way configure any
other netdev. You add an IP address to the switch port, you get the
Ethernet statistics from the switch port, routing protocols use the
switch port.

You design needs to be the same. All configuration needs to happen via
eth1.

Please look at the DSA architecture. What you have here is very
similar to a two port DSA switch. In DSA terminology, we would call
eth0 the master interface.  It needs to be up, but otherwise the user
does not configure it. eth1 is the slave interface. It is the user
facing interface of the switch. All configuration happens on this
interface. Linux can also send/receive packets on this netdev. The
slave TX function forwards the frame to the master interface netdev,
via a DSA tagger. Frames which eth0 receive are passed through the
tagger and then passed to the slave interface.

All the infrastructure you need is already in place. Please use
it. I'm not saying you need to write a DSA driver, but you should make
use of the same ideas and low level hooks in the network stack which
DSA uses.

> > What about the QSPF socket? Can the host get access to the I2C bus?
> > The pins for TX enable, etc. ethtool -m?
> 
> No, the QSPF/I2C are also managed by the BMC firmware, and host doesn't
> have interface to talk to BMC firmware about QSPF.

So can i even tell what SFP is in the socket? 

> > > +Speed/Duplex
> > > +------------
> > > +The Ether Group doesn't support auto-negotiation. The link speed is fixed to
> > > +10G, 25G or 40G full duplex according to which Ether Group IP is programmed.
> > 
> > So that means, if i pop out the SFP and put in a different one which
> > supports a different speed, it is expected to be broken until the FPGA
> > is reloaded?
> 
> It is expected to be broken.

And since i have no access to the SFP information, i have no idea what
is actually broken? How i should configure the various layers?

> Now the line side is expected to be configured to 4x10G, 4x25G, 2x25G, 1x25G.
> host side is expected to be 4x10G or 2x40G for XL710.
> 
> So 4 channel SFP is expected to be inserted to front panel. And we should use
> 4x25G SFP, which is compatible to 4x10G connection.

So if you had exported the SFP to linux, phylink could of handled some
of this for you. Probably with some extensions to phylink, but Russell
King would of probably helped you. phylink has a good idea how to
decode the SFP EEPROM and figure out the link mode. It has interfaces
to configure PCS blocks, So it could probably deal with the line side
and host side PCS. And it would of been easy to send a udev
notification that the SFP has changed, maybe user space needs to
download a different FPGA bit file? So the user would not see a broken
interface, the hardware could be reconfigured on the fly.

This is one problem i have with this driver. It is based around this
somewhat broken reference design. phylib, along with the hacks you
have, are enough for this reference design. But really you want to
make use of phylink in order to support less limited designs which
will follow. Or you need to push a lot more into the BMC, and don't
use phylib at all.

    Andrew

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Xu Yilun]

On Mon, Oct 26, 2020 at 02:00:01PM +0100, Andrew Lunn wrote:
> > > > +The Intel(R) PAC N3000 is a FPGA based SmartNIC platform for multi-workload
> > > > +networking application acceleration. A simple diagram below to for the board:
> > > > +
> > > > +                     +----------------------------------------+
> > > > +                     |                  FPGA                  |
> > > > ++----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
> > > > +|QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
> > > > ++----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
> > > > +                     |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
> > > > +                     +-----------+  |offloading|  +-----------+   +----------+
> > > > +                     |              +----------+              |
> > > > +                     |                                        |
> > > > +                     +----------------------------------------+
> > > 
> > > Is XL710 required? I assume any MAC with the correct MII interface
> > > will work?
> > 
> > The XL710 is required for this implementation, in which we have the Host
> > Side Ether Group facing the host.  The Host Side Ether Group actually
> > contains the same IP blocks as Line Side. It contains the compacted MAC &
> > PHY functionalities for 25G/40G case. The 25G MAC-PHY soft IP SPEC can
> > be found at:
> > 
> > https://www.intel.com/content/www/us/en/programmable/documentation/ewo1447742896786.html
> > 
> > So raw serial data is output from Host Side FPGA, and XL710 is good to
> > handle this.
> 
> What i have seen working with Marvell Ethernet switches, is that
> Marvell normally recommends connecting them to the Ethernet interfaces
> of Marvell SoCs. But the switch just needs a compatible MII interface,
> and lots of boards make use of non-Marvell MAC chips. Freescale FEC is
> very popular.
> 
> What i'm trying to say is that ideally we need a collection of generic
> drivers for the different major components on the board, and a board
> driver which glues it all together. That then allows somebody to build
> other boards, or integrate the FPGA directly into an embedded system
> directly connected to a SoC, etc.
> 
> > > Do you really mean PHY? I actually expect it is PCS? 
> > 
> > For this implementation, yes.
> 
> Yes, you have a PHY? Or Yes, it is PCS?

Sorry, I mean I have a PHY.

> 
> To me, the phylib maintainer, having a PHY means you have a base-T
> interface, 25Gbase-T, 40Gbase-T?  That would be an odd and expensive
> architecture when you should be able to just connect SERDES interfaces
> together.

I see your concerns about the SERDES interface between FPGA & XL710.

Considering the DSA, we just enable the cpu facing ports, seems the
SERDES interface connection doesn't impact the software. It's just too
expensive.

> 
> > > > +The DFL Ether Group driver registers netdev for each line side link. Users
> > > > +could use standard commands (ethtool, ip, ifconfig) for configuration and
> > > > +link state/statistics reading. For host side links, they are always connected
> > > > +to the host ethernet controller, so they should always have same features as
> > > > +the host ethernet controller. There is no need to register netdevs for them.
> > > 
> > > So lets say the XL710 is eth0. The line side netif is eth1. Where do i
> > > put the IP address? What interface do i add to quagga OSPF? 
> > 
> > The IP address should be put in eth0. eth0 should always be used for the
> > tools.
> 
> That was what i was afraid of :-)
> 
> > 
> > The line/host side Ether Group is not the terminal of the network data stream.
> > Eth1 will not paticipate in the network data exchange to host.
> > 
> > The main purposes for eth1 are:
> > 1. For users to monitor the network statistics on Line Side, and by comparing the
> > statistics between eth0 & eth1, users could get some knowledge of how the User
> > logic is taking function.
> > 
> > 2. Get the link state of the front panel. The XL710 is now connected to
> > Host Side of the FPGA and the its link state would be always on. So to
> > check the link state of the front panel, we need to query eth1.
> 
> This is very non-intuitive. We try to avoid this in the kernel and the
> API to userspace. Ethernet switches are always modelled as
> accelerators for what the Linux network stack can already do. You
> configure an Ethernet switch port in just the same way configure any
> other netdev. You add an IP address to the switch port, you get the
> Ethernet statistics from the switch port, routing protocols use the
> switch port.
> 
> You design needs to be the same. All configuration needs to happen via
> eth1.
> 
> Please look at the DSA architecture. What you have here is very
> similar to a two port DSA switch. In DSA terminology, we would call
> eth0 the master interface.  It needs to be up, but otherwise the user
> does not configure it. eth1 is the slave interface. It is the user
> facing interface of the switch. All configuration happens on this
> interface. Linux can also send/receive packets on this netdev. The
> slave TX function forwards the frame to the master interface netdev,
> via a DSA tagger. Frames which eth0 receive are passed through the
> tagger and then passed to the slave interface.
> 
> All the infrastructure you need is already in place. Please use
> it. I'm not saying you need to write a DSA driver, but you should make
> use of the same ideas and low level hooks in the network stack which
> DSA uses.

I did some investigation about the DSA, and actually I wrote a
experimental DSA driver. It works and almost meets my need, I can make
configuration, run pktgen on slave inf.

A main concern for dsa is the wiring from slave inf to master inf depends
on the user logic. If FPGA users want to make their own user logic, they
may need a new driver. But our original design for the FPGA is, kernel
drivers support the fundamental parts - FPGA FIU (where Ether Group is in)
& other peripherals on board, and userspace direct I/O access for User
logic. Then FPGA user don't have to write & compile a driver for their
user logic change.
It seems not that case for netdev. The user logic is a part of the whole
functionality of the netdev, we cannot split part of the hardware
component to userspace and the rest in kernel. I really need to
reconsider this.

> 
> > > What about the QSPF socket? Can the host get access to the I2C bus?
> > > The pins for TX enable, etc. ethtool -m?
> > 
> > No, the QSPF/I2C are also managed by the BMC firmware, and host doesn't
> > have interface to talk to BMC firmware about QSPF.
> 
> So can i even tell what SFP is in the socket? 

No.

> 
> > > > +Speed/Duplex
> > > > +------------
> > > > +The Ether Group doesn't support auto-negotiation. The link speed is fixed to
> > > > +10G, 25G or 40G full duplex according to which Ether Group IP is programmed.
> > > 
> > > So that means, if i pop out the SFP and put in a different one which
> > > supports a different speed, it is expected to be broken until the FPGA
> > > is reloaded?
> > 
> > It is expected to be broken.
> 
> And since i have no access to the SFP information, i have no idea what
> is actually broken? How i should configure the various layers?

With this hardware implementation, I'm afraid host can not know what is broken.
It can just see the Speed of the slave inf is never changed, and the link state
is "No" on slave inf. Is it like the fixed phy or fixed link mode?

Is it possible just see it as fixed and configure the layers?

> 
> > Now the line side is expected to be configured to 4x10G, 4x25G, 2x25G, 1x25G.
> > host side is expected to be 4x10G or 2x40G for XL710.
> > 
> > So 4 channel SFP is expected to be inserted to front panel. And we should use
> > 4x25G SFP, which is compatible to 4x10G connection.
> 
> So if you had exported the SFP to linux, phylink could of handled some
> of this for you. Probably with some extensions to phylink, but Russell
> King would of probably helped you. phylink has a good idea how to
> decode the SFP EEPROM and figure out the link mode. It has interfaces
> to configure PCS blocks, So it could probably deal with the line side
> and host side PCS. And it would of been easy to send a udev
> notification that the SFP has changed, maybe user space needs to
> download a different FPGA bit file? So the user would not see a broken
> interface, the hardware could be reconfigured on the fly.
> 
> This is one problem i have with this driver. It is based around this
> somewhat broken reference design. phylib, along with the hacks you
> have, are enough for this reference design. But really you want to
> make use of phylink in order to support less limited designs which
> will follow. Or you need to push a lot more into the BMC, and don't
> use phylib at all.

Mm.. seems the hardware should be changed, either let host directly
access the QSFP, or re-design the BMC to provide more info for QSFP.

Is it possible we didn't change the hardware, and we support the
components (QSFP, retimer) by fixed-link mode. I know this makes the
driver specific to the board, but the boards are being used by
customers and I'm trying to make them supported without hardware
changes...


Thanks for your very detailed explaination and guide.
Yilun

> 
>     Andrew

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Jakub Kicinski]

On Tue, 27 Oct 2020 01:38:04 +0800 Xu Yilun wrote:
> > > The line/host side Ether Group is not the terminal of the network data stream.
> > > Eth1 will not paticipate in the network data exchange to host.
> > > 
> > > The main purposes for eth1 are:
> > > 1. For users to monitor the network statistics on Line Side, and by comparing the
> > > statistics between eth0 & eth1, users could get some knowledge of how the User
> > > logic is taking function.
> > > 
> > > 2. Get the link state of the front panel. The XL710 is now connected to
> > > Host Side of the FPGA and the its link state would be always on. So to
> > > check the link state of the front panel, we need to query eth1.  
> > 
> > This is very non-intuitive. We try to avoid this in the kernel and the
> > API to userspace. Ethernet switches are always modelled as
> > accelerators for what the Linux network stack can already do. You
> > configure an Ethernet switch port in just the same way configure any
> > other netdev. You add an IP address to the switch port, you get the
> > Ethernet statistics from the switch port, routing protocols use the
> > switch port.
> > 
> > You design needs to be the same. All configuration needs to happen via
> > eth1.
> > 
> > Please look at the DSA architecture. What you have here is very
> > similar to a two port DSA switch. In DSA terminology, we would call
> > eth0 the master interface.  It needs to be up, but otherwise the user
> > does not configure it. eth1 is the slave interface. It is the user
> > facing interface of the switch. All configuration happens on this
> > interface. Linux can also send/receive packets on this netdev. The
> > slave TX function forwards the frame to the master interface netdev,
> > via a DSA tagger. Frames which eth0 receive are passed through the
> > tagger and then passed to the slave interface.
> > 
> > All the infrastructure you need is already in place. Please use
> > it. I'm not saying you need to write a DSA driver, but you should make
> > use of the same ideas and low level hooks in the network stack which
> > DSA uses.  
> 
> I did some investigation about the DSA, and actually I wrote a
> experimental DSA driver. It works and almost meets my need, I can make
> configuration, run pktgen on slave inf.
> 
> A main concern for dsa is the wiring from slave inf to master inf depends
> on the user logic. If FPGA users want to make their own user logic, they
> may need a new driver. But our original design for the FPGA is, kernel
> drivers support the fundamental parts - FPGA FIU (where Ether Group is in)
> & other peripherals on board, and userspace direct I/O access for User
> logic. Then FPGA user don't have to write & compile a driver for their
> user logic change.
> It seems not that case for netdev. The user logic is a part of the whole
> functionality of the netdev, we cannot split part of the hardware
> component to userspace and the rest in kernel. I really need to
> reconsider this.

This is obviously on purpose. Your design as it stands will not fly
upstream, sorry.

From netdev perspective the user should not care how many hardware
blocks are in the pipeline, and on which piece of silicon. You have 
a 2 port (modulo port splitting) card, there should be 2 netdevs, and
the link config and forwarding should be configured through those.

Please let folks at Intel know that we don't like the "SDK in user
space with reuse [/abuse] of parts of netdev infra" architecture.
This is a second of those we see in a short time. Kernel is not a
library for your SDK to use. 

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Andrew Lunn]

> > > > Do you really mean PHY? I actually expect it is PCS? 
> > > 
> > > For this implementation, yes.
> > 
> > Yes, you have a PHY? Or Yes, it is PCS?
> 
> Sorry, I mean I have a PHY.
> 
> > 
> > To me, the phylib maintainer, having a PHY means you have a base-T
> > interface, 25Gbase-T, 40Gbase-T?  That would be an odd and expensive
> > architecture when you should be able to just connect SERDES interfaces
> > together.

You really have 25Gbase-T, 40Gbase-T? Between the FPGA & XL710?
What copper PHYs are using? 

> I see your concerns about the SERDES interface between FPGA & XL710.

I have no concerns about direct SERDES connections. That is the normal
way of doing this. It keeps it a lot simpler, since you don't have to
worry about driving the PHYs.

> I did some investigation about the DSA, and actually I wrote a
> experimental DSA driver. It works and almost meets my need, I can make
> configuration, run pktgen on slave inf.

Cool. As i said, I don't know if this actually needs to be a DSA
driver. It might just need to borrow some ideas from DSA.

> Mm.. seems the hardware should be changed, either let host directly
> access the QSFP, or re-design the BMC to provide more info for QSFP.

At a minimum, you need to support ethtool -m. It could be a firmware
call to the BMC, our you expose the i2c bus somehow. There are plenty
of MAC drivers which implement eththool -m without using phylink.

But i think you need to take a step back first, and look at the bigger
picture. What is Intel's goal? Are they just going to sell complete
cards? Or do they also want to sell the FPGA as a components anybody
get put onto their own board?

If there are only ever going to be compete cards, then you can go the
firmware direction, push a lot of functionality into the BMC, and have
the card driver make firmware calls to control the SFP, retimer,
etc. You can then throw away your mdio and phy driver hacks.

If however, the FPGA is going to be available as a component, can you
also assume there is a BMC? Running Intel firmware? Can the customer
also modify this firmware for their own needs? I think that is going
to be difficult. So you need to push as much as possible towards
linux, and let Linux drive all the hardware, the SFP, retimer, FPGA,
etc.

	Andrew

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Xu Yilun]

On Mon, Oct 26, 2020 at 11:35:52AM -0700, Jakub Kicinski wrote:
> On Tue, 27 Oct 2020 01:38:04 +0800 Xu Yilun wrote:
> > > > The line/host side Ether Group is not the terminal of the network data stream.
> > > > Eth1 will not paticipate in the network data exchange to host.
> > > > 
> > > > The main purposes for eth1 are:
> > > > 1. For users to monitor the network statistics on Line Side, and by comparing the
> > > > statistics between eth0 & eth1, users could get some knowledge of how the User
> > > > logic is taking function.
> > > > 
> > > > 2. Get the link state of the front panel. The XL710 is now connected to
> > > > Host Side of the FPGA and the its link state would be always on. So to
> > > > check the link state of the front panel, we need to query eth1.  
> > > 
> > > This is very non-intuitive. We try to avoid this in the kernel and the
> > > API to userspace. Ethernet switches are always modelled as
> > > accelerators for what the Linux network stack can already do. You
> > > configure an Ethernet switch port in just the same way configure any
> > > other netdev. You add an IP address to the switch port, you get the
> > > Ethernet statistics from the switch port, routing protocols use the
> > > switch port.
> > > 
> > > You design needs to be the same. All configuration needs to happen via
> > > eth1.
> > > 
> > > Please look at the DSA architecture. What you have here is very
> > > similar to a two port DSA switch. In DSA terminology, we would call
> > > eth0 the master interface.  It needs to be up, but otherwise the user
> > > does not configure it. eth1 is the slave interface. It is the user
> > > facing interface of the switch. All configuration happens on this
> > > interface. Linux can also send/receive packets on this netdev. The
> > > slave TX function forwards the frame to the master interface netdev,
> > > via a DSA tagger. Frames which eth0 receive are passed through the
> > > tagger and then passed to the slave interface.
> > > 
> > > All the infrastructure you need is already in place. Please use
> > > it. I'm not saying you need to write a DSA driver, but you should make
> > > use of the same ideas and low level hooks in the network stack which
> > > DSA uses.  
> > 
> > I did some investigation about the DSA, and actually I wrote a
> > experimental DSA driver. It works and almost meets my need, I can make
> > configuration, run pktgen on slave inf.
> > 
> > A main concern for dsa is the wiring from slave inf to master inf depends
> > on the user logic. If FPGA users want to make their own user logic, they
> > may need a new driver. But our original design for the FPGA is, kernel
> > drivers support the fundamental parts - FPGA FIU (where Ether Group is in)
> > & other peripherals on board, and userspace direct I/O access for User
> > logic. Then FPGA user don't have to write & compile a driver for their
> > user logic change.
> > It seems not that case for netdev. The user logic is a part of the whole
> > functionality of the netdev, we cannot split part of the hardware
> > component to userspace and the rest in kernel. I really need to
> > reconsider this.
> 
> This is obviously on purpose. Your design as it stands will not fly
> upstream, sorry.
> 
> >From netdev perspective the user should not care how many hardware
> blocks are in the pipeline, and on which piece of silicon. You have 
> a 2 port (modulo port splitting) card, there should be 2 netdevs, and
> the link config and forwarding should be configured through those.
> 
> Please let folks at Intel know that we don't like the "SDK in user
> space with reuse [/abuse] of parts of netdev infra" architecture.
> This is a second of those we see in a short time. Kernel is not a
> library for your SDK to use. 

I get your point. I'll share the information internally and reconsider
the design.

Thanks,
Yilun

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Xu Yilun]

On Mon, Oct 26, 2020 at 08:14:00PM +0100, Andrew Lunn wrote:
> > > > > Do you really mean PHY? I actually expect it is PCS? 
> > > > 
> > > > For this implementation, yes.
> > > 
> > > Yes, you have a PHY? Or Yes, it is PCS?
> > 
> > Sorry, I mean I have a PHY.
> > 
> > > 
> > > To me, the phylib maintainer, having a PHY means you have a base-T
> > > interface, 25Gbase-T, 40Gbase-T?  That would be an odd and expensive
> > > architecture when you should be able to just connect SERDES interfaces
> > > together.
> 
> You really have 25Gbase-T, 40Gbase-T? Between the FPGA & XL710?
> What copper PHYs are using? 

Sorry for the confusing. I'll check with our board designer and reply
later.

> 
> > I see your concerns about the SERDES interface between FPGA & XL710.
> 
> I have no concerns about direct SERDES connections. That is the normal
> way of doing this. It keeps it a lot simpler, since you don't have to
> worry about driving the PHYs.
> 
> > I did some investigation about the DSA, and actually I wrote a
> > experimental DSA driver. It works and almost meets my need, I can make
> > configuration, run pktgen on slave inf.
> 
> Cool. As i said, I don't know if this actually needs to be a DSA
> driver. It might just need to borrow some ideas from DSA.
> 
> > Mm.. seems the hardware should be changed, either let host directly
> > access the QSFP, or re-design the BMC to provide more info for QSFP.
> 
> At a minimum, you need to support ethtool -m. It could be a firmware
> call to the BMC, our you expose the i2c bus somehow. There are plenty
> of MAC drivers which implement eththool -m without using phylink.
> 
> But i think you need to take a step back first, and look at the bigger
> picture. What is Intel's goal? Are they just going to sell complete
> cards? Or do they also want to sell the FPGA as a components anybody
> get put onto their own board?
> 
> If there are only ever going to be compete cards, then you can go the
> firmware direction, push a lot of functionality into the BMC, and have
> the card driver make firmware calls to control the SFP, retimer,
> etc. You can then throw away your mdio and phy driver hacks.
> 
> If however, the FPGA is going to be available as a component, can you
> also assume there is a BMC? Running Intel firmware? Can the customer
> also modify this firmware for their own needs? I think that is going
> to be difficult. So you need to push as much as possible towards
> linux, and let Linux drive all the hardware, the SFP, retimer, FPGA,
> etc.

This is a very helpful. I'll share with our team and reconsider about the
design.

Thanks,
Yilun

> 
> 	Andrew
> 

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Xu Yilun]

Hi Andrew:

On Mon, Oct 26, 2020 at 08:14:00PM +0100, Andrew Lunn wrote:
> > > > > Do you really mean PHY? I actually expect it is PCS? 
> > > > 
> > > > For this implementation, yes.
> > > 
> > > Yes, you have a PHY? Or Yes, it is PCS?
> > 
> > Sorry, I mean I have a PHY.
> > 
> > > 
> > > To me, the phylib maintainer, having a PHY means you have a base-T
> > > interface, 25Gbase-T, 40Gbase-T?  That would be an odd and expensive
> > > architecture when you should be able to just connect SERDES interfaces
> > > together.
> 
> You really have 25Gbase-T, 40Gbase-T? Between the FPGA & XL710?
> What copper PHYs are using? 
> 
> > I see your concerns about the SERDES interface between FPGA & XL710.
> 
> I have no concerns about direct SERDES connections. That is the normal
> way of doing this. It keeps it a lot simpler, since you don't have to
> worry about driving the PHYs.
>

I did some investigation and now I have some details.
The term 'PHY' described in Ether Group Spec should be the PCS + PMA, a figure
below for one configuration:

 +------------------------+          +-----------------+
 | Host Side Ether Group  |          |      XL710      |
 |                        |          |                 |
 | +--------------------+ |          |                 |
 | | 40G Ether IP       | |          |                 |
 | |                    | |          |                 |
 | |       +---------+  | |  XLAUI   |                 |
 | | MAC - |PCS - PMA|  | |----------| PMA - PCS - MAC |
 | |       +---------+  | |          |                 |
 +-+--------------------+-+          +-----------------+

Thanks,
Yilun

Re: [RFC PATCH 1/6] docs: networking: add the document for DFL Ether Group driver

[Andrew Lunn]

> I did some investigation and now I have some details.
> The term 'PHY' described in Ether Group Spec should be the PCS + PMA, a figure
> below for one configuration:
> 
>  +------------------------+          +-----------------+
>  | Host Side Ether Group  |          |      XL710      |
>  |                        |          |                 |
>  | +--------------------+ |          |                 |
>  | | 40G Ether IP       | |          |                 |
>  | |                    | |          |                 |
>  | |       +---------+  | |  XLAUI   |                 |
>  | | MAC - |PCS - PMA|  | |----------| PMA - PCS - MAC |
>  | |       +---------+  | |          |                 |
>  +-+--------------------+-+          +-----------------+

Thanks, that makes a lot more sense.

	Andrew