> Am 2025-06-15 10:57, schrieb Lorenzo Bianconi: > > > From: Frank Wunderlich > > > > > > 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 > > > Reviewed-by: Simon Horman > > > > Hi Frank, > > > > I guess my comments on v1 apply even in v2. Can you please take a look? sure > > adding your comments (and mine as context) from v1 here: > > Am 2025-06-15 10:57, schrieb Lorenzo Bianconi: > > > From: Frank Wunderlich > > > > I had to leave flow compatible with this: > > > > > > > > > > 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. > > i areee, but imho it should be a separate patch because these are 2 > different changes I am fine to have it in a separate patch but I would prefer to have this patch in the same series, I think it is more clear. > > > > 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). > > afaik i have no SHARED_INT board (only mt7621, mt7628) so changing the > index-logic will require testing on such boards too. I think the change will not heavily impact SHARED_INT devices. Regards, Lorenzo > > i looked a bit into it and see mt7623 and mt7622 have 3 IRQs defined > (!SHARED_INT) and i'm not 100% sure if the first is also skipped (as far as > i understood code it should always be skipped). > > In the end i would change the irq-index part in separate patch once this is > accepted to have clean changes and not mixing index with names (at least to > allow a revert of second in case of regression). > > Am 2025-06-15 11:26, schrieb Daniel Golle: > > In addition to Lorenzo's comment to reduce the array to the actually > > used > > IRQs, I think it would be nice to introduce precompiler macros for the > > irq > > array index, ie. once the array is reduce to size 2 it could be > > something > > like > > > > #define MTK_ETH_IRQ_SHARED 0 > > #define MTK_ETH_IRQ_TX 0 > > #define MTK_ETH_IRQ_RX 1 > > #define __MTK_ETH_IRQ_MAX MTK_ETH_IRQ_RX > > > > That would make all the IRQ code more readable than having to deal with > > numerical values. > > makes sense, i will take this into the second patch. > > I hope you can agree my thoughts about not mixing these 2 parts :) > > regards Frank