[Xenomai] Problem with gpio interrupts for xenomai3 on Intel joule

[Nitin Kulkarni <nitink@kth.se>]

Hi Philippe,
Correcting the subject of this thread and adding it to the mailing list.
Thanks for those precise code suggestions. There are two issues.

1. I found that the ipipe_irq_cascade handler is called but the problem is that
there are 5 pinctrl devices on this soc and all of them share the same irq line. Hence I see that the intel_gpio_probe()
runs for all the 5 pin controllers during kernel initialization and overwrites the irq descriptor with respective pinctrl data each time the probe runs.
When the interrupt is triggered ipipe_irq_cascade invoked with the irq desc, I always see that the pinctrl data retrieved from
irq_desc using  irq_desc_get_handler_data is always the last pinctrl. Hence I do not read the right pinctrl to call intel_gpio_irq() and read the right interrupt enable reg for the gpio pin I enabled as interrupt.

2. When I forced it to read the right pinctrl data by storing that information when intel_gpio_irq_enable is executed.
The rtdm handller from my module is executed but the ack call intel_gpio_irq_ack is not called. Hence the irq flags were never cleared and the mmc driver also starts complaining about interrupts not being received. The whole system slows down and virtually hangs.

*I tried some fixes*
-> I changed the flow handler type from handle_simple_irq to handle_edge_irq  while calling  gpiochip_irqchip_add()
  in intel_gpio_probe.
-> I called the ack function in ipipe_irq_cascade using the irq_desc which it has.
It seems to work perfectly with this fix but this is a raw and a hackish way of doing. I am not sure if this has any consequences.
Can you suggest me whats wrong with the ack not being called and how to deal with multiple pinctrl devices sharing the same irq.

Regards,
Nitin





________________________________________
From: Philippe Gerum <rpm@xenomai.org>
Sent: Thursday, June 1, 2017 9:11 AM
To: Nitin Kulkarni; xenomai@xenomai.org
Subject: Re: [Xenomai] problem gpio interrupts xenomai3 on the raspberry pi 2 (or 3)

On 05/31/2017 12:34 PM, Nitin Kulkarni wrote:
> This is the output from /proc/interrupts  (Note: irq 300 is the one I have requested )
>
>  root@intel-corei7-64:~# cat /proc/interrupts
>             CPU0       CPU1       CPU2       CPU3

[snip]

>  142:          0          0          0          0  intel-gpio   19  bq25890_irq
>  300:          0          0          0          0  intel-gpio   22  test_interrupt

[snip]

>  373:          0          0          0          0  bxtwc_irq_chip_level2    0  pmic_thermal
>  374:          0          0          0          0  bxtwc_irq_chip_level2    1  pmic_thermal
>  375:          0          0          0          0  bxtwc_irq_chip_level2    2  pmic_thermal
>  383:          1          0          0          0  Whiskey Cove    7  ACPI:Event
>  470:          1          0          0          0  bxtwc_irq_chip_level2    7  bxt_wcove_gpio
>  471:          1          0          0          0  bxtwc_irq_chip_level2    5  wcove_typec
>  472:          0          0          0          0  bxtwc_irq_chip_tmu   10  bxt_wcove_tmu

There are several IRQ chip controllers which are not I-pipe aware on this Soc. The bxtwc and the pin controller allow multiple GPIO modules to sit on the same IRQ vector (IRQF_SHARED), this is not going to work with pipelined interrupts, at least not with the current implementation of  the I-pipe patch (at some point in the future this will be the case, but we are not there yet). We can still fix the situation for a single GPIO module per IRQ line though.

This is how I would fix up the code for the pin controller driver you are interested in; not tested, not even compiled, just some hints about the way to do it. Until there are fixups for the other drivers (bxtwc abd whiskey cove), you may want to disable them, only to make sure that they won't freak out if/when receiving an interrupt.

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 0144376..f8bf54b 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -91,7 +91,11 @@ struct intel_pinctrl_context {
  */
 struct intel_pinctrl {
        struct device *dev;
+#ifdef CONFIG_IPIPE
+       ipipe_spinlock_t lock;
+#else
        raw_spinlock_t lock;
+#endif
        struct pinctrl_desc pctldesc;
        struct pinctrl_dev *pctldev;
        struct gpio_chip chip;
@@ -647,15 +651,13 @@ static const struct gpio_chip intel_gpio_chip = {
        .set = intel_gpio_set,
 };

-static void intel_gpio_irq_ack(struct irq_data *d)
+static void __intel_gpio_irq_ack(struct irq_data *d)
 {
        struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
        struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
        const struct intel_community *community;
        unsigned pin = irqd_to_hwirq(d);

-       raw_spin_lock(&pctrl->lock);
-
        community = intel_get_community(pctrl, pin);
        if (community) {
                unsigned padno = pin_to_padno(community, pin);
@@ -664,7 +666,14 @@ static void intel_gpio_irq_ack(struct irq_data *d)

                writel(BIT(gpp_offset), community->regs + GPI_IS + gpp * 4);
        }
+}

+static void intel_gpio_irq_ack(struct irq_data *d)
+{
+       struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+
+       raw_spin_lock(&pctrl->lock);
+       __intel_gpio_irq_ack(d);
        raw_spin_unlock(&pctrl->lock);
 }

@@ -694,18 +703,17 @@ static void intel_gpio_irq_enable(struct irq_data *d)
                writel(value, community->regs + community->ie_offset + gpp * 4);
        }

+       ipipe_unlock_irq(d->irq);
+
        raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }

-static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
+static void __intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 {
        struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
        struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
        const struct intel_community *community;
        unsigned pin = irqd_to_hwirq(d);
-       unsigned long flags;
-
-       raw_spin_lock_irqsave(&pctrl->lock, flags);

        community = intel_get_community(pctrl, pin);
        if (community) {
@@ -723,20 +731,55 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
                        value |= BIT(gpp_offset);
                writel(value, reg);
        }
-
-       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }

 static void intel_gpio_irq_mask(struct irq_data *d)
 {
-       intel_gpio_irq_mask_unmask(d, true);
+       struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+       unsigned long flags;
+
+       raw_spin_lock_irqsave(&pctrl->lock, flags);
+       ipipe_lock_irq(d->irq);
+       __intel_gpio_irq_mask_unmask(d, true);
+       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }

 static void intel_gpio_irq_unmask(struct irq_data *d)
 {
-       intel_gpio_irq_mask_unmask(d, false);
+       struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+       unsigned long flags;
+
+       raw_spin_lock_irqsave(&pctrl->lock, flags);
+       __intel_gpio_irq_mask_unmask(d, false);
+       ipipe_unlock_irq(d->irq);
+       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+#ifdef CONFIG_IPIPE
+
+static void intel_gpio_irq_hold(struct irq_data *d)
+{
+       struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+       unsigned long flags;
+
+       raw_spin_lock_irqsave(&pctrl->lock, flags);
+       __intel_gpio_irq_mask_unmask(d, true);
+       __intel_gpio_irq_ack(d);
+       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+static void intel_gpio_irq_release(struct irq_data *d)
+{
+       struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+       unsigned long flags;
+
+       raw_spin_lock_irqsave(&pctrl->lock, flags);
+       __intel_gpio_irq_mask_unmask(d, false);
+       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }

+#endif
+
 static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 {
        struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -837,7 +880,7 @@ static irqreturn_t intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,

                        irq = irq_find_mapping(gc->irqdomain,
                                               community->pin_base + padno);
-                       generic_handle_irq(irq);
+                       ipipe_handle_demuxed_irq(irq);

                        ret |= IRQ_HANDLED;
                }
@@ -862,6 +905,14 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
        return ret;
 }

+static void ipipe_irq_cascade(struct irq_desc *desc)
+{
+#ifdef CONFIG_IPIPE
+       intel_gpio_irq(irq_desc_get_irq(desc),
+                      irq_desc_get_handler_data(desc));
+#endif
+}
+
 static struct irq_chip intel_gpio_irqchip = {
        .name = "intel-gpio",
        .irq_enable = intel_gpio_irq_enable,
@@ -870,6 +921,10 @@ static struct irq_chip intel_gpio_irqchip = {
        .irq_unmask = intel_gpio_irq_unmask,
        .irq_set_type = intel_gpio_irq_type,
        .irq_set_wake = intel_gpio_irq_wake,
+#ifdef CONFIG_IPIPE
+       .irq_hold = intel_gpio_irq_hold,
+       .irq_release = intel_gpio_irq_release,
+#endif
 };

 static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
@@ -902,12 +957,17 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
         * to the irq directly) because on some platforms several GPIO
         * controllers share the same interrupt line.
         */
-       ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
-                              IRQF_SHARED | IRQF_NO_THREAD,
-                              dev_name(pctrl->dev), pctrl);
-       if (ret) {
-               dev_err(pctrl->dev, "failed to request interrupt\n");
-               goto fail;
+       if (IS_ENABLED(CONFIG_IPIPE))
+               irq_set_chained_handler_and_data(irq,
+                                       ipipe_irq_cascade, pctrl);
+       else {
+               ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
+                                      IRQF_SHARED | IRQF_NO_THREAD,
+                                      dev_name(pctrl->dev), pctrl);
+               if (ret) {
+                       dev_err(pctrl->dev, "failed to request interrupt\n");
+                       goto fail;
+               }
        }

        ret = gpiochip_irqchip_add(&pctrl->chip, &intel_gpio_irqchip, 0,

--
Philippe.