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

[Nitin Kulkarni <nitink@kth.se>]

Major change is use of handle_level_irq flow handler instead of handle_simple_irq
& calling the ack function manually in ipipe_irq_cascade.
Here is the Patch :

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3b19ef0..4f41532 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -22,6 +22,9 @@
 #include <linux/pinctrl/pinmux.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
+#include <linux/ipipe.h> 
 
 #include "pinctrl-intel.h"
 
@@ -65,6 +68,8 @@
 #define PADCFG1_TERM_5K			2
 #define PADCFG1_TERM_1K			1
 
+static struct intel_pinctrl *hack_pctrl;
+
 struct intel_pad_context {
 	u32 padcfg0;
 	u32 padcfg1;
@@ -94,7 +99,11 @@ struct intel_pinctrl_context {
  */
 struct intel_pinctrl {
 	struct device *dev;
-	spinlock_t lock;
+#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;
@@ -324,7 +333,7 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 	unsigned long flags;
 	int i;
 
-	spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	/*
 	 * All pins in the groups needs to be accessible and writable
@@ -332,7 +341,7 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 	 */
 	for (i = 0; i < grp->npins; i++) {
 		if (!intel_pad_usable(pctrl, grp->pins[i])) {
-			spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 			return -EBUSY;
 		}
 	}
@@ -351,7 +360,7 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 		writel(value, padcfg0);
 	}
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return 0;
 }
@@ -365,10 +374,10 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	unsigned long flags;
 	u32 value;
 
-	spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	if (!intel_pad_usable(pctrl, pin)) {
-		spin_unlock_irqrestore(&pctrl->lock, flags);
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 		return -EBUSY;
 	}
 
@@ -383,7 +392,7 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	value |= PADCFG0_GPIOTXDIS;
 	writel(value, padcfg0);
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return 0;
 }
@@ -397,7 +406,7 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
 	unsigned long flags;
 	u32 value;
 
-	spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
 
@@ -408,7 +417,7 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
 		value &= ~PADCFG0_GPIOTXDIS;
 	writel(value, padcfg0);
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return 0;
 }
@@ -496,7 +505,7 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned pin,
 	int ret = 0;
 	u32 value;
 
-	spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	padcfg1 = intel_get_padcfg(pctrl, pin, PADCFG1);
 	value = readl(padcfg1);
@@ -550,7 +559,7 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned pin,
 	if (!ret)
 		writel(value, padcfg1);
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return ret;
 }
@@ -617,14 +626,14 @@ static void intel_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 		unsigned long flags;
 		u32 padcfg0;
 
-		spin_lock_irqsave(&pctrl->lock, flags);
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
 		padcfg0 = readl(reg);
 		if (value)
 			padcfg0 |= PADCFG0_GPIOTXSTATE;
 		else
 			padcfg0 &= ~PADCFG0_GPIOTXSTATE;
 		writel(padcfg0, reg);
-		spin_unlock_irqrestore(&pctrl->lock, flags);
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 	}
 }
 
@@ -650,14 +659,14 @@ 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);
 
-	spin_lock(&pctrl->lock);
 
 	community = intel_get_community(pctrl, pin);
 	if (community) {
@@ -668,20 +677,32 @@ static void intel_gpio_irq_ack(struct irq_data *d)
 		writel(BIT(gpp_offset), community->regs + GPI_IS + gpp * 4);
 	}
 
-	spin_unlock(&pctrl->lock);
 }
 
+
+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);
+
+	raw_spin_lock(&pctrl->lock);
+	__intel_gpio_irq_ack(d);
+	raw_spin_unlock(&pctrl->lock);
+}
+
+
 static void intel_gpio_irq_enable(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);
 	unsigned long flags;
-
-	spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	community = intel_get_community(pctrl, pin);
+
 	if (community) {
 		unsigned padno = pin_to_padno(community, pin);
 		unsigned gpp_size = community->gpp_size;
@@ -694,21 +715,24 @@ static void intel_gpio_irq_enable(struct irq_data *d)
 
 		value = readl(community->regs + community->ie_offset + gpp * 4);
 		value |= BIT(gpp_offset);
-		writel(value, community->regs + community->ie_offset + gpp * 4);
+		writel(value, community->regs + community->ie_offset + gpp * 4);	
+
 	}
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+	hack_pctrl = pctrl;
+
+	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;
-
-	spin_lock_irqsave(&pctrl->lock, flags);
 
 	community = intel_get_community(pctrl, pin);
 	if (community) {
@@ -727,23 +751,63 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 		writel(value, reg);
 	}
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
+
 static void intel_gpio_irq_mask(struct irq_data *d)
 {
-	intel_gpio_irq_mask_unmask(d, true);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	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 gpio_chip *gc = irq_data_get_irq_chip_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);
+	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 gpio_chip *gc = irq_data_get_irq_chip_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);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+}
+
+static void intel_gpio_irq_release(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_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);
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned pin = irqd_to_hwirq(d);
 	unsigned long flags;
 	void __iomem *reg;
@@ -763,7 +827,7 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 		return -EPERM;
 	}
 
-	spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	value = readl(reg);
 
@@ -790,7 +854,7 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 	else if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(d, handle_level_irq);
 
-	spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return 0;
 }
@@ -840,12 +904,14 @@ 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;
 		}
 	}
 
+
 	return ret;
 }
 
@@ -853,11 +919,13 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
 {
 	const struct intel_community *community;
 	struct intel_pinctrl *pctrl = data;
+
 	irqreturn_t ret = IRQ_NONE;
 	int i;
 
 	/* Need to check all communities for pending interrupts */
 	for (i = 0; i < pctrl->ncommunities; i++) {
+
 		community = &pctrl->communities[i];
 		ret |= intel_gpio_community_irq_handler(pctrl, community);
 	}
@@ -865,6 +933,20 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
 	return ret;
 }
 
+static void ipipe_irq_cascade(struct irq_desc *desc)
+{
+
+#ifdef CONFIG_IPIPE
+	struct intel_pinctrl *pctrl = hack_pctrl;
+	int irq = irq_desc_get_irq(desc);
+
+	desc->irq_data.chip->irq_ack(&desc->irq_data);
+
+	intel_gpio_irq(irq,pctrl);
+#endif
+
+}
+
 static struct irq_chip intel_gpio_irqchip = {
 	.name = "intel-gpio",
 	.irq_enable = intel_gpio_irq_enable,
@@ -873,11 +955,18 @@ 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)
 {
 	int ret;
+	
 
 	pctrl->chip = intel_gpio_chip;
 
@@ -887,6 +976,7 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	pctrl->chip.base = -1;
 	pctrl->irq = irq;
 
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "failed to register gpiochip\n");
@@ -900,20 +990,28 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 		goto fail;
 	}
 
+	if (IS_ENABLED(CONFIG_IPIPE))
+	{	
+		irq_set_chained_handler_and_data(irq,
+					ipipe_irq_cascade,pctrl);
+	}
+	else {
 	/*
 	 * We need to request the interrupt here (instead of providing chip
 	 * 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,
-			       dev_name(pctrl->dev), pctrl);
-	if (ret) {
-		dev_err(pctrl->dev, "failed to request interrupt\n");
-		goto fail;
+		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,
-				   handle_simple_irq, IRQ_TYPE_NONE);
+				   handle_level_irq, IRQ_TYPE_NONE);
 	if (ret) {
 		dev_err(pctrl->dev, "failed to add irqchip\n");
 		goto fail;
@@ -921,6 +1019,8 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 
 	gpiochip_set_chained_irqchip(&pctrl->chip, &intel_gpio_irqchip, irq,
 				     NULL);
+
+
 	return 0;
 
 fail:
@@ -981,7 +1081,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 
 	pctrl->dev = &pdev->dev;
 	pctrl->soc = soc_data;
-	spin_lock_init(&pctrl->lock);
+	raw_spin_lock_init(&pctrl->lock);
 
 	/*
 	 * Make a copy of the communities which we can use to hold pointers

Note: I am working on kernel version 4.4.43, hence there are some differences from the recent version of pinctrl-intel.
For example, I had to change the spinlocks into raw spinlocks. 

regards,
Nitin 
________________________________________
From: Philippe Gerum <rpm@xenomai.org>
Sent: Wednesday, June 14, 2017 10:27 AM
To: Nitin Kulkarni; xenomai@xenomai.org
Subject: Re: [Xenomai] Problem with gpio interrupts for xenomai3 on Intel joule

On 06/12/2017 01:02 PM, Nitin Kulkarni wrote:
> 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.
>

Please send a patch including your changes; it should be easier to
review and comment.

--
Philippe.