demuxing irqs

demuxing irqs

[Jon Smirl]

Can someone point me at a simple example of how to demux irqs using
the powerpc irq functions? I have eight devices on a single irq and I
want to turn them into virtual irqs.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Roland Dreier]

 > Can someone point me at a simple example of how to demux irqs using
 > the powerpc irq functions? I have eight devices on a single irq and I
 > want to turn them into virtual irqs.

Sorry about the previous reply.

Anyway, what are you going to demux based on?  Do you have some other
signal you can read in the interrupt dispatch code that tells you which
device raised the interrupt?  What happens if two devices raise an
interrupt at the same time?

If you just have 8 interrupt lines wire-ORed together then you probably
just need to register your interrupt handlers with IRQF_SHARED and run 8
interrupt handlers on an interrupt.

 - R.

Re: demuxing irqs

[Jon Smirl]

On Sat, Sep 13, 2008 at 6:41 PM, Roland Dreier <rdreier@cisco.com> wrote:
>  > Can someone point me at a simple example of how to demux irqs using
>  > the powerpc irq functions? I have eight devices on a single irq and I
>  > want to turn them into virtual irqs.
>
> Sorry about the previous reply.
>
> Anyway, what are you going to demux based on?  Do you have some other
> signal you can read in the interrupt dispatch code that tells you which
> device raised the interrupt?  What happens if two devices raise an
> interrupt at the same time?
>
> If you just have 8 interrupt lines wire-ORed together then you probably
> just need to register your interrupt handlers with IRQF_SHARED and run 8
> interrupt handlers on an interrupt.

The muxed interrupts are inside a SOC CPU.  For example eight GPIOs
can each individually be enabled to trigger hardware interrupt 7. When
I get hw interrupt 7 i want to demux it into 8 virtual interrupts.
There are eight bit registers for individually acking, enabling, etc
each of the eight multiplexed interrupts. With eight virutal
interrupts each user can register a different handler and isn't aware
the muxing is going on.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Roland Dreier]

 > The muxed interrupts are inside a SOC CPU.  For example eight GPIOs
 > can each individually be enabled to trigger hardware interrupt 7. When
 > I get hw interrupt 7 i want to demux it into 8 virtual interrupts.
 > There are eight bit registers for individually acking, enabling, etc
 > each of the eight multiplexed interrupts. With eight virutal
 > interrupts each user can register a different handler and isn't aware
 > the muxing is going on.

I see... well, assuming all the issues around handling multiple
simultaneous (or overlapping) interrupts are OK, you could look at how
arch/powerpc/sysdev/uic.c handles the cascaded interrupt controllers for
4xx SoCs.  The idea of that code is that you get an interrupt, and look
look at interrupt cause register 0, and see interrupt X, and to handle
interrupt X you read interrupt cause register 1 to see which the real
interrupt is.  And you might see interrupt Y, which means to go onto
interrupt cause register 2.

 - R.

Re: demuxing irqs

[Jon Smirl]

On Sat, Sep 13, 2008 at 7:04 PM, Roland Dreier <rdreier@cisco.com> wrote:
>  > The muxed interrupts are inside a SOC CPU.  For example eight GPIOs
>  > can each individually be enabled to trigger hardware interrupt 7. When
>  > I get hw interrupt 7 i want to demux it into 8 virtual interrupts.
>  > There are eight bit registers for individually acking, enabling, etc
>  > each of the eight multiplexed interrupts. With eight virutal
>  > interrupts each user can register a different handler and isn't aware
>  > the muxing is going on.
>
> I see... well, assuming all the issues around handling multiple
> simultaneous (or overlapping) interrupts are OK, you could look at how
> arch/powerpc/sysdev/uic.c handles the cascaded interrupt controllers for
> 4xx SoCs.  The idea of that code is that you get an interrupt, and look
> look at interrupt cause register 0, and see interrupt X, and to handle
> interrupt X you read interrupt cause register 1 to see which the real
> interrupt is.  And you might see interrupt Y, which means to go onto
> interrupt cause register 2.

I'm getting bogged down in the details. For example after doing
irq_alloc_host() some drivers follow it with
set_irq_chained_handler(). It the uic case it doesn't.

I'm not getting my main hardware int acknowleged, my handler gets
called repeatedly. This is probably from using a chained_handler.

Do I need a match function in irq_host_ops?

I have a 'struct irq_chip' with the mask/unmask/ack/etc functions.

I also need to know what the first virq number is out of the eight I
asked for in irq_alloc_host().


>
>  - R.
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Jon Smirl]

How do the virq numbers get assigned?
What do I pass into request_irq when I use one of these pins?

The gpio device...
		gpio_wkup: gpio-wkup@c00 {
			compatible = "fsl,mpc5200b-gpio-wkup","fsl,mpc5200-gpio-wkup";
			reg = <0xc00 0x40>;
			interrupts = <0x1 0x8 0x0 0x0 0x3 0x0>;
			interrupt-parent = <&mpc5200_pic>;
			gpio-controller;
			#gpio-cells = <4>;
		};

use of a gpio from the device...
		ir@0 {
			compatible = "digispeaker,gpio-ir";

			gpios = <&gpio_wkup 0 0 1 0>;
		};


0 - gpio number in the device
0 - enable open drain
1 - class of interrupt, normal or wakeup
0 - type of interrupt, rising, falling, both, pulse


-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Jon Smirl]

I'm making some progress on this. I implemented gpio_to_irq() and
irq_to_gpio() to cause a 1:1 mapping.

Currently my irqhost->match() function is never getting called with my
device tree node so I need to figure out why. Without matching my
irqhost->map() function doesn't get called.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

I have demultiplexing the GPIO interrupts working well enough to make
my hardware work. But now I've discovered that these interrupts can't
do what I need.

Anton, Grant - are either of you interested in this code? It's not
finished but the main ideas are in place.

I need to switch over to using timer pins. They don't have multiplexed
interrupts.

-- 
Jon Smirl
jonsmirl@gmail.com

[-- Attachment #2: jds-gpio --]
[-- Type: application/octet-stream, Size: 16783 bytes --]

Modify gpiolib to pass irqs functions to chip drivers

From: Jon Smirl <jonsmirl@gmail.com>


---

 arch/powerpc/boot/dts/dspeak01.dts         |    6 -
 arch/powerpc/include/asm/gpio.h            |   11 -
 arch/powerpc/platforms/52xx/mpc52xx_gpio.c |  268 +++++++++++++++++++++++++++-
 drivers/input/lirc/lirc_dspeak.c           |   34 +++-
 4 files changed, 289 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/boot/dts/dspeak01.dts b/arch/powerpc/boot/dts/dspeak01.dts
index cead9e5..a089381 100644
--- a/arch/powerpc/boot/dts/dspeak01.dts
+++ b/arch/powerpc/boot/dts/dspeak01.dts
@@ -164,7 +164,7 @@
 			interrupts = <0x1 0x8 0x0 0x0 0x3 0x0>;
 			interrupt-parent = <&mpc5200_pic>;
 			gpio-controller;
-			#gpio-cells = <2>;
+			#gpio-cells = <4>;
 		};
 
 		spi@f00 {
@@ -331,8 +331,8 @@
 		ir {
 			compatible = "digispeaker,gpio-ir";
 
-			gpios = <&gpio_wkup 0 0>;
-		}
+			gpios = <&gpio_wkup 0 0 1 0>;
+		};
 
 		/* This is only an example device to show the usage of gpios. It maps all available
 		 * gpios to the "gpio-provider" device.
diff --git a/arch/powerpc/include/asm/gpio.h b/arch/powerpc/include/asm/gpio.h
index ea04632..2283d19 100644
--- a/arch/powerpc/include/asm/gpio.h
+++ b/arch/powerpc/include/asm/gpio.h
@@ -38,17 +38,14 @@ static inline int gpio_cansleep(unsigned int gpio)
 	return __gpio_cansleep(gpio);
 }
 
-/*
- * Not implemented, yet.
- */
-static inline int gpio_to_irq(unsigned int gpio)
+static inline unsigned int gpio_to_irq(unsigned int gpio)
 {
-	return -ENOSYS;
+	return gpio;
 }
 
-static inline int irq_to_gpio(unsigned int irq)
+static inline unsigned int irq_to_gpio(unsigned int irq)
 {
-	return -EINVAL;
+	return irq;
 }
 
 #endif /* CONFIG_GPIOLIB */
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
index 8a455eb..2556c12 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
@@ -22,18 +22,29 @@
 #include <linux/of_gpio.h>
 #include <linux/io.h>
 #include <linux/of_platform.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 
 #include <asm/gpio.h>
 #include <asm/mpc52xx.h>
 #include <sysdev/fsl_soc.h>
 
-static DEFINE_SPINLOCK(gpio_lock);
+#include "mpc52xx_pic.h"
+
+#define IRQ_GPIO(x)  (MPC52xx_IRQ_HIGHTESTHWIRQ + x)
+#define IRQ_GPIO_WKUP(x)  (IRQ_GPIO(32) + x)
 
 struct mpc52xx_gpiochip {
 	struct of_mm_gpio_chip mmchip;
 	unsigned int shadow_dvo;
 	unsigned int shadow_gpioe;
 	unsigned int shadow_ddr;
+	unsigned int shadow_ode;
+	unsigned int shadow_inten;
+	unsigned int shadow_iinten;
+	unsigned int shadow_itype;
+	spinlock_t lock;
+	struct irq_host	*irqhost;
 };
 
 /*
@@ -84,13 +95,16 @@ __mpc52xx_wkup_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 static void
 mpc52xx_wkup_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 {
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+			struct mpc52xx_gpiochip, mmchip);
 	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
+	spin_lock_irqsave(&chip->lock, flags);
 
 	__mpc52xx_wkup_gpio_set(gc, gpio, val);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
 
 	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
 }
@@ -103,7 +117,7 @@ static int mpc52xx_wkup_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	struct mpc52xx_gpio_wkup __iomem *regs = mm_gc->regs;
 	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
+	spin_lock_irqsave(&chip->lock, flags);
 
 	/* set the direction */
 	chip->shadow_ddr &= ~(1 << (7 - gpio));
@@ -113,7 +127,7 @@ static int mpc52xx_wkup_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	chip->shadow_gpioe |= 1 << (7 - gpio);
 	out_8(&regs->wkup_gpioe, chip->shadow_gpioe);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
 
 	return 0;
 }
@@ -127,7 +141,7 @@ mpc52xx_wkup_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 			struct mpc52xx_gpiochip, mmchip);
 	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
+	spin_lock_irqsave(&chip->lock, flags);
 
 	__mpc52xx_wkup_gpio_set(gc, gpio, val);
 
@@ -139,33 +153,230 @@ mpc52xx_wkup_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	chip->shadow_gpioe |= 1 << (7 - gpio);
 	out_8(&regs->wkup_gpioe, chip->shadow_gpioe);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
 
 	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
 
 	return 0;
 }
 
+static int
+mpc52xx_wkup_gpiochip_xlate(struct of_gpio_chip *of_gc, struct device_node *np, const void *gpio_spec)
+{
+	struct of_mm_gpio_chip *mm_gc = container_of(of_gc, struct of_mm_gpio_chip, of_gc);
+	struct mpc52xx_gpio_wkup __iomem *regs = mm_gc->regs;
+	struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+			struct mpc52xx_gpiochip, mmchip);
+	const u32 *spec = gpio_spec;
+	u32 gpio = *spec;
+	u32 drain = *(spec + 1);
+	u32 interrupt = *(spec + 2);
+	u32 type = *(spec + 3);
+
+	if (gpio > of_gc->gc.ngpio)
+		return -EINVAL;
+
+	printk("mpc52xx_wkup_gpiochip_xlate %d %d %d %d\n", gpio, drain, interrupt, type);
+
+	out_8(&regs->wkup_maste, 0);
+
+	chip->shadow_ode |= 1 << (7 - gpio);
+	out_8(&regs->wkup_ode, chip->shadow_ode);
+
+	if (interrupt & 2) {
+		chip->shadow_inten |= 1 << (7 - gpio);
+		out_8(&regs->wkup_inten, chip->shadow_inten);
+	}
+	if (interrupt & 1) {
+		chip->shadow_iinten |= 1 << (7 - gpio);
+		out_8(&regs->wkup_iinten, chip->shadow_iinten);
+	}
+
+	chip->shadow_itype |= type << (15 - (gpio * 2));
+	out_be16(&regs->wkup_itype, chip->shadow_itype);
+
+	printk("mpc52xx_wkup_gpiochip_xlate status %x\n", in_8(&regs->wkup_istat));
+	out_8(&regs->wkup_istat, 0xFF);
+
+	printk("mpc52xx_wkup_gpiochip_xlate bits is %x\n", (chip->shadow_iinten | chip->shadow_inten));
+	out_8(&regs->wkup_maste, (chip->shadow_iinten | chip->shadow_inten) != 0);
+	printk("mpc52xx_wkup_gpiochip_xlate enable is %x\n", in_8(&regs->wkup_maste));
+
+	return gpio;
+}
+
+unsigned int gpio_wkup_irq_to_hw(unsigned int virq)
+{
+	return virq - IRQ_GPIO_WKUP(0);
+}
+
+static void gpio_wkup_unmask_irq(unsigned int virq)
+{
+	struct irq_desc *desc = get_irq_desc(virq);
+	struct mpc52xx_gpiochip *chip = get_irq_chip_data(virq);
+	unsigned int irq = gpio_wkup_irq_to_hw(virq);
+	unsigned long flags;
+
+	printk("gpio_wkup_unmask_irq");
+
+	spin_lock_irqsave(&chip->lock, flags);
+	/* ack level-triggered interrupts here */
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static void gpio_wkup_mask_irq(unsigned int virq)
+{
+	struct irq_desc *desc = get_irq_desc(virq);
+	struct mpc52xx_gpiochip *chip = get_irq_chip_data(virq);
+	unsigned int irq = gpio_wkup_irq_to_hw(virq);
+	unsigned long flags;
+
+	printk("gpio_wkup_mask_irq");
+
+	spin_lock_irqsave(&chip->lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static void gpio_wkup_ack_irq(unsigned int virq)
+{
+	struct irq_desc *desc = get_irq_desc(virq);
+	struct mpc52xx_gpiochip *chip = get_irq_chip_data(virq);
+	unsigned int irq = gpio_wkup_irq_to_hw(virq);
+	unsigned long flags;
+
+	printk("gpio_wkup_ack_irq");
+
+	spin_lock_irqsave(&chip->lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static void gpio_wkup_mask_ack_irq(unsigned int virq)
+{
+	struct irq_desc *desc = get_irq_desc(virq);
+	struct mpc52xx_gpiochip *chip = get_irq_chip_data(virq);
+	unsigned int irq = gpio_wkup_irq_to_hw(virq);
+	unsigned long flags;
+
+	printk("gpio_wkup_mask_ack_irq");
+
+	spin_lock_irqsave(&chip->lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static int gpio_wkup_set_irq_type(unsigned int virq, unsigned int flow_type)
+{
+	struct irq_desc *desc = get_irq_desc(virq);
+	struct mpc52xx_gpiochip *chip = get_irq_chip_data(virq);
+	unsigned int irq = gpio_wkup_irq_to_hw(virq);
+	unsigned long flags;
+	int trigger, polarity;
+
+	printk("gpio_wkup_set_irq_type");
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_NONE:
+		gpio_wkup_mask_irq(virq);
+		return 0;
+
+	case IRQ_TYPE_EDGE_RISING:
+		trigger = 1; polarity = 1;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		trigger = 1; polarity = 0;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		trigger = 0; polarity = 1;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		trigger = 0; polarity = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL);
+	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
+	if (!trigger)
+		desc->status |= IRQ_LEVEL;
+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static struct irq_chip gpio_wkup_irq_chip = {
+	.typename	= " GPIO_WKUP  ",
+	.unmask		= gpio_wkup_unmask_irq,
+	.mask		= gpio_wkup_mask_irq,
+ 	.mask_ack	= gpio_wkup_mask_ack_irq,
+	.ack		= gpio_wkup_ack_irq,
+	.set_type	= gpio_wkup_set_irq_type,
+};
+
+
+static void
+gpio_wkup_demux(unsigned int irq, struct irq_desc *desc)
+{
+	struct mpc52xx_gpiochip *chip = desc->handler_data;
+	struct mpc52xx_gpio_wkup __iomem *regs = chip->mmchip.regs;
+	int gpio = irq - IRQ_GPIO_WKUP(0);
+	static int count;
+
+	desc->chip->mask_ack(irq);
+	out_8(&regs->wkup_istat, 0xFF);
+	printk("gpio_wkup_irq %d %02x\n", count++, in_8(&regs->wkup_ival) & 0x80);
+	desc->chip->unmask(irq);
+}
+
+static int
+gpio_wkup_map(struct irq_host *h, unsigned int virq,
+			     irq_hw_number_t hw)
+{
+	struct mpc52xx_gpiochip *chip = h->host_data;
+
+	printk("gpio_wkup_map virq %d hw %ld\n", virq, hw);
+
+	set_irq_chip_data(virq, chip);
+	set_irq_chip_and_handler(virq, &gpio_wkup_irq_chip, handle_level_irq);
+	set_irq_type(virq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+static struct
+irq_host_ops gpio_wkup_ops = {
+	.map = gpio_wkup_map,
+};
+
 static int __devinit mpc52xx_wkup_gpiochip_probe(struct of_device *ofdev,
 					const struct of_device_id *match)
 {
 	struct mpc52xx_gpiochip *chip;
 	struct mpc52xx_gpio_wkup __iomem *regs;
 	struct of_gpio_chip *ofchip;
+	unsigned int irq, i;
 	int ret;
 
+	printk("mpc52xx_wkup_gpiochip_probe\n");
+
 	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
 	ofchip = &chip->mmchip.of_gc;
 
-	ofchip->gpio_cells          = 2;
+	ofchip->gpio_cells          = 4;
 	ofchip->gc.ngpio            = 8;
 	ofchip->gc.direction_input  = mpc52xx_wkup_gpio_dir_in;
 	ofchip->gc.direction_output = mpc52xx_wkup_gpio_dir_out;
 	ofchip->gc.get              = mpc52xx_wkup_gpio_get;
 	ofchip->gc.set              = mpc52xx_wkup_gpio_set;
+	ofchip->gc.base             = IRQ_GPIO_WKUP(0);
+	ofchip->xlate               = mpc52xx_wkup_gpiochip_xlate;
+
 
 	ret = of_mm_gpiochip_add(ofdev->node, &chip->mmchip);
 	if (ret)
@@ -175,6 +386,30 @@ static int __devinit mpc52xx_wkup_gpiochip_probe(struct of_device *ofdev,
 	chip->shadow_gpioe = in_8(&regs->wkup_gpioe);
 	chip->shadow_ddr = in_8(&regs->wkup_ddr);
 	chip->shadow_dvo = in_8(&regs->wkup_dvo);
+	chip->shadow_ode = in_8(&regs->wkup_ode);
+	chip->shadow_inten = in_8(&regs->wkup_inten);
+	chip->shadow_iinten = in_8(&regs->wkup_iinten);
+	chip->shadow_itype = in_be16(&regs->wkup_itype);
+	spin_lock_init(&chip->lock);
+
+	irq = irq_of_parse_and_map(ofdev->node, 0);
+	if (irq == NO_IRQ)
+		return 0;
+	printk("mpc52xx_wkup_gpiochip_probe cascade irq %d\n", irq);
+	printk("mpc52xx_wkup_gpiochip_probe node %p\n", ofdev->node);
+
+	chip->irqhost = irq_alloc_host(ofdev->node, IRQ_HOST_MAP_LINEAR, 8, &gpio_wkup_ops, -1);
+	if (!chip->irqhost) {
+		printk(KERN_ERR "GPIO WKUP: failed to allocate irq host!\n");
+		return 0;
+	}
+	set_irq_chained_handler(irq, gpio_wkup_demux);
+	set_irq_data(irq, chip);
+
+	for (i = 0; i < 8; i++) {
+		irq = irq_create_mapping(chip->irqhost, IRQ_GPIO_WKUP(i));
+		printk("mpc52xx_wkup_gpiochip_probe %d %d\n", irq, IRQ_GPIO_WKUP(i));
+	}
 
 	return 0;
 }
@@ -244,13 +479,16 @@ __mpc52xx_simple_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 static void
 mpc52xx_simple_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 {
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+			struct mpc52xx_gpiochip, mmchip);
 	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
+	spin_lock_irqsave(&chip->lock, flags);
 
 	__mpc52xx_simple_gpio_set(gc, gpio, val);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
 
 	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
 }
@@ -263,7 +501,7 @@ static int mpc52xx_simple_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	struct mpc52xx_gpio __iomem *regs = mm_gc->regs;
 	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
+	spin_lock_irqsave(&chip->lock, flags);
 
 	/* set the direction */
 	chip->shadow_ddr &= ~(1 << (31 - gpio));
@@ -273,7 +511,7 @@ static int mpc52xx_simple_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	chip->shadow_gpioe |= 1 << (31 - gpio);
 	out_be32(&regs->simple_gpioe, chip->shadow_gpioe);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
 
 	return 0;
 }
@@ -287,7 +525,7 @@ mpc52xx_simple_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	struct mpc52xx_gpio __iomem *regs = mm_gc->regs;
 	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
+	spin_lock_irqsave(&chip->lock, flags);
 
 	/* First set initial value */
 	__mpc52xx_simple_gpio_set(gc, gpio, val);
@@ -300,7 +538,7 @@ mpc52xx_simple_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	chip->shadow_gpioe |= 1 << (31 - gpio);
 	out_be32(&regs->simple_gpioe, chip->shadow_gpioe);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	spin_unlock_irqrestore(&chip->lock, flags);
 
 	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
 
@@ -323,6 +561,7 @@ static int __devinit mpc52xx_simple_gpiochip_probe(struct of_device *ofdev,
 
 	ofchip->gpio_cells          = 2;
 	ofchip->gc.ngpio            = 32;
+	ofchip->gc.base             = IRQ_GPIO(0);
 	ofchip->gc.direction_input  = mpc52xx_simple_gpio_dir_in;
 	ofchip->gc.direction_output = mpc52xx_simple_gpio_dir_out;
 	ofchip->gc.get              = mpc52xx_simple_gpio_get;
@@ -333,6 +572,7 @@ static int __devinit mpc52xx_simple_gpiochip_probe(struct of_device *ofdev,
 		return ret;
 
 	regs = chip->mmchip.regs;
+	spin_lock_init(&chip->lock);
 	chip->shadow_gpioe = in_be32(&regs->simple_gpioe);
 	chip->shadow_ddr = in_be32(&regs->simple_ddr);
 	chip->shadow_dvo = in_be32(&regs->simple_dvo);
diff --git a/drivers/input/lirc/lirc_dspeak.c b/drivers/input/lirc/lirc_dspeak.c
index d093b37..39ef6f8 100644
--- a/drivers/input/lirc/lirc_dspeak.c
+++ b/drivers/input/lirc/lirc_dspeak.c
@@ -5,6 +5,8 @@
  * Copyright (C) 2008 Secret Lab Technologies Ltd.
  */
 
+#define DEBUG
+
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -13,11 +15,26 @@
 #include <linux/of_platform.h>
 #include <linux/of_gpio.h>
 
+
 struct dspeak_ir {
 	int gpio;
 	spinlock_t lock;
 };
 
+/*
+ * Interrupt handlers
+ */
+static irqreturn_t dpeak_ir_irq(int irq, void *_ir)
+{
+	struct dspeak_ir *dspeak_ir = _ir;
+
+	printk("dpeak_ir_irq\n");
+
+	return IRQ_HANDLED;
+}
+
+
+
 /* ---------------------------------------------------------------------
  * OF platform bus binding code:
  * - Probe/remove operations
@@ -26,10 +43,11 @@ struct dspeak_ir {
 static int __devinit dspeak_ir_of_probe(struct of_device *op,
 				      const struct of_device_id *match)
 {
-	struct device_node *np = op->node, *child;
+	struct device_node *np = op->node;
 	struct dspeak_ir *dspeak_ir;
+	int rc;
 
-	dev_dbg(&op->dev, "probing dspeaker ir device\n");
+	dev_dbg(&op->dev, "dspeak_ir_of_probe\n");
 
 	/* Allocate and initialize the driver private data */
 	dspeak_ir = kzalloc(sizeof *dspeak_ir, GFP_KERNEL);
@@ -37,9 +55,13 @@ static int __devinit dspeak_ir_of_probe(struct of_device *op,
 		return -ENOMEM;
 	}
 
-	for_each_child_of_node(np, child) {
-		dspeak_ir->gpio = of_get_gpio(child, 0);
-	}
+	dspeak_ir->gpio = of_get_gpio(np, 0);
+	gpio_direction_input(dspeak_ir->gpio);
+	printk("dspeak_ir_of_probe irq = %d\n", gpio_to_irq(dspeak_ir->gpio));
+
+	rc = request_irq(gpio_to_irq(dspeak_ir->gpio), &dpeak_ir_irq, IRQF_SHARED,
+			 "digispeaker-ir", dspeak_ir);
+	printk("dspeak_ir_of_probe request = %d\n", rc);
 
 	spin_lock_init(&dspeak_ir->lock);
 
@@ -78,10 +100,10 @@ static struct of_platform_driver dspeak_ir_driver = {
 
 /* ---------------------------------------------------------------------
  * Module setup and teardown; simply register the of_platform driver
- * for the PSC in I2S mode.
  */
 static int __init dspeak_ir_init(void)
 {
+	printk("dspeak_ir_init\n");
 	return of_register_platform_driver(&dspeak_ir_driver);
 }
 module_init(dspeak_ir_init);

Re: demuxing irqs

[Anton Vorontsov]

Hi Jon,

On Sun, Sep 14, 2008 at 11:06:23PM -0400, Jon Smirl wrote:
> I have demultiplexing the GPIO interrupts working well enough to make
> my hardware work. But now I've discovered that these interrupts can't
> do what I need.
> 
> Anton, Grant - are either of you interested in this code? It's not
> finished but the main ideas are in place.

I think there is a small issue that ruins the whole approach.. :-/

> --- a/arch/powerpc/include/asm/gpio.h
> +++ b/arch/powerpc/include/asm/gpio.h
> @@ -38,17 +38,14 @@ static inline int gpio_cansleep(unsigned int gpio)
>         return __gpio_cansleep(gpio);
> }
> 
> -/*
> - * Not implemented, yet.
> - */
> -static inline int gpio_to_irq(unsigned int gpio)
> +static inline unsigned int gpio_to_irq(unsigned int gpio)
>  {
> -       return -ENOSYS;
> +       return gpio;

"GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
can't do 1:1 mapping. :-(

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 8:17 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Hi Jon,
>
> On Sun, Sep 14, 2008 at 11:06:23PM -0400, Jon Smirl wrote:
>> I have demultiplexing the GPIO interrupts working well enough to make
>> my hardware work. But now I've discovered that these interrupts can't
>> do what I need.
>>
>> Anton, Grant - are either of you interested in this code? It's not
>> finished but the main ideas are in place.
>
> I think there is a small issue that ruins the whole approach.. :-/
>
>> --- a/arch/powerpc/include/asm/gpio.h
>> +++ b/arch/powerpc/include/asm/gpio.h
>> @@ -38,17 +38,14 @@ static inline int gpio_cansleep(unsigned int gpio)
>>         return __gpio_cansleep(gpio);
>> }
>>
>> -/*
>> - * Not implemented, yet.
>> - */
>> -static inline int gpio_to_irq(unsigned int gpio)
>> +static inline unsigned int gpio_to_irq(unsigned int gpio)
>>  {
>> -       return -ENOSYS;
>> +       return gpio;
>
> "GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
> can't do 1:1 mapping. :-(

I changed the GPIO numbers inside of Linux to match the virqs.

	ofchip->gc.base             = IRQ_GPIO_WKUP(0);


>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Anton Vorontsov]

On Tue, Sep 16, 2008 at 08:37:55AM -0400, Jon Smirl wrote:
> On Tue, Sep 16, 2008 at 8:17 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > Hi Jon,
> >
> > On Sun, Sep 14, 2008 at 11:06:23PM -0400, Jon Smirl wrote:
> >> I have demultiplexing the GPIO interrupts working well enough to make
> >> my hardware work. But now I've discovered that these interrupts can't
> >> do what I need.
> >>
> >> Anton, Grant - are either of you interested in this code? It's not
> >> finished but the main ideas are in place.
> >
> > I think there is a small issue that ruins the whole approach.. :-/
> >
> >> --- a/arch/powerpc/include/asm/gpio.h
> >> +++ b/arch/powerpc/include/asm/gpio.h
> >> @@ -38,17 +38,14 @@ static inline int gpio_cansleep(unsigned int gpio)
> >>         return __gpio_cansleep(gpio);
> >> }
> >>
> >> -/*
> >> - * Not implemented, yet.
> >> - */
> >> -static inline int gpio_to_irq(unsigned int gpio)
> >> +static inline unsigned int gpio_to_irq(unsigned int gpio)
> >>  {
> >> -       return -ENOSYS;
> >> +       return gpio;
> >
> > "GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
> > can't do 1:1 mapping. :-(
> 
> I changed the GPIO numbers inside of Linux to match the virqs.
> 
> 	ofchip->gc.base             = IRQ_GPIO_WKUP(0);

Well, I didn't say that it will not work, what I'm trying to say
is that I don't quite like the idea of 1:1 mapping for all gpio
chips.

It is error prone, i.e. gpio_to_irq() can't fail, so you can't
tell if gpio to irq translation really happened or not. Plus
we might decide to not do 1:1 mapping for other gpio chips.

I think that this translation should go via gpiolib's callback
(there is no .to_irq callback, but we should implement one).

In the callback you can still do 1:1 mapping, but this mapping
will work only for this particular gpio chip, and not for others.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 9:12 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Sep 16, 2008 at 08:37:55AM -0400, Jon Smirl wrote:
>> On Tue, Sep 16, 2008 at 8:17 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > Hi Jon,
>> >
>> > On Sun, Sep 14, 2008 at 11:06:23PM -0400, Jon Smirl wrote:
>> >> I have demultiplexing the GPIO interrupts working well enough to make
>> >> my hardware work. But now I've discovered that these interrupts can't
>> >> do what I need.
>> >>
>> >> Anton, Grant - are either of you interested in this code? It's not
>> >> finished but the main ideas are in place.
>> >
>> > I think there is a small issue that ruins the whole approach.. :-/
>> >
>> >> --- a/arch/powerpc/include/asm/gpio.h
>> >> +++ b/arch/powerpc/include/asm/gpio.h
>> >> @@ -38,17 +38,14 @@ static inline int gpio_cansleep(unsigned int gpio)
>> >>         return __gpio_cansleep(gpio);
>> >> }
>> >>
>> >> -/*
>> >> - * Not implemented, yet.
>> >> - */
>> >> -static inline int gpio_to_irq(unsigned int gpio)
>> >> +static inline unsigned int gpio_to_irq(unsigned int gpio)
>> >>  {
>> >> -       return -ENOSYS;
>> >> +       return gpio;
>> >
>> > "GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
>> > can't do 1:1 mapping. :-(
>>
>> I changed the GPIO numbers inside of Linux to match the virqs.
>>
>>       ofchip->gc.base             = IRQ_GPIO_WKUP(0);
>
> Well, I didn't say that it will not work, what I'm trying to say
> is that I don't quite like the idea of 1:1 mapping for all gpio
> chips.
>
> It is error prone, i.e. gpio_to_irq() can't fail, so you can't
> tell if gpio to irq translation really happened or not. Plus
> we might decide to not do 1:1 mapping for other gpio chips.

>From reading the ARM code my understanding is that gpio_to_irq() and
irq_to_gpio() are meant to be fast paths without error checking.  In
the gpiolib doc it says these functions should only take a couple of
instructions.

You'll detect errors if you take an invalid IRQ from the function and
feed it into any of the rest of the IRQ API.
The idea for setting the GPIO number equal to the VIRQ number came out
of the ARM code. Making GPIO==VIRQ greatly simplified the code.


>
> I think that this translation should go via gpiolib's callback
> (there is no .to_irq callback, but we should implement one).
>
> In the callback you can still do 1:1 mapping, but this mapping
> will work only for this particular gpio chip, and not for others.
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Anton Vorontsov]

On Tue, Sep 16, 2008 at 09:36:22AM -0400, Jon Smirl wrote:
[...]
> >> >> -/*
> >> >> - * Not implemented, yet.
> >> >> - */
> >> >> -static inline int gpio_to_irq(unsigned int gpio)
> >> >> +static inline unsigned int gpio_to_irq(unsigned int gpio)
> >> >>  {
> >> >> -       return -ENOSYS;
> >> >> +       return gpio;
> >> >
> >> > "GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
> >> > can't do 1:1 mapping. :-(
> >>
> >> I changed the GPIO numbers inside of Linux to match the virqs.
> >>
> >>       ofchip->gc.base             = IRQ_GPIO_WKUP(0);
> >
> > Well, I didn't say that it will not work, what I'm trying to say
> > is that I don't quite like the idea of 1:1 mapping for all gpio
> > chips.
> >
> > It is error prone, i.e. gpio_to_irq() can't fail, so you can't
> > tell if gpio to irq translation really happened or not. Plus
> > we might decide to not do 1:1 mapping for other gpio chips.
> 
> From reading the ARM code my understanding is that gpio_to_irq() and
> irq_to_gpio() are meant to be fast paths without error checking.

Nope. gpio_to_irq and irq_to_gpio don't have to be fast.

> In
> the gpiolib doc it says these functions should only take a couple of
> instructions.

This is for gpio_get/set_value and gpio_set_direction*.

> You'll detect errors if you take an invalid IRQ from the function and
> feed it into any of the rest of the IRQ API.

Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
valid virq b/c it is mapped for another IRQ controller (particularly
lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
the PIC and IRQ8 is widely used on PowerPC).

So gpio_to_irq will succeed, and request_irq will succeed too. Though
this would be not correct.

> The idea for setting the GPIO number equal to the VIRQ number came out
> of the ARM code. Making GPIO==VIRQ greatly simplified the code.

Adding .to_irq callback won't complicate code either, but will let
you do the things right.

As for the ARM code... ARM has been using GPIO API long before
GPIOLIB, and back then you didn't have many options.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 10:14 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Sep 16, 2008 at 09:36:22AM -0400, Jon Smirl wrote:
> [...]
>> >> >> -/*
>> >> >> - * Not implemented, yet.
>> >> >> - */
>> >> >> -static inline int gpio_to_irq(unsigned int gpio)
>> >> >> +static inline unsigned int gpio_to_irq(unsigned int gpio)
>> >> >>  {
>> >> >> -       return -ENOSYS;
>> >> >> +       return gpio;
>> >> >
>> >> > "GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
>> >> > can't do 1:1 mapping. :-(
>> >>
>> >> I changed the GPIO numbers inside of Linux to match the virqs.
>> >>
>> >>       ofchip->gc.base             = IRQ_GPIO_WKUP(0);
>> >
>> > Well, I didn't say that it will not work, what I'm trying to say
>> > is that I don't quite like the idea of 1:1 mapping for all gpio
>> > chips.
>> >
>> > It is error prone, i.e. gpio_to_irq() can't fail, so you can't
>> > tell if gpio to irq translation really happened or not. Plus
>> > we might decide to not do 1:1 mapping for other gpio chips.
>>
>> From reading the ARM code my understanding is that gpio_to_irq() and
>> irq_to_gpio() are meant to be fast paths without error checking.
>
> Nope. gpio_to_irq and irq_to_gpio don't have to be fast.
>
>> In
>> the gpiolib doc it says these functions should only take a couple of
>> instructions.
>
> This is for gpio_get/set_value and gpio_set_direction*.
>
>> You'll detect errors if you take an invalid IRQ from the function and
>> feed it into any of the rest of the IRQ API.
>
> Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
> valid virq b/c it is mapped for another IRQ controller (particularly
> lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
> the PIC and IRQ8 is widely used on PowerPC).

Set the base in the GPIO struct such that this won't happen.  You can
set the base greater than MAX_IRQ.

Just because a CPU has a GPIO 8 in the manual does not mean that is
has to be assigned GPIO 8 in the kernel. The GPIO numbers in the
kernel are just handles, they can be any number. VIRQs don't use the
physical IRQ number.

In the code I posted all of the internal GPIO numbers were in the
150-180 range.

Assign GPIO numbers above the maximum VIRQ on the CPU...

#define IRQ_GPIO(x)  (MPC52xx_IRQ_HIGHTESTHWIRQ + x)
#define IRQ_GPIO_WKUP(x)  (IRQ_GPIO(32) + x)

Use the base of the GPIO struct to translate to the internal pin number.
	ofchip->gc.base             = IRQ_GPIO_WKUP(0);


static int mpc52xx_wkup_gpio_get(struct gpio_chip *gc, unsigned int gpio)
{
	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
	struct mpc52xx_gpio_wkup __iomem *regs = mm_gc->regs;
	unsigned int ret;

Adjust for the offset .,..
	ret = (in_8(&regs->wkup_ival) >> (7 - gpio - gc.base)) & 1;

	pr_debug("%s: gpio: %d ret: %d\n", __func__, gpio, ret);

	return ret;
}



>
> So gpio_to_irq will succeed, and request_irq will succeed too. Though
> this would be not correct.
>
>> The idea for setting the GPIO number equal to the VIRQ number came out
>> of the ARM code. Making GPIO==VIRQ greatly simplified the code.
>
> Adding .to_irq callback won't complicate code either, but will let
> you do the things right.
>
> As for the ARM code... ARM has been using GPIO API long before
> GPIOLIB, and back then you didn't have many options.
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Scott Wood]

On Tue, Sep 16, 2008 at 10:24:35AM -0400, Jon Smirl wrote:
> On Tue, Sep 16, 2008 at 10:14 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
> > valid virq b/c it is mapped for another IRQ controller (particularly
> > lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
> > the PIC and IRQ8 is widely used on PowerPC).
> 
> Set the base in the GPIO struct such that this won't happen.  You can
> set the base greater than MAX_IRQ.

And then you'll conflict with some other subsystem that decides to engage
in the same shenanigans.

Just allocate a chunk of virq space like any other cascaded IRQ
controller.

-Scott

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 1:49 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, Sep 16, 2008 at 10:24:35AM -0400, Jon Smirl wrote:
>> On Tue, Sep 16, 2008 at 10:14 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
>> > valid virq b/c it is mapped for another IRQ controller (particularly
>> > lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
>> > the PIC and IRQ8 is widely used on PowerPC).
>>
>> Set the base in the GPIO struct such that this won't happen.  You can
>> set the base greater than MAX_IRQ.
>
> And then you'll conflict with some other subsystem that decides to engage
> in the same shenanigans.

That comment was target at GPIO's that don't support interrupts. Give
those GPIO numbers greater than MAX_IRQ in case someone tries to use
them with the IRQ subsystem. Then they'll get errors.

> Just allocate a chunk of virq space like any other cascaded IRQ
> controller.

That is what I did.

>
> -Scott
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Anton Vorontsov]

On Tue, Sep 16, 2008 at 02:32:45PM -0400, Jon Smirl wrote:
> On Tue, Sep 16, 2008 at 1:49 PM, Scott Wood <scottwood@freescale.com> wrote:
> > On Tue, Sep 16, 2008 at 10:24:35AM -0400, Jon Smirl wrote:
> >> On Tue, Sep 16, 2008 at 10:14 AM, Anton Vorontsov
> >> <avorontsov@ru.mvista.com> wrote:
> >> > Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
> >> > valid virq b/c it is mapped for another IRQ controller (particularly
> >> > lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
> >> > the PIC and IRQ8 is widely used on PowerPC).
> >>
> >> Set the base in the GPIO struct such that this won't happen.  You can
> >> set the base greater than MAX_IRQ.
> >
> > And then you'll conflict with some other subsystem that decides to engage
> > in the same shenanigans.
> 
> That comment was target at GPIO's that don't support interrupts. Give
> those GPIO numbers greater than MAX_IRQ in case someone tries to use
> them with the IRQ subsystem. Then they'll get errors.

Or we can do the right thing, without messing all other gpio
controllers, i.e. implementing MAX_IRQ hacks. Right?

I still don't see any problems with .to_irq callback, can you
point out any?

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 5:42 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Sep 16, 2008 at 02:32:45PM -0400, Jon Smirl wrote:
>> On Tue, Sep 16, 2008 at 1:49 PM, Scott Wood <scottwood@freescale.com> wrote:
>> > On Tue, Sep 16, 2008 at 10:24:35AM -0400, Jon Smirl wrote:
>> >> On Tue, Sep 16, 2008 at 10:14 AM, Anton Vorontsov
>> >> <avorontsov@ru.mvista.com> wrote:
>> >> > Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
>> >> > valid virq b/c it is mapped for another IRQ controller (particularly
>> >> > lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
>> >> > the PIC and IRQ8 is widely used on PowerPC).
>> >>
>> >> Set the base in the GPIO struct such that this won't happen.  You can
>> >> set the base greater than MAX_IRQ.
>> >
>> > And then you'll conflict with some other subsystem that decides to engage
>> > in the same shenanigans.
>>
>> That comment was target at GPIO's that don't support interrupts. Give
>> those GPIO numbers greater than MAX_IRQ in case someone tries to use
>> them with the IRQ subsystem. Then they'll get errors.
>
> Or we can do the right thing, without messing all other gpio
> controllers, i.e. implementing MAX_IRQ hacks. Right?
>
> I still don't see any problems with .to_irq callback, can you
> point out any?


You have to map between GPIO and IRQ inside the interrupt handlers so
it has to be reasonably fast. This gets done on every shared interrupt
so you will end up building mapping tables. Also, gpio_to_irq()
doesn't take the gpio chip struct as a parameter.

Why does this mess with all of ther GPIO controllers? If they generate
interrupts they obviously have to coordinate with the VIRQ system.
This may be an issue with the way gpio lib is designed, the API for
the library assumes all gpios in the system are assigned unique
identifiers.

Is there any other problem with 1:1 other than it doesn't return an
error if gpio_to_irq() is called with a gpio number that doesn't
support irqs? You could always implement gpio_to_irq() like this:

if (gpio < MAX_HW_IRQ)
   return -ENOSYSl
return gpio;

Sure your proposal works too, it's just more complicated. 1:1 mapping
is working for ARM, why does PowerPC need to be different? I initially
started coding it the way you propose but then I stumbled across the
ARM solution and it was way simpler.


-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Scott Wood]

On Tue, Sep 16, 2008 at 06:08:34PM -0400, Jon Smirl wrote:
> You have to map between GPIO and IRQ inside the interrupt handlers so
> it has to be reasonably fast. This gets done on every shared interrupt
> so you will end up building mapping tables. Also, gpio_to_irq()
> doesn't take the gpio chip struct as a parameter.

Well, that sucks.  We should be removing magic global numberspaces, not
adding them.

> Why does this mess with all of ther GPIO controllers? If they generate
> interrupts they obviously have to coordinate with the VIRQ system.

And if they don't generate interrupts, and they decide they can also
just park themselves at MAX_IRQ?

-Scott

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 7:24 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, Sep 16, 2008 at 06:08:34PM -0400, Jon Smirl wrote:
>> You have to map between GPIO and IRQ inside the interrupt handlers so
>> it has to be reasonably fast. This gets done on every shared interrupt
>> so you will end up building mapping tables. Also, gpio_to_irq()
>> doesn't take the gpio chip struct as a parameter.
>
> Well, that sucks.  We should be removing magic global numberspaces, not
> adding them.
>
>> Why does this mess with all of ther GPIO controllers? If they generate
>> interrupts they obviously have to coordinate with the VIRQ system.
>
> And if they don't generate interrupts, and they decide they can also
> just park themselves at MAX_IRQ?

More coordination has to be going on here, as far as I can tell
gpiolib has a built in assumption that all gpios in a system are
uniquely numbered.  I guess that since most systems can't dynamically
add GPIO the problem hasn't been addressed.

It could be fixed by creating a function for obtaining the max gpio id
in use and then using it as a base for any that are added later. The
core complain here is: what happens if you assign non-interrupting
GPIOs to 1-8 which are also hardware IRQs and then use irq functions
on these non-interrupting GPIO ids. My answer to that is don't assign
GPIO ids that are legal hardware interrupts. The function for
determing max GPIO ID in use may even exist in the ARM code, I haven't
gone looking for it.

In the mpc5200 most (maybe all?)  GPIOs are capable of being an
interrupt source so a 1:1 mapping with VIRQs is the natural solution.

mpc5200_pic.h reserves 208 spots for hardware interrupts. I don't know
why, there aren't that many possible. There are 56 GPIO pins, so you
end up with 264 virqs total. PowerPC defaults to 512 virqs available.



-- 
Jon Smirl
jonsmirl@gmail.com

Re: demuxing irqs

[Anton Vorontsov]

On Tue, Sep 16, 2008 at 06:08:34PM -0400, Jon Smirl wrote:
[...]
> >> >> > Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
> >> >> > valid virq b/c it is mapped for another IRQ controller (particularly
> >> >> > lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
> >> >> > the PIC and IRQ8 is widely used on PowerPC).
> >> >>
> >> >> Set the base in the GPIO struct such that this won't happen.  You can
> >> >> set the base greater than MAX_IRQ.
> >> >
> >> > And then you'll conflict with some other subsystem that decides to engage
> >> > in the same shenanigans.
> >>
> >> That comment was target at GPIO's that don't support interrupts. Give
> >> those GPIO numbers greater than MAX_IRQ in case someone tries to use
> >> them with the IRQ subsystem. Then they'll get errors.
> >
> > Or we can do the right thing, without messing all other gpio
> > controllers, i.e. implementing MAX_IRQ hacks. Right?
> >
> > I still don't see any problems with .to_irq callback, can you
> > point out any?
> 
> 
> You have to map between GPIO and IRQ inside the interrupt handlers so
> it has to be reasonably fast. This gets done on every shared interrupt
> so you will end up building mapping tables.

I don't get it. The mapping for your gpio controller will be 1:1.
But only for your GPIO controller. You don't have to create any tables.

That is,

static unsigned int your_controller_gpio_to_irq(stuct gpio_chip *gc,
						unsigned int gpio)
{
	return gc->base + gpio; /* guaranteed for this particular
				   irq/gpio controller bundle, because
				   gc->base == virq_base AND we
				   use 1:1 mapping. */
}

gpio_chip->to_irq = your_controller_gpio_to_irq;

Where is the table?

> Also, gpio_to_irq()
> doesn't take the gpio chip struct as a parameter.

You don't need this, since gpio_to_irq will call gpiolib's
__gpio_to_irq(), and gpiolib will call gpio_to_chip() to get the
chip struct. The approach is the same as we do for
gpio_{get,set}_value via gpiolib.

> Why does this mess with all of ther GPIO controllers? If they generate
> interrupts they obviously have to coordinate with the VIRQ system.

Btw, why do you need the gpio_to_irq call in the first place?
Why don't you just configure a gpio to serve as an interrupt source
(inside one of irq_host_ops), and just specify "interrupts = <>"
along side with "gpios = <>" in the "ir" node?

For example,

	gpio_wkup: gpio-wkup@c00 {
		compatible = "fsl,mpc5200b-gpio-wkup","fsl,mpc5200-gpio-wkup";
		reg = <0xc00 0x40>;
		interrupts = <0x1 0x8 0x0 0x0 0x3 0x0>;
		interrupt-parent = <&mpc5200_pic>;
		gpio-controller;
		#gpio-cells = <2>;
		interrupt-controller; <-- added
		#interrupt-cells = <2>; <-- added
	};

	ir {
		interrupts = <0 1>; <-- notice that irq-specific flags
		                         placed where they should.
		interrupt-parent = <&gpio_wkup>;
		gpios = <&gpio_w 0 0>; <-- notice that 1:1 mapping is explicit
	};

...and you don't need the gpio_to_irq.

(Plus. I would rather split the gpio-wkup node into two:
interrupt-controller, and gpio-controller).

> This may be an issue with the way gpio lib is designed, the API for
> the library assumes all gpios in the system are assigned unique
> identifiers.
> 
> Is there any other problem with 1:1 other than it doesn't return an
> error if gpio_to_irq() is called with a gpio number that doesn't
> support irqs?

Yes. We might want non-1:1 mapping for other gpio controllers.

And we can't handle gpio_to_irq() for GPIO0 (yes, sure, we can
implement another hack: reserve GPIO0 for no use. ;-)

> You could always implement gpio_to_irq() like this:
> 
> if (gpio < MAX_HW_IRQ)
>    return -ENOSYSl
> return gpio;

Don't know anything about MAX_HW_IRQ... maybe NR_IRQS? I heard
some rumors about making NR_IRQS dynamic...

> Sure your proposal works too, it's just more complicated. 1:1 mapping
> is working for ARM, why does PowerPC need to be different? I initially
> started coding it the way you propose but then I stumbled across the
> ARM solution and it was way simpler.

I don't see why adding one more gpiolib callback would complicate
things. Today you're _forcing_ every gpio controller to have 1:1
irq:gpio mapping. I think later we will encounter more problems
with it and then we will blame our lack of foresight...

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: demuxing irqs

[Jon Smirl]

Both solutions can be implemented. This is really a question of style.
Since this impacts all of the PowerPC implementations we should get
feedback from more people  I'll go along with whatever the group
wants.

The basic question is: Should GPIOs and VIRQs each have their own
namespace, or should there be a single unified namespace?

-- 
Jon Smirl
jonsmirl@gmail.com

RE: demuxing irqs

[Stephen Neuendorffer]

It doesn't seem to me that the problem (hierarchical interrupts) is one
that only happens with GPIOs, so why treat them specially?  Since there
seems to be a reasonable solution that keeps them in a
separate namespace, that seems like the better way to go...

Steve

> -----Original Message-----
> From: linuxppc-dev-bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org] On Behalf Of Jon
Smirl
> Sent: Wednesday, September 17, 2008 7:09 AM
> To: avorontsov@ru.mvista.com
> Cc: Scott Wood; linuxppc-dev; Roland Dreier
> Subject: Re: demuxing irqs
> =

> Both solutions can be implemented. This is really a question of style.
> Since this impacts all of the PowerPC implementations we should get
> feedback from more people  I'll go along with whatever the group
> wants.
> =

> The basic question is: Should GPIOs and VIRQs each have their own
> namespace, or should there be a single unified namespace?
> =

> --
> Jon Smirl
> jonsmirl@gmail.com
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

Re: demuxing irqs

[Jon Smirl]

On Tue, Sep 16, 2008 at 10:14 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Sep 16, 2008 at 09:36:22AM -0400, Jon Smirl wrote:
> [...]
>> >> >> -/*
>> >> >> - * Not implemented, yet.
>> >> >> - */
>> >> >> -static inline int gpio_to_irq(unsigned int gpio)
>> >> >> +static inline unsigned int gpio_to_irq(unsigned int gpio)
>> >> >>  {
>> >> >> -       return -ENOSYS;
>> >> >> +       return gpio;
>> >> >
>> >> > "GPIO 0" is valid gpio, but "IRQ 0" isn't valid virq. So you
>> >> > can't do 1:1 mapping. :-(
>> >>
>> >> I changed the GPIO numbers inside of Linux to match the virqs.
>> >>
>> >>       ofchip->gc.base             = IRQ_GPIO_WKUP(0);
>> >
>> > Well, I didn't say that it will not work, what I'm trying to say
>> > is that I don't quite like the idea of 1:1 mapping for all gpio
>> > chips.
>> >
>> > It is error prone, i.e. gpio_to_irq() can't fail, so you can't
>> > tell if gpio to irq translation really happened or not. Plus
>> > we might decide to not do 1:1 mapping for other gpio chips.
>>
>> From reading the ARM code my understanding is that gpio_to_irq() and
>> irq_to_gpio() are meant to be fast paths without error checking.
>
> Nope. gpio_to_irq and irq_to_gpio don't have to be fast.

You need to do gpio_to_irq in the interrupt handler doing the
demuxing. I hadn't written that piece yet.


>
>> In
>> the gpiolib doc it says these functions should only take a couple of
>> instructions.
>
> This is for gpio_get/set_value and gpio_set_direction*.
>
>> You'll detect errors if you take an invalid IRQ from the function and
>> feed it into any of the rest of the IRQ API.
>
> Assume that GPIO 8 does not translate to any IRQ, but IRQ 8 is still
> valid virq b/c it is mapped for another IRQ controller (particularly
> lots of kernel code assumes that IRQ 8 is 8259 PIC's CMOS interrupt,
> the PIC and IRQ8 is widely used on PowerPC).
>
> So gpio_to_irq will succeed, and request_irq will succeed too. Though
> this would be not correct.
>
>> The idea for setting the GPIO number equal to the VIRQ number came out
>> of the ARM code. Making GPIO==VIRQ greatly simplified the code.
>
> Adding .to_irq callback won't complicate code either, but will let
> you do the things right.
>
> As for the ARM code... ARM has been using GPIO API long before
> GPIOLIB, and back then you didn't have many options.
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>



-- 
Jon Smirl
jonsmirl@gmail.com