new IRQ work status

new IRQ work status

[Benjamin Herrenschmidt]

Here's a progress report on my new irq management work.

I've ported pseries, iseries (well tested) and cell (barely tested) at
this point, and tested various funky combinations of device-trees to
validate as much as the core as possible. I still need to port over chrp
and maple which are easy, and powermac which will be a bit more
difficult as it relies a lot more on intrs/n_intrs in the device-nodes
which I've now removed.

Hopefully, it should be finished this week. The current serie of patch
applies on top of 2.6.17-mm1. I will do a version on top of the powerpc
tree as soon as the genirq port is upstream (which hopefully should
happen soon, possibly tomorrow).

I don't yet want to post the patches on the list (too much), they can be
found at:

http://gate.crashing.org/~benh/irq-WIP/

Comments welcome.

Cheers,
Ben.

Re: new IRQ work status

[Arnd Bergmann]

On Thursday 29 June 2006 10:36, Benjamin Herrenschmidt wrote:
> http://gate.crashing.org/~benh/irq-WIP/
> 
> Comments welcome.
> 

Looks great!

> -}
> -
>  /* Get an IRQ number from the pending state register of the IIC */
> -int iic_get_irq(struct pt_regs *regs)
> +unsigned int iic_get_irq(struct pt_regs *regs)

Is unsigned the right thing here? NO_IRQ is defined as (-1).

> +
> +/*
> + * Hardcoded setup part to be compatible with older firmware. We don't
> + * associate a device-node to the hosts in this case, which mean that
> + * no mapping from the device-tree is supposed. That's ok as there is
> + * none anyway
> + */
>  static int __init setup_iic_hardcoded(void)
>  {

I'd prefer to throw out the hardcoded part from the official tree
completely. It's only needed on the very first FW versions that
were shipped and those need some other patches that we never merged
upstream anyway. So better move it to an extra patch that we keep
external for some time and eventually drop.

>  static irqreturn_t iic_ipi_action(int irq, void *dev_id, struct pt_regs
> *regs) {
> -	smp_message_recv(iic_irq_to_ipi(irq), regs);
> +	int ipi = (int)(long)dev_id;
> +	smp_message_recv(ipi, regs);
>  	return IRQ_HANDLED;
>  }

Nice one. this looks a lot cleaner than looking at the irq number here.

>  static void iic_request_ipi(int ipi, const char *name)
>  {
> -	int irq;
> -
> -	irq = iic_ipi_to_irq(ipi);
> +	int node, virq;
>
> -	/* IPIs are marked SA_INTERRUPT as they must run with irqs
> -	 * disabled */
> -	set_irq_chip_and_handler(irq, &iic_chip, handle_percpu_irq);
> -	request_irq(irq, iic_ipi_action, SA_INTERRUPT, name, NULL);
> +	for (node = 0; node < IIC_NODE_COUNT; node++) {
> +		char *rname;
> +		if (iic_hosts[node] == NULL)
> +			continue;
> +		virq = irq_create_mapping(iic_hosts[node],
> +					  iic_ipi_to_irq(ipi), 0);
> +
> +		if (virq == NO_IRQ) {
> +			printk(KERN_ERR
> +			       "iic: failed to map IPI %s on node %d\n",
> +			       name, node);
> +			continue;
> +		}
> +		rname = kzalloc(strlen(name) + 16, GFP_KERNEL);
> +		if (rname)
> +			sprintf(rname, "%s node %d", name, node);
> +		else
> +			rname = (char *)name;

Considering that name is know to be rather short and we have a fixed
number of these, can't we just put the strings into struct iic
instead of doing the kzalloc?

>  enum {
> -	IIC_EXT_OFFSET   = 0x00, /* Start of south bridge IRQs */
> -	IIC_EXT_CASCADE  = 0x20, /* There is no interrupt 32 on spider */
> -	IIC_NUM_EXT      = 0x40, /* Number of south bridge IRQs */
> -	IIC_SPE_OFFSET   = 0x40, /* Start of SPE interrupts */
> -	IIC_CLASS_STRIDE = 0x10, /* SPE IRQs per class    */
> -	IIC_IPI_OFFSET   = 0x70, /* Start of IPI IRQs */
> -	IIC_NUM_IPIS     = 0x10, /* IRQs reserved for IPI */
> -	IIC_NODE_STRIDE  = 0x80, /* Total IRQs per node   */
> +	IIC_IRQ_INVALID		= 0xff,

Where is IIC_IRQ_INVALID used?


> +			}
> +		} else if (device_is_compatible(dn, "sti,platform-spider-pic")
> +			   && (chip < 2)) {
> +			static long hard_coded_pics[] =
> +				{ 0x24000008000, 0x34000008000 };
> +			r.start = hard_coded_pics[chip];
>  		} else

See above about the hardcoded stuff.


> @@ -664,11 +688,18 @@ static int __init create_spu(struct devi
>  	if (ret)
>  		goto out_free;
>
> +	/* XXX FIXME: Those node vs. nid things are crap, we need
> +	 * only one information, but fixing that goes along with fixing
> +	 * all of the node vs chip vs thread code all over the cell
> +	 * platform. To do soon hopefully...
> +	 */
>  	spu->node = find_spu_node_id(spe);
>  	spu->nid = of_node_to_nid(spe);
>  	if (spu->nid == -1)
>  		spu->nid = 0;

The idea at some point was to keep the notion of nid (as in address
on the inter-chip protocol) separate from the NUMA node ID, which
may in theory be less fine-grained. Defining them to be the same
at least requires the device tree to be very careful with the numbers
in ibm,associativity properties to match the bus numbers.


> Index: linux-work-mm/include/asm-powerpc/spu.h
> ===================================================================
> --- linux-work-mm.orig/include/asm-powerpc/spu.h	2006-06-23
> 13:38:19.000000000 +1000 +++
> linux-work-mm/include/asm-powerpc/spu.h	2006-06-29 16:32:35.000000000 +1000
> @@ -117,7 +117,7 @@ struct spu {
>  	struct list_head sched_list;
>  	int number;
>  	int nid;
> -	u32 isrc;
> +	unsigned int irqs[3];
>  	u32 node;
>  	u64 flags;
>  	u64 dar;

We've started exporting the isrc property in sysfs recently, so I think
we need to keep that in here.

	Arnd <><

Re: new IRQ work status

[Segher Boessenkool]

>>  /* Get an IRQ number from the pending state register of the IIC */
>> -int iic_get_irq(struct pt_regs *regs)
>> +unsigned int iic_get_irq(struct pt_regs *regs)
>
> Is unsigned the right thing here? NO_IRQ is defined as (-1).

It used to be -1, it's 0 now...


Segher

Re: new IRQ work status

[Benjamin Herrenschmidt]

> >  /* Get an IRQ number from the pending state register of the IIC */
> > -int iic_get_irq(struct pt_regs *regs)
> > +unsigned int iic_get_irq(struct pt_regs *regs)
> 
> Is unsigned the right thing here? NO_IRQ is defined as (-1).

One of the patches changes NO_IRQ to 0 and generally virtual irq numbers
to unsigned int

> I'd prefer to throw out the hardcoded part from the official tree
> completely. It's only needed on the very first FW versions that
> were shipped and those need some other patches that we never merged
> upstream anyway. So better move it to an extra patch that we keep
> external for some time and eventually drop.

Ok, will move it to a separate patch then.

> >  static irqreturn_t iic_ipi_action(int irq, void *dev_id, struct pt_regs
> > *regs) {
> > -	smp_message_recv(iic_irq_to_ipi(irq), regs);
> > +	int ipi = (int)(long)dev_id;
> > +	smp_message_recv(ipi, regs);
> >  	return IRQ_HANDLED;
> >  }
> 
> Nice one. this looks a lot cleaner than looking at the irq number here.

Yeah, I had the calculation wrong at first :) the irq number you get
here is the virtual one. So you 'd need to convert it through the
reverse map. With the above trick, I avoid that.

> Considering that name is know to be rather short and we have a fixed
> number of these, can't we just put the strings into struct iic
> instead of doing the kzalloc?

Good point, I'll look into it.

> >  enum {
> > -	IIC_EXT_OFFSET   = 0x00, /* Start of south bridge IRQs */
> > -	IIC_EXT_CASCADE  = 0x20, /* There is no interrupt 32 on spider */
> > -	IIC_NUM_EXT      = 0x40, /* Number of south bridge IRQs */
> > -	IIC_SPE_OFFSET   = 0x40, /* Start of SPE interrupts */
> > -	IIC_CLASS_STRIDE = 0x10, /* SPE IRQs per class    */
> > -	IIC_IPI_OFFSET   = 0x70, /* Start of IPI IRQs */
> > -	IIC_NUM_IPIS     = 0x10, /* IRQs reserved for IPI */
> > -	IIC_NODE_STRIDE  = 0x80, /* Total IRQs per node   */
> > +	IIC_IRQ_INVALID		= 0xff,
> 
> Where is IIC_IRQ_INVALID used?

Just to give an invalid number to the remapping core. It uses that to
fill up allocated but not-yet mapped slots 

> See above about the hardcoded stuff.

Yup.

> 
> > @@ -664,11 +688,18 @@ static int __init create_spu(struct devi
> >  	if (ret)
> >  		goto out_free;
> >
> > +	/* XXX FIXME: Those node vs. nid things are crap, we need
> > +	 * only one information, but fixing that goes along with fixing
> > +	 * all of the node vs chip vs thread code all over the cell
> > +	 * platform. To do soon hopefully...
> > +	 */
> >  	spu->node = find_spu_node_id(spe);
> >  	spu->nid = of_node_to_nid(spe);
> >  	if (spu->nid == -1)
> >  		spu->nid = 0;
> 
> The idea at some point was to keep the notion of nid (as in address
> on the inter-chip protocol) separate from the NUMA node ID, which
> may in theory be less fine-grained. Defining them to be the same
> at least requires the device tree to be very careful with the numbers
> in ibm,associativity properties to match the bus numbers.

Yeah, I've discussed that with Paul and Anton, I want to do a generic
layer to take care of all these conversions with a 4 level
representation:

 - thread -> match linux CPU numbers
 - core
 - chip
 - node

Anyway, that's the sort of cleanup I'll do later.

> > Index: linux-work-mm/include/asm-powerpc/spu.h
> > ===================================================================
> > --- linux-work-mm.orig/include/asm-powerpc/spu.h	2006-06-23
> > 13:38:19.000000000 +1000 +++
> > linux-work-mm/include/asm-powerpc/spu.h	2006-06-29 16:32:35.000000000 +1000
> > @@ -117,7 +117,7 @@ struct spu {
> >  	struct list_head sched_list;
> >  	int number;
> >  	int nid;
> > -	u32 isrc;
> > +	unsigned int irqs[3];
> >  	u32 node;
> >  	u64 flags;
> >  	u64 dar;
> 
> We've started exporting the isrc property in sysfs recently, so I think
> we need to keep that in here.

Why would we do that ? It makes no sense on HV platforms. Is there
anything that makes use of it ?

Ben.

Re: new IRQ work status

[Bob Brose]

Ben Said:
> 
> Here's a progress report on my new irq management work.
.
.
> validate as much as the core as possible. I still need to port over chrp
> and maple which are easy, and powermac which will be a bit more
> difficult as it relies a lot more on intrs/n_intrs in the device-nodes
> which I've now removed.
> 
> Hopefully, it should be finished this week. The current serie of patch
> applies on top of 2.6.17-mm1. I will do a version on top of the powerpc
> tree as soon as the genirq port is upstream (which hopefully should
> happen soon, possibly tomorrow).
.
. 
> Comments welcome.

Any chance you can fix the 6 slot S900 irq problem when you do the powermac
changes??

Bob

Re: new IRQ work status

[Benjamin Herrenschmidt]

On Fri, 2006-06-30 at 04:57 +0000, Bob Brose wrote:
> Ben Said:
> > 
> > Here's a progress report on my new irq management work.
> .
> .
> > validate as much as the core as possible. I still need to port over chrp
> > and maple which are easy, and powermac which will be a bit more
> > difficult as it relies a lot more on intrs/n_intrs in the device-nodes
> > which I've now removed.
> > 
> > Hopefully, it should be finished this week. The current serie of patch
> > applies on top of 2.6.17-mm1. I will do a version on top of the powerpc
> > tree as soon as the genirq port is upstream (which hopefully should
> > happen soon, possibly tomorrow).
> .
> . 
> > Comments welcome.
> 
> Any chance you can fix the 6 slot S900 irq problem when you do the powermac
> changes??

I'll look at it after I have done the initial set of patches... it's a
bug fix so there is a bit less hurry for 2.6.18 merge window

Ben.