On 2020-05-18 18:57, Daniel Mack wrote:
Hi Maarten,
On 5/18/20 1:14 PM, Maarten Brock wrote:
On 2020-05-17 22:44, Daniel Mack wrote:
Summerizing:
- After switching to a threaded IRQ, the trigger could be switched to
IRQF_TRIGGER_LOW and with that interrupt sharing can be enabled for
this device with IRQF_SHARED.
Yes, but we don't need that. As discussed, the UART driver can cope
with
edge IRQs just fine.
- Some (your) interrupt controllers do not support IRQF_TRIGGER_LOW.
For those only IRQF_TRIGGER_FALLING can be used for this device and
thus IRQF_SHARED cannot be used.
True. Interrupts cannot be shared for this device then. That's a fair
limitation, and it has always been like that.
It has always been like that for this driver. But that should be no
reason why the driver might not be improved. I wonder how the 8250
handles this. PC's have always shared interrupts for COM1/2/3/4 AFAIK.
- The driver for your interrupt controller should be improved to
support
level IRQs.
It's a controller that sits behind another hardware bus itself, so
polling is expensive. If the controller would need to check for level
IRQs it would need to poll, and then we could as well just poll the
UART
directly, that's just as good :)
That depends on the IRQ coming out of the interrupt controller. If that
is
a level interrupt itself, then it is easy to see if all interrupts are
handled. Further polling zooms in on the devices that require attention.
But again - the UART driver works perfectly fine with edge IRQs as long
as the interrupt is not shared.
If you would require multiple sc16is7xx devices on I2C would you like to
connect multiple interrupt lines? Or just SCL,SDA and *one* IRQ?
OTOH for SPI you would require multiple CS already.
This makes me wonder if it would be better to let the device tree
specify
the interrupt configuration.
There can be flags in the 2nd cell of the node, but their meaning is
specific to the controller. Hence the SPI/I2C layers don't pass that
information up.
What many drivers do is try with one setting, and if that fails because
the interrupt controller returns an error, they fall back to something
else. We could do the same here of course, but it'd be another patch on
top, as it's unrelated to the concrete change the patch we're
commenting
on is bringing in.
So what I can add is logic that first tries with IRQF_LOW|IRQF_SHARED,
and if that fails, we fall back to IRQF_FALLING and retry. WDYT?
That sounds like a decent plan.
Thanks,
Daniel
Kind regards,
Maarten
