Re: [PATCH v9 5/6] fpga: dfl: add support for N3000 Nios private feature
From: Tom Rix
Date: Mon Oct 12 2020 - 10:19:55 EST
On 10/11/20 11:51 PM, Xu Yilun wrote:
> On Sat, Oct 10, 2020 at 02:21:45PM -0700, Tom Rix wrote:
>> On 10/10/20 12:09 AM, Xu Yilun wrote:
>>> This patch adds support for the Nios handshake private feature on Intel
>>> PAC (Programmable Acceleration Card) N3000.
>>>
>>> The Nios is the embedded processor on the FPGA card. This private feature
>>> provides a handshake interface to FPGA Nios firmware, which receives
>>> retimer configuration command from host and executes via an internal SPI
>>> master (spi-altera). When Nios finishes the configuration, host takes over
>>> the ownership of the SPI master to control an Intel MAX10 BMC (Board
>>> Management Controller) Chip on the SPI bus.
>>>
>>> For Nios firmware handshake part, this driver requests the retimer
>>> configuration for Nios firmware on probe, and adds some sysfs nodes for
>>> user to query the onboard retimer's working mode and Nios firmware
>>> version.
>>>
>>> For SPI part, this driver adds a spi-altera platform device as well as
>>> the MAX10 BMC spi slave info. A spi-altera driver will be matched to
>>> handle the following SPI work.
>>>
>>> [mdf@xxxxxxxxxx: Fixed up MAINTAINERS file to include added ABI doc]
>>> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
>>> Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
>>> ---
>>> v3: add the doc for this driver
>>> minor fixes for comments from Tom
>>> v4: move the err log in regmap implementation, and delete
>>> n3000_nios_writel/readl(), they have nothing to wrapper now.
>>> some minor fixes and comments improvement.
>>> v5: fix the output of fec_mode sysfs inf to "no" on 10G configuration,
>>> cause no FEC mode could be configured for 10G.
>>> rename the dfl_n3000_nios_* to n3000_nios_*
>>> improves comments.
>>> v6: fix the output of fec_mode sysfs inf to "not supported" if in 10G,
>>> or the firmware version major < 3.
>>> minor fixes and improves comments.
>>> v7: improves comments.
>>> v8: add sysfs interfaces for retimer mode, also doc update.
>>> delete duplicated sysfs interfaces description in doc.
>>> minor fixes.
>>> v9: delete the retimer FEC mode configuration via module_parameter.
>>> update the kernel version of the sysfs interfaces in Doc.
>>> merge the patch "Make m10_n3000_info static" (https://lore.kernel.org/linux-fpga/52d8411e-13d8-1e91-756d-131802f5f445@xxxxxxxxxx/T/#t)
>>> remove the tags of Maintainers, this implementation is changed.
>>> ---
>>> .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios | 47 ++
>>> MAINTAINERS | 2 +-
>>> drivers/fpga/Kconfig | 11 +
>>> drivers/fpga/Makefile | 2 +
>>> drivers/fpga/dfl-n3000-nios.c | 566 +++++++++++++++++++++
>>> 5 files changed, 627 insertions(+), 1 deletion(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>> create mode 100644 drivers/fpga/dfl-n3000-nios.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>> new file mode 100644
>>> index 0000000..f2f6e5e
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>> @@ -0,0 +1,47 @@
>>> +What: /sys/bus/dfl/devices/dfl_dev.X/fec_mode
>>> +Date: Oct 2020
>>> +KernelVersion: 5.11
>>> +Contact: Xu Yilun <yilun.xu@xxxxxxxxx>
>>> +Description: Read-only. It returns the FEC mode of the 25G links of the
>> Could drop the 'It' and start with 'Returns' here and similar
> Yes, I can change it.
>
>>> + ethernet retimers configured by Nios firmware. "rs" for Reed
>>> + Solomon FEC, "kr" for Fire Code FEC, "no" for NO FEC.
>>> + "not supported" if the FEC mode setting is not supported, this
>> setting ? the description says 'Read-only'
> The setting is done by Nios firmware, host cannot change it at runtime
> so it is 'Read-only'. It is previously described in this section. Is it
> good?
ok
>
>>> + happens when the Nios firmware version major < 3, or no link is
>>> + configured to 25G.
>>> + Format: string
>>> +
>>> +What: /sys/bus/dfl/devices/dfl_dev.X/retimer_A_mode
>>> +Date: Oct 2020
>>> +KernelVersion: 5.11
>>> +Contact: Xu Yilun <yilun.xu@xxxxxxxxx>
>>> +Description: Read-only. It returns the enumeration value of the working mode
>>> + of the retimer A configured by the Nios firmware. The value is
>>> + read out from shared registers filled by the Nios firmware. Now
>>> + the values could be:
>>> +
>>> + - "0": Reset
>>> + - "1": 4x10G
>>> + - "2": 4x25G
>>> + - "3": 2x25G
>>> + - "4": 2x25G+2x10G
>>> + - "5": 1x25G
>>> +
>>> + If the Nios firmware is updated in future to support more
>>> + retimer modes, more enumeration value is expected.
>>> + Format: 0x%x
>>> +
>>> +What: /sys/bus/dfl/devices/dfl_dev.X/retimer_B_mode
>>> +Date: Oct 2020
>>> +KernelVersion: 5.11
>>> +Contact: Xu Yilun <yilun.xu@xxxxxxxxx>
>>> +Description: Read-only. It returns the enumeration value of the working mode
>>> + of the retimer B configured by the Nios firmware. The value
>>> + format is the same as retimer_A_mode.
>>> +
>>> +What: /sys/bus/dfl/devices/dfl_dev.X/nios_fw_version
>>> +Date: Oct 2020
>>> +KernelVersion: 5.11
>>> +Contact: Xu Yilun <yilun.xu@xxxxxxxxx>
>>> +Description: Read-only. It returns the version of the Nios firmware in FPGA.
>> Can drop 'in FPGA'.
> The PAC N3000 card has 2 Nios firmwares. The one for this driver is
> embedded in A10 FPGA. Another one is embedded in MAX10 BMC chip.
>
> So to be clear, I think we may keep the 'in FPGA'.
ok, then change to
'in the FPGA'
>
>>> + Its format is "major.minor.patch".
>>> + Format: %x.%x.%x
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 9d46097..3a81cac 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -6881,7 +6881,7 @@ M: Wu Hao <hao.wu@xxxxxxxxx>
>>> R: Tom Rix <trix@xxxxxxxxxx>
>>> L: linux-fpga@xxxxxxxxxxxxxxx
>>> S: Maintained
>>> -F: Documentation/ABI/testing/sysfs-bus-dfl
>>> +F: Documentation/ABI/testing/sysfs-bus-dfl*
>>> F: Documentation/fpga/dfl.rst
>>> F: drivers/fpga/dfl*
>>> F: include/linux/fpga/dfl.h
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 7cd5a29..38c7130 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -191,6 +191,17 @@ config FPGA_DFL_AFU
>>> to the FPGA infrastructure via a Port. There may be more than one
>>> Port/AFU per DFL based FPGA device.
>>>
>>> +config FPGA_DFL_NIOS_INTEL_PAC_N3000
>>> + tristate "FPGA DFL NIOS Driver for Intel PAC N3000"
>>> + depends on FPGA_DFL
>> Would 'depends on FPGA_DFL_PCI' be better ?
> The relationship between the 2 config is, the n3000 nios private feature
> now only appears on PAC N3000 which is a PCIe card.
>
> But the code has no dependency to FPGA_DFL_PCI. Is it necessary we add the
> 'depends on'?
as-is is fine
i am thinking a card specific config is needed but that is another topic.
>
>>> + select REGMAP
>>> + help
>> Last 3 lines have whitespace issues, need to convert spaces to tabs
> Thanks for catching this.
>
>>> + This is the driver for the N3000 Nios private feature on Intel
>>> + PAC (Programmable Acceleration Card) N3000. It communicates
>>> + with the embedded Nios processor to configure the retimers on
>>> + the card. It also instantiates the SPI master (spi-altera) for
>>> + the card's BMC (Board Management Controller) control.
>> can drop 'control'
>>
>> If you read out 'card's board mangement controller control' it sounds awkward.
> It's true. I'll drop it.
>
>>> +
>>> config FPGA_DFL_PCI
>>> tristate "FPGA DFL PCIe Device Driver"
>>> depends on PCI && FPGA_DFL
>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>> index d8e21df..18dc9885 100644
>>> --- a/drivers/fpga/Makefile
>>> +++ b/drivers/fpga/Makefile
>>> @@ -44,5 +44,7 @@ dfl-fme-objs += dfl-fme-perf.o
>>> dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>>> dfl-afu-objs += dfl-afu-error.o
>>>
>>> +obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000) += dfl-n3000-nios.o
>>> +
>>> # Drivers for FPGAs which implement DFL
>>> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
>>> diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
>>> new file mode 100644
>>> index 0000000..e7c3862
>>> --- /dev/null
>>> +++ b/drivers/fpga/dfl-n3000-nios.c
>>> @@ -0,0 +1,566 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * DFL device driver for Nios private feature on Intel PAC (Programmable
>>> + * Acceleration Card) N3000
>>> + *
>>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>>> + *
>>> + * Authors:
>>> + * Wu Hao <hao.wu@xxxxxxxxx>
>>> + * Xu Yilun <yilun.xu@xxxxxxxxx>
>>> + */
>>> +#include <linux/bitfield.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/fpga/dfl.h>
>>> +#include <linux/io.h>
>>> +#include <linux/io-64-nonatomic-lo-hi.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/stddef.h>
>>> +#include <linux/spi/altera.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/types.h>
>>> +
>>> +/* N3000 Nios private feature registers */
>>> +#define NIOS_SPI_PARAM 0x8
>> The #defines should have a consistent prefix that is unlikely to conflict.
>>
>> Maybe N3000_NIOS_
> Yes, I'll try to refactor the macro names.
> The register names in spec are NIOS_SPI_xx, how about we compress the prefixes,
> 'N3000_NS' & 'n3000_ns' for 'N3000 NIOS SPI'
ok for n3000_ns. just move the comment for 'ns' up this first use.
>
>>> +#define PARAM_SHIFT_MODE_MSK BIT_ULL(1)
>>> +#define PARAM_SHIFT_MODE_MSB 0
>>> +#define PARAM_SHIFT_MODE_LSB 1
>>> +#define PARAM_DATA_WIDTH GENMASK_ULL(7, 2)
>>> +#define PARAM_NUM_CS GENMASK_ULL(13, 8)
>>> +#define PARAM_CLK_POL BIT_ULL(14)
>>> +#define PARAM_CLK_PHASE BIT_ULL(15)
>>> +#define PARAM_PERIPHERAL_ID GENMASK_ULL(47, 32)
>>> +
>>> +#define NIOS_SPI_CTRL 0x10
>>> +#define CTRL_WR_DATA GENMASK_ULL(31, 0)
>>> +#define CTRL_ADDR GENMASK_ULL(44, 32)
>>> +#define CTRL_CMD_MSK GENMASK_ULL(63, 62)
>>> +#define CTRL_CMD_NOP 0
>>> +#define CTRL_CMD_RD 1
>>> +#define CTRL_CMD_WR 2
>>> +
>>> +#define NIOS_SPI_STAT 0x18
>>> +#define STAT_RD_DATA GENMASK_ULL(31, 0)
>>> +#define STAT_RW_VAL BIT_ULL(32)
>>> +
>>> +/* Nios handshake registers, indirect access */
>>> +#define NIOS_INIT 0x1000
>>> +#define NIOS_INIT_DONE BIT(0)
>>> +#define NIOS_INIT_START BIT(1)
>>> +/* Mode for retimer A, link 0, the same below */
>>> +#define REQ_FEC_MODE_A0_MSK GENMASK(9, 8)
>>> +#define REQ_FEC_MODE_A1_MSK GENMASK(11, 10)
>>> +#define REQ_FEC_MODE_A2_MSK GENMASK(13, 12)
>>> +#define REQ_FEC_MODE_A3_MSK GENMASK(15, 14)
>>> +#define REQ_FEC_MODE_B0_MSK GENMASK(17, 16)
>>> +#define REQ_FEC_MODE_B1_MSK GENMASK(19, 18)
>>> +#define REQ_FEC_MODE_B2_MSK GENMASK(21, 20)
>>> +#define REQ_FEC_MODE_B3_MSK GENMASK(23, 22)
>>> +#define REQ_FEC_MODE_NO 0x0
>>> +#define REQ_FEC_MODE_KR 0x1
>>> +#define REQ_FEC_MODE_RS 0x2
>>> +
>>> +#define NIOS_FW_VERSION 0x1004
>>> +#define NIOS_FW_VERSION_PATCH GENMASK(23, 20)
>>> +#define NIOS_FW_VERSION_MINOR GENMASK(27, 24)
>>> +#define NIOS_FW_VERSION_MAJOR GENMASK(31, 28)
>>> +
>>> +/* The retimers we use on Intel PAC N3000 is Parkvale, abbreviated to PKVL */
>> Parkvale is codename, why not just RETIMER_ ?
> The register fields names in Hardware spec is PKVL_, so I mean to keep
> align with the spec.
ok
>
>>> +#define PKVL_A_MODE_STS 0x1020
>>> +#define PKVL_B_MODE_STS 0x1024
>>> +#define PKVL_MODE_STS_GROUP_MSK GENMASK(15, 8)
>>> +#define PKVL_MODE_STS_GROUP_OK 0x0
>>> +#define PKVL_MODE_STS_ID_MSK GENMASK(7, 0)
>>> +/* When GROUP MASK field == GROUP_OK */
>>> +#define PKVL_MODE_ID_RESET 0x0
>>> +#define PKVL_MODE_ID_4X10G 0x1
>>> +#define PKVL_MODE_ID_4X25G 0x2
>>> +#define PKVL_MODE_ID_2X25G 0x3
>>> +#define PKVL_MODE_ID_2X25G_2X10G 0x4
>>> +#define PKVL_MODE_ID_1X25G 0x5
>>> +
>>> +#define NS_REGBUS_WAIT_TIMEOUT 10000 /* loop count */
>> NS ? Expand.
> I could explain the NS - NIOS SPI in comments, and is N3000_NS_REGBUS_RETRY_COUNT good?
yes this is fine. for use of 'ns' which has a more common meaning of 'nano second' need to take care when meaning could be confused with 'nios spi'
>
>> 'WAIT_TIMEOUT' should be changed to 'RETRY_COUNT' to reflect its use
> It makes sense.
>
>>> +#define NIOS_INIT_TIMEOUT 10000000 /* usec */
>>> +#define NIOS_INIT_TIME_INTV 100000 /* usec */
>>> +
>>> +struct n3000_nios {
>>> + void __iomem *base;
>>> + struct regmap *regmap;
>>> + struct device *dev;
>>> + struct platform_device *altera_spi;
>>> +};
>>> +
>>> +static ssize_t nios_fw_version_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct n3000_nios *ns = dev_get_drvdata(dev);
>>> + unsigned int val;
>>> + int ret;
>>> +
>>> + ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sprintf(buf, "%x.%x.%x\n",
>>> + (u8)FIELD_GET(NIOS_FW_VERSION_MAJOR, val),
>>> + (u8)FIELD_GET(NIOS_FW_VERSION_MINOR, val),
>>> + (u8)FIELD_GET(NIOS_FW_VERSION_PATCH, val));
>>> +}
>>> +static DEVICE_ATTR_RO(nios_fw_version);
>>> +
>>> +#define IS_MODE_STATUS_OK(mode_stat) \
>>> + (FIELD_GET(PKVL_MODE_STS_GROUP_MSK, (mode_stat)) == \
>>> + PKVL_MODE_STS_GROUP_OK)
>>> +
>>> +#define IS_RETIMER_FEC_SUPPORTED(retimer_mode) \
>>> + ((retimer_mode) != PKVL_MODE_ID_RESET && \
>>> + (retimer_mode) != PKVL_MODE_ID_4X10G)
>>> +
>>> +static int get_retimer_mode(struct n3000_nios *ns, unsigned int mode_stat_reg,
>>> + unsigned int *retimer_mode)
>>> +{
>>> + unsigned int val;
>>> + int ret;
>>> +
>>> + ret = regmap_read(ns->regmap, mode_stat_reg, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (!IS_MODE_STATUS_OK(val))
>>> + return -EFAULT;
>>> +
>>> + *retimer_mode = FIELD_GET(PKVL_MODE_STS_ID_MSK, val);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static ssize_t retimer_A_mode_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct n3000_nios *ns = dev_get_drvdata(dev);
>>> + unsigned int mode;
>>> + int ret;
>>> +
>>> + ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sprintf(buf, "0x%x\n", mode);
>>> +}
>>> +static DEVICE_ATTR_RO(retimer_A_mode);
>>> +
>>> +static ssize_t retimer_B_mode_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct n3000_nios *ns = dev_get_drvdata(dev);
>>> + unsigned int mode;
>>> + int ret;
>>> +
>>> + ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sprintf(buf, "0x%x\n", mode);
>>> +}
>>> +static DEVICE_ATTR_RO(retimer_B_mode);
>>> +
>>> +static ssize_t fec_mode_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + unsigned int val, retimer_a_mode, retimer_b_mode, fec_mode;
>>> + struct n3000_nios *ns = dev_get_drvdata(dev);
>>> + int ret;
>>> +
>>> + /* FEC mode setting is not supported in early FW versions */
>>> + ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) < 3)
>>> + return sprintf(buf, "not supported\n");
>>> +
>>> + /* If no 25G links, FEC mode setting is not supported either */
>>> + ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &retimer_a_mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &retimer_b_mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (!IS_RETIMER_FEC_SUPPORTED(retimer_a_mode) &&
>>> + !IS_RETIMER_FEC_SUPPORTED(retimer_b_mode))
>>> + return sprintf(buf, "not supported\n");
>>> +
>>> + /* get the valid FEC mode for 25G links */
>>> + ret = regmap_read(ns->regmap, NIOS_INIT, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /*
>>> + * FEC mode should always be the same for all links, as we set them
>>> + * in this way.
>>> + */
>>> + fec_mode = FIELD_GET(REQ_FEC_MODE_A0_MSK, val);
>>> + if (fec_mode != FIELD_GET(REQ_FEC_MODE_A1_MSK, val) ||
>>> + fec_mode != FIELD_GET(REQ_FEC_MODE_A2_MSK, val) ||
>>> + fec_mode != FIELD_GET(REQ_FEC_MODE_A3_MSK, val) ||
>>> + fec_mode != FIELD_GET(REQ_FEC_MODE_B0_MSK, val) ||
>>> + fec_mode != FIELD_GET(REQ_FEC_MODE_B1_MSK, val) ||
>>> + fec_mode != FIELD_GET(REQ_FEC_MODE_B2_MSK, val) ||
>>> + fec_mode != FIELD_GET(REQ_FEC_MODE_B3_MSK, val))
>>> + return -EFAULT;
>> a precomputed table setting the fields would reduce the this to a lookup and 1 check.
>>
>> similar table could be used below for the setting.
> Yes, I can do it.
>
>>> +
>>> + switch (fec_mode) {
>>> + case REQ_FEC_MODE_NO:
>>> + return sprintf(buf, "no\n");
>>> + case REQ_FEC_MODE_KR:
>>> + return sprintf(buf, "kr\n");
>>> + case REQ_FEC_MODE_RS:
>>> + return sprintf(buf, "rs\n");
>>> + }
>>> +
>>> + return -EFAULT;
>>> +}
>>> +static DEVICE_ATTR_RO(fec_mode);
>>> +
>>> +static struct attribute *n3000_nios_attrs[] = {
>>> + &dev_attr_nios_fw_version.attr,
>>> + &dev_attr_retimer_A_mode.attr,
>>> + &dev_attr_retimer_B_mode.attr,
>>> + &dev_attr_fec_mode.attr,
>>> + NULL,
>>> +};
>>> +ATTRIBUTE_GROUPS(n3000_nios);
>>> +
>>> +static bool init_error_detected(struct n3000_nios *ns)
>>> +{
>>> + unsigned int val;
>>> +
>>> + if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
>>> + return true;
>>> +
>>> + if (!IS_MODE_STATUS_OK(val))
>>> + return true;
>>> +
>>> + if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
>>> + return true;
>>> +
>>> + if (!IS_MODE_STATUS_OK(val))
>>> + return true;
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static void dump_error_stat(struct n3000_nios *ns)
>>> +{
>>> + unsigned int val;
>>> +
>>> + if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
>>> + return;
>>> +
>>> + dev_err(ns->dev, "PKVL_A_MODE_STS 0x%x\n", val);
>>> +
>>> + if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
>>> + return;
>>> +
>>> + dev_err(ns->dev, "PKVL_B_MODE_STS 0x%x\n", val);
>>> +}
>>> +
>>> +static int n3000_nios_init_done_check(struct n3000_nios *ns)
>>> +{
>>> + struct device *dev = ns->dev;
>>> + unsigned int val, mode;
>>> + int ret;
>>> +
>>> + /*
>>> + * this SPI is shared by Nios core inside FPGA, Nios will use this SPI
>> 'by the Nios core inside the FPGA ... '
> Yes.
>
>>> + * master to do some one time initialization after power up, and then
>>> + * release the control to OS. driver needs to poll on INIT_DONE to
>> 'The driver..'
> Yes.
>
>>> + * see when driver could take the control.
>>> + *
>>> + * Please note that after Nios firmware version 3.0.0, INIT_START is
>>> + * introduced, so driver needs to trigger START firstly and then check
>>> + * INIT_DONE.
>>> + */
>>> +
>>> + ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /*
>>> + * If Nios version register is totally uninitialized(== 0x0), then the
>>> + * Nios firmware is missing. So host could take control of SPI master
>>> + * safely, but initialization work for Nios is not done. To restore the
>>> + * card, we need to reprogram a new Nios firmware via the BMC chip on
>>> + * SPI bus. So the driver doesn't error out, it continues to create the
>>> + * spi controller device and spi_board_info for BMC.
>>> + */
>>> + if (val == 0) {
>>> + dev_err(dev, "Nios version reg = 0x%x, skip INIT_DONE check, but the retimer may be uninitialized\n",
>>> + val);
>>> + return 0;
>>> + }
>>> +
>>> + if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
>>> + /* read NIOS_INIT to check if retimer initialization is done */
>>> + ret = regmap_read(ns->regmap, NIOS_INIT, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* check if retimers are initialized already */
>>> + if (val & NIOS_INIT_DONE || val & NIOS_INIT_START)
>> val & (NIOS_INIT_DONE | NIOS_INIT_START)
> Yes.
>
>>> + goto nios_init_done;
>>> +
>>> + /* configure FEC mode per module param */
>>> + val = NIOS_INIT_START;
>>> +
>>> + /*
>>> + * When the retimer is to be set to 10G mode, there is no FEC
>>> + * mode setting, so the REQ_FEC_MODE field will be ignored by
>>> + * Nios firmware in this case. But we should still fill the FEC
>>> + * mode field cause host could not get the retimer working mode
>>> + * until the Nios init is done.
>>> + *
>>> + * For now the driver doesn't support the retimer FEC mode
>>> + * switching per user's request. It is always set to Reed
>>> + * Solomon FEC.
>>> + */
>>> + mode = REQ_FEC_MODE_RS;
>>> +
>>> + /* set the same FEC mode for all links */
>>> + val |= FIELD_PREP(REQ_FEC_MODE_A0_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_A1_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_A2_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_A3_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B0_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B1_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B2_MSK, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B3_MSK, mode);
> I'll also optimize the code here.
>
>>> +
>>> + ret = regmap_write(ns->regmap, NIOS_INIT, val);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> +nios_init_done:
>>> + /* polls on NIOS_INIT_DONE */
>>> + ret = regmap_read_poll_timeout(ns->regmap, NIOS_INIT, val,
>>> + val & NIOS_INIT_DONE,
>>> + NIOS_INIT_TIME_INTV,
>>> + NIOS_INIT_TIMEOUT);
>>> + if (ret) {
>>> + dev_err(dev, "NIOS_INIT_DONE %s\n",
>>> + (ret == -ETIMEDOUT) ? "timed out" : "check error");
>>> + goto dump_sts;
>>> + }
>>> +
>>> + /*
>>> + * After INIT_DONE is detected, it still needs to check if any error
>>> + * detected.
>>> + */
>>> + if (init_error_detected(ns)) {
>>> + /*
>>> + * If the retimer configuration is failed, the Nios firmware
>>> + * will still release the spi controller for host to
>>> + * communicate with the BMC. It makes possible for people to
>>> + * reprogram a new Nios firmware and restore the card. So the
>>> + * driver doesn't error out, it continues to create the spi
>>> + * controller device and spi_board_info for BMC.
>>> + */
>>> + dev_err(dev, "NIOS_INIT_DONE OK, but err found during init\n");
>>> + goto dump_sts;
>>> + }
>>> + return 0;
>>> +
>>> +dump_sts:
>>> + dump_error_stat(ns);
>> Could init_error_detected and dump_error_stat be combined ?
> Yes, I could try to refactor it.
>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static struct spi_board_info m10_n3000_info = {
>>> + .modalias = "m10-n3000",
>>> + .max_speed_hz = 12500000,
>>> + .bus_num = 0,
>>> + .chip_select = 0,
>>> +};
>>> +
>>> +static int create_altera_spi_controller(struct n3000_nios *ns)
>>> +{
>>> + struct altera_spi_platform_data pdata = { 0 };
>>> + struct platform_device_info pdevinfo = { 0 };
>>> + void __iomem *base = ns->base;
>>> + u64 v;
>>> +
>>> + v = readq(base + NIOS_SPI_PARAM);
>>> +
>>> + pdata.mode_bits = SPI_CS_HIGH;
>>> + if (FIELD_GET(PARAM_CLK_POL, v))
>>> + pdata.mode_bits |= SPI_CPOL;
>>> + if (FIELD_GET(PARAM_CLK_PHASE, v))
>>> + pdata.mode_bits |= SPI_CPHA;
>>> +
>>> + pdata.num_chipselect = FIELD_GET(PARAM_NUM_CS, v);
>>> + pdata.bits_per_word_mask =
>>> + SPI_BPW_RANGE_MASK(1, FIELD_GET(PARAM_DATA_WIDTH, v));
>>> +
>>> + pdata.num_devices = 1;
>>> + pdata.devices = &m10_n3000_info;
>>> +
>>> + dev_dbg(ns->dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
>>> + pdata.num_chipselect, pdata.bits_per_word_mask,
>>> + pdata.mode_bits);
>>> +
>>> + pdevinfo.name = "subdev_spi_altera";
>>> + pdevinfo.id = PLATFORM_DEVID_AUTO;
>>> + pdevinfo.parent = ns->dev;
>>> + pdevinfo.data = &pdata;
>>> + pdevinfo.size_data = sizeof(pdata);
>>> +
>>> + ns->altera_spi = platform_device_register_full(&pdevinfo);
>>> + return PTR_ERR_OR_ZERO(ns->altera_spi);
>>> +}
>>> +
>>> +static void destroy_altera_spi_controller(struct n3000_nios *ns)
>>> +{
>>> + platform_device_unregister(ns->altera_spi);
>>> +}
>>> +
>>> +/* ns is the abbreviation of nios_spi */
>>> +static int ns_poll_stat_timeout(void __iomem *base, u64 *v)
>>> +{
>>> + int loops = NS_REGBUS_WAIT_TIMEOUT;
>>> +
>>> + /*
>>> + * We don't use the time based timeout here for performance.
>>> + *
>>> + * The regbus read/write is on the critical path of Intel PAC N3000
>>> + * image programing. The time based timeout checking will add too much
>>> + * overhead on it. Usually the state changes in 1 or 2 loops on the
>>> + * test server, and we set 10000 times loop here for safety.
>>> + */
>>> + do {
>>> + *v = readq(base + NIOS_SPI_STAT);
>>> + if (*v & STAT_RW_VAL)
>>> + break;
>>> + cpu_relax();
>>> + } while (--loops);
>> This becomes an infinite loop if loops <= 0
> The loops count is always initialized at the start of the function.
> Do you mean we should lower the impact when the developer inputs the
> wrong loop count?
>
> Is it better we change to:
>
> unsigned int loops = NS_REGBUS_WAIT_TIMEOUT;
>
> while (loops--) {
>
> }
yes
>>> +
>>> + return loops ? 0 : -ETIMEDOUT;
>>> +}
>>> +
>>> +static int ns_reg_write(void *context, unsigned int reg, unsigned int val)
>>> +{
>>> + struct n3000_nios *ns = context;
>>> + u64 v = 0;
>>> + int ret;
>>> +
>>> + v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_WR);
>>> + v |= FIELD_PREP(CTRL_ADDR, reg);
>>> + v |= FIELD_PREP(CTRL_WR_DATA, val);
>>> + writeq(v, ns->base + NIOS_SPI_CTRL);
>>> +
>>> + ret = ns_poll_stat_timeout(ns->base, &v);
>>> + if (ret)
>>> + dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
>>> + reg, val, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int ns_reg_read(void *context, unsigned int reg, unsigned int *val)
>>> +{
>>> + struct n3000_nios *ns = context;
>>> + u64 v = 0;
>>> + int ret;
>>> +
>>> + v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_RD);
>> v = , and save the intialiation
>>
>> or v = ( ... | ... )
> Yes.
>
>>> + v |= FIELD_PREP(CTRL_ADDR, reg);
>>> + writeq(v, ns->base + NIOS_SPI_CTRL);
>>> +
>>> + ret = ns_poll_stat_timeout(ns->base, &v);
>>> + if (ret)
>>> + dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
>>> + else
>>> + *val = FIELD_GET(STAT_RD_DATA, v);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static const struct regmap_config ns_regbus_cfg = {
>>> + .reg_bits = 32,
>>> + .reg_stride = 4,
>>> + .val_bits = 32,
>>> + .fast_io = true,
>>> +
>>> + .reg_write = ns_reg_write,
>>> + .reg_read = ns_reg_read,
>>> +};
>>> +
>>> +static int n3000_nios_probe(struct dfl_device *ddev)
>>> +{
>>> + struct device *dev = &ddev->dev;
>>> + struct n3000_nios *ns;
>>> + int ret;
>>> +
>>> + ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
>>> + if (!ns)
>>> + return -ENOMEM;
>>> +
>>> + dev_set_drvdata(&ddev->dev, ns);
>>> +
>>> + ns->dev = dev;
>>> +
>>> + ns->base = devm_ioremap_resource(&ddev->dev, &ddev->mmio_res);
>>> + if (IS_ERR(ns->base))
>>> + return PTR_ERR(ns->base);
>>> +
>>> + ns->regmap = devm_regmap_init(dev, NULL, ns, &ns_regbus_cfg);
>>> + if (IS_ERR(ns->regmap))
>>> + return PTR_ERR(ns->regmap);
>>> +
>>> + ret = n3000_nios_init_done_check(ns);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = create_altera_spi_controller(ns);
>>> + if (ret)
>>> + dev_err(dev, "altera spi controller create failed: %d\n", ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void n3000_nios_remove(struct dfl_device *ddev)
>>> +{
>>> + struct n3000_nios *ns = dev_get_drvdata(&ddev->dev);
>>> +
>>> + destroy_altera_spi_controller(ns);
>>> +}
>>> +
>>> +#define FME_FEATURE_ID_N3000_NIOS 0xd
>>> +
>>> +static const struct dfl_device_id n3000_nios_ids[] = {
>>> + { FME_ID, FME_FEATURE_ID_N3000_NIOS },
>>> + { }
>>> +};
>>> +
>>> +static struct dfl_driver n3000_nios_driver = {
>>> + .drv = {
>>> + .name = "n3000-nios",
>> "dfl-n3000-nios" ?
> Yes. I think it is better.
>
> Thanks,
> Yilun
Tom
>> Tom
>>
>>> + .dev_groups = n3000_nios_groups,
>>> + },
>>> + .id_table = n3000_nios_ids,
>>> + .probe = n3000_nios_probe,
>>> + .remove = n3000_nios_remove,
>>> +};
>>> +
>>> +module_dfl_driver(n3000_nios_driver);
>>> +
>>> +MODULE_DEVICE_TABLE(dfl, n3000_nios_ids);
>>> +MODULE_DESCRIPTION("Driver for Nios private feature on Intel PAC N3000");
>>> +MODULE_AUTHOR("Intel Corporation");
>>> +MODULE_LICENSE("GPL v2");