| 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: <4F3A3678.8090706@atmel.com>
Date: Tue, 14 Feb 2012 11:24:56 +0100
From: Nicolas Ferre <nicolas.ferre@...el.com>
To: Rob Herring <robherring2@...il.com>, plagnioj@...osoft.com
CC: linux-arm-kernel@...ts.infradead.org, grant.likely@...retlab.ca,
rob.herring@...xeda.com, linux-kernel@...r.kernel.org,
tglx@...utronix.de, devicetree-discuss@...ts.ozlabs.org,
avictor.za@...il.com
Subject: Re: [PATCH v5 1/9] ARM: at91/aic: add irq domain and device tree
support
On 02/13/2012 11:10 PM, Rob Herring :
> On 02/13/2012 08:43 AM, Nicolas Ferre wrote:
>> Add an irqdomain for the AIC interrupt controller.
>> The device tree support is mapping the registers and
>> is using the irq_domain_add_legacy() to manage hwirq
>> translation.
>> The documentation is describing the meaning of the
>> two cells required for using this "interrupt-controller"
>> in a device tree node.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@...el.com>
>> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
>> ---
>> v5: - rebased on top of Grant's "irq_domain generalization and refinement"
>> patch series
>> - use of irq_domain_add_legacy() API
>>
>> v4: - use irq_alloc_descs() to find irq_base
>> - add a new constant AIC_BASE_IRQ that will allow to skip
>> first interrupt numbers (in the future)
>>
>> v3: - change number of cells to define an AIC interrupt (irq trigger type)
>> - change current .dtsi files to match specification
>> - use irq_domain_simple_ops (for DT mapping)
>>
>> v2: - use of_irq_init() function for device tree probing
>> - add documentation
>> - use own simple struct irq_domain_ops
>>
>>
>> .../devicetree/bindings/arm/atmel-aic.txt | 38 ++++++++++
>> arch/arm/Kconfig | 1 +
>> arch/arm/boot/dts/at91sam9g20.dtsi | 18 +++---
>> arch/arm/boot/dts/at91sam9g45.dtsi | 16 ++--
>> arch/arm/mach-at91/include/mach/irqs.h | 3 +-
>> arch/arm/mach-at91/irq.c | 74 ++++++++++++++++----
>> 6 files changed, 119 insertions(+), 31 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/arm/atmel-aic.txt
[..]
>> --- a/arch/arm/mach-at91/include/mach/irqs.h
>> +++ b/arch/arm/mach-at91/include/mach/irqs.h
>> @@ -25,6 +25,7 @@
>> #include <mach/at91_aic.h>
>>
>> #define NR_AIC_IRQS 32
>> +#define AIC_BASE_IRQ 0
>>
>>
>> /*
>> @@ -40,7 +41,7 @@
>> * symbols in gpio.h for ones handled indirectly as GPIOs.
>> * We make provision for 5 banks of GPIO.
>> */
>> -#define NR_IRQS (NR_AIC_IRQS + (5 * 32))
>> +#define NR_IRQS (AIC_BASE_IRQ + NR_AIC_IRQS + (5 * 32))
>
> Why? You're not changing anything.
>
> You should turn on SPARSE_IRQ at some point and then this value goes away.
Yes, my intention is to move to this step by step. I have tried hard to
rework completely the AIC driver without success so far. I plan:
- move the AIC_BASE_IRQ away from irq0
=> this is why I introduce a constant here
- use SPARSE_IRQ
- move to generic irq & irq domain helpers (move DT case to "linear")
*once* the code for handling all this is stabilized and integrated in
mainline.
This rework is designed for 3.4 and a lot of code depend on this for all
our at91 DT move.
>> /* FIQ is AIC source 0. */
>> #define FIQ_START AT91_ID_FIQ
>> diff --git a/arch/arm/mach-at91/irq.c b/arch/arm/mach-at91/irq.c
>> index be6b639..880da98 100644
>> --- a/arch/arm/mach-at91/irq.c
>> +++ b/arch/arm/mach-at91/irq.c
>> @@ -24,6 +24,12 @@
>> #include <linux/module.h>
>> #include <linux/mm.h>
>> #include <linux/types.h>
>> +#include <linux/irq.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/err.h>
>>
>> #include <mach/hardware.h>
>> #include <asm/irq.h>
>> @@ -34,22 +40,24 @@
>> #include <asm/mach/map.h>
>>
>> void __iomem *at91_aic_base;
>> +static struct irq_domain *at91_aic_domain;
>> +static struct device_node *at91_aic_np;
>>
>> static void at91_aic_mask_irq(struct irq_data *d)
>> {
>> /* Disable interrupt on AIC */
>> - at91_aic_write(AT91_AIC_IDCR, 1 << d->irq);
>> + at91_aic_write(AT91_AIC_IDCR, 1 << d->hwirq);
>> }
>>
>> static void at91_aic_unmask_irq(struct irq_data *d)
>> {
>> /* Enable interrupt on AIC */
>> - at91_aic_write(AT91_AIC_IECR, 1 << d->irq);
>> + at91_aic_write(AT91_AIC_IECR, 1 << d->hwirq);
>> }
>>
>> unsigned int at91_extern_irq;
>>
>> -#define is_extern_irq(irq) ((1 << (irq)) & at91_extern_irq)
>> +#define is_extern_irq(hwirq) ((1 << (hwirq)) & at91_extern_irq)
>>
>> static int at91_aic_set_type(struct irq_data *d, unsigned type)
>> {
>> @@ -63,13 +71,13 @@ static int at91_aic_set_type(struct irq_data *d, unsigned type)
>> srctype = AT91_AIC_SRCTYPE_RISING;
>> break;
>> case IRQ_TYPE_LEVEL_LOW:
>> - if ((d->irq == AT91_ID_FIQ) || is_extern_irq(d->irq)) /* only supported on external interrupts */
>> + if ((d->hwirq == AT91_ID_FIQ) || is_extern_irq(d->hwirq)) /* only supported on external interrupts */
>> srctype = AT91_AIC_SRCTYPE_LOW;
>> else
>> return -EINVAL;
>> break;
>> case IRQ_TYPE_EDGE_FALLING:
>> - if ((d->irq == AT91_ID_FIQ) || is_extern_irq(d->irq)) /* only supported on external interrupts */
>> + if ((d->hwirq == AT91_ID_FIQ) || is_extern_irq(d->hwirq)) /* only supported on external interrupts */
>> srctype = AT91_AIC_SRCTYPE_FALLING;
>> else
>> return -EINVAL;
>> @@ -78,8 +86,8 @@ static int at91_aic_set_type(struct irq_data *d, unsigned type)
>> return -EINVAL;
>> }
>>
>> - smr = at91_aic_read(AT91_AIC_SMR(d->irq)) & ~AT91_AIC_SRCTYPE;
>> - at91_aic_write(AT91_AIC_SMR(d->irq), smr | srctype);
>> + smr = at91_aic_read(AT91_AIC_SMR(d->hwirq)) & ~AT91_AIC_SRCTYPE;
>> + at91_aic_write(AT91_AIC_SMR(d->hwirq), smr | srctype);
>> return 0;
>> }
>>
>> @@ -90,13 +98,13 @@ static u32 backups;
>>
>> static int at91_aic_set_wake(struct irq_data *d, unsigned value)
>> {
>> - if (unlikely(d->irq >= 32))
>> + if (unlikely(d->hwirq >= NR_AIC_IRQS))
>> return -EINVAL;
>>
>> if (value)
>> - wakeups |= (1 << d->irq);
>> + wakeups |= (1 << d->hwirq);
>> else
>> - wakeups &= ~(1 << d->irq);
>> + wakeups &= ~(1 << d->hwirq);
>>
>> return 0;
>> }
>> @@ -127,24 +135,64 @@ static struct irq_chip at91_aic_chip = {
>> .irq_set_wake = at91_aic_set_wake,
>> };
>>
>> +#if defined(CONFIG_OF)
>> +static int __init __at91_aic_of_init(struct device_node *node,
>> + struct device_node *parent)
>> +{
>> + at91_aic_base = of_iomap(node, 0);
>> + at91_aic_np = node;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id aic_ids[] __initconst = {
>> + { .compatible = "atmel,at91rm9200-aic", .data = __at91_aic_of_init },
>> + { /*sentinel*/ }
>> +};
>> +
>> +static void __init at91_aic_of_init(void)
>> +{
>> + of_irq_init(aic_ids);
>
> This call and match str belongs in your board file (init_irq function)
> and you should be doing all setup in __at91_aic_of_init.
It has already been discussed here:
http://article.gmane.org/gmane.linux.drivers.devicetree/9834
I feel that this self-contained driver is the best solution for the
moment. Once the priorities will be encoded in the device tree (and
moreover once they will be useful for threaded interrupts), we may
re-consider this.
>> +}
>> +#else
>> +static void __init at91_aic_of_init(void) {}
>> +#endif
>> +
>> /*
>> * Initialize the AIC interrupt controller.
>> */
>> void __init at91_aic_init(unsigned int priority[NR_AIC_IRQS])
>
> Perhaps the priority should be a cell in your interrupts property?
Yes, it is a good idea. But that will prevent me from using a standard
irq_domain simple xlate operation... Maybe we can think about this a bit
more and implement it in the future.
>> {
>> unsigned int i;
>> + int irq_base;
>>
>> - at91_aic_base = ioremap(AT91_AIC, 512);
>> + if (of_have_populated_dt())
>> + at91_aic_of_init();
>
> You should get to this point because of_irq_init called you and you
> should already have a node ptr at point.
>
>> + else
>> + at91_aic_base = ioremap(AT91_AIC, 512);
>>
>> if (!at91_aic_base)
>> - panic("Impossible to ioremap AT91_AIC\n");
>> + panic("Unable to ioremap AIC registers\n");
>> +
>> + /* Add irq domain for AIC */
>> + irq_base = irq_alloc_descs(-1, AIC_BASE_IRQ, NR_AIC_IRQS, 0);
>
> Really, you don't want to use irq0, but that would break your irq entry
> code.
Exactly. I have the code for switching to MULTI_IRQ_HANDLER ready. But
that will require a little modification of each board file. This
scattered modification is maybe a bit late for 3.4 inclusion...
Best regards,
--
Nicolas Ferre
--
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