Message ID | 20090107192505.26320.23112.stgit@localhost.localdomain (mailing list archive) |
---|---|
State | Accepted, archived |
Commit | d30239a0ef2000cfae6c60f8e870da2d8dba4d22 |
Delegated to: | Grant Likely |
Headers | show |
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
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.
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
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.
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
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 --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,