Message-ID: <87ttosqvbq.ffs@tglx>
Date:   Fri, 08 Dec 2023 21:43:21 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Ben Wolsieffer <ben.wolsieffer@...ring.com>,
        linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org
Cc:     Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Ben Wolsieffer <ben.wolsieffer@...ring.com>
Subject: Re: [PATCH 2/2] pinctrl: stm32: fix GPIO level interrupts

Ben!

On Mon, Dec 04 2023 at 15:33, Ben Wolsieffer wrote:
> The STM32 doesn't support GPIO level interrupts in hardware, so the
> driver tries to emulate them using edge interrupts, by retriggering the
> interrupt if necessary based on the pin state after the handler
> finishes.
>
> Currently, this functionality does not work because the irqchip uses
> handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask()
> callbacks after handling the interrupt. This patch fixes this by using
> handle_level_irq() for level interrupts, which causes irq_unmask() to be
> called to retrigger the interrupt.

This does not make any sense at all. irq_unmask() does not retrigger
anything. It sets the corresponding bit in the mask register, not more
not less.

Switching to handle_level_irq() makes the following difference
vs. handle_edge_irq() when an interrupt is handled (ignoring the inner
loop):

      + irq_mask();
        irq_ack();
        ....
        handle();
        ....
      + irq_unmask();

So in both cases irq_ack() clears the interrupt in the Pending register,
right?

Now comes the interesting difference.

When the interrupt is raised again after irq_ack() while the handler is
running, i.e. a full toggle from active to inactive and back to active
where the back to active transition causes the edge detector to trigger,
then:

  1) in case of handle_edge_irq() this should immediately set it in the
     pending register again and raise another CPU interrupt, which
     should be handled once the interrupt service routine returned.

  2) in case of handle_level_irq() this does not set it in the pending
     register because it's masked. The unmask will set the pending
     register bit _if_ and only _if_ the edge detector has latched the
     detection. No idea whether that's the case. The manual is
     exceptionally blury about this.

So in theory both #1 and #2 should work. But the explanation in the
changelog is fairy tale material.

As I couldn't figure out why #1 would not work, I looked at the driver
in more detail and also at the STM32 manual. That tells me that the
irqchip driver is at least suboptimal. Why?

The EXTI controller is just an intermediate between the peripheral
(including GPIO pins) and the NVIC:

         |--------------|
         | Edge config  |   |-----------------|
 Source -|              |---| Int. Mask logic |---> Dedicated NVIC interrupt
         | Edge detect  | | |-----------------|
         |--------------| |
                          | |-----------------|   
                          |-| Evt. Mask logic |---> CPU event input
                          | |-----------------|   
                          |
                          | |-----------------|   
                          |-| Wakeup logic    |--->....
                            |-----------------|   

So there are two classes of sources conntect to EXTI:

   1) Direct events

      - Have a fixed edge
      - Can be masked for Interrupt and Event generation
      - No software trigger
      - Not tracked in the Pending register
      - Can evtl. wakeup the CPUs or from D3

   2) Configurable events

      - Have a configurable edge
      - Can be masked for Interrupt and Event generation
      - Software trigger
      - Tracked in the Pending register
      - Can evtl. wakeup the CPUs or from D3

The CPU event is a single input to the CPU which can be triggered by any
source which has the Event mask enabled.

For both classes there are sources which have no connection to the NVIC,
they can only be used to generate CPU events or trigger the wakeup
logic.

For direct events there is a category where the peripherial interrupt is
routed to both the EXTI and the NVIC directly. The EXTI does not provide
a connection to the NVIC and the event cannot be masked in EXTI to
prevent CPU interrupts. Only the CPU event masking works.

GPIO pins are configurable events which are connected to the NVIC via
the EXTI.

But the EXTI driver implements a chained interrupt handler which listens
on a ton of NVIC interrupts. I.e. for the STM32H7 on:

  <1>, <2>, <3>, <6>, <7>, <8>, <9>, <10>, <23>, <40>, <41>, <62>, <76>

NVIC   1: PVD_PVM           EXTI-SRC 16
NVIC   2: RTC_TAMP_STAMP    EXTI-SRC 18
NVIC   3: RTC_WAKEUP        EXTI-SRC 19
NVIC   6: EXTI0             EXTI-SRC  0
NVIC   7: EXTI1             EXTI-SRC  1
NVIC   8: EXTI2             EXTI-SRC  2
NVIC   9: EXTI3             EXTI-SRC  3
NVIC  10: EXTI4             EXTI-SRC  4
NVIC  23: EXTI5-9           EXTI-SRC  5-9
NVIC  40: EXTI10-15         EXTI-SRC 10-15
NVIC  41: RTC_ALARM         EXTI-SRC 17
NVIC  62: ETH_WKUP          EXTI-SRC 86
NVIC  76: OTG_HS_WKUP       EXTI-SRC 43

Each of these chained interrupts handles the full EXTI interrupt domain
with all three banks. This does not make any sense at all especially not
on a SMP machine.

Though it _should_ work, but it might cause interrupts handlers to be
invoked when nothing is pending when the edge handler is active. Which
in turn can confuse the underlying device driver depending on the
quality...

CPU0					CPU1

NVIC int X                      	NVIC int Y

 // read_pending() is serialized by a lock, but both read the same state
 pend = read_pending()			pend = read_pending()
 
 for_each_bit(bit, pend)	 	for_each_bit(bit, pend)	
    handle_irq(domain, base + bit)         handle_irq(domain, base + bit)
      lock(desc);                            lock(desc);
      ack();
      do {
         clear(PENDING);
         set(IN_PROGRESS);
         unlock(desc);
         handle();                           if (IN_PROGRESS) {
         lock(desc);                           ack();
                                               set(PENDING);
         				       unlock(desc);
         clear(IN_PROGRESS);                   return;
                                             }
      } while (PENDING); <- Will loop

See?

In fact the only NVIC interrupts which actually need demultiplexing are
NVIC #23 and NVIC #40 and those should only care about the EXTI
interrupts which are actually multiplexed on them. This let's randomly
run whatever is pending on any demux handler is far from correct.

All others are direct NVIC interrupts which just have the extra EXTI
interrupt masking, event masking and the wakeup magic. The indirection
via the chained handler is just pointless overhead and not necessarily
correct.

The exti_h variant of that driver does the right thing and installs a
hierarchical interrupt domain which acts as man in the middle between
the source and the NVIC. Though granted they don't have the odd problem
of multiplexing several GPIO interrupts to a single NVIC interrupt.

But in fact the regular exti driver could do the same and just handle
the two NVIC interrupts which need demultiplexing separately and let
everything else go through the hierarchy without bells and whistles.

Thanks,

        tglx

Powered by blists - more mailing lists