[PATCHv2 0/4] Stratix10 EDAC Improvements

[PATCHv2 0/4] Stratix10 EDAC Improvements

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

This patch series includes a fix, an improvement and the
addition of OCRAM EDAC for Stratix10 SOCFPGA.

V2: Ensure the Fixes: line is on one line.

Thor Thayer (4):
  EDAC, altera: Fix S10 persistent register offset
  EDAC, altera: Less Intrusive Error Injection
  EDAC, altera: Add Stratix10 OCRAM ECC support
  arm64: dts: stratix10: Add OCRAM EDAC node

 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi |  7 +++
 drivers/edac/altera_edac.c                        | 52 ++++++++++++++---------
 drivers/edac/altera_edac.h                        |  6 +--
 3 files changed, 43 insertions(+), 22 deletions(-)

-- 
2.7.4

[PATCHv2 1/4] EDAC, altera: Fix S10 persistent register offset

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Correct the persistent register offset where address
and status are stored.

Fixes: 08f08bfb7b4c ("EDAC, altera: Merge Stratix10 into the Arria10 SDRAM probe routine")
Cc: stable@vger.kernel.org
Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2  Fixes tag should be on one line instead of wrapped.
---
 drivers/edac/altera_edac.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index 4213cb0bb2a7..f8664bac9fa8 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -295,8 +295,8 @@ struct altr_sdram_mc_data {
 #define S10_SYSMGR_ECC_INTSTAT_DERR_OFST  0xA0
 
 /* Sticky registers for Uncorrected Errors */
-#define S10_SYSMGR_UE_VAL_OFST            0x120
-#define S10_SYSMGR_UE_ADDR_OFST           0x124
+#define S10_SYSMGR_UE_VAL_OFST            0x220
+#define S10_SYSMGR_UE_ADDR_OFST           0x224
 
 #define S10_DDR0_IRQ_MASK                 BIT(16)
 
-- 
2.7.4

[PATCHv2 2/4] EDAC, altera: Less Intrusive Error Injection

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Improve the Arria10 and Stratix10 error injection routines
by reading the data and changing just 1 bit before writing
back out. Previous routine would set the first bytes to 0
then change to 1 but this method is less intrusive.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2  No Changes
---
 drivers/edac/altera_edac.c | 30 +++++++++++++-----------------
 drivers/edac/altera_edac.h |  2 +-
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index c89d82aa2776..56af98d18645 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1756,28 +1756,24 @@ static ssize_t altr_edac_a10_device_trig2(struct file *file,
 	if (trig_type == ALTR_UE_TRIGGER_CHAR) {
 		writel(priv->ue_set_mask, set_addr);
 	} else {
-		/* Setup write of 0 to first 4 bytes */
-		writel(0x0, drvdata->base + ECC_BLK_WDATA0_OFST);
-		writel(0x0, drvdata->base + ECC_BLK_WDATA1_OFST);
-		writel(0x0, drvdata->base + ECC_BLK_WDATA2_OFST);
-		writel(0x0, drvdata->base + ECC_BLK_WDATA3_OFST);
-		/* Setup write of 4 bytes */
+		/* Setup read/write of 4 bytes */
 		writel(ECC_WORD_WRITE, drvdata->base + ECC_BLK_DBYTECTRL_OFST);
 		/* Setup Address to 0 */
-		writel(0x0, drvdata->base + ECC_BLK_ADDRESS_OFST);
-		/* Setup accctrl to write & data override */
-		writel(ECC_WRITE_DOVR, drvdata->base + ECC_BLK_ACCCTRL_OFST);
-		/* Kick it. */
-		writel(ECC_XACT_KICK, drvdata->base + ECC_BLK_STARTACC_OFST);
-		/* Setup accctrl to read & ecc override */
-		writel(ECC_READ_EOVR, drvdata->base + ECC_BLK_ACCCTRL_OFST);
+		writel(0, drvdata->base + ECC_BLK_ADDRESS_OFST);
+		/* Setup accctrl to read & ecc & data override */
+		writel(ECC_READ_EDOVR, drvdata->base + ECC_BLK_ACCCTRL_OFST);
 		/* Kick it. */
 		writel(ECC_XACT_KICK, drvdata->base + ECC_BLK_STARTACC_OFST);
 		/* Setup write for single bit change */
-		writel(0x1, drvdata->base + ECC_BLK_WDATA0_OFST);
-		writel(0x0, drvdata->base + ECC_BLK_WDATA1_OFST);
-		writel(0x0, drvdata->base + ECC_BLK_WDATA2_OFST);
-		writel(0x0, drvdata->base + ECC_BLK_WDATA3_OFST);
+		writel(readl(drvdata->base + ECC_BLK_RDATA0_OFST) ^ 0x1,
+		       drvdata->base + ECC_BLK_WDATA0_OFST);
+		writel(readl(drvdata->base + ECC_BLK_RDATA1_OFST),
+		       drvdata->base + ECC_BLK_WDATA1_OFST);
+		writel(readl(drvdata->base + ECC_BLK_RDATA2_OFST),
+		       drvdata->base + ECC_BLK_WDATA2_OFST);
+		writel(readl(drvdata->base + ECC_BLK_RDATA3_OFST),
+		       drvdata->base + ECC_BLK_WDATA3_OFST);
+
 		/* Copy Read ECC to Write ECC */
 		writel(readl(drvdata->base + ECC_BLK_RECC0_OFST),
 		       drvdata->base + ECC_BLK_WECC0_OFST);
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index f8664bac9fa8..a1acca99c50b 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -319,7 +319,7 @@ struct altr_sdram_mc_data {
 #define ECC_BLK_STARTACC_OFST             0x7C
 
 #define ECC_XACT_KICK                     0x10000
-#define ECC_WORD_WRITE                    0xF
+#define ECC_WORD_WRITE                    0xFF
 #define ECC_WRITE_DOVR                    0x101
 #define ECC_WRITE_EDOVR                   0x103
 #define ECC_READ_EOVR                     0x2
-- 
2.7.4

[PATCHv2 3/4] EDAC, altera: Add Stratix10 OCRAM ECC support

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Use the newer ECC error injection method for Arria10 and
Stratix10 OCRAM.
OCRAM can't call the common peripheral init routine since
OCRAM is being used. An OCRAM specific init routine ensures
the OCRAM ECC is setup without clearing memory.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2  No changes
---
 drivers/edac/altera_edac.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 56af98d18645..6a8993e6a278 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1216,8 +1216,26 @@ static const struct edac_device_prv_data ocramecc_data = {
 	.inject_fops = &altr_edac_device_inject_fops,
 };
 
+static int __maybe_unused
+altr_a10_check_ecc_deps_init(struct altr_edac_device_dev *device)
+{
+	void __iomem  *base = device->base;
+	int ret;
+
+	ret = altr_check_ecc_deps(device);
+	if (ret)
+		return ret;
+
+	/* Enable IRQ on Single Bit Error */
+	writel(ALTR_A10_ECC_SERRINTEN, (base + ALTR_A10_ECC_ERRINTENS_OFST));
+	/* Ensure all writes complete */
+	wmb();
+
+	return 0;
+}
+
 static const struct edac_device_prv_data a10_ocramecc_data = {
-	.setup = altr_check_ecc_deps,
+	.setup = altr_a10_check_ecc_deps_init,
 	.ce_clear_mask = ALTR_A10_ECC_SERRPENA,
 	.ue_clear_mask = ALTR_A10_ECC_DERRPENA,
 	.irq_status_mask = A10_SYSMGR_ECC_INTSTAT_OCRAM,
@@ -1227,7 +1245,7 @@ static const struct edac_device_prv_data a10_ocramecc_data = {
 	.ue_set_mask = ALTR_A10_ECC_TDERRA,
 	.set_err_ofst = ALTR_A10_ECC_INTTEST_OFST,
 	.ecc_irq_handler = altr_edac_a10_ecc_irq,
-	.inject_fops = &altr_edac_a10_device_inject_fops,
+	.inject_fops = &altr_edac_a10_device_inject2_fops,
 	/*
 	 * OCRAM panic on uncorrectable error because sleep/resume
 	 * functions and FPGA contents are stored in OCRAM. Prefer
-- 
2.7.4

[PATCHv2 4/4] arm64: dts: stratix10: Add OCRAM EDAC node

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Add the OCRAM ECC node following the Arria10 format.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2  No changes
---
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index 8253a1a9e985..a625dc472b91 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -494,6 +494,13 @@
 				interrupts = <16 4>, <48 4>;
 			};
 
+			ocram-ecc@ff8cc000 {
+				compatible = "altr,socfpga-a10-ocram-ecc";
+				reg = <0xff8cc000 0x100>;
+				interrupts = <1 4>,
+					     <33 4>;
+			};
+
 			usb0-ecc@ff8c4000 {
 				compatible = "altr,socfpga-usb-ecc";
 				reg = <0xff8c4000 0x100>;
-- 
2.7.4

Re: [PATCHv2 4/4] arm64: dts: stratix10: Add OCRAM EDAC node

[Dinh Nguyen]

On 1/22/19 11:48 AM, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> Add the OCRAM ECC node following the Arria10 format.
> 
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
> v2  No changes
> ---
>  arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index 8253a1a9e985..a625dc472b91 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> @@ -494,6 +494,13 @@
>  				interrupts = <16 4>, <48 4>;
>  			};
>  
> +			ocram-ecc@ff8cc000 {
> +				compatible = "altr,socfpga-a10-ocram-ecc";

Are you absolutely sure there are no differences in the Stratix10 versus
A10? I wonder if it would be safer to have a platform specific binding
for Stratix10 rather than re-using A10. It would prevent from having to
change bindings later.

Dinh

Re: [PATCHv2 4/4] arm64: dts: stratix10: Add OCRAM EDAC node

[Thor Thayer]

On 1/23/19 10:56 AM, Dinh Nguyen wrote:
> 
> 
> On 1/22/19 11:48 AM, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> Add the OCRAM ECC node following the Arria10 format.
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>> v2  No changes
>> ---
>>   arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>> index 8253a1a9e985..a625dc472b91 100644
>> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
>> @@ -494,6 +494,13 @@
>>   				interrupts = <16 4>, <48 4>;
>>   			};
>>   
>> +			ocram-ecc@ff8cc000 {
>> +				compatible = "altr,socfpga-a10-ocram-ecc";
> 
> Are you absolutely sure there are no differences in the Stratix10 versus
> A10? I wonder if it would be safer to have a platform specific binding
> for Stratix10 rather than re-using A10. It would prevent from having to
> change bindings later.
> 
> Dinh
> 

They are the same functionally. However, you bring up a good point. 
There are differences related to the underlying architecture - 32bit vs 
64bit.

I will respin this series (except for the first fixup patch) with new 
bindings for S10.

Thanks,

Thor

Re: [PATCHv2 0/4] Stratix10 EDAC Improvements

[Borislav Petkov]

On Tue, Jan 22, 2019 at 11:48:03AM -0600, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> This patch series includes a fix, an improvement and the
> addition of OCRAM EDAC for Stratix10 SOCFPGA.
> 
> V2: Ensure the Fixes: line is on one line.
> 
> Thor Thayer (4):
>   EDAC, altera: Fix S10 persistent register offset
>   EDAC, altera: Less Intrusive Error Injection
>   EDAC, altera: Add Stratix10 OCRAM ECC support
>   arm64: dts: stratix10: Add OCRAM EDAC node
> 
>  arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi |  7 +++
>  drivers/edac/altera_edac.c                        | 52 ++++++++++++++---------
>  drivers/edac/altera_edac.h                        |  6 +--
>  3 files changed, 43 insertions(+), 22 deletions(-)

Looks ok, I'll take them when I get an ACK for the DT change in patch 4.

Patch 1 I'll send to Linus now as the broken commit is in 4.20 too.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.