| Message ID | 1548799428-10541-1-git-send-email-thor.thayer@linux.intel.com |
|---|---|
| Headers | show |
| Series | Update Stratix10 EDAC Bindings | expand |
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
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
On 1/29/19 4:03 PM, thor.thayer@linux.intel.com wrote: > From: Thor Thayer <thor.thayer@linux.intel.com> > > Instead of using the Arria10 (ARM32) EDAC bindings for > Stratix10 (ARM64), create Stratix10 specific EDAC bindings > to capture architecture differences between ARM32 and ARM64. > This requires fixing the previous Stratix10 bindings. > Also add the peripheral bindings for the Stratix10. > > Thor Thayer (4): > Documentation: dt: edac: Fix Stratix10 IRQ bindings > Documentation: dt: edac: Add Stratix10 Peripheral bindings > EDAC, altera: Skip DB IRQ for Stratix10 > arm64: dts: stratix10: Use new Stratix10 EDAC bindings > > .../devicetree/bindings/edac/socfpga-eccmgr.txt | 129 +++++++++++++++++++-- > arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 25 ++-- > drivers/edac/altera_edac.c | 31 ++--- > 3 files changed, 153 insertions(+), 32 deletions(-) > Abandoning this review. As Dinh suggested, there are a few places I can use the Stratix10 compatibles to make the code cleaner (although a few socfpga_is_a10() calls will remain where it makes sense). I'll resubmit a new version with more Stratix10 compatibles.
From: Thor Thayer <thor.thayer@linux.intel.com> Instead of using the Arria10 (ARM32) EDAC bindings for Stratix10 (ARM64), create Stratix10 specific EDAC bindings to capture architecture differences between ARM32 and ARM64. This requires fixing the previous Stratix10 bindings. Also add the peripheral bindings for the Stratix10. Thor Thayer (4): Documentation: dt: edac: Fix Stratix10 IRQ bindings Documentation: dt: edac: Add Stratix10 Peripheral bindings EDAC, altera: Skip DB IRQ for Stratix10 arm64: dts: stratix10: Use new Stratix10 EDAC bindings .../devicetree/bindings/edac/socfpga-eccmgr.txt | 129 +++++++++++++++++++-- arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 25 ++-- drivers/edac/altera_edac.c | 31 ++--- 3 files changed, 153 insertions(+), 32 deletions(-)