diff mbox

[V2,22/33] xen/arm: Use the device tree to map the address range and IRQ to dom0

Message ID 6e7f063528064751438ffa12fea9c8f5229be78d.1367979526.git.julien.grall@linaro.org
State Changes Requested, archived
Headers show

Commit Message

Julien Grall May 8, 2013, 2:33 a.m. UTC
- gic_route_irq_to_guest takes a dt_irq instead of an IRQ number
- remove hardcoded address/IRQ

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

Changes in v2:
    - Use the new function dt_irq_is_level_trigger
    - Disable DEBUG_DT by default
    - Rename parse_device_tree to map_device_from_device_tree
---
 xen/arch/arm/domain_build.c |  144 +++++++++++++++++++++++++++++++++++++------
 xen/arch/arm/gic.c          |   12 ++--
 xen/include/asm-arm/gic.h   |    3 +-
 3 files changed, 136 insertions(+), 23 deletions(-)

Comments

Ian Campbell May 8, 2013, 3:28 p.m. UTC | #1
On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
> - gic_route_irq_to_guest takes a dt_irq instead of an IRQ number
> - remove hardcoded address/IRQ
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Changes in v2:
>     - Use the new function dt_irq_is_level_trigger
>     - Disable DEBUG_DT by default
>     - Rename parse_device_tree to map_device_from_device_tree

Should be map_devices_...

> +        /**

Javadoc!

> +         * Don't map IRQ that have no physical meaning
> +         * ie: IRQ whose controller is not the GIC
> +         */
> +        if ( rirq.controller != dt_interrupt_controller )
> +        {
> +            DPRINT("irq %u skipped it controller (%s)\n",

you might mean "skipped its controller" ? But I think a clearer message
would be "irq %u not connected to primary controller" or something.

> +                   i, dt_node_full_name(rirq.controller));
> +            continue;
> +        }
> +
> +        res = dt_irq_translate(&rirq, &irq);
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR "Unable to translate irq %u for %s\n",
> +                   i, dt_node_full_name(dev));
> +            return res;
> +        }
> +
> +        DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type);

The # format specifier is required by $STANDARD to not be all that
sensible and/or consistent when when the value is 0, i.e.
	printf("%#x\n", 0) => "0"
	printf("%#x\n", 1) => "0x1"
Worse if you use widths then:
        printf("%#02x\n", 0); => "00"
        printf("%#02x\n", 1); => "0x1"
        printf("%#04x\n", 0); => "0000"
    printf("%#04x\n", 1); => "0x01"
        
For this reason we tend to avoid # and just use "0x%...", assuming
irq==0 is a possibility, and likewise below addr==0 it's probably better
to avoid #.

> +        /* Don't check return because the IRQ can be use by multiple device */

"used by"

> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index f7b9889..ddad0c8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -692,13 +692,14 @@ void gic_inject(void)
>          gic_inject_irq_start();
>  }
>  
> -int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
> +int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>                             const char * devname)
>  {
>      struct irqaction *action;
> -    struct irq_desc *desc = irq_to_desc(irq);
> +    struct irq_desc *desc = irq_to_desc(irq->irq);
>      unsigned long flags;
>      int retval;
> +    bool_t level;
>  
>      action = xmalloc(struct irqaction);
>      if (!action)
> @@ -706,6 +707,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>  
>      action->dev_id = d;
>      action->name = devname;
> +    action->free_on_release = 1;
>  
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic.lock);
> @@ -713,9 +715,11 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>      desc->handler = &gic_guest_irq_type;
>      desc->status |= IRQ_GUEST;
>  
> -    gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0);
> +    level = dt_irq_is_level_trigger(irq);
> +
> +    gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0);

By the end of this series are all callers going through the above dance?
git_set_irq_properties could take a dt_irq?

Ian.
Julien Grall May 8, 2013, 4:59 p.m. UTC | #2
On 05/08/2013 04:28 PM, Ian Campbell wrote:

> On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
>> - gic_route_irq_to_guest takes a dt_irq instead of an IRQ number
>> - remove hardcoded address/IRQ
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Changes in v2:
>>     - Use the new function dt_irq_is_level_trigger
>>     - Disable DEBUG_DT by default
>>     - Rename parse_device_tree to map_device_from_device_tree
> 
> Should be map_devices_...

Will be fix on the next patch series.

>> +        /**
> 
> Javadoc!
> 
>> +         * Don't map IRQ that have no physical meaning
>> +         * ie: IRQ whose controller is not the GIC
>> +         */
>> +        if ( rirq.controller != dt_interrupt_controller )
>> +        {
>> +            DPRINT("irq %u skipped it controller (%s)\n",
> 
> you might mean "skipped its controller" ? But I think a clearer message
> would be "irq %u not connected to primary controller" or something.
> 
>> +                   i, dt_node_full_name(rirq.controller));
>> +            continue;
>> +        }
>> +
>> +        res = dt_irq_translate(&rirq, &irq);
>> +        if ( res )
>> +        {
>> +            printk(XENLOG_ERR "Unable to translate irq %u for %s\n",
>> +                   i, dt_node_full_name(dev));
>> +            return res;
>> +        }
>> +
>> +        DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type);
> 
> The # format specifier is required by $STANDARD to not be all that
> sensible and/or consistent when when the value is 0, i.e.
> 	printf("%#x\n", 0) => "0"
> 	printf("%#x\n", 1) => "0x1"
> Worse if you use widths then:
>         printf("%#02x\n", 0); => "00"
>         printf("%#02x\n", 1); => "0x1"
>         printf("%#04x\n", 0); => "0000"
>     printf("%#04x\n", 1); => "0x01"
>         
> For this reason we tend to avoid # and just use "0x%...", assuming
> irq==0 is a possibility, and likewise below addr==0 it's probably better
> to avoid #.

Thanks for this hint. I will replace all my %#x by 0x%x.

>> +        /* Don't check return because the IRQ can be use by multiple device */
> 
> "used by"
> 
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index f7b9889..ddad0c8 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -692,13 +692,14 @@ void gic_inject(void)
>>          gic_inject_irq_start();
>>  }
>>  
>> -int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>> +int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>                             const char * devname)
>>  {
>>      struct irqaction *action;
>> -    struct irq_desc *desc = irq_to_desc(irq);
>> +    struct irq_desc *desc = irq_to_desc(irq->irq);
>>      unsigned long flags;
>>      int retval;
>> +    bool_t level;
>>  
>>      action = xmalloc(struct irqaction);
>>      if (!action)
>> @@ -706,6 +707,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>>  
>>      action->dev_id = d;
>>      action->name = devname;
>> +    action->free_on_release = 1;
>>  
>>      spin_lock_irqsave(&desc->lock, flags);
>>      spin_lock(&gic.lock);
>> @@ -713,9 +715,11 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>>      desc->handler = &gic_guest_irq_type;
>>      desc->status |= IRQ_GUEST;
>>  
>> -    gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0);
>> +    level = dt_irq_is_level_trigger(irq);
>> +
>> +    gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0);
> 
> By the end of this series are all callers going through the above dance?
> git_set_irq_properties could take a dt_irq?


No. I can add patch to use a dt_irq.
diff mbox

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6581492..0b762a9 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -14,6 +14,7 @@ 
 #include <asm/setup.h>
 
 #include <asm/gic.h>
+#include <xen/irq.h>
 #include "kernel.h"
 
 static unsigned int __initdata opt_dom0_max_vcpus;
@@ -30,6 +31,14 @@  static void __init parse_dom0_mem(const char *s)
 }
 custom_param("dom0_mem", parse_dom0_mem);
 
+//#define DEBUG_DT
+
+#ifdef DEBUG_DT
+# define DPRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
+#else
+# define DPRINT(fmt, args...) do {} while ( 0 )
+#endif
+
 /*
  * Amount of extra space required to dom0's device tree.  No new nodes
  * are added (yet) but one terminating reserve map entry (16 bytes) is
@@ -303,6 +312,122 @@  static int write_nodes(struct domain *d, struct kernel_info *kinfo,
     return 0;
 }
 
+/* Map the device in the domain */
+static int map_device(struct domain *d, const struct dt_device_node *dev)
+{
+    unsigned int nirq;
+    unsigned int naddr;
+    unsigned int i;
+    int res;
+    struct dt_irq irq;
+    struct dt_raw_irq rirq;
+    u64 addr, size;
+
+    nirq = dt_number_of_irq(dev);
+    naddr = dt_number_of_address(dev);
+
+    DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr);
+
+    /* Map IRQs */
+    for ( i = 0; i < nirq; i++ )
+    {
+        res = dt_device_get_raw_irq(dev, i, &rirq);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
+                   i, dt_node_full_name(dev));
+            return res;
+        }
+
+        /**
+         * Don't map IRQ that have no physical meaning
+         * ie: IRQ whose controller is not the GIC
+         */
+        if ( rirq.controller != dt_interrupt_controller )
+        {
+            DPRINT("irq %u skipped it controller (%s)\n",
+                   i, dt_node_full_name(rirq.controller));
+            continue;
+        }
+
+        res = dt_irq_translate(&rirq, &irq);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to translate irq %u for %s\n",
+                   i, dt_node_full_name(dev));
+            return res;
+        }
+
+        DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type);
+        /* Don't check return because the IRQ can be use by multiple device */
+        gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
+    }
+
+    /* Map the address ranges */
+    for ( i = 0; i < naddr; i++ )
+    {
+        res = dt_device_get_address(dev, i, &addr, &size);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+                   i, dt_node_full_name(dev));
+            return res;
+        }
+
+        DPRINT("addr %u = %#"PRIx64" - %#"PRIx64"\n", i, addr, addr + size - 1);
+
+        res = map_mmio_regions(d, addr & PAGE_MASK,
+                               PAGE_ALIGN(addr + size) - 1,
+                               addr & PAGE_MASK);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to map %#"PRIx64" - %#"PRIx64" in dom0\n",
+                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+            return res;
+        }
+    }
+
+    return 0;
+}
+
+static int handle_node(struct domain *d, const struct dt_device_node *np)
+{
+    const struct dt_device_node *child;
+    int res;
+
+    DPRINT("handle %s\n", dt_node_full_name(np));
+
+    /* Skip theses nodes and the sub-nodes */
+    if ( dt_device_is_compatible(np, "xen,xen") ||
+         dt_device_type_is_equal(np, "memory") ||
+         !strcmp("/chosen", dt_node_full_name(np)) )
+        return 0;
+
+    if ( dt_device_used_by(np) != DT_USED_BY_XEN )
+    {
+        res = map_device(d, np);
+
+        if ( res )
+            return res;
+    }
+
+    for ( child = np->child; child != NULL; child = child->sibling )
+    {
+        res = handle_node(d, child);
+        if ( res )
+            return res;
+    }
+
+    return 0;
+}
+
+static int map_device_from_device_tree(struct domain *d)
+{
+    ASSERT(dt_host && (dt_host->sibling == NULL));
+
+    return handle_node(d, dt_host);
+}
+
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 {
     void *fdt;
@@ -385,24 +510,7 @@  int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
-    printk("Map CS2 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x18000000ULL, 0x1BFFFFFFULL);
-    map_mmio_regions(d, 0x18000000, 0x1BFFFFFF, 0x18000000);
-    printk("Map CS3 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x1C000000ULL, 0x1FFFFFFFULL);
-    map_mmio_regions(d, 0x1C000000, 0x1FFFFFFF, 0x1C000000);
-
-    printk("Routing peripheral interrupts to guest\n");
-    /* TODO Get from device tree */
-    gic_route_irq_to_guest(d, 34, "timer0");
-    /*gic_route_irq_to_guest(d, 37, "uart0"); -- XXX used by Xen*/
-    gic_route_irq_to_guest(d, 38, "uart1");
-    gic_route_irq_to_guest(d, 39, "uart2");
-    gic_route_irq_to_guest(d, 40, "uart3");
-    gic_route_irq_to_guest(d, 41, "mmc0-1");
-    gic_route_irq_to_guest(d, 42, "mmc0-2");
-    gic_route_irq_to_guest(d, 44, "keyboard");
-    gic_route_irq_to_guest(d, 45, "mouse");
-    gic_route_irq_to_guest(d, 46, "lcd");
-    gic_route_irq_to_guest(d, 47, "eth");
+    map_device_from_device_tree(d);
 
     /* The following loads use the domain's p2m */
     p2m_load_VTTBR(d);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index f7b9889..ddad0c8 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -692,13 +692,14 @@  void gic_inject(void)
         gic_inject_irq_start();
 }
 
-int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
+int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                            const char * devname)
 {
     struct irqaction *action;
-    struct irq_desc *desc = irq_to_desc(irq);
+    struct irq_desc *desc = irq_to_desc(irq->irq);
     unsigned long flags;
     int retval;
+    bool_t level;
 
     action = xmalloc(struct irqaction);
     if (!action)
@@ -706,6 +707,7 @@  int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
 
     action->dev_id = d;
     action->name = devname;
+    action->free_on_release = 1;
 
     spin_lock_irqsave(&desc->lock, flags);
     spin_lock(&gic.lock);
@@ -713,9 +715,11 @@  int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
     desc->handler = &gic_guest_irq_type;
     desc->status |= IRQ_GUEST;
 
-    gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0);
+    level = dt_irq_is_level_trigger(irq);
+
+    gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0);
 
-    retval = __setup_irq(desc, irq, action);
+    retval = __setup_irq(desc, irq->irq, action);
     if (retval) {
         xfree(action);
         goto out;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index e7608dc..513c1fc 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -157,7 +157,8 @@  extern int gic_events_need_delivery(void);
 extern void __cpuinit init_maintenance_interrupt(void);
 extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int state, unsigned int priority);
-extern int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
+extern int gic_route_irq_to_guest(struct domain *d,
+                                  const struct dt_irq *irq,
                                   const char * devname);
 
 /* Accept an interrupt from the GIC and dispatch its handler */