What if the unmask/mask functions are dropped and only interrupt enable/disable functions are provided? I.e: rename the old unmask/mask functions to enable/disable and register them as enable/disable? I did try that, and it works too, no spurious IRQ's and no missing IRQ's.
Cheers, Henk. On Wed, Mar 2, 2011 at 10:30 PM, Grant Likely <grant.lik...@secretlab.ca> wrote: > [fixed top-posted reply] > > On Wed, Feb 9, 2011 at 3:16 AM, Henk Stegeman <henk.stege...@gmail.com> wrote: >> On Mon, Feb 7, 2011 at 12:05 AM, Benjamin Herrenschmidt >> <b...@kernel.crashing.org> wrote: >>> On Sat, 2011-01-15 at 02:28 +0100, Henk Stegeman wrote: >>>> When using the GPT as interrupt controller interupts were missing. >>>> It turned out the IrqEn bit of the GPT is not a mask bit, but it also >>>> disables interrupt generation. This modification masks interrupt one >>>> level down the cascade. Note that masking one level down the cascade >>>> is only valid here because the GPT as interrupt ontroller only serves >>>> one IRQ. >>> >>> I'm not too sure here... You shouldn't implemen t both mask/unmask and >>> enable/disable on the same irq_chip and certainly not cal >>> enable_irq/disable_irq from a mask or an unmask callback... >>> >>> Now, I'm not familiar with the HW here, can you tell me more about what >>> exactly is happening, how things are layed out and what you are trying >>> to fix ? >>> > [...] >> Because the old code in the unmask/mask function did enable/disable >> and I didn't want to just drop that code, I provided it via the >> enable/disable function. >> What is wrong by implementing & registering both mask/unmask and >> enable/disable for the same irq_chip? >> If it is wrong it would be nice to let the kernel print a big fat >> warning when this is registered. > > After some digging, yes Ben is right. It doesn't make much sense to > provide an enable/disable function along with the mask/unmask. I > think you can safely drop the old enable/disable code. I'm going to > drop this patch from my tree and you can respin and retest. > > g. > >> >> Cheers, >> >> Henk >> > >>> >>>> Signed-off-by: Henk Stegeman <henk.stege...@gmail.com> >>>> --- >>>> arch/powerpc/platforms/52xx/mpc52xx_gpt.c | 25 ++++++++++++++++++++++--- >>>> 1 files changed, 22 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c >>>> b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c >>>> index 6f8ebe1..9ae2045 100644 >>>> --- a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c >>>> +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c >>>> @@ -91,7 +91,7 @@ struct mpc52xx_gpt_priv { >>>> struct irq_host *irqhost; >>>> u32 ipb_freq; >>>> u8 wdt_mode; >>>> - >>>> + int cascade_virq; >>>> #if defined(CONFIG_GPIOLIB) >>>> struct of_gpio_chip of_gc; >>>> #endif >>>> @@ -136,18 +136,35 @@ DEFINE_MUTEX(mpc52xx_gpt_list_mutex); >>>> static void mpc52xx_gpt_irq_unmask(unsigned int virq) >>>> { >>>> struct mpc52xx_gpt_priv *gpt = get_irq_chip_data(virq); >>>> + >>>> + enable_irq(gpt->cascade_virq); >>>> + >>>> +} >>>> + >>>> +static void mpc52xx_gpt_irq_mask(unsigned int virq) >>>> +{ >>>> + struct mpc52xx_gpt_priv *gpt = get_irq_chip_data(virq); >>>> + >>>> + disable_irq(gpt->cascade_virq); >>>> +} >>>> + >>>> +static void mpc52xx_gpt_irq_enable(unsigned int virq) >>>> +{ >>>> + struct mpc52xx_gpt_priv *gpt = get_irq_chip_data(virq); >>>> unsigned long flags; >>>> >>>> + dev_dbg(gpt->dev, "%s %d\n", __func__, virq); >>>> spin_lock_irqsave(&gpt->lock, flags); >>>> setbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN); >>>> spin_unlock_irqrestore(&gpt->lock, flags); >>>> } >>>> >>>> -static void mpc52xx_gpt_irq_mask(unsigned int virq) >>>> +static void mpc52xx_gpt_irq_disable(unsigned int virq) >>>> { >>>> struct mpc52xx_gpt_priv *gpt = get_irq_chip_data(virq); >>>> unsigned long flags; >>>> >>>> + dev_dbg(gpt->dev, "%s %d\n", __func__, virq); >>>> spin_lock_irqsave(&gpt->lock, flags); >>>> clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN); >>>> spin_unlock_irqrestore(&gpt->lock, flags); >>>> @@ -184,6 +201,8 @@ static struct irq_chip mpc52xx_gpt_irq_chip = { >>>> .name = "MPC52xx GPT", >>>> .unmask = mpc52xx_gpt_irq_unmask, >>>> .mask = mpc52xx_gpt_irq_mask, >>>> + .enable = mpc52xx_gpt_irq_enable, >>>> + .disable = mpc52xx_gpt_irq_disable, >>>> .ack = mpc52xx_gpt_irq_ack, >>>> .set_type = mpc52xx_gpt_irq_set_type, >>>> }; >>>> @@ -268,7 +287,7 @@ mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt, >>>> struct device_node *node) >>>> if ((mode & MPC52xx_GPT_MODE_MS_MASK) == 0) >>>> out_be32(&gpt->regs->mode, mode | MPC52xx_GPT_MODE_MS_IC); >>>> spin_unlock_irqrestore(&gpt->lock, flags); >>>> - >>>> + gpt->cascade_virq = cascade_virq; >>>> dev_dbg(gpt->dev, "%s() complete. virq=%i\n", __func__, >>>> cascade_virq); >>>> } >>>> >>> >>> >>> >> > > > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev