Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC
On Fri, Jan 08, 2021 at 10:05:26AM +0800, Xu Yilun wrote:
> On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote:
> > On Thu, Jan 07, 2021 at 02:07:08PM +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.
> >
> > Networking people, please look at this sysfs file:
> >
> > > +What:
> > > /sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > > +Date:Jan 2021
> > > +KernelVersion: 5.12
> > > +Contact: Xu Yilun
> > > +Description: Read only. Returns the status of each line side link.
> > > "1" for
> > > + link up, "0" for link down.
> > > + Format: "%u".
> >
> > as I need your approval to add it because it is not the "normal" way for
> > link status to be exported to userspace.
> >
> > One code issue:
> >
> > > +#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, );
> > > + 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) }
> >
> > Why is this a "raw" attribute and not a device attribute?
>
> It is actually a device_attribute. The device_attribute is embedded in
> link_attr, like:
>
> struct link_attr {
> struct device_attribute attr;
> u32 index;
> };
>
> An index for the link is appended along with the device_attribute, so we
> could identify which link is being queried on link_status_show(). There
> are 4 links and this is to avoid duplicated code like
> link_status_1_show(), link_status_2_show() ...
Duplicated code is better to read than complex code :)
> > Please just use a normal DEVICE_ATTR_RO() macro to make it simpler and
>
> DEVICE_ATTR_RO() is to define a standalone device_attribute variable, but
> here we are initializing a field in struct link_attr.
Then use the correct initialization macro that is given to you for that,
do not roll your own.
greg k-h
Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC
On Thu, Jan 07, 2021 at 03:51:38PM +0100, Andrew Lunn wrote: > On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote: > > On Thu, Jan 07, 2021 at 02:07:08PM +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. > > > > Networking people, please look at this sysfs file: > > > > > +What: > > > /sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX > > > +Date:Jan 2021 > > > +KernelVersion: 5.12 > > > +Contact: Xu Yilun > > > +Description: Read only. Returns the status of each line side link. > > > "1" for > > > + link up, "0" for link down. > > > + Format: "%u". > > > > as I need your approval to add it because it is not the "normal" way for > > link status to be exported to userspace. > > Hi Greg > > Correct, this is not going to be acceptable. > > The whole architecture needs to cleanly fit into the phylink model for > controlling the SFP and the retimer. > > I'm guessing Intel needs to rewrite portions of the BMC firmware to > either transparently pass through access to the SFP socket and the > retimer for phylink and a C827 specific driver, or add a high level > API which a MAC driver can use, and completely hide away these PHY > details from Linux, which is what many of the Intel Ethernet drivers > do. Got it, Thanks for your explanation. Yilun
Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC
On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote:
> On Thu, Jan 07, 2021 at 02:07:08PM +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.
>
> Networking people, please look at this sysfs file:
>
> > +What:
> > /sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > +Date: Jan 2021
> > +KernelVersion: 5.12
> > +Contact: Xu Yilun
> > +Description: Read only. Returns the status of each line side link.
> > "1" for
> > + link up, "0" for link down.
> > + Format: "%u".
>
> as I need your approval to add it because it is not the "normal" way for
> link status to be exported to userspace.
>
> One code issue:
>
> > +#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, );
> > + 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) }
>
> Why is this a "raw" attribute and not a device attribute?
It is actually a device_attribute. The device_attribute is embedded in
link_attr, like:
struct link_attr {
struct device_attribute attr;
u32 index;
};
An index for the link is appended along with the device_attribute, so we
could identify which link is being queried on link_status_show(). There
are 4 links and this is to avoid duplicated code like
link_status_1_show(), link_status_2_show() ...
>
> Please just use a normal DEVICE_ATTR_RO() macro to make it simpler and
DEVICE_ATTR_RO() is to define a standalone device_attribute variable, but
here we are initializing a field in struct link_attr.
Thanks,
Yilun
> easier to understand over time, what you are doing here. I can't
> determine what is happening with this code now...
>
> thanks,
>
> greg k-h
Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC
On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote: > On Thu, Jan 07, 2021 at 02:07:08PM +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. > > Networking people, please look at this sysfs file: > > > +What: > > /sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX > > +Date: Jan 2021 > > +KernelVersion: 5.12 > > +Contact: Xu Yilun > > +Description: Read only. Returns the status of each line side link. > > "1" for > > + link up, "0" for link down. > > + Format: "%u". > > as I need your approval to add it because it is not the "normal" way for > link status to be exported to userspace. Hi Greg Correct, this is not going to be acceptable. The whole architecture needs to cleanly fit into the phylink model for controlling the SFP and the retimer. I'm guessing Intel needs to rewrite portions of the BMC firmware to either transparently pass through access to the SFP socket and the retimer for phylink and a C827 specific driver, or add a high level API which a MAC driver can use, and completely hide away these PHY details from Linux, which is what many of the Intel Ethernet drivers do. Andrew
Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC
On Thu, Jan 07, 2021 at 02:07:08PM +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.
Networking people, please look at this sysfs file:
> +What:
> /sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> +Date:Jan 2021
> +KernelVersion: 5.12
> +Contact: Xu Yilun
> +Description: Read only. Returns the status of each line side link. "1" for
> + link up, "0" for link down.
> + Format: "%u".
as I need your approval to add it because it is not the "normal" way for
link status to be exported to userspace.
One code issue:
> +#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, );
> + 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) }
Why is this a "raw" attribute and not a device attribute?
Please just use a normal DEVICE_ATTR_RO() macro to make it simpler and
easier to understand over time, what you are doing here. I can't
determine what is happening with this code now...
thanks,
greg k-h
[RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC
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
---
.../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 000..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
+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
+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
+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
+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 000..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
+#include
+#include
+#include
+#include
+
+#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 =
