diff mbox series

[v1] fpga: dfl: fme: adding reserved bits for revision of FME/Port error

Message ID 20220412063523.54587-1-tianfei.zhang@intel.com (mailing list archive)
State New
Headers show
Series [v1] fpga: dfl: fme: adding reserved bits for revision of FME/Port error | expand

Commit Message

Zhang, Tianfei April 12, 2022, 6:35 a.m. UTC 
From: Tianfei zhang <tianfei.zhang@intel.com>

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.

Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
---
 drivers/fpga/dfl-fme-error.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

Comments

Xu Yilun April 16, 2022, 3:52 p.m. UTC | #1 
On Tue, Apr 12, 2022 at 02:35:23AM -0400, Tianfei Zhang wrote:
> From: Tianfei zhang <tianfei.zhang@intel.com>
> 
> 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.

> 
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> ---
>  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?

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.

> +
> +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.

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
Zhang, Tianfei April 18, 2022, 1:15 a.m. UTC | #2 
> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Saturday, April 16, 2022 11:53 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>
> Cc: Wu, Hao <hao.wu@intel.com>; trix@redhat.com; mdf@kernel.org; linux-
> fpga@vger.kernel.org
> 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@intel.com>
> >
> > 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@intel.com>
> > ---
> >  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
Wu, Hao April 18, 2022, 1:52 a.m. UTC | #3 
> > -----Original Message-----
> > From: Xu, Yilun <yilun.xu@intel.com>
> > Sent: Saturday, April 16, 2022 11:53 PM
> > To: Zhang, Tianfei <tianfei.zhang@intel.com>
> > Cc: Wu, Hao <hao.wu@intel.com>; trix@redhat.com; mdf@kernel.org; linux-
> > fpga@vger.kernel.org
> > 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@intel.com>
> > >
> > > 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@intel.com>
> > > ---
> > >  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
Zhang, Tianfei April 18, 2022, 2:50 a.m. UTC | #4 
> -----Original Message-----
> From: Wu, Hao <hao.wu@intel.com>
> Sent: Monday, April 18, 2022 9:52 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Xu, Yilun <yilun.xu@intel.com>
> Cc: trix@redhat.com; mdf@kernel.org; linux-fpga@vger.kernel.org
> Subject: RE: [PATCH v1] fpga: dfl: fme: adding reserved bits for revision of
> FME/Port error
> 
> 
> > > -----Original Message-----
> > > From: Xu, Yilun <yilun.xu@intel.com>
> > > Sent: Saturday, April 16, 2022 11:53 PM
> > > To: Zhang, Tianfei <tianfei.zhang@intel.com>
> > > Cc: Wu, Hao <hao.wu@intel.com>; trix@redhat.com; mdf@kernel.org;
> > > linux- fpga@vger.kernel.org
> > > 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@intel.com>
> > > >
> > > > 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@intel.com>
> > > > ---
> > > >  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
diff mbox series

Patch

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)
+
+static u64 set_error_revision(struct device *dev, u64 value)
+{
+	void __iomem *base;
+	u64 dfh;
+	u64 revision;
+
+	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,