| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20091203225912.6ea38585@mycelium.queued.net>
Date: Thu, 3 Dec 2009 22:59:12 -0500
From: Andres Salomon <dilinger@...labora.co.uk>
To: linux-kernel@...r.kernel.org
Cc: linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC][PATCH 01/10] arm: mxc: New interrupt controller (TZIC)
for i.MX5 family
Hi,
Some minor style comments/changes suggested below..
On Fri, 4 Dec 2009 04:47:01 +0200
Amit Kucheria <amit.kucheria@...onical.com> wrote:
> Freescale i.MX51 processor uses a different interrupt controller. Add the
> driver and fix the Makefile to account for it.
>
[...]
> +/*
> + *****************************************
> + * TZIC Registers *
> + *****************************************
> + */
> +
> +#define TZIC_INTCNTL 0x0000 /* control register */
> +#define TZIC_INTTYPE 0x0004 /* Controller type register */
> +#define TZIC_IMPID 0x0008 /* Distributor Implementer Identification Register */
> +#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 */
s/Indent/Ident/ ?
> +
> +void __iomem *tzic_base;
This should be static, no?
> +
> +/*!
> + * Disable interrupt number "irq" in the TZIC
> + *
> + * @param irq interrupt source number
> + */
> +static void mxc_mask_irq(unsigned int irq)
> +{
> + int index, off;
Presumably you'll want to use unsigned values here for bit
twiddling, to play it safe.
> +
> + index = irq >> 5;
> + off = irq & 0x1F;
> + __raw_writel(1 << off, tzic_base + TZIC_ENCLEAR0 + (index << 2));
> +}
> +
> +/*!
> + * Enable interrupt number "irq" in the TZIC
> + *
> + * @param irq interrupt source number
> + */
> +static void mxc_unmask_irq(unsigned int irq)
> +{
> + int index, off;
Again, unsigned?
> +
> + 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 mxc_set_wake_irq(unsigned int irq, unsigned int enable)
> +{
> + unsigned int index, off;
> +
> + index = irq >> 5;
> + off = irq & 0x1F;
> +
> + if (index > 3)
> + return -1;
I'd suggest -EINVAL here or something. Not that most set_irq_wake()
callers actually check the results, but still.
> +
> + 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 = mxc_mask_irq,
> + .mask = mxc_mask_irq,
> + .unmask = mxc_unmask_irq,
> + .set_wake = mxc_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 mxc_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);
> +
> + __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);
> + 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);
> + }
Either the loops should be merged, or the { } should be dropped from each.
> +
> + /* 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 "MXC IRQ initialized\n");
> +}
> +
> +/*
> + * enable wakeup interrupt
> + *
> + * @param is_idle 1 if called in idle loop (enset registers);
> + * 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;
> +
> + 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);
> + }
> + }
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists