> On Tue, 10 Nov 2015 15:36:21 +0100, Maarten Brock wrote:
> > > On Sat, 07 Nov 2015 17:32:55 +0100, Maarten Brock wrote:
> > > > > On Fri, 6 Nov 2015 16:48:20 +0100, Maarten Brock wrote:
> > > > > > Enable shared interrupts for the sc16is7xx serial driver.
> > > > > >
> > > > > > Signed-off-by: Maarten Brock <m.brock@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/tty/serial/sc16is7xx.c | 3 ++-
> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/sc16is7xx.c
> > > > > b/drivers/tty/serial/sc16is7xx.c
> > > > > > index 72ffd0d..13f5f69 100644
> > > > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > > > @@ -1230,7 +1230,8 @@ static int sc16is7xx_probe(struct device
> *dev,
> > > > > >
> > > > > > /* Setup interrupt */
> > > > > > ret = devm_request_irq(dev, irq, sc16is7xx_irq,
> > > > > > - IRQF_ONESHOT | flags, dev_name(dev), s);
> > > > > > + IRQF_SHARED | IRQF_ONESHOT | flags,
> > > > > > + dev_name(dev), s);
> > > > > > if (!ret)
> > > > > > return 0;
> > > > > >
> > > > >
> > > > > The reason why shared IRQs were not enabled is that we cannot read
> > > > > the status register from IRQ handler and therefore cannot decide if
> > > > > the interrupt was actually ours (we always return IRQ_HANDLED).
> Would
> > > > > you consider setting the IRQF_SHARED in your device tree instead of
> > > > > changing the default?
> > > >
> > > > Ok, I think I understand why it's difficult or impossible to read the
> > > > status register from the IRQ handler. But why does that pose a problem
> when
> > > > you always return IRQ_HANDLED? Your last remark seems to indicate it
> doesn't.
> > >
> > > I think it doesn't, you'll just loose the ability to detect spurious
> > > interrupts. We never mask our IRQ so you should be fine (IIUC ONESHOT
> > > flag is ignored for non-threaded IRQs).
> > >
> > > > Or would you recommend never to use shared interrupts with these
> devices?
> > >
> > > Sure, always try not to share IRQ line. Sharing IRQs wastes CPU cycles
> > > and increases the interrupt latency. I see nothing wrong with your
> > > patch per-se if it works for you, though. If you still want it
> > > included in mainline you should probably repost putting Greg KH into
> > > To: and Cc: linux-serial@xxxxxxxxxxxxxxx.
> > >
> > > > Btw. I tried to set it through the device tree, but that didn't seem
> to
> > > > work.
> > > > sc16is740: sc16is740@0 {
> > > > compatible = "nxp,sc16is740";
> > > > <snip>
> > > > interrupts = <25 0x82>; /* 0x80 for shared, 0x02 for falling
> */
> > > > }
> > >
> > > Sorry if it doesn't work, just something which came to my mind.
> >
> > I looked a bit more at the probe functions and there is nothing reading
> the
> > device tree interrupt flags. No wonder my IRQF_SHARED was ignored. Or
> should
> > this have been done by another piece of the kernel?
>
> I *think* by other part of the kernel. But
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> says only egde/level can be configured that way which makes my
> suggestion to use it for IRQF_SHARED quite nonsensical.
>
> > I don't understand this part though:
> > if (spi->dev.of_node) {
> > const struct of_device_id *of_id =
> > of_match_device(sc16is7xx_dt_ids, &spi->dev);
> >
> > devtype = (struct sc16is7xx_devtype *)of_id->data;
> > } else {
> > const struct spi_device_id *id_entry =
> spi_get_device_id(spi);
> >
> > devtype = (struct sc16is7xx_devtype
> *)id_entry->driver_data;
> > flags = IRQF_TRIGGER_FALLING;
> > }
> >
> > Why is the IRQF_TRIGGER_FALLING only set in the else clause? This is a
> > hard property of this chip, isn't it? And taking this further, isn't it
> > really a level interrupt that should use IRQF_TRIGGER_LOW? It would
> > certainly need the IRQF_ONESHOT with the actual handling being done in
> > sc16is7xx_port_irq.
>
> We want the edge.
I don't think you do. It's exactly this that breaks shared interrupts. Consider
the following scenario. There are two SC16IS7xx's sharing their interrupt.
Assume A to be handled first and B second. A character arrives at B which makes
the /IRQ_B go low. The handler for A is run and it finds no flags in IIR so it
ends. Then the handler for B is run and the character is retrieved. But in the
meantime A also receives a character and makes /IRQ_A low. The /IRQ input of the
interrupt controller never sees a second falling edge. When the handler for B is
done, /IRQ_B will go inactive but the line stays low since /IRQ_A is still
active. There is however nothing retriggering the handler of A.
So how could a level interrupt help here? When /IRQ_B goes low first handler A
and then handler B are executed as before. Due to the ONESHOT setting the
interrupt is disabled in the interrupt controller until all threaded handlers
have reenabled the interrupt. If at this point A still has an unhandled
interrupt request the line is still low and the interrupt triggered. This leads
to both handlers to be called again and the character in A retrieved.
> If I understand this code correctly (note: it's
> copied from max310x.c on which this driver was originally based so it's
> quite old code) the else branch handles non-device tree configurations.
> Kernel will read this flag from the device tree normally, but if the
> device is probed using some other mechanism, it will default to the
> setting of *_FALLING.
>
> We don't hardwire to *_FALLING always, though, since people may use
> inverters etc.
>
> > And why is the irq registered with devm_request_irq and not with
> > devm_request_threaded_irq? It looks to me like it's threaded with this
> > queue_kthread_work in sc16is7xx_irq. Am I totally misunderstanding this
> stuff?
>
> We create our own kworker for handling async configuration anyway (since
> RT-prio control on workqueues is a mess) so we just use that for irq as
> well. It saves some resources and simplifies the code a bit.
>
> > In the meantime I'm going to patch my hardware not to use shared
> > interrupts, because I have the feeling this driver is not ready for it.
> > Even though it did seem to work.
>
> The driver should work fine - but if you can - definitely try to avoid
> sharing IRQs.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
