diff mbox

[Xen-devel,v6,1/6] xen/arm: IRQ: Store IRQ type in arch_irq_desc

Message ID 1399917438-21475-2-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall May 12, 2014, 5:57 p.m. UTC 
For now, ARM uses different IRQ functions to setup an interrupt handler. This
is a bit annoying for common driver because we have to add idefery when
an IRQ is setup (see ns16550_init_postirq for an example).

To avoid to completely fork the IRQ management code, we can introduce a field
to store the IRQ type (e.g level/edge ...).

This patch also adds platform_get_irq which will retrieve the IRQ from the
device tree and setup correctly the IRQ type.

In order to use this solution, we have to move init_IRQ earlier for the boot
CPU. It's fine because the code only depends on percpu.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v6:
        - Introduce DT_IRQ_TYPE_INVALID to initialize IRQ type
        - Introduce local_irqs_type to avoid use boot CPU PPIs type
        config. Will be usefull for hotplug
        - Export irq_set_type and restrict for SPIs
        - Split IRQ set type for PPIs in another function

    Changes in v5:
        - Update comment in init_local_irq_data
        - Don't set desc.arch.type on boot cpu in init_local_irq_data
        - Use new macro boot_cpu instead of percpu(myvar, 0)

    Changes in v4:
        - Add an ASSERT to check if irq_set_type hasn't failed for PPI
        on other CPU than 0
        - platform_get_irq return -1 in case of error.

    Changes in v3:
        - irqflags is unsigned long not unsigned int
        - fix comment
        - don't need to set IRQ type when NONE is used
        (already set at startup).

    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c            |   25 +++++----
 xen/arch/arm/irq.c            |  125 ++++++++++++++++++++++++++++++++++++++---
 xen/arch/arm/setup.c          |    3 +-
 xen/include/asm-arm/gic.h     |    5 +-
 xen/include/asm-arm/irq.h     |    6 ++
 xen/include/xen/device_tree.h |    3 +
 6 files changed, 146 insertions(+), 21 deletions(-)

Comments

Ian Campbell May 15, 2014, 3:45 p.m. UTC | #1 
On Mon, 2014-05-12 at 18:57 +0100, Julien Grall wrote:
> @@ -383,6 +403,97 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>      BUG();
>  }
>  
> +static bool_t irq_check_type(unsigned int curr, unsigned new)

irq_validate_new_type? irq_type_change_allowed?

> +{
> +    return (curr == DT_IRQ_TYPE_INVALID || curr == new );
> +}
> +
> +int irq_set_type(unsigned int spi, unsigned int type)
> +{
> +    unsigned long flags;
> +    struct irq_desc *desc = irq_to_desc(spi);
> +    int ret = -EBUSY;
> +
> +    /* This function should not be used for other than SPIs */

Perhaps name the function irq_set_spi_type or something then?

> +    for_each_cpu( cpu, &cpu_online_map )
> +    {
> +        desc = &per_cpu(local_irq_desc, cpu)[irq];
> +        spin_lock_irqsave(&desc->lock, flags);
> +        desc->arch.type = type;

Don't you need to write ICFGR on each CPU?

> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 5542958..e677da9 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -131,6 +131,7 @@ struct dt_phandle_args {
>   * DT_IRQ_TYPE_LEVEL_LOW       - low level triggered
>   * DT_IRQ_TYPE_LEVEL_MASK      - Mask to filter out the level bits
>   * DT_IRQ_TYPE_SENSE_MASK      - Mask for all the above bits
> + * DT_IRQ_TYPE_INVALID         - Use to initialize the type
>   */
>  #define DT_IRQ_TYPE_NONE           0x00000000
>  #define DT_IRQ_TYPE_EDGE_RISING    0x00000001
> @@ -143,6 +144,8 @@ struct dt_phandle_args {
>      (DT_IRQ_TYPE_LEVEL_LOW | DT_IRQ_TYPE_LEVEL_HIGH)
>  #define DT_IRQ_TYPE_SENSE_MASK     0x0000000f
>  
> +#define DT_IRQ_TYPE_INVALID        0x00000010

0xffffffff perhaps?
Julien Grall May 15, 2014, 5:03 p.m. UTC | #2 
On 05/15/2014 04:45 PM, Ian Campbell wrote:
>> +int irq_set_type(unsigned int spi, unsigned int type)
>> +{
>> +    unsigned long flags;
>> +    struct irq_desc *desc = irq_to_desc(spi);
>> +    int ret = -EBUSY;
>> +
>> +    /* This function should not be used for other than SPIs */
> 
> Perhaps name the function irq_set_spi_type or something then?
> 
>> +    for_each_cpu( cpu, &cpu_online_map )
>> +    {
>> +        desc = &per_cpu(local_irq_desc, cpu)[irq];
>> +        spin_lock_irqsave(&desc->lock, flags);
>> +        desc->arch.type = type;
> 
> Don't you need to write ICFGR on each CPU?

This function will bail out if local_irqs_type[irq] == DT_IRQ_TYPE_INVALID.

This value means, the IRQ has not been configured and set up (see ASSERT
in configure which prevent it).

The ICFGR will be configure when Xen set up an handler to this IRQ (see
setup_irq).

> 
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 5542958..e677da9 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -131,6 +131,7 @@ struct dt_phandle_args {
>>   * DT_IRQ_TYPE_LEVEL_LOW       - low level triggered
>>   * DT_IRQ_TYPE_LEVEL_MASK      - Mask to filter out the level bits
>>   * DT_IRQ_TYPE_SENSE_MASK      - Mask for all the above bits
>> + * DT_IRQ_TYPE_INVALID         - Use to initialize the type
>>   */
>>  #define DT_IRQ_TYPE_NONE           0x00000000
>>  #define DT_IRQ_TYPE_EDGE_RISING    0x00000001
>> @@ -143,6 +144,8 @@ struct dt_phandle_args {
>>      (DT_IRQ_TYPE_LEVEL_LOW | DT_IRQ_TYPE_LEVEL_HIGH)
>>  #define DT_IRQ_TYPE_SENSE_MASK     0x0000000f
>>  
>> +#define DT_IRQ_TYPE_INVALID        0x00000010
> 
> 0xffffffff perhaps?

The value 0x010 will always return false for edge and level. This is not
the case for 0xffffffff because edge/level use different bit.

Regards,
Ian Campbell May 16, 2014, 10:16 a.m. UTC | #3 
On Thu, 2014-05-15 at 18:03 +0100, Julien Grall wrote:
> On 05/15/2014 04:45 PM, Ian Campbell wrote:
> >> +int irq_set_type(unsigned int spi, unsigned int type)
> >> +{
> >> +    unsigned long flags;
> >> +    struct irq_desc *desc = irq_to_desc(spi);
> >> +    int ret = -EBUSY;
> >> +
> >> +    /* This function should not be used for other than SPIs */
> > 
> > Perhaps name the function irq_set_spi_type or something then?
> > 
> >> +    for_each_cpu( cpu, &cpu_online_map )
> >> +    {
> >> +        desc = &per_cpu(local_irq_desc, cpu)[irq];
> >> +        spin_lock_irqsave(&desc->lock, flags);
> >> +        desc->arch.type = type;
> > 
> > Don't you need to write ICFGR on each CPU?
> 
> This function will bail out if local_irqs_type[irq] == DT_IRQ_TYPE_INVALID.

Did you mean != ?

> This value means, the IRQ has not been configured and set up (see ASSERT
> in configure which prevent it).
> 
> The ICFGR will be configure when Xen set up an handler to this IRQ (see
> setup_irq).

OK.

> >> +#define DT_IRQ_TYPE_INVALID        0x00000010
> > 
> > 0xffffffff perhaps?
> 
> The value 0x010 will always return false for edge and level. This is not
> the case for 0xffffffff because edge/level use different bit.

OK.
Julien Grall May 16, 2014, 10:43 a.m. UTC | #4 
On 05/16/2014 11:16 AM, Ian Campbell wrote:
> On Thu, 2014-05-15 at 18:03 +0100, Julien Grall wrote:
>> On 05/15/2014 04:45 PM, Ian Campbell wrote:
>>>> +int irq_set_type(unsigned int spi, unsigned int type)
>>>> +{
>>>> +    unsigned long flags;
>>>> +    struct irq_desc *desc = irq_to_desc(spi);
>>>> +    int ret = -EBUSY;
>>>> +
>>>> +    /* This function should not be used for other than SPIs */
>>>
>>> Perhaps name the function irq_set_spi_type or something then?
>>>
>>>> +    for_each_cpu( cpu, &cpu_online_map )
>>>> +    {
>>>> +        desc = &per_cpu(local_irq_desc, cpu)[irq];
>>>> +        spin_lock_irqsave(&desc->lock, flags);
>>>> +        desc->arch.type = type;
>>>
>>> Don't you need to write ICFGR on each CPU?
>>
>> This function will bail out if local_irqs_type[irq] == DT_IRQ_TYPE_INVALID.
> 
> Did you mean != ?

Yes. We only want to configure IRQs that as not been configured.

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 577d85b..13eeb33 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -216,14 +216,21 @@  static hw_irq_controller gic_guest_irq_type = {
 /*
  * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
+ * - desc.lock must be held
+ * - arch.type must be valid (i.e != DT_IRQ_TYPE_INVALID)
  */
-static void gic_set_irq_properties(unsigned int irq, bool_t level,
+static void gic_set_irq_properties(struct irq_desc *desc,
                                    const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
     volatile unsigned char *bytereg;
     uint32_t cfg, edgebit;
     unsigned int mask;
+    unsigned int irq = desc->irq;
+    unsigned int type = desc->arch.type;
+
+    ASSERT(type != DT_IRQ_TYPE_INVALID);
+    ASSERT(spin_is_locked(&desc->lock));
 
     spin_lock(&gic.lock);
 
@@ -232,9 +239,9 @@  static void gic_set_irq_properties(unsigned int irq, bool_t level,
     /* Set edge / level */
     cfg = GICD[GICD_ICFGR + irq / 16];
     edgebit = 2u << (2 * (irq % 16));
-    if ( level )
+    if ( type & DT_IRQ_TYPE_LEVEL_MASK )
         cfg &= ~edgebit;
-    else
+    else if ( type & DT_IRQ_TYPE_EDGE_BOTH )
         cfg |= edgebit;
     GICD[GICD_ICFGR + irq / 16] = cfg;
 
@@ -252,8 +259,8 @@  static void gic_set_irq_properties(unsigned int irq, bool_t level,
 /* Program the GIC to route an interrupt to the host (i.e. Xen)
  * - needs to be called with desc.lock held
  */
-void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
-                          const cpumask_t *cpu_mask, unsigned int priority)
+void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
+                          unsigned int priority)
 {
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(desc->irq < gic.lines);/* Can't route interrupts that don't exist */
@@ -262,15 +269,14 @@  void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
 
     desc->handler = &gic_host_irq_type;
 
-    gic_set_irq_properties(desc->irq, level, cpu_mask, priority);
+    gic_set_irq_properties(desc, cpu_mask, priority);
 }
 
 /* Program the GIC to route an interrupt to a guest
  *   - desc.lock must be held
  */
 void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
-                            bool_t level, const cpumask_t *cpu_mask,
-                            unsigned int priority)
+                            const cpumask_t *cpu_mask, unsigned int priority)
 {
     struct pending_irq *p;
     ASSERT(spin_is_locked(&desc->lock));
@@ -278,8 +284,7 @@  void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
     desc->handler = &gic_guest_irq_type;
     desc->status |= IRQ_GUEST;
 
-    gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()),
-                           GIC_PRI_IRQ);
+    gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
 
     /* TODO: do not assume delivery to vcpu0 */
     p = irq_to_pending(d->vcpu[0], desc->irq);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 44696e7..0b67089 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -27,6 +27,9 @@ 
 
 #include <asm/gic.h>
 
+static unsigned int local_irqs_type[NR_LOCAL_IRQS];
+static DEFINE_SPINLOCK(local_irqs_type_lock);
+
 static void ack_none(struct irq_desc *irq)
 {
     printk("unexpected IRQ trap at irq %02x\n", irq->irq);
@@ -55,6 +58,7 @@  irq_desc_t *__irq_to_desc(int irq)
 
 int __init arch_init_one_irq_desc(struct irq_desc *desc)
 {
+    desc->arch.type = DT_IRQ_TYPE_INVALID;
     return 0;
 }
 
@@ -77,18 +81,37 @@  static int __cpuinit init_local_irq_data(void)
 {
     int irq;
 
+    spin_lock(&local_irqs_type_lock);
+
     for (irq = 0; irq < NR_LOCAL_IRQS; irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         init_one_irq_desc(desc);
         desc->irq = irq;
         desc->action  = NULL;
+
+        /* PPIs are included in local_irqs, we copy the IRQ type from
+         * local_irqs_type when bringing up local IRQ for this CPU in
+         * order to pick up any configuration done before this CPU came
+         * up. For interrupts configured after this point this is done in
+         * irq_set_type.
+         */
+        desc->arch.type = local_irqs_type[irq];
     }
 
+    spin_unlock(&local_irqs_type_lock);
+
     return 0;
 }
 
 void __init init_IRQ(void)
 {
+    int irq;
+
+    spin_lock(&local_irqs_type_lock);
+    for ( irq = 0; irq < NR_LOCAL_IRQS; irq++ )
+        local_irqs_type[irq] = DT_IRQ_TYPE_INVALID;
+    spin_unlock(&local_irqs_type_lock);
+
     BUG_ON(init_local_irq_data() < 0);
     BUG_ON(init_irq_data() < 0);
 }
@@ -275,9 +298,6 @@  int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     /* First time the IRQ is setup */
     if ( disabled )
     {
-        bool_t level;
-
-        level = dt_irq_is_level_triggered(irq);
         /* It's fine to use smp_processor_id() because:
          * For PPI: irq_desc is banked
          * For SPI: we don't care for now which CPU will receive the
@@ -285,7 +305,8 @@  int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
          * TODO: Handle case where SPI is setup on different CPU than
          * the targeted CPU and the priority.
          */
-        gic_route_irq_to_xen(desc, level, cpumask_of(smp_processor_id()),
+        desc->arch.type = irq->type;
+        gic_route_irq_to_xen(desc, cpumask_of(smp_processor_id()),
                              GIC_PRI_IRQ);
         desc->handler->startup(desc);
     }
@@ -303,7 +324,6 @@  int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     struct irq_desc *desc = irq_to_desc(irq->irq);
     unsigned long flags;
     int retval = 0;
-    bool_t level;
 
     action = xmalloc(struct irqaction);
     if (!action)
@@ -341,8 +361,8 @@  int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     if ( retval )
         goto out;
 
-    level = dt_irq_is_level_triggered(irq);
-    gic_route_irq_to_guest(d, desc, level, cpumask_of(smp_processor_id()),
+    desc->arch.type = irq->type;
+    gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()),
                            GIC_PRI_IRQ);
     spin_unlock_irqrestore(&desc->lock, flags);
     return 0;
@@ -383,6 +403,97 @@  void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
     BUG();
 }
 
+static bool_t irq_check_type(unsigned int curr, unsigned new)
+{
+    return (curr == DT_IRQ_TYPE_INVALID || curr == new );
+}
+
+int irq_set_type(unsigned int spi, unsigned int type)
+{
+    unsigned long flags;
+    struct irq_desc *desc = irq_to_desc(spi);
+    int ret = -EBUSY;
+
+    /* This function should not be used for other than SPIs */
+    if ( spi < NR_LOCAL_IRQS )
+        return -EINVAL;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    if ( !irq_check_type(desc->arch.type, type) )
+        goto err;
+
+    desc->arch.type = type;
+
+    ret = 0;
+
+err:
+    spin_unlock_irqrestore(&desc->lock, flags);
+    return ret;
+}
+
+static int irq_local_set_type(unsigned int irq, unsigned int type)
+{
+    unsigned int cpu;
+    unsigned int old_type;
+    unsigned long flags;
+    int ret = -EBUSY;
+    struct irq_desc *desc;
+
+    ASSERT(irq < NR_LOCAL_IRQS);
+
+    spin_lock(&local_irqs_type_lock);
+
+    old_type = local_irqs_type[irq];
+
+    if ( !irq_check_type(old_type, type) )
+        goto unlock;
+
+    ret = 0;
+    /* We don't need to reconfigure if the type is correctly set */
+    if ( old_type == type )
+        goto unlock;
+
+    local_irqs_type[irq] = type;
+
+    for_each_cpu( cpu, &cpu_online_map )
+    {
+        desc = &per_cpu(local_irq_desc, cpu)[irq];
+        spin_lock_irqsave(&desc->lock, flags);
+        desc->arch.type = type;
+        spin_unlock_irqrestore(&desc->lock, flags);
+    }
+
+unlock:
+    spin_unlock(&local_irqs_type_lock);
+    return ret;
+}
+
+int platform_get_irq(const struct dt_device_node *device, int index)
+{
+    struct dt_irq dt_irq;
+    unsigned int type, irq;
+    int res;
+
+    res = dt_device_get_irq(device, index, &dt_irq);
+    if ( res )
+        return -1;
+
+    irq = dt_irq.irq;
+    type = dt_irq.type;
+
+    /* Setup the IRQ type */
+    if ( irq < NR_LOCAL_IRQS )
+        res = irq_local_set_type(irq, type);
+    else
+        res = irq_set_type(irq, type);
+
+    if ( res )
+            return -1;
+
+    return irq;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index dec5950..40ba26e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -688,6 +688,8 @@  void __init start_xen(unsigned long boot_phys_offset,
     dt_unflatten_host_device_tree();
     dt_irq_xlate = gic_irq_xlate;
 
+    init_IRQ();
+
     dt_uart_init();
     console_init_preirq();
 
@@ -717,7 +719,6 @@  void __init start_xen(unsigned long boot_phys_offset,
 
     tasklet_subsys_init();
 
-    init_IRQ();
 
     xsm_dt_init();
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index b750b17..80f8dd2 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -168,11 +168,10 @@  extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
 /* Program the GIC to route an interrupt */
-extern void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
-                                 const cpumask_t *cpu_mask,
+extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
                                  unsigned int priority);
 extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc,
-                                   bool_t level, const cpumask_t *cpu_mask,
+                                   const cpumask_t *cpu_mask,
                                    unsigned int priority);
 
 extern void gic_inject(void);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index b52c26f..e791ed1 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -16,6 +16,7 @@  struct arch_pirq
 
 struct arch_irq_desc {
     int eoi_cpu;
+    unsigned int type;
 };
 
 #define NR_LOCAL_IRQS	32
@@ -47,6 +48,11 @@  int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
 int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                           const char *devname);
 
+/* Set IRQ type for an SPI */
+int irq_set_type(unsigned int spi, unsigned int type);
+
+int platform_get_irq(const struct dt_device_node *device, int index);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5542958..e677da9 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -131,6 +131,7 @@  struct dt_phandle_args {
  * DT_IRQ_TYPE_LEVEL_LOW       - low level triggered
  * DT_IRQ_TYPE_LEVEL_MASK      - Mask to filter out the level bits
  * DT_IRQ_TYPE_SENSE_MASK      - Mask for all the above bits
+ * DT_IRQ_TYPE_INVALID         - Use to initialize the type
  */
 #define DT_IRQ_TYPE_NONE           0x00000000
 #define DT_IRQ_TYPE_EDGE_RISING    0x00000001
@@ -143,6 +144,8 @@  struct dt_phandle_args {
     (DT_IRQ_TYPE_LEVEL_LOW | DT_IRQ_TYPE_LEVEL_HIGH)
 #define DT_IRQ_TYPE_SENSE_MASK     0x0000000f
 
+#define DT_IRQ_TYPE_INVALID        0x00000010
+
 /**
  * dt_irq - describe an IRQ in the device tree
  * @irq: IRQ number