IRQ's missing with MPC5200 GPT as an interrupt controller

IRQ's missing with MPC5200 GPT as an interrupt controller

[Henk Stegeman]

I'm trying to make use of the GPT as interrupt controller.
My driver is getting most, but not all of the interrupts, besides that
I'm getting a whole bunch of spurious IRQs, so I'm trying to figure
out what's wrong.

		gpt6: timer@660 {	
			compatible = "fsl,mpc5200b-gpt","fsl,mpc5200-gpt";
			reg = <0x660 0x10>;
			interrupts = <1 15 0>;
			interrupt-controller;
			#interrupt-cells = <1>;
		};

My device has the interrupt-parent property which links it to the
above interrupt controller.

		spi@f00 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "fsl,mpc5200b-spi","fsl,mpc5200-spi";
			reg = <0xf00 0x20>;
			interrupts = <2 13 0 2 14 0>;

			io-controller@0 {
				compatible = "microkey,smc4000io";
				linux,modalias = "of_smc4000io";
				spi-max-frequency = <800000>;
				spi-cpha;
				reg = <0>;
				word-delay-us = <30>;
				interrupt-parent = <&gpt6>;
				interrupts = <2>; // And make it edge falling
			};

There are two things I find suspicious:
- First of all my interrupt is listed as virq# 16, shouldn't it be
virq# 0x50 ? (16 is te L2 value virq# 0x50 of mpc52xx_irqhost_map)
- Secondly in mpc52xx_gpt.c  mpc52xx_gpt_irq_mask() and
mpc52xx_gpt_irq_unmask() resp. clear and set the IrqEn bit of the GPT,
which is described in the MPC5200B_UM as "enables interrupt generation
to the CPU for all modes". The word 'generation' makes me suspicious,
because it could mean that while irqEn is cleared an edge on the GPT's
input does not even request an IRQ immediately after the bit is
cleared.  Or in other words, clearing the bit could do more that just
masking.

~ # cat /proc/interrupts
           CPU0
 16:    1338686   MPC52xx GPT      Edge         smc4000io
129:      83224   MPC52xx Peripherals Level        mpc52xx_psc_uart
130:          1   MPC52xx Peripherals Level        mpc52xx_psc_uart
133:          0   MPC52xx Peripherals Level        mpc52xx-fec_ctrl
134:          0   MPC52xx Peripherals Level        ohci_hcd:usb1
135:      16127   MPC52xx Peripherals Level        mpc52xx_ata
141:          0   MPC52xx Peripherals Level        mpc5200-spi-modf
142:      79184   MPC52xx Peripherals Level        mpc5200-spi-spif
192:          0   MPC52xx SDMA     Level        ATA task
193:         15   MPC52xx SDMA     Level        mpc52xx-fec_rx
194:      35615   MPC52xx SDMA     Level        mpc52xx-fec_tx
LOC:     134286   Local timer interrupts
SPU:     576958   Spurious interrupts
CNT:          0   Performance monitoring interrupts
MCE:          0   Machine check exceptions

~ # dmesg |grep 660
[    1.975928] irq: irq 0 on host /soc5200@f0000000/timer@660 mapped
to virtual irq 16

~ # dmesg | grep host_map
[    0.000000] mpc52xx_irqhost_map: virq=81, l1=2, l2=1
[    0.319272] mpc52xx_irqhost_map: External IRQ1 virq=41, hw=41. type=8
[    0.376117] mpc52xx_irqhost_map: virq=49, l1=1, l2=9
[    0.417213] mpc52xx_irqhost_map: virq=4a, l1=1, l2=10
[    0.452119] mpc52xx_irqhost_map: virq=4b, l1=1, l2=11
[    0.487031] mpc52xx_irqhost_map: virq=4c, l1=1, l2=12
[    0.529899] mpc52xx_irqhost_map: virq=4d, l1=1, l2=13
[    0.564821] mpc52xx_irqhost_map: virq=4e, l1=1, l2=14
[    0.599744] mpc52xx_irqhost_map: virq=4f, l1=1, l2=15
[    0.634669] mpc52xx_irqhost_map: virq=50, l1=1, l2=16
[    1.629764] mpc52xx_irqhost_map: virq=82, l1=2, l2=2
[    1.667194] mpc52xx_irqhost_map: virq=84, l1=2, l2=4
[    1.763299] mpc52xx_irqhost_map: virq=87, l1=2, l2=7
[    1.790510] mpc52xx_irqhost_map: virq=c0, l1=3, l2=0
[    1.909341] mpc52xx_irqhost_map: virq=8d, l1=2, l2=13
[    1.936712] mpc52xx_irqhost_map: virq=8e, l1=2, l2=14
[    2.055579] mpc52xx_irqhost_map: virq=c1, l1=3, l2=1
[    2.083018] mpc52xx_irqhost_map: virq=c2, l1=3, l2=2
[    2.110345] mpc52xx_irqhost_map: virq=85, l1=2, l2=5
[    2.211890] mpc52xx_irqhost_map: virq=86, l1=2, l2=6
~

Re: IRQ's missing with MPC5200 GPT as an interrupt controller

[Grant Likely]

Hi Henk,

On Mon, Mar 8, 2010 at 8:20 AM, Henk Stegeman <henk.stegeman@gmail.com> wro=
te:
> I'm trying to make use of the GPT as interrupt controller.
> My driver is getting most, but not all of the interrupts, besides that
> I'm getting a whole bunch of spurious IRQs, so I'm trying to figure
> out what's wrong.
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpt6: timer@660 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "fsl,mpc520=
0b-gpt","fsl,mpc5200-gpt";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0x660 0x10>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupts =3D <1 15 0>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupt-controller;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#interrupt-cells =3D <1>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0};
>
> My device has the interrupt-parent property which links it to the
> above interrupt controller.
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spi@f00 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#size-cells =3D <0>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "fsl,mpc520=
0b-spi","fsl,mpc5200-spi";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0xf00 0x20>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupts =3D <2 13 0 2 1=
4 0>;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0io-controller@0 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible=
 =3D "microkey,smc4000io";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0linux,moda=
lias =3D "of_smc4000io";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spi-max-fr=
equency =3D <800000>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spi-cpha;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0=
>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0word-delay=
-us =3D <30>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupt-=
parent =3D <&gpt6>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupts=
 =3D <2>; // And make it edge falling
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0};
>
> There are two things I find suspicious:
> - First of all my interrupt is listed as virq# 16, shouldn't it be
> virq# 0x50 ? (16 is te L2 value virq# 0x50 of mpc52xx_irqhost_map)

The GPT irq registered as a different IRQ controller, so it will be
assigned an entirely different number within the virq range.

> - Secondly in mpc52xx_gpt.c =A0mpc52xx_gpt_irq_mask() and
> mpc52xx_gpt_irq_unmask() resp. clear and set the IrqEn bit of the GPT,
> which is described in the MPC5200B_UM as "enables interrupt generation
> to the CPU for all modes". The word 'generation' makes me suspicious,
> because it could mean that while irqEn is cleared an edge on the GPT's
> input does not even request an IRQ immediately after the bit is
> cleared. =A0Or in other words, clearing the bit could do more that just
> masking.

Could be, the IRQ support has not gotten extensive testing.  It may
require additional work.

> ~ # cat /proc/interrupts
> =A0 =A0 =A0 =A0 =A0 CPU0
> =A016: =A0 =A01338686 =A0 MPC52xx GPT =A0 =A0 =A0Edge =A0 =A0 =A0 =A0 smc=
4000io
> 129: =A0 =A0 =A083224 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0mpc52x=
x_psc_uart
> 130: =A0 =A0 =A0 =A0 =A01 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0mp=
c52xx_psc_uart
> 133: =A0 =A0 =A0 =A0 =A00 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0mp=
c52xx-fec_ctrl
> 134: =A0 =A0 =A0 =A0 =A00 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0oh=
ci_hcd:usb1
> 135: =A0 =A0 =A016127 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0mpc52x=
x_ata
> 141: =A0 =A0 =A0 =A0 =A00 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0mp=
c5200-spi-modf
> 142: =A0 =A0 =A079184 =A0 MPC52xx Peripherals Level =A0 =A0 =A0 =A0mpc520=
0-spi-spif
> 192: =A0 =A0 =A0 =A0 =A00 =A0 MPC52xx SDMA =A0 =A0 Level =A0 =A0 =A0 =A0A=
TA task
> 193: =A0 =A0 =A0 =A0 15 =A0 MPC52xx SDMA =A0 =A0 Level =A0 =A0 =A0 =A0mpc=
52xx-fec_rx
> 194: =A0 =A0 =A035615 =A0 MPC52xx SDMA =A0 =A0 Level =A0 =A0 =A0 =A0mpc52=
xx-fec_tx
> LOC: =A0 =A0 134286 =A0 Local timer interrupts
> SPU: =A0 =A0 576958 =A0 Spurious interrupts
> CNT: =A0 =A0 =A0 =A0 =A00 =A0 Performance monitoring interrupts
> MCE: =A0 =A0 =A0 =A0 =A00 =A0 Machine check exceptions
>
> ~ # dmesg |grep 660
> [ =A0 =A01.975928] irq: irq 0 on host /soc5200@f0000000/timer@660 mapped
> to virtual irq 16
>
> ~ # dmesg | grep host_map
> [ =A0 =A00.000000] mpc52xx_irqhost_map: virq=3D81, l1=3D2, l2=3D1
> [ =A0 =A00.319272] mpc52xx_irqhost_map: External IRQ1 virq=3D41, hw=3D41.=
 type=3D8
> [ =A0 =A00.376117] mpc52xx_irqhost_map: virq=3D49, l1=3D1, l2=3D9
> [ =A0 =A00.417213] mpc52xx_irqhost_map: virq=3D4a, l1=3D1, l2=3D10
> [ =A0 =A00.452119] mpc52xx_irqhost_map: virq=3D4b, l1=3D1, l2=3D11
> [ =A0 =A00.487031] mpc52xx_irqhost_map: virq=3D4c, l1=3D1, l2=3D12
> [ =A0 =A00.529899] mpc52xx_irqhost_map: virq=3D4d, l1=3D1, l2=3D13
> [ =A0 =A00.564821] mpc52xx_irqhost_map: virq=3D4e, l1=3D1, l2=3D14
> [ =A0 =A00.599744] mpc52xx_irqhost_map: virq=3D4f, l1=3D1, l2=3D15
> [ =A0 =A00.634669] mpc52xx_irqhost_map: virq=3D50, l1=3D1, l2=3D16
> [ =A0 =A01.629764] mpc52xx_irqhost_map: virq=3D82, l1=3D2, l2=3D2
> [ =A0 =A01.667194] mpc52xx_irqhost_map: virq=3D84, l1=3D2, l2=3D4
> [ =A0 =A01.763299] mpc52xx_irqhost_map: virq=3D87, l1=3D2, l2=3D7
> [ =A0 =A01.790510] mpc52xx_irqhost_map: virq=3Dc0, l1=3D3, l2=3D0
> [ =A0 =A01.909341] mpc52xx_irqhost_map: virq=3D8d, l1=3D2, l2=3D13
> [ =A0 =A01.936712] mpc52xx_irqhost_map: virq=3D8e, l1=3D2, l2=3D14
> [ =A0 =A02.055579] mpc52xx_irqhost_map: virq=3Dc1, l1=3D3, l2=3D1
> [ =A0 =A02.083018] mpc52xx_irqhost_map: virq=3Dc2, l1=3D3, l2=3D2
> [ =A0 =A02.110345] mpc52xx_irqhost_map: virq=3D85, l1=3D2, l2=3D5
> [ =A0 =A02.211890] mpc52xx_irqhost_map: virq=3D86, l1=3D2, l2=3D6

This grep only shows the output from mpc52xx_intc.c.  The GPT driver
has its own IRQ mapping function; mpc52xx_gpt_irq_map(), which is why
you're not seeing your IRQ map debug message.

Cheers,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH] Fix masking of interrupts for 52xx GPT IRQ.

[Henk Stegeman]

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(-)

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);
 }
 
-- 
1.5.4.3

Re: [PATCH] Fix masking of interrupts for 52xx GPT IRQ.

[Henk Stegeman]

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 =3D < 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 =3D "fsl,mpc5200b-gpt","fsl,mpc5200-gpt";
                       reg =3D <0x660 0x10>;
                       interrupts =3D <1 15 0>;
                       interrupt-controller;
                       #interrupt-cells =3D <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>
>> ---
>> =A0arch/powerpc/platforms/52xx/mpc52xx_gpt.c | =A0 25 ++++++++++++++++++=
++++---
>> =A01 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c b/arch/powerpc/pl=
atforms/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 {
>> =A0 =A0 =A0 struct irq_host *irqhost;
>> =A0 =A0 =A0 u32 ipb_freq;
>> =A0 =A0 =A0 u8 wdt_mode;
>> -
>> + =A0 =A0 int cascade_virq;
>> =A0#if defined(CONFIG_GPIOLIB)
>> =A0 =A0 =A0 struct of_gpio_chip of_gc;
>> =A0#endif
>> @@ -136,18 +136,35 @@ DEFINE_MUTEX(mpc52xx_gpt_list_mutex);
>> =A0static void mpc52xx_gpt_irq_unmask(unsigned int virq)
>> =A0{
>> =A0 =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>> +
>> + =A0 =A0 enable_irq(gpt->cascade_virq);
>> +
>> +}
>> +
>> +static void mpc52xx_gpt_irq_mask(unsigned int virq)
>> +{
>> + =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>> +
>> + =A0 =A0 disable_irq(gpt->cascade_virq);
>> +}
>> +
>> +static void mpc52xx_gpt_irq_enable(unsigned int virq)
>> +{
>> + =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>> =A0 =A0 =A0 unsigned long flags;
>>
>> + =A0 =A0 dev_dbg(gpt->dev, "%s %d\n", __func__, virq);
>> =A0 =A0 =A0 spin_lock_irqsave(&gpt->lock, flags);
>> =A0 =A0 =A0 setbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>> =A0}
>>
>> -static void mpc52xx_gpt_irq_mask(unsigned int virq)
>> +static void mpc52xx_gpt_irq_disable(unsigned int virq)
>> =A0{
>> =A0 =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>> =A0 =A0 =A0 unsigned long flags;
>>
>> + =A0 =A0 dev_dbg(gpt->dev, "%s %d\n", __func__, virq);
>> =A0 =A0 =A0 spin_lock_irqsave(&gpt->lock, flags);
>> =A0 =A0 =A0 clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>> @@ -184,6 +201,8 @@ static struct irq_chip mpc52xx_gpt_irq_chip =3D {
>> =A0 =A0 =A0 .name =3D "MPC52xx GPT",
>> =A0 =A0 =A0 .unmask =3D mpc52xx_gpt_irq_unmask,
>> =A0 =A0 =A0 .mask =3D mpc52xx_gpt_irq_mask,
>> + =A0 =A0 .enable =3D mpc52xx_gpt_irq_enable,
>> + =A0 =A0 .disable =3D mpc52xx_gpt_irq_disable,
>> =A0 =A0 =A0 .ack =3D mpc52xx_gpt_irq_ack,
>> =A0 =A0 =A0 .set_type =3D mpc52xx_gpt_irq_set_type,
>> =A0};
>> @@ -268,7 +287,7 @@ mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt, =
struct device_node *node)
>> =A0 =A0 =A0 if ((mode & MPC52xx_GPT_MODE_MS_MASK) =3D=3D 0)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(&gpt->regs->mode, mode | MPC52xx_GP=
T_MODE_MS_IC);
>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>> -
>> + =A0 =A0 gpt->cascade_virq =3D cascade_virq;
>> =A0 =A0 =A0 dev_dbg(gpt->dev, "%s() complete. virq=3D%i\n", __func__, ca=
scade_virq);
>> =A0}
>>
>
>
>

Re: [PATCH] Fix masking of interrupts for 52xx GPT IRQ.

[Grant Likely]

[fixed top-posted reply]

On Wed, Feb 9, 2011 at 3:16 AM, Henk Stegeman <henk.stegeman@gmail.com> wro=
te:
> 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>
>>> ---
>>> =A0arch/powerpc/platforms/52xx/mpc52xx_gpt.c | =A0 25 +++++++++++++++++=
+++++---
>>> =A01 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c b/arch/powerpc/p=
latforms/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 {
>>> =A0 =A0 =A0 struct irq_host *irqhost;
>>> =A0 =A0 =A0 u32 ipb_freq;
>>> =A0 =A0 =A0 u8 wdt_mode;
>>> -
>>> + =A0 =A0 int cascade_virq;
>>> =A0#if defined(CONFIG_GPIOLIB)
>>> =A0 =A0 =A0 struct of_gpio_chip of_gc;
>>> =A0#endif
>>> @@ -136,18 +136,35 @@ DEFINE_MUTEX(mpc52xx_gpt_list_mutex);
>>> =A0static void mpc52xx_gpt_irq_unmask(unsigned int virq)
>>> =A0{
>>> =A0 =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>> +
>>> + =A0 =A0 enable_irq(gpt->cascade_virq);
>>> +
>>> +}
>>> +
>>> +static void mpc52xx_gpt_irq_mask(unsigned int virq)
>>> +{
>>> + =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>> +
>>> + =A0 =A0 disable_irq(gpt->cascade_virq);
>>> +}
>>> +
>>> +static void mpc52xx_gpt_irq_enable(unsigned int virq)
>>> +{
>>> + =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>> =A0 =A0 =A0 unsigned long flags;
>>>
>>> + =A0 =A0 dev_dbg(gpt->dev, "%s %d\n", __func__, virq);
>>> =A0 =A0 =A0 spin_lock_irqsave(&gpt->lock, flags);
>>> =A0 =A0 =A0 setbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
>>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>>> =A0}
>>>
>>> -static void mpc52xx_gpt_irq_mask(unsigned int virq)
>>> +static void mpc52xx_gpt_irq_disable(unsigned int virq)
>>> =A0{
>>> =A0 =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>> =A0 =A0 =A0 unsigned long flags;
>>>
>>> + =A0 =A0 dev_dbg(gpt->dev, "%s %d\n", __func__, virq);
>>> =A0 =A0 =A0 spin_lock_irqsave(&gpt->lock, flags);
>>> =A0 =A0 =A0 clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
>>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>>> @@ -184,6 +201,8 @@ static struct irq_chip mpc52xx_gpt_irq_chip =3D {
>>> =A0 =A0 =A0 .name =3D "MPC52xx GPT",
>>> =A0 =A0 =A0 .unmask =3D mpc52xx_gpt_irq_unmask,
>>> =A0 =A0 =A0 .mask =3D mpc52xx_gpt_irq_mask,
>>> + =A0 =A0 .enable =3D mpc52xx_gpt_irq_enable,
>>> + =A0 =A0 .disable =3D mpc52xx_gpt_irq_disable,
>>> =A0 =A0 =A0 .ack =3D mpc52xx_gpt_irq_ack,
>>> =A0 =A0 =A0 .set_type =3D mpc52xx_gpt_irq_set_type,
>>> =A0};
>>> @@ -268,7 +287,7 @@ mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt,=
 struct device_node *node)
>>> =A0 =A0 =A0 if ((mode & MPC52xx_GPT_MODE_MS_MASK) =3D=3D 0)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(&gpt->regs->mode, mode | MPC52xx_G=
PT_MODE_MS_IC);
>>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>>> -
>>> + =A0 =A0 gpt->cascade_virq =3D cascade_virq;
>>> =A0 =A0 =A0 dev_dbg(gpt->dev, "%s() complete. virq=3D%i\n", __func__, c=
ascade_virq);
>>> =A0}
>>>
>>
>>
>>
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Fix masking of interrupts for 52xx GPT IRQ.

[Henk Stegeman]

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> w=
rote:
> [fixed top-posted reply]
>
> On Wed, Feb 9, 2011 at 3:16 AM, Henk Stegeman <henk.stegeman@gmail.com> w=
rote:
>> 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. =A0It doesn't make much sense to
> provide an enable/disable function along with the mask/unmask. =A0I
> think you can safely drop the old enable/disable code. =A0I'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>
>>>> ---
>>>> =A0arch/powerpc/platforms/52xx/mpc52xx_gpt.c | =A0 25 ++++++++++++++++=
++++++---
>>>> =A01 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 {
>>>> =A0 =A0 =A0 struct irq_host *irqhost;
>>>> =A0 =A0 =A0 u32 ipb_freq;
>>>> =A0 =A0 =A0 u8 wdt_mode;
>>>> -
>>>> + =A0 =A0 int cascade_virq;
>>>> =A0#if defined(CONFIG_GPIOLIB)
>>>> =A0 =A0 =A0 struct of_gpio_chip of_gc;
>>>> =A0#endif
>>>> @@ -136,18 +136,35 @@ DEFINE_MUTEX(mpc52xx_gpt_list_mutex);
>>>> =A0static void mpc52xx_gpt_irq_unmask(unsigned int virq)
>>>> =A0{
>>>> =A0 =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>>> +
>>>> + =A0 =A0 enable_irq(gpt->cascade_virq);
>>>> +
>>>> +}
>>>> +
>>>> +static void mpc52xx_gpt_irq_mask(unsigned int virq)
>>>> +{
>>>> + =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>>> +
>>>> + =A0 =A0 disable_irq(gpt->cascade_virq);
>>>> +}
>>>> +
>>>> +static void mpc52xx_gpt_irq_enable(unsigned int virq)
>>>> +{
>>>> + =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>>> =A0 =A0 =A0 unsigned long flags;
>>>>
>>>> + =A0 =A0 dev_dbg(gpt->dev, "%s %d\n", __func__, virq);
>>>> =A0 =A0 =A0 spin_lock_irqsave(&gpt->lock, flags);
>>>> =A0 =A0 =A0 setbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
>>>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>>>> =A0}
>>>>
>>>> -static void mpc52xx_gpt_irq_mask(unsigned int virq)
>>>> +static void mpc52xx_gpt_irq_disable(unsigned int virq)
>>>> =A0{
>>>> =A0 =A0 =A0 struct mpc52xx_gpt_priv *gpt =3D get_irq_chip_data(virq);
>>>> =A0 =A0 =A0 unsigned long flags;
>>>>
>>>> + =A0 =A0 dev_dbg(gpt->dev, "%s %d\n", __func__, virq);
>>>> =A0 =A0 =A0 spin_lock_irqsave(&gpt->lock, flags);
>>>> =A0 =A0 =A0 clrbits32(&gpt->regs->mode, MPC52xx_GPT_MODE_IRQ_EN);
>>>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>>>> @@ -184,6 +201,8 @@ static struct irq_chip mpc52xx_gpt_irq_chip =3D {
>>>> =A0 =A0 =A0 .name =3D "MPC52xx GPT",
>>>> =A0 =A0 =A0 .unmask =3D mpc52xx_gpt_irq_unmask,
>>>> =A0 =A0 =A0 .mask =3D mpc52xx_gpt_irq_mask,
>>>> + =A0 =A0 .enable =3D mpc52xx_gpt_irq_enable,
>>>> + =A0 =A0 .disable =3D mpc52xx_gpt_irq_disable,
>>>> =A0 =A0 =A0 .ack =3D mpc52xx_gpt_irq_ack,
>>>> =A0 =A0 =A0 .set_type =3D mpc52xx_gpt_irq_set_type,
>>>> =A0};
>>>> @@ -268,7 +287,7 @@ mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt=
, struct device_node *node)
>>>> =A0 =A0 =A0 if ((mode & MPC52xx_GPT_MODE_MS_MASK) =3D=3D 0)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(&gpt->regs->mode, mode | MPC52xx_=
GPT_MODE_MS_IC);
>>>> =A0 =A0 =A0 spin_unlock_irqrestore(&gpt->lock, flags);
>>>> -
>>>> + =A0 =A0 gpt->cascade_virq =3D cascade_virq;
>>>> =A0 =A0 =A0 dev_dbg(gpt->dev, "%s() complete. virq=3D%i\n", __func__, =
cascade_virq);
>>>> =A0}
>>>>
>>>
>>>
>>>
>>
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>

Re: [PATCH] Fix masking of interrupts for 52xx GPT IRQ.

[Benjamin Herrenschmidt]

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.