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

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 <[email protected]> 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 <[email protected]>
> > ---
> > 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/[email protected]/
> >   - 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
>