From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Define and use a DFHv1 parameter to add generic support for MSIX
interrupts for DFL devices.
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2: fix kernel doc
clarify use of DFH_VERSION field
---
drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
include/linux/dfl.h | 14 +++++++++++
2 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 1132f3c10440..dfd3f563c92d 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
void __iomem *base = binfo->ioaddr + ofst;
unsigned int i, ibase, inr = 0;
enum dfl_id_type type;
- int virq;
+ int virq, off;
u64 v;
type = feature_dev_id_type(binfo->feature_dev);
/*
* Ideally DFL framework should only read info from DFL header, but
- * current version DFL only provides mmio resources information for
+ * current version, DFHv0, 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
+ * DFHv1 supports generic interrupt resource information in DFHv1
+ * parameter blocks. But in order to be compatible to old version
* DFL, the driver may still fall back to these quirks.
*/
if (type == PORT_ID) {
@@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
}
}
+ if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
+ fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
+
+ v = FIELD_GET(DFH_VERSION, readq(base));
+ switch (v) {
+ case 0:
+ break;
+
+ case 1:
+ v = readq(base + DFHv1_CSR_SIZE_GRP);
+ if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+ off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
+ DFHv1_PARAM_ID_MSIX);
+ if (off >= 0) {
+ ibase = readl(base + DFHv1_PARAM_HDR +
+ off + DFHv1_PARAM_MSIX_STARTV);
+ inr = readl(base + DFHv1_PARAM_HDR +
+ off + DFHv1_PARAM_MSIX_NUMV);
+ dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
+ ibase, inr, fid);
+ }
+ }
+ break;
+
+ default:
+ dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
+ break;
+ }
+ }
+
if (!inr) {
*irq_base = 0;
*nr_irqs = 0;
@@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
}
EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
+int dfl_find_param(void __iomem *base, resource_size_t max, int param)
+{
+ int off = 0;
+ u64 v, next;
+
+ while (off < max) {
+ v = readq(base + off);
+ if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+ return off;
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ if (!next)
+ break;
+
+ off += next;
+ }
+
+ return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(dfl_find_param);
+
static void __exit dfl_fpga_exit(void)
{
dfl_chardev_uinit();
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 1e53468ba8d8..33e21c360671 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -63,6 +63,10 @@
#define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
#define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
+#define DFHv1_PARAM_ID_MSIX 0x1
+#define DFHv1_PARAM_MSIX_STARTV 0x8
+#define DFHv1_PARAM_MSIX_NUMV 0xc
+
/**
* enum dfl_id_type - define the DFL FIU types
*/
@@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
module_driver(__dfl_driver, dfl_driver_register, \
dfl_driver_unregister)
+/**
+ * dfl_find_param() - find the offset of the given parameter
+ * @base: base pointer to start of dfl parameters in DFH
+ * @max: maximum offset to search
+ * @param: id of dfl parameter
+ *
+ * Return: positive offset on success, negative error code otherwise.
+ */
+int dfl_find_param(void __iomem *base, resource_size_t max, int param);
+
#endif /* __LINUX_DFL_H */
--
2.25.1On 2022-09-23 at 05:17:43 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
> include/linux/dfl.h | 14 +++++++++++
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1132f3c10440..dfd3f563c92d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> enum dfl_id_type type;
> - int virq;
> + int virq, off;
> u64 v;
>
> type = feature_dev_id_type(binfo->feature_dev);
>
> /*
> * Ideally DFL framework should only read info from DFL header, but
> - * current version DFL only provides mmio resources information for
> + * current version, DFHv0, only provides mmio resources information for
With this patchset, it's not 'current version' anymore.
> * 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
> + * DFHv1 supports generic interrupt resource information in DFHv1
> + * parameter blocks. But in order to be compatible to old version
> * DFL, the driver may still fall back to these quirks.
> */
> if (type == PORT_ID) {
> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> }
> }
>
> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
> + v = FIELD_GET(DFH_VERSION, readq(base));
> + switch (v) {
> + case 0:
> + break;
In last version, you mentioned that there will be no quirk for DFLv1, so
how about:
v = FIELD_GET(DFH_VERSION, readq(base));
if (v == 0) {
/* quirks */
} else {
/* parse PARAM MSIX */
}
No need to check specific feature ids again.
Thanks,
Yilun
> +
> + case 1:
> + v = readq(base + DFHv1_CSR_SIZE_GRP);
> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> + DFHv1_PARAM_ID_MSIX);
> + if (off >= 0) {
> + ibase = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_STARTV);
> + inr = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_NUMV);
> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> + ibase, inr, fid);
> + }
> + }
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> + break;
> + }
> + }
> +
> if (!inr) {
> *irq_base = 0;
> *nr_irqs = 0;
> @@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> }
> EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> + int off = 0;
> + u64 v, next;
> +
> + while (off < max) {
> + v = readq(base + off);
> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> + return off;
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + if (!next)
> + break;
> +
> + off += next;
> + }
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(dfl_find_param);
> +
> static void __exit dfl_fpga_exit(void)
> {
> dfl_chardev_uinit();
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 1e53468ba8d8..33e21c360671 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -63,6 +63,10 @@
> #define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
> #define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
>
> +#define DFHv1_PARAM_ID_MSIX 0x1
> +#define DFHv1_PARAM_MSIX_STARTV 0x8
> +#define DFHv1_PARAM_MSIX_NUMV 0xc
> +
> /**
> * enum dfl_id_type - define the DFL FIU types
> */
> @@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> module_driver(__dfl_driver, dfl_driver_register, \
> dfl_driver_unregister)
>
> +/**
> + * dfl_find_param() - find the offset of the given parameter
> + * @base: base pointer to start of dfl parameters in DFH
> + * @max: maximum offset to search
> + * @param: id of dfl parameter
> + *
> + * Return: positive offset on success, negative error code otherwise.
> + */
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
> +
> #endif /* __LINUX_DFL_H */
> --
> 2.25.1
>
On Fri, 30 Sep 2022, Xu Yilun wrote:
> On 2022-09-23 at 05:17:43 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
>> include/linux/dfl.h | 14 +++++++++++
>> 2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1132f3c10440..dfd3f563c92d 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> void __iomem *base = binfo->ioaddr + ofst;
>> unsigned int i, ibase, inr = 0;
>> enum dfl_id_type type;
>> - int virq;
>> + int virq, off;
>> u64 v;
>>
>> type = feature_dev_id_type(binfo->feature_dev);
>>
>> /*
>> * Ideally DFL framework should only read info from DFL header, but
>> - * current version DFL only provides mmio resources information for
>> + * current version, DFHv0, only provides mmio resources information for
>
> With this patchset, it's not 'current version' anymore.
I will update the comment. Thanks.
>
>> * 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
>> + * DFHv1 supports generic interrupt resource information in DFHv1
>> + * parameter blocks. But in order to be compatible to old version
>> * DFL, the driver may still fall back to these quirks.
>> */
>> if (type == PORT_ID) {
>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> }
>> }
>>
>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +
>> + v = FIELD_GET(DFH_VERSION, readq(base));
>> + switch (v) {
>> + case 0:
>> + break;
>
> In last version, you mentioned that there will be no quirk for DFLv1, so
> how about:
>
> v = FIELD_GET(DFH_VERSION, readq(base));
>
> if (v == 0) {
> /* quirks */
> } else {
> /* parse PARAM MSIX */
> }
>
> No need to check specific feature ids again.
With v3 changes I will use a switch state and not need quirks for v1.
>
> Thanks,
> Yilun
>
>> +
>> + case 1:
>> + v = readq(base + DFHv1_CSR_SIZE_GRP);
>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> + DFHv1_PARAM_ID_MSIX);
>> + if (off >= 0) {
>> + ibase = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_NUMV);
>> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> + ibase, inr, fid);
>> + }
>> + }
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> + break;
>> + }
>> + }
>> +
>> if (!inr) {
>> *irq_base = 0;
>> *nr_irqs = 0;
>> @@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>> }
>> EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>>
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
>> +{
>> + int off = 0;
>> + u64 v, next;
>> +
>> + while (off < max) {
>> + v = readq(base + off);
>> + if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> + return off;
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + if (!next)
>> + break;
>> +
>> + off += next;
>> + }
>> +
>> + return -ENOENT;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_find_param);
>> +
>> static void __exit dfl_fpga_exit(void)
>> {
>> dfl_chardev_uinit();
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 1e53468ba8d8..33e21c360671 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -63,6 +63,10 @@
>> #define DFHv1_PARAM_HDR_VERSION GENMASK_ULL(31, 16) /* Version Param */
>> #define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 32) /* Offset of next Param */
>>
>> +#define DFHv1_PARAM_ID_MSIX 0x1
>> +#define DFHv1_PARAM_MSIX_STARTV 0x8
>> +#define DFHv1_PARAM_MSIX_NUMV 0xc
>> +
>> /**
>> * enum dfl_id_type - define the DFL FIU types
>> */
>> @@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>> module_driver(__dfl_driver, dfl_driver_register, \
>> dfl_driver_unregister)
>>
>> +/**
>> + * dfl_find_param() - find the offset of the given parameter
>> + * @base: base pointer to start of dfl parameters in DFH
>> + * @max: maximum offset to search
>> + * @param: id of dfl parameter
>> + *
>> + * Return: positive offset on success, negative error code otherwise.
>> + */
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
>> +
>> #endif /* __LINUX_DFL_H */
>> --
>> 2.25.1
>>
>
On Fri, Sep 23, 2022 at 05:17:43AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
...
> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
Unneeded blank line.
> + v = FIELD_GET(DFH_VERSION, readq(base));
> + switch (v) {
This v...
> + case 0:
> + break;
> +
> + case 1:
> + v = readq(base + DFHv1_CSR_SIZE_GRP);
Extra space.
...and this v are semantically different. It's quite hard to deduce their
semantics.
> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> + DFHv1_PARAM_ID_MSIX);
I guess I have suggested to use temporary variable(s) here.
void __iomem *dfhv1 = base + DFHv1...;
void __iomem *nth;
> + if (off >= 0) {
nth = dfhv1 + off;
> + ibase = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_STARTV);
> + inr = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_NUMV);
ibase = readl(nth + DFHv1_PARAM_MSIX_STARTV);
inr = readl(nth + DFHv1_PARAM_MSIX_NUMV);
> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> + ibase, inr, fid);
> + }
> + }
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> + break;
> + }
> + }
--
With Best Regards,
Andy Shevchenko
On Fri, 23 Sep 2022, Andy Shevchenko wrote:
> On Fri, Sep 23, 2022 at 05:17:43AM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>
> ...
>
>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>
>> +
>
> Unneeded blank line.
I will remove the blank line.
>
>> + v = FIELD_GET(DFH_VERSION, readq(base));
>> + switch (v) {
>
> This v...
>
>> + case 0:
>> + break;
>> +
>> + case 1:
>> + v = readq(base + DFHv1_CSR_SIZE_GRP);
>
> Extra space.
>
> ...and this v are semantically different. It's quite hard to deduce their
> semantics.
I was trying to be consistent with the existing code where the read was
stored in a temporary variable, v, and the FIELD_GET would be used for the
specific field. Will it be sufficiently clear if the v used above is
changed to dfl_ver, and this use of v followed by FIELD_GET remains as is?
>
>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> + DFHv1_PARAM_ID_MSIX);
>
> I guess I have suggested to use temporary variable(s) here.
>
> void __iomem *dfhv1 = base + DFHv1...;
> void __iomem *nth;
>
>> + if (off >= 0) {
>
> nth = dfhv1 + off;
>
>> + ibase = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_NUMV);
>
> ibase = readl(nth + DFHv1_PARAM_MSIX_STARTV);
> inr = readl(nth + DFHv1_PARAM_MSIX_NUMV);
>
>> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> + ibase, inr, fid);
>> + }
>> + }
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> + break;
>> + }
>> + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
> include/linux/dfl.h | 14 +++++++++++
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1132f3c10440..dfd3f563c92d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> void __iomem *base = binfo->ioaddr + ofst;
> unsigned int i, ibase, inr = 0;
> enum dfl_id_type type;
> - int virq;
> + int virq, off;
> u64 v;
>
> type = feature_dev_id_type(binfo->feature_dev);
>
> /*
> * Ideally DFL framework should only read info from DFL header, but
> - * current version DFL only provides mmio resources information for
> + * current version, DFHv0, 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
> + * DFHv1 supports generic interrupt resource information in DFHv1
> + * parameter blocks. But in order to be compatible to old version
> * DFL, the driver may still fall back to these quirks.
> */
> if (type == PORT_ID) {
> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> }
> }
>
> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
> + v = FIELD_GET(DFH_VERSION, readq(base));
I'd call this variable version (or ver) if you want to store it but it
would also fit to switch () line so that no extra variable is needed.
> + switch (v) {
> + case 0:
> + break;
> +
> + case 1:
> + v = readq(base + DFHv1_CSR_SIZE_GRP);
Extra space.
> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> + DFHv1_PARAM_ID_MSIX);
> + if (off >= 0) {
I'd reverse these 2 conditions and break when there's nothing to do.
> + ibase = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_STARTV);
> + inr = readl(base + DFHv1_PARAM_HDR +
> + off + DFHv1_PARAM_MSIX_NUMV);
> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> + ibase, inr, fid);
> + }
> + }
> + break;
> +
> + default:
> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> + break;
> + }
> + }
> +
> if (!inr) {
> *irq_base = 0;
> *nr_irqs = 0;
--
i.
On Fri, 23 Sep 2022, Ilpo Järvinen wrote:
> On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
>> include/linux/dfl.h | 14 +++++++++++
>> 2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1132f3c10440..dfd3f563c92d 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> void __iomem *base = binfo->ioaddr + ofst;
>> unsigned int i, ibase, inr = 0;
>> enum dfl_id_type type;
>> - int virq;
>> + int virq, off;
>> u64 v;
>>
>> type = feature_dev_id_type(binfo->feature_dev);
>>
>> /*
>> * Ideally DFL framework should only read info from DFL header, but
>> - * current version DFL only provides mmio resources information for
>> + * current version, DFHv0, 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
>> + * DFHv1 supports generic interrupt resource information in DFHv1
>> + * parameter blocks. But in order to be compatible to old version
>> * DFL, the driver may still fall back to these quirks.
>> */
>> if (type == PORT_ID) {
>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> }
>> }
>>
>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +
>> + v = FIELD_GET(DFH_VERSION, readq(base));
>
> I'd call this variable version (or ver) if you want to store it but it
> would also fit to switch () line so that no extra variable is needed.
I will change the v to dfh_ver to be clearer. I want to store the
value because it is used in the default case in the error message. The
error message helps to debug broken FPGA images.
>
>> + switch (v) {
>> + case 0:
>> + break;
>> +
>> + case 1:
>> + v = readq(base + DFHv1_CSR_SIZE_GRP);
>
> Extra space.
I will remove extra space.
>
>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> + off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> + DFHv1_PARAM_ID_MSIX);
>> + if (off >= 0) {
>
> I'd reverse these 2 conditions and break when there's nothing to do.
I'm not sure what you mean by reversing these conditions because a DFHv1
may or may not have parameters (the first condition), and a DFHv1 may have
parameters but may not have a MSI-X parameter (the second condition).
>
>> + ibase = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_STARTV);
>> + inr = readl(base + DFHv1_PARAM_HDR +
>> + off + DFHv1_PARAM_MSIX_NUMV);
>> + dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> + ibase, inr, fid);
>> + }
>> + }
>> + break;
>> +
>> + default:
>> + dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> + break;
>> + }
>> + }
>> +
>> if (!inr) {
>> *irq_base = 0;
>> *nr_irqs = 0;
>
> --
> i.
>
>
On Mon, 26 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>
>
> On Fri, 23 Sep 2022, Ilpo Järvinen wrote:
>
> > On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
> >
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > >
> > > Define and use a DFHv1 parameter to add generic support for MSIX
> > > interrupts for DFL devices.
> > >
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > > v2: fix kernel doc
> > > clarify use of DFH_VERSION field
> > > ---
> > > drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
> > > include/linux/dfl.h | 14 +++++++++++
> > > 2 files changed, 69 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 1132f3c10440..dfd3f563c92d 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > > void __iomem *base = binfo->ioaddr + ofst;
> > > unsigned int i, ibase, inr = 0;
> > > enum dfl_id_type type;
> > > - int virq;
> > > + int virq, off;
> > > u64 v;
> > >
> > > type = feature_dev_id_type(binfo->feature_dev);
> > >
> > > /*
> > > * Ideally DFL framework should only read info from DFL header, but
> > > - * current version DFL only provides mmio resources information for
> > > + * current version, DFHv0, 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
> > > + * DFHv1 supports generic interrupt resource information in DFHv1
> > > + * parameter blocks. But in order to be compatible to old version
> > > * DFL, the driver may still fall back to these quirks.
> > > */
> > > if (type == PORT_ID) {
> > > @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > > }
> > > }
> > >
> > > + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> > > + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> > > +
> > > + v = FIELD_GET(DFH_VERSION, readq(base));
> >
> > I'd call this variable version (or ver) if you want to store it but it
> > would also fit to switch () line so that no extra variable is needed.
>
> I will change the v to dfh_ver to be clearer. I want to store the value
> because it is used in the default case in the error message. The error
> message helps to debug broken FPGA images.
Right, I missed that (or didn't think it too much and all being called
"v" didn't help either :-)).
> > > + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> > > + off = dfl_find_param(base + DFHv1_PARAM_HDR,
> > > ofst,
> > > + DFHv1_PARAM_ID_MSIX);
> > > + if (off >= 0) {
> >
> > I'd reverse these 2 conditions and break when there's nothing to do.
>
> I'm not sure what you mean by reversing these conditions because a DFHv1 may
> or may not have parameters (the first condition), and a DFHv1 may have
> parameters but may not have a MSI-X parameter (the second condition).
This is what I meant:
if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v))
break;
off = dfl_find_param(...);
if (off < 0)
break;
ibase = ...
--
i.
> > > + ibase = readl(base + DFHv1_PARAM_HDR +
> > > + off +
> > > DFHv1_PARAM_MSIX_STARTV);
> > > + inr = readl(base + DFHv1_PARAM_HDR +
> > > + off +
> > > DFHv1_PARAM_MSIX_NUMV);
> > > + dev_dbg(binfo->dev, "start %d num %d
> > > fid 0x%x\n",
> > > + ibase, inr, fid);
> > > + }
> > > + }
> > > + break;
On Tue, 27 Sep 2022, Ilpo Järvinen wrote:
> On Mon, 26 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>
>>
>>
>> On Fri, 23 Sep 2022, Ilpo Järvinen wrote:
>>
>>> On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>>>
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Define and use a DFHv1 parameter to add generic support for MSIX
>>>> interrupts for DFL devices.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>> v2: fix kernel doc
>>>> clarify use of DFH_VERSION field
>>>> ---
>>>> drivers/fpga/dfl.c | 60 +++++++++++++++++++++++++++++++++++++++++----
>>>> include/linux/dfl.h | 14 +++++++++++
>>>> 2 files changed, 69 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>>> index 1132f3c10440..dfd3f563c92d 100644
>>>> --- a/drivers/fpga/dfl.c
>>>> +++ b/drivers/fpga/dfl.c
>>>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct
>>>> build_feature_devs_info *binfo,
>>>> void __iomem *base = binfo->ioaddr + ofst;
>>>> unsigned int i, ibase, inr = 0;
>>>> enum dfl_id_type type;
>>>> - int virq;
>>>> + int virq, off;
>>>> u64 v;
>>>>
>>>> type = feature_dev_id_type(binfo->feature_dev);
>>>>
>>>> /*
>>>> * Ideally DFL framework should only read info from DFL header, but
>>>> - * current version DFL only provides mmio resources information for
>>>> + * current version, DFHv0, 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
>>>> + * DFHv1 supports generic interrupt resource information in DFHv1
>>>> + * parameter blocks. But in order to be compatible to old version
>>>> * DFL, the driver may still fall back to these quirks.
>>>> */
>>>> if (type == PORT_ID) {
>>>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct
>>>> build_feature_devs_info *binfo,
>>>> }
>>>> }
>>>>
>>>> + if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>>>> + fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>>>> +
>>>> + v = FIELD_GET(DFH_VERSION, readq(base));
>>>
>>> I'd call this variable version (or ver) if you want to store it but it
>>> would also fit to switch () line so that no extra variable is needed.
>>
>> I will change the v to dfh_ver to be clearer. I want to store the value
>> because it is used in the default case in the error message. The error
>> message helps to debug broken FPGA images.
>
> Right, I missed that (or didn't think it too much and all being called
> "v" didn't help either :-)).
>
>>>> + if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>>>> + off = dfl_find_param(base + DFHv1_PARAM_HDR,
>>>> ofst,
>>>> + DFHv1_PARAM_ID_MSIX);
>>>> + if (off >= 0) {
>>>
>>> I'd reverse these 2 conditions and break when there's nothing to do.
>>
>> I'm not sure what you mean by reversing these conditions because a DFHv1 may
>> or may not have parameters (the first condition), and a DFHv1 may have
>> parameters but may not have a MSI-X parameter (the second condition).
>
> This is what I meant:
>
> if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v))
> break;
>
> off = dfl_find_param(...);
> if (off < 0)
> break;
>
> ibase = ...
I understand now. This is a good suggestion because the resulting
indentation is better.
Thanks,
Matthew
>
>
> --
> i.
>
>
>>>> + ibase = readl(base + DFHv1_PARAM_HDR +
>>>> + off +
>>>> DFHv1_PARAM_MSIX_STARTV);
>>>> + inr = readl(base + DFHv1_PARAM_HDR +
>>>> + off +
>>>> DFHv1_PARAM_MSIX_NUMV);
>>>> + dev_dbg(binfo->dev, "start %d num %d
>>>> fid 0x%x\n",
>>>> + ibase, inr, fid);
>>>> + }
>>>> + }
>>>> + break;
>
© 2016 - 2026 Red Hat, Inc.