Patchwork [3/4] EDAC, altera: Skip DB IRQ for Stratix10

login
register
mail settings
Submitter thor.thayer@linux.intel.com
Date Jan. 29, 2019, 10:03 p.m.
Message ID <1548799428-10541-4-git-send-email-thor.thayer@linux.intel.com>
Download mbox | patch
Permalink /patch/712919/
State New
Headers show

Comments

thor.thayer@linux.intel.com - Jan. 29, 2019, 10:03 p.m.
From: Thor Thayer <thor.thayer@linux.intel.com>

Stratix10 Double Bit errors are configured as SErrors
so skip the Double Bit IRQ initialization if Stratix10.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
 drivers/edac/altera_edac.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)
Dinh Nguyen - Feb. 1, 2019, 3:37 p.m.
Hi Thor,

On 1/29/19 4:03 PM, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> Stratix10 Double Bit errors are configured as SErrors
> so skip the Double Bit IRQ initialization if Stratix10.
> 
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
>  drivers/edac/altera_edac.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index c89d82aa2776..6a460c742e3f 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -1924,20 +1924,25 @@ static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
>  		goto err_release_group1;
>  	}
>  
> -	altdev->db_irq = irq_of_parse_and_map(np, 1);
> -	if (!altdev->db_irq) {
> -		edac_printk(KERN_ERR, EDAC_DEVICE, "Error allocating DBIRQ\n");
> -		rc = -ENODEV;
> -		goto err_release_group1;
> -	}
> -	rc = devm_request_irq(edac->dev, altdev->db_irq, prv->ecc_irq_handler,
> -			      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -			      ecc_name, altdev);
> -	if (rc) {
> -		edac_printk(KERN_ERR, EDAC_DEVICE, "No DBERR IRQ resource\n");
> -		goto err_release_group1;
> +	/* Arria10 has double bit error IRQs. Stratix10 uses SErrors */
> +	if (socfpga_is_a10()) {

I see that there are socfpga_is_a10() and socfpga_is_s10() sprinkled
around the driver. Since you're adding a specific binding for s10, would
it make sense to remove these functions? I've gotten comments in the
past from ARM maintainers that we want to avoid looking up platforms at
runtime and make the differentiation at load time.

Dinh
thor.thayer@linux.intel.com - Feb. 1, 2019, 4:51 p.m.
Hi Dinh,

On 2/1/19 9:37 AM, Dinh Nguyen wrote:
> Hi Thor,
> 
> On 1/29/19 4:03 PM, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> Stratix10 Double Bit errors are configured as SErrors
>> so skip the Double Bit IRQ initialization if Stratix10.
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>>   drivers/edac/altera_edac.c | 31 ++++++++++++++++++-------------
>>   1 file changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index c89d82aa2776..6a460c742e3f 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -1924,20 +1924,25 @@ static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
>>   		goto err_release_group1;
>>   	}
>>   
>> -	altdev->db_irq = irq_of_parse_and_map(np, 1);
>> -	if (!altdev->db_irq) {
>> -		edac_printk(KERN_ERR, EDAC_DEVICE, "Error allocating DBIRQ\n");
>> -		rc = -ENODEV;
>> -		goto err_release_group1;
>> -	}
>> -	rc = devm_request_irq(edac->dev, altdev->db_irq, prv->ecc_irq_handler,
>> -			      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
>> -			      ecc_name, altdev);
>> -	if (rc) {
>> -		edac_printk(KERN_ERR, EDAC_DEVICE, "No DBERR IRQ resource\n");
>> -		goto err_release_group1;
>> +	/* Arria10 has double bit error IRQs. Stratix10 uses SErrors */
>> +	if (socfpga_is_a10()) {
> 
> I see that there are socfpga_is_a10() and socfpga_is_s10() sprinkled
> around the driver. Since you're adding a specific binding for s10, would
> it make sense to remove these functions? I've gotten comments in the
> past from ARM maintainers that we want to avoid looking up platforms at
> runtime and make the differentiation at load time.
> 
> Dinh
> 

If there were larger differences between the Arria10 and Stratix10 then 
I'd agree. The differences between Arria10 and Stratix10 are minor 
because the ECC blocks are similar.

Since all the different families of Altera EDACs are in the same file, I 
think the runtime allocation is warranted especially since these checks 
are in the initialization functions. The interrupt handling functions 
are clean.

If the maintainers have a strong preference for separate functions, I 
can make that change but the Stratix10 functions will be very similar to 
the Arria10 functions resulting in a much larger file.

My preference would be to keep the method in this patch but of course, 
I'll follow the consensus of the maintainers.

Thanks for the review and comment!

Thor

Patch

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index c89d82aa2776..6a460c742e3f 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1924,20 +1924,25 @@  static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
 		goto err_release_group1;
 	}
 
-	altdev->db_irq = irq_of_parse_and_map(np, 1);
-	if (!altdev->db_irq) {
-		edac_printk(KERN_ERR, EDAC_DEVICE, "Error allocating DBIRQ\n");
-		rc = -ENODEV;
-		goto err_release_group1;
-	}
-	rc = devm_request_irq(edac->dev, altdev->db_irq, prv->ecc_irq_handler,
-			      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
-			      ecc_name, altdev);
-	if (rc) {
-		edac_printk(KERN_ERR, EDAC_DEVICE, "No DBERR IRQ resource\n");
-		goto err_release_group1;
+	/* Arria10 has double bit error IRQs. Stratix10 uses SErrors */
+	if (socfpga_is_a10()) {
+		altdev->db_irq = irq_of_parse_and_map(np, 1);
+		if (!altdev->db_irq) {
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "Error allocating DBIRQ\n");
+			rc = -ENODEV;
+			goto err_release_group1;
+		}
+		rc = devm_request_irq(edac->dev, altdev->db_irq,
+				      prv->ecc_irq_handler,
+				      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+				      ecc_name, altdev);
+		if (rc) {
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "No DBERR IRQ resource\n");
+			goto err_release_group1;
+		}
 	}
-
 	rc = edac_device_add_device(dci);
 	if (rc) {
 		dev_err(edac->dev, "edac_device_add_device failed\n");