Re: RE: RE: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs in correct order

[Frank Li]

On Thu, Oct 17, 2024 at 04:21:33PM +0200, Marc Kleine-Budde wrote:
> On 17.10.2024 10:03:51, Frank Li wrote:
> > > > > Yes, that is IMHO the correct description of the IP core, but the
> > > > > i.MX8M/N/Q DTS have the wrong order of IRQs. And for compatibility
> > > > > reasons (fixed DTS with old driver) it's IMHO not possible to change the
> > > > > DTS.
> > > > >
> > > >
> > > > I don't think it is a correct behavior for old drivers to use new DTBs or new
> > > > drivers to use old DTBs. Maybe you are correct, Frank also asked the same
> > > > question, let's see how Frank responded.
> > >
> > > DTBs should be considered stable ABI.
> > >
> >
> > ABI defined at binding doc.
> >   interrupt-names:
> >     oneOf:
> >       - items:
> >           - const: int0
> >       - items:
> >           - const: int0
> >           - const: pps
> >       - items:
> >           - const: int0
> >           - const: int1
> >           - const: int2
> >       - items:
> >           - const: int0
> >           - const: int1
> >           - const: int2
> >           - const: pps
> >
> > DTB should align binding doc. There are not 'descriptions' at 'interrupt',
> > which should match 'interrupt-names'. So IMX8MP dts have not match ABI,
> > which defined by binding doc. So it is DTS implement wrong.
>
> I follow your conclusion. But keep in mind, fixing the DTB would break
> compatibility. The wrong DTS looks like this:
>
> - const: int1
> - const: int2
> - const: int0
> - const: pps
>
> Currently we have broken DTS on the i.MX8M* and the
> FEC_QUIRK_WAKEUP_FROM_INT2 that "fixes" this.
>
> This patch uses this quirk to correct the IRQ <-> queue assignment in
> the driver.

This current code

for (i = 0; i < irq_cnt; i++) {
                snprintf(irq_name, sizeof(irq_name), "int%d", i);
                irq = platform_get_irq_byname_optional(pdev, irq_name);
		      ^^^^^^^^^^^^^^^^^^^^^

You just need add interrupt-names at imx8mp dts and reorder it to pass
DTB check.

                if (irq < 0)
                        irq = platform_get_irq(pdev, i);
                if (irq < 0) {
                        ret = irq;
                        goto failed_irq;
                }
                ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
                                       0, pdev->name, ndev);
                if (ret)
                        goto failed_irq;

                fep->irq[i] = irq;
        }

All irq handle by the same fec_enet_interrupt().  Change dts irq orders
doesn't broken compatiblity.

"pre-equeue" irq is new features. You can enable this feature only
when "interrupt-names" exist in future.

>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |