* [RESEND PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC @ 2021-01-07 6:07 Xu Yilun 2021-01-07 6:07 ` [RESEND PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices Xu Yilun 2021-01-07 6:07 ` [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC Xu Yilun 0 siblings, 2 replies; 8+ messages in thread From: Xu Yilun @ 2021-01-07 6:07 UTC (permalink / raw) To: andrew, arnd, lee.jones, gregkh Cc: netdev, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, russell.h.weight I resend this patchset to loop in networking developers for comments. This is the previous thread. I'll fix other comments when I have a v2. https://lore.kernel.org/lkml/X%2FV9hvXYlUOT9U2n@kroah.com/ The patchset is for the retimers connected to Intel MAX 10 BMC on Intel PAC (Programmable Acceleration Card) N3000 Card. 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| +-----------+ +----------+ | +----------+ | | | +----------------------------------------+ 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. https://lore.kernel.org/netdev/1603442745-13085-2-git-send-email-yilun.xu@intel.com/ The blocking issues I have 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. 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. The Intel OPAE (Open Programmable Acceleration Engine) lib provides tools to read these attributes. 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. 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RESEND PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices 2021-01-07 6:07 [RESEND PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC Xu Yilun @ 2021-01-07 6:07 ` Xu Yilun 2021-01-07 6:07 ` [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC Xu Yilun 1 sibling, 0 replies; 8+ messages in thread From: Xu Yilun @ 2021-01-07 6:07 UTC (permalink / raw) To: andrew, arnd, lee.jones, gregkh Cc: netdev, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, russell.h.weight 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC 2021-01-07 6:07 [RESEND PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC Xu Yilun 2021-01-07 6:07 ` [RESEND PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices Xu Yilun @ 2021-01-07 6:07 ` Xu Yilun 2021-01-07 9:26 ` Greg KH 1 sibling, 1 reply; 8+ messages in thread From: Xu Yilun @ 2021-01-07 6:07 UTC (permalink / raw) To: andrew, arnd, lee.jones, gregkh Cc: netdev, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, russell.h.weight 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC 2021-01-07 6:07 ` [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC Xu Yilun @ 2021-01-07 9:26 ` Greg KH 2021-01-07 14:51 ` Andrew Lunn 2021-01-08 2:05 ` Xu Yilun 0 siblings, 2 replies; 8+ messages in thread From: Greg KH @ 2021-01-07 9:26 UTC (permalink / raw) To: Xu Yilun Cc: andrew, arnd, lee.jones, netdev, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC 2021-01-07 9:26 ` Greg KH @ 2021-01-07 14:51 ` Andrew Lunn 2021-01-08 2:08 ` Xu Yilun 2021-01-08 2:05 ` Xu Yilun 1 sibling, 1 reply; 8+ messages in thread From: Andrew Lunn @ 2021-01-07 14:51 UTC (permalink / raw) To: Greg KH Cc: Xu Yilun, arnd, lee.jones, netdev, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC 2021-01-07 14:51 ` Andrew Lunn @ 2021-01-08 2:08 ` Xu Yilun 0 siblings, 0 replies; 8+ messages in thread From: Xu Yilun @ 2021-01-08 2:08 UTC (permalink / raw) To: Andrew Lunn Cc: Greg KH, arnd, lee.jones, netdev, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, yilun.xu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC 2021-01-07 9:26 ` Greg KH 2021-01-07 14:51 ` Andrew Lunn @ 2021-01-08 2:05 ` Xu Yilun 2021-01-08 7:47 ` Greg KH 1 sibling, 1 reply; 8+ messages in thread From: Xu Yilun @ 2021-01-08 2:05 UTC (permalink / raw) To: Greg KH Cc: andrew, arnd, lee.jones, netdev, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, yilun.xu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC 2021-01-08 2:05 ` Xu Yilun @ 2021-01-08 7:47 ` Greg KH 0 siblings, 0 replies; 8+ messages in thread From: Greg KH @ 2021-01-08 7:47 UTC (permalink / raw) To: Xu Yilun Cc: andrew, arnd, lee.jones, netdev, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-08 7:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-07 6:07 [RESEND PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC Xu Yilun 2021-01-07 6:07 ` [RESEND PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices Xu Yilun 2021-01-07 6:07 ` [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC Xu Yilun 2021-01-07 9:26 ` Greg KH 2021-01-07 14:51 ` Andrew Lunn 2021-01-08 2:08 ` Xu Yilun 2021-01-08 2:05 ` Xu Yilun 2021-01-08 7:47 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.