> > -----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 provide more detailed background information to explain why we need
this.
> >
> > 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.
We need more clear information, why we have 2 different register layouts, and
why 2 different register layouts requires 4 bits in HW, or even 4 bits will that
be enough?
>
> >
> > >
> > > 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
