Re: [PATCH v7 3/3] fpga: dfl: add support for N3000 Nios private feature

[Xu Yilun <yilun.xu@intel.com>]

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.