[PATCHv3 0/7] Add Ethernet EDAC & peripheral init functions

[PATCHv3 0/7] Add Ethernet EDAC & peripheral init functions

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

This patch set adds the Ethernet EDAC and memory initialization functions
for Altera's Arria10 peripherals. The ECC memory init functions are common
to all the peripheral memory buffers.

Thor Thayer (7):
  EDAC, altera: Check parent status for Arria10 EDAC block
  EDAC, altera: Add panic flag check to A10 IRQ
  EDAC, altera: Share Arria10 check_deps & IRQ functions
  Documentation: dt: socfpga: Add Arria10 Ethernet binding
  EDAC, altera: Add Arria10 ECC memory init functions
  EDAC, altera: Add Arria10 Ethernet EDAC support
  ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

 .../bindings/arm/altera/socfpga-eccmgr.txt         |   24 ++
 arch/arm/boot/dts/socfpga_arria10.dtsi             |   16 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/altera_edac.c                         |  314 ++++++++++++++++++--
 drivers/edac/altera_edac.h                         |   12 +
 5 files changed, 344 insertions(+), 29 deletions(-)

-- 
1.7.9.5

[PATCHv3 1/7] EDAC, altera: Check parent status for Arria10 EDAC block

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

In preparation for the Arria10 ECC modules, check the status
of the parent in the device tree to ensure the block is enabled.
Skip if no parent phandle is set in the device tree.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2  No change
v3  Move check into validate_parent_available().
---
 drivers/edac/altera_edac.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 6f5d586..926bcaf 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1125,6 +1125,20 @@ static void altr_edac_a10_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int validate_parent_available(struct device_node *np)
+{
+	struct device_node *parent;
+
+	/* Ensure parent device is enabled if parent node exists */
+	parent = of_parse_phandle(np, "parent", 0);
+	if (parent && !of_device_is_available(parent)) {
+		of_node_put(parent);
+		return -ENODEV;
+	}
+	of_node_put(parent);
+	return 0;
+}
+
 static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
 				    struct device_node *np)
 {
@@ -1146,6 +1160,9 @@ static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
 	if (IS_ERR_OR_NULL(prv))
 		return -ENODEV;
 
+	if (validate_parent_available(np))
+		return -ENODEV;
+
 	if (!devres_open_group(edac->dev, altr_edac_a10_device_add, GFP_KERNEL))
 		return -ENOMEM;
 
-- 
1.7.9.5

[PATCHv3 1/7] EDAC, altera: Check parent status for Arria10 EDAC block

[Borislav Petkov]

On Mon, Jun 13, 2016 at 04:19:06PM -0500, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> In preparation for the Arria10 ECC modules, check the status
> of the parent in the device tree to ensure the block is enabled.
> Skip if no parent phandle is set in the device tree.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2  No change
> v3  Move check into validate_parent_available().
> ---
>  drivers/edac/altera_edac.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 6f5d586..926bcaf 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -1125,6 +1125,20 @@ static void altr_edac_a10_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> +static int validate_parent_available(struct device_node *np)
> +{
> +	struct device_node *parent;
> +
> +	/* Ensure parent device is enabled if parent node exists */
> +	parent = of_parse_phandle(np, "parent", 0);
> +	if (parent && !of_device_is_available(parent)) {
> +		of_node_put(parent);
> +		return -ENODEV;
> +	}
> +	of_node_put(parent);
> +	return 0;
> +}

How about this - it is a bit simpler:

static int validate_parent_available(struct device_node *np)
{
        struct device_node *parent;
        int ret = 0;

        /* Ensure parent device is enabled if parent node exists */
        parent = of_parse_phandle(np, "parent", 0);
        if (parent && !of_device_is_available(parent))
                ret = -ENODEV;

        of_node_put(parent);
        return ret;
}

?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 1/7] EDAC, altera: Check parent status for Arria10 EDAC block

[Thor Thayer]

On 06/17/2016 11:46 AM, Borislav Petkov wrote:
> On Mon, Jun 13, 2016 at 04:19:06PM -0500, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> In preparation for the Arria10 ECC modules, check the status
>> of the parent in the device tree to ensure the block is enabled.
>> Skip if no parent phandle is set in the device tree.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2  No change
>> v3  Move check into validate_parent_available().
>> ---
>>   drivers/edac/altera_edac.c |   17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index 6f5d586..926bcaf 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -1125,6 +1125,20 @@ static void altr_edac_a10_irq_handler(struct irq_desc *desc)
>>   	chained_irq_exit(chip, desc);
>>   }
>>
>> +static int validate_parent_available(struct device_node *np)
>> +{
>> +	struct device_node *parent;
>> +
>> +	/* Ensure parent device is enabled if parent node exists */
>> +	parent = of_parse_phandle(np, "parent", 0);
>> +	if (parent && !of_device_is_available(parent)) {
>> +		of_node_put(parent);
>> +		return -ENODEV;
>> +	}
>> +	of_node_put(parent);
>> +	return 0;
>> +}
>
> How about this - it is a bit simpler:
>
> static int validate_parent_available(struct device_node *np)
> {
>          struct device_node *parent;
>          int ret = 0;
>
>          /* Ensure parent device is enabled if parent node exists */
>          parent = of_parse_phandle(np, "parent", 0);
>          if (parent && !of_device_is_available(parent))
>                  ret = -ENODEV;
>
>          of_node_put(parent);
>          return ret;
> }
>
> ?
>
Yes. Thanks! I will make the change.

[PATCHv3 1/7] EDAC, altera: Check parent status for Arria10 EDAC block

[Borislav Petkov]

On Fri, Jun 17, 2016 at 11:54:04AM -0500, Thor Thayer wrote:
> Yes. Thanks! I will make the change.

No need, already did and applied. You can drop this patch from your
queue.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 2/7] EDAC, altera: Add panic flag check to A10 IRQ

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

In preparation for additional memory module ECCs, the
IRQ function will check a panic flag before doing a
kernel panic on double bit errors. ECCs on buffers
will not cause a kernel panic on DBERRs.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2  New patch. Add panic flag to IRQ function.
v3  No change
---
 drivers/edac/altera_edac.c |    4 +++-
 drivers/edac/altera_edac.h |    1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 926bcaf..a9d8fa7 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -897,7 +897,8 @@ static irqreturn_t altr_edac_a10_ecc_irq(int irq, void *dev_id)
 		writel(ALTR_A10_ECC_DERRPENA,
 		       base + ALTR_A10_ECC_INTSTAT_OFST);
 		edac_device_handle_ue(dci->edac_dev, 0, 0, dci->edac_dev_name);
-		panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
+		if (dci->data->panic)
+			panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
 
 		return IRQ_HANDLED;
 	}
@@ -936,6 +937,7 @@ const struct edac_device_prv_data a10_ocramecc_data = {
 	.set_err_ofst = ALTR_A10_ECC_INTTEST_OFST,
 	.ecc_irq_handler = altr_edac_a10_ecc_irq,
 	.inject_fops = &altr_edac_a10_device_inject_fops,
+	.panic = true,
 };
 
 #endif	/* CONFIG_EDAC_ALTERA_OCRAM */
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index 62b0fa0..cf4e8cb 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -298,6 +298,7 @@ struct edac_device_prv_data {
 	irqreturn_t (*ecc_irq_handler)(int irq, void *dev_id);
 	int trig_alloc_sz;
 	const struct file_operations *inject_fops;
+	bool panic;
 };
 
 struct altr_edac_device_dev {
-- 
1.7.9.5

[PATCHv3 2/7] EDAC, altera: Add panic flag check to A10 IRQ

[Borislav Petkov]

On Mon, Jun 13, 2016 at 04:19:07PM -0500, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> In preparation for additional memory module ECCs, the
> IRQ function will check a panic flag before doing a
> kernel panic on double bit errors. ECCs on buffers
> will not cause a kernel panic on DBERRs.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2  New patch. Add panic flag to IRQ function.
> v3  No change
> ---
>  drivers/edac/altera_edac.c |    4 +++-
>  drivers/edac/altera_edac.h |    1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 926bcaf..a9d8fa7 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -897,7 +897,8 @@ static irqreturn_t altr_edac_a10_ecc_irq(int irq, void *dev_id)
>  		writel(ALTR_A10_ECC_DERRPENA,
>  		       base + ALTR_A10_ECC_INTSTAT_OFST);
>  		edac_device_handle_ue(dci->edac_dev, 0, 0, dci->edac_dev_name);
> -		panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
> +		if (dci->data->panic)
> +			panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
>  
>  		return IRQ_HANDLED;
>  	}
> @@ -936,6 +937,7 @@ const struct edac_device_prv_data a10_ocramecc_data = {
>  	.set_err_ofst = ALTR_A10_ECC_INTTEST_OFST,
>  	.ecc_irq_handler = altr_edac_a10_ecc_irq,
>  	.inject_fops = &altr_edac_a10_device_inject_fops,
> +	.panic = true,

So I could use a bit more detailed explanation here why OCRAM must panic
and the others don't. Consider me an external guy who doesn't know the
hardware and is looking at the driver and is wondering why this IP must
panic on double-bit errors and the others don't.

:-)

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 2/7] EDAC, altera: Add panic flag check to A10 IRQ

[Thor Thayer]

Hi Boris,

On 06/17/2016 11:51 AM, Borislav Petkov wrote:
> On Mon, Jun 13, 2016 at 04:19:07PM -0500, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> In preparation for additional memory module ECCs, the
>> IRQ function will check a panic flag before doing a
>> kernel panic on double bit errors. ECCs on buffers
>> will not cause a kernel panic on DBERRs.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2  New patch. Add panic flag to IRQ function.
>> v3  No change
>> ---
>>   drivers/edac/altera_edac.c |    4 +++-
>>   drivers/edac/altera_edac.h |    1 +
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index 926bcaf..a9d8fa7 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -897,7 +897,8 @@ static irqreturn_t altr_edac_a10_ecc_irq(int irq, void *dev_id)
>>   		writel(ALTR_A10_ECC_DERRPENA,
>>   		       base + ALTR_A10_ECC_INTSTAT_OFST);
>>   		edac_device_handle_ue(dci->edac_dev, 0, 0, dci->edac_dev_name);
>> -		panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
>> +		if (dci->data->panic)
>> +			panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
>>
>>   		return IRQ_HANDLED;
>>   	}
>> @@ -936,6 +937,7 @@ const struct edac_device_prv_data a10_ocramecc_data = {
>>   	.set_err_ofst = ALTR_A10_ECC_INTTEST_OFST,
>>   	.ecc_irq_handler = altr_edac_a10_ecc_irq,
>>   	.inject_fops = &altr_edac_a10_device_inject_fops,
>> +	.panic = true,
>
> So I could use a bit more detailed explanation here why OCRAM must panic
> and the others don't. Consider me an external guy who doesn't know the
> hardware and is looking at the driver and is wondering why this IP must
> panic on double-bit errors and the others don't.
>
> :-)
>
> Thanks.
>
That is a good question. We have 2 important uses for OCRAM 1) to hold 
our power-down/sleep and resume functions and 2) to hold our FPGA 
contents during sleep. If either of these is corrupted, it is better to 
panic than to load something that would cause incorrect.

In the cases of the FIFOs such as Ethernet and USB, the plan is to add 
code to drop the packet so that we'll get a re-transmission. In that 
case, it is sort of recoverable.

Thor

[PATCHv3 2/7] EDAC, altera: Add panic flag check to A10 IRQ

[Borislav Petkov]

On Fri, Jun 17, 2016 at 12:05:41PM -0500, Thor Thayer wrote:
> That is a good question. We have 2 important uses for OCRAM 1) to hold our
> power-down/sleep and resume functions and 2) to hold our FPGA contents
> during sleep. If either of these is corrupted, it is better to panic than to
> load something that would cause incorrect.
> 
> In the cases of the FIFOs such as Ethernet and USB, the plan is to add code
> to drop the packet so that we'll get a re-transmission. In that case, it is
> sort of recoverable.

Much better. Now put that explanation in the code please!

:-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 2/7] EDAC, altera: Add panic flag check to A10 IRQ

[Thor Thayer]

On 06/17/2016 12:02 PM, Borislav Petkov wrote:
> On Fri, Jun 17, 2016 at 12:05:41PM -0500, Thor Thayer wrote:
>> That is a good question. We have 2 important uses for OCRAM 1) to hold our
>> power-down/sleep and resume functions and 2) to hold our FPGA contents
>> during sleep. If either of these is corrupted, it is better to panic than to
>> load something that would cause incorrect.
>>
>> In the cases of the FIFOs such as Ethernet and USB, the plan is to add code
>> to drop the packet so that we'll get a re-transmission. In that case, it is
>> sort of recoverable.
>
> Much better. Now put that explanation in the code please!
>
> :-)
>
I'll add that in the next revision. Thanks for reviewing.

[PATCHv3 3/7] EDAC, altera: Share Arria10 check_deps & IRQ functions

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

In preparation for additional memory module ECCs, the IRQ and
check_deps() functions are being made available to all the memory
buffers. Move them outside of the OCRAM only area.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2  New patch. Move shared functions outside OCRAM only area.
v3  Change title line - check_deps & IRQ.
---
 drivers/edac/altera_edac.c |   62 ++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index a9d8fa7..a3f490d 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -825,9 +825,9 @@ static struct platform_driver altr_edac_device_driver = {
 };
 module_platform_driver(altr_edac_device_driver);
 
-/*********************** OCRAM EDAC Device Functions *********************/
+/******************* Arria10 Device ECC Shared Functions *****************/
 
-#ifdef CONFIG_EDAC_ALTERA_OCRAM
+#if defined(CONFIG_EDAC_ALTERA_OCRAM) || defined(CONFIG_EDAC_ALTERA_ETHERNET)
 /*
  *  Test for memory's ECC dependencies upon entry because platform specific
  *  startup should have initialized the memory and enabled the ECC.
@@ -848,6 +848,38 @@ static int altr_check_ecc_deps(struct altr_edac_device_dev *device)
 	return -ENODEV;
 }
 
+static irqreturn_t altr_edac_a10_ecc_irq(int irq, void *dev_id)
+{
+	struct altr_edac_device_dev *dci = dev_id;
+	void __iomem  *base = dci->base;
+
+	if (irq == dci->sb_irq) {
+		writel(ALTR_A10_ECC_SERRPENA,
+		       base + ALTR_A10_ECC_INTSTAT_OFST);
+		edac_device_handle_ce(dci->edac_dev, 0, 0, dci->edac_dev_name);
+
+		return IRQ_HANDLED;
+	} else if (irq == dci->db_irq) {
+		writel(ALTR_A10_ECC_DERRPENA,
+		       base + ALTR_A10_ECC_INTSTAT_OFST);
+		edac_device_handle_ue(dci->edac_dev, 0, 0, dci->edac_dev_name);
+		if (dci->data->panic)
+			panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
+
+		return IRQ_HANDLED;
+	}
+
+	WARN_ON(1);
+
+	return IRQ_NONE;
+}
+
+#endif /* CONFIG_EDAC_ALTERA_OCRAM || CONFIG_EDAC_ALTERA_ETHERNET */
+
+/*********************** OCRAM EDAC Device Functions *********************/
+
+#ifdef CONFIG_EDAC_ALTERA_OCRAM
+
 static void *ocram_alloc_mem(size_t size, void **other)
 {
 	struct device_node *np;
@@ -882,32 +914,6 @@ static void ocram_free_mem(void *p, size_t size, void *other)
 	gen_pool_free((struct gen_pool *)other, (u32)p, size);
 }
 
-static irqreturn_t altr_edac_a10_ecc_irq(int irq, void *dev_id)
-{
-	struct altr_edac_device_dev *dci = dev_id;
-	void __iomem  *base = dci->base;
-
-	if (irq == dci->sb_irq) {
-		writel(ALTR_A10_ECC_SERRPENA,
-		       base + ALTR_A10_ECC_INTSTAT_OFST);
-		edac_device_handle_ce(dci->edac_dev, 0, 0, dci->edac_dev_name);
-
-		return IRQ_HANDLED;
-	} else if (irq == dci->db_irq) {
-		writel(ALTR_A10_ECC_DERRPENA,
-		       base + ALTR_A10_ECC_INTSTAT_OFST);
-		edac_device_handle_ue(dci->edac_dev, 0, 0, dci->edac_dev_name);
-		if (dci->data->panic)
-			panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
-
-		return IRQ_HANDLED;
-	}
-
-	WARN_ON(1);
-
-	return IRQ_NONE;
-}
-
 const struct edac_device_prv_data ocramecc_data = {
 	.setup = altr_check_ecc_deps,
 	.ce_clear_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_SERR),
-- 
1.7.9.5

[PATCHv3 3/7] EDAC, altera: Share Arria10 check_deps & IRQ functions

[Borislav Petkov]

On Mon, Jun 13, 2016 at 04:19:08PM -0500, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> In preparation for additional memory module ECCs, the IRQ and
> check_deps() functions are being made available to all the memory
> buffers. Move them outside of the OCRAM only area.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2  New patch. Move shared functions outside OCRAM only area.
> v3  Change title line - check_deps & IRQ.
> ---
>  drivers/edac/altera_edac.c |   62 ++++++++++++++++++++++++--------------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index a9d8fa7..a3f490d 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -825,9 +825,9 @@ static struct platform_driver altr_edac_device_driver = {
>  };
>  module_platform_driver(altr_edac_device_driver);
>  
> -/*********************** OCRAM EDAC Device Functions *********************/
> +/******************* Arria10 Device ECC Shared Functions *****************/
>  
> -#ifdef CONFIG_EDAC_ALTERA_OCRAM
> +#if defined(CONFIG_EDAC_ALTERA_OCRAM) || defined(CONFIG_EDAC_ALTERA_ETHERNET)

Do you need the ifdeffery here at all?

IOW, can we leave this function compiled-in unconditionally?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 3/7] EDAC, altera: Share Arria10 check_deps & IRQ functions

[Thor Thayer]

Hi Boris,

On 06/17/2016 12:00 PM, Borislav Petkov wrote:
> On Mon, Jun 13, 2016 at 04:19:08PM -0500, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> In preparation for additional memory module ECCs, the IRQ and
>> check_deps() functions are being made available to all the memory
>> buffers. Move them outside of the OCRAM only area.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2  New patch. Move shared functions outside OCRAM only area.
>> v3  Change title line - check_deps & IRQ.
>> ---
>>   drivers/edac/altera_edac.c |   62 ++++++++++++++++++++++++--------------------
>>   1 file changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index a9d8fa7..a3f490d 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -825,9 +825,9 @@ static struct platform_driver altr_edac_device_driver = {
>>   };
>>   module_platform_driver(altr_edac_device_driver);
>>
>> -/*********************** OCRAM EDAC Device Functions *********************/
>> +/******************* Arria10 Device ECC Shared Functions *****************/
>>
>> -#ifdef CONFIG_EDAC_ALTERA_OCRAM
>> +#if defined(CONFIG_EDAC_ALTERA_OCRAM) || defined(CONFIG_EDAC_ALTERA_ETHERNET)
>
> Do you need the ifdeffery here at all?
>
> IOW, can we leave this function compiled-in unconditionally?
>
Since each peripheral's EDAC can be individually selected, the build 
generates a warning of an unused function if just L2 cache was selected.

The ifdeffery is ugly but it removes that warning in the L2 only case.

Thanks for reviewing!

Thor

[PATCHv3 3/7] EDAC, altera: Share Arria10 check_deps & IRQ functions

[Borislav Petkov]

On Fri, Jun 17, 2016 at 12:09:59PM -0500, Thor Thayer wrote:
> Since each peripheral's EDAC can be individually selected, the build
> generates a warning of an unused function if just L2 cache was selected.
> 
> The ifdeffery is ugly but it removes that warning in the L2 only case.

You could add __maybe_unused to the function definition but I guess you
don't want that code in there if only L2 is selected.

Ok.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 3/7] EDAC, altera: Share Arria10 check_deps & IRQ functions

[Thor Thayer]

Hi Boris,

On 06/17/2016 12:11 PM, Borislav Petkov wrote:
> On Fri, Jun 17, 2016 at 12:09:59PM -0500, Thor Thayer wrote:
>> Since each peripheral's EDAC can be individually selected, the build
>> generates a warning of an unused function if just L2 cache was selected.
>>
>> The ifdeffery is ugly but it removes that warning in the L2 only case.
>
> You could add __maybe_unused to the function definition but I guess you
> don't want that code in there if only L2 is selected.
>
> Ok.
>
I like that because otherwise I'll need to add the other peripheral 
defines in the future so this will be a multi-line #ifdef

I wasn't aware of that macro but it is much cleaner. I'll use that. Thanks!

[PATCHv3 4/7] Documentation: dt: socfpga: Add Arria10 Ethernet binding

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

Add the device tree bindings needed to support the Altera Ethernet
FIFO buffers on the Arria10 chip.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2  No Change
v3  Change to common compatible string based on maintainer comments
    Add local IRQ values.
---
 .../bindings/arm/altera/socfpga-eccmgr.txt         |   24 ++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
index 15eb0df..e9febbb 100644
--- a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
@@ -82,6 +82,14 @@ Required Properties:
 - interrupts : Should be single bit error interrupt, then double bit error
 	interrupt, in this order.
 
+Ethernet FIFO ECC
+Required Properties:
+- compatible : Should be "altr,socfpga-eth-mac-ecc"
+- reg        : Address and size for ECC block registers.
+- parent     : phandle to parent Ethernet node.
+- interrupts : Should be single bit error interrupt, then double bit error
+	interrupt, in this order.
+
 Example:
 
 	eccmgr: eccmgr at ffd06000 {
@@ -108,4 +116,20 @@ Example:
 			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>,
 				     <33 IRQ_TYPE_LEVEL_HIGH> ;
 		};
+
+		emac0-rx-ecc at ff8c0800 {
+			compatible = "altr,socfpga-eth-mac-ecc";
+			reg = <0xff8c0800 0x400>;
+			parent = <&gmac0>;
+			interrupts = <4 IRQ_TYPE_LEVEL_HIGH>,
+				     <36 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		emac0-tx-ecc at ff8c0c00 {
+			compatible = "altr,socfpga-eth-mac-ecc";
+			reg = <0xff8c0c00 0x400>;
+			parent = <&gmac0>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>,
+				     <37 IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
-- 
1.7.9.5

[PATCHv3 4/7] Documentation: dt: socfpga: Add Arria10 Ethernet binding

[Rob Herring]

On Mon, Jun 13, 2016 at 04:19:09PM -0500, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Add the device tree bindings needed to support the Altera Ethernet
> FIFO buffers on the Arria10 chip.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2  No Change
> v3  Change to common compatible string based on maintainer comments
>     Add local IRQ values.
> ---
>  .../bindings/arm/altera/socfpga-eccmgr.txt         |   24 ++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
> index 15eb0df..e9febbb 100644
> --- a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
> @@ -82,6 +82,14 @@ Required Properties:
>  - interrupts : Should be single bit error interrupt, then double bit error
>  	interrupt, in this order.
>  
> +Ethernet FIFO ECC
> +Required Properties:
> +- compatible : Should be "altr,socfpga-eth-mac-ecc"
> +- reg        : Address and size for ECC block registers.
> +- parent     : phandle to parent Ethernet node.

parent is too vague. How about altr,ethernet-mac.

> +- interrupts : Should be single bit error interrupt, then double bit error
> +	interrupt, in this order.
> +
>  Example:
>  
>  	eccmgr: eccmgr at ffd06000 {
> @@ -108,4 +116,20 @@ Example:
>  			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>,
>  				     <33 IRQ_TYPE_LEVEL_HIGH> ;
>  		};
> +
> +		emac0-rx-ecc at ff8c0800 {
> +			compatible = "altr,socfpga-eth-mac-ecc";
> +			reg = <0xff8c0800 0x400>;
> +			parent = <&gmac0>;
> +			interrupts = <4 IRQ_TYPE_LEVEL_HIGH>,
> +				     <36 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		emac0-tx-ecc at ff8c0c00 {
> +			compatible = "altr,socfpga-eth-mac-ecc";
> +			reg = <0xff8c0c00 0x400>;
> +			parent = <&gmac0>;
> +			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>,
> +				     <37 IRQ_TYPE_LEVEL_HIGH>;
> +		};
>  	};
> -- 
> 1.7.9.5
> 

[PATCHv3 4/7] Documentation: dt: socfpga: Add Arria10 Ethernet binding

[Thor Thayer]

Hi Rob,

On 06/16/2016 01:39 PM, Rob Herring wrote:
> On Mon, Jun 13, 2016 at 04:19:09PM -0500, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add the device tree bindings needed to support the Altera Ethernet
>> FIFO buffers on the Arria10 chip.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2  No Change
>> v3  Change to common compatible string based on maintainer comments
>>      Add local IRQ values.
>> ---
>>   .../bindings/arm/altera/socfpga-eccmgr.txt         |   24 ++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>> index 15eb0df..e9febbb 100644
>> --- a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
>> @@ -82,6 +82,14 @@ Required Properties:
>>   - interrupts : Should be single bit error interrupt, then double bit error
>>   	interrupt, in this order.
>>
>> +Ethernet FIFO ECC
>> +Required Properties:
>> +- compatible : Should be "altr,socfpga-eth-mac-ecc"
>> +- reg        : Address and size for ECC block registers.
>>
>
> parent is too vague. How about altr,ethernet-mac.
>
OK. I'll change to this.

+- parent     : phandle to parent altr,ethernet-mac node

Thanks for reviewing!

>> +- interrupts : Should be single bit error interrupt, then double bit error
>> +	interrupt, in this order.
>> +
>>   Example:
>>
>>   	eccmgr: eccmgr at ffd06000 {
>> @@ -108,4 +116,20 @@ Example:
>>   			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>,
>>   				     <33 IRQ_TYPE_LEVEL_HIGH> ;
>>   		};
>> +
>> +		emac0-rx-ecc at ff8c0800 {
>> +			compatible = "altr,socfpga-eth-mac-ecc";
>> +			reg = <0xff8c0800 0x400>;
>> +			parent = <&gmac0>;
>> +			interrupts = <4 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <36 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		emac0-tx-ecc at ff8c0c00 {
>> +			compatible = "altr,socfpga-eth-mac-ecc";
>> +			reg = <0xff8c0c00 0x400>;
>> +			parent = <&gmac0>;
>> +			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <37 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>>   	};
>> --
>> 1.7.9.5
>>

[PATCHv3 5/7] EDAC, altera: Add Arria10 ECC memory init functions

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

In preparation for additional memory module ECCs, add the
memory initialization functions and helper functions used
for memory initialization.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2: Specify INTMODE selection -> IRQ on each ECC error.
    Insert functions above memory-specific functions so that function
    declarations are not required.
    Use ERRINTENS & ERRINTENR registers instead of read/modify/write.
v3: Changes for common compatibility string:
    - Pass node instead of compatibility string.
    - New altr_init_a10_ecc_device_type() for peripherals.
    - Add __init to altr_init_a10_ecc_block().
    - Add a10_get_irq_mask().
---
 drivers/edac/altera_edac.c |  197 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/edac/altera_edac.h |    8 ++
 2 files changed, 205 insertions(+)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index a3f490d..f27bb29 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -19,6 +19,7 @@
 
 #include <asm/cacheflush.h>
 #include <linux/ctype.h>
+#include <linux/delay.h>
 #include <linux/edac.h>
 #include <linux/genalloc.h>
 #include <linux/interrupt.h>
@@ -876,6 +877,202 @@ static irqreturn_t altr_edac_a10_ecc_irq(int irq, void *dev_id)
 
 #endif /* CONFIG_EDAC_ALTERA_OCRAM || CONFIG_EDAC_ALTERA_ETHERNET */
 
+/******************* Arria10 Memory Buffer Functions *********************/
+
+#if defined(CONFIG_EDAC_ALTERA_ETHERNET)
+
+static inline int a10_get_irq_mask(struct device_node *np)
+{
+	int irq;
+	const u32 *handle = of_get_property(np, "interrupts", NULL);
+
+	if (!handle)
+		return -ENODEV;
+	irq = be32_to_cpup(handle);
+	return irq;
+}
+
+static inline void ecc_set_bits(u32 bit_mask, void __iomem *ioaddr)
+{
+	u32 value = readl(ioaddr);
+
+	value |= bit_mask;
+	writel(value, ioaddr);
+}
+
+static inline void ecc_clear_bits(u32 bit_mask, void __iomem *ioaddr)
+{
+	u32 value = readl(ioaddr);
+
+	value &= ~bit_mask;
+	writel(value, ioaddr);
+}
+
+static inline int ecc_test_bits(u32 bit_mask, void __iomem *ioaddr)
+{
+	u32 value = readl(ioaddr);
+
+	return (value & bit_mask) ? 1 : 0;
+}
+
+/*
+ * This function uses the memory initialization block in the Arria10 ECC
+ * controller to initialize/clear the entire memory data and ECC data.
+ */
+static int altr_init_memory_port(void __iomem *ioaddr, int port)
+{
+	int limit = ALTR_A10_ECC_INIT_WATCHDOG_10US;
+	u32 init_mask = ALTR_A10_ECC_INITA;
+	u32 stat_mask = ALTR_A10_ECC_INITCOMPLETEA;
+	u32 clear_mask = ALTR_A10_ECC_ERRPENA_MASK;
+	int ret = 0;
+
+	if (port) {
+		init_mask = ALTR_A10_ECC_INITB;
+		stat_mask = ALTR_A10_ECC_INITCOMPLETEB;
+		clear_mask = ALTR_A10_ECC_ERRPENB_MASK;
+	}
+
+	ecc_set_bits(init_mask, (ioaddr + ALTR_A10_ECC_CTRL_OFST));
+	while (limit--) {
+		if (ecc_test_bits(stat_mask,
+				  (ioaddr + ALTR_A10_ECC_INITSTAT_OFST)))
+			break;
+		udelay(1);
+	}
+	if (limit < 0)
+		ret = -EBUSY;
+
+	/* Clear any pending ECC interrupts */
+	writel(clear_mask, (ioaddr + ALTR_A10_ECC_INTSTAT_OFST));
+
+	return ret;
+}
+
+/*
+ * Aside from the L2 ECC, the Arria10 ECC memories have a common register
+ * layout so the following functions can be shared between all peripherals.
+ */
+static __init int altr_init_a10_ecc_block(struct device_node *np, u32 irq_mask,
+					  u32 ecc_ctrl_en_mask, bool dual_port)
+{
+	int ret = 0;
+	void __iomem *ecc_block_base;
+	struct regmap *ecc_mgr_map;
+	char *ecc_name;
+	struct device_node *np_eccmgr;
+
+	ecc_name = (char *)np->name;
+
+	/* Get the ECC Manager - parent of the device EDACs */
+	np_eccmgr = of_get_parent(np);
+	ecc_mgr_map = syscon_regmap_lookup_by_phandle(np_eccmgr,
+						      "altr,sysmgr-syscon");
+	of_node_put(np_eccmgr);
+	if (IS_ERR(ecc_mgr_map)) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to get syscon altr,sysmgr-syscon\n");
+		return -ENODEV;
+	}
+
+	/* Map the ECC Block */
+	ecc_block_base = of_iomap(np, 0);
+	if (!ecc_block_base) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to map %s ECC block\n", ecc_name);
+		return -ENODEV;
+	}
+
+	/* Disable ECC */
+	regmap_write(ecc_mgr_map, A10_SYSMGR_ECC_INTMASK_SET_OFST, irq_mask);
+	writel(ALTR_A10_ECC_SERRINTEN,
+	       (ecc_block_base + ALTR_A10_ECC_ERRINTENR_OFST));
+	ecc_clear_bits(ecc_ctrl_en_mask,
+		       (ecc_block_base + ALTR_A10_ECC_CTRL_OFST));
+	/* Ensure all writes complete */
+	wmb();
+	/* Use HW initialization block to initialize memory for ECC */
+	ret = altr_init_memory_port(ecc_block_base, 0);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "ECC: cannot init %s PORTA memory\n", ecc_name);
+		goto out;
+	}
+
+	if (dual_port) {
+		ret = altr_init_memory_port(ecc_block_base, 1);
+		if (ret) {
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "ECC: cannot init %s PORTB memory\n",
+				    ecc_name);
+			goto out;
+		}
+	}
+
+	/* Interrupt mode set to every SBERR */
+	regmap_write(ecc_mgr_map, ALTR_A10_ECC_INTMODE_OFST,
+		     ALTR_A10_ECC_INTMODE);
+	/* Enable ECC */
+	ecc_set_bits(ecc_ctrl_en_mask, (ecc_block_base +
+					ALTR_A10_ECC_CTRL_OFST));
+	writel(ALTR_A10_ECC_SERRINTEN,
+	       (ecc_block_base + ALTR_A10_ECC_ERRINTENS_OFST));
+	regmap_write(ecc_mgr_map, A10_SYSMGR_ECC_INTMASK_CLR_OFST, irq_mask);
+	/* Ensure all writes complete */
+	wmb();
+out:
+	iounmap(ecc_block_base);
+	return ret;
+}
+
+static int validate_parent_available(struct device_node *np);
+static const struct of_device_id altr_edac_a10_device_of_match[];
+static int __init altr_init_a10_ecc_device_type(char *compat)
+{
+	int irq;
+	struct device_node *child, *np = of_find_compatible_node(NULL, NULL,
+					"altr,socfpga-a10-ecc-manager");
+	if (!np) {
+		edac_printk(KERN_ERR, EDAC_DEVICE, "ECC Manager not found\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(np, child) {
+		const struct of_device_id *pdev_id;
+		const struct edac_device_prv_data *prv;
+
+		if (!of_device_is_available(child))
+			continue;
+		if (!of_device_is_compatible(child, compat))
+			continue;
+
+		if (validate_parent_available(child))
+			continue;
+
+		irq = a10_get_irq_mask(child);
+		if (irq < 0)
+			continue;
+
+		/* Get matching node and check for valid result */
+		pdev_id = of_match_node(altr_edac_a10_device_of_match, child);
+		if (IS_ERR_OR_NULL(pdev_id))
+			continue;
+
+		/* Validate private data pointer before dereferencing */
+		prv = pdev_id->data;
+		if (!prv)
+			continue;
+
+		altr_init_a10_ecc_block(child, BIT(irq),
+					prv->ecc_enable_mask, 0);
+	}
+
+	of_node_put(np);
+	return 0;
+}
+
+#endif	/* CONFIG_EDAC_ALTERA_ETHERNET */
+
 /*********************** OCRAM EDAC Device Functions *********************/
 
 #ifdef CONFIG_EDAC_ALTERA_OCRAM
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index cf4e8cb..aa7c690 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -230,8 +230,13 @@ struct altr_sdram_mc_data {
 #define ALTR_A10_ECC_INITCOMPLETEB      BIT(8)
 
 #define ALTR_A10_ECC_ERRINTEN_OFST      0x10
+#define ALTR_A10_ECC_ERRINTENS_OFST     0x14
+#define ALTR_A10_ECC_ERRINTENR_OFST     0x18
 #define ALTR_A10_ECC_SERRINTEN          BIT(0)
 
+#define ALTR_A10_ECC_INTMODE_OFST       0x1C
+#define ALTR_A10_ECC_INTMODE            BIT(0)
+
 #define ALTR_A10_ECC_INTSTAT_OFST       0x20
 #define ALTR_A10_ECC_SERRPENA           BIT(0)
 #define ALTR_A10_ECC_DERRPENA           BIT(8)
@@ -280,6 +285,9 @@ struct altr_sdram_mc_data {
 /* Arria 10 OCRAM ECC Management Group Defines */
 #define ALTR_A10_OCRAM_ECC_EN_CTL       (BIT(1) | BIT(0))
 
+/* A10 ECC Controller memory initialization timeout */
+#define ALTR_A10_ECC_INIT_WATCHDOG_10US      10000
+
 struct altr_edac_device_dev;
 
 struct edac_device_prv_data {
-- 
1.7.9.5

[PATCHv3 5/7] EDAC, altera: Add Arria10 ECC memory init functions

[Borislav Petkov]

On Mon, Jun 13, 2016 at 04:19:10PM -0500, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> In preparation for additional memory module ECCs, add the
> memory initialization functions and helper functions used
> for memory initialization.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2: Specify INTMODE selection -> IRQ on each ECC error.
>     Insert functions above memory-specific functions so that function
>     declarations are not required.
>     Use ERRINTENS & ERRINTENR registers instead of read/modify/write.
> v3: Changes for common compatibility string:
>     - Pass node instead of compatibility string.
>     - New altr_init_a10_ecc_device_type() for peripherals.
>     - Add __init to altr_init_a10_ecc_block().
>     - Add a10_get_irq_mask().
> ---
>  drivers/edac/altera_edac.c |  197 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/edac/altera_edac.h |    8 ++
>  2 files changed, 205 insertions(+)

> +/*
> + * This function uses the memory initialization block in the Arria10 ECC
> + * controller to initialize/clear the entire memory data and ECC data.
> + */
> +static int altr_init_memory_port(void __iomem *ioaddr, int port)
> +{
> +	int limit = ALTR_A10_ECC_INIT_WATCHDOG_10US;
> +	u32 init_mask = ALTR_A10_ECC_INITA;
> +	u32 stat_mask = ALTR_A10_ECC_INITCOMPLETEA;
> +	u32 clear_mask = ALTR_A10_ECC_ERRPENA_MASK;
> +	int ret = 0;
> +
> +	if (port) {
> +		init_mask = ALTR_A10_ECC_INITB;
> +		stat_mask = ALTR_A10_ECC_INITCOMPLETEB;
> +		clear_mask = ALTR_A10_ECC_ERRPENB_MASK;
> +	}

Do a
	u32 init_mask, stat_mask, clear_mask;

	if (port) {
		init_mask = ALTR_A10_ECC_INITB;
		...
	} else {
		init_mask = ALTR_A10_ECC_INITA;
		...
	}

so that you don't have to repeat the assignments in the if (port) case.

> +
> +	ecc_set_bits(init_mask, (ioaddr + ALTR_A10_ECC_CTRL_OFST));
> +	while (limit--) {
> +		if (ecc_test_bits(stat_mask,
> +				  (ioaddr + ALTR_A10_ECC_INITSTAT_OFST)))
> +			break;
> +		udelay(1);
> +	}
> +	if (limit < 0)
> +		ret = -EBUSY;
> +
> +	/* Clear any pending ECC interrupts */
> +	writel(clear_mask, (ioaddr + ALTR_A10_ECC_INTSTAT_OFST));
> +
> +	return ret;
> +}
> +
> +/*
> + * Aside from the L2 ECC, the Arria10 ECC memories have a common register
> + * layout so the following functions can be shared between all peripherals.

I don't understand - we're here under

#if defined(CONFIG_EDAC_ALTERA_ETHERNET)

What sharing do you mean?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 5/7] EDAC, altera: Add Arria10 ECC memory init functions

[Thor Thayer]

On 06/17/2016 12:21 PM, Borislav Petkov wrote:
> On Mon, Jun 13, 2016 at 04:19:10PM -0500, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> In preparation for additional memory module ECCs, add the
>> memory initialization functions and helper functions used
>> for memory initialization.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2: Specify INTMODE selection -> IRQ on each ECC error.
>>      Insert functions above memory-specific functions so that function
>>      declarations are not required.
>>      Use ERRINTENS & ERRINTENR registers instead of read/modify/write.
>> v3: Changes for common compatibility string:
>>      - Pass node instead of compatibility string.
>>      - New altr_init_a10_ecc_device_type() for peripherals.
>>      - Add __init to altr_init_a10_ecc_block().
>>      - Add a10_get_irq_mask().
>> ---
>>   drivers/edac/altera_edac.c |  197 ++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/edac/altera_edac.h |    8 ++
>>   2 files changed, 205 insertions(+)
>
>> +/*
>> + * This function uses the memory initialization block in the Arria10 ECC
>> + * controller to initialize/clear the entire memory data and ECC data.
>> + */
>> +static int altr_init_memory_port(void __iomem *ioaddr, int port)
>> +{
>> +	int limit = ALTR_A10_ECC_INIT_WATCHDOG_10US;
>> +	u32 init_mask = ALTR_A10_ECC_INITA;
>> +	u32 stat_mask = ALTR_A10_ECC_INITCOMPLETEA;
>> +	u32 clear_mask = ALTR_A10_ECC_ERRPENA_MASK;
>> +	int ret = 0;
>> +
>> +	if (port) {
>> +		init_mask = ALTR_A10_ECC_INITB;
>> +		stat_mask = ALTR_A10_ECC_INITCOMPLETEB;
>> +		clear_mask = ALTR_A10_ECC_ERRPENB_MASK;
>> +	}
>
> Do a
> 	u32 init_mask, stat_mask, clear_mask;
>
> 	if (port) {
> 		init_mask = ALTR_A10_ECC_INITB;
> 		...
> 	} else {
> 		init_mask = ALTR_A10_ECC_INITA;
> 		...
> 	}
>
> so that you don't have to repeat the assignments in the if (port) case.
>
OK. I only have one PORTB peripheral so normally the if (port) branch 
won't execute but I see your point.

>> +
>> +	ecc_set_bits(init_mask, (ioaddr + ALTR_A10_ECC_CTRL_OFST));
>> +	while (limit--) {
>> +		if (ecc_test_bits(stat_mask,
>> +				  (ioaddr + ALTR_A10_ECC_INITSTAT_OFST)))
>> +			break;
>> +		udelay(1);
>> +	}
>> +	if (limit < 0)
>> +		ret = -EBUSY;
>> +
>> +	/* Clear any pending ECC interrupts */
>> +	writel(clear_mask, (ioaddr + ALTR_A10_ECC_INTSTAT_OFST));
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Aside from the L2 ECC, the Arria10 ECC memories have a common register
>> + * layout so the following functions can be shared between all peripherals.
>
> I don't understand - we're here under
>
> #if defined(CONFIG_EDAC_ALTERA_ETHERNET)
>
> What sharing do you mean?
>
I'll be adding a number of peripheral FIFOs in future patches but I 
agree that the comment is misleading in this case. I'll fix it.

[PATCHv3 6/7] EDAC, altera: Add Arria10 Ethernet EDAC support

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

Add Altera Arria10 Ethernet FIFO memory EDAC support. Update
to support a common compatibility string for all ethernet
FIFOs in the DT.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2  Remove (void *) cast from altr_edac_device_of_match[]
    Addition of panic flag to ethernet private data.
v3  Use common compatiblity string.
    Simplify socfpga_init_ethernet_ecc().
---
 drivers/edac/Kconfig       |    7 +++++++
 drivers/edac/altera_edac.c |   38 ++++++++++++++++++++++++++++++++++++--
 drivers/edac/altera_edac.h |    3 +++
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 6ca7474..d0c1dab 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -391,6 +391,13 @@ config EDAC_ALTERA_OCRAM
 	  Support for error detection and correction on the
 	  Altera On-Chip RAM Memory for Altera SoCs.
 
+config EDAC_ALTERA_ETHERNET
+	bool "Altera Ethernet FIFO ECC"
+	depends on EDAC_ALTERA=y
+	help
+	  Support for error detection and correction on the
+	  Altera Ethernet FIFO Memory for Altera SoCs.
+
 config EDAC_SYNOPSYS
 	tristate "Synopsys DDR Memory Controller"
 	depends on EDAC_MM_EDAC && ARCH_ZYNQ
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index f27bb29..14f99ae 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1260,6 +1260,34 @@ const struct edac_device_prv_data a10_l2ecc_data = {
 
 #endif	/* CONFIG_EDAC_ALTERA_L2C */
 
+/********************* Ethernet Device Functions ********************/
+
+#ifdef CONFIG_EDAC_ALTERA_ETHERNET
+
+const struct edac_device_prv_data a10_enet0rxecc_data = {
+	.setup = altr_check_ecc_deps,
+	.ce_clear_mask = ALTR_A10_ECC_SERRPENA,
+	.ue_clear_mask = ALTR_A10_ECC_DERRPENA,
+	.dbgfs_name = "altr_trigger",
+	.ecc_enable_mask = ALTR_A10_ETHERNET_ECC_EN_CTL,
+	.ecc_en_ofst = ALTR_A10_ECC_CTRL_OFST,
+	.ce_set_mask = ALTR_A10_ECC_TSERRA,
+	.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,
+	.panic = false,
+};
+
+static int __init socfpga_init_ethernet_ecc(void)
+{
+	return altr_init_a10_ecc_device_type("altr,socfpga-eth-mac-ecc");
+}
+
+early_initcall(socfpga_init_ethernet_ecc);
+
+#endif	/* CONFIG_EDAC_ALTERA_ETHERNET */
+
 /********************* Arria10 EDAC Device Functions *************************/
 static const struct of_device_id altr_edac_a10_device_of_match[] = {
 #ifdef CONFIG_EDAC_ALTERA_L2C
@@ -1269,6 +1297,10 @@ static const struct of_device_id altr_edac_a10_device_of_match[] = {
 	{ .compatible = "altr,socfpga-a10-ocram-ecc",
 	  .data = &a10_ocramecc_data },
 #endif
+#ifdef CONFIG_EDAC_ALTERA_ETHERNET
+	{ .compatible = "altr,socfpga-eth-mac-ecc",
+	  .data = &a10_enet0rxecc_data },
+#endif
 	{},
 };
 MODULE_DEVICE_TABLE(of, altr_edac_a10_device_of_match);
@@ -1557,8 +1589,10 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
 			continue;
 		if (of_device_is_compatible(child, "altr,socfpga-a10-l2-ecc"))
 			altr_edac_a10_device_add(edac, child);
-		else if (of_device_is_compatible(child,
-						 "altr,socfpga-a10-ocram-ecc"))
+		else if ((of_device_is_compatible(child,
+					"altr,socfpga-a10-ocram-ecc")) ||
+			 (of_device_is_compatible(child,
+					"altr,socfpga-eth-mac-ecc")))
 			altr_edac_a10_device_add(edac, child);
 		else if (of_device_is_compatible(child,
 						 "altr,sdram-edac-a10"))
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index aa7c690..9e3f986 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -285,6 +285,9 @@ struct altr_sdram_mc_data {
 /* Arria 10 OCRAM ECC Management Group Defines */
 #define ALTR_A10_OCRAM_ECC_EN_CTL       (BIT(1) | BIT(0))
 
+/* Arria 10 Ethernet ECC Management Group Defines */
+#define ALTR_A10_ETHERNET_ECC_EN_CTL    BIT(0)
+
 /* A10 ECC Controller memory initialization timeout */
 #define ALTR_A10_ECC_INIT_WATCHDOG_10US      10000
 
-- 
1.7.9.5

[PATCHv3 6/7] EDAC, altera: Add Arria10 Ethernet EDAC support

[Borislav Petkov]

On Mon, Jun 13, 2016 at 04:19:11PM -0500, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Add Altera Arria10 Ethernet FIFO memory EDAC support. Update
> to support a common compatibility string for all ethernet
> FIFOs in the DT.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2  Remove (void *) cast from altr_edac_device_of_match[]
>     Addition of panic flag to ethernet private data.
> v3  Use common compatiblity string.
>     Simplify socfpga_init_ethernet_ecc().
> ---
>  drivers/edac/Kconfig       |    7 +++++++
>  drivers/edac/altera_edac.c |   38 ++++++++++++++++++++++++++++++++++++--
>  drivers/edac/altera_edac.h |    3 +++
>  3 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 6ca7474..d0c1dab 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -391,6 +391,13 @@ config EDAC_ALTERA_OCRAM
>  	  Support for error detection and correction on the
>  	  Altera On-Chip RAM Memory for Altera SoCs.
>  
> +config EDAC_ALTERA_ETHERNET
> +	bool "Altera Ethernet FIFO ECC"
> +	depends on EDAC_ALTERA=y
> +	help
> +	  Support for error detection and correction on the
> +	  Altera Ethernet FIFO Memory for Altera SoCs.
> +
>  config EDAC_SYNOPSYS
>  	tristate "Synopsys DDR Memory Controller"
>  	depends on EDAC_MM_EDAC && ARCH_ZYNQ
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index f27bb29..14f99ae 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -1260,6 +1260,34 @@ const struct edac_device_prv_data a10_l2ecc_data = {
>  
>  #endif	/* CONFIG_EDAC_ALTERA_L2C */
>  
> +/********************* Ethernet Device Functions ********************/
> +
> +#ifdef CONFIG_EDAC_ALTERA_ETHERNET
> +
> +const struct edac_device_prv_data a10_enet0rxecc_data = {

This needs to be "static const" and then you don't need to init panic to
false.

Your other edac_device_prv_data things are not static too, please
convert them in a separate patch too.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[PATCHv3 6/7] EDAC, altera: Add Arria10 Ethernet EDAC support

[Thor Thayer]

Hi Boris,

On 06/17/2016 12:29 PM, Borislav Petkov wrote:
> On Mon, Jun 13, 2016 at 04:19:11PM -0500, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add Altera Arria10 Ethernet FIFO memory EDAC support. Update
>> to support a common compatibility string for all ethernet
>> FIFOs in the DT.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2  Remove (void *) cast from altr_edac_device_of_match[]
>>      Addition of panic flag to ethernet private data.
>> v3  Use common compatiblity string.
>>      Simplify socfpga_init_ethernet_ecc().
>> ---
>>   drivers/edac/Kconfig       |    7 +++++++
>>   drivers/edac/altera_edac.c |   38 ++++++++++++++++++++++++++++++++++++--
>>   drivers/edac/altera_edac.h |    3 +++
>>   3 files changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 6ca7474..d0c1dab 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -391,6 +391,13 @@ config EDAC_ALTERA_OCRAM
>>   	  Support for error detection and correction on the
>>   	  Altera On-Chip RAM Memory for Altera SoCs.
>>
>> +config EDAC_ALTERA_ETHERNET
>> +	bool "Altera Ethernet FIFO ECC"
>> +	depends on EDAC_ALTERA=y
>> +	help
>> +	  Support for error detection and correction on the
>> +	  Altera Ethernet FIFO Memory for Altera SoCs.
>> +
>>   config EDAC_SYNOPSYS
>>   	tristate "Synopsys DDR Memory Controller"
>>   	depends on EDAC_MM_EDAC && ARCH_ZYNQ
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index f27bb29..14f99ae 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -1260,6 +1260,34 @@ const struct edac_device_prv_data a10_l2ecc_data = {
>>
>>   #endif	/* CONFIG_EDAC_ALTERA_L2C */
>>
>> +/********************* Ethernet Device Functions ********************/
>> +
>> +#ifdef CONFIG_EDAC_ALTERA_ETHERNET
>> +
>> +const struct edac_device_prv_data a10_enet0rxecc_data = {
>
> This needs to be "static const" and then you don't need to init panic to
> false.
>
> Your other edac_device_prv_data things are not static too, please
> convert them in a separate patch too.
>
OK, I'll make the change. Thanks!

[PATCHv3 7/7] ARM: dts: Add Arria10 Ethernet EDAC devicetree entry

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

Add the device tree entries needed to support the Altera Ethernet
FIFO buffer EDAC on the Arria10 chip.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2  No change
v3  Add interrupts for SBERR and DBERR.
---
 arch/arm/boot/dts/socfpga_arria10.dtsi |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi b/arch/arm/boot/dts/socfpga_arria10.dtsi
index 21f6c3c..5cf4dc5 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -628,6 +628,22 @@
 				interrupts = <1 IRQ_TYPE_LEVEL_HIGH>,
 					     <33 IRQ_TYPE_LEVEL_HIGH>;
 			};
+
+			emac0-rx-ecc at ff8c0800 {
+				compatible = "altr,socfpga-eth-mac-ecc";
+				reg = <0xff8c0800 0x400>;
+				parent = <&gmac0>;
+				interrupts = <4 IRQ_TYPE_LEVEL_HIGH>,
+					     <36 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
+			emac0-tx-ecc at ff8c0c00 {
+				compatible = "altr,socfpga-eth-mac-ecc";
+				reg = <0xff8c0c00 0x400>;
+				parent = <&gmac0>;
+				interrupts = <5 IRQ_TYPE_LEVEL_HIGH>,
+					     <37 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		rst: rstmgr at ffd05000 {
-- 
1.7.9.5