From: "Grant Likely" <grant.likely@secretlab•ca>
To: "Nicolas DET" <nd@bplan-gmbh•de>
Cc: linuxppc-dev@ozlabs•org, sl@bplan-gmbh•de, linuxppc-embedded@ozlabs•org
Subject: Re: [PATCH/RFC] powerpc: Add MPC5200 Interrupt Controller support.
Date: Sun, 5 Nov 2006 23:55:07 -0700 [thread overview]
Message-ID: <528646bc0611052255m6faf6466of48fcbb3d74ffaa5@mail.gmail.com> (raw)
In-Reply-To: <454A5952.6090801@bplan-gmbh.de>
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 <nd@bplan-gmbh•de> wrote:
> --- a/arch/powerpc/sysdev/mpc52xx_pic.c 1970-01-01 01:00:00.000000000 +0100
> +++ b/arch/powerpc/sysdev/mpc52xx_pic.c 2006-11-02 17:56:10.000000000 +0100
> +static inline void io_be_setbit(u32 __iomem *addr, int bitno)
> +{
> + out_be32(addr, in_be32(addr) | (1 << bitno) );
> +}
> +
> +static inline void io_be_clrbit(u32 __iomem *addr, int bitno)
> +{
> + out_be32(addr, in_be32(addr) & ~(1 << bitno));
> +}
> +
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)
> +static inline struct device_node *find_mpc52xx_picnode(void)
> +{
> + return of_find_compatible_node(NULL, "interrupt-controller",
> + "mpc5200-pic");
> +}
> +
> +static inline struct device_node *find_mpc52xx_sdmanode(void)
> +{
> + return of_find_compatible_node(NULL, "dma-controller",
> + "mpc5200-bestcomm");
> +}
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.
> +
> +/*
> + * IRQ[0-3] interrupt irq_chip
> +*/
> +
> +static void mpc52xx_extirq_mask(unsigned int virq)
> +{
> + int irq;
> + int l2irq;
> +
> + irq = irq_map[virq].hwirq;
> + l2irq = (irq & MPC52xx_IRQ_L2_MASK) >> MPC52xx_IRQ_L2_OFFSET;
L2 offset will always be 0; why not just omit all references to it?
Keeps the code slightly more concise.
> +/*
> + * init (public)
> +*/
> +
> +void __init mpc52xx_init_irq(void)
> +{
> + struct device_node *picnode;
> + int picnode_regsize;
> + u32 picnode_regoffset;
> +
> + struct device_node *sdmanode;
> + int sdmanode_regsize;
> + u32 sdmanode_regoffset;
> +
> + u64 size64;
> + int flags;
> +
> + u32 intr_ctrl;
> +
> + picnode = find_mpc52xx_picnode();
> + sdmanode = find_mpc52xx_sdmanode();
> +
> + if ( (picnode == NULL) || (sdmanode == NULL) )
> + return;
I don't think you should fail silently here.
> +
> + /* Retrieve PIC ressources */
> + picnode_regoffset = (u32) of_get_address(picnode, 0, &size64, &flags);
> + if (picnode_regoffset == 0)
> + return ;
ditto
> +
> + picnode_regoffset = of_translate_address(picnode, (u32 *) picnode_regoffset);
> + picnode_regsize = (int)size64;
> +
> + /* Retrieve SDMA ressources */
> + sdmanode_regoffset = (u32) of_get_address(sdmanode, 0, &size64, &flags);
> + if (sdmanode_regoffset == 0)
> + return ;
ditto
> +
> + sdmanode_regoffset = of_translate_address(sdmanode, (u32 *) sdmanode_regoffset);
> + sdmanode_regsize = (int)size64;
> +
> + /* Remap the necessary zones */
> + intr = ioremap(picnode_regoffset, picnode_regsize);
> + sdma = ioremap(sdmanode_regoffset, sdmanode_regsize);
> +
> + if ((intr == NULL) || (sdma == NULL))
> + panic("Can't ioremap PIC/SDMA register or init_irq !");
> +
> + printk(KERN_INFO "Remapped MPC52xx PIC at 0x%8.8x\n", picnode_regoffset);
> + printk(KERN_INFO "Remapped MPC52xx SDMA at 0x%8.8x\n", sdmanode_regoffset);
> +
> + of_node_put(picnode);
> + of_node_put(sdmanode);
Silent failure cases above will bypass these calls to of_node_put().
> +
> + /* Disable all interrupt sources. */
> + out_be32(&sdma->IntPend, 0xffffffff); /* 1 means clear pending */
> + out_be32(&sdma->IntMask, 0xffffffff); /* 1 means disabled */
> + out_be32(&intr->per_mask, 0x7ffffc00); /* 1 means disabled */
> + out_be32(&intr->main_mask, 0x00010fff); /* 1 means disabled */
> + intr_ctrl = in_be32(&intr->ctrl);
> + intr_ctrl &= 0x00ff0000; /* Keeps IRQ[0-3] config */
> + intr_ctrl |= 0x0f000000 | /* clear IRQ 0-3 */
> + 0x00001000 | /* MEE master external enable */
> + 0x00000000 | /* 0 means disable IRQ 0-3 */
> + 0x00000001; /* CEb route critical normally */
> + out_be32(&intr->ctrl, intr_ctrl);
> +
> + /* Zero a bunch of the priority settings. */
> + out_be32(&intr->per_pri1, 0);
> + out_be32(&intr->per_pri2, 0);
> + out_be32(&intr->per_pri3, 0);
> + out_be32(&intr->main_pri1, 0);
> + out_be32(&intr->main_pri2, 0);
> +
> + /*
> + * As last step, add an irq host to translate the real
> + * hw irq information provided by the ofw to linux virq
> + */
> +
> + mpc52xx_irqhost =
> + irq_alloc_host(IRQ_HOST_MAP_LINEAR, MPC52xx_IRQ_HIGHTESTVIRQ,
> + &mpc52xx_irqhost_ops, -1);
> +
> + if (mpc52xx_irqhost)
> + mpc52xx_irqhost->host_data = picnode;
> +}
> +
> +/*
> + * get_irq (public)
> +*/
> +unsigned int mpc52xx_get_irq(void)
> +{
> + u32 status;
> + int irq = NO_IRQ_IGNORE;
> +
> + status = in_be32(&intr->enc_status);
> + if (status & 0x00000400) { /* critical */
> + irq = (status >> 8) & 0x3;
> + if (irq == 2) /* high priority peripheral */
> + goto peripheral;
> + irq |=
> + (MPC52xx_IRQ_L1_CRIT << MPC52xx_IRQ_L1_OFFSET) &
> + MPC52xx_IRQ_L1_MASK;
> + } else if (status & 0x00200000) { /* main */
> + irq = (status >> 16) & 0x1f;
> + if (irq == 4) /* low priority peripheral */
> + goto peripheral;
> + irq |=
> + (MPC52xx_IRQ_L1_MAIN << MPC52xx_IRQ_L1_OFFSET) &
> + MPC52xx_IRQ_L1_MASK;
> + } else if (status & 0x20000000) { /* peripheral */
> + peripheral:
> + irq = (status >> 24) & 0x1f;
> + if (irq == 0) { /* bestcomm */
> + status = in_be32(&sdma->IntPend);
> + irq = ffs(status) - 1;
> + irq |=
> + (MPC52xx_IRQ_L1_SDMA << MPC52xx_IRQ_L1_OFFSET) &
> + MPC52xx_IRQ_L1_MASK;
> + } else
> + irq |=
> + (MPC52xx_IRQ_L1_PERP << MPC52xx_IRQ_L1_OFFSET) &
> + MPC52xx_IRQ_L1_MASK;
> +
> + }
> +
> + pr_debug("%s: irq=%x. virq=%d\n", __func__, irq,
> + irq_linear_revmap(mpc52xx_irqhost, irq));
> +
> + return irq_linear_revmap(mpc52xx_irqhost, irq);
> +}
> --- a/include/asm-powerpc/mpc52xx.h 1970-01-01 01:00:00.000000000 +0100
> +++ b/include/asm-powerpc/mpc52xx.h 2006-11-02 17:54:37.000000000 +0100
> @@ -0,0 +1,319 @@
> +/*
> + *
> + * Prototypes, etc. for the Freescale MPC52xx embedded cpu chips
> + * May need to be cleaned as the port goes on ...
> + *
> + * Copyright (C) 2004-2005 Sylvain Munaut <tnt@246tNt•com>
> + * 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.
> + */
> +
> +#ifndef __ASM_POWERPC_MPC52xx_H__
> +#define __ASM_POWERPC_MPC52xx_H__
> +
> +#ifndef __ASSEMBLY__
> +#include <asm/types.h>
> +#include <asm/prom.h>
> +
> +#endif /* __ASSEMBLY__ */
> +
> +/* ======================================================================== */
> +/* Main registers/struct addresses */
> +/* ======================================================================== */
> +
> +/* Registers zone offset/size */
> +#define MPC52xx_MMAP_CTL_OFFSET 0x0000
> +#define MPC52xx_MMAP_CTL_SIZE 0x068
> +#define MPC52xx_SDRAM_OFFSET 0x0100
> +#define MPC52xx_SDRAM_SIZE 0x010
> +#define MPC52xx_CDM_OFFSET 0x0200
> +#define MPC52xx_CDM_SIZE 0x038
> +#define MPC52xx_INTR_OFFSET 0x0500
> +#define MPC52xx_INTR_SIZE 0x04c
> +#define MPC52xx_GPTx_OFFSET(x) (0x0600 + ((x)<<4))
> +#define MPC52xx_GPT_SIZE 0x010
> +#define MPC52xx_RTC_OFFSET 0x0800
> +#define MPC52xx_RTC_SIZE 0x024
> +#define MPC52xx_GPIO_OFFSET 0x0b00
> +#define MPC52xx_GPIO_SIZE 0x040
> +#define MPC52xx_GPIO_WKUP_OFFSET 0x0c00
> +#define MPC52xx_GPIO_WKUP_SIZE 0x028
> +#define MPC52xx_PCI_OFFSET 0x0d00
> +#define MPC52xx_PCI_SIZE 0x100
> +#define MPC52xx_SDMA_OFFSET 0x1200
> +#define MPC52xx_SDMA_SIZE 0x100
> +#define MPC52xx_XLB_OFFSET 0x1f00
> +#define MPC52xx_XLB_SIZE 0x100
> +#define MPC52xx_PSCx_OFFSET(x) (((x)!=6)?(0x1e00+((x)<<9)):0x2c00)
> +#define MPC52xx_PSC_SIZE 0x0a0
> +
> +/* SRAM used for SDMA */
> +#define MPC52xx_SRAM_OFFSET 0x8000
> +#define MPC52xx_SRAM_SIZE 0x4000
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
next prev parent reply other threads:[~2006-11-06 6:55 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-11-01 20:27 [PATCH/RFC] powerpc: Add MPC5200 Interrupt Controller support Nicolas DET
2006-11-01 22:05 ` Dale Farnsworth
2006-11-01 22:07 ` Sven Luther
2006-11-01 22:21 ` Dale Farnsworth
2006-11-01 22:12 ` Benjamin Herrenschmidt
2006-11-02 16:27 ` Nicolas DET
2006-11-02 20:47 ` Nicolas DET
2006-11-04 23:35 ` Benjamin Herrenschmidt
2006-11-05 0:27 ` Sylvain Munaut
2006-11-05 1:13 ` Benjamin Herrenschmidt
2006-11-06 6:28 ` Grant Likely
2006-11-06 8:39 ` Sylvain Munaut
2006-11-05 10:17 ` Nicolas DET
2006-11-05 10:37 ` Benjamin Herrenschmidt
2006-11-05 11:30 ` Nicolas DET
2006-11-05 13:02 ` Benjamin Herrenschmidt
2006-11-05 13:16 ` Nicolas DET
2006-11-05 14:32 ` Sylvain Munaut
2006-11-06 6:55 ` Grant Likely [this message]
-- strict thread matches above, loose matches on Subject: below --
2006-11-06 10:26 Nicolas DET
2006-11-06 11:03 Nicolas DET
2006-11-06 23:35 ` Sylvain Munaut
2006-11-07 9:22 ` Nicolas DET
2006-11-07 10:40 Nicolas DET
2006-11-07 10:45 ` Sylvain Munaut
2006-11-07 16:30 ` Grant Likely
2006-11-07 20:52 ` Sylvain Munaut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=528646bc0611052255m6faf6466of48fcbb3d74ffaa5@mail.gmail.com \
--to=grant.likely@secretlab$(echo .)ca \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=linuxppc-embedded@ozlabs$(echo .)org \
--cc=nd@bplan-gmbh$(echo .)de \
--cc=sl@bplan-gmbh$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox