> -----Original Message-----
> From: Wu, Hao <hao.wu@xxxxxxxxx>
> Sent: Monday, April 18, 2022 9:52 AM
> To: Zhang, Tianfei <tianfei.zhang@xxxxxxxxx>; Xu, Yilun <yilun.xu@xxxxxxxxx>
> Cc: trix@xxxxxxxxxx; mdf@xxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v1] fpga: dfl: fme: adding reserved bits for revision of
> FME/Port error
>
>
> > > -----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.
The background is that it is not a good method that it use one sysfs node' value to determine the usage of other sysfs node.
The sysfs node should be has a persistent value or usage.
In our previous patch, we provide a "revision" sysfs node for user application to get the register layout information while paring the FME/Port error registers.
>
> > >
> > > 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?
The old production like PAC N3000 was used the old format, and the new production like PAC N6000 was used the new register format.
The new register format will be more reasonable.
The revision in DFH register was 4 bits width, so we reserved 4 bits is 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
