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.1
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. > * 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 - 2024 Red Hat, Inc.