> -----Original Message-----
> From: Xu, Yilun <yilun.xu@xxxxxxxxx>
> Sent: Saturday, April 16, 2022 11:53 PM
> To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>
> Cc: Wu, Hao <hao.wu@xxxxxxxxx>; trix@xxxxxxxxxx; mdf@xxxxxxxxxx; linux-
> fpga@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1] fpga: dfl: fme: adding reserved bits for revision of
> FME/Port error
>
> On Tue, Apr 12, 2022 at 02:35:23AM -0400, Tianfei Zhang wrote:
> > From: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
> >
> > There are 2 different register layouts for FME/Port error registers.
> > This patch introduces 4 reserved bits (Bit[59:56]) to indicate the
> > revision of register layout for userland application.
>
> Please specify that the 4 bits are reserved by HW so that SW appends revision
> info on these bits for the attr readout value.
Yes, correct, I will make the git log message clear. How about this git commit message:
There are 2 different register layouts for FME/Port error registers.
This patch introduces 4 reserved bits (Bit[59:56]) which are reserved by HW, dfl driver appends the
FME/Port error revision info on those bits for attribute on the readout value.
>
> >
> > Signed-off-by: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
> > ---
> > drivers/fpga/dfl-fme-error.c | 36
> > ++++++++++++++++++++++++++++++------
> > 1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-fme-error.c
> > b/drivers/fpga/dfl-fme-error.c index 51c2892ec06d..3b54470f56ca 100644
> > --- a/drivers/fpga/dfl-fme-error.c
> > +++ b/drivers/fpga/dfl-fme-error.c
> > @@ -39,6 +39,22 @@
> >
> > #define ERROR_MASK GENMASK_ULL(63, 0)
> >
> > +/* Bit[59:56] was reserved by software for error revision */ #define
> > +ERROR_SW_REVISION_MASK GENMASK_ULL(59, 56)
>
> This will change the user behavior for the error interfaces. Now they need to
> recognize the revision bits and discard them before clearing the errors, is it?
Yes, the end-user write to those error register without this revision info.
>
> How users know the revision fields in output values? Maybe it still involves
> change in Documentation/ABI/testing/sysfs-platform-dfl-fme,
> which should be reconsidered carefully.
I will add those info in sysfs node documentation.
>
> > +
> > +static u64 set_error_revision(struct device *dev, u64 value) {
> > + void __iomem *base;
> > + u64 dfh;
> > + u64 revision;
>
> Better we follow the reverse xmas tree declaration, so reverse the 2 lines
> please.
Yes, I will fix it on next version.
>
> Thanks,
> Yilun
>
> > +
> > + base = dfl_get_feature_ioaddr_by_id(dev,
> FME_FEATURE_ID_GLOBAL_ERR);
> > + dfh = readq(base);
> > + revision = FIELD_GET(DFH_REVISION, dfh);
> > +
> > + return value | FIELD_PREP(ERROR_SW_REVISION_MASK, revision); }
> > +
> > static ssize_t pcie0_errors_show(struct device *dev,
> > struct device_attribute *attr, char *buf) { @@
> -52,7 +68,8 @@
> > static ssize_t pcie0_errors_show(struct device *dev,
> > value = readq(base + PCIE0_ERROR);
> > mutex_unlock(&pdata->lock);
> >
> > - return sprintf(buf, "0x%llx\n", (unsigned long long)value);
> > + return sprintf(buf, "0x%llx\n",
> > + (unsigned long long)set_error_revision(dev, value));
> > }
> >
> > static ssize_t pcie0_errors_store(struct device *dev, @@ -97,7 +114,8
> > @@ static ssize_t pcie1_errors_show(struct device *dev,
> > value = readq(base + PCIE1_ERROR);
> > mutex_unlock(&pdata->lock);
> >
> > - return sprintf(buf, "0x%llx\n", (unsigned long long)value);
> > + return sprintf(buf, "0x%llx\n",
> > + (unsigned long long)set_error_revision(dev, value));
> > }
> >
> > static ssize_t pcie1_errors_store(struct device *dev, @@ -133,11
> > +151,13 @@ static ssize_t nonfatal_errors_show(struct device *dev,
> > struct device_attribute *attr, char *buf) {
> > void __iomem *base;
> > + u64 value;
> >
> > base = dfl_get_feature_ioaddr_by_id(dev,
> FME_FEATURE_ID_GLOBAL_ERR);
> > + value = readq(base + RAS_NONFAT_ERROR);
> >
> > return sprintf(buf, "0x%llx\n",
> > - (unsigned long long)readq(base + RAS_NONFAT_ERROR));
> > + (unsigned long long)set_error_revision(dev, value));
> > }
> > static DEVICE_ATTR_RO(nonfatal_errors);
> >
> > @@ -145,11 +165,13 @@ static ssize_t catfatal_errors_show(struct device
> *dev,
> > struct device_attribute *attr, char *buf) {
> > void __iomem *base;
> > + u64 value;
> >
> > base = dfl_get_feature_ioaddr_by_id(dev,
> FME_FEATURE_ID_GLOBAL_ERR);
> > + value = readq(base + RAS_CATFAT_ERROR);
> >
> > return sprintf(buf, "0x%llx\n",
> > - (unsigned long long)readq(base + RAS_CATFAT_ERROR));
> > + (unsigned long long)set_error_revision(dev, value));
> > }
> > static DEVICE_ATTR_RO(catfatal_errors);
> >
> > @@ -165,9 +187,10 @@ static ssize_t inject_errors_show(struct device *dev,
> > mutex_lock(&pdata->lock);
> > v = readq(base + RAS_ERROR_INJECT);
> > mutex_unlock(&pdata->lock);
> > + v = FIELD_GET(INJECT_ERROR_MASK, v);
> >
> > return sprintf(buf, "0x%llx\n",
> > - (unsigned long long)FIELD_GET(INJECT_ERROR_MASK, v));
> > + (unsigned long long)set_error_revision(dev, v));
> > }
> >
> > static ssize_t inject_errors_store(struct device *dev, @@ -211,7
> > +234,8 @@ static ssize_t fme_errors_show(struct device *dev,
> > value = readq(base + FME_ERROR);
> > mutex_unlock(&pdata->lock);
> >
> > - return sprintf(buf, "0x%llx\n", (unsigned long long)value);
> > + return sprintf(buf, "0x%llx\n",
> > + (unsigned long long)set_error_revision(dev, value));
> > }
> >
> > static ssize_t fme_errors_store(struct device *dev,
> > --
> > 2.26.2
