[PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC

[PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC

[Xu Yilun]

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

The 2 retimer chips connect to the Intel MAX 10 BMC on the card. They are
managed by the BMC firmware. Host could query their link states and
firmware version information via retimer interfaces (Shared registers) on
the BMC. The driver creates sysfs interfaces for users to query these
information.


Xu Yilun (2):
  mfd: intel-m10-bmc: specify the retimer sub devices
  misc: add support for retimers interfaces on Intel MAX 10 BMC

 .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
 drivers/mfd/intel-m10-bmc.c                        |  19 ++-
 drivers/misc/Kconfig                               |  10 ++
 drivers/misc/Makefile                              |   1 +
 drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h                  |   7 +
 6 files changed, 226 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
 create mode 100644 drivers/misc/intel-m10-bmc-retimer.c

-- 
2.7.4

[PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices

[Xu Yilun]

The patch specifies the 2 retimer sub devices and their resources in the
parent driver's mfd_cell. It also adds the register definition of the
retimer sub devices.

There are 2 ethernet retimer chips (C827) connected to the Intel MAX 10
BMC. They are managed by the BMC firmware, and host could query them via
retimer interfaces (shared registers) on the BMC. The 2 retimers have
identical register interfaces in different register addresses or fields,
so it is better we define 2 retimer devices and handle them with the same
driver.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/mfd/intel-m10-bmc.c       | 19 ++++++++++++++++++-
 include/linux/mfd/intel-m10-bmc.h |  7 +++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index b84579b..e0a99a0 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -17,9 +17,26 @@ enum m10bmc_type {
 	M10_N3000,
 };
 
+static struct resource retimer0_resources[] = {
+	{M10BMC_PKVL_A_VER, M10BMC_PKVL_A_VER, "version", IORESOURCE_REG, },
+};
+
+static struct resource retimer1_resources[] = {
+	{M10BMC_PKVL_B_VER, M10BMC_PKVL_B_VER, "version", IORESOURCE_REG, },
+};
+
 static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
 	{ .name = "n3000bmc-hwmon" },
-	{ .name = "n3000bmc-retimer" },
+	{
+		.name = "n3000bmc-retimer",
+		.num_resources = ARRAY_SIZE(retimer0_resources),
+		.resources = retimer0_resources,
+	},
+	{
+		.name = "n3000bmc-retimer",
+		.num_resources = ARRAY_SIZE(retimer1_resources),
+		.resources = retimer1_resources,
+	},
 	{ .name = "n3000bmc-secure" },
 };
 
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index c8ef2f1..d6216f9 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -21,6 +21,13 @@
 #define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
 #define M10BMC_VER_LEGACY_INVALID	0xffffffff
 
+/* Retimer related registers, in system register region */
+#define M10BMC_PKVL_LSTATUS		0x164
+#define M10BMC_PKVL_A_VER		0x254
+#define M10BMC_PKVL_B_VER		0x258
+#define M10BMC_PKVL_SERDES_VER		GENMASK(15, 0)
+#define M10BMC_PKVL_SBUS_VER		GENMASK(31, 16)
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
-- 
2.7.4

[PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Xu Yilun]

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

C827 is an Intel(R) Ethernet serdes transceiver chip that supports
up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
10G/25G retimer mode. Host could query their link states and firmware
version information via retimer interfaces (Shared registers) on Intel
MAX 10 BMC. The driver creates sysfs interfaces for users to query these
information.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
 drivers/misc/Kconfig                               |  10 ++
 drivers/misc/Makefile                              |   1 +
 drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
 4 files changed, 201 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
 create mode 100644 drivers/misc/intel-m10-bmc-retimer.c

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
new file mode 100644
index 0000000..528712a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
@@ -0,0 +1,32 @@
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/tag
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the tag of the retimer chip. Now there are 2
+		retimer chips on Intel PAC N3000, they are tagged as
+		'retimer_A' and 'retimer_B'.
+		Format: "retimer_%c".
+
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/sbus_version
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the Transceiver bus firmware version of
+		the retimer chip.
+		Format: "0x%04x".
+
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/serdes_version
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the SERDES firmware version of the retimer
+		chip.
+		Format: "0x%04x".
+
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the status of each line side link. "1" for
+		link up, "0" for link down.
+		Format: "%u".
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0..7cb9433 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -466,6 +466,16 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config INTEL_M10_BMC_RETIMER
+	tristate "Intel(R) MAX 10 BMC ethernet retimer interface support"
+	depends on MFD_INTEL_M10_BMC
+	help
+	  This driver supports the ethernet retimer (C827) 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.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d23231e..67883cf 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
 obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_INTEL_M10_BMC_RETIMER)	+= intel-m10-bmc-retimer.o
diff --git a/drivers/misc/intel-m10-bmc-retimer.c b/drivers/misc/intel-m10-bmc-retimer.c
new file mode 100644
index 0000000..d845342b
--- /dev/null
+++ b/drivers/misc/intel-m10-bmc-retimer.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 BMC Retimer Interface Driver
+ *
+ * Copyright (C) 2021 Intel Corporation, Inc.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define N3000BMC_RETIMER_DEV_NAME "n3000bmc-retimer"
+
+struct m10bmc_retimer {
+	struct device *dev;
+	struct intel_m10bmc *m10bmc;
+	u32 ver_reg;
+	u32 id;
+};
+
+static ssize_t tag_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "retimer_%c\n", 'A' + retimer->id);
+}
+static DEVICE_ATTR_RO(tag);
+
+static ssize_t sbus_version_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(retimer->m10bmc, retimer->ver_reg, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%04x\n",
+			  (u16)FIELD_GET(M10BMC_PKVL_SBUS_VER, val));
+}
+static DEVICE_ATTR_RO(sbus_version);
+
+static ssize_t serdes_version_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(retimer->m10bmc, retimer->ver_reg, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%04x\n",
+			  (u16)FIELD_GET(M10BMC_PKVL_SERDES_VER, val));
+}
+static DEVICE_ATTR_RO(serdes_version);
+
+struct link_attr {
+	struct device_attribute attr;
+	u32 index;
+};
+
+#define to_link_attr(dev_attr) \
+	container_of(dev_attr, struct link_attr, attr)
+
+static ssize_t
+link_status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+	struct link_attr *lattr = to_link_attr(attr);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(retimer->m10bmc, M10BMC_PKVL_LSTATUS, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n",
+			  !!(val & BIT((retimer->id << 2) + lattr->index)));
+}
+
+#define link_status_attr(_index)				\
+	static struct link_attr link_attr_status##_index =	\
+		{ .attr = __ATTR(link_status##_index, 0444,	\
+				 link_status_show, NULL),	\
+		  .index = (_index) }
+
+link_status_attr(0);
+link_status_attr(1);
+link_status_attr(2);
+link_status_attr(3);
+
+static struct attribute *m10bmc_retimer_attrs[] = {
+	&dev_attr_tag.attr,
+	&dev_attr_sbus_version.attr,
+	&dev_attr_serdes_version.attr,
+	&link_attr_status0.attr.attr,
+	&link_attr_status1.attr.attr,
+	&link_attr_status2.attr.attr,
+	&link_attr_status3.attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(m10bmc_retimer);
+
+static int intel_m10bmc_retimer_probe(struct platform_device *pdev)
+{
+	struct intel_m10bmc *m10bmc = dev_get_drvdata(pdev->dev.parent);
+	struct m10bmc_retimer *retimer;
+	struct resource *res;
+
+	retimer = devm_kzalloc(&pdev->dev, sizeof(*retimer), GFP_KERNEL);
+	if (!retimer)
+		return -ENOMEM;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_REG, "version");
+	if (!res) {
+		dev_err(&pdev->dev, "No REG resource for version\n");
+		return -EINVAL;
+	}
+
+	/* find the id of the retimer via the addr of the version register */
+	if (res->start == M10BMC_PKVL_A_VER) {
+		retimer->id = 0;
+	} else if (res->start == M10BMC_PKVL_B_VER) {
+		retimer->id = 1;
+	} else {
+		dev_err(&pdev->dev, "version REG resource invalid\n");
+		return -EINVAL;
+	}
+
+	retimer->ver_reg = res->start;
+	retimer->dev = &pdev->dev;
+	retimer->m10bmc = m10bmc;
+
+	dev_set_drvdata(&pdev->dev, retimer);
+
+	return 0;
+}
+
+static struct platform_driver intel_m10bmc_retimer_driver = {
+	.probe = intel_m10bmc_retimer_probe,
+	.driver = {
+		.name = N3000BMC_RETIMER_DEV_NAME,
+		.dev_groups = m10bmc_retimer_groups,
+	},
+};
+module_platform_driver(intel_m10bmc_retimer_driver);
+
+MODULE_ALIAS("platform:" N3000BMC_RETIMER_DEV_NAME);
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel MAX 10 BMC retimer driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Greg KH]

On Wed, Jan 06, 2021 at 03:36:07PM +0800, Xu Yilun wrote:
> This driver supports the ethernet retimers (C827) for the Intel PAC
> (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> 
> C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> 10G/25G retimer mode. Host could query their link states and firmware
> version information via retimer interfaces (Shared registers) on Intel
> MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> information.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
>  drivers/misc/Kconfig                               |  10 ++
>  drivers/misc/Makefile                              |   1 +
>  drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
>  4 files changed, 201 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
>  create mode 100644 drivers/misc/intel-m10-bmc-retimer.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> new file mode 100644
> index 0000000..528712a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> @@ -0,0 +1,32 @@
> +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/tag
> +Date:		Jan 2021
> +KernelVersion:	5.12
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the tag of the retimer chip. Now there are 2
> +		retimer chips on Intel PAC N3000, they are tagged as
> +		'retimer_A' and 'retimer_B'.
> +		Format: "retimer_%c".
> +
> +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/sbus_version
> +Date:		Jan 2021
> +KernelVersion:	5.12
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the Transceiver bus firmware version of
> +		the retimer chip.
> +		Format: "0x%04x".
> +
> +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/serdes_version
> +Date:		Jan 2021
> +KernelVersion:	5.12
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the SERDES firmware version of the retimer
> +		chip.
> +		Format: "0x%04x".
> +
> +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> +Date:		Jan 2021
> +KernelVersion:	5.12
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the status of each line side link. "1" for
> +		link up, "0" for link down.
> +		Format: "%u".

Who is going to use all of these read-only attributes?

And why isn't this information exported in the "normal" way for network
devices?  Having them as custom sysfs attributes ensures that no
existing tools will work with these at all, right?  Why not do the
standard thing here isntead?

thanks,

greg k-h

Re: [PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices

[Lee Jones]

On Wed, 06 Jan 2021, Xu Yilun wrote:

> The patch specifies the 2 retimer sub devices and their resources in the
> parent driver's mfd_cell. It also adds the register definition of the
> retimer sub devices.
> 
> There are 2 ethernet retimer chips (C827) connected to the Intel MAX 10
> BMC. They are managed by the BMC firmware, and host could query them via
> retimer interfaces (shared registers) on the BMC. The 2 retimers have
> identical register interfaces in different register addresses or fields,
> so it is better we define 2 retimer devices and handle them with the same
> driver.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/mfd/intel-m10-bmc.c       | 19 ++++++++++++++++++-
>  include/linux/mfd/intel-m10-bmc.h |  7 +++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> index b84579b..e0a99a0 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -17,9 +17,26 @@ enum m10bmc_type {
>  	M10_N3000,
>  };
>  
> +static struct resource retimer0_resources[] = {
> +	{M10BMC_PKVL_A_VER, M10BMC_PKVL_A_VER, "version", IORESOURCE_REG, },
> +};
> +
> +static struct resource retimer1_resources[] = {
> +	{M10BMC_PKVL_B_VER, M10BMC_PKVL_B_VER, "version", IORESOURCE_REG, },
> +};

Please use the DEFINE_RES_*() helpers for this.

>  static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
>  	{ .name = "n3000bmc-hwmon" },
> -	{ .name = "n3000bmc-retimer" },
> +	{
> +		.name = "n3000bmc-retimer",
> +		.num_resources = ARRAY_SIZE(retimer0_resources),
> +		.resources = retimer0_resources,
> +	},
> +	{
> +		.name = "n3000bmc-retimer",
> +		.num_resources = ARRAY_SIZE(retimer1_resources),
> +		.resources = retimer1_resources,
> +	},
>  	{ .name = "n3000bmc-secure" },
>  };
>  
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index c8ef2f1..d6216f9 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -21,6 +21,13 @@
>  #define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
>  #define M10BMC_VER_LEGACY_INVALID	0xffffffff
>  
> +/* Retimer related registers, in system register region */
> +#define M10BMC_PKVL_LSTATUS		0x164
> +#define M10BMC_PKVL_A_VER		0x254
> +#define M10BMC_PKVL_B_VER		0x258
> +#define M10BMC_PKVL_SERDES_VER		GENMASK(15, 0)
> +#define M10BMC_PKVL_SBUS_VER		GENMASK(31, 16)
> +
>  /**
>   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
>   * @dev: this device

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Xu Yilun]

On Wed, Jan 06, 2021 at 08:56:42AM +0100, Greg KH wrote:
> On Wed, Jan 06, 2021 at 03:36:07PM +0800, Xu Yilun wrote:
> > This driver supports the ethernet retimers (C827) for the Intel PAC
> > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > 
> > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > 10G/25G retimer mode. Host could query their link states and firmware
> > version information via retimer interfaces (Shared registers) on Intel
> > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > information.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> >  .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
> >  drivers/misc/Kconfig                               |  10 ++
> >  drivers/misc/Makefile                              |   1 +
> >  drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
> >  4 files changed, 201 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> >  create mode 100644 drivers/misc/intel-m10-bmc-retimer.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > new file mode 100644
> > index 0000000..528712a
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > @@ -0,0 +1,32 @@
> > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/tag
> > +Date:		Jan 2021
> > +KernelVersion:	5.12
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read only. Returns the tag of the retimer chip. Now there are 2
> > +		retimer chips on Intel PAC N3000, they are tagged as
> > +		'retimer_A' and 'retimer_B'.
> > +		Format: "retimer_%c".
> > +
> > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/sbus_version
> > +Date:		Jan 2021
> > +KernelVersion:	5.12
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read only. Returns the Transceiver bus firmware version of
> > +		the retimer chip.
> > +		Format: "0x%04x".
> > +
> > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/serdes_version
> > +Date:		Jan 2021
> > +KernelVersion:	5.12
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read only. Returns the SERDES firmware version of the retimer
> > +		chip.
> > +		Format: "0x%04x".
> > +
> > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > +Date:		Jan 2021
> > +KernelVersion:	5.12
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read only. Returns the status of each line side link. "1" for
> > +		link up, "0" for link down.
> > +		Format: "%u".
> 
> Who is going to use all of these read-only attributes?

The Intel OPAE (Open Programmable Acceleration Engine) lib handles these
attrs.

For the version attrs, the OPAE retimer firmware update tool will query
them to make sure the update is succeed.

For the link_status attrs, the OPAE net tools handles it.

> 
> And why isn't this information exported in the "normal" way for network
> devices?  Having them as custom sysfs attributes ensures that no
> existing tools will work with these at all, right?  Why not do the
> standard thing here isntead?

I had sent some RFC patches to expose the Line Side Ether Group + retimer +
QSFP as a netdev, and got some comments from netdev Maintainers.

The network part of the N3000 card is like the following:

                       +----------------------------------------+
                       |                  FPGA                  |
  +----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
  |QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
  +----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
                       |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
                       +-----------+  |offloading|  +-----------+   +----------+
                       |              +----------+              |
                       |                                        |
                       +----------------------------------------+

The main concern is that physically the QSFP & retimer is managed by the
BMC and host could only get the retimer link states. This is not enough
to support some necessary netdev ops.  E.g. host cannot realize the
type/speed of the SFP by "ethtool -m", then users could not configure the
various layers accordingly.

This means the existing net tool can not work with it, so this patch just
expose the link states as custom sysfs attrs.

Thanks,
Yilun 

Re: [PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Greg KH]

On Wed, Jan 06, 2021 at 04:53:29PM +0800, Xu Yilun wrote:
> On Wed, Jan 06, 2021 at 08:56:42AM +0100, Greg KH wrote:
> > On Wed, Jan 06, 2021 at 03:36:07PM +0800, Xu Yilun wrote:
> > > This driver supports the ethernet retimers (C827) for the Intel PAC
> > > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > > 
> > > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > > 10G/25G retimer mode. Host could query their link states and firmware
> > > version information via retimer interfaces (Shared registers) on Intel
> > > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > > information.
> > > 
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > ---
> > >  .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
> > >  drivers/misc/Kconfig                               |  10 ++
> > >  drivers/misc/Makefile                              |   1 +
> > >  drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
> > >  4 files changed, 201 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > >  create mode 100644 drivers/misc/intel-m10-bmc-retimer.c
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > > new file mode 100644
> > > index 0000000..528712a
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > > @@ -0,0 +1,32 @@
> > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/tag
> > > +Date:		Jan 2021
> > > +KernelVersion:	5.12
> > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > +Description:	Read only. Returns the tag of the retimer chip. Now there are 2
> > > +		retimer chips on Intel PAC N3000, they are tagged as
> > > +		'retimer_A' and 'retimer_B'.
> > > +		Format: "retimer_%c".
> > > +
> > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/sbus_version
> > > +Date:		Jan 2021
> > > +KernelVersion:	5.12
> > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > +Description:	Read only. Returns the Transceiver bus firmware version of
> > > +		the retimer chip.
> > > +		Format: "0x%04x".
> > > +
> > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/serdes_version
> > > +Date:		Jan 2021
> > > +KernelVersion:	5.12
> > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > +Description:	Read only. Returns the SERDES firmware version of the retimer
> > > +		chip.
> > > +		Format: "0x%04x".
> > > +
> > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > > +Date:		Jan 2021
> > > +KernelVersion:	5.12
> > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > +Description:	Read only. Returns the status of each line side link. "1" for
> > > +		link up, "0" for link down.
> > > +		Format: "%u".
> > 
> > Who is going to use all of these read-only attributes?
> 
> The Intel OPAE (Open Programmable Acceleration Engine) lib handles these
> attrs.

I have no idea what that is, you should put a pointer to the source for
this in either the changelog comment, or here in the sysfs entries to
show who is using this.

> For the version attrs, the OPAE retimer firmware update tool will query
> them to make sure the update is succeed.

Why does anyone care about that?  The firmware download logic should
handle that properly, right?

> For the link_status attrs, the OPAE net tools handles it.

So not the normal userspace networking tools?

If not, we need to get an ack from the networking developers as to why
you are not following their existing user/kernel apis.

thanks,

greg k-h

Re: [PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Xu Yilun]

On Wed, Jan 06, 2021 at 10:06:14AM +0100, Greg KH wrote:
> On Wed, Jan 06, 2021 at 04:53:29PM +0800, Xu Yilun wrote:
> > On Wed, Jan 06, 2021 at 08:56:42AM +0100, Greg KH wrote:
> > > On Wed, Jan 06, 2021 at 03:36:07PM +0800, Xu Yilun wrote:
> > > > This driver supports the ethernet retimers (C827) for the Intel PAC
> > > > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > > > 
> > > > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > > > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > > > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > > > 10G/25G retimer mode. Host could query their link states and firmware
> > > > version information via retimer interfaces (Shared registers) on Intel
> > > > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > > > information.
> > > > 
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > ---
> > > >  .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
> > > >  drivers/misc/Kconfig                               |  10 ++
> > > >  drivers/misc/Makefile                              |   1 +
> > > >  drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
> > > >  4 files changed, 201 insertions(+)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > > >  create mode 100644 drivers/misc/intel-m10-bmc-retimer.c
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > > > new file mode 100644
> > > > index 0000000..528712a
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
> > > > @@ -0,0 +1,32 @@
> > > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/tag
> > > > +Date:		Jan 2021
> > > > +KernelVersion:	5.12
> > > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > > +Description:	Read only. Returns the tag of the retimer chip. Now there are 2
> > > > +		retimer chips on Intel PAC N3000, they are tagged as
> > > > +		'retimer_A' and 'retimer_B'.
> > > > +		Format: "retimer_%c".
> > > > +
> > > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/sbus_version
> > > > +Date:		Jan 2021
> > > > +KernelVersion:	5.12
> > > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > > +Description:	Read only. Returns the Transceiver bus firmware version of
> > > > +		the retimer chip.
> > > > +		Format: "0x%04x".
> > > > +
> > > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/serdes_version
> > > > +Date:		Jan 2021
> > > > +KernelVersion:	5.12
> > > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > > +Description:	Read only. Returns the SERDES firmware version of the retimer
> > > > +		chip.
> > > > +		Format: "0x%04x".
> > > > +
> > > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > > > +Date:		Jan 2021
> > > > +KernelVersion:	5.12
> > > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > > +Description:	Read only. Returns the status of each line side link. "1" for
> > > > +		link up, "0" for link down.
> > > > +		Format: "%u".
> > > 
> > > Who is going to use all of these read-only attributes?
> > 
> > The Intel OPAE (Open Programmable Acceleration Engine) lib handles these
> > attrs.
> 
> I have no idea what that is, you should put a pointer to the source for
> this in either the changelog comment, or here in the sysfs entries to
> show who is using this.

This is the source of the OPAE lib.

https://github.com/OPAE/opae-sdk/

Generally it facilitate the development on all the DFL (Device Feature
List) based FPGA Cards, including the management of static region &
dynamic region reprogramming, accelerators accessing and the board
specific peripherals.

I'll add the link in the changelog.

> 
> > For the version attrs, the OPAE retimer firmware update tool will query
> > them to make sure the update is succeed.
> 
> Why does anyone care about that?  The firmware download logic should
> handle that properly, right?

Yes, the firmware download tool could always tells the update is succeed
or fail. But on another day we may need to check where we are by reading
the version numbers. An OPAE tool help collects the versions and users
could then check the release notes to see the detailed change.

> 
> > For the link_status attrs, the OPAE net tools handles it.
> 
> So not the normal userspace networking tools?
> 
> If not, we need to get an ack from the networking developers as to why
> you are not following their existing user/kernel apis.

This is the previous thread:

https://lore.kernel.org/netdev/1603442745-13085-2-git-send-email-yilun.xu@intel.com/

I'll resend the patch and loop in the netdev maintainers for further
discussion.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices

[Xu Yilun]

On Wed, Jan 06, 2021 at 08:23:30AM +0000, Lee Jones wrote:
> On Wed, 06 Jan 2021, Xu Yilun wrote:
> 
> > The patch specifies the 2 retimer sub devices and their resources in the
> > parent driver's mfd_cell. It also adds the register definition of the
> > retimer sub devices.
> > 
> > There are 2 ethernet retimer chips (C827) connected to the Intel MAX 10
> > BMC. They are managed by the BMC firmware, and host could query them via
> > retimer interfaces (shared registers) on the BMC. The 2 retimers have
> > identical register interfaces in different register addresses or fields,
> > so it is better we define 2 retimer devices and handle them with the same
> > driver.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> >  drivers/mfd/intel-m10-bmc.c       | 19 ++++++++++++++++++-
> >  include/linux/mfd/intel-m10-bmc.h |  7 +++++++
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> > index b84579b..e0a99a0 100644
> > --- a/drivers/mfd/intel-m10-bmc.c
> > +++ b/drivers/mfd/intel-m10-bmc.c
> > @@ -17,9 +17,26 @@ enum m10bmc_type {
> >  	M10_N3000,
> >  };
> >  
> > +static struct resource retimer0_resources[] = {
> > +	{M10BMC_PKVL_A_VER, M10BMC_PKVL_A_VER, "version", IORESOURCE_REG, },
> > +};
> > +
> > +static struct resource retimer1_resources[] = {
> > +	{M10BMC_PKVL_B_VER, M10BMC_PKVL_B_VER, "version", IORESOURCE_REG, },
> > +};
> 
> Please use the DEFINE_RES_*() helpers for this.

Yes, will change it.

Thanks,
Yilun