| Message ID | 20241016-fec-cleanups-v1-7-de783bd15e6a@pengutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: fec: cleanups, update quirk, update IRQ naming | expand |
On Wed, Oct 16, 2024 at 11:51:55PM +0200, Marc Kleine-Budde wrote: > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > tree is not optimal. The driver expects the first three IRQs to match > their corresponding queue, while the last (fourth) IRQ is used for the > PPS: > > - 1st IRQ: "int0": queue0 + other IRQs > - 2nd IRQ: "int1": queue1 > - 3rd IRQ: "int2": queue2 > - 4th IRQ: "pps": pps > > However, the i.MX8MQ and compatible SoCs do not use the > "interrupt-names" property and specify the IRQs in the wrong order: > > - 1st IRQ: queue1 > - 2nd IRQ: queue2 > - 3rd IRQ: queue0 + other IRQs > - 4th IRQ: pps why not fix dts? Frank > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back > in the correct order, this is done in fec_probe(). > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/net/ethernet/freescale/fec.h | 24 ++++++++++++++++++++++-- > drivers/net/ethernet/freescale/fec_main.c | 18 +++++++++++------- > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h > index 63744a86752540fcede7fc4c29865b2529492526..b0f1a3e28d5c8052be3a8a0fa18303a1df2bb5bd 100644 > --- a/drivers/net/ethernet/freescale/fec.h > +++ b/drivers/net/ethernet/freescale/fec.h > @@ -504,8 +504,28 @@ struct bufdesc_ex { > */ > #define FEC_QUIRK_DELAYED_CLKS_SUPPORT (1 << 21) > > -/* i.MX8MQ SoC integration mix wakeup interrupt signal into "int2" interrupt line. */ > -#define FEC_QUIRK_WAKEUP_FROM_INT2 (1 << 22) > +/* With i.MX8MQ and compatible SoCs, the order of the IRQs in the > + * device tree is not optimal. The driver expects the first three IRQs > + * to match their corresponding queue, while the last (fourth) IRQ is > + * used for the PPS: > + * > + * - 1st IRQ: "int0": queue0 + other IRQs > + * - 2nd IRQ: "int1": queue1 > + * - 3rd IRQ: "int2": queue2 > + * - 4th IRQ: "pps": pps > + * > + * However, the i.MX8MQ and compatible SoCs do not use the > + * "interrupt-names" property and specify the IRQs in the wrong order: > + * > + * - 1st IRQ: queue1 > + * - 2nd IRQ: queue2 > + * - 3rd IRQ: queue0 + other IRQs > + * - 4th IRQ: pps > + * > + * If the following quirk is active, put the IRQs back in the correct > + * order, this is done in fec_probe(). > + */ > +#define FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ BIT(22) > > /* i.MX6Q adds pm_qos support */ > #define FEC_QUIRK_HAS_PMQOS BIT(23) > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index d948ed9810027d5fabe521dc3af2cf505dacd13e..f124ffe3619d82dc089f8494d33d2398e6f631fb 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -157,7 +157,7 @@ static const struct fec_devinfo fec_imx8mq_info = { > FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE | > FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE | > FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES | > - FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 | > + FEC_QUIRK_HAS_EEE | FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ | > FEC_QUIRK_HAS_MDIO_C45, > }; > > @@ -4260,10 +4260,7 @@ static void fec_enet_get_wakeup_irq(struct platform_device *pdev) > struct net_device *ndev = platform_get_drvdata(pdev); > struct fec_enet_private *fep = netdev_priv(ndev); > > - if (fep->quirks & FEC_QUIRK_WAKEUP_FROM_INT2) > - fep->wake_irq = fep->irq[2]; > - else > - fep->wake_irq = fep->irq[0]; > + fep->wake_irq = fep->irq[0]; > } > > static int fec_enet_init_stop_mode(struct fec_enet_private *fep, > @@ -4495,10 +4492,17 @@ fec_probe(struct platform_device *pdev) > goto failed_init; > > for (i = 0; i < irq_cnt; i++) { > - snprintf(irq_name, sizeof(irq_name), "int%d", i); > + int irq_num; > + > + if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ) > + irq_num = (i + irq_cnt - 1) % irq_cnt; > + else > + irq_num = i; > + > + snprintf(irq_name, sizeof(irq_name), "int%d", irq_num); > irq = platform_get_irq_byname_optional(pdev, irq_name); > if (irq < 0) > - irq = platform_get_irq(pdev, i); > + irq = platform_get_irq(pdev, irq_num); > if (irq < 0) { > ret = irq; > goto failed_irq; > > -- > 2.45.2 > >
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2024年10月17日 5:52 > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>; > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard > Cochran <richardcochran@gmail.com> > Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de> > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs > in correct order > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > tree is not optimal. The driver expects the first three IRQs to match > their corresponding queue, while the last (fourth) IRQ is used for the > PPS: > > - 1st IRQ: "int0": queue0 + other IRQs > - 2nd IRQ: "int1": queue1 > - 3rd IRQ: "int2": queue2 > - 4th IRQ: "pps": pps > > However, the i.MX8MQ and compatible SoCs do not use the > "interrupt-names" property and specify the IRQs in the wrong order: > > - 1st IRQ: queue1 > - 2nd IRQ: queue2 > - 3rd IRQ: queue0 + other IRQs > - 4th IRQ: pps > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back > in the correct order, this is done in fec_probe(). > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2 is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM and its compatible SoCs, but i.MX8QM uses a different solution. I don't know why there are two different ways of doing it, as I don't know the history. But you can refer to the solution of i.MX8QM, which I think is more suitable. See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is placed first. fec1: ethernet@5b040000 { reg = <0x5b040000 0x10000>; interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>; > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/net/ethernet/freescale/fec.h | 24 ++++++++++++++++++++++-- > drivers/net/ethernet/freescale/fec_main.c | 18 +++++++++++------- > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec.h > b/drivers/net/ethernet/freescale/fec.h > index > 63744a86752540fcede7fc4c29865b2529492526..b0f1a3e28d5c8052be3a8a0 > fa18303a1df2bb5bd 100644 > --- a/drivers/net/ethernet/freescale/fec.h > +++ b/drivers/net/ethernet/freescale/fec.h > @@ -504,8 +504,28 @@ struct bufdesc_ex { > */ > #define FEC_QUIRK_DELAYED_CLKS_SUPPORT (1 << 21) > > -/* i.MX8MQ SoC integration mix wakeup interrupt signal into "int2" interrupt > line. */ > -#define FEC_QUIRK_WAKEUP_FROM_INT2 (1 << 22) > +/* With i.MX8MQ and compatible SoCs, the order of the IRQs in the > + * device tree is not optimal. The driver expects the first three IRQs > + * to match their corresponding queue, while the last (fourth) IRQ is > + * used for the PPS: > + * > + * - 1st IRQ: "int0": queue0 + other IRQs > + * - 2nd IRQ: "int1": queue1 > + * - 3rd IRQ: "int2": queue2 > + * - 4th IRQ: "pps": pps > + * > + * However, the i.MX8MQ and compatible SoCs do not use the > + * "interrupt-names" property and specify the IRQs in the wrong order: > + * > + * - 1st IRQ: queue1 > + * - 2nd IRQ: queue2 > + * - 3rd IRQ: queue0 + other IRQs > + * - 4th IRQ: pps > + * > + * If the following quirk is active, put the IRQs back in the correct > + * order, this is done in fec_probe(). > + */ > +#define FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ BIT(22) > > /* i.MX6Q adds pm_qos support */ > #define FEC_QUIRK_HAS_PMQOS BIT(23) > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index > d948ed9810027d5fabe521dc3af2cf505dacd13e..f124ffe3619d82dc089f8494 > d33d2398e6f631fb 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -157,7 +157,7 @@ static const struct fec_devinfo fec_imx8mq_info = { > FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE | > FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE | > FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES > | > - FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 | > + FEC_QUIRK_HAS_EEE | FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ | > FEC_QUIRK_HAS_MDIO_C45, > }; > > @@ -4260,10 +4260,7 @@ static void fec_enet_get_wakeup_irq(struct > platform_device *pdev) > struct net_device *ndev = platform_get_drvdata(pdev); > struct fec_enet_private *fep = netdev_priv(ndev); > > - if (fep->quirks & FEC_QUIRK_WAKEUP_FROM_INT2) > - fep->wake_irq = fep->irq[2]; > - else > - fep->wake_irq = fep->irq[0]; > + fep->wake_irq = fep->irq[0]; > } > > static int fec_enet_init_stop_mode(struct fec_enet_private *fep, > @@ -4495,10 +4492,17 @@ fec_probe(struct platform_device *pdev) > goto failed_init; > > for (i = 0; i < irq_cnt; i++) { > - snprintf(irq_name, sizeof(irq_name), "int%d", i); > + int irq_num; > + > + if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ) > + irq_num = (i + irq_cnt - 1) % irq_cnt; > + else > + irq_num = i; > + > + snprintf(irq_name, sizeof(irq_name), "int%d", irq_num); > irq = platform_get_irq_byname_optional(pdev, irq_name); > if (irq < 0) > - irq = platform_get_irq(pdev, i); > + irq = platform_get_irq(pdev, irq_num); > if (irq < 0) { > ret = irq; > goto failed_irq; > > -- > 2.45.2 >
On 16.10.2024 22:15:43, Frank Li wrote: > On Wed, Oct 16, 2024 at 11:51:55PM +0200, Marc Kleine-Budde wrote: > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > > tree is not optimal. The driver expects the first three IRQs to match > > their corresponding queue, while the last (fourth) IRQ is used for the > > PPS: > > > > - 1st IRQ: "int0": queue0 + other IRQs > > - 2nd IRQ: "int1": queue1 > > - 3rd IRQ: "int2": queue2 > > - 4th IRQ: "pps": pps > > > > However, the i.MX8MQ and compatible SoCs do not use the > > "interrupt-names" property and specify the IRQs in the wrong order: > > > > - 1st IRQ: queue1 > > - 2nd IRQ: queue2 > > - 3rd IRQ: queue0 + other IRQs > > - 4th IRQ: pps > > why not fix dts? Because of compatibility. You could update the fec driver and try to detect if the IRQs in the DTS are in the "correct" order, but the new DTS will be incompatible with the old driver. I'm working on a patch series that updates the fec driver to "per queue IRQ and NAPI" handling. For this approach it's crucial that the IRQs match the queue number. regards, Marc
On 17.10.2024 03:09:15, Wei Fang wrote: > > -----Original Message----- > > From: Marc Kleine-Budde <mkl@pengutronix.de> > > Sent: 2024年10月17日 5:52 > > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>; > > Clark Wang <xiaoning.wang@nxp.com>; David S. Miller > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard > > Cochran <richardcochran@gmail.com> > > Cc: imx@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > > kernel@pengutronix.de; Marc Kleine-Budde <mkl@pengutronix.de> > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring IRQs > > in correct order > > > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > > tree is not optimal. The driver expects the first three IRQs to match > > their corresponding queue, while the last (fourth) IRQ is used for the > > PPS: > > > > - 1st IRQ: "int0": queue0 + other IRQs > > - 2nd IRQ: "int1": queue1 > > - 3rd IRQ: "int2": queue2 > > - 4th IRQ: "pps": pps > > > > However, the i.MX8MQ and compatible SoCs do not use the > > "interrupt-names" property and specify the IRQs in the wrong order: > > > > - 1st IRQ: queue1 > > - 2nd IRQ: queue2 > > - 3rd IRQ: queue0 + other IRQs > > - 4th IRQ: pps > > > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. > > > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back > > in the correct order, this is done in fec_probe(). > > > > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2 > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM and > its compatible SoCs, but i.MX8QM uses a different solution. I don't know why > there are two different ways of doing it, as I don't know the history. But you can > refer to the solution of i.MX8QM, which I think is more suitable. > > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is > placed first. 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. > fec1: ethernet@5b040000 { > reg = <0x5b040000 0x10000>; > interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>; regards, Marc
> > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring > IRQs > > > in correct order > > > > > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > > > tree is not optimal. The driver expects the first three IRQs to match > > > their corresponding queue, while the last (fourth) IRQ is used for the > > > PPS: > > > > > > - 1st IRQ: "int0": queue0 + other IRQs > > > - 2nd IRQ: "int1": queue1 > > > - 3rd IRQ: "int2": queue2 > > > - 4th IRQ: "pps": pps > > > > > > However, the i.MX8MQ and compatible SoCs do not use the > > > "interrupt-names" property and specify the IRQs in the wrong order: > > > > > > - 1st IRQ: queue1 > > > - 2nd IRQ: queue2 > > > - 3rd IRQ: queue0 + other IRQs > > > - 4th IRQ: pps > > > > > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to > > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. > > > > > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back > > > in the correct order, this is done in fec_probe(). > > > > > > > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2 > > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM > and > > its compatible SoCs, but i.MX8QM uses a different solution. I don't know > why > > there are two different ways of doing it, as I don't know the history. But you > can > > refer to the solution of i.MX8QM, which I think is more suitable. > > > > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is > > placed first. > > 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. > > fec1: ethernet@5b040000 { > > reg = <0x5b040000 0x10000>; > > interrupts = <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH>; >
On 17.10.2024 07:43:55, Wei Fang wrote: > > > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring > > IRQs > > > > in correct order > > > > > > > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > > > > tree is not optimal. The driver expects the first three IRQs to match > > > > their corresponding queue, while the last (fourth) IRQ is used for the > > > > PPS: > > > > > > > > - 1st IRQ: "int0": queue0 + other IRQs > > > > - 2nd IRQ: "int1": queue1 > > > > - 3rd IRQ: "int2": queue2 > > > > - 4th IRQ: "pps": pps > > > > > > > > However, the i.MX8MQ and compatible SoCs do not use the > > > > "interrupt-names" property and specify the IRQs in the wrong order: > > > > > > > > - 1st IRQ: queue1 > > > > - 2nd IRQ: queue2 > > > > - 3rd IRQ: queue0 + other IRQs > > > > - 4th IRQ: pps > > > > > > > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to > > > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. > > > > > > > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back > > > > in the correct order, this is done in fec_probe(). > > > > > > > > > > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2 > > > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM > > and > > > its compatible SoCs, but i.MX8QM uses a different solution. I don't know > > why > > > there are two different ways of doing it, as I don't know the history. But you > > can > > > refer to the solution of i.MX8QM, which I think is more suitable. > > > > > > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is > > > placed first. > > > > 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. regards, Marc
On Thu, Oct 17, 2024 at 12:21:10PM +0200, Marc Kleine-Budde wrote: > On 17.10.2024 07:43:55, Wei Fang wrote: > > > > > Subject: [PATCH net-next 07/13] net: fec: fec_probe(): update quirk: bring > > > IRQs > > > > > in correct order > > > > > > > > > > With i.MX8MQ and compatible SoCs, the order of the IRQs in the device > > > > > tree is not optimal. The driver expects the first three IRQs to match > > > > > their corresponding queue, while the last (fourth) IRQ is used for the > > > > > PPS: > > > > > > > > > > - 1st IRQ: "int0": queue0 + other IRQs > > > > > - 2nd IRQ: "int1": queue1 > > > > > - 3rd IRQ: "int2": queue2 > > > > > - 4th IRQ: "pps": pps > > > > > > > > > > However, the i.MX8MQ and compatible SoCs do not use the > > > > > "interrupt-names" property and specify the IRQs in the wrong order: > > > > > > > > > > - 1st IRQ: queue1 > > > > > - 2nd IRQ: queue2 > > > > > - 3rd IRQ: queue0 + other IRQs > > > > > - 4th IRQ: pps > > > > > > > > > > First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to > > > > > FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. > > > > > > > > > > If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back > > > > > in the correct order, this is done in fec_probe(). > > > > > > > > > > > > > I think FEC_QUIRK_INT2_IS_MAIN_IRQ or FEC_QUIRK_WAKEUP_FROM_INT2 > > > > is *NO* needed anymore. Actually, INT2 is also the main IRQ for i.MX8QM > > > and > > > > its compatible SoCs, but i.MX8QM uses a different solution. I don't know > > > why > > > > there are two different ways of doing it, as I don't know the history. But you > > > can > > > > refer to the solution of i.MX8QM, which I think is more suitable. > > > > > > > > See arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi, the IRQ 258 is > > > > placed first. > > > > > > 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. Frank > regards, > 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 |
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. Marc
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 |
On 17.10.2024 11:30:45, Frank Li wrote: > 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. ACK > > 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. I'm sorry, but this is not 100% correct. Changing the _order_ of IRQs does break compatibility. New DT (with changed IRQ order) with old driver breaks wakeup functionality. Have a look at b7cdc9658ac8 ("net: fec: add WoL support for i.MX8MQ"), but keep in mind the patch description is not 100% correct: | By default FEC driver treat irq[0] (i.e. int0 described in dt-binding) | as wakeup interrupt, but this situation changed on i.MX8M serials, SoC | integration guys mix wakeup interrupt signal into int2 interrupt line. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This statement is wrong. The SoC integration is correct, the DT is wrong. | This patch introduces FEC_QUIRK_WAKEUP_FROM_INT2 to indicate int2 as | wakeup interrupt for i.MX8MQ. > "pre-equeue" irq is new features. You can enable this feature only > when "interrupt-names" exist in future. regards, Marc
On Fri, Oct 18, 2024 at 11:16:46AM +0200, Marc Kleine-Budde wrote: > On 17.10.2024 11:30:45, Frank Li wrote: > > 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. > > ACK > > > > > 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. > > I'm sorry, but this is not 100% correct. Changing the _order_ of IRQs > does break compatibility. New DT (with changed IRQ order) with old > driver breaks wakeup functionality. > > Have a look at b7cdc9658ac8 ("net: fec: add WoL support for i.MX8MQ"), > but keep in mind the patch description is not 100% correct: > > | By default FEC driver treat irq[0] (i.e. int0 described in dt-binding) > | as wakeup interrupt, but this situation changed on i.MX8M serials, SoC > | integration guys mix wakeup interrupt signal into int2 interrupt line. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > This statement is wrong. The SoC integration is correct, the DT is > wrong. We should fix wrong thing instead of continuing on the wrong path. No much user use wakeup funcationlity. Actually you enable both int0 and int2 as wakeup source if try to keep compatility. for example: fec->wake_irq = fep->irq[0]; if (FEC_QUIRK_WAKEUP_FROM_INT2) fec->wake_irq2 = fep->irq[2]; ... if (fep->wake_irq2 > 0) { disable_irq(fep->wake_irq2); enable_irq_wake(fep->wake_irq2); } I perfer fix dts and remove FEC_QUIRK_WAKEUP_FROM_INT2. > > | This patch introduces FEC_QUIRK_WAKEUP_FROM_INT2 to indicate int2 as > | wakeup interrupt for i.MX8MQ. > > > "pre-equeue" irq is new features. You can enable this feature only > > when "interrupt-names" exist in future. > > regards, > 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 |
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index 63744a86752540fcede7fc4c29865b2529492526..b0f1a3e28d5c8052be3a8a0fa18303a1df2bb5bd 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -504,8 +504,28 @@ struct bufdesc_ex { */ #define FEC_QUIRK_DELAYED_CLKS_SUPPORT (1 << 21) -/* i.MX8MQ SoC integration mix wakeup interrupt signal into "int2" interrupt line. */ -#define FEC_QUIRK_WAKEUP_FROM_INT2 (1 << 22) +/* With i.MX8MQ and compatible SoCs, the order of the IRQs in the + * device tree is not optimal. The driver expects the first three IRQs + * to match their corresponding queue, while the last (fourth) IRQ is + * used for the PPS: + * + * - 1st IRQ: "int0": queue0 + other IRQs + * - 2nd IRQ: "int1": queue1 + * - 3rd IRQ: "int2": queue2 + * - 4th IRQ: "pps": pps + * + * However, the i.MX8MQ and compatible SoCs do not use the + * "interrupt-names" property and specify the IRQs in the wrong order: + * + * - 1st IRQ: queue1 + * - 2nd IRQ: queue2 + * - 3rd IRQ: queue0 + other IRQs + * - 4th IRQ: pps + * + * If the following quirk is active, put the IRQs back in the correct + * order, this is done in fec_probe(). + */ +#define FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ BIT(22) /* i.MX6Q adds pm_qos support */ #define FEC_QUIRK_HAS_PMQOS BIT(23) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index d948ed9810027d5fabe521dc3af2cf505dacd13e..f124ffe3619d82dc089f8494d33d2398e6f631fb 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -157,7 +157,7 @@ static const struct fec_devinfo fec_imx8mq_info = { FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE | FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES | - FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 | + FEC_QUIRK_HAS_EEE | FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ | FEC_QUIRK_HAS_MDIO_C45, }; @@ -4260,10 +4260,7 @@ static void fec_enet_get_wakeup_irq(struct platform_device *pdev) struct net_device *ndev = platform_get_drvdata(pdev); struct fec_enet_private *fep = netdev_priv(ndev); - if (fep->quirks & FEC_QUIRK_WAKEUP_FROM_INT2) - fep->wake_irq = fep->irq[2]; - else - fep->wake_irq = fep->irq[0]; + fep->wake_irq = fep->irq[0]; } static int fec_enet_init_stop_mode(struct fec_enet_private *fep, @@ -4495,10 +4492,17 @@ fec_probe(struct platform_device *pdev) goto failed_init; for (i = 0; i < irq_cnt; i++) { - snprintf(irq_name, sizeof(irq_name), "int%d", i); + int irq_num; + + if (fep->quirks & FEC_QUIRK_DT_IRQ2_IS_MAIN_IRQ) + irq_num = (i + irq_cnt - 1) % irq_cnt; + else + irq_num = i; + + snprintf(irq_name, sizeof(irq_name), "int%d", irq_num); irq = platform_get_irq_byname_optional(pdev, irq_name); if (irq < 0) - irq = platform_get_irq(pdev, i); + irq = platform_get_irq(pdev, irq_num); if (irq < 0) { ret = irq; goto failed_irq;
With i.MX8MQ and compatible SoCs, the order of the IRQs in the device tree is not optimal. The driver expects the first three IRQs to match their corresponding queue, while the last (fourth) IRQ is used for the PPS: - 1st IRQ: "int0": queue0 + other IRQs - 2nd IRQ: "int1": queue1 - 3rd IRQ: "int2": queue2 - 4th IRQ: "pps": pps However, the i.MX8MQ and compatible SoCs do not use the "interrupt-names" property and specify the IRQs in the wrong order: - 1st IRQ: queue1 - 2nd IRQ: queue2 - 3rd IRQ: queue0 + other IRQs - 4th IRQ: pps First rename the quirk from FEC_QUIRK_WAKEUP_FROM_INT2 to FEC_QUIRK_INT2_IS_MAIN_IRQ, to better reflect it's functionality. If the FEC_QUIRK_INT2_IS_MAIN_IRQ quirk is active, put the IRQs back in the correct order, this is done in fec_probe(). Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> --- drivers/net/ethernet/freescale/fec.h | 24 ++++++++++++++++++++++-- drivers/net/ethernet/freescale/fec_main.c | 18 +++++++++++------- 2 files changed, 33 insertions(+), 9 deletions(-)