diff mbox

powerpc/mpc52xx: Properly update irq_desc when set_type() is called.

Message ID 20090107192505.26320.23112.stgit@localhost.localdomain (mailing list archive)
State Accepted, archived
Commit d30239a0ef2000cfae6c60f8e870da2d8dba4d22
Delegated to: Grant Likely
Headers show

Commit Message

Grant Likely Jan. 7, 2009, 7:25 p.m. UTC 
From: Grant Likely <grant.likely@secretlab.ca>

The MPC5200 PIC driver doesn't correctly update the .status field of
the irq_desc structure when the set_type hook is called.  This patch
adds the required code.

Also cleans up the external IRQ typename field to be something easier
to read (very minor).

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 arch/powerpc/platforms/52xx/mpc52xx_pic.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Comments

Matt Sealey Jan. 8, 2009, 3:51 a.m. UTC | #1 
Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> The MPC5200 PIC driver doesn't correctly update the .status field of
> the irq_desc structure when the set_type hook is called.  This patch
> adds the required code.
> 
> Also cleans up the external IRQ typename field to be something easier
> to read (very minor).
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Hi Grant,

What does this affect?

-- Matt
Grant Likely Jan. 8, 2009, 4:53 a.m. UTC | #2 
On Wed, Jan 7, 2009 at 8:51 PM, Matt Sealey <matt@genesi-usa.com> wrote:
> Grant Likely wrote:
>>
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> The MPC5200 PIC driver doesn't correctly update the .status field of
>> the irq_desc structure when the set_type hook is called.  This patch
>> adds the required code.
>>
>> Also cleans up the external IRQ typename field to be something easier
>> to read (very minor).
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> Hi Grant,
>
> What does this affect?

cat /proc/interrupts

g.
Matt Sealey Jan. 8, 2009, 2:40 p.m. UTC | #3 
Grant Likely wrote:
> On Wed, Jan 7, 2009 at 8:51 PM, Matt Sealey <matt@genesi-usa.com> wrote:
>> Grant Likely wrote:
>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>
>>> The MPC5200 PIC driver doesn't correctly update the .status field of
>>> the irq_desc structure when the set_type hook is called.  This patch
>>> adds the required code.
>>>
>>> Also cleans up the external IRQ typename field to be something easier
>>> to read (very minor).
>>>
>>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> Hi Grant,
>>
>> What does this affect?
> 
> cat /proc/interrupts

Ah so, completely cosmetic in the end. I was worried I'd have to build a 
new kernel for a second.. :D

-- Matt
Grant Likely Jan. 9, 2009, 9:02 p.m. UTC | #4 
On Thu, Jan 8, 2009 at 7:40 AM, Matt Sealey <matt@genesi-usa.com> wrote:
> Grant Likely wrote:
>>
>> On Wed, Jan 7, 2009 at 8:51 PM, Matt Sealey <matt@genesi-usa.com> wrote:
>>>
>>> Grant Likely wrote:
>>>>
>>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>>
>>>> The MPC5200 PIC driver doesn't correctly update the .status field of
>>>> the irq_desc structure when the set_type hook is called.  This patch
>>>> adds the required code.
>>>>
>>>> Also cleans up the external IRQ typename field to be something easier
>>>> to read (very minor).
>>>>
>>>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>>>
>>> Hi Grant,
>>>
>>> What does this affect?
>>
>> cat /proc/interrupts
>
> Ah so, completely cosmetic in the end. I was worried I'd have to build a new
> kernel for a second.. :D

Not entirely; it affects the details about how irqs are handled.
check_irq_resend() for example.  I don't think any 5200 platforms
currently care; but it could be an issue with cascaded IRQ handlers.

g.
Matt Sealey Jan. 10, 2009, 5:19 p.m. UTC | #5 
Grant Likely wrote:
> 
> Not entirely; it affects the details about how irqs are handled.
> check_irq_resend() for example.  I don't think any 5200 platforms
> currently care; but it could be an issue with cascaded IRQ handlers.

I'm not sure if that would be a problem or not. Would it shed some light 
on why Efika ATA DMA doesn't work? :)

-- Matt Sealey
Grant Likely Jan. 14, 2009, 6:03 a.m. UTC | #6 
On Tue, Jan 13, 2009 at 6:00 AM, Matt Sealey <matt@genesi-usa.com> wrote:
> Grant Likely wrote:
>>
>>
>> Theoretically it could call IRQ handlers to be called too often.  It
>> might possibly be the cause of too many 'BAD' irqs being recorded....
>
> That might explain a few USB explosions, where high bandwidth transfers
> (restoring partimage disks) would make the host controller die (a freeze,
> then it comes back with 'nobody cared about irq 134' etc.) and perhaps a
> lockup we saw with openSUSE 11.0 installer..
>
>> Now that I think of it, I should probably also change all the internal
>> IRQs to be LEVEL instead of EDGE.
>
> And what does that really affect? I wouldn't think the internal stuff would
> make any difference what it senses on?

It took me most of the morning, but I traced through all the code and
figured out how it is supposed to work.  In this case the type field
in status is indeed cosmetic.  The only time I can see that it really
comes into play is when edge IRQs are enabled after being disabled for
replaying IRQs.  It might be the source of some of the BAD irq count
on the mpc5200, but it is unlikely.

The real logic behind handling an IRQ is defined by the handler
registered with set_irq_chip_and_handler() which the current mpc5200
PIC driver seems to be doing correctly.  However, for external IRQs,
the handler is not changed when the IRQ switches from the default edge
sensitivity to level sensitivity.  Using the wrong handler doesn't
seem to actively break things, but I'm still investigating to find out
if it affects performance or results in BAD irq counts.

Regardless, I'm looking at a number of cleanups to the PIC code and
I'll be posting a patch soon with the intent of getting it into .30.

Cheers,
g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
diff mbox

Patch

diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pic.c b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
index 72865e8..0a093f0 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_pic.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
@@ -196,6 +196,7 @@  static void mpc52xx_extirq_ack(unsigned int virq)
 
 static int mpc52xx_extirq_set_type(unsigned int virq, unsigned int flow_type)
 {
+	struct irq_desc *desc = get_irq_desc(virq);
 	u32 ctrl_reg, type;
 	int irq;
 	int l2irq;
@@ -222,6 +223,11 @@  static int mpc52xx_extirq_set_type(unsigned int virq, unsigned int flow_type)
 		type = 0;
 	}
 
+	desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL);
+	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
+	if (flow_type & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		desc->status |= IRQ_LEVEL;
+
 	ctrl_reg = in_be32(&intr->ctrl);
 	ctrl_reg &= ~(0x3 << (22 - (l2irq * 2)));
 	ctrl_reg |= (type << (22 - (l2irq * 2)));
@@ -231,7 +237,7 @@  static int mpc52xx_extirq_set_type(unsigned int virq, unsigned int flow_type)
 }
 
 static struct irq_chip mpc52xx_extirq_irqchip = {
-	.typename = " MPC52xx IRQ[0-3] ",
+	.typename = "MPC52xx External",
 	.mask = mpc52xx_extirq_mask,
 	.unmask = mpc52xx_extirq_unmask,
 	.ack = mpc52xx_extirq_ack,