Messages in this thread | | | Date | Mon, 7 Sep 2020 14:13:48 +0800 | From | Xu Yilun <> | Subject | Re: [PATCH v7 3/3] fpga: dfl: add support for N3000 Nios private feature |
| |
On Fri, Sep 04, 2020 at 05:45:12PM -0700, Moritz Fischer wrote: > Hi Xu, > > On Wed, Aug 19, 2020 at 03:45:21PM +0800, 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 Nois firmware, which receives > FPGA *NIOS* or *Nios* I think ;-)
Thanks for catching the misspelling.
> > +Configuring the ethernet retimer > > +================================ > > + > > +The Intel PAC N3000 is a FPGA based SmartNIC platform which could be programmed > > +to various configurations (with different link numbers and speeds, e.g. 8x10G, > > +4x25G ...). And the retimer chips should also be configured correspondingly by > > +Nios firmware. There are 2 retimer chips on the board, each of them supports 4 > > +links. For example, in 8x10G configuration, the 2 retimer chips are both set to > > +4x10G mode, while in 4x25G configuration, retimer A is set to 4x25G and retimer > > +B is in reset. For now, the Nios firmware only supports 10G and 25G mode > > +setting for the retimer chips. > Make sure your API above exposes all of that.
Yes. I could add the sysfs interfaces for query of the retimer mode.
> > +Sysfs Attributes > > +================ > > + > > +The driver creates the sysfs file in /sys/bus/dfl/devices/dfl_dev.X/ > > + > > +* fec_mode: > > + Read-only. It shows the FEC mode of the 25G links of the ethernet retimers. > > + The possible values are the same as the fec_mode module parameter. An extra > > + value "not supported" is returned if firmware version major < 3, or no link > > + is configured to 25G. > > This seems somewhat duplicate, runs the risk to get out of sync with the > ABI doc?
Yes. I could just reference the file in Documentation/ABI
> > +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; > Can you make this reverse x-mas tree if possible?
Yes, I'll change it.
> > +static int init_error_detected(struct n3000_nios *ns) > Do you want to make the function either bool, or the return values ints?
I can make it bool.
> > +{ > > + 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; > > +}
> > + if (val == 0) { > > + dev_err(dev, "Nios version reg = 0x%x, skip INIT_DONE check, but the retimer may be uninitialized\n", > This looks long, any chance you can linebreak this?
I tried to break the line by dev_err(dev, "XXX" "XXX", val);
But checkpatch says "WARNING: quoted string split across lines".
I see checkpatch allows long lines for logging functions, so could we keep it unchanged?
> > +struct spi_board_info m10_n3000_info = { > > + .modalias = "m10-n3000", > Did I miss the patch for this, or did this only go to the SPI folks?
I didn't send the MAX 10 patchset in fpga domain, It goes to the SPI & MFD maintainers, please see: https://lkml.org/lkml/2020/8/19/172
The regmap-spi-avmm has been applied for linux-next. The MAX 10 base driver is under review.
> > + .max_speed_hz = 12500000, > > + .bus_num = 0, > > + .chip_select = 0, > > +};
Thanks, Yilun.
| |