| 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: <20131008132421.GC1412@e106331-lin.cambridge.arm.com>
Date: Tue, 8 Oct 2013 14:24:22 +0100
From: Mark Rutland <mark.rutland@....com>
To: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc: Jason Cooper <jason@...edaemon.net>,
Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
Arnd Bergmann <arnd@...db.de>,
Thomas Gleixner <tglx@...utronix.de>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/8] irqchip: add DesignWare APB ICTL interrupt
controller
On Tue, Oct 08, 2013 at 01:24:26PM +0100, Sebastian Hesselbarth wrote:
> This adds an irqchip driver and corresponding devicetree binding for the
> secondary interrupt controllers based on Synopsys DesignWare IP dw_apb_ictl.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
> ---
> Changelog:
> RFCv1->RFCv2:
> - added copyright reference
>
> Cc: Jason Cooper <jason@...edaemon.net>
> Cc: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: devicetree@...r.kernel.org
> Cc: linux-doc@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> ---
> .../interrupt-controller/snps,dw-apb-ictl.txt | 29 ++++
> drivers/irqchip/Kconfig | 4 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-dw-apb-ictl.c | 142 ++++++++++++++++++++
> 4 files changed, 176 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt
> create mode 100644 drivers/irqchip/irq-dw-apb-ictl.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt b/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt
> new file mode 100644
> index 0000000..7ccd1ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt
> @@ -0,0 +1,29 @@
> +Synopsys DesignWare APB interrupt controller (dw_apb_ictl)
> +
> +Synopsys DesignWare provides interrupt controller IP for APB known as
> +dw_apb_ictl. The IP is used as secondary interrupt controller in some SoCs with
> +APB bus, e.g. Marvell Armada 1500.
> +
> +Required properties:
> +- compatible: shall be "snps,dw-apb-ictl"
> +- reg: base address of interrupt registers starting with ENABLE_LOW register
Is ENABLE_LOW the first register? Or are there registers before?
Is there only one bank of registers that needs to be defined?
This isn't just a base address, as it has a size too. The terminology's
rather inconsistent for reg properties in general...
> +- interrupt-controller: identifies the node as an interrupt controller
> +- #interrupt-cells: number of cells to encode an interrupt source, shall be 1
s/interrupt source/interrupt-specifier/
> +- interrupts: interrupt reference to primary interrupt controller
- interrupts: interrupts specifier for the sole interrupt fed to the
parent interrupt controller.
Is there only a single output interrupt?
Is this required? Is it possible for this to be wired directly into a
CPU rather than another interrupt controller?
> +- interrupt-parent: (optional) reference specific primary interrupt controller
> +
> +The interrupt sources map to the corresponding bits in the interrupt
> +registers, i.e.
> +- 0 maps to bit 0 of low interrupts,
> +- 1 maps to bit 1 of low interrupts,
> +- 32 maps to bit 0 of high interrupts, and so on.
I couldn't see any public documentation for this, so I can't really
follow the "and so on", but I saw that this had optional FIQ support so
I assume there are more interrupt values that can be encoded?
> +
> +Example:
> + aic: interrupt-controller@...0 {
> + compatible = "snps,dw-apb-ictl";
> + reg = <0x3000 0xc00>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> + };
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3792a1a..940638d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -30,6 +30,10 @@ config ARM_VIC_NR
> The maximum number of VICs available in the system, for
> power management.
>
> +config DW_APB_ICTL
> + bool
> + select IRQ_DOMAIN
> +
> config IMGPDC_IRQ
> bool
> select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c60b901..6427323 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_MMP) += irq-mmp.o
> obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
> obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
> obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
> +obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
> obj-$(CONFIG_METAG) += irq-metag-ext.o
> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
> obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
> new file mode 100644
> index 0000000..bbcacee
> --- /dev/null
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -0,0 +1,142 @@
> +/*
> + * Synopsys DW APB ICTL irqchip driver.
> + *
> + * Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
> + *
> + * based on GPL'ed 2.6 kernel sources
> + * (c) Marvell International Ltd.
> + *
> + * 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.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include "irqchip.h"
> +
> +#define APB_INT_ENABLE_L 0x00
> +#define APB_INT_ENABLE_H 0x04
> +#define APB_INT_MASK_L 0x08
> +#define APB_INT_MASK_H 0x0c
> +#define APB_INT_FINALSTATUS_L 0x30
> +#define APB_INT_FINALSTATUS_H 0x34
> +
> +static void dw_apb_ictl_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_get_chip(irq);
> + struct irq_chip_generic *gc = irq_get_handler_data(irq);
> + struct irq_domain *d = gc->private;
> + u32 stat;
> + int n;
> +
> + chained_irq_enter(chip, desc);
> +
> + for (n = 0; n < gc->num_ct; n++) {
> + stat = readl_relaxed(gc->reg_base +
> + APB_INT_FINALSTATUS_L + 4 * n);
> + while (stat) {
> + u32 hwirq = ffs(stat) - 1;
> + generic_handle_irq(irq_find_mapping(d,
> + gc->irq_base + hwirq + 32 * n));
> + stat &= ~(1 << hwirq);
> + }
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int __init dw_apb_ictl_init(struct device_node *np,
> + struct device_node *parent)
> +{
> + unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
> + struct resource r;
> + struct irq_domain *domain;
> + struct irq_chip_generic *gc;
> + void __iomem *iobase;
> + int ret, nrirqs, irq;
> + u32 reg;
> +
> + /* Map the parent interrupt for the chained handler */
> + irq = irq_of_parse_and_map(np, 0);
> + if (irq <= 0) {
> + pr_err("%s: unable to parse irq\n", np->name);
> + return -EINVAL;
> + }
> +
> + ret = of_address_to_resource(np, 0, &r);
> + if (ret) {
> + pr_err("%s: unable to get resource\n", np->name);
> + return ret;
> + }
> +
> + if (!request_mem_region(r.start, resource_size(&r), np->name)) {
> + pr_err("%s: unable to request mem region\n", np->name);
> + return -ENOMEM;
> + }
> +
> + iobase = ioremap(r.start, resource_size(&r));
> + if (!iobase) {
> + pr_err("%s: unable to map resource\n", np->name);
> + return -ENOMEM;
> + }
Could you not use of_iomap?
Also, I'd recommend using np->full_name for error messages, as that
gives you the full path for the node, which is far more helpful for
debugging than just the unqualified node name.
> +
> + /*
> + * DW IP can be configured to allow 2-64 irqs. We can determine
> + * the number of irqs supported by writing into enable register
> + * and look for bits not set, as corresponding flip-flops will
> + * have been removed by sythesis tool.
> + */
Is that always true?
Cheers,
Mark.
--
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