Re: [PATCHv2 01/11] arm: mxc: TrustZone interrupt controller (TZIC) for i.MX5 family

From: Eric Miao
Date: Wed Feb 03 2010 - 01:30:08 EST

Next message: Eric Miao: "Re: [PATCHv2 03/11] mxc: Fix Drive Strength Field in the IOMUX controller"
Previous message: Wu Fengguang: "Re: [PATCH 00/11] [RFC] 512K readahead size with thrashing safereadahead"
In reply to: Grant Likely: "Re: [PATCHv2 02/11] mxc timer: refactor timer code to use timer versions"
Next in thread: Sascha Hauer: "Re: [PATCHv2 01/11] arm: mxc: TrustZone interrupt controller(TZIC) for i.MX5 family"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Amit,

Just some nit-picking review comments, see below:

On Tue, Feb 2, 2010 at 9:16 PM, Amit Kucheria
<amit.kucheria@xxxxxxxxxxxxx> wrote:
> Freescale i.MX51 processor uses a new interrupt controller. Add
> driver for TrustZone Interrupt Controller
>
> Signed-off-by: Amit Kucheria <amit.kucheria@xxxxxxxxxxxxx>
> ---
> Âarch/arm/plat-mxc/Kconfig Â| Â Â8 ++
> Âarch/arm/plat-mxc/Makefile | Â Â3 +
> Âarch/arm/plat-mxc/tzic.c Â | Â182 ++++++++++++++++++++++++++++++++++++++++++++
> Â3 files changed, 193 insertions(+), 0 deletions(-)
> Âcreate mode 100644 arch/arm/plat-mxc/tzic.c
>
> diff --git a/arch/arm/plat-mxc/Kconfig b/arch/arm/plat-mxc/Kconfig
> index 8b0a1ee..59558c4 100644
> --- a/arch/arm/plat-mxc/Kconfig
> +++ b/arch/arm/plat-mxc/Kconfig
> @@ -62,6 +62,14 @@ config MXC_IRQ_PRIOR
> Â Â Â Â Ârequirements for timing.
> Â Â Â Â ÂSay N here, unless you have a specialized requirement.
>
> +config MXC_TZIC
> + Â Â Â bool "Enable TrustZone Interrupt Controller"
> + Â Â Â depends on ARCH_MX51

This is the first patch of the base port, yet I cannot find any reference to
this ARCH_MX51, did you miss something?

> + Â Â Â help
> + Â Â Â Â This will be automatically selected for all processors
> + Â Â Â Â containing this interrupt controller.
> + Â Â Â Â Say N here only if you are really sure.
> +
> Âconfig MXC_PWM
> Â Â Â Âtristate "Enable PWM driver"
> Â Â Â Âdepends on ARCH_MXC
> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> index 996cbac..0202ad9 100644
> --- a/arch/arm/plat-mxc/Makefile
> +++ b/arch/arm/plat-mxc/Makefile
> @@ -5,6 +5,9 @@
> Â# Common support
> Âobj-y := irq.o clock.o gpio.o time.o devices.o cpu.o system.o
>
> +# MX51 uses the TZIC interrupt controller, older platforms use AVIC (irq.o)
> +obj-$(CONFIG_MXC_TZIC) += tzic.o
> +
> Âobj-$(CONFIG_ARCH_MX1) += iomux-mx1-mx2.o dma-mx1-mx2.o
> Âobj-$(CONFIG_ARCH_MX2) += iomux-mx1-mx2.o dma-mx1-mx2.o
> Âobj-$(CONFIG_ARCH_MXC_IOMUX_V3) += iomux-v3.o
> diff --git a/arch/arm/plat-mxc/tzic.c b/arch/arm/plat-mxc/tzic.c
> new file mode 100644
> index 0000000..00cb0ad
> --- /dev/null
> +++ b/arch/arm/plat-mxc/tzic.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2004-2009 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +
> +#include <asm/mach/irq.h>
> +
> +#include <mach/hardware.h>
> +
> +/*
> + *****************************************
> + * TZIC Registers Â Â Â Â Â Â Â Â Â Â Â Â*
> + *****************************************
> + */
> +
> +#define TZIC_INTCNTL Â Â Â Â Â Â0x0000 /* Control register */
> +#define TZIC_INTTYPE Â Â Â Â Â Â0x0004 /* Controller Type register */
> +#define TZIC_IMPID Â Â Â Â Â Â Â0x0008 /* Distributor Implementer Identification */
> +#define TZIC_PRIOMASK Â Â Â Â Â 0x000C /* Priority Mask Reg */
> +#define TZIC_SYNCCTRL Â Â Â Â Â 0x0010 /* Synchronizer Control register */
> +#define TZIC_DSMINT Â Â Â Â Â Â 0x0014 /* DSM interrupt Holdoffregister */
> +#define TZIC_INTSEC0 Â Â Â Â Â Â0x0080 /* Interrupt Security register 0 */
> +#define TZIC_ENSET0 Â Â Â Â Â Â 0x0100 /* Enable Set Register 0 */
> +#define TZIC_ENCLEAR0 Â Â Â Â Â 0x0180 /* Enable Clear Register 0 */
> +#define TZIC_SRCSET0 Â Â Â Â Â Â0x0200 /* Source Set Register 0 */
> +#define TZIC_SRCCLAR0 Â Â Â Â Â 0x0280 /* Source Clear Register 0 */
> +#define TZIC_PRIORITY0 Â Â Â Â Â0x0400 /* Priority Register 0 */
> +#define TZIC_PND0 Â Â Â Â Â Â Â 0x0D00 /* Pending Register 0 */
> +#define TZIC_HIPND0 Â Â Â Â Â Â 0x0D80 /* High Priority Pending Register */
> +#define TZIC_WAKEUP0 Â Â Â Â Â Â0x0E00 /* Wakeup Config Register */
> +#define TZIC_SWINT Â Â Â Â Â Â Â0x0F00 /* Software Interrupt Rigger Register */
> +#define TZIC_ID0 Â Â Â Â Â Â Â Â0x0FD0 /* Indentification Register 0 */
> +
> +void __iomem *tzic_base;

This can just be made to 'static' if it's not used elsewhere, and I'm
wondering if it's neater to define them as:

#define TZIC_INTCNTL (tzic_base + 0x0000)

so to make the code below short and handy.

> +
> +/*
> + * Disable interrupt number "irq" in the TZIC

I don't think this follows kernel API doc exactly, you may want to have a
look into Documentation/kernel-doc-nano-HOWTO.txt.

> + *
> + * @param Âirq Â Â Â Â Âinterrupt source number
> + */
> +static void tzic_mask_irq(unsigned int irq)
> +{
> + Â Â Â int index, off;
> +
> + Â Â Â index = irq >> 5;
> + Â Â Â off = irq & 0x1F;
> + Â Â Â __raw_writel(1 << off, tzic_base + TZIC_ENCLEAR0 + (index << 2));

I'll normally define TZIC_ENCLEAR0 then as:

#define TZIC_ENCLEAR(i) (0x0180 + ((i) << 2))

so the above can be written as:

__raw_writel(1 << off, tzic_base + TZIC_ENCLEAR(index));

or by including tzic_base into TZIC_*, simply as:

__raw_writel(1 << off, TZIC_ENCLEAR(index));

> +}
> +
> +/*
> + * Enable interrupt number "irq" in the TZIC
> + *
> + * @param Âirq Â Â Â Â Âinterrupt source number
> + */
> +static void tzic_unmask_irq(unsigned int irq)
> +{
> + Â Â Â int index, off;
> +
> + Â Â Â index = irq >> 5;
> + Â Â Â off = irq & 0x1F;
> + Â Â Â __raw_writel(1 << off, tzic_base + TZIC_ENSET0 + (index << 2));
> +}
> +
> +static unsigned int wakeup_intr[4];
> +
> +/*
> + * Set interrupt number "irq" in the TZIC as a wake-up source.
> + *
> + * @param Âirq Â Â Â Â Âinterrupt source number
> + * @param Âenable Â Â Â enable as wake-up if equal to non-zero
> + * Â Â Â Â Â Â Â Â Â Â disble as wake-up if equal to zero
> + *
> + * @return Â Â Â This function returns 0 on success.
> + */
> +static int tzic_set_wake_irq(unsigned int irq, unsigned int enable)
> +{
> + Â Â Â unsigned int index, off;
> +
> + Â Â Â index = irq >> 5;
> + Â Â Â off = irq & 0x1F;
> +
> + Â Â Â if (index > 3)
> + Â Â Â Â Â Â Â return -EINVAL;
> +
> + Â Â Â if (enable)
> + Â Â Â Â Â Â Â wakeup_intr[index] |= (1 << off);
> + Â Â Â else
> + Â Â Â Â Â Â Â wakeup_intr[index] &= ~(1 << off);
> +
> + Â Â Â return 0;
> +}
> +
> +static struct irq_chip mxc_tzic_chip = {
> + Â Â Â .name = "MXC_TZIC",
> + Â Â Â .ack = tzic_mask_irq,
> + Â Â Â .mask = tzic_mask_irq,
> + Â Â Â .unmask = tzic_unmask_irq,
> + Â Â Â .set_wake = tzic_set_wake_irq,
> +};
> +
> +/*
> + * This function initializes the TZIC hardware and disables all the
> + * interrupts. It registers the interrupt enable and disable functions
> + * to the kernel for each interrupt source.
> + */
> +void __init tzic_init_irq(void __iomem *irqbase)
> +{
> + Â Â Â int i;
> +
> + Â Â Â tzic_base = irqbase;
> + Â Â Â /* put the TZIC into the reset value with
> + Â Â Â Â* all interrupts disabled
> + Â Â Â Â*/
> + Â Â Â i = __raw_readl(tzic_base + TZIC_INTCNTL);

Mixing the use of 'i' as both a signed counter and register value might
not be a good idea, provided it's not guaranteed from theory that 'i' as
an integer could not be sufficient to hold the value returned from
__raw_readl()

> +
> + Â Â Â __raw_writel(0x80010001, tzic_base + TZIC_INTCNTL);
> + Â Â Â i = __raw_readl(tzic_base + TZIC_INTCNTL);
> + Â Â Â __raw_writel(0x1f, tzic_base + TZIC_PRIOMASK);
> + Â Â Â i = __raw_readl(tzic_base + TZIC_PRIOMASK);
> + Â Â Â __raw_writel(0x02, tzic_base + TZIC_SYNCCTRL);
> + Â Â Â i = __raw_readl(tzic_base + TZIC_SYNCCTRL);

Are these read-back really necessary? We can start without them and add them
later if they do cause issues.

> +
> + Â Â Â for (i = 0; i < 4; i++)
> + Â Â Â Â Â Â Â __raw_writel(0xFFFFFFFF, tzic_base + TZIC_INTSEC0 + i * 4);
> +
> + Â Â Â /* disable all interrupts */
> + Â Â Â for (i = 0; i < 4; i++)
> + Â Â Â Â Â Â Â __raw_writel(0xFFFFFFFF, tzic_base + TZIC_ENCLEAR0 + i * 4);
> +
> + Â Â Â /* all IRQ no FIQ Warning :: No selection */
> +
> + Â Â Â for (i = 0; i < MXC_INTERNAL_IRQS; i++) {
> + Â Â Â Â Â Â Â set_irq_chip(i, &mxc_tzic_chip);
> + Â Â Â Â Â Â Â set_irq_handler(i, handle_level_irq);
> + Â Â Â Â Â Â Â set_irq_flags(i, IRQF_VALID);
> + Â Â Â }
> +
> + Â Â Â printk(KERN_INFO "TrustZone Interrupt Controller (TZIC) initialized\n");

You may want to use pr_info() for short.

> +}
> +
> +/*
> + * enable wakeup interrupt
> + *
> + * @param is_idle Â Â Â Â Â Â Â1 if called in idle loop (ENSET register);
> + * Â Â Â Â Â Â Â Â Â Â Â Â Â Â 0 to be used when called from low power entry
> + * @return Â Â Â Â Â Â Â Â Â Â 0 if successful; non-zero otherwise
> + *
> + */
> +int tzic_enable_wake(int is_idle)
> +{
> + Â Â Â unsigned int i, v;
> +
> + Â Â Â __raw_writel(1, tzic_base + TZIC_DSMINT);
> + Â Â Â if (unlikely(__raw_readl(tzic_base + TZIC_DSMINT) == 0))
> + Â Â Â Â Â Â Â return -EAGAIN;

Looks like an unnecessary read-back provided the silicon is sane enough.

> +
> + Â Â Â if (likely(is_idle)) {
> + Â Â Â Â Â Â Â for (i = 0; i < 4; i++) {
> + Â Â Â Â Â Â Â Â Â Â Â v = __raw_readl(tzic_base + TZIC_ENSET0 + i * 4);
> + Â Â Â Â Â Â Â Â Â Â Â __raw_writel(v, tzic_base + TZIC_WAKEUP0 + i * 4);
> + Â Â Â Â Â Â Â }
> + Â Â Â } else {
> + Â Â Â Â Â Â Â for (i = 0; i < 4; i++) {
> + Â Â Â Â Â Â Â Â Â Â Â v = wakeup_intr[i];
> + Â Â Â Â Â Â Â Â Â Â Â __raw_writel(v, tzic_base + TZIC_WAKEUP0 + i * 4);
> + Â Â Â Â Â Â Â }
> + Â Â Â }

Or could be simplified to:

for (i = 0; i < 4; i++) {
v = is_idle ? __raw_readl(TZIC_ENSET(i)) : wakeup_intr[i];
__raw_writel(v, TZIC_WAKEUP(i));
}

but just nit-picking comments, so it's up to you.

> + Â Â Â return 0;
> +}

Mmmm.... this being called elsewhere, I'm thinking about making this a
sys_device and having this called within sysdev_class.suspend() to make
this file rather self-contained.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Eric Miao: "Re: [PATCHv2 03/11] mxc: Fix Drive Strength Field in the IOMUX controller"
Previous message: Wu Fengguang: "Re: [PATCH 00/11] [RFC] 512K readahead size with thrashing safereadahead"
In reply to: Grant Likely: "Re: [PATCHv2 02/11] mxc timer: refactor timer code to use timer versions"
Next in thread: Sascha Hauer: "Re: [PATCHv2 01/11] arm: mxc: TrustZone interrupt controller(TZIC) for i.MX5 family"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]