[PATCH/RFC] powerpc: Add MPC5200 Interrupt Controller support.

This patch add MPC52xx Interrupt controller for ARCH=powerpc.

It includes the main code in arch/powerpc/sysdev/ ad well as an header file in include/asm-powerpc.

PS: I did not success to properly inline the file using thrunderbird.

Signed-off-by: Nicolas DET <redacted>

On Wed, Nov 01, 2006 at 10:05:43PM -0000, Dale Farnsworth wrote:

In article [off-list ref] Nicolas wrote:

quoted

--- a/arch/powerpc/sysdev/mpc52xx_pic.c	1970-01-01 01:00:00.000000000 +0100
+++ b/arch/powerpc/sysdev/mpc52xx_pic.c	2006-11-01 21:16:40.000000000 +0100

@@ -0,0 +1,566 @@
+/*
+ *
+ * Programmable Interrupt Controller functions for the Freescale MPC52xx.
+ * 
+ * Copyright (C) 2006 bplan GmbH
+ * 
+ * This file is licensed under the terms of the GNU General Public License

Once again, please restore the copyright lines you removed from this file.

Hehe, you are aware that, IIRW, someone suggested to cut them earlier in this
thread ? 

Friendly,

Sven Luther

My only remaining comment is what I said earlier about the way you
divided the interrupt in levels.

In fact, I've finally dug into the doc and found out that the situation
is even less clear :)

The whole point of this division is to avoid special cases and properly
deal with the separation in different HW registers. We have basically 4
register sets controlling irqs:

 - intr->ctrl
 - intr->main_mask
 - intr->per_mask
 - sdma (different device)

Now, your current setup ends up defining a "level" and 5 irq_chips !!!

 - intr->ctrl bit 11 has it's own chip
 - intr->ctrl other bits or intr->main_mask (2 different chips)
 - intr->per_mask a chip 
 - sdma a chip

which seems to totally gross since the first one is basically "main" irq
0, and has no reason whatsoever to be kept separate from "main". Which
would give us something much more sensible with 4 levels and 4 chips:

 - intr->ctrl based interrupts (crit and main, crit being just main 0)
 - intr->main_mask
 - intr->per_mask
 - sdma

In that case, you have a nice 1 level == 1 irq_chip == one register set.

Now, in addition to that, we have another issue I haven't spotted
before, but it might be worth considering:

Do we actually want the sdma interrupts handled there ? Because if you
look closely, the SDMA is basically a cascaded interrupt controller. It
hangs of per interrupt 0 :)

Thus we could simply remove the code for it from that driver and
implement it as as separate controller with a separate interrupt domain.
I know you won't like that idea because it means having a different
interrupt tree but it's worth having the discussion.

Ben.

In article [off-list ref] Nicolas wrote:

Once again, please restore the copyright lines you removed from this file.

Dale Farnsworth

On Wed, Nov 01, 2006 at 11:07:08PM +0100, Sven Luther wrote:

On Wed, Nov 01, 2006 at 10:05:43PM -0000, Dale Farnsworth wrote:

quoted

In article [off-list ref] Nicolas wrote:

quoted

--- a/arch/powerpc/sysdev/mpc52xx_pic.c	1970-01-01 01:00:00.000000000 +0100
+++ b/arch/powerpc/sysdev/mpc52xx_pic.c	2006-11-01 21:16:40.000000000 +0100

@@ -0,0 +1,566 @@
+/*
+ *
+ * Programmable Interrupt Controller functions for the Freescale MPC52xx.
+ * 
+ * Copyright (C) 2006 bplan GmbH
+ * 
+ * This file is licensed under the terms of the GNU General Public License

Once again, please restore the copyright lines you removed from this file.

Hehe, you are aware that, IIRW, someone suggested to cut them earlier in this
thread ? 

I think people called for removing the "Maintained by" and the
changelog-like commentary, but not the copyrights.

-Dale

Benjamin Herrenschmidt wrote:

Now, in addition to that, we have another issue I haven't spotted
before, but it might be worth considering:

Do we actually want the sdma interrupts handled there ? Because if you
look closely, the SDMA is basically a cascaded interrupt controller. It
hangs of per interrupt 0 :)

I know. I already spot this point (IRQ as far as I remenber and in private mails) and I got no answer so I assumed it was ok like I did. It's also really simplier and easier to add SDMA has another irq_chip.
Keep is simple ;-)

Thus we could simply remove the code for it from that driver and
implement it as as separate controller with a separate interrupt domain.
I know you won't like that idea because it means having a different
interrupt tree but it's worth having the discussion.

The device tree won't change anyway...

I'll submit a new buch of patches pretty soon. Maybe the final version.

"On y est presque !" ;-)

Regards

Nicolas DET wrote:

Benjamin Herrenschmidt wrote:

quoted

Now, in addition to that, we have another issue I haven't spotted
before, but it might be worth considering:

Do we actually want the sdma interrupts handled there ? Because if you
look closely, the SDMA is basically a cascaded interrupt controller. It
hangs of per interrupt 0 :)

I know. I already spot this point (IRQ as far as I remenber and in private mails) and I got no answer so I assumed it was ok like I did. It's also really simplier and easier to add SDMA has another irq_chip.
Keep is simple ;-)

quoted

Thus we could simply remove the code for it from that driver and
implement it as as separate controller with a separate interrupt domain.
I know you won't like that idea because it means having a different
interrupt tree but it's worth having the discussion.

The device tree won't change anyway...

I'll submit a new buch of patches pretty soon. Maybe the final version.

"On y est presque !" ;-)

I attached the new Interrupt Controller patch. I have been very busy today. I hope I did not overlook anything.

RFC

On Thu, 2006-11-02 at 21:47 +0100, Nicolas DET wrote:

You did significant changes, you should add your (c) before the "Based
on..." line.

You have changed the mapping of L1/L2 but not the device-tree
representation to match them. Which means that you need the ugly hack
above. That is not goot. Fix your device-tree instead and do the same
split in the tree for L1/L2 than you do in the code.

In fact, I'm wondering if it might not be worth going back to 2 cells
and have cell 0 just contain the HW number now that we have sanitized
those... In any way, the above special casing should go.

+	picnode = find_mpc52xx_picnode();
+	sdmanode = find_mpc52xx_sdmanode();

Any reason why you have those inline 1-line functions and just not put
the actual of_find_* call in here ?

You probably also need some of_node_put() in the error case. You are
doing a whole bunch of "return" that bypass them.

The rest looks ok.

Please submit the io.h patch separately. Also, you may need to fix the
default case (which is the one we are using) in there to work with
CONFIG_PCI not set (by defining PCI_DRAM_BASE to 0 among others).

Ben.

quoted

+	picnode = find_mpc52xx_picnode();
+	sdmanode = find_mpc52xx_sdmanode();
    

Any reason why you have those inline 1-line functions and just not put
the actual of_find_* call in here ?
  

I think it might be worth creating a
arch/powerpc/sysdev/mpc52xx_common.c (we'll probably need it later on
anyway)
with a helper that would do
 - The find_node
 - get_address / translate /  get_size
 - ioremap

Something like :

intr = mpc52xx_find_and_map("mpc52xx-intr");
sdma = mpc52xx_find_and_map("mpc52xx-sdma");

would be more elegant. Especially since finding and mapping things like
intr/sdma/xlb/cdm ... will be done at several place. That would prevent
repeating all that code for nothing.




Also, in the Makefile, I would make the compilation conditionnal to
CONFIG_PPC_MPC52xx and not CONFIG_PPC_MPC52xx_PIC ...
If you're on a 52xx, you most likely want the interrupt controller ...

But the CONFIG_PPC_MPC52xx option should be in arch/powerpc/Kconfig
and not in the platform/embedded6xx/Kconfig


        Sylvain

On Sun, 2006-11-05 at 01:27 +0100, Sylvain Munaut wrote:

quoted

quoted

+	picnode = find_mpc52xx_picnode();
+	sdmanode = find_mpc52xx_sdmanode();
    

Any reason why you have those inline 1-line functions and just not put
the actual of_find_* call in here ?
  

I think it might be worth creating a
arch/powerpc/sysdev/mpc52xx_common.c (we'll probably need it later on
anyway)
with a helper that would do

- The find_node

I don't see why we need a helper for that at all in the first place :)

 - get_address / translate /  get_size

That too

 - ioremap

Something like :

intr = mpc52xx_find_and_map("mpc52xx-intr");
sdma = mpc52xx_find_and_map("mpc52xx-sdma");

Hrm... I don't care that much but I also don't think we need that
helper. It's not saving much.

would be more elegant. Especially since finding and mapping things like
intr/sdma/xlb/cdm ... will be done at several place. That would prevent
repeating all that code for nothing.

It's not much code to repeat ;) but if you want, I have no big problem
with that.

Also, in the Makefile, I would make the compilation conditionnal to
CONFIG_PPC_MPC52xx and not CONFIG_PPC_MPC52xx_PIC ...
If you're on a 52xx, you most likely want the interrupt controller ...

But the CONFIG_PPC_MPC52xx option should be in arch/powerpc/Kconfig
and not in the platform/embedded6xx/Kconfig

Ben.

On Sun, 05 Nov 2006 10:35:40 +1100
Benjamin Herrenschmidt [off-list ref] wrote:

On Thu, 2006-11-02 at 21:47 +0100, Nicolas DET wrote:

quoted

+ *
+ * Programmable Interrupt Controller functions for the Freescale MPC52xx.
+ *
+ * Based on (well, mostly copied from) the code from the 2.4 kernel by
+ * Dale Farnsworth <dfarnsworth@mvista.com> and Kent Borg.
+ * 
+ * Copyright (C) 2004 Sylvain Munaut <tnt@246tNt.com>
+ * Copyright (C) 2003 Montavista Software, Inc

You did significant changes, you should add your (c) before the "Based
on..." line.

Ok. It now looks likes this:

+/*
+ *
+ * Programmable Interrupt Controller functions for the Freescale MPC52xx.
+ *
+ * Copyright (C) 2006 bplan GmbH
+ *
+ * Based on (well, mostly copied from) the code from the 2.4 kernel by
+ * Dale Farnsworth [off-list ref] and Kent Borg.
+ * 
+ * Copyright (C) 2004 Sylvain Munaut [off-list ref]
+ * Copyright (C) 2003 Montavista Software, Inc
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ *
+ */

quoted

+	switch (l1irq) {
+	case MPC52xx_IRQ_L1_CRIT:
+		pr_debug("%s: Critical. l2=%x\n", __func__, l2irq);
+
+		BUG_ON(l2irq != 0);
+
+		type = mpc52xx_irqx_gettype(l2irq);
+		good_irqchip = &mpc52xx_extirq_irqchip;
+		break;
+
+	case MPC52xx_IRQ_L1_MAIN:
+		pr_debug("%s: Main IRQ[1-3] l2=%x\n", __func__, l2irq);
+
+		if ((l2irq >= 1) && (l2irq <= 3)) {
+			type = mpc52xx_irqx_gettype(l2irq);
+			good_irqchip = &mpc52xx_extirq_irqchip;
+		} else {
+			good_irqchip = &mpc52xx_main_irqchip;
+		}
+		break;

You have changed the mapping of L1/L2 but not the device-tree
representation to match them. Which means that you need the ugly hack
above. That is not goot. Fix your device-tree instead and do the same
split in the tree for L1/L2 than you do in the code.

There is no option here.

quoted

+	picnode = find_mpc52xx_picnode();
+	sdmanode = find_mpc52xx_sdmanode();

Any reason why you have those inline 1-line functions and just not put
the actual of_find_* call in here ?

You probably also need some of_node_put() in the error case. You are
doing a whole bunch of "return" that bypass them.

I have done it because other platform may add their how type/compatible/way to find the node. For example, I did not find any bestcomm node in the recent lite5200 device-tree.

http://ozlabs.org/pipermail/linuxppc-embedded/2006-November/024970.html

The rest looks ok.

;-)

Please submit the io.h patch separately. Also, you may need to fix the
default case (which is the one we are using) in there to work with
CONFIG_PCI not set (by defining PCI_DRAM_BASE to 0 among others).

Sylvain was faster than me. :-)

Regards

quoted

You have changed the mapping of L1/L2 but not the device-tree
representation to match them. Which means that you need the ugly hack
above. That is not goot. Fix your device-tree instead and do the same
split in the tree for L1/L2 than you do in the code.

There is no option here.

I don't agree. Your firmware can always be upgraded. There is no way you
will impose a broken format on everybody else. Unfortunately, being the
first implementation, that's just what you are doing...

At worst, you'll have to do a fixup pass in prom_init.c

I have done it because other platform may add their how type/compatible/way to find the node.
For example, I did not find any bestcomm node in the recent lite5200 device-tree.

http://ozlabs.org/pipermail/linuxppc-embedded/2006-November/024970.html

They should get there ultimately.

Ben.

On Sun, 05 Nov 2006 21:37:11 +1100
Benjamin Herrenschmidt [off-list ref] wrote:

quoted

quoted

You have changed the mapping of L1/L2 but not the device-tree
representation to match them. Which means that you need the ugly hack
above. That is not goot. Fix your device-tree instead and do the same
split in the tree for L1/L2 than you do in the code.

There is no option here.

I don't agree. Your firmware can always be upgraded. There is no way you
will impose a broken format on everybody else. Unfortunately, being the
first implementation, that's just what you are doing...

As said, no option here. I think there is different way to see how works the PIC. However, from a register point of view. There is just critical, main, peripherals, SDMA.

Loot at "ICTL Perstat, MainStat, MainStat, CritStat Encoded Register--MBAR + 0x0524"

Page 160 of the MPC5200BUM.pdf.

At worst, you'll have to do a fixup pass in prom_init.c

If you really want to change the pic driver, tell me the way to adapt prom_init.c
However, I do think we do it the right way.

Regards,

On Sun, 2006-11-05 at 12:30 +0100, Nicolas DET wrote:

On Sun, 05 Nov 2006 21:37:11 +1100
Benjamin Herrenschmidt [off-list ref] wrote:

quoted

quoted

quoted

You have changed the mapping of L1/L2 but not the device-tree
representation to match them. Which means that you need the ugly hack
above. That is not goot. Fix your device-tree instead and do the same
split in the tree for L1/L2 than you do in the code.

There is no option here.

I don't agree. Your firmware can always be upgraded. There is no way you
will impose a broken format on everybody else. Unfortunately, being the
first implementation, that's just what you are doing...

As said, no option here. I think there is different way to see how works the PIC. However, from a register point of view. There is just critical, main, peripherals, SDMA.

Loot at "ICTL Perstat, MainStat, MainStat, CritStat Encoded Register--MBAR + 0x0524"

You should have your device-tree match your internal numbering. As you
noticed, the CRIT interrupt and the EXT interrupts are just the same.
And you properly folded them under the same level 1... now just make
the device-tree expose the same informations.

I don't care if you have firmwares in production or whatever... As a
matter of fact, you guys have been working on this board for long enough
you could have dealt with that issue long ago :-)

So right now, what you should do is figure out a proper encoding for the
firmware, and then either fix your device-tree, or do a hack in
prom_init.c that fixes it up.

Ben.

Benjamin Herrenschmidt wrote:

quoted

As said, no option here. I think there is different way to see how works the PIC. However, from a register point of view. There is just critical, main, peripherals, SDMA.

Loot at "ICTL Perstat, MainStat, MainStat, CritStat Encoded Register--MBAR + 0x0524"

You should have your device-tree match your internal numbering. As you
noticed, the CRIT interrupt and the EXT interrupts are just the same.

As I see, they aren't.
Critical interrupts defines IRQ 0, as well as slice timer0, WakeUp from deep-sleep mode (CCS) interrupt, etc..

Where as main is IRQ[1-3] as well as others stuff.

I did implement/change the irqchip, on your request, because in this case (IRQ0 -> l2=1, IRQ[13] -> l2=1->3). and the ack/mask bit are the same, it makes sense to have the same func called.

So right now, what you should do is figure out a proper encoding for the
firmware, and then either fix your device-tree, or do a hack in
prom_init.c that fixes it up.

I think the OFW has proper encoding, moreover, this encoding was done up on your request and opinion.

If you still want to change the Linux interrupt encoding, I will then hack prom_init.c and submmit new patches...

Regards,

quoted

As said, no option here. I think there is different way to see how works the PIC. However, from a register point of view. There is just critical, main, peripherals, SDMA.

Loot at "ICTL Perstat, MainStat, MainStat, CritStat Encoded Register--MBAR + 0x0524"
    

You should have your device-tree match your internal numbering. As you
noticed, the CRIT interrupt and the EXT interrupts are just the same.
And you properly folded them under the same level 1... now just make
the device-tree expose the same informations.

I don't care if you have firmwares in production or whatever... As a
matter of fact, you guys have been working on this board for long enough
you could have dealt with that issue long ago :-)

So right now, what you should do is figure out a proper encoding for the
firmware, and then either fix your device-tree, or do a hack in
prom_init.c that fixes it up.
  

I think Nicolas's choice makes sense.
The HW provides 4 registers which each contain the "number" of the IRQ that
happenned. And IRQ0 is encoded separatly from IRQ[1-3].

So when IRQ0 happens,  you really have a flag saying that a critical
interrupt
happenned, and when you look at the encoded critical interrupt number,
you get
"00" which is IRQ0.

If IRQ2 happend, the flag say a main interrupt happenned and when looking in
the encoded main interrupt number, we get "000001" which is IRQ2 ...

So the Hardware maps the hw IRQ number like this, so it makes sense to
use that mapping. Sure we could just not use that mapping and put IRQ0 hwirq
number just before IRQ1 but then the hack is just moved in get_irq and
we have
a numbering scheme which is not really what's in hw.


        Sylvain

On 11/4/06, Benjamin Herrenschmidt [off-list ref] wrote:

On Sun, 2006-11-05 at 01:27 +0100, Sylvain Munaut wrote:

quoted

with a helper that would do
- The find_node
 - get_address / translate /  get_size
 - ioremap

Something like :

intr = mpc52xx_find_and_map("mpc52xx-intr");
sdma = mpc52xx_find_and_map("mpc52xx-sdma");

Hrm... I don't care that much but I also don't think we need that
helper. It's not saving much.

While on this topic... if a helper is added, what about it is 52xx
specific?  Wouldn't the same code apply to all platforms?

g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

I still haven't had the time to get this code working on my board, and
I haven't followed the discussion over the last week very carefully,
but here's my comments anyway.  :)

On 11/2/06, Nicolas DET [off-list ref] wrote:

Do these belong somewhere else?  In common code somewhere?  Does this
have the side effect of looking like an atomic operation to the casual
observer?  (That's a coding convention question directed at Ben)

I don't think this is a very good idea from a maintenance perspective.
Effectively, they replace a single line of code with 5 lines (4 for
the static func, 1 where it is called).  If in the future they ever
need to be called in more than 1 or 2 places, then this could be
revisited.

L2 offset will always be 0; why not just omit all references to it?
Keeps the code slightly more concise.

I don't think you should fail silently here.

ditto

ditto

Silent failure cases above will bypass these calls to of_node_put().

Register addresses/lengths are being passed by the device tree.  I
think they should be taken out of here to remove the temptation for
driver writers to use them.

g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Grant Likely wrote:

On 11/4/06, Benjamin Herrenschmidt [off-list ref] wrote:

quoted

On Sun, 2006-11-05 at 01:27 +0100, Sylvain Munaut wrote:

quoted

with a helper that would do
- The find_node
 - get_address / translate /  get_size
 - ioremap

Something like :

intr = mpc52xx_find_and_map("mpc52xx-intr");
sdma = mpc52xx_find_and_map("mpc52xx-sdma");

Hrm... I don't care that much but I also don't think we need that
helper. It's not saving much.

While on this topic... if a helper is added, what about it is 52xx
specific?  Wouldn't the same code apply to all platforms?

g.

The code would look like what I included at the end (untested). Not that
I used
of_find_by_name and not find_compatible. We can fix a naming convention
for those units ...

I think it does save quite a few lines and variable. It also simplifies the
error path ... granted it's not an exceptionnal reduction but still
worth it.
If it's not included now it's not that bad, I'll probably submitt a
patch later
when it's used in more places than mpc52xx_pic.c ...

About the use on other platform, maybe but do other platform need that a
lot ?
Here we have several unit that need to be mapped at different places ...


Sylvain


---

void __iomem *mpc52xx_find_and_map(const char *name)
{
        struct device_node *ofn;
        const u32 *regaddr_p;
        u64 regaddr64, size64;

        ofn = of_find_by_name(NULL, name);
        if (!ofn)
                return NULL;

        regaddr_p = of_get_address(ofn, 0, &size64, NULL);
        if (!regaddr_p) {
                of_node_put(ofn);
                return NULL;
        }

        regaddr64 = of_translate_address(ofn, regaddr_p);

        of_node_put(ofn);

        return ioremap((u32)regaddr64, (u32)size64);
}