On 09/28/2011 05:41 AM, Jamie Iles wrote:
> This adds a device tree binding for the VIC based on the of_irq_init()
> support.  This adds an irqdomain to the vic and always registers all
> vics in the static vic array rather than for pm only to keep track of
> the irq domain.  struct irq_data::hwirq is used where appropriate rather
> than runtime masking.
> 
> v2:   - use irq_domain_simple_ops
>       - remove stub implementation of vic_of_init for !CONFIG_OF
>       - Make VIC select IRQ_DOMAIN
> 
> Cc: Rob Herring <robherri...@gmail.com>
> Cc: Grant Likely <grant.lik...@secretlab.ca>
> Signed-off-by: Jamie Iles <ja...@jamieiles.com>

Looks good. One minor comment below, but otherwise:

Reviewed-by: Rob Herring <rob.herr...@calxeda.com>

> ---
>  Documentation/devicetree/bindings/arm/vic.txt |   29 +++++++
>  arch/arm/common/Kconfig                       |    1 +
>  arch/arm/common/vic.c                         |  106 
> ++++++++++++++++++-------
>  arch/arm/include/asm/hardware/vic.h           |   10 ++-
>  4 files changed, 117 insertions(+), 29 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/vic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/vic.txt 
> b/Documentation/devicetree/bindings/arm/vic.txt
> new file mode 100644
> index 0000000..266716b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/vic.txt
> @@ -0,0 +1,29 @@
> +* ARM Vectored Interrupt Controller
> +
> +One or more Vectored Interrupt Controllers (VIC's) can be connected in an ARM
> +system for interrupt routing.  For multiple controllers they can either be
> +nested or have the outputs wire-OR'd together.
> +
> +Required properties:
> +
> +- compatible : should be one of
> +     "arm,pl190-vic"
> +     "arm,pl192-vic"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : The number of cells to define the interrupts.  Must be 
> 1 as
> +  the VIC has no configuration options for interrupt sources.  The cell is a 
> u32
> +  and defines the interrupt number.
> +- reg : The register bank for the VIC.
> +
> +Optional properties:
> +
> +- interrupts : Interrupt source for parent controllers if the VIC is nested.
> +
> +Example:
> +
> +     vic0: interrupt-controller@60000 {
> +             compatible = "arm,pl192-vic";
> +             interrupt-controller;
> +             #interrupt-cells = <1>;
> +             reg = <0x60000 0x1000>;
> +     };
> diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
> index 4b71766..43e9d1a 100644
> --- a/arch/arm/common/Kconfig
> +++ b/arch/arm/common/Kconfig
> @@ -2,6 +2,7 @@ config ARM_GIC
>       bool
>  
>  config ARM_VIC
> +     select IRQ_DOMAIN
>       bool
>  
>  config ARM_VIC_NR
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index 7aa4262..3f9c8f2 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -22,6 +22,10 @@
>  #include <linux/init.h>
>  #include <linux/list.h>
>  #include <linux/io.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/device.h>
>  #include <linux/amba/bus.h>
> @@ -29,7 +33,6 @@
>  #include <asm/mach/irq.h>
>  #include <asm/hardware/vic.h>
>  
> -#ifdef CONFIG_PM
>  /**
>   * struct vic_device - VIC PM device
>   * @irq: The IRQ number for the base of the VIC.
> @@ -40,6 +43,7 @@
>   * @int_enable: Save for VIC_INT_ENABLE.
>   * @soft_int: Save for VIC_INT_SOFT.
>   * @protect: Save for VIC_PROTECT.
> + * @domain: The IRQ domain for the VIC.
>   */
>  struct vic_device {
>       void __iomem    *base;
> @@ -50,13 +54,13 @@ struct vic_device {
>       u32             int_enable;
>       u32             soft_int;
>       u32             protect;
> +     struct irq_domain domain;
>  };
>  
>  /* we cannot allocate memory when VICs are initially registered */
>  static struct vic_device vic_devices[CONFIG_ARM_VIC_NR];
>  
>  static int vic_id;
> -#endif /* CONFIG_PM */
>  
>  /**
>   * vic_init2 - common initialisation code
> @@ -156,39 +160,50 @@ static int __init vic_pm_init(void)
>       return 0;
>  }
>  late_initcall(vic_pm_init);
> +#endif /* CONFIG_PM */
>  
>  /**
> - * vic_pm_register - Register a VIC for later power management control
> + * vic_register() - Register a VIC.
>   * @base: The base address of the VIC.
>   * @irq: The base IRQ for the VIC.
>   * @resume_sources: bitmask of interrupts allowed for resume sources.
> + * @node: The device tree node associated with the VIC.
>   *
>   * Register the VIC with the system device tree so that it can be notified
>   * of suspend and resume requests and ensure that the correct actions are
>   * taken to re-instate the settings on resume.
> + *
> + * This also configures the IRQ domain for the VIC.
>   */
> -static void __init vic_pm_register(void __iomem *base, unsigned int irq, u32 
> resume_sources)
> +static void __init vic_register(void __iomem *base, unsigned int irq,
> +                             u32 resume_sources, struct device_node *node)
>  {
>       struct vic_device *v;
>  
> -     if (vic_id >= ARRAY_SIZE(vic_devices))
> +     if (vic_id >= ARRAY_SIZE(vic_devices)) {
>               printk(KERN_ERR "%s: too few VICs, increase 
> CONFIG_ARM_VIC_NR\n", __func__);
> -     else {
> -             v = &vic_devices[vic_id];
> -             v->base = base;
> -             v->resume_sources = resume_sources;
> -             v->irq = irq;
> -             vic_id++;
> +             return;
>       }
> +
> +     v = &vic_devices[vic_id];
> +     v->base = base;
> +     v->resume_sources = resume_sources;
> +     v->irq = irq;
> +     vic_id++;
> +
> +     v->domain.irq_base = irq;
> +     v->domain.nr_irq = 32;
> +#ifdef CONFIG_OF_IRQ
> +     v->domain.of_node = of_node_get(node);
> +     v->domain.ops = &irq_domain_simple_ops;
> +#endif /* CONFIG_OF */
> +     irq_domain_add(&v->domain);
>  }
> -#else
> -static inline void vic_pm_register(void __iomem *base, unsigned int irq, u32 
> arg1) { }
> -#endif /* CONFIG_PM */
>  
>  static void vic_ack_irq(struct irq_data *d)
>  {
>       void __iomem *base = irq_data_get_irq_chip_data(d);
> -     unsigned int irq = d->irq & 31;
> +     unsigned int irq = d->hwirq;
>       writel(1 << irq, base + VIC_INT_ENABLE_CLEAR);
>       /* moreover, clear the soft-triggered, in case it was the reason */
>       writel(1 << irq, base + VIC_INT_SOFT_CLEAR);
> @@ -197,14 +212,14 @@ static void vic_ack_irq(struct irq_data *d)
>  static void vic_mask_irq(struct irq_data *d)
>  {
>       void __iomem *base = irq_data_get_irq_chip_data(d);
> -     unsigned int irq = d->irq & 31;
> +     unsigned int irq = d->hwirq;
>       writel(1 << irq, base + VIC_INT_ENABLE_CLEAR);
>  }
>  
>  static void vic_unmask_irq(struct irq_data *d)
>  {
>       void __iomem *base = irq_data_get_irq_chip_data(d);
> -     unsigned int irq = d->irq & 31;
> +     unsigned int irq = d->hwirq;
>       writel(1 << irq, base + VIC_INT_ENABLE);
>  }
>  
> @@ -226,7 +241,7 @@ static struct vic_device *vic_from_irq(unsigned int irq)
>  static int vic_set_wake(struct irq_data *d, unsigned int on)
>  {
>       struct vic_device *v = vic_from_irq(d->irq);
> -     unsigned int off = d->irq & 31;
> +     unsigned int off = d->hwirq;
>       u32 bit = 1 << off;
>  
>       if (!v)
> @@ -331,15 +346,9 @@ static void __init vic_init_st(void __iomem *base, 
> unsigned int irq_start,
>       vic_set_irq_sources(base, irq_start, vic_sources);
>  }
>  
> -/**
> - * vic_init - initialise a vectored interrupt controller
> - * @base: iomem base address
> - * @irq_start: starting interrupt number, must be muliple of 32
> - * @vic_sources: bitmask of interrupt sources to allow
> - * @resume_sources: bitmask of interrupt sources to allow for resume
> - */
> -void __init vic_init(void __iomem *base, unsigned int irq_start,
> -                  u32 vic_sources, u32 resume_sources)
> +static void __init __vic_init(void __iomem *base, unsigned int irq_start,
> +                           u32 vic_sources, u32 resume_sources,
> +                           struct device_node *node)
>  {
>       unsigned int i;
>       u32 cellid = 0;
> @@ -375,5 +384,46 @@ void __init vic_init(void __iomem *base, unsigned int 
> irq_start,
>  
>       vic_set_irq_sources(base, irq_start, vic_sources);
>  
> -     vic_pm_register(base, irq_start, resume_sources);
> +     vic_register(base, irq_start, resume_sources, node);
> +}
> +
> +/**
> + * vic_init() - initialise a vectored interrupt controller
> + * @base: iomem base address
> + * @irq_start: starting interrupt number, must be muliple of 32
> + * @vic_sources: bitmask of interrupt sources to allow
> + * @resume_sources: bitmask of interrupt sources to allow for resume
> + */
> +void __init vic_init(void __iomem *base, unsigned int irq_start,
> +                  u32 vic_sources, u32 resume_sources)
> +{
> +     __vic_init(base, irq_start, vic_sources, resume_sources, NULL);
> +}
> +
> +#ifdef CONFIG_OF
> +int __init vic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +     void __iomem *regs;
> +     int irq_base;
> +
> +     if (WARN(parent, "non-root VICs are not supported"))
> +             return -EINVAL;
> +
> +     regs = of_iomap(node, 0);
> +     if (WARN_ON(!regs))
> +             return -EIO;
> +
> +     irq_base = irq_alloc_descs(-1, 0, 32, numa_node_id());
> +     if (WARN_ON(irq_base < 0))
> +             goto out_unmap;
> +
> +     __vic_init(regs, irq_base, ~0, ~0, node);
> +
> +     return 0;
> +
> + out_unmap:
> +     iounmap(regs);
> +
> +     return -EIO;
>  }
> +#endif /* CONFIG OF */
> diff --git a/arch/arm/include/asm/hardware/vic.h 
> b/arch/arm/include/asm/hardware/vic.h
> index 5d72550..0135215 100644
> --- a/arch/arm/include/asm/hardware/vic.h
> +++ b/arch/arm/include/asm/hardware/vic.h
> @@ -41,7 +41,15 @@
>  #define VIC_PL192_VECT_ADDR          0xF00
>  
>  #ifndef __ASSEMBLY__
> +#include <linux/compiler.h>
> +#include <linux/types.h>
> +
> +struct device_node;
>  void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, 
> u32 resume_sources);
> -#endif
>  
> +#ifdef CONFIG_OF
> +int vic_of_init(struct device_node *node, struct device_node *parent);
> +#endif /* CONFIG_OF */

You don't need an ifdef around this.

Rob
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Reply via email to