Re: [PATCH 6/6] gpio: macsmc: Add IRQ support

[Linus Walleij <linus.walleij@linaro.org>]

On Mon, Sep 5, 2022 at 2:47 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Fri, Sep 02, 2022 at 03:21:31PM +0200, Linus Walleij wrote:

> > > +       local_irq_save(flags);
> > > +       ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset);
> > > +       local_irq_restore(flags);
> >
> > Isn't irq_bus_lock/unlock protecting us here already?
> > (I might be getting it wrong...)
>
> Hmm, where does irq_bus_lock get called? Given this function is called
> while running a blocking notifier chain, interrupts will not be
> disabled on entry to this function. I haven't found a place in the maze
> of irq handling code that generic_handle_domain_irq() would end up using
> the bus lock/unlock functions - and if they did, with the above IRQ
> saving, the kernel would WARN() about calling mutex_lock() with IRQs
> disabled. So it doesn't.

Ah I get it now, the notification mechanism goes entirely orthogonal
to the irqchip, that's what got me confused. You're right, keep this.

> > That callback will only be used by edge triggered IRQs but
> > I guess that would realistically be all we support anyway?
> > (See comment below on .set_type)
>
> I would imagine it depends on how the SMC GPIO interrupt works -
> whether the ACK is ACK as we know it in Linux (x86 PIC) or whether
> it's ACK as in a notification to the SMC that we have finished
> handling the interrupt and it can send us the next interrupt.
>
> I suspect we don't know that level of detail of the platform, so
> given that this is what the Asahi kernel does, that's the best we
> have.

OK

> > > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > > +{
> > > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > > +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
> > > +       int offset = irqd_to_hwirq(d);
> > > +       u32 mode;
> > > +
> > > +       if (!test_bit(offset, smcgp->irq_supported))
> > > +               return -EINVAL;
> > > +
> > > +       switch (type & IRQ_TYPE_SENSE_MASK) {
> > > +       case IRQ_TYPE_LEVEL_HIGH:
> > > +               mode = IRQ_MODE_HIGH;
> > > +               break;
> > > +       case IRQ_TYPE_LEVEL_LOW:
> > > +               mode = IRQ_MODE_LOW;
> > > +               break;
> > > +       case IRQ_TYPE_EDGE_RISING:
> > > +               mode = IRQ_MODE_RISING;
> > > +               break;
> > > +       case IRQ_TYPE_EDGE_FALLING:
> > > +               mode = IRQ_MODE_FALLING;
> > > +               break;
> > > +       case IRQ_TYPE_EDGE_BOTH:
> > > +               mode = IRQ_MODE_BOTH;
> > > +               break;
> > > +       default:
> > > +               return -EINVAL;
> >
> > I don't know how level IRQs would work on this essentially
> > message-passing process context interrupt. Maybe I am getting
> > it all wrong, but for level the line should be held low/high until
> > the IRQ is serviced, it would be possible to test if this actually
> > works by *not* servicing an IRQ and see if the SMC then sends
> > another message notifier for the same IRQ.
>
> If level IRQs are not supported, then it's strange that the Asahi
> folk have been able to reverse engineer the CMD_IRQ_MODE codes for
> these states.
>
> Maybe the SMC issues another message for a level IRQ after it receives
> a CMD_IRQ_ACK message if the level interrupt is still asserted?
(...)
> > > +       gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip);
> > > +       smcgp->gc.irq.parent_handler = NULL;
> > > +       smcgp->gc.irq.num_parents = 0;
> > > +       smcgp->gc.irq.parents = NULL;
> > > +       smcgp->gc.irq.default_type = IRQ_TYPE_NONE;
> > > +       smcgp->gc.irq.handler = handle_simple_irq;
> >
> > I would consider setting this to handle_edge_irq() and implement
> > .irq_ack(). I might be wrong.
>
> I don't think that's suitable, because then we'll be calling irq_ack()
> before the handler has run - and we won't be notifying the SMC that
> the interrupt has been masked. So it could send another notification
> for the same IRQ while it's still being handled. Then there's the
> question about level IRQs as well.
>
> I think, given that I don't know how the SMC works (presumably the
> Asahi folk have a bit more of an idea, but that will be based on
> reverse engineering effort) that I am not going to modify this driver's
> behaviour drastically by changing the flow handler to the edge flow
> handler from the simple flow. To me, that could well be a disaster
> for this driver. That would be something for the Asahi folk to look
> at.

Fair enough. From my end this will be fine to merge after you considered
the things brought up and it is certainly not necessary to have any
"perfect" solution, to me it is clear that what we need to do is enable the
target so that people can use it and then we/Asahi can comb through it
and reexamine things like this once the whole system is usable as a whole.

I've seen that Konrad has even started using the M1 infrastructure
to kickstart a few iPhone/iPad devices, so given how much hardware this
is (in absolute units) I think it's pretty important we get to usable ASAP.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel