> -----Original Message----- > From: Wu, Hao <hao.wu@xxxxxxxxx> > Sent: Thursday, March 17, 2022 9:48 AM > To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>; trix@xxxxxxxxxx; > mdf@xxxxxxxxxx; Xu, Yilun <yilun.xu@xxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx; > linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Cc: corbet@xxxxxxx > Subject: RE: [PATCH v3] fpga: dfl: check feature type before parse irq info > > > -----Original Message----- > > From: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx> > > Sent: Friday, March 11, 2022 10:11 AM > > To: Wu, Hao <hao.wu@xxxxxxxxx>; trix@xxxxxxxxxx; mdf@xxxxxxxxxx; Xu, > > Yilun <yilun.xu@xxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx; > > linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > > Cc: corbet@xxxxxxx; Zhang, Tianfei <tianfei.zhang@xxxxxxxxx> > > Subject: [PATCH v3] fpga: dfl: check feature type before parse irq > > info > > > > The feature ID of "Port User Interrupt" and the "PMCI Subsystem" are > > identical, 0x12, but one is for FME, other is for Port. It should > > check the feature type While parsing the irq info in > > parse_feature_irqs(). > > > > What about this > "Previously the feature IDs defined are unique, no matter which feature type. > But currently we want to extend its usage to have a per-type feature ID space, > so this patch adds feature type checking as well just before look into feature ID > for different features which have irq info." This commit looks good for me. > > > --- > > v3: Remove "Fixes" in commit log with Hao's comment, this is a > > extension not a bug fix. > > > > v2: add DFL Feature ID Registry in documentation. > > > > Signed-off-by: Tianfei Zhang <tianfei.zhang@xxxxxxxxx> > > Move up your signed off line before these changelogs. > > > --- > > Documentation/fpga/dfl.rst | 11 +++++++++++ > > drivers/fpga/dfl.c | 38 ++++++++++++++++++++++---------------- > > 2 files changed, 33 insertions(+), 16 deletions(-) > > > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst > > index ef9eec71f6f3..14f342bb85e4 100644 > > --- a/Documentation/fpga/dfl.rst > > +++ b/Documentation/fpga/dfl.rst > > @@ -502,6 +502,17 @@ Developer only needs to provide a sub feature > > driver with matched feature id. > > FME Partial Reconfiguration Sub Feature driver (see > > drivers/fpga/dfl-fme-pr.c) could be a reference. > > > > +Individual DFL drivers are bound DFL devices based on Feature Type > > +and > > Feature ID. > > +The definition of Feature Type and Feature ID can be found: > > + > > +https://github.com/OPAE/linux-dfl-feature-id/blob/master/dfl-feature- > > +ids.rst > > + > > +If you want to add a new feature ID for FPGA DFL feature device, we > > recommend that > > "recommend" or "must" I think it is "must" if you want to use linux dfl driver. > > > +use a pull request to reserve a feature ID for DFL. Here is the DFL > > +Feature ID > > +Registry: > > + > > +https://github.com/OPAE/linux-dfl-feature-id > > + > > Actually we don't have to put all details in kernel doc, and feature ID is used > inside the device, it could be used in linux or windows or other OS. > What about putting a README file in your repo to describe the detailed process, > not in kernel doc. And we can just put a link to that README here. > Then split this one into another patch? How do you think? I think the process is that submit a patch/pull request to " https://github.com/OPAE/linux-dfl-feature-id"; for review if you have a new FeatureID. > > > Location of DFLs on a PCI Device > > ================================ > > The original method for finding a DFL on a PCI device assumed the > > start of the diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c > > index 599bb21d86af..6bff39ff21a0 100644 > > --- a/drivers/fpga/dfl.c > > +++ b/drivers/fpga/dfl.c > > @@ -940,9 +940,12 @@ 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; > > 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 > > @@ -957,22 +960,25 @@ static int parse_feature_irqs(struct > > build_feature_devs_info *binfo, > > * code will be added. But in order to be compatible to old version > > * DFL, the driver may still fall back to these quirks. > > */ > > - 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; > > - 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; > > + 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) { > > + 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); > > + } > > } > > > > if (!inr) { > > -- > > 2.26.2
