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

Sat Jun 3 07:30:15 EST 2006

The items 1 and 2 not needed. I moved them from the tsi108_pic.c but really they have been leftovers from the old code there. Originally code ppc/syslib/open_pic.c was used as a template, which was good fit for the blocking output mode of the tsi108 PIC (plus some extra tweaks). In that mode EOI has to be signaled to PIC to deactivate interrupt line to the CPU before it re-enables local interrupts. This is why we have got ack routine. We changed mode later and removed most of workarounds except these two.

I removed code for 1 and 2 and will send a patch to Roy after retesting (probably this weekend
with some other stuff).

Alex.

-----Original Message-----
From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
Sent: Thursday, June 01, 2006 6:06 PM
To: Alexandre Bounine
Cc: Kumar Gala; Zang Roy-r61911; linuxppc-dev list; Yang Xin-Xin-r48390;
Paul Mackerras
Subject: RE: [PATCH/2.6.17-rc4 4/10]Powerpc: Add tsi108 pic support

On Thu, 2006-06-01 at 16:45 -0400, Alexandre Bounine wrote:
> All differences in the Tsi108/109 PIC code from the standard MPIC are caused by the HW behavior. The Tsi108/109 PIC looks like standard OpenPIC but, in fact, is different in registers mapping and behavior. Its logic is close but not exactly as MPIC.  
> 
> Here are replies on comments to the code:
> 
> 1.Why do you have to check if its a LEVEL irq?
> 
> Check for LEVEL irqs is required in the ack/end pair to enable nested interrupt servicing and
> does not hang when core (local) interrupts are re-enabled in the ISR. Otherwise we have to use 
> SA_INTERRUPT flag for all level signaled interrupts.

Can you be more precise about what exactly happens and why ? Unless you
EOI handling is broken of course, there should be no need to do anything
other than a single eoi in end(), period.

> 2. if the PIC works like other openpic's you dont need an 'ack' we  
> handle it via 'end'
> 
> Tsi108/109 needs it.

What for ? Please, give the low level details.

> 3. why the changes to where we do mpic_eoi for TSI108?
> The Tsi108 PIC requires EOI for spurious interrupt (as all other interrupt sources).

Ok, that is acceptable.

> The do_IRQ() does not call end routine for spurious interrupts.  
> 
> The MPIC code patch for Tsi108/109 demonstrates HW differences and why we originally considered having separate code for Tsi108 pic.

Please tell me more about the HW differences :)

Ben.

> 
> 
> -----Original Message-----
> From: Kumar Gala [mailto:galak at kernel.crashing.org]
> Sent: Tuesday, May 30, 2006 3:18 PM
> To: Zang Roy-r61911
> Cc: Benjamin Herrenschmidt; linuxppc-dev list; Yang Xin-Xin-r48390; Paul
> Mackerras; Alexandre Bounine
> Subject: Re: [PATCH/2.6.17-rc4 4/10]Powerpc: Add tsi108 pic support
> 
> 
> 
> On May 29, 2006, at 10:28 PM, Zang Roy-r61911 wrote:
> 
> >>
> >> On Wed, 2006-05-17 at 18:14 +0800, Zang Roy-r61911 wrote:
> >>> Add Tundra Semiconductor tsi108 host bridge interrupt
> >> controller support.
> >>
> >> It looks a bit like an hacked up MPIC... Is it different
> >> enough to justify a separate driver ? Or would it be possible
> >> to define a TSI108 flag to pass the current mpic driver and
> >> add the necessary bits to it ?
> >>
> >
> > Tsi108 implementation of MPIC has many differences form the  
> > original one,  the following
> > code implements it with mpic. Any comment? The following patch is  
> > just based on
> > my previous send out patches.
> 
> In the future please provide it against the base, much easier to read.
> 
> > Integrate Tundra Semiconductor tsi108 host bridge interrupt controller
> > to mpic arch.
> >
> > Signed-off-by: Alexandre Bounine <alexandreb at tundra.com>
> >
> 
> [snip]
> 
> >
> > --- linux-2.6.17-rc4-tun/arch/powerpc/sysdev/mpic.c	2006-03-20  
> > 00:53:29.000000000 -0500
> > +++ linux-2.6.17-rc4-hpc2/arch/powerpc/sysdev/mpic.c	2006-05-29  
> > 16:07:03.000000000 -0400
> > @@ -81,7 +81,7 @@ static inline void _mpic_write(unsigned
> >  static inline u32 _mpic_ipi_read(struct mpic *mpic, unsigned int ipi)
> >  {
> >  	unsigned int be = (mpic->flags & MPIC_BIG_ENDIAN) != 0;
> > -	unsigned int offset = MPIC_GREG_IPI_VECTOR_PRI_0 + (ipi * 0x10);
> > +	unsigned int offset = MPIC_GREG_IPI_VECTOR_PRI_0 + (ipi *  
> > MPIC_GREG_IPI_STRIDE);
> >
> >  	if (mpic->flags & MPIC_BROKEN_IPI)
> >  		be = !be;
> > @@ -90,7 +90,7 @@ static inline u32 _mpic_ipi_read(struct
> >
> >  static inline void _mpic_ipi_write(struct mpic *mpic, unsigned int  
> > ipi, u32 value)
> >  {
> > -	unsigned int offset = MPIC_GREG_IPI_VECTOR_PRI_0 + (ipi * 0x10);
> > +	unsigned int offset = MPIC_GREG_IPI_VECTOR_PRI_0 + (ipi *  
> > MPIC_GREG_IPI_STRIDE);
> >
> >  	_mpic_write(mpic->flags & MPIC_BIG_ENDIAN, mpic->gregs, offset,  
> > value);
> >  }
> > @@ -393,7 +393,11 @@ static inline struct mpic * mpic_from_ir
> >  static inline void mpic_eoi(struct mpic *mpic)
> >  {
> >  	mpic_cpu_write(MPIC_CPU_EOI, 0);
> > +#ifndef CONFIG_TSI108_BRIDGE
> >  	(void)mpic_cpu_read(MPIC_CPU_WHOAMI);
> > +#else
> > +	(void)mpic_cpu_read(MPIC_CPU_OUTPUT);
> > +#endif
> >  }
> >
> >  #ifdef CONFIG_SMP
> > @@ -514,9 +518,26 @@ static void mpic_end_irq(unsigned int ir
> >  	}
> >  #endif /* CONFIG_MPIC_BROKEN_U3 */
> >
> > +#ifdef CONFIG_TSI108_BRIDGE
> > +	if ((irq_desc[irq].status & IRQ_LEVEL) != 0)
> > +#endif
> 
> Why do you have to check if its a LEVEL irq?
> 
> >  	mpic_eoi(mpic);
> >  }
> >
> > +#ifdef CONFIG_TSI108_BRIDGE
> > +static void mpic_ack_irq(unsigned int irq)
> > +{
> > +	struct mpic *mpic = mpic_from_irq(irq);
> > +
> > +#ifdef DEBUG_IRQ
> > +	DBG("%s: ack_irq: %d\n", mpic->name, irq);
> > +#endif
> > +
> > +	if ((irq_desc[irq].status & IRQ_LEVEL) == 0)
> > +		mpic_eoi(mpic);
> > +}
> > +#endif /* CONFIG_TSI108_BRIDGE */
> > +
> 
> if the PIC works like other openpic's you dont need an 'ack' we  
> handle it via 'end'
> 
> >  #ifdef CONFIG_SMP
> >
> >  static void mpic_enable_ipi(unsigned int irq)
> > @@ -596,6 +617,9 @@ struct mpic * __init mpic_alloc(unsigned
> >  	mpic->hc_irq.enable = mpic_enable_irq;
> >  	mpic->hc_irq.disable = mpic_disable_irq;
> >  	mpic->hc_irq.end = mpic_end_irq;
> > +#ifdef CONFIG_TSI108_BRIDGE
> > +	mpic->hc_irq.ack = mpic_ack_irq;
> > +#endif
> >  	if (flags & MPIC_PRIMARY)
> >  		mpic->hc_irq.set_affinity = mpic_set_affinity;
> >  #ifdef CONFIG_SMP
> > @@ -955,8 +979,13 @@ void mpic_send_ipi(unsigned int ipi_no,
> >  	DBG("%s: send_ipi(ipi_no: %d)\n", mpic->name, ipi_no);
> >  #endif
> >
> > +#ifndef CONFIG_TSI108_BRIDGE
> >  	mpic_cpu_write(MPIC_CPU_IPI_DISPATCH_0 + ipi_no * 0x10,
> >  		       mpic_physmask(cpu_mask & cpus_addr(cpu_online_map)[0]));
> > +#else /* CONFIG_TSI108_BRIDGE */
> > +	mpic_write(mpic->gregs, MPIC_CPU_IPI_DISPATCH,
> > +		       mpic_physmask(cpu_mask & cpus_addr(cpu_online_map)[0]));
> > +#endif /* !CONFIG_TSI108_BRIDGE */
> >  }
> >
> >  int mpic_get_one_irq(struct mpic *mpic, struct pt_regs *regs)
> > @@ -972,11 +1001,20 @@ int mpic_get_one_irq(struct mpic *mpic,
> >  		DBG("%s: cascading ...\n", mpic->name);
> >  #endif
> >  		irq = mpic->cascade(regs, mpic->cascade_data);
> > +#ifdef DEBUG_LOW
> > +		DBG("%s: cascaded irq: %d\n", mpic->name, irq);
> > +#endif
> > +#ifndef CONFIG_TSI108_BRIDGE
> >  		mpic_eoi(mpic);
> > +#endif
> >  		return irq;
> >  	}
> > -	if (unlikely(irq == MPIC_VEC_SPURRIOUS))
> > +	if (unlikely(irq == MPIC_VEC_SPURRIOUS)) {
> > +#ifdef CONFIG_TSI108_BRIDGE
> > +		mpic_eoi(mpic);
> > +#endif
> >  		return -1;
> > +	}
> 
> why the changes to where we do mpic_eoi for TSI108?
> 
> >  	if (irq < MPIC_VEC_IPI_0) {
> >  #ifdef DEBUG_IRQ
> >  		DBG("%s: irq %d\n", mpic->name, irq + mpic->irq_offset);
> 
> 
> 
> [snip]