Messages in this thread | | | Date | Mon, 2 Jan 2023 10:25:00 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1 |
| |
On Fri, 30 Dec 2022, Xu Yilun wrote:
> On 2022-12-28 at 10:16:23 -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 >> 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() > > Could you help check my comments? > https://lore.kernel.org/linux-fpga/alpine.DEB.2.22.394.2212211421210.570436@rhweight-WRK1/T/#md86e3836130ebacd3c088f5c512ba741aac8a4d1
Sorry I missed your earlier comments. I have since responded to them specificially.
> > [...] > >> >> +static u64 *find_param(u64 *params, resource_size_t max, int param_id) >> +{ >> + u64 *end = params + max / sizeof(u64); >> + u64 v, next; >> + >> + 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; >> + } >> + >> + 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); > > Didn't find where to use it?
I understand. Don't export a function unless there is a comsumer for it.
> >> + >> +/** >> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id > > There is no rule to say one u64 for each property in the parameter block. > So I don't see the reason for DFL core to provide u64 array for the API, > And the size of the parameter block is decided by HW, why make the user > input the value? > > As we discussed before, dfl core doesn't try to look into the parameter > block. So please just provide the const void *data & data_size for drivers. > This is the most common way to represent a data block.
I will move the parameter parsing helper function to the driver itself.
Thanks for the feedback, Matthew Gerlach
> > Thanks, > Yilun > >> + * @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); >> + >> + for (i = 0; i < nvals; i++) >> + *pval++ = param[i + 1]; >> + >> + return param; >> +} >> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals); >> + >
| |