Date:   Fri, 14 Aug 2020 06:55:32 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     "Xu, Yilun" <yilun.xu@...el.com>
CC:     "mdf@...nel.org" <mdf@...nel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "trix@...hat.com" <trix@...hat.com>,
        "lgoncalv@...hat.com" <lgoncalv@...hat.com>,
        Matthew Gerlach <matthew.gerlach@...ux.intel.com>,
        "Weight, Russell H" <russell.h.weight@...el.com>
Subject: RE: [PATCH v5 3/3] fpga: dfl: add support for N3000 Nios private
  feature

> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > index 7cd5a29..f6252cd 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_N3000_NIOS
> >
> > FPGA_DFL_NIOS_INTEL_PAC_N3000
> 
> Why we need to be that specific? I think we don't have to add so many
> info for the naming. dfl N3000 nios is unique and aligned with the file
> name and driver name.

Looks like this driver only works for this card, not designed for common
reuse I think. This is why I am thinking if it can be more specific, and 
matches with descriptions below.

> 
> >
> > > +        tristate "FPGA DFL N3000 NIOS Driver"
> >
> > FPGA DFL NIOS Driver for Intel PAC N3000
> >
> > > +        depends on FPGA_DFL
> > > +        select REGMAP
> > > +        help
> > > +  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.
> > > +
> > >  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..27f20f2 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_N3000_NIOS)      += 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..aeac224
> > > --- /dev/null
> > > +++ b/drivers/fpga/dfl-n3000-nios.c
> > > @@ -0,0 +1,528 @@
> > > +// 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@...el.com>
> > > + *   Xu Yilun <yilun.xu@...el.com>
> > > + */
> > > +#include <linux/bitfield.h>
> > > +#include <linux/errno.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>
> > > +
> > > +#include "dfl.h"
> > > +
> > > +static char *fec_mode = "rs";
> > > +module_param(fec_mode, charp, 0444);
> > > +MODULE_PARM_DESC(fec_mode, "FEC mode of the ethernet retimer on
> > > Intel PAC N3000");
> > > +
> > > +/* N3000 Nios private feature registers */
> > > +#define NIOS_SPI_PARAM0x8
> > > +#define PARAM_SHIFT_MODE_MSKBIT_ULL(1)
> > > +#define PARAM_SHIFT_MODE_MSB0
> > > +#define PARAM_SHIFT_MODE_LSB1
> > > +#define PARAM_DATA_WIDTHGENMASK_ULL(7, 2)
> > > +#define PARAM_NUM_CSGENMASK_ULL(13, 8)
> > > +#define PARAM_CLK_POLBIT_ULL(14)
> > > +#define PARAM_CLK_PHASEBIT_ULL(15)
> > > +#define PARAM_PERIPHERAL_IDGENMASK_ULL(47, 32)
> > > +
> > > +#define NIOS_SPI_CTRL0x10
> > > +#define CTRL_WR_DATAGENMASK_ULL(31, 0)
> > > +#define CTRL_ADDRGENMASK_ULL(44, 32)
> > > +#define CTRL_CMD_MSKGENMASK_ULL(63, 62)
> > > +#define CTRL_CMD_NOP0
> > > +#define CTRL_CMD_RD1
> > > +#define CTRL_CMD_WR2
> > > +
> > > +#define NIOS_SPI_STAT0x18
> > > +#define STAT_RD_DATAGENMASK_ULL(31, 0)
> > > +#define STAT_RW_VALBIT_ULL(32)
> > > +
> > > +/* Nios handshake registers, indirect access */
> > > +#define NIOS_INIT0x1000
> > > +#define NIOS_INIT_DONEBIT(0)
> > > +#define NIOS_INIT_STARTBIT(1)
> > > +/* Mode for PKVL A, link 0, the same below */
> > > +#define REQ_FEC_MODE_A0_MSKGENMASK(9, 8)
> > > +#define REQ_FEC_MODE_A1_MSKGENMASK(11, 10)
> > > +#define REQ_FEC_MODE_A2_MSKGENMASK(13, 12)
> > > +#define REQ_FEC_MODE_A3_MSKGENMASK(15, 14)
> > > +#define REQ_FEC_MODE_B0_MSKGENMASK(17, 16)
> > > +#define REQ_FEC_MODE_B1_MSKGENMASK(19, 18)
> > > +#define REQ_FEC_MODE_B2_MSKGENMASK(21, 20)
> > > +#define REQ_FEC_MODE_B3_MSKGENMASK(23, 22)
> > > +#define REQ_FEC_MODE_NO0x0
> > > +#define REQ_FEC_MODE_KR0x1
> > > +#define REQ_FEC_MODE_RS0x2
> > > +
> > > +#define NIOS_FW_VERSION0x1004
> > > +#define NIOS_FW_VERSION_PATCHGENMASK(23, 20)
> > > +#define NIOS_FW_VERSION_MINORGENMASK(27, 24)
> > > +#define NIOS_FW_VERSION_MAJORGENMASK(31, 28)
> > > +
> > > +#define PKVL_A_MODE_STS0x1020
> > > +#define PKVL_B_MODE_STS0x1024
> > > +#define PKVL_MODE_STS_GROUP_MSKGENMASK(15, 8)
> > > +#define PKVL_MODE_STS_GROUP_OK0x0
> > > +#define PKVL_MODE_STS_ID_MSKGENMASK(7, 0)
> > > +/* When GROUP MASK field == GROUP_OK  */
> > > +#define PKVL_MODE_ID_RESET0x0
> > > +#define PKVL_MODE_ID_4X10G0x1
> > > +#define PKVL_MODE_ID_4X25G0x2
> > > +#define PKVL_MODE_ID_2X25G0x3
> > > +#define PKVL_MODE_ID_2X25G_2X10G0x4
> > > +#define PKVL_MODE_ID_1X25G0x5
> > > +
> > > +#define NS_REGBUS_WAIT_TIMEOUT10000/* loop count */
> > > +#define NIOS_INIT_TIMEOUT10000000/* usec */
> > > +#define NIOS_INIT_TIME_INTV100000/* 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_CONFIGURABLE(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 fec_mode_show(struct device *dev,
> > > +     struct device_attribute *attr, char *buf)
> > > +{
> > > +struct n3000_nios *ns = dev_get_drvdata(dev);
> > > +unsigned int val, retimer_a_mode, retimer_b_mode, fec_mode;
> > > +int ret;
> > > +
> > > +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_CONFIGURABLE(retimer_a_mode) &&
> > > +    !IS_RETIMER_FEC_CONFIGURABLE(retimer_b_mode))
> > > +return sprintf(buf, "no\n");
> >
> > It needs to be defined clearly, configurable seems a little confusing.
> > It seems that hardware supports FEC mode but software can't change it.
> 
> So let's name it IS_RETIMER_FEC_SUPPORTED(), is that OK?
> 
> > Is that true? If it's in some hardware version doesn't support FEC, then
> > We can make this sysfs not visible, right?
> 
> Since now we always accepts the Module Parameter regardless the FW
> version, I think it would be reasonable we always have the sysfs to
> feedback the result of configuration.
> 
> How do you think about it?

Yes, we can keep it, or return "not supported" in this sysfs in case it's not supported.

> > > +static void dump_error_stat(struct n3000_nios *ns)
> > > +{
> > > +unsigned int val;
> > > +
> > > +if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
> > > +return;
> > > +
> > > +dev_info(ns->dev, "PKVL_A_MODE_STS 0x%x\n", val);
> >
> > dev_err is better?
> 
> The logs will be printed out when the retimer configuration fails, in
> this case the module load will still success. Would it be confusing that
> driver prints error level msg but it doesn't fail out.

will these messages print only when hits errors/fails configuration with 
hardware? 

Thanks
Hao

Powered by blists - more mailing lists