Re: [PATCH v3] EDAC/altera: use ECC manager compatible to select A10/S10 IRQ layout

From: Rounak Das

Date: Fri Jun 26 2026 - 04:29:56 EST

Hi Dinh,
I'm glad to know that it tested clean on both platforms.

To make sure that I understand the approach correctly, do you mean to
carry the A10/S10 distinction in the `altr_edac_a10_of_match[]` table
via the `.data` field?
And then reading it back in probe through `device_get_match_data()`?

If I got it right, I'll send it as v4.

Thanks,
Rounak

On Thu, Jun 25, 2026 at 4:09 PM Dinh Nguyen <dinguyen@xxxxxxxxxx> wrote:
>
>
>
> On 6/16/26 19:07, Rounak Das wrote:
> > The SDMMC ECC IRQ layout selection uses CONFIG_64BIT to distinguish
> > between Arria10 and Stratix10 paths. This is architecture-based,
> > while the interrupt layout is a hardware property described by DT
> > compatible strings.
> >
> > Detect the SoC once at probe via the altr,socfpga-s10-ecc-manager
> > compatible on the ECC manager node, store it under the struct
> > altr_arria10_edac, and use it in altr_portb_setup() and
> > altr_edac_a10_device_add() in place of CONFIG_64BIT.
> >
> > Selecting on the manager compatible keeps the decision SoC-wide and
> > correct for every ECC child device (OCRAM, USB, EMAC, SD/MMC),
> > and avoids repeated compatible lookups. Stratix10 and
> > Agilex both declare altr,socfpga-s10-ecc-manager and Arria10 does not,
> > so behaviour is unchanged on all three SoCs.
> >
> > Signed-off-by: Rounak Das <rounakdas2025@xxxxxxxxx>
> > ---
> > v3:
> > - Use compatible string altr,socfpga-s10-ecc-manager instead of
> > sdmmc-ecc (Dinh).
> > - Set it once into struct altr_arria10_edac::is_s10 to
> > avoid repeated of_device_is_compatible() calls (Dinh).
> > - Fix the checkpatch open-parenthesis alignment.
> >
> > v2: https://lore.kernel.org/linux-edac/20260616081709.48774-1-rounakdas2025@xxxxxxxxx/
> > - Use legal name in Signed-off-by (Borislav).
> >
> > drivers/edac/altera_edac.c | 106 +++++++++++++++++++------------------
> > drivers/edac/altera_edac.h | 1 +
> > 2 files changed, 55 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> > index 4edd2088c2db6..c728cd474abdf 100644
> > --- a/drivers/edac/altera_edac.c
> > +++ b/drivers/edac/altera_edac.c
> > @@ -1507,6 +1507,7 @@ static int altr_portb_setup(struct altr_edac_device_dev *device)
> > int edac_idx, rc;
> > struct device_node *np;
> > const struct edac_device_prv_data *prv = &a10_sdmmceccb_data;
> > + bool is_s10 = device->edac->is_s10;
> >
> > rc = altr_check_ecc_deps(device);
> > if (rc)
> > @@ -1548,15 +1549,14 @@ static int altr_portb_setup(struct altr_edac_device_dev *device)
> >
> > /*
> > * Update the PortB IRQs - A10 has 4, S10 has 2, Index accordingly
> > - *
> > - * FIXME: Instead of ifdefs with different architectures the driver
> > - * should properly use compatibles.
> > */
> > -#ifdef CONFIG_64BIT
> > - altdev->sb_irq = irq_of_parse_and_map(np, 1);
> > -#else
> > - altdev->sb_irq = irq_of_parse_and_map(np, 2);
> > -#endif
> > +
> > + /* Using compatibles to determine the IRQ Index */
> > + if (is_s10)
> > + altdev->sb_irq = irq_of_parse_and_map(np, 1);
> > + else
> > + altdev->sb_irq = irq_of_parse_and_map(np, 2);
> > +
> > if (!altdev->sb_irq) {
> > edac_printk(KERN_ERR, EDAC_DEVICE, "Error PortB SBIRQ alloc\n");
> > rc = -ENODEV;
> > @@ -1570,29 +1570,28 @@ static int altr_portb_setup(struct altr_edac_device_dev *device)
> > goto err_release_group_1;
> > }
> >
> > -#ifdef CONFIG_64BIT
> > - /* Use IRQ to determine SError origin instead of assigning IRQ */
> > - rc = of_property_read_u32_index(np, "interrupts", 1, &altdev->db_irq);
> > - if (rc) {
> > - edac_printk(KERN_ERR, EDAC_DEVICE,
> > - "Error PortB DBIRQ alloc\n");
> > - goto err_release_group_1;
> > - }
> > -#else
> > - altdev->db_irq = irq_of_parse_and_map(np, 3);
> > - if (!altdev->db_irq) {
> > - edac_printk(KERN_ERR, EDAC_DEVICE, "Error PortB DBIRQ alloc\n");
> > - rc = -ENODEV;
> > - goto err_release_group_1;
> > - }
> > - rc = devm_request_irq(&altdev->ddev, altdev->db_irq,
> > - prv->ecc_irq_handler, IRQF_TRIGGER_HIGH,
> > - ecc_name, altdev);
> > - if (rc) {
> > - edac_printk(KERN_ERR, EDAC_DEVICE, "PortB DBERR IRQ error\n");
> > - goto err_release_group_1;
> > + if (is_s10) {
> > + /* Use IRQ to determine SError origin instead of assigning IRQ */
> > + rc = of_property_read_u32_index(np, "interrupts", 1, &altdev->db_irq);
> > + if (rc) {
> > + edac_printk(KERN_ERR, EDAC_DEVICE, "Error PortB DBIRQ alloc\n");
> > + goto err_release_group_1;
> > + }
> > + } else {
> > + altdev->db_irq = irq_of_parse_and_map(np, 3);
> > + if (!altdev->db_irq) {
> > + edac_printk(KERN_ERR, EDAC_DEVICE, "Error PortB DBIRQ alloc\n");
> > + rc = -ENODEV;
> > + goto err_release_group_1;
> > + }
> > + rc = devm_request_irq(&altdev->ddev, altdev->db_irq,
> > + prv->ecc_irq_handler, IRQF_TRIGGER_HIGH,
> > + ecc_name, altdev);
> > + if (rc) {
> > + edac_printk(KERN_ERR, EDAC_DEVICE, "PortB DBERR IRQ error\n");
> > + goto err_release_group_1;
> > + }
> > }
> > -#endif
> >
> > rc = edac_device_add_device(dci);
> > if (rc) {
> > @@ -1974,29 +1973,29 @@ static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
> > goto err_release_group1;
> > }
> >
> > -#ifdef CONFIG_64BIT
> > - /* Use IRQ to determine SError origin instead of assigning IRQ */
> > - rc = of_property_read_u32_index(np, "interrupts", 0, &altdev->db_irq);
> > - if (rc) {
> > - edac_printk(KERN_ERR, EDAC_DEVICE,
> > - "Unable to parse DB IRQ index\n");
> > - goto err_release_group1;
> > - }
> > -#else
> > - 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_TRIGGER_HIGH,
> > - ecc_name, altdev);
> > - if (rc) {
> > - edac_printk(KERN_ERR, EDAC_DEVICE, "No DBERR IRQ resource\n");
> > - goto err_release_group1;
> > + if (edac->is_s10) {
> > + /* Use IRQ to determine SError origin instead of assigning IRQ */
> > + rc = of_property_read_u32_index(np, "interrupts", 0, &altdev->db_irq);
> > + if (rc) {
> > + edac_printk(KERN_ERR, EDAC_DEVICE,
> > + "Unable to parse DB IRQ index\n");
> > + goto err_release_group1;
> > + }
> > + } else {
> > + 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_TRIGGER_HIGH,
> > + ecc_name, altdev);
> > + if (rc) {
> > + edac_printk(KERN_ERR, EDAC_DEVICE, "No DBERR IRQ resource\n");
> > + goto err_release_group1;
> > + }
> > }
> > -#endif
> >
> > rc = edac_device_add_device(dci);
> > if (rc) {
> > @@ -2122,6 +2121,9 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
> > platform_set_drvdata(pdev, edac);
> > INIT_LIST_HEAD(&edac->a10_ecc_devices);
> >
> > + edac->is_s10 = of_device_is_compatible(pdev->dev.of_node,
> > + "altr,socfpga-s10-ecc-manager");
> > +
>
>
> I tested this version on both 32-bit and 64-bit platforms and there were
> no regression. But I'd like to see if we can avoid using
> of_device_is_compatible in runtime code and see if we can just bind to
> the "altr,socfpga-s10-ecc-manager" if possible?
>
> Thanks,
> Dinh
>