RE: [PATCH net-next 1/3] dt-bindings: net: fsl, fec: add "fsl, wakeup-irq" property

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: 2021年8月14日 2:23
To: Joakim Zhang <redacted>
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 [off-list ref]; 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:

quoted

Hi Andrew,

quoted

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: 2021年8月10日 22:33
To: Joakim Zhang <redacted>
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 [off-list ref]; 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

quoted

quoted

quoted

1) FEC controller has up to 4 interrupt lines and all of these
are routed to GIC

interrupt controller.

quoted

2) FEC has a wakeup interrupt signal and always are mixed with
other

interrupt signals, and then output to one interrupt line.

quoted

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?

quoted

quoted

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.

quoted

quoted

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).

quoted

quoted

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

quoted

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.

quoted

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.

quoted

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.

quoted

So I would prefer solution 1, it's easier and under-control. I can have a try if

you strongly recommend solution 2.

quoted

Best Regards,
Joakim Zhang

quoted

	  Andrew