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,

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

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

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

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.

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

quoted

	  Andrew