[PATCH/2.6.17-rc4 4/10]Powerpc: Add tsi108 pic support

[Alexandre Bounine]

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
> Sent: Tuesday, June 06, 2006 6:17 AM
> To: Zang Roy-r61911
> Cc: Alexandre Bounine; Kumar Gala; linuxppc-dev list; Yang
> Xin-Xin-r48390; Paul Mackerras
> Subject: RE: [PATCH/2.6.17-rc4 4/10]Powerpc: Add tsi108 pic support
> 
> 
> On Tue, 2006-06-06 at 17:43 +0800, Zang Roy-r61911 wrote:
> 
> > Update Tsi108 implementation of MPIC.
> > Any comment? 
> > 
> > Integrate Tundra Semiconductor tsi108 host bridge interrupt 
> controller 
> > to mpic arch.
> 
> Looks much better :) Still a few things... 
>

Sounds good. We are moving in right direction :)
 
> > +	mpic = mpic_alloc(mpic_paddr,
> > +			MPIC_PRIMARY | MPIC_BIG_ENDIAN | 
> MPIC_WANTS_RESET |
> > +			MPIC_SPV_EOI | MPIC_CASC_NOEOI | 
> > +			MPIC_MOD_ID(MPIC_ID_TSI108),
> > +			0, /* num_sources used */
> > +			TSI108_IRQ_BASE,
> > +			0, /* num_sources used */
> > +			NR_IRQS - 4 /* XXXX */,
> > +			mpc7448_hpc2_pic_initsenses,
> > +			sizeof(mpc7448_hpc2_pic_initsenses), 
> "Tsi108_PIC");
> 
> That's a hell lot of new flags... I'm not sure we need that many or a
> single TSI108 one that encloses all the new ones. Also, I'm 
> not sure we
> need that model ID encoding thing. Let's do things simple, besides, I
> don't want to encourage HW folks into doing the same kind of 
> contraption
> in the future

More details in comments below.

>(btw, tell the TSI folks for me that they had a BAD BAD
> BAD idea to muck around with the base design that way, especially
> changing the register map in incompatible ways for no good reason).
> 

Done!

> > +	/* Configure MPIC outputs to CPU0 */
> > +	tsi108_write_reg(TSI108_MPIC_OFFSET + 0x30c, 0);
> >  }
> 
> It doesn't use the standard multiple processor outputs mecanism of
> MPIC ?
>  
> > +static struct mpic_info mpic_infos[] = {
> > +	[0] = {	/* Original OpenPIC compatible MPIC */
> > +	.greg_base	= MPIC_GREG_BASE,
> > +	.greg_frr0	= MPIC_GREG_FEATURE_0,
> > +	.greg_config0	= MPIC_GREG_GLOBAL_CONF_0,
> > +	.greg_vendor_id	= MPIC_GREG_VENDOR_ID,
> > +	.greg_ipi_vp0	= MPIC_GREG_IPI_VECTOR_PRI_0,
> > +	.greg_ipi_stride	= MPIC_GREG_IPI_STRIDE,
> > +	.greg_spurious	= MPIC_GREG_SPURIOUS,
> > +	.greg_tfrr	= MPIC_GREG_TIMER_FREQ,
> > +
> 
>    .../...
> 
> It's a bit sad to have to go all the way to doing such tables, but I
> suspect it's probably the best way to handle it at this 
> point.

> Send more
> nastygrams to the HW folks for me.
> 

Done:)

> >  	mpic->num_sources = 0; /* so far */
> >  	mpic->senses = senses;
> >  	mpic->senses_count = senses_count;
> > +	mpic->hw_set = &mpic_infos[MPIC_GET_MOD_ID(flags)];
> 
> Well... the model ID thing might not be that a bad idea in 
> the end :) I
> need to think about it. I might have to deal with yet another 
> MPIC that
> has another regiser map (yeah yeah, TSI aren't the only ones 
> to not get
> it)... 
>

I'll tell this to HW guys as well :) 

>   .../...
> 
> > @@ -963,7 +1043,7 @@ int mpic_get_one_irq(struct mpic *mpic, 
> >  {
> >  	u32 irq;
> >  
> > -	irq = mpic_cpu_read(MPIC_CPU_INTACK) & MPIC_VECPRI_VECTOR_MASK;
> > +	irq = mpic_cpu_read(mpic->hw_set->cpu_intack) & 
> mpic->hw_set->irq_vpr_vector;
> >  #ifdef DEBUG_LOW
> >  	DBG("%s: get_one_irq(): %d\n", mpic->name, irq);
> >  #endif
> > @@ -972,11 +1052,18 @@ #ifdef DEBUG_LOW
> >  		DBG("%s: cascading ...\n", mpic->name);
> >  #endif
> >  		irq = mpic->cascade(regs, mpic->cascade_data);
> > -		mpic_eoi(mpic);
> > +#ifdef DEBUG_LOW
> > +		DBG("%s: cascaded irq: %d\n", mpic->name, irq);
> > +#endif
> > +		if (!(mpic->flags & MPIC_CASC_NOEOI))
> > +			mpic_eoi(mpic);
> >  		return irq;
> >  	}
> 
> Can you tell me why you need the above ? (Why you aren't EOI'ing the
> cascade ?) Note that the cascade handling is going away from 
> mpic anyway
> with the port to genirq that I'll publish later this week for 
> 2.6.18 and
> it will almost be handled as a normal interrupt...
> 

We have a level-signalled irq from the cascaded PCI interrupt controller. If I do EOI at 
this time, level request will not have chance to be cleared (unless all PCI interrupts have
an SA_INTERRUPT flag) and result in recurring interrupts. 

I chose to have an individual flag instead of checking model ID to avoid multiple checks within ISR (in case if we have more that one mpic version requiring this option). I also expect that it may be useful for any external level-signalling cascades connected to MPIC.      

> > -	if (unlikely(irq == MPIC_VEC_SPURRIOUS))
> > +	if (unlikely(irq == MPIC_VEC_SPURRIOUS)) {
> > +		if (mpic->flags & MPIC_SPV_EOI)
> > +			mpic_eoi(mpic);
> >  		return -1;
> > +	}
> 
> I think the above thing could just test the model ID. It's 
> unlikely that
> another implementation need the same "feature", so just test the model
> ID rather than adding a flag and if we ever have another 
> model with the
> same "feature", then we'll go back to adding a flag :)
> 

Motivation is the same as above - I just do not want to have multiple ID checks here. I agree that it is driven by mpic type (model ID) only. I can remove this one if you do not expect any
new "broken" MPICs on horizon.  

> Cheers,
> Ben.
> 
Thanks for your feedback,
Alex.
> 
>