[PATCH v2 04/15] ARM: mxs: Add interrupt support

Uwe Kleine-König u.kleine-koenig at pengutronix.de
Tue Dec 7 16:03:52 EST 2010


Hello Shawn,

On Wed, Dec 08, 2010 at 12:31:54AM +0800, Shawn Guo wrote:
> Add Interrupt Collector (ICOLL) support for MXS-based.
> 
> Signed-off-by: Shawn Guo <shawn.guo at freescale.com>
> ---
> Changes for v2:
>  - Get icoll base in get_irqnr_preamble
>  - Check the case of single SoC build and use hard-coding address if possible
> 
>  arch/arm/mach-mxs/icoll.c                    |   77 ++++++++++++++++++++++++++
>  arch/arm/mach-mxs/include/mach/entry-macro.S |   44 +++++++++++++++
>  2 files changed, 121 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/icoll.c
>  create mode 100644 arch/arm/mach-mxs/include/mach/entry-macro.S
> 
> diff --git a/arch/arm/mach-mxs/icoll.c b/arch/arm/mach-mxs/icoll.c
> new file mode 100644
> index 0000000..a7a7d4f
> --- /dev/null
> +++ b/arch/arm/mach-mxs/icoll.c
> @@ -0,0 +1,77 @@
> +/*
> + * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +
> +#include <mach/hardware.h>
> +#include <mach/common.h>
> +
> +#define HW_ICOLL_VECTOR				0x0000
> +#define HW_ICOLL_LEVELACK			0x0010
> +#define HW_ICOLL_CTRL				0x0020
> +#define HW_ICOLL_INTERRUPTn_SET(n)		(0x0124 + (n) * 0x10)
> +#define HW_ICOLL_INTERRUPTn_CLR(n)		(0x0128 + (n) * 0x10)
> +#define BM_ICOLL_INTERRUPTn_ENABLE		0x00000004
> +#define BV_ICOLL_LEVELACK_IRQLEVELACK__LEVEL0	0x1
> +
> +void __iomem *icoll_base;
> +
> +static void icoll_ack_irq(unsigned int irq)
> +{
> +	__raw_writel(0, icoll_base + HW_ICOLL_VECTOR);
You need to write this before handling the irq, no?  Note this question
is a repetition.  You answered "No need, indeed.  Will remove it."
According to MCIMX28RM a write to HW_ICOLL_VECTOR "indicates the
in-service state".  Are you sure this is not needed?  If no (i.e. it's
needed) the write should go into entry-macro.S.  As I don't have
hardware yet I cannot test that.  Maybe it's only needed when the
priority levels are used?!

> +
> +	/* ACK current interrupt (level 0) */
I suggest to extend this a bit, like:

	The Interrupt Collector is able to prioritize irqs.  Currently
	only level 0 is used.  So acking can use
	BV_ICOLL_LEVELACK_IRQLEVELACK__LEVEL0 unconditionally.

> +	__raw_writel(BV_ICOLL_LEVELACK_IRQLEVELACK__LEVEL0,
> +			icoll_base + HW_ICOLL_LEVELACK);
> +}
> +
> +static void icoll_mask_irq(unsigned int irq)
> +{
> +	__raw_writel(BM_ICOLL_INTERRUPTn_ENABLE,
> +			icoll_base + HW_ICOLL_INTERRUPTn_CLR(irq));
> +}
> +
> +static void icoll_unmask_irq(unsigned int irq)
> +{
> +	__raw_writel(BM_ICOLL_INTERRUPTn_ENABLE,
> +			icoll_base + HW_ICOLL_INTERRUPTn_SET(irq));
> +}
> +
> +static struct irq_chip mxs_icoll_chip = {
> +	.ack = icoll_ack_irq,
> +	.mask = icoll_mask_irq,
> +	.unmask = icoll_unmask_irq,
> +};
> +
> +void __init icoll_init_irq(void __iomem *irqbase)
> +{
> +	int i;
> +
> +	icoll_base = irqbase;
> +
> +	/* Reset icoll */
> +	mxs_reset_block(irqbase + HW_ICOLL_CTRL);
I assume this initializes the priority for each irq to level 0.  Maybe
this should be documented here.

> +
> +	for (i = 0; i < MXS_INTERNAL_IRQS; i++) {
> +		set_irq_chip(i, &mxs_icoll_chip);
> +		set_irq_handler(i, handle_level_irq);
> +		set_irq_flags(i, IRQF_VALID);
> +	}
> +}
> diff --git a/arch/arm/mach-mxs/include/mach/entry-macro.S b/arch/arm/mach-mxs/include/mach/entry-macro.S
> new file mode 100644
> index 0000000..2c7755b
> --- /dev/null
> +++ b/arch/arm/mach-mxs/include/mach/entry-macro.S
> @@ -0,0 +1,44 @@
> +/*
> + * Low-level IRQ helper macros for Freescale MXS-based
> + *
> + * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <mach/hardware.h>
> +
> +	.macro	disable_fiq
> +	.endm
I think is isn't needed.  Only when you start to actually use the fiq.

> +	.macro	get_irqnr_and_base, irqnr, irqstat, base, tmp
> +	ldr	\irqnr, [\base, #0x70]
> +	cmp	\irqnr, #0x7F
> +	moveqs	\irqnr, #0
> +	.endm
> +
> +	.macro  get_irqnr_preamble, base, tmp
> +#if defined (CONFIG_SOC_IMX23) && defined (CONFIG_SOC_IMX28)
no space between defined and ( please.

> +	ldr	\base, =icoll_base
> +	ldr	\base, [\base]
> +#elif defined (CONFIG_SOC_IMX23)
> +	ldr	\base, =MX23_ICOLL_BASE_ADDR
> +#else
> +	ldr	\base, =MX28_ICOLL_BASE_ADDR
> +#endif
Mhh, you said the base addresses are different.  Unless I'm mistaken
they are not.  So you can just remove icoll_base.

> +	.endm
> +
> +	.macro  arch_ret_to_user, tmp1, tmp2
> +	.endm

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |



More information about the linux-arm-kernel mailing list