Re: [net-next v1] net: ethernet: mtk_eth_soc: support named IRQs

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4227 bytes --]

On Jun 14, Frank Wunderlich wrote:
> Am 14. Juni 2025 09:48:58 MESZ schrieb Lorenzo Bianconi <lorenzo@kernel.org>:
> >> From: Frank Wunderlich <frank-w@public-files.de>
> >> 
> >> Add named interrupts and keep index based fallback for exiting devicetrees.
> >> 
> >> Currently only rx and tx IRQs are defined to be used with mt7988, but
> >> later extended with RSS/LRO support.
> >> 
> >> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> >> ---
> >>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 24 +++++++++++++--------
> >>  1 file changed, 15 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> >> index b76d35069887..fcec5f95685e 100644
> >> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> >> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> >> @@ -5106,17 +5106,23 @@ static int mtk_probe(struct platform_device *pdev)
> >>  		}
> >>  	}
> >>  
> >> -	for (i = 0; i < 3; i++) {
> >> -		if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT) && i > 0)
> >> -			eth->irq[i] = eth->irq[0];
> >> -		else
> >> -			eth->irq[i] = platform_get_irq(pdev, i);
> >> -		if (eth->irq[i] < 0) {
> >> -			dev_err(&pdev->dev, "no IRQ%d resource found\n", i);
> >> -			err = -ENXIO;
> >> -			goto err_wed_exit;
> >> +	eth->irq[1] = platform_get_irq_byname(pdev, "tx");
> >> +	eth->irq[2] = platform_get_irq_byname(pdev, "rx");
> >
> >Hi Frank,
> >
> >doing so you are not setting eth->irq[0] for MT7988 devices but it is actually
> >used in mtk_add_mac() even for non-MTK_SHARED_INT devices. I guess we can reduce
> >the eth->irq array size to 2 and start from 0 even for the MT7988 case.
> >What do you think?
> 
> Hi Lorenzo,
> 
> Thank you for reviewing my patch
> 
> I had to leave flow compatible with this:
> 
> <https://github.com/frank-w/BPI-Router-Linux/blob/bd7e1983b9f0a69cf47cc9b9631138910d6c1d72/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L5176>

I guess the best would be to start from 0 even here (and wherever it is
necessary) and avoid reading current irq[0] since it is not actually used for
!shared_int devices (e.g. MT7988).  Agree?

> 
> Here the irqs are taken from index 1 and 2 for
>  registration (!shared_int else only 0). So i avoided changing the
>  index,but yes index 0 is unset at this time.
> 
> I guess the irq0 is not really used here...
> I tested the code on bpi-r4 and have traffic
>  rx+tx and no crash.
>  imho this field is not used on !shared_int
>  because other irq-handlers are used and
>  assigned in position above.

agree. I have not reviewed the code in detail, but this is why
I think we can avoid reading it.

> 
> It looks like the irq[0] is read before...there is a
>  message printed for mediatek frame engine
>  which uses index 0 and shows an irq 102 on
>  index way and 0 on named version...but the
>  102 in index way is not visible in /proc/interrupts.
> So imho this message is misleading.
> 
> Intention for this patch is that irq 0 and 3 on
>  mt7988 (sdk) are reserved (0 is skipped on 
> !shared_int and 3 never read) and should imho
>  not listed in devicetree. For further cleaner
>  devicetrees (with only needed irqs) and to
>  extend additional irqs for rss/lro imho irq
>  names make it better readable.

Same here, if you are not listing them in the device tree, you can remove them
in the driver too (and adjust the code to keep the backward compatibility).

Regards,
Lorenzo

> 
> >Regards,
> >Lorenzo
> >
> >> +	if (eth->irq[1] < 0 || eth->irq[2] < 0) {
> >> +		for (i = 0; i < 3; i++) {
> >> +			if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT) && i > 0)
> >> +				eth->irq[i] = eth->irq[0];
> >> +			else
> >> +				eth->irq[i] = platform_get_irq(pdev, i);
> >> +
> >> +			if (eth->irq[i] < 0) {
> >> +				dev_err(&pdev->dev, "no IRQ%d resource found\n", i);
> >> +				err = -ENXIO;
> >> +				goto err_wed_exit;
> >> +			}
> >>  		}
> >>  	}
> >> +
> >>  	for (i = 0; i < ARRAY_SIZE(eth->clks); i++) {
> >>  		eth->clks[i] = devm_clk_get(eth->dev,
> >>  					    mtk_clks_source_name[i]);
> >> -- 
> >> 2.43.0
> >> 
> 
> 
> regards Frank

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]