[v4] rework IRQ handling in mtk_eth_soc

[net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely

Posted by Frank Wunderlich 3 months, 3 weeks ago

From: Frank Wunderlich <frank-w@public-files.de>

On SoCs without MTK_SHARED_INT capability (mt7621 + mt7628) the first
IRQ (eth->irq[0]) was read but never used. Do not read it and reduce
the IRQ-count to 2 because of skipped index 0.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
v4:
- drop >2 condition as max is already 2 and drop the else continue
- update comment to explain which IRQs are taken in legacy way
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 20 ++++++++++++++++----
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  4 ++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 3ecb399dcf81..f3fcbb00822c 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -3341,16 +3341,28 @@ static int mtk_get_irqs(struct platform_device *pdev, struct mtk_eth *eth)
 {
 	int i;
 
+	/* future SoCs beginning with MT7988 should use named IRQs in dts */
 	eth->irq[MTK_ETH_IRQ_TX] = platform_get_irq_byname(pdev, "tx");
 	eth->irq[MTK_ETH_IRQ_RX] = platform_get_irq_byname(pdev, "rx");
 	if (eth->irq[MTK_ETH_IRQ_TX] >= 0 && eth->irq[MTK_ETH_IRQ_RX] >= 0)
 		return 0;
 
+	/* legacy way:
+	 * On MTK_SHARED_INT SoCs (MT7621 + MT7628) the first IRQ is taken from
+	 * devicetree and used for rx+tx.
+	 * On SoCs with non-shared IRQ the first was not used, second entry is
+	 * TX and third is RX.
+	 */
+
 	for (i = 0; i < MTK_ETH_IRQ_MAX; i++) {
-		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT) && i > 0)
-			eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
-		else
-			eth->irq[i] = platform_get_irq(pdev, i);
+		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT)) {
+			if (i == 0)
+				eth->irq[MTK_ETH_IRQ_SHARED] = platform_get_irq(pdev, i);
+			else
+				eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
+		} else {
+			eth->irq[i] = platform_get_irq(pdev, i + 1);
+		}
 
 		if (eth->irq[i] < 0) {
 			dev_err(&pdev->dev, "no IRQ%d resource found\n", i);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 96b724dca0e2..9d91fe721ad0 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -643,8 +643,8 @@
 #define MTK_MAC_FSM(x)		(0x1010C + ((x) * 0x100))
 
 #define MTK_ETH_IRQ_SHARED	0
-#define MTK_ETH_IRQ_TX		1
-#define MTK_ETH_IRQ_RX		2
+#define MTK_ETH_IRQ_TX		0
+#define MTK_ETH_IRQ_RX		1
 #define MTK_ETH_IRQ_MAX		(MTK_ETH_IRQ_RX + 1)
 
 struct mtk_rx_dma {
-- 
2.43.0

Re: [net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely

Posted by Simon Horman 3 months, 3 weeks ago

On Mon, Jun 16, 2025 at 10:07:36AM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> On SoCs without MTK_SHARED_INT capability (mt7621 + mt7628) the first
> IRQ (eth->irq[0]) was read but never used. Do not read it and reduce
> the IRQ-count to 2 because of skipped index 0.

Describing the first IRQ as read seems a bit confusing to me - do we read
it? And saying get or got seems hard to parse. So perhaps something like
this would be clearer?

... platform_get_irq() is called for the first IRQ (eth->irq[0]) but
it is never used.

> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v4:
> - drop >2 condition as max is already 2 and drop the else continue
> - update comment to explain which IRQs are taken in legacy way
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 20 ++++++++++++++++----
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  4 ++--
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 3ecb399dcf81..f3fcbb00822c 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -3341,16 +3341,28 @@ static int mtk_get_irqs(struct platform_device *pdev, struct mtk_eth *eth)
>  {
>  	int i;
>  
> +	/* future SoCs beginning with MT7988 should use named IRQs in dts */

Perhaps this comment belongs in the patch that adds support for named IRQs.

>  	eth->irq[MTK_ETH_IRQ_TX] = platform_get_irq_byname(pdev, "tx");
>  	eth->irq[MTK_ETH_IRQ_RX] = platform_get_irq_byname(pdev, "rx");
>  	if (eth->irq[MTK_ETH_IRQ_TX] >= 0 && eth->irq[MTK_ETH_IRQ_RX] >= 0)
>  		return 0;
>  
> +	/* legacy way:
> +	 * On MTK_SHARED_INT SoCs (MT7621 + MT7628) the first IRQ is taken from
> +	 * devicetree and used for rx+tx.
> +	 * On SoCs with non-shared IRQ the first was not used, second entry is
> +	 * TX and third is RX.

Maybe I am slow. But I had a bit of trouble parsing this.
Perhaps this is clearer?

        * devicetree and used for both RX and TX - it is shared.
	* On SoCs with non-shared IRQs the first entry is not used,
        * the second is for TX, and the third is for RX.

> +	 */
> +
>  	for (i = 0; i < MTK_ETH_IRQ_MAX; i++) {
> -		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT) && i > 0)
> -			eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
> -		else
> -			eth->irq[i] = platform_get_irq(pdev, i);
> +		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT)) {
> +			if (i == 0)
> +				eth->irq[MTK_ETH_IRQ_SHARED] = platform_get_irq(pdev, i);
> +			else
> +				eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
> +		} else {
> +			eth->irq[i] = platform_get_irq(pdev, i + 1);
> +		}
>  
>  		if (eth->irq[i] < 0) {
>  			dev_err(&pdev->dev, "no IRQ%d resource found\n", i);

...

Aw: Re: [net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely

Posted by Frank Wunderlich 3 months, 3 weeks ago

Hi Simon,

thanks for your review

> Gesendet: Mittwoch, 18. Juni 2025 um 10:35
> Von: "Simon Horman" <horms@kernel.org>
> Betreff: Re: [net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely
>
> On Mon, Jun 16, 2025 at 10:07:36AM +0200, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w@public-files.de>
> > 
> > On SoCs without MTK_SHARED_INT capability (mt7621 + mt7628) the first
> > IRQ (eth->irq[0]) was read but never used. Do not read it and reduce
> > the IRQ-count to 2 because of skipped index 0.
> 
> Describing the first IRQ as read seems a bit confusing to me - do we read
> it? And saying get or got seems hard to parse. So perhaps something like
> this would be clearer?
> 
> ... platform_get_irq() is called for the first IRQ (eth->irq[0]) but
> it is never used.

ok, i change it in next version

> > 
> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > ---
> > v4:
> > - drop >2 condition as max is already 2 and drop the else continue
> > - update comment to explain which IRQs are taken in legacy way
> > ---
> >  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 20 ++++++++++++++++----
> >  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  4 ++--
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > index 3ecb399dcf81..f3fcbb00822c 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > @@ -3341,16 +3341,28 @@ static int mtk_get_irqs(struct platform_device *pdev, struct mtk_eth *eth)
> >  {
> >  	int i;
> >  
> > +	/* future SoCs beginning with MT7988 should use named IRQs in dts */
> 
> Perhaps this comment belongs in the patch that adds support for named IRQs.

also thought that after sending it :)

> >  	eth->irq[MTK_ETH_IRQ_TX] = platform_get_irq_byname(pdev, "tx");
> >  	eth->irq[MTK_ETH_IRQ_RX] = platform_get_irq_byname(pdev, "rx");
> >  	if (eth->irq[MTK_ETH_IRQ_TX] >= 0 && eth->irq[MTK_ETH_IRQ_RX] >= 0)
> >  		return 0;
> >  
> > +	/* legacy way:
> > +	 * On MTK_SHARED_INT SoCs (MT7621 + MT7628) the first IRQ is taken from
> > +	 * devicetree and used for rx+tx.
> > +	 * On SoCs with non-shared IRQ the first was not used, second entry is
> > +	 * TX and third is RX.
> 
> Maybe I am slow. But I had a bit of trouble parsing this.
> Perhaps this is clearer?
> 
>         * devicetree and used for both RX and TX - it is shared.
> 	* On SoCs with non-shared IRQs the first entry is not used,
>         * the second is for TX, and the third is for RX.

I would also move this comment in first patch with your changes requested.

/* legacy way:
 * On MTK_SHARED_INT SoCs (MT7621 + MT7628) the first IRQ is taken from
 * devicetree and used for both RX and TX - it is shared.
 * On SoCs with non-shared IRQs the first entry is not used,
 * the second is for TX, and the third is for RX.
 */

i can keep your RB there?

> > +	 */
> > +
> >  	for (i = 0; i < MTK_ETH_IRQ_MAX; i++) {
> > -		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT) && i > 0)
> > -			eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
> > -		else
> > -			eth->irq[i] = platform_get_irq(pdev, i);
> > +		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT)) {
> > +			if (i == 0)
> > +				eth->irq[MTK_ETH_IRQ_SHARED] = platform_get_irq(pdev, i);
> > +			else
> > +				eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
> > +		} else {
> > +			eth->irq[i] = platform_get_irq(pdev, i + 1);
> > +		}
> >  
> >  		if (eth->irq[i] < 0) {
> >  			dev_err(&pdev->dev, "no IRQ%d resource found\n", i);

code changes are OK?

regards Frank

Re: Re: [net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely

Posted by Simon Horman 3 months, 3 weeks ago

On Wed, Jun 18, 2025 at 09:14:47AM +0000, Frank Wunderlich wrote:
> Hi Simon,
> 
> thanks for your review
> 
> > Gesendet: Mittwoch, 18. Juni 2025 um 10:35
> > Von: "Simon Horman" <horms@kernel.org>
> > Betreff: Re: [net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely
> >
> > On Mon, Jun 16, 2025 at 10:07:36AM +0200, Frank Wunderlich wrote:
> > > From: Frank Wunderlich <frank-w@public-files.de>
> > > 
> > > On SoCs without MTK_SHARED_INT capability (mt7621 + mt7628) the first
> > > IRQ (eth->irq[0]) was read but never used. Do not read it and reduce
> > > the IRQ-count to 2 because of skipped index 0.
> > 
> > Describing the first IRQ as read seems a bit confusing to me - do we read
> > it? And saying get or got seems hard to parse. So perhaps something like
> > this would be clearer?
> > 
> > ... platform_get_irq() is called for the first IRQ (eth->irq[0]) but
> > it is never used.
> 
> ok, i change it in next version
> 
> > > 
> > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > > ---
> > > v4:
> > > - drop >2 condition as max is already 2 and drop the else continue
> > > - update comment to explain which IRQs are taken in legacy way
> > > ---
> > >  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 20 ++++++++++++++++----
> > >  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  4 ++--
> > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > index 3ecb399dcf81..f3fcbb00822c 100644
> > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > @@ -3341,16 +3341,28 @@ static int mtk_get_irqs(struct platform_device *pdev, struct mtk_eth *eth)
> > >  {
> > >  	int i;
> > >  
> > > +	/* future SoCs beginning with MT7988 should use named IRQs in dts */
> > 
> > Perhaps this comment belongs in the patch that adds support for named IRQs.
> 
> also thought that after sending it :)
> 
> > >  	eth->irq[MTK_ETH_IRQ_TX] = platform_get_irq_byname(pdev, "tx");
> > >  	eth->irq[MTK_ETH_IRQ_RX] = platform_get_irq_byname(pdev, "rx");
> > >  	if (eth->irq[MTK_ETH_IRQ_TX] >= 0 && eth->irq[MTK_ETH_IRQ_RX] >= 0)
> > >  		return 0;
> > >  
> > > +	/* legacy way:
> > > +	 * On MTK_SHARED_INT SoCs (MT7621 + MT7628) the first IRQ is taken from
> > > +	 * devicetree and used for rx+tx.
> > > +	 * On SoCs with non-shared IRQ the first was not used, second entry is
> > > +	 * TX and third is RX.
> > 
> > Maybe I am slow. But I had a bit of trouble parsing this.
> > Perhaps this is clearer?
> > 
> >         * devicetree and used for both RX and TX - it is shared.
> > 	* On SoCs with non-shared IRQs the first entry is not used,
> >         * the second is for TX, and the third is for RX.
> 
> I would also move this comment in first patch with your changes requested.

Yes, good point.

> 
> /* legacy way:
>  * On MTK_SHARED_INT SoCs (MT7621 + MT7628) the first IRQ is taken from
>  * devicetree and used for both RX and TX - it is shared.
>  * On SoCs with non-shared IRQs the first entry is not used,
>  * the second is for TX, and the third is for RX.
>  */
> 
> i can keep your RB there?

Yes, thanks for asking.

> 
> > > +	 */
> > > +
> > >  	for (i = 0; i < MTK_ETH_IRQ_MAX; i++) {
> > > -		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT) && i > 0)
> > > -			eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
> > > -		else
> > > -			eth->irq[i] = platform_get_irq(pdev, i);
> > > +		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT)) {
> > > +			if (i == 0)
> > > +				eth->irq[MTK_ETH_IRQ_SHARED] = platform_get_irq(pdev, i);
> > > +			else
> > > +				eth->irq[i] = eth->irq[MTK_ETH_IRQ_SHARED];
> > > +		} else {
> > > +			eth->irq[i] = platform_get_irq(pdev, i + 1);
> > > +		}
> > >  
> > >  		if (eth->irq[i] < 0) {
> > >  			dev_err(&pdev->dev, "no IRQ%d resource found\n", i);
> 
> code changes are OK?

Yes. I did try to think of a more succinct way to express the logic.
But I think what you have is good.

In any case, let me review the next revision of this patch.
That will be easier for me than imagining what it looks like
with the non-code changes discussed above in place.

[net-next v4 1/3] net: ethernet: mtk_eth_soc: support named IRQs
[net-next v4 2/3] net: ethernet: mtk_eth_soc: add consts for irq index
[net-next v4 3/3] net: ethernet: mtk_eth_soc: change code to skip first IRQ completely