| Message ID | 1295054884-18540-1-git-send-email-henk.stegeman@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
What I'm trying to achieve is to have an interrupt service routine
called for every rising edge on pin E2 of the MPC5200B.
The only CPU register setup (AFAIK) to let the CPU generate this
interrupt is to use the GPT (general purpose timer) function pin E2 is
attached to.
This function is supported by mpc52xx_gpt.c.
Excerpt from mpc52xx_gpt.c:
* To use the IRQ controller function, the following two properties must
* be added to the device tree node for the gpt device:
* interrupt-controller;
* #interrupt-cells = < 1 >;
* The IRQ controller binding only uses one cell to specify the interrupt,
* and the IRQ flags are encoded in the cell. A cell is not used to encode
* the IRQ number because the GPT only has a single IRQ source. For flags,
* a value of '1' means rising edge sensitive and '2' means falling edge.
In my dts it's setup like so:
gpt6: timer@660 { // General Purpose Timer GPT6
in GPIO mode for
SMC4000IO sample irq.
compatible = "fsl,mpc5200b-gpt","fsl,mpc5200-gpt";
reg = <0x660 0x10>;
interrupts = <1 15 0>;
interrupt-controller;
#interrupt-cells = <1>;
};
When I put a 512Hz signal on pin E2, I was not getting 512 interrupt
calls per second.
I then found that the GPT's "Interrupt Enable" bit is touched via the
mask/unmask functions.
However when 0, the "Interrupt Enable" bit stops generation of
interrupts by the GPT, so while 0, any edge on pin E2 is lost.
If the GPT had it's own mask bit I would probably have changed the
mask/unmask to use this bit instead.
Now the bit that actually would mask/unmask the interrupt is in the
mask register of the MPC5200B's interrupt controller.
This bit (Bit 15 of the interrupts controller's main_mask register) is
controlled by mpc52xx_main_mask / mpc52xx_main_unmask in
mpc52xx_pic.c.
It does feel odd to have it masked/unmask a level down the cascade, if
the GPT provided more interrupt sources they'd be disabled too, but
that's not the case here, only one interrupt is provided by the GPT.
So that's why I did it anyways. It works, and even if it is sane, it's
confusing and I'd sure like to know of a better way to do this.
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.
Cheers,
Henk
On Mon, Feb 7, 2011 at 12:05 AM, Benjamin Herrenschmidt
<benh@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 ?
>
> Cheers,
> Ben.
>
>> Signed-off-by: Henk Stegeman <henk.stegeman@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);
>> }
>>
>
>
>
[fixed top-posted reply] On Wed, Feb 9, 2011 at 3:16 AM, Henk Stegeman <henk.stegeman@gmail.com> wrote: > On Mon, Feb 7, 2011 at 12:05 AM, Benjamin Herrenschmidt > <benh@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.stegeman@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); >>> } >>> >> >> >> >
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.likely@secretlab.ca> wrote: > [fixed top-posted reply] > > On Wed, Feb 9, 2011 at 3:16 AM, Henk Stegeman <henk.stegeman@gmail.com> wrote: >> On Mon, Feb 7, 2011 at 12:05 AM, Benjamin Herrenschmidt >> <benh@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.stegeman@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. >
On Fri, 2011-03-04 at 23:40 +0100, Henk Stegeman wrote: > What if the unmask/mask functions are dropped and only interrupt > enable/disable functions are provided? No, that's backward. enable/disable are handled by the core, leave them there. > 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. > Ben.
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); }
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. Signed-off-by: Henk Stegeman <henk.stegeman@gmail.com> --- arch/powerpc/platforms/52xx/mpc52xx_gpt.c | 25 ++++++++++++++++++++++--- 1 files changed, 22 insertions(+), 3 deletions(-)