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

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

On Fri, Sep 02, 2022 at 03:21:31PM +0200, Linus Walleij wrote:
> On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
> > +       DECLARE_BITMAP(irq_enable_shadow, MAX_GPIO);
> 
> Please rename irq_unmasked_shadow as it is tracking
> this and not what the irqchip core calls enabled/disabled.
> 
> > +       DECLARE_BITMAP(irq_enable, MAX_GPIO);
> 
> I think this state should be possible to set/get from the irqchip
> core. !irqd_irq_masked(d) on the descriptor, correct me if I'm wrong.

I think you're getting the two mixed up. irq_enable_shadow
(irq_unmasked_shadow) is updated from the ->irq_mask and ->irq_unmask
callbacaks, and will track !irqd_irq_masked(d) state. So, I think we
can get rid of irq_enable_shadow and just use !irqd_irq_masked(d).

The irq_enable bit array tracks the state on the SMC, and is used to
indicate whether we need to update that state when we unlock the bus
(which is when the driver talks to the SMC to reconfigure it.)

So, I think killing irq_enable_shadow and replacing irq_enable with
irq_unmasked would be correct - and going a bit further,
irq_smc_unmasked to show that it's the SMC's status.

> > +static int macsmc_gpio_event(struct notifier_block *nb, unsigned long event, void *data)
> > +{
> > +       struct macsmc_gpio *smcgp = container_of(nb, struct macsmc_gpio, nb);
> > +       u16 type = event >> 16;
> > +       u8 offset = (event >> 8) & 0xff;
> > +       smc_key key = macsmc_gpio_key(offset);
> > +       unsigned long flags;
> > +        int ret;
> > +
> > +       if (type != SMC_EV_GPIO)
> > +               return NOTIFY_DONE;
> > +
> > +       if (offset > MAX_GPIO) {
> > +               dev_err(smcgp->dev, "GPIO event index %d out of range\n", offset);
> > +               return NOTIFY_BAD;
> > +       }
> > +
> > +       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.

This actually entirely negates any benefit of the kernel trying to mask
or unmask an interrupt in "hardware" while running the handler - since
macsmc_gpio_irq_bus_sync_unlock() won't be called, the state on the SMC
won't get touched.

> Since this is coming from a notifier and not an IRQ or threaded
> IRQ I actually am a bit puzzled on how to handle it... you probably
> know it better than me, maybe ask Marc Z if anything is
> unclear.

It's been years since I did any real platform porting work, so deep
knowledge of the IRQ subsystem has evaporated.

> > +       if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0)
> > +               dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key);
> 
> isn't this one of those cases where we should implement the
> irqchip callback .irq_ack() specifically for 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.

> > +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?

> I strongly suspect that actually only edges are supported, but
> there might be semantics I don't understand here.
> 
> >         }
> >
> > +       smcgp->irq_mode_shadow[offset] = mode;
> 
> Hm yeah I guess this shadow mode is necessary for the sync
> to work.

Ineed.

> > +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
> > +       smc_key key = macsmc_gpio_key(irqd_to_hwirq(d));
> > +       int offset = irqd_to_hwirq(d);
> > +       bool val;
> > +
> > +       if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) {
> > +               u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset];
> > +               if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0)
> > +                       dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd);
> > +               else
> > +                       smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset];
> > +       }
> > +
> > +       val = test_bit(offset, smcgp->irq_enable_shadow);
> > +       if (test_bit(offset, smcgp->irq_enable) != val) {
> 
> So what you want to know for each line is (correct me if I'm wrong):
> - Is it enabled (unmasked) or not?
> - Did it get changed enabled->disabled, disabled->enabled since
>   macsmc_gpio_irq_bus_lock()?

I think you mean here "did the 'enable' state change between
macsmc_gpio_irq_bus_lock() and macsmc_gpio_irq_bus_unlock".

> Doesn't the irqchip core track the first part of this for you?
> irqd_irq_masked(d) should tell you the same thing as
> irq_enable, just inverted.
> 
> irq_enable_shadow is a bit tricker, I guess you might need that since
> the irqchip doesn't track state changes.

Yep, which is what I've said above in this reply where these bitmaps
are declared.

> >  static int macsmc_gpio_probe(struct platform_device *pdev)
> >  {
> >         struct macsmc_gpio *smcgp;
> > @@ -221,6 +365,18 @@ static int macsmc_gpio_probe(struct platform_device *pdev)
> >         smcgp->gc.base = -1;
> >         smcgp->gc.parent = &pdev->dev;
> >
> > +       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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!