Message ID | 1609999628-12748-3-git-send-email-yilun.xu@intel.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | Add retimer interfaces support for Intel MAX 10 BMC | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Not a local patch |
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 <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". 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, &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) } 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
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 <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". > > 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
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 <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". > > 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, &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) } > > 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
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 <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". > > > > 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
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 <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". > > > > 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, &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) } > > > > 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
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");
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