[v8,3/4] fpga: dfl: add basic support for DFHv1

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Version 1 of the Device Feature Header (DFH) definition adds
functionality to the DFL bus.

A DFHv1 header may have one or more parameter blocks that
further describes the HW to SW.  Add support to the DFL bus
to parse the MSI-X parameter.

The location of a feature's register set is explicitly
described in DFHv1 and can be relative to the base of the DFHv1
or an absolute address.  Parse the location and pass the information
to DFL driver.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v8: use struct_size() from overflow.h
    add dfh_get_u64_param_vals()

v7: no change

v6: move MSI_X parameter definitions to drivers/fpga/dfl.h

v5: update field names
    fix find_param/dfh_get_psize
    clean up mmio_res assignments
    use u64* instead of void*
    use FIELD_GET instead of masking

v4: s/MSIX/MSI_X
    move kernel doc to implementation
    use structure assignment
    fix decode of absolute address
    clean up comment in parse_feature_irqs
    remove use of csr_res

v3: remove unneeded blank line
    use clearer variable name
    pass finfo into parse_feature_irqs()
    refactor code for better indentation
    use switch statement for irq parsing
    squash in code parsing register location

v2: fix kernel doc
    clarify use of DFH_VERSION field
---
 drivers/fpga/dfl.c  | 265 +++++++++++++++++++++++++++++++++++---------
 drivers/fpga/dfl.h  |   9 ++
 include/linux/dfl.h |   6 +
 3 files changed, 229 insertions(+), 51 deletions(-)

On Thu, 29 Dec 2022, Andy Shevchenko wrote:

> On Wed, Dec 28, 2022 at 10:16:23AM -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Version 1 of the Device Feature Header (DFH) definition adds
>> functionality to the DFL bus.
>>
>> A DFHv1 header may have one or more parameter blocks that
>> further describes the HW to SW.  Add support to the DFL bus
>
> Single space is enough.

Yes, single space is enough. Two spaces after a period is hard habit for 
me break. I will update in v9.

>
>> to parse the MSI-X parameter.
>>
>> The location of a feature's register set is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address.  Parse the location and pass the information
>> to DFL driver.
>
> I'm wondering what DFL states for.

I will define DFL in the commit message like DFH in the next revision.

>
> ...
>
>> +/**
>> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + * @pval: location of parameter data destination
>> + * @nvals: number of u64 elements of parameter data
>> + *
>> + * Return: pointer to start of parameter block, PTR_ERR otherwise
>> + */
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
>> +{
>> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +	u64 next;
>> +	int i;
>> +
>> +	if (!param)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
>> +
>> +	if (nvals >= next)
>> +		return ERR_PTR(-ENOMEM);
>
> ENODATA ?

ENODATA does seem to be more accurate than ENOMEM in this case.

>
>> +	for (i = 0; i < nvals; i++)
>> +		*pval++ = param[i + 1];
>
> memcpy() ?

Using memcpy() will make code cleaner.

>
>> +	return param;
>> +}
>
> ...
>
>> +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);
>
> ' / ' (mind the spaces)

Yes, I will mind the spaces.

>
> Also, perhaps better to use sizeof(*params) or what is the member of that
> structure. So it will be more robust against possible changes.

params is the name of the structure member that is the trailing array.

>
>>  	if (!finfo)
>>  		return -ENOMEM;
>
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks for the feedback,
Matthew Gerlach

>
>
>

On Thu, 29 Dec 2022, Tom Rix wrote:

> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> 
>> +
>> +	while (params < end) {
>> +		v = *params;
>> +		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return params;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		params += next;
>> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
>> +			break;
> Move before next =, and save some work calculating unsed next

Thanks for the suggestion.  Yes, the change will save some work, and the 
code will look better.

>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find parameter block for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to start of parameter block, NULL otherwise.
>> + */
>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>> +{
>> +	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>
> This and similar should be noop's for dfh v0
>
> Generally there should be better checking if code is running a v1 vs  v0 
> header.

With v0, dfl_dev->params will be NULL and dfl_dev->param_size will be 
zero. Earlier feedback suggested to use the internal NULL/size checking 
instead of adding redundant checks.

>
>> +
>> +/**
>> + * dfh_get_u64_param_vals() - get array of u64 param values for given 
>> parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + * @pval: location of parameter data destination
>> + * @nvals: number of u64 elements of parameter data
>> + *
>> + * Return: pointer to start of parameter block, PTR_ERR otherwise
>> + */
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 
>> *pval, int nvals)
> nvals should be unsigned
>> +{
>> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, 
>> param_id);
>> +	u64 next;
>> +	int i;
>> +
>> +	if (!param)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
>> +
>> +	if (nvals >= next)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = 0; i < nvals; i++)
>> +		*pval++ = param[i + 1];
> memcpy ?

Yes, memcpy would be better.

>> +
>> +	return param;
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
>> +
>>   static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> -			      resource_size_t ofst, u16 fid,
>> -			      unsigned int *irq_base, unsigned int *nr_irqs)
>> +			      resource_size_t ofst, struct dfl_feature_info 
>> *finfo)
>>   {
>>   	void __iomem *base = binfo->ioaddr + ofst;
>>   	unsigned int i, ibase, inr = 0;
>> +	void *params = finfo->params;
>>   	enum dfl_id_type type;
>> +	u16 fid = finfo->fid;
>>   	int virq;
>> +	u64 *p;
>>   	u64 v;
>>   -	type = feature_dev_id_type(binfo->feature_dev);
>> +	switch (finfo->dfh_version) {
>> +	case 0:
>
> A switch makes this code complicated to read.

Previous feedback suggested a switch statement better than if/else.

>
> Have you thought about a function pointer table to separate out similar v1 vs 
> v0 code blocks?

I haven't though of a function pointer table, and I'm not sure how much 
clearer it would be, but I'll consider it.

>
>> +		/*
>> +		 * DFHv0 only provides MMIO resource information for each 
>> feature
>> +		 * in the DFL header.  There is no generic interrupt 
>> information.
>> +		 * Instead, features with interrupt functionality provide
>> +		 * the information in feature specific registers.
>> +		 */
>> +		type = feature_dev_id_type(binfo->feature_dev);
>> +		if (type == PORT_ID) {
>> +			switch (fid) {
>> +			case PORT_FEATURE_ID_UINT:
>> +				v = readq(base + PORT_UINT_CAP);
>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +				break;
>> +			case PORT_FEATURE_ID_ERROR:
>> +				v = readq(base + PORT_ERROR_CAP);
>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, 
>> v);
>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		} else if (type == FME_ID) {
>> +			switch (fid) {
>> +			case FME_FEATURE_ID_GLOBAL_ERR:
>> +				v = readq(base + FME_ERROR_CAP);
>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		}
>> +		break;
>>   -	/*
>> -	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> -	 * each feature in DFL Header, no field for interrupt resources.
>> -	 * Interrupt resource information is provided by specific mmio
>> -	 * registers of each private feature which supports interrupt. So in
>> -	 * order to parse and assign irq resources, DFL framework has to look
>> -	 * into specific capability registers of these private features.
>> -	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> -	 * DFL, the driver may still fall back to these quirks.
>> -	 */
>> -	if (type == PORT_ID) {
>> -		switch (fid) {
>> -		case PORT_FEATURE_ID_UINT:
>> -			v = readq(base + PORT_UINT_CAP);
>> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> -			break;
>> -		case PORT_FEATURE_ID_ERROR:
>> -			v = readq(base + PORT_ERROR_CAP);
>> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +	case 1:
>> +		/*
>> +		 * DFHv1 provides interrupt resource information in DFHv1
>> +		 * parameter blocks.
>> +		 */
>> +		p = find_param(params, finfo->param_size, 
>> DFHv1_PARAM_ID_MSI_X);
>> +		if (!p)
>>   			break;
>> -		}
>> -	} else if (type == FME_ID) {
>> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> -			v = readq(base + FME_ERROR_CAP);
>> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> -		}
>> +
>> +		p++;
>> +		ibase = FIELD_GET(DFHv1_PARAM_MSI_X_STARTV, *p);
>> +		inr = FIELD_GET(DFHv1_PARAM_MSI_X_NUMV, *p);
>> +		break;
>> +
>> +	default:
>> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", 
>> finfo->dfh_version);
>> +		break;
>>   	}
>>     	if (!inr) {
>> -		*irq_base = 0;
>> -		*nr_irqs = 0;
>> +		finfo->irq_base = 0;
>> +		finfo->nr_irqs = 0;
>>   		return 0;
>>   	}
>>   @@ -1006,12 +1114,37 @@ static int parse_feature_irqs(struct 
>> build_feature_devs_info *binfo,
>>   		}
>>   	}
>>   -	*irq_base = ibase;
>> -	*nr_irqs = inr;
>> +	finfo->irq_base = ibase;
>> +	finfo->nr_irqs = inr;
>>     	return 0;
>>   }
>>   +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> for clarity, psize should be expanded to param_size

s/psize/param_size would be clearer and more consistent.

>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!next)
>> +			return -EINVAL;
>> +
>> +		size += next * sizeof(u64);
>> +
>> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
>> +			return size;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>>   /*
>>    * when create sub feature instances, for private features, it doesn't 
>> need
>>    * to provide resource size and feature id as they could be read from DFH
>> @@ -1023,39 +1156,69 @@ static int
>>   create_feature_instance(struct build_feature_devs_info *binfo,
>>   			resource_size_t ofst, resource_size_t size, u16 fid)
>>   {
>> -	unsigned int irq_base, nr_irqs;
>>   	struct dfl_feature_info *finfo;
>> +	resource_size_t start, end;
>> +	int dfh_psize = 0;
>>   	u8 revision = 0;
>> +	u64 v, addr_off;
>> +	u8 dfh_ver = 0;
>>   	int ret;
>> -	u64 v;
>>     	if (fid != FEATURE_ID_AFU) {
>>   		v = readq(binfo->ioaddr + ofst);
>>   		revision = FIELD_GET(DFH_REVISION, v);
>> -
>> +		dfh_ver = FIELD_GET(DFH_VERSION, v);
>>   		/* read feature size and id if inputs are invalid */
>>   		size = size ? size : feature_size(v);
>>   		fid = fid ? fid : feature_id(v);
>> +		if (dfh_ver == 1) {
>> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, 
>> size);
>> +			if (dfh_psize < 0) {
>> +				dev_err(binfo->dev,
>> +					"failed to read size of DFHv1 
>> parameters %d\n",
>> +					dfh_psize);
>> +				return dfh_psize;
>> +			}
>> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
>> +		}
>>   	}
>>     	if (binfo->len - ofst < size)
>>   		return -EINVAL;
>>   -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> -	if (ret)
>> -		return ret;
>> -
>> -	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>> +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), 
>> GFP_KERNEL);
>>   	if (!finfo)
>>   		return -ENOMEM;
>>   +	memcpy_fromio(finfo->params, binfo->ioaddr + ofst + DFHv1_PARAM_HDR, 
>> dfh_psize);
>> +	finfo->param_size = dfh_psize;
>> +
>>   	finfo->fid = fid;
>>   	finfo->revision = revision;
>> -	finfo->mmio_res.start = binfo->start + ofst;
>> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	finfo->dfh_version = dfh_ver;
>> +	if (dfh_ver == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		addr_off = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +		if (FIELD_GET(DFHv1_CSR_ADDR_REL, v))
>> +			start = addr_off << 1;
>> +		else
>> +			start = binfo->start + ofst + addr_off;
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		end = start + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>> +	} else {
>> +		start = binfo->start + ofst;
>> +		end = start + size - 1;
>> +	}
>>   	finfo->mmio_res.flags = IORESOURCE_MEM;
>> -	finfo->irq_base = irq_base;
>> -	finfo->nr_irqs = nr_irqs;
>> +	finfo->mmio_res.start = start;
>> +	finfo->mmio_res.end = end;
>> +
>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>> +	if (ret) {
>> +		kfree(finfo);
>> +		return ret;
>> +	}
>>     	list_add_tail(&finfo->node, &binfo->sub_features);
>>   	binfo->feature_num++;
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index fc59f33367ee..a21065cec04e 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -111,6 +111,10 @@
>>   #define DFHv1_PARAM_HDR_NEXT_EOP	BIT_ULL(32)
>>   #define DFHv1_PARAM_DATA		0x08  /* Offset of Param data from 
>> Param header */
>>   +#define DFHv1_PARAM_ID_MSI_X		0x1
>> +#define DFHv1_PARAM_MSI_X_NUMV		GENMASK_ULL(63, 32)
>> +#define DFHv1_PARAM_MSI_X_STARTV	GENMASK_ULL(31, 0)
>> +
>>   /* Next AFU Register Bitfield */
>>   #define NEXT_AFU_NEXT_DFH_OFST	GENMASK_ULL(23, 0)	/* Offset to 
>> next AFU */
>>   @@ -272,11 +276,14 @@ struct dfl_feature_irq_ctx {
>>    * @ops: ops of this sub feature.
>>    * @ddev: ptr to the dfl device of this sub feature.
>>    * @priv: priv data of this feature.
>> + * @param_size: size of dfh parameters
>> + * @params: point to memory copy of dfh parameters
>>    */
>>   struct dfl_feature {
>>   	struct platform_device *dev;
>>   	u16 id;
>>   	u8 revision;
>> +	u8 dfh_version;
>
> Needs a description in the comments above.

Thanks for catching that.

>
> I am concerned generally about backward compatibility and oot drivers that 
> are dfh v0.

I agree it would be much safter to add the new fields to the end of the 
structure for compatibility reasons. I will do this for struct dfl_device 
as well.

Thanks for the review,
Matthew Gerlach


>
> Adding an element in the middle will break things.
>
> These structures could be more explicit about what is common, and what is v0 
> and v1 specific.
>
> Tom
>
>>   	int resource_index;
>>   	void __iomem *ioaddr;
>>   	struct dfl_feature_irq_ctx *irq_ctx;
>> @@ -284,6 +291,8 @@ struct dfl_feature {
>>   	const struct dfl_feature_ops *ops;
>>   	struct dfl_device *ddev;
>>   	void *priv;
>> +	unsigned int param_size;
>> +	void *params;
>>   };
>>     #define FEATURE_DEV_ID_UNUSED	(-1)
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..3138ea42d02e 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -39,9 +39,12 @@ struct dfl_device {
>>   	u16 type;
>>   	u16 feature_id;
>>   	u8 revision;
>> +	u8 dfh_version;
>>   	struct resource mmio_res;
>>   	int *irqs;
>>   	unsigned int num_irqs;
>> +	unsigned int param_size;
>> +	void *params;
>>   	struct dfl_fpga_cdev *cdev;
>>   	const struct dfl_device_id *id_entry;
>>   };
>> @@ -84,4 +87,7 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>>   	module_driver(__dfl_driver, dfl_driver_register, \
>>   		      dfl_driver_unregister)
>>   +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param);
>> +
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 
>> *pval, int nvals);
>>   #endif /* __LINUX_DFL_H */
>
>

On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:

...

> >   struct dfl_feature_info {
> >   	u16 fid;
> >   	u8 revision;
> > +	u8 dfh_version;
> >   	struct resource mmio_res;
> >   	void __iomem *ioaddr;
> >   	struct list_head node;
> >   	unsigned int irq_base;
> >   	unsigned int nr_irqs;
> > +	unsigned int param_size;
> > +	u64 params[];
> u64 *params

This will break the overflow.h macros, no?
Besides that it will break the code for sure as it's not an equivalent.

> >   };

On Mon, Jan 02, 2023 at 08:54:48AM -0800, matthew.gerlach@linux.intel.com wrote:
> On Thu, 29 Dec 2022, Andy Shevchenko wrote:
> > On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
> > > On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:

...

> > > > +	u64 params[];
> > > u64 *params

> > This will break the overflow.h macros, no?
> > Besides that it will break the code for sure as it's not an equivalent.

> I don't understand how this will break the overflow.h macros. The definition
> of struct dfl_feature_info and all of its uses are in a single file, dfl.c.

Hint: __must_be_array()

As I said, the proposed change is not acceptable since it's not an equivalent.

On Mon, 2 Jan 2023, Andy Shevchenko wrote:

> On Mon, Jan 02, 2023 at 07:27:06PM +0200, Andy Shevchenko wrote:
>> On Mon, Jan 02, 2023 at 08:54:48AM -0800, matthew.gerlach@linux.intel.com wrote:
>>> On Thu, 29 Dec 2022, Andy Shevchenko wrote:
>>>> On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
>>>>> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>>>> +	u64 params[];
>>>>> u64 *params
>>
>>>> This will break the overflow.h macros, no?
>>>> Besides that it will break the code for sure as it's not an equivalent.
>>
>>> I don't understand how this will break the overflow.h macros. The definition
>>> of struct dfl_feature_info and all of its uses are in a single file, dfl.c.
>>
>> Hint: __must_be_array()
>>
>> As I said, the proposed change is not acceptable since it's not an equivalent.
>
> Ah, you meant that there is no use of macros from overflow in the dfl.c?
> IIRC we discussed that some of the code may make use of them, or am I
> mistaken?

There currently is one usage of struct_size() from overflow.h in dfl.c, 
and my patch adds another usage of struct_size(). struct dfl_feature_info 
ends with a trailing array of u64.

I think the confusion is with struct dfl_feature and/or struct struct 
dfl_device. Those structs don't end with a trailing array, and those 
structs are not used with macros from overview.h.

Thanks for the feedback,
Matthew Gerlach

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>