> -----Original Message----- > From: Florian Fainelli <f.fainelli@xxxxxxxxx> > Sent: 2021年8月11日 1:45 > To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; davem@xxxxxxxxxxxxx; > kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; > s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; andrew@xxxxxxx > Cc: kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, > wakeup-irq" property > > > > On 8/9/2021 7:21 PM, Joakim Zhang wrote: > > > > Hi Florian, > > > >> -----Original Message----- > >> From: Florian Fainelli <f.fainelli@xxxxxxxxx> > >> Sent: 2021年8月10日 2:40 > >> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; davem@xxxxxxxxxxxxx; > >> kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; > >> s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; andrew@xxxxxxx > >> Cc: kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > >> netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > >> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add > >> "fsl, wakeup-irq" property > >> > >> > >> > >> On 8/8/2021 10:08 PM, Joakim Zhang wrote: > >>> > >>> Hi Florian, > >>> > >>>> -----Original Message----- > >>>> From: Florian Fainelli <f.fainelli@xxxxxxxxx> > >>>> Sent: 2021年8月5日 17:18 > >>>> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; davem@xxxxxxxxxxxxx; > >>>> kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; > >>>> s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; andrew@xxxxxxx > >>>> Cc: kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > >>>> netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > >>>> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >>>> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add > >>>> "fsl, wakeup-irq" property > >>>> > >>>> > >>>> > >>>> On 8/5/2021 12:46 AM, Joakim Zhang wrote: > >>>>> Add "fsl,wakeup-irq" property for FEC controller to select wakeup > >>>>> irq source. > >>>>> > >>>>> Signed-off-by: Fugang Duan <fugang.duan@xxxxxxx> > >>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@xxxxxxx> > >>>> > >>>> Why are not you making use of the standard interrupts-extended > >>>> property which allows different interrupt lines to be originating > >>>> from different interrupt controllers, e.g.: > >>>> > >>>> interrupts-extended = <&gic GIC_SPI 112 4>, <&wakeup_intc 0>; > >>> > >>> Thanks. > >>> > >>> AFAIK, interrupts-extended should be used instead of interrupts when > >>> a device is connected to multiple interrupt controllers as it > >>> encodes a parent phandle with each interrupt specifier. However, for > >>> FEC controller, all > >> interrupt lines are originating from the same interrupt controllers. > >> > >> OK, so why this custom property then? > >> > >>> > >>> 1) FEC controller has up to 4 interrupt lines and all of these are > >>> routed to GIC > >> interrupt controller. > >>> 2) FEC has a wakeup interrupt signal and always are mixed with other > >> interrupt signals, and then output to one interrupt line. > >>> 3) For legacy SoCs, wakeup interrupt are mixed to int0 line, but for > >>> i.MX8M > >> serials, are mixed to int2 line. > >>> 4) Now driver treat int0 as the wakeup source by default, it is > >>> broken for > >> i.MX8M. > >> > >> I don't really know what to make of your response, it seems to me > >> that you are carrying some legacy Device Tree properties that were > >> invented when interrupts-extended did not exist and we did not know any > better. > > > > As I described in former mail, it is not related to interrupts-extended > property. > > > > Let's take a look, e.g. > > > > 1) arch/arm/boot/dts/imx7d.dtsi > > interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-names = "int0", "int1", "int2", "pps"; > > > > For these 4 interrupts are originating from GIC interrupt controller, > > "int0" for queue 0 and other interrupt signals, containing wakeup; "int1" for > queue 1; "int2" for queue 2. > > > > 2) arch/arm64/boot/dts/freescale/imx8mq.dtsi > > interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-names = "int0", "int1", "int2", "pps"; > > > > For these 4 interrupts are also originating from GIC interrupt > > controller, "int0" for queue 0; "int1" for queue 1; "int2" for queue 2 and other > interrupt signals, containing wakeup. > > > > If we want to use WoL feature, we need invoke enable_irq_wake() to let > > this specific interrupt line be a wakeup source. For FEC driver now, > > it treats "int0" as wakeup interrupt by default. Obviously it's not fine for > i.MX8M serials, since SoC mix wakeup interrupt signal into "int2", so I add this > "fsl,wakeup-irq" custom property to indicate which interrupt line contains > wakeup signal. > > > > Not sure if I have explained it clearly enough, from my point of view, I think > interrupts-extended property can't fix this issue, right? > > This is clearer, and indeed interrupts-extended won't fix that, however it seems > to me that this is a problem that ought to be fixed at the interrupt > controller/irq_chip level which should know and be told which interrupt lines > can be made wake-up interrupts or not. From there on, the driver can test with > enable_irq_wake() whether this has a chance of working or not. How can we test with enable_irq_wake()? I agree that interrupt controller can recognize wakeup interrupt is better. > It seems to me that the 'fsl,wakeup-irq' property ought to be within the > interrupt controller Device Tree node (where it would be easier to validate that > the specific interrupt line is correct) as opposed to within the consumer (FEC) > Device Tree node. Not quite understand, could you explain more? > > > > If there is any common properties can be used for it, please let me know. Or > any other better solutions also be appreciated. Thanks. > > There is a standard 'wakeup-source' boolean property that can be added > to any Device Tree node to indicate it can be a wake-up source, but what > you need here is a bitmask, so introducing a custom property may be > appropriate here. Yes, I know 'wakeup-source' boolean property that identify this DEVICE can be a wakeup source, it's not related to interrupt. Best Regards, Joakim Zhang > -- > Florian
