* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-10 2:21 ` Joakim Zhang
0 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-10 2:21 UTC (permalink / raw)
To: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
Hi Florian,
> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: 2021年8月10日 2:40
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> 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@gmail.com>
> >> Sent: 2021年8月5日 17:18
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> >> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> >> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> >> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> >> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >> 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@nxp.com>
> >>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >>
> >> 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?
If there is any common properties can be used for it, please let me know. Or any other better solutions also be appreciated. Thanks.
Best Regards,
Joakim Zhang
> --
> Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-10 2:21 ` Joakim Zhang
@ 2021-08-10 14:32 ` Andrew Lunn
-1 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2021-08-10 14:32 UTC (permalink / raw)
To: Joakim Zhang
Cc: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
> > > 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.
So you need to know which of the interrupts listed is the wake up
interrupt.
I can see a few ways to do this:
The FEC driver already has quirks. Add a quirk to fec_imx8mq_info and
fec_imx8qm_info to indicate these should use int2.
or
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
b) two cells
------------
The #interrupt-cells property is set to 2 and the first cell defines the
index of the interrupt within the controller, while the second cell is used
to specify any of the following flags:
- bits[3:0] trigger type and level flags
1 = low-to-high edge triggered
2 = high-to-low edge triggered
4 = active high level-sensitive
8 = active low level-sensitive
You could add
18 = wakeup source
and extend to core to either do all the work for you, or tell you this
interrupt is flagged as being a wakeup source. This solution has the
advantage of it should be usable in other drivers.
Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-10 14:32 ` Andrew Lunn
0 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2021-08-10 14:32 UTC (permalink / raw)
To: Joakim Zhang
Cc: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
> > > 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.
So you need to know which of the interrupts listed is the wake up
interrupt.
I can see a few ways to do this:
The FEC driver already has quirks. Add a quirk to fec_imx8mq_info and
fec_imx8qm_info to indicate these should use int2.
or
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
b) two cells
------------
The #interrupt-cells property is set to 2 and the first cell defines the
index of the interrupt within the controller, while the second cell is used
to specify any of the following flags:
- bits[3:0] trigger type and level flags
1 = low-to-high edge triggered
2 = high-to-low edge triggered
4 = active high level-sensitive
8 = active low level-sensitive
You could add
18 = wakeup source
and extend to core to either do all the work for you, or tell you this
interrupt is flagged as being a wakeup source. This solution has the
advantage of it should be usable in other drivers.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-10 14:32 ` Andrew Lunn
@ 2021-08-11 7:57 ` Joakim Zhang
-1 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-11 7:57 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2021年8月10日 22:33
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl,
> wakeup-irq" property
>
> > > > 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.
>
> So you need to know which of the interrupts listed is the wake up interrupt.
>
> I can see a few ways to do this:
>
> The FEC driver already has quirks. Add a quirk to fec_imx8mq_info and
> fec_imx8qm_info to indicate these should use int2.
>
> or
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>
> b) two cells
> ------------
> The #interrupt-cells property is set to 2 and the first cell defines the
> index of the interrupt within the controller, while the second cell is used
> to specify any of the following flags:
> - bits[3:0] trigger type and level flags
> 1 = low-to-high edge triggered
> 2 = high-to-low edge triggered
> 4 = active high level-sensitive
> 8 = active low level-sensitive
>
> You could add
>
> 18 = wakeup source
>
> and extend to core to either do all the work for you, or tell you this interrupt is
> flagged as being a wakeup source. This solution has the advantage of it should
> be usable in other drivers.
Thanks a lot for your comments first!
I just look into the irq code, if we extend bit[5] to carry wakeup info ( due to bit[4] is used for IRQ_TYPE_PROBE),
then configure it in the TYPE field of 'interrupts' property, so that interrupt controller would know which interrupt
is wakeup capable.
I think there is no much work core would do, may just set this interrupt wakup capable. Another functionality is
driver side get this info to identify which mixed interrupt has wakeup capability, we can export symbol from kernel/irq/irqdomain.c.
The intention is to let driver know which interrupt is wakeup capable, I would choose to provider this in specific driver,
instead of interrupt controller, it seems to me that others may all choose this solution for wakeup mixed interrupt.
So I would prefer solution 1, it's easier and under-control. I can have a try if you strongly recommend solution 2.
Best Regards,
Joakim Zhang
> Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-11 7:57 ` Joakim Zhang
0 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-11 7:57 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2021年8月10日 22:33
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl,
> wakeup-irq" property
>
> > > > 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.
>
> So you need to know which of the interrupts listed is the wake up interrupt.
>
> I can see a few ways to do this:
>
> The FEC driver already has quirks. Add a quirk to fec_imx8mq_info and
> fec_imx8qm_info to indicate these should use int2.
>
> or
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>
> b) two cells
> ------------
> The #interrupt-cells property is set to 2 and the first cell defines the
> index of the interrupt within the controller, while the second cell is used
> to specify any of the following flags:
> - bits[3:0] trigger type and level flags
> 1 = low-to-high edge triggered
> 2 = high-to-low edge triggered
> 4 = active high level-sensitive
> 8 = active low level-sensitive
>
> You could add
>
> 18 = wakeup source
>
> and extend to core to either do all the work for you, or tell you this interrupt is
> flagged as being a wakeup source. This solution has the advantage of it should
> be usable in other drivers.
Thanks a lot for your comments first!
I just look into the irq code, if we extend bit[5] to carry wakeup info ( due to bit[4] is used for IRQ_TYPE_PROBE),
then configure it in the TYPE field of 'interrupts' property, so that interrupt controller would know which interrupt
is wakeup capable.
I think there is no much work core would do, may just set this interrupt wakup capable. Another functionality is
driver side get this info to identify which mixed interrupt has wakeup capability, we can export symbol from kernel/irq/irqdomain.c.
The intention is to let driver know which interrupt is wakeup capable, I would choose to provider this in specific driver,
instead of interrupt controller, it seems to me that others may all choose this solution for wakeup mixed interrupt.
So I would prefer solution 1, it's easier and under-control. I can have a try if you strongly recommend solution 2.
Best Regards,
Joakim Zhang
> Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-11 7:57 ` Joakim Zhang
@ 2021-08-11 16:16 ` Andrew Lunn
-1 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2021-08-11 16:16 UTC (permalink / raw)
To: Joakim Zhang
Cc: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
> So I would prefer solution 1, it's easier and under-control.
Hi Joakim
Using quirks is fine by me.
Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-11 16:16 ` Andrew Lunn
0 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2021-08-11 16:16 UTC (permalink / raw)
To: Joakim Zhang
Cc: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
> So I would prefer solution 1, it's easier and under-control.
Hi Joakim
Using quirks is fine by me.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-11 7:57 ` Joakim Zhang
@ 2021-08-13 18:22 ` Rob Herring
-1 siblings, 0 replies; 32+ messages in thread
From: Rob Herring @ 2021-08-13 18:22 UTC (permalink / raw)
To: Joakim Zhang
Cc: Andrew Lunn, Florian Fainelli, davem, kuba, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
On Wed, Aug 11, 2021 at 07:57:46AM +0000, Joakim Zhang wrote:
>
> Hi Andrew,
>
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: 2021年8月10日 22:33
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> > kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl,
> > wakeup-irq" property
> >
> > > > > 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.
> >
> > So you need to know which of the interrupts listed is the wake up interrupt.
We already have a way to do this by using 'wakeup' for the
interrupt-names entry. But I guess that ship has sailed here and that
wouldn't work well if not just a wakeup source (though you could repeat
an interrupt line that's the wakeup source).
> >
> > I can see a few ways to do this:
> >
> > The FEC driver already has quirks. Add a quirk to fec_imx8mq_info and
> > fec_imx8qm_info to indicate these should use int2.
Bingo!
Note that if the device is wakeup capable, it should have a
'wakeup-source' property in this case.
> >
> > or
> >
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >
> > b) two cells
> > ------------
> > The #interrupt-cells property is set to 2 and the first cell defines the
> > index of the interrupt within the controller, while the second cell is used
> > to specify any of the following flags:
> > - bits[3:0] trigger type and level flags
> > 1 = low-to-high edge triggered
> > 2 = high-to-low edge triggered
> > 4 = active high level-sensitive
> > 8 = active low level-sensitive
> >
> > You could add
> >
> > 18 = wakeup source
I'd be okay with this (though it should be a power of 2 number).
> >
> > and extend to core to either do all the work for you, or tell you this interrupt is
> > flagged as being a wakeup source. This solution has the advantage of it should
> > be usable in other drivers.
Another option is couldn't you just enable all the interrupts as wakeup
sources? Presumably, only one of them would trigger a wakeup.
>
> Thanks a lot for your comments first!
>
> I just look into the irq code, if we extend bit[5] to carry wakeup info ( due to bit[4] is used for IRQ_TYPE_PROBE),
> then configure it in the TYPE field of 'interrupts' property, so that interrupt controller would know which interrupt
> is wakeup capable.
> I think there is no much work core would do, may just set this interrupt wakup capable. Another functionality is
> driver side get this info to identify which mixed interrupt has wakeup capability, we can export symbol from kernel/irq/irqdomain.c.
>
> The intention is to let driver know which interrupt is wakeup capable, I would choose to provider this in specific driver,
> instead of interrupt controller, it seems to me that others may all choose this solution for wakeup mixed interrupt.
>
> So I would prefer solution 1, it's easier and under-control. I can have a try if you strongly recommend solution 2.
>
> Best Regards,
> Joakim Zhang
> > Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-13 18:22 ` Rob Herring
0 siblings, 0 replies; 32+ messages in thread
From: Rob Herring @ 2021-08-13 18:22 UTC (permalink / raw)
To: Joakim Zhang
Cc: Andrew Lunn, Florian Fainelli, davem, kuba, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
On Wed, Aug 11, 2021 at 07:57:46AM +0000, Joakim Zhang wrote:
>
> Hi Andrew,
>
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: 2021年8月10日 22:33
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> > kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl,
> > wakeup-irq" property
> >
> > > > > 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.
> >
> > So you need to know which of the interrupts listed is the wake up interrupt.
We already have a way to do this by using 'wakeup' for the
interrupt-names entry. But I guess that ship has sailed here and that
wouldn't work well if not just a wakeup source (though you could repeat
an interrupt line that's the wakeup source).
> >
> > I can see a few ways to do this:
> >
> > The FEC driver already has quirks. Add a quirk to fec_imx8mq_info and
> > fec_imx8qm_info to indicate these should use int2.
Bingo!
Note that if the device is wakeup capable, it should have a
'wakeup-source' property in this case.
> >
> > or
> >
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >
> > b) two cells
> > ------------
> > The #interrupt-cells property is set to 2 and the first cell defines the
> > index of the interrupt within the controller, while the second cell is used
> > to specify any of the following flags:
> > - bits[3:0] trigger type and level flags
> > 1 = low-to-high edge triggered
> > 2 = high-to-low edge triggered
> > 4 = active high level-sensitive
> > 8 = active low level-sensitive
> >
> > You could add
> >
> > 18 = wakeup source
I'd be okay with this (though it should be a power of 2 number).
> >
> > and extend to core to either do all the work for you, or tell you this interrupt is
> > flagged as being a wakeup source. This solution has the advantage of it should
> > be usable in other drivers.
Another option is couldn't you just enable all the interrupts as wakeup
sources? Presumably, only one of them would trigger a wakeup.
>
> Thanks a lot for your comments first!
>
> I just look into the irq code, if we extend bit[5] to carry wakeup info ( due to bit[4] is used for IRQ_TYPE_PROBE),
> then configure it in the TYPE field of 'interrupts' property, so that interrupt controller would know which interrupt
> is wakeup capable.
> I think there is no much work core would do, may just set this interrupt wakup capable. Another functionality is
> driver side get this info to identify which mixed interrupt has wakeup capability, we can export symbol from kernel/irq/irqdomain.c.
>
> The intention is to let driver know which interrupt is wakeup capable, I would choose to provider this in specific driver,
> instead of interrupt controller, it seems to me that others may all choose this solution for wakeup mixed interrupt.
>
> So I would prefer solution 1, it's easier and under-control. I can have a try if you strongly recommend solution 2.
>
> Best Regards,
> Joakim Zhang
> > Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-13 18:22 ` Rob Herring
@ 2021-08-16 6:48 ` Joakim Zhang
-1 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-16 6:48 UTC (permalink / raw)
To: Rob Herring
Cc: Andrew Lunn, Florian Fainelli, davem, kuba, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2021年8月14日 2:23
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>;
> davem@davemloft.net; kuba@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl,
> wakeup-irq" property
>
> On Wed, Aug 11, 2021 at 07:57:46AM +0000, Joakim Zhang wrote:
> >
> > Hi Andrew,
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: 2021年8月10日 22:33
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> > > kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > > dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add
> > > "fsl, wakeup-irq" property
> > >
> > > > > > 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.
> > >
> > > So you need to know which of the interrupts listed is the wake up interrupt.
>
> We already have a way to do this by using 'wakeup' for the interrupt-names
> entry. But I guess that ship has sailed here and that wouldn't work well if not
> just a wakeup source (though you could repeat an interrupt line that's the
> wakeup source).
Yes, I am not sure whether it's a common solution, such as:
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>,
<GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
interrupt-names = "int0", "int1", "int2", "pps", "wakeup";
Where we repeat interrupt 120 for both "int2" and "wakeup", does you mean this?
> > >
> > > I can see a few ways to do this:
> > >
> > > The FEC driver already has quirks. Add a quirk to fec_imx8mq_info
> > > and fec_imx8qm_info to indicate these should use int2.
>
> Bingo!
>
> Note that if the device is wakeup capable, it should have a 'wakeup-source'
> property in this case.
>
> > >
> > > or
> > >
> > > Documentation/devicetree/bindings/interrupt-controller/interrupts.tx
> > > t
> > >
> > > b) two cells
> > > ------------
> > > The #interrupt-cells property is set to 2 and the first cell defines the
> > > index of the interrupt within the controller, while the second cell is used
> > > to specify any of the following flags:
> > > - bits[3:0] trigger type and level flags
> > > 1 = low-to-high edge triggered
> > > 2 = high-to-low edge triggered
> > > 4 = active high level-sensitive
> > > 8 = active low level-sensitive
> > >
> > > You could add
> > >
> > > 18 = wakeup source
>
> I'd be okay with this (though it should be a power of 2 number).
>
> > >
> > > and extend to core to either do all the work for you, or tell you
> > > this interrupt is flagged as being a wakeup source. This solution
> > > has the advantage of it should be usable in other drivers.
>
> Another option is couldn't you just enable all the interrupts as wakeup sources?
> Presumably, only one of them would trigger a wakeup.
Yes, another solution, I thought it but not implemented it as we had better let users know which
interrupt is wakeup capable.
Best Regards,
Joakim Zhang
> >
> > Thanks a lot for your comments first!
> >
> > I just look into the irq code, if we extend bit[5] to carry wakeup
> > info ( due to bit[4] is used for IRQ_TYPE_PROBE), then configure it in
> > the TYPE field of 'interrupts' property, so that interrupt controller would know
> which interrupt is wakeup capable.
> > I think there is no much work core would do, may just set this
> > interrupt wakup capable. Another functionality is driver side get this info to
> identify which mixed interrupt has wakeup capability, we can export symbol
> from kernel/irq/irqdomain.c.
> >
> > The intention is to let driver know which interrupt is wakeup capable,
> > I would choose to provider this in specific driver, instead of interrupt
> controller, it seems to me that others may all choose this solution for wakeup
> mixed interrupt.
> >
> > So I would prefer solution 1, it's easier and under-control. I can have a try if
> you strongly recommend solution 2.
> >
> > Best Regards,
> > Joakim Zhang
> > > Andrew
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-16 6:48 ` Joakim Zhang
0 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-16 6:48 UTC (permalink / raw)
To: Rob Herring
Cc: Andrew Lunn, Florian Fainelli, davem, kuba, shawnguo, s.hauer,
festevam, kernel, dl-linux-imx, netdev, devicetree, linux-kernel,
linux-arm-kernel
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2021年8月14日 2:23
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>;
> davem@davemloft.net; kuba@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl,
> wakeup-irq" property
>
> On Wed, Aug 11, 2021 at 07:57:46AM +0000, Joakim Zhang wrote:
> >
> > Hi Andrew,
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: 2021年8月10日 22:33
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > Cc: Florian Fainelli <f.fainelli@gmail.com>; davem@davemloft.net;
> > > kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > > dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add
> > > "fsl, wakeup-irq" property
> > >
> > > > > > 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.
> > >
> > > So you need to know which of the interrupts listed is the wake up interrupt.
>
> We already have a way to do this by using 'wakeup' for the interrupt-names
> entry. But I guess that ship has sailed here and that wouldn't work well if not
> just a wakeup source (though you could repeat an interrupt line that's the
> wakeup source).
Yes, I am not sure whether it's a common solution, such as:
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>,
<GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
interrupt-names = "int0", "int1", "int2", "pps", "wakeup";
Where we repeat interrupt 120 for both "int2" and "wakeup", does you mean this?
> > >
> > > I can see a few ways to do this:
> > >
> > > The FEC driver already has quirks. Add a quirk to fec_imx8mq_info
> > > and fec_imx8qm_info to indicate these should use int2.
>
> Bingo!
>
> Note that if the device is wakeup capable, it should have a 'wakeup-source'
> property in this case.
>
> > >
> > > or
> > >
> > > Documentation/devicetree/bindings/interrupt-controller/interrupts.tx
> > > t
> > >
> > > b) two cells
> > > ------------
> > > The #interrupt-cells property is set to 2 and the first cell defines the
> > > index of the interrupt within the controller, while the second cell is used
> > > to specify any of the following flags:
> > > - bits[3:0] trigger type and level flags
> > > 1 = low-to-high edge triggered
> > > 2 = high-to-low edge triggered
> > > 4 = active high level-sensitive
> > > 8 = active low level-sensitive
> > >
> > > You could add
> > >
> > > 18 = wakeup source
>
> I'd be okay with this (though it should be a power of 2 number).
>
> > >
> > > and extend to core to either do all the work for you, or tell you
> > > this interrupt is flagged as being a wakeup source. This solution
> > > has the advantage of it should be usable in other drivers.
>
> Another option is couldn't you just enable all the interrupts as wakeup sources?
> Presumably, only one of them would trigger a wakeup.
Yes, another solution, I thought it but not implemented it as we had better let users know which
interrupt is wakeup capable.
Best Regards,
Joakim Zhang
> >
> > Thanks a lot for your comments first!
> >
> > I just look into the irq code, if we extend bit[5] to carry wakeup
> > info ( due to bit[4] is used for IRQ_TYPE_PROBE), then configure it in
> > the TYPE field of 'interrupts' property, so that interrupt controller would know
> which interrupt is wakeup capable.
> > I think there is no much work core would do, may just set this
> > interrupt wakup capable. Another functionality is driver side get this info to
> identify which mixed interrupt has wakeup capability, we can export symbol
> from kernel/irq/irqdomain.c.
> >
> > The intention is to let driver know which interrupt is wakeup capable,
> > I would choose to provider this in specific driver, instead of interrupt
> controller, it seems to me that others may all choose this solution for wakeup
> mixed interrupt.
> >
> > So I would prefer solution 1, it's easier and under-control. I can have a try if
> you strongly recommend solution 2.
> >
> > Best Regards,
> > Joakim Zhang
> > > Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-10 2:21 ` Joakim Zhang
@ 2021-08-10 17:45 ` Florian Fainelli
-1 siblings, 0 replies; 32+ messages in thread
From: Florian Fainelli @ 2021-08-10 17:45 UTC (permalink / raw)
To: Joakim Zhang, davem, kuba, robh+dt, shawnguo, s.hauer, festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
On 8/9/2021 7:21 PM, Joakim Zhang wrote:
>
> Hi Florian,
>
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: 2021年8月10日 2:40
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> 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@gmail.com>
>>>> Sent: 2021年8月5日 17:18
>>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>> 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@nxp.com>
>>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>>>
>>>> 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.
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.
>
> 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.
--
Florian
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-10 17:45 ` Florian Fainelli
0 siblings, 0 replies; 32+ messages in thread
From: Florian Fainelli @ 2021-08-10 17:45 UTC (permalink / raw)
To: Joakim Zhang, davem, kuba, robh+dt, shawnguo, s.hauer, festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
On 8/9/2021 7:21 PM, Joakim Zhang wrote:
>
> Hi Florian,
>
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: 2021年8月10日 2:40
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> 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@gmail.com>
>>>> Sent: 2021年8月5日 17:18
>>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>> 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@nxp.com>
>>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>>>
>>>> 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.
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.
>
> 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.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-10 17:45 ` Florian Fainelli
@ 2021-08-11 8:06 ` Joakim Zhang
-1 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-11 8:06 UTC (permalink / raw)
To: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: 2021年8月11日 1:45
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> 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@gmail.com>
> >> Sent: 2021年8月10日 2:40
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> >> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> >> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> >> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> >> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >> 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@gmail.com>
> >>>> Sent: 2021年8月5日 17:18
> >>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> >>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> >>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> >>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> >>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >>>> 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@nxp.com>
> >>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >>>>
> >>>> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-11 8:06 ` Joakim Zhang
0 siblings, 0 replies; 32+ messages in thread
From: Joakim Zhang @ 2021-08-11 8:06 UTC (permalink / raw)
To: Florian Fainelli, davem, kuba, robh+dt, shawnguo, s.hauer,
festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: 2021年8月11日 1:45
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> 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@gmail.com>
> >> Sent: 2021年8月10日 2:40
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> >> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> >> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> >> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> >> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >> 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@gmail.com>
> >>>> Sent: 2021年8月5日 17:18
> >>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
> >>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
> >>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
> >>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> >>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> >>>> 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@nxp.com>
> >>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >>>>
> >>>> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
2021-08-11 8:06 ` Joakim Zhang
@ 2021-08-12 9:46 ` Florian Fainelli
-1 siblings, 0 replies; 32+ messages in thread
From: Florian Fainelli @ 2021-08-12 9:46 UTC (permalink / raw)
To: Joakim Zhang, davem, kuba, robh+dt, shawnguo, s.hauer, festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
On 8/11/2021 1:06 AM, Joakim Zhang wrote:
>
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: 2021年8月11日 1:45
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> 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@gmail.com>
>>>> Sent: 2021年8月10日 2:40
>>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>> 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@gmail.com>
>>>>>> Sent: 2021年8月5日 17:18
>>>>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>>>>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>>>>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>>>>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>>>>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>>>> 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@nxp.com>
>>>>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>>>>>
>>>>>> 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.
If enable_irq_wake() returns -ENOTSUPP you would know that wake-up for
that interrupt controller's line is not capable of wake-up?
>
>> 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?
What I mean is that if you need to express which interrupt lines within
an interrupt controller are capable of wake-up, then there should be a
property that tells us that and that property needs to be within the
interrupt controller, not within the consumer of that interrupt since
the consumer has no idea how the system is wired up. Andrew's suggestion
is sort of the same thing, except that it punts the responsibility of
specifying the interrupt's capability towards the consumer of that
interrupt.
--
Florian
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property
@ 2021-08-12 9:46 ` Florian Fainelli
0 siblings, 0 replies; 32+ messages in thread
From: Florian Fainelli @ 2021-08-12 9:46 UTC (permalink / raw)
To: Joakim Zhang, davem, kuba, robh+dt, shawnguo, s.hauer, festevam, andrew
Cc: kernel, dl-linux-imx, netdev, devicetree, linux-kernel, linux-arm-kernel
On 8/11/2021 1:06 AM, Joakim Zhang wrote:
>
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: 2021年8月11日 1:45
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> 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@gmail.com>
>>>> Sent: 2021年8月10日 2:40
>>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>> 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@gmail.com>
>>>>>> Sent: 2021年8月5日 17:18
>>>>>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; davem@davemloft.net;
>>>>>> kuba@kernel.org; robh+dt@kernel.org; shawnguo@kernel.org;
>>>>>> s.hauer@pengutronix.de; festevam@gmail.com; andrew@lunn.ch
>>>>>> Cc: kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
>>>>>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>>>>> 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@nxp.com>
>>>>>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>>>>>>
>>>>>> 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.
If enable_irq_wake() returns -ENOTSUPP you would know that wake-up for
that interrupt controller's line is not capable of wake-up?
>
>> 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?
What I mean is that if you need to express which interrupt lines within
an interrupt controller are capable of wake-up, then there should be a
property that tells us that and that property needs to be within the
interrupt controller, not within the consumer of that interrupt since
the consumer has no idea how the system is wired up. Andrew's suggestion
is sort of the same thing, except that it punts the responsibility of
specifying the interrupt's capability towards the consumer of that
interrupt.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread