Re: [PATCH 3/8] MIPS: Octeon: Add irq_create_of_mapping() and GPIOinterrupts.

[David Daney]

On 11/11/2011 08:08 AM, Rob Herring wrote:
> On 11/10/2011 08:21 PM, ddaney.cavm@xxxxxxxxx wrote:
> > From: David Daney<david.daney@xxxxxxxxxx>
> >
> > This is needed for Octeon to use the Device Tree.
> >
> > The GPIO interrupts are configured based on Device Tree properties
> >
> > Signed-off-by: David Daney<david.daney@xxxxxxxxxx>
> > ---
> >   arch/mips/cavium-octeon/octeon-irq.c |  188 +++++++++++++++++++++++++++++++++-
> >   1 files changed, 187 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/mips/cavium-octeon/octeon-irq.c b/arch/mips/cavium-octeon/octeon-irq.c
> > index ffd4ae6..bb10546 100644
> > --- a/arch/mips/cavium-octeon/octeon-irq.c
> > +++ b/arch/mips/cavium-octeon/octeon-irq.c
> > @@ -8,11 +8,14 @@
> >
> >   #include<linux/interrupt.h>
> >   #include<linux/bitops.h>
> > +#include<linux/module.h>
> >   #include<linux/percpu.h>
> > +#include<linux/of_irq.h>
> >   #include<linux/irq.h>
> >   #include<linux/smp.h>
> >
> >   #include<asm/octeon/octeon.h>
> > +#include<asm/octeon/cvmx-gpio-defs.h>
> >
> >   static DEFINE_RAW_SPINLOCK(octeon_irq_ciu0_lock);
> >   static DEFINE_RAW_SPINLOCK(octeon_irq_ciu1_lock);
> > @@ -58,6 +61,95 @@ static void __init octeon_irq_set_ciu_mapping(int irq, int line, int bit,
> >   	octeon_irq_ciu_to_irq[line][bit] = irq;
> >   }
> >
> > +static unsigned int octeon_irq_gpio_mapping(struct device_node *controller,
> > +					    const u32 *intspec,
> > +					    unsigned int intsize)
> > +{
> > +	struct of_irq oirq;
> > +	int i;
> > +	unsigned int irq = 0;
> > +	unsigned int type;
> > +	unsigned int ciu = 0, bit = 0;
> > +	unsigned int pin = be32_to_cpup(intspec);
> > +	unsigned int trigger = be32_to_cpup(intspec + 1);
> > +	bool set_edge_handler = false;
> > +
> > +	if (pin>= 16)
> > +		goto err;
> > +	i = of_irq_map_one(controller, 0,&oirq);
> > +	if (i)
> > +		goto err;
> > +	if (oirq.size != 2)
> > +		goto err_put;
> > +
> > +	ciu = oirq.specifier[0];
> > +	bit = oirq.specifier[1] + pin;
> > +
> > +	if (ciu>= 8 || bit>= 64)
> > +		goto err_put;
> > +
> > +	irq = octeon_irq_ciu_to_irq[ciu][bit];
> > +	if (!irq)
> > +		goto err_put;
> > +
> > +	switch (trigger&  0xf) {
> > +	case 1:
> > +		type = IRQ_TYPE_EDGE_RISING;
> > +		set_edge_handler = true;
> > +		break;
> > +	case 2:
> > +		type = IRQ_TYPE_EDGE_FALLING;
> > +		set_edge_handler = true;
> > +		break;
> > +	case 4:
> > +		type = IRQ_TYPE_LEVEL_HIGH;
> > +		break;
> > +	case 8:
> > +		type = IRQ_TYPE_LEVEL_LOW;
> > +		break;
> > +	default:
> > +		pr_err("Error: Invalid irq trigger specification: %x\n",
> > +		       trigger);
> > +		type = IRQ_TYPE_LEVEL_LOW;
> > +		break;
> > +	}
> > +
> > +	irq_set_irq_type(irq, type);
> > +
> > +	if (set_edge_handler)
> > +		__irq_set_handler(irq, handle_edge_irq, 0, NULL);
> > +
> > +err_put:
> > +	of_node_put(oirq.controller);
> > +err:
> > +	return irq;
> > +}
> > +
> > +/*
> > + * irq_create_of_mapping - Hook to resolve OF irq specifier into a Linux irq#
> > + *
> > + * Octeon irq maps are a pair of indexes.  The first selects either
> > + * ciu0 or ciu1, the second is the bit within the ciu register.
> > + */
> > +unsigned int irq_create_of_mapping(struct device_node *controller,
> > +				   const u32 *intspec, unsigned int intsize)
> > +{
> > +	unsigned int irq = 0;
> > +	unsigned int ciu, bit;
> > +
> > +	if (of_device_is_compatible(controller, "cavium,octeon-3860-gpio"))
> > +		return octeon_irq_gpio_mapping(controller, intspec, intsize);
> > +
> > +	ciu = be32_to_cpup(intspec);
> > +	bit = be32_to_cpup(intspec + 1);
> > +
> > +	if (ciu<  8&&  bit<  64)
> > +		irq = octeon_irq_ciu_to_irq[ciu][bit];
> > +
> > +	return irq;
> > +}
> > +EXPORT_SYMBOL_GPL(irq_create_of_mapping);
>
> Have you looked at irq_domains (kernel/irq/irqdomain.c)? That is what
> you should be using for your (gpio) interrupt controller and then use
> the common irq_create_of_mapping.

Unfortunatly, although a good idea, kernel/irq/irqdomain.c makes a bunch 
of assumptions that don't hold for Octeon.  We may be able to improve it 
so that it flexible enough to suit us.


Here are the problems I see:

1) It is assumed that there is some sort of linear correspondence 
between 'hwirq' and 'irq', and that the range of valid values is contiguous.

2) It is assumed that the concepts of nr_irq, irq_base and hwirq_base 
have easy to determin values and you can do iteration over their ranges 
by adding indexes to the bases.

I think we can fix this by adding iteration helper functions to struct 
irq_domain.  If these are present, we would just ignore the irq_base, 
nr_irq and hwirq_base elements of the structure. 
irq_domain_for_each_hwirq() and irq_domain_for_each_irq() would be 
modified to use the iteration helper functions.

David Daney



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/