[PATCH v2 0/3] xen: arm: Parse PCI DT nodes' ranges and interrupt-map

[PATCH v2 0/3] xen: arm: Parse PCI DT nodes' ranges and interrupt-map

[Ian Campbell]

This series adds parsing of the DT ranges and interrupt-map properties
for PCI devices, these contain the MMIOs and IRQs used by children on
the bus. This replaces the specific mapping stuff on xgene.

This is pretty much a rewrite of v1, which was wrong in several aspects
relating to its parsing of the various properties.

There are several TODOs in the final patch, I'm mainly sending this now
because in [0] I suggested to Vijay that this might make the need for
the GSER specific mapping on ThunderX. Vijay, it would be great if you
could confirm or deny this in practice.

Suravee, I think you were interested in this for Seattle too so I'm
interested how you get on there.

I've tested on Mustang and on a FastModel with virtio-pci based rootfs.

Ian.

[0] http://article.gmane.org/gmane.comp.emulators.xen.devel/234959

[PATCH v2 1/3] xen: dt: add dt_for_each_irq_map helper

[Ian Campbell]

This function iterates over a nodes interrupt-map property and calls a
callback for each interrupt. For now it only supplies the raw IRQ
since my use case has no need of e.g. child unit address. These can be
added as needed by any future users.

This follows much the same logic as dt_irq_map_raw when parsing the
interrupt-map, but doesn't walk up the tree doing the actual
translation and it iterates over all entries instead of just looking
for the first match.

I looked into refactoring dt_irq_map_raw but I couldn't find a way
which I was confident in, plus I was reluctant to diverge from the
Linux roots of this function any further.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/common/device_tree.c      |  140 +++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |   12 ++++
 2 files changed, 152 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 89491b2..ecc67da 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -816,6 +816,146 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)
     return (psize / onesize);
 }
 
+int dt_for_each_irq_map(const struct dt_device_node *dev,
+                        int (*cb)(const struct dt_device_node *,
+                                  const struct dt_raw_irq *,
+                                  void *),
+                        void *data)
+{
+    const struct dt_device_node *ipar, *tnode, *old = NULL;
+    const __be32 *tmp, *imap;
+    u32 intsize = 1, addrsize, pintsize = 0, paddrsize = 0;
+    u32 imaplen;
+    int i, ret;
+
+    struct dt_raw_irq dt_raw_irq;
+
+    dt_dprintk("%s: par=%s cb=%p data=%p\n", __func__,
+               dev->full_name, cb, data);
+
+    ipar = dev;
+
+    /* First get the #interrupt-cells property of the current cursor
+     * that tells us how to interpret the passed-in intspec. If there
+     * is none, we are nice and just walk up the tree
+     */
+    do {
+        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
+        if ( tmp != NULL )
+        {
+            intsize = be32_to_cpu(*tmp);
+            break;
+        }
+        tnode = ipar;
+        ipar = dt_irq_find_parent(ipar);
+    } while ( ipar );
+    if ( ipar == NULL )
+    {
+        dt_dprintk(" -> no parent found !\n");
+        goto fail;
+    }
+
+    dt_dprintk("%s: ipar=%s, size=%d\n", __func__, ipar->full_name, intsize);
+
+    if ( intsize > DT_MAX_IRQ_SPEC )
+    {
+        dt_dprintk(" -> too many irq specifier cells\n");
+        goto fail;
+    }
+
+    /* Look for this #address-cells. We have to implement the old linux
+     * trick of looking for the parent here as some device-trees rely on it
+     */
+    old = ipar;
+    do {
+        tmp = dt_get_property(old, "#address-cells", NULL);
+        tnode = dt_get_parent(old);
+        old = tnode;
+    } while ( old && tmp == NULL );
+
+    old = NULL;
+    addrsize = (tmp == NULL) ? 2 : be32_to_cpu(*tmp);
+
+    dt_dprintk(" -> addrsize=%d\n", addrsize);
+
+    /* Now look for an interrupt-map */
+    imap = dt_get_property(dev, "interrupt-map", &imaplen);
+    /* No interrupt map, check for an interrupt parent */
+    if ( imap == NULL )
+    {
+        dt_dprintk(" -> no map, ignoring\n");
+        goto fail;
+    }
+    imaplen /= sizeof(u32);
+
+    /* Parse interrupt-map */
+    while ( imaplen > (addrsize + intsize + 1) )
+    {
+        /* skip child unit address and child interrupt specifier */
+        imap += addrsize + intsize;
+        imaplen -= addrsize + intsize;
+
+        /* Get the interrupt parent */
+        ipar = dt_find_node_by_phandle(be32_to_cpup(imap));
+        imap++;
+        --imaplen;
+
+        /* Check if not found */
+        if ( ipar == NULL )
+        {
+            dt_dprintk(" -> imap parent not found !\n");
+            goto fail;
+        }
+
+        dt_dprintk(" -> ipar %s\n", dt_node_name(ipar));
+
+        /* Get #interrupt-cells and #address-cells of new
+         * parent
+         */
+        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
+        if ( tmp == NULL )
+        {
+            dt_dprintk(" -> parent lacks #interrupt-cells!\n");
+            goto fail;
+        }
+        pintsize = be32_to_cpu(*tmp);
+        tmp = dt_get_property(ipar, "#address-cells", NULL);
+        paddrsize = (tmp == NULL) ? 0 : be32_to_cpu(*tmp);
+
+        dt_dprintk(" -> pintsize=%d, paddrsize=%d\n",
+                   pintsize, paddrsize);
+
+        /* Check for malformed properties */
+        if ( imaplen < (paddrsize + pintsize) )
+            goto fail;
+
+        imap += paddrsize;
+        imaplen -= paddrsize;
+
+        dt_raw_irq.controller = ipar;
+        dt_raw_irq.size = pintsize;
+        for ( i = 0; i < pintsize; i++ )
+            dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
+
+        ret = cb(dev, &dt_raw_irq, data);
+        if ( ret < 0 )
+        {
+            dt_dprintk(" -> callback failed=%d\n", ret);
+            return ret;
+        }
+
+        imap += pintsize;
+        imaplen -= pintsize;
+
+        dt_dprintk(" -> imaplen=%d\n", imaplen);
+    }
+
+    return 0;
+
+fail:
+    return -EINVAL;
+}
+
 /**
  * dt_irq_map_raw - Low level interrupt tree parsing
  * @parent:     the device interrupt parent
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index b7455cd..93e313d 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -529,6 +529,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device, int index,
 int dt_irq_translate(const struct dt_raw_irq *raw, struct dt_irq *out_irq);
 
 /**
+ * dt_for_each_irq_map - Iterate over a nodes interrupt-map property
+ * @dev: The node whose interrupt-map property should be iterated over
+ * @cb: Call back to call for each entry
+ * @data: Caller data passed to callback
+ */
+int dt_for_each_irq_map(const struct dt_device_node *dev,
+                        int (*cb)(const struct dt_device_node *,
+                                  const struct dt_raw_irq *,
+                                  void *),
+                        void *data);
+
+/**
  * dt_n_size_cells - Helper to retrieve the number of cell for the size
  * @np: node to get the value
  *
-- 
1.7.10.4

[PATCH v2 2/3] xen: arm: propagate gic's #interrupt-cells property to dom0.

[Ian Campbell]

This is similar to 816f5bb1f074 "xen: arm: propagate gic's
should propagate (rather than invent our own value) since this value
is used to size fields within other properties within the tree.

I'm not sure why I didn't do this as part of 816f5bb1f074. I think
probably just because #interrupt-cells must always be 3 for a GIC
whereas #address-cells can legitimately differ. Regardless, I think we
might as well do this in common code.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |   18 +++++++++++++-----
 xen/arch/arm/gic-hip04.c    |    4 ----
 xen/arch/arm/gic-v2.c       |    4 ----
 xen/arch/arm/gic-v3.c       |    4 ----
 4 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ab4ad65..2a2fc2b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -784,8 +784,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
 {
     const struct dt_device_node *gic = dt_interrupt_controller;
     int res = 0;
-    const void *addrcells;
-    u32 addrcells_len;
+    const void *cells;
+    u32 cells_len;
 
     /*
      * Xen currently supports only a single GIC. Discard any secondary
@@ -815,10 +815,18 @@ static int make_gic_node(const struct domain *d, void *fdt,
             return res;
     }
 
-    addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
-    if ( addrcells )
+    cells = dt_get_property(gic, "#address-cells", &cells_len);
+    if ( cells )
     {
-        res = fdt_property(fdt, "#address-cells", addrcells, addrcells_len);
+        res = fdt_property(fdt, "#address-cells", cells, cells_len);
+        if ( res )
+            return res;
+    }
+
+    cells = dt_get_property(gic, "#interrupt-cells", &cells_len);
+    if ( cells )
+    {
+        res = fdt_property(fdt, "#interrupt-cells", cells, cells_len);
         if ( res )
             return res;
     }
diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 073ad33..42af10b 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -636,10 +636,6 @@ static int hip04gic_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
 
     if ( res )
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 20cdbc9..8d1a07d 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -622,10 +622,6 @@ static int gicv2_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
 
     if ( res )
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index ab80670..528500a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1102,10 +1102,6 @@ static int gicv3_make_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
-    if ( res )
-        return res;
-
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
     if ( res )
         return res;
-- 
1.7.10.4

Re: [PATCH v2 2/3] xen: arm: propagate gic's #interrupt-cells property to dom0.

[Julien Grall]

Hi Ian,

On 12/03/15 17:17, Ian Campbell wrote:
> This is similar to 816f5bb1f074 "xen: arm: propagate gic's
> should propagate (rather than invent our own value) since this value
> is used to size fields within other properties within the tree.
> I'm not sure why I didn't do this as part of 816f5bb1f074. I think
> probably just because #interrupt-cells must always be 3 for a GIC
> whereas #address-cells can legitimately differ. Regardless, I think we
> might as well do this in common code.

Hmmm... We are creating some interrupt ourself assuming the number of
interrupt cells is 3. So it makes sense to hard-code (not really invent)
the value.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   18 +++++++++++++-----
>  xen/arch/arm/gic-hip04.c    |    4 ----
>  xen/arch/arm/gic-v2.c       |    4 ----
>  xen/arch/arm/gic-v3.c       |    4 ----
>  4 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index ab4ad65..2a2fc2b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -784,8 +784,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
>  {
>      const struct dt_device_node *gic = dt_interrupt_controller;
>      int res = 0;
> -    const void *addrcells;
> -    u32 addrcells_len;
> +    const void *cells;
> +    u32 cells_len;
>  
>      /*
>       * Xen currently supports only a single GIC. Discard any secondary
> @@ -815,10 +815,18 @@ static int make_gic_node(const struct domain *d, void *fdt,
>              return res;
>      }
>  
> -    addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
> -    if ( addrcells )
> +    cells = dt_get_property(gic, "#address-cells", &cells_len);
> +    if ( cells )
>      {
> -        res = fdt_property(fdt, "#address-cells", addrcells, addrcells_len);
> +        res = fdt_property(fdt, "#address-cells", cells, cells_len);
> +        if ( res )
> +            return res;
> +    }
> +
> +    cells = dt_get_property(gic, "#interrupt-cells", &cells_len);
> +    if ( cells )
> +    {
> +        res = fdt_property(fdt, "#interrupt-cells", cells, cells_len);

The #interrupt-cells as to be present at any time for the GIC. So I
don't think it's worth to check if it presents. Maybe an ASSERT would be
enough?

Also, I would check somewhere that the value is effectively 3 otherwise
we are in trouble for the timer/evtchn interrupt creation. Though, it
was there before too.

>          if ( res )
>              return res;
>      }
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 073ad33..42af10b 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -636,10 +636,6 @@ static int hip04gic_make_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> -    if ( res )
> -        return res;
> -
>      res = fdt_property(fdt, "interrupt-controller", NULL, 0);
>  
>      if ( res )
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 20cdbc9..8d1a07d 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -622,10 +622,6 @@ static int gicv2_make_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> -    if ( res )
> -        return res;
> -
>      res = fdt_property(fdt, "interrupt-controller", NULL, 0);
>  
>      if ( res )
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index ab80670..528500a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1102,10 +1102,6 @@ static int gicv3_make_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> -    if ( res )
> -        return res;
> -
>      res = fdt_property(fdt, "interrupt-controller", NULL, 0);
>      if ( res )
>          return res;
> 

While you move #interrupt-cells to common code. Could you move
interrupt-controller too?

Regards,

-- 
Julien Grall

Re: [PATCH v2 2/3] xen: arm: propagate gic's #interrupt-cells property to dom0.

[Ian Campbell]

On Mon, 2015-03-16 at 16:01 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 12/03/15 17:17, Ian Campbell wrote:
> > This is similar to 816f5bb1f074 "xen: arm: propagate gic's
> > should propagate (rather than invent our own value) since this value
> > is used to size fields within other properties within the tree.
> > I'm not sure why I didn't do this as part of 816f5bb1f074. I think
> > probably just because #interrupt-cells must always be 3 for a GIC
> > whereas #address-cells can legitimately differ. Regardless, I think we
> > might as well do this in common code.
> 
> Hmmm... We are creating some interrupt ourself assuming the number of
> interrupt cells is 3. So it makes sense to hard-code (not really invent)
> the value.

I'll move the addition to common code but leave it as hard coded then.

> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/domain_build.c |   18 +++++++++++++-----
> >  xen/arch/arm/gic-hip04.c    |    4 ----
> >  xen/arch/arm/gic-v2.c       |    4 ----
> >  xen/arch/arm/gic-v3.c       |    4 ----
> >  4 files changed, 13 insertions(+), 17 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index ab4ad65..2a2fc2b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -784,8 +784,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
> >  {
> >      const struct dt_device_node *gic = dt_interrupt_controller;
> >      int res = 0;
> > -    const void *addrcells;
> > -    u32 addrcells_len;
> > +    const void *cells;
> > +    u32 cells_len;
> >  
> >      /*
> >       * Xen currently supports only a single GIC. Discard any secondary
> > @@ -815,10 +815,18 @@ static int make_gic_node(const struct domain *d, void *fdt,
> >              return res;
> >      }
> >  
> > -    addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
> > -    if ( addrcells )
> > +    cells = dt_get_property(gic, "#address-cells", &cells_len);
> > +    if ( cells )
> >      {
> > -        res = fdt_property(fdt, "#address-cells", addrcells, addrcells_len);
> > +        res = fdt_property(fdt, "#address-cells", cells, cells_len);
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    cells = dt_get_property(gic, "#interrupt-cells", &cells_len);
> > +    if ( cells )
> > +    {
> > +        res = fdt_property(fdt, "#interrupt-cells", cells, cells_len);
> 
> The #interrupt-cells as to be present at any time for the GIC. So I
> don't think it's worth to check if it presents. Maybe an ASSERT would be
> enough?

With the change discussed above it becomes moot.

> Also, I would check somewhere that the value is effectively 3 otherwise
> we are in trouble for the timer/evtchn interrupt creation. Though, it
> was there before too.

Probably somewhere should but I'm not sure where.

> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index ab80670..528500a 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1102,10 +1102,6 @@ static int gicv3_make_dt_node(const struct domain *d,
> >      if ( res )
> >          return res;
> >  
> > -    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> > -    if ( res )
> > -        return res;
> > -
> >      res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> >      if ( res )
> >          return res;
> > 
> 
> While you move #interrupt-cells to common code. Could you move
> interrupt-controller too?

I suppose I may as well.

Ian.

[PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Ian Campbell]

These properties are defined in ePAPR (2.3.8 and 2.4.3.1 respectively)
and the OpenFirmware PCI Bus Binding Specification (IEEE Std 1275-1994).

This replaces the xgene specific mapping. Tested on Mustang and on a
model with a PCI virtio controller.

TODO: Use a helper iterator (e.g. dt_for_each_range) for the ranges
property, like is already done for interrupts using
dt_for_each_irq_map.

TODO: Should we stop calling map_device for nodes beneath this one
(since we should have mapped everything underneath)? I think this is
complex for cases which map interrupt but not ranges or vice versa,
perhaps meaning we need to recurse on them separately. Maybe we can
continue to descend and the mappings may just be harmlessly
instantiated twice.

TODO: Related to the above there are also some buses for which we
cannot use dt_bus_default_{map,translate}. We might want to pull in
the of_bus_pci stuff from Linux, although I think that would be
orthogonal to this fix.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: This is essentially a complete reworking, which actually parses
things properly (obeying #{address,size,interrupt}-cells on the
appriopriate nodes) and includes handling of interrupt-map too.
---
 xen/arch/arm/domain_build.c          |  156 ++++++++++++++++++++++++++++++++++
 xen/arch/arm/platforms/xgene-storm.c |  143 -------------------------------
 2 files changed, 156 insertions(+), 143 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2a2fc2b..704c2aa 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -23,6 +23,21 @@
 #include <xen/irq.h>
 #include "kernel.h"
 
+/*
+ * Definitions for implementing parts of the OpenFirmware PCI Bus Binding
+ * Specification (IEEE Std 1275-1994).
+ */
+
+struct of_pci_unit_address {
+	__be32 hi, mid, lo;
+} __attribute__((packed));
+
+struct of_pci_ranges_entry {
+	struct of_pci_unit_address	pci_addr;
+	__be64				cpu_addr;
+	__be64				length;
+} __attribute__((packed));
+
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
@@ -911,6 +926,139 @@ static int make_timer_node(const struct domain *d, void *fdt,
     return res;
 }
 
+static int map_pci_device_ranges(struct domain *d,
+                                 const struct dt_device_node *dev,
+                                 const struct of_pci_ranges_entry  *ranges,
+                                 const u32 len)
+{
+    int parent_size_cells, parent_addr_cells;
+    int i, nr, res;
+
+    parent_size_cells = dt_n_size_cells(dev);
+    parent_addr_cells = dt_n_addr_cells(dev);
+
+    /*
+     * Range is child address, host address (#address-cells), length
+     * (#size-cells),see ePAPR 2.3.8.
+     *
+     * PCI child address is u32 space + u64 address, see ePAPR 6.2.2.
+     *
+     */
+    nr = len / sizeof(*ranges);
+
+    for ( i = 0; i < nr ; i++ )
+    {
+        const struct of_pci_ranges_entry *range = &ranges[i];
+        u64 addr, len;
+
+        len = fdt64_to_cpu(range->length);
+
+        addr = dt_translate_address(dev, (const __be32 *)&range->cpu_addr);
+        DPRINT("PCI SPACE 0x%08x.%08x.%08x 0x%"PRIx64" size 0x%"PRIx64"\n",
+               fdt32_to_cpu(range->pci_addr.hi),
+               fdt32_to_cpu(range->pci_addr.mid),
+               fdt32_to_cpu(range->pci_addr.lo),
+               addr, len);
+
+        res = map_mmio_regions(d,
+                               paddr_to_pfn(addr & PAGE_MASK),
+                               DIV_ROUND_UP(len, PAGE_SIZE),
+                               paddr_to_pfn(addr & PAGE_MASK));
+        if ( res < 0 )
+        {
+            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
+                   " - 0x%"PRIx64" in domain %d\n",
+                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
+                   d->domain_id);
+            return res;
+        }
+    }
+    return 0;
+}
+
+static int map_device_ranges(struct domain *d, const struct dt_device_node *dev)
+{
+    const void *ranges;
+    u32 len;
+
+    ranges = dt_get_property(dev, "ranges", &len);
+    /* No ranges, nothing to do */
+    if ( !ranges )
+        return 0;
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+        return map_pci_device_ranges(d, dev, ranges, len);
+
+    printk("Cannot handle ranges for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+
+    /* Lets not worry for now... */
+    return 0;
+}
+
+static int map_interrupt_to_domain(const struct dt_device_node *dev,
+                                   const struct dt_raw_irq *dt_raw_irq,
+                                   void *data)
+{
+    struct domain *d = data;
+    struct dt_irq dt_irq;
+    int res;
+
+    res = dt_irq_translate(dt_raw_irq, &dt_irq);
+    if ( res < 0 )
+    {
+        printk(XENLOG_ERR "%s: Failed to translate IRQ: %d\n",
+               dt_node_name(dev), res);
+        return res;
+    }
+
+    if ( dt_irq.irq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
+               dt_node_name(dev), dt_irq.irq);
+        return -EINVAL;
+    }
+
+    /* Setup the IRQ type */
+    res = irq_set_spi_type(dt_irq.irq, dt_irq.type);
+    if ( res )
+    {
+        printk(XENLOG_ERR
+               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
+               dt_node_name(dev), dt_irq.irq, d->domain_id);
+        return res;
+    }
+
+    res = route_irq_to_guest(d, dt_irq.irq, dt_node_name(dev));
+    if ( res < 0 )
+    {
+        printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
+               dt_irq.irq, d->domain_id);
+        return res;
+    }
+
+    DPRINT("PCI IRQ %u mapped to guest", dt_irq.irq);
+
+    return 0;
+}
+
+static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
+{
+
+    if ( !dt_property_read_bool(dev, "interrupt-map") )
+        return 0; /* No interrupt map to handle */
+
+    if ( dt_device_type_is_equal(dev, "pci") )
+        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
+
+    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
+           dt_node_name(dev), dev->type);
+
+    /* Lets not worry for now... */
+    return 0;
+}
+
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, struct dt_device_node *dev)
 {
@@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
         }
     }
 
+    res = map_device_ranges(d, dev);
+    if ( res )
+        return res;
+
+    res = map_device_interrupts(d, dev);
+    if ( res )
+        return res;
+
     return 0;
 }
 
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index eee650e..2355795 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -40,148 +40,6 @@ static uint32_t xgene_storm_quirks(void)
     return PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
 }
 
-static int map_one_mmio(struct domain *d, const char *what,
-                         unsigned long start, unsigned long end)
-{
-    int ret;
-
-    printk("Additional MMIO %lx-%lx (%s)\n",
-           start, end, what);
-    ret = map_mmio_regions(d, start, end - start, start);
-    if ( ret )
-        printk("Failed to map %s @ %lx to dom%d\n",
-               what, start, d->domain_id);
-    return ret;
-}
-
-static int map_one_spi(struct domain *d, const char *what,
-                       unsigned int spi, unsigned int type)
-{
-    unsigned int irq;
-    int ret;
-
-    irq = spi + 32; /* SPIs start at IRQ 32 */
-
-    ret = irq_set_spi_type(irq, type);
-    if ( ret )
-    {
-        printk("Failed to set the type for IRQ%u\n", irq);
-        return ret;
-    }
-
-    printk("Additional IRQ %u (%s)\n", irq, what);
-
-    if ( !vgic_reserve_virq(d, irq) )
-        printk("Failed to reserve vIRQ %u on dom%d\n",
-               irq, d->domain_id);
-
-    ret = route_irq_to_guest(d, irq, what);
-    if ( ret )
-        printk("Failed to route %s to dom%d\n", what, d->domain_id);
-
-    return ret;
-}
-
-/* Creates MMIO mappings base..end as well as 4 SPIs from the given base. */
-static int xgene_storm_pcie_specific_mapping(struct domain *d,
-                                             const struct dt_device_node *node,
-                                             paddr_t base, paddr_t end,
-                                             int base_spi)
-{
-    int ret;
-
-    printk("Mapping additional regions for PCIe device %s\n",
-           dt_node_full_name(node));
-
-    /* Map the PCIe bus resources */
-    ret = map_one_mmio(d, "PCI MEMORY", paddr_to_pfn(base), paddr_to_pfn(end));
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTA", base_spi+0, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTB", base_spi+1, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTC", base_spi+2, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = map_one_spi(d, "PCI#INTD", base_spi+3, DT_IRQ_TYPE_LEVEL_HIGH);
-    if ( ret )
-        goto err;
-
-    ret = 0;
-err:
-    return ret;
-}
-
-/*
- * Xen does not currently support mapping MMIO regions and interrupt
- * for bus child devices (referenced via the "ranges" and
- * "interrupt-map" properties to domain 0). Instead for now map the
- * necessary resources manually.
- */
-static int xgene_storm_specific_mapping(struct domain *d)
-{
-    struct dt_device_node *node = NULL;
-    int ret;
-
-    while ( (node = dt_find_compatible_node(node, "pci", "apm,xgene-pcie")) )
-    {
-        u64 addr;
-
-        /* Identify the bus via it's control register address */
-        ret = dt_device_get_address(node, 0, &addr, NULL);
-        if ( ret < 0 )
-            return ret;
-
-        if ( !dt_device_is_available(node) )
-            continue;
-
-       switch ( addr )
-        {
-        case 0x1f2b0000: /* PCIe0 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0e000000000UL, 0x10000000000UL, 0xc2);
-            break;
-        case 0x1f2c0000: /* PCIe1 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0d000000000UL, 0x0e000000000UL, 0xc8);
-            break;
-        case 0x1f2d0000: /* PCIe2 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x09000000000UL, 0x0a000000000UL, 0xce);
-            break;
-        case 0x1f500000: /* PCIe3 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0a000000000UL, 0x0c000000000UL, 0xd4);
-            break;
-        case 0x1f510000: /* PCIe4 */
-            ret = xgene_storm_pcie_specific_mapping(d,
-                node,
-                0x0c000000000UL, 0x0d000000000UL, 0xda);
-            break;
-
-        default:
-            printk("Ignoring unknown PCI bus %s\n", dt_node_full_name(node));
-            continue;
-        }
-
-        if ( ret < 0 )
-            return ret;
-    }
-
-    return 0;
-}
-
 static void xgene_storm_reset(void)
 {
     void __iomem *addr;
@@ -230,7 +88,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .init = xgene_storm_init,
     .reset = xgene_storm_reset,
     .quirks = xgene_storm_quirks,
-    .specific_mapping = xgene_storm_specific_mapping,
 
     .dom0_gnttab_start = 0x1f800000,
     .dom0_gnttab_size = 0x20000,
-- 
1.7.10.4

Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Julien Grall]

Hi Ian,

On 12/03/15 17:17, Ian Campbell wrote:
> These properties are defined in ePAPR (2.3.8 and 2.4.3.1 respectively)
> and the OpenFirmware PCI Bus Binding Specification (IEEE Std 1275-1994).
> 
> This replaces the xgene specific mapping. Tested on Mustang and on a
> model with a PCI virtio controller.
> 
> TODO: Use a helper iterator (e.g. dt_for_each_range) for the ranges
> property, like is already done for interrupts using
> dt_for_each_irq_map.
> 
> TODO: Should we stop calling map_device for nodes beneath this one
> (since we should have mapped everything underneath)? I think this is
> complex for cases which map interrupt but not ranges or vice versa,
> perhaps meaning we need to recurse on them separately. Maybe we can
> continue to descend and the mappings may just be harmlessly
> instantiated twice.

There is not harm to route twice the same IRQ. Although it's pointless
to continue.

How difficult would it be to introduce a new bus?

> TODO: Related to the above there are also some buses for which we
> cannot use dt_bus_default_{map,translate}. We might want to pull in
> the of_bus_pci stuff from Linux, although I think that would be
> orthogonal to this fix.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: This is essentially a complete reworking, which actually parses
> things properly (obeying #{address,size,interrupt}-cells on the
> appriopriate nodes) and includes handling of interrupt-map too.
> ---
>  xen/arch/arm/domain_build.c          |  156 ++++++++++++++++++++++++++++++++++
>  xen/arch/arm/platforms/xgene-storm.c |  143 -------------------------------
>  2 files changed, 156 insertions(+), 143 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2a2fc2b..704c2aa 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -23,6 +23,21 @@
>  #include <xen/irq.h>
>  #include "kernel.h"
>  
> +/*
> + * Definitions for implementing parts of the OpenFirmware PCI Bus Binding
> + * Specification (IEEE Std 1275-1994).
> + */
> +
> +struct of_pci_unit_address {
> +	__be32 hi, mid, lo;
> +} __attribute__((packed));
> +
> +struct of_pci_ranges_entry {
> +	struct of_pci_unit_address	pci_addr;
> +	__be64				cpu_addr;
> +	__be64				length;
> +} __attribute__((packed));
> +
>  static unsigned int __initdata opt_dom0_max_vcpus;
>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>  
> @@ -911,6 +926,139 @@ static int make_timer_node(const struct domain *d, void *fdt,
>      return res;
>  }
>  
> +static int map_pci_device_ranges(struct domain *d,
> +                                 const struct dt_device_node *dev,
> +                                 const struct of_pci_ranges_entry  *ranges,
> +                                 const u32 len)
> +{
> +    int parent_size_cells, parent_addr_cells;
> +    int i, nr, res;
> +
> +    parent_size_cells = dt_n_size_cells(dev);
> +    parent_addr_cells = dt_n_addr_cells(dev);
> +
> +    /*
> +     * Range is child address, host address (#address-cells), length
> +     * (#size-cells),see ePAPR 2.3.8.
> +     *
> +     * PCI child address is u32 space + u64 address, see ePAPR 6.2.2.
> +     *
> +     */
> +    nr = len / sizeof(*ranges);
> +
> +    for ( i = 0; i < nr ; i++ )
> +    {
> +        const struct of_pci_ranges_entry *range = &ranges[i];
> +        u64 addr, len;
> +
> +        len = fdt64_to_cpu(range->length);
> +
> +        addr = dt_translate_address(dev, (const __be32 *)&range->cpu_addr);
> +        DPRINT("PCI SPACE 0x%08x.%08x.%08x 0x%"PRIx64" size 0x%"PRIx64"\n",
> +               fdt32_to_cpu(range->pci_addr.hi),
> +               fdt32_to_cpu(range->pci_addr.mid),
> +               fdt32_to_cpu(range->pci_addr.lo),
> +               addr, len);
> +
> +        res = map_mmio_regions(d,
> +                               paddr_to_pfn(addr & PAGE_MASK),
> +                               DIV_ROUND_UP(len, PAGE_SIZE),
> +                               paddr_to_pfn(addr & PAGE_MASK));
> +        if ( res < 0 )
> +        {
> +            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> +                   " - 0x%"PRIx64" in domain %d\n",
> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> +                   d->domain_id);
> +            return res;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int map_device_ranges(struct domain *d, const struct dt_device_node *dev)
> +{
> +    const void *ranges;
> +    u32 len;
> +
> +    ranges = dt_get_property(dev, "ranges", &len);
> +    /* No ranges, nothing to do */
> +    if ( !ranges )
> +        return 0;
> +
> +    if ( dt_device_type_is_equal(dev, "pci") )
> +        return map_pci_device_ranges(d, dev, ranges, len);
> +
> +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
> +           dt_node_name(dev), dev->type);
> +

Is the printk really necessary? It will a spurious log on platform where
ranges is not empty (midway, arndale, foundation model...).

In general, we should not handle ranges by default as we may overlap the
mapping done during the domain creation (such as the GIC).

> +    /* Lets not worry for now... */
> +    return 0;
> +}
> +
> +static int map_interrupt_to_domain(const struct dt_device_node *dev,
> +                                   const struct dt_raw_irq *dt_raw_irq,
> +                                   void *data)
> +{
> +    struct domain *d = data;
> +    struct dt_irq dt_irq;
> +    int res;
> +
> +    res = dt_irq_translate(dt_raw_irq, &dt_irq);
> +    if ( res < 0 )
> +    {
> +        printk(XENLOG_ERR "%s: Failed to translate IRQ: %d\n",
> +               dt_node_name(dev), res);
> +        return res;
> +    }
> +
> +    if ( dt_irq.irq < NR_LOCAL_IRQS )
> +    {
> +        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
> +               dt_node_name(dev), dt_irq.irq);
> +        return -EINVAL;
> +    }
> +
> +    /* Setup the IRQ type */
> +    res = irq_set_spi_type(dt_irq.irq, dt_irq.type);
> +    if ( res )
> +    {
> +        printk(XENLOG_ERR
> +               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
> +               dt_node_name(dev), dt_irq.irq, d->domain_id);
> +        return res;
> +    }
> +
> +    res = route_irq_to_guest(d, dt_irq.irq, dt_node_name(dev));
> +    if ( res < 0 )
> +    {
> +        printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> +               dt_irq.irq, d->domain_id);
> +        return res;
> +    }
> +
> +    DPRINT("PCI IRQ %u mapped to guest", dt_irq.irq);
> +
> +    return 0;
> +}
> +
> +static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
> +{
> +
> +    if ( !dt_property_read_bool(dev, "interrupt-map") )

It's strange to use dt_property_read_bool here and dt_get_property above
to check the emptiness.

I prefer the dt_get_property version which is less confusing.

Anyway, why do you need to check interrupt-map. Can't your new helper
deal with empty property?

> +        return 0; /* No interrupt map to handle */
> +
> +    if ( dt_device_type_is_equal(dev, "pci") )
> +        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
> +
> +    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
> +           dt_node_name(dev), dev->type);
> +
> +    /* Lets not worry for now... */
> +    return 0;
> +}
> +
> +
>  /* Map the device in the domain */
>  static int map_device(struct domain *d, struct dt_device_node *dev)
>  {
> @@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>          }
>      }
>  
> +    res = map_device_ranges(d, dev);
> +    if ( res )
> +        return res;
> +
> +    res = map_device_interrupts(d, dev);

The name of the function doesn't make much sense. We already map the
interrupt above (see platform get_irq).

Regards,

-- 
Julien Grall

Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Ian Campbell]

On Mon, 2015-03-16 at 16:14 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 12/03/15 17:17, Ian Campbell wrote:
> > These properties are defined in ePAPR (2.3.8 and 2.4.3.1 respectively)
> > and the OpenFirmware PCI Bus Binding Specification (IEEE Std 1275-1994).
> > 
> > This replaces the xgene specific mapping. Tested on Mustang and on a
> > model with a PCI virtio controller.
> > 
> > TODO: Use a helper iterator (e.g. dt_for_each_range) for the ranges
> > property, like is already done for interrupts using
> > dt_for_each_irq_map.
> > 
> > TODO: Should we stop calling map_device for nodes beneath this one
> > (since we should have mapped everything underneath)? I think this is
> > complex for cases which map interrupt but not ranges or vice versa,
> > perhaps meaning we need to recurse on them separately. Maybe we can
> > continue to descend and the mappings may just be harmlessly
> > instantiated twice.
> 
> There is not harm to route twice the same IRQ. Although it's pointless
> to continue.
> 
> How difficult would it be to introduce a new bus?

It's a couple of reasonably trivial hook functions (which one would
naturally just get from Linux) and a struct tying a name to those. Not
too hard.

> > +    if ( dt_device_type_is_equal(dev, "pci") )
> > +        return map_pci_device_ranges(d, dev, ranges, len);
> > +
> > +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
> > +           dt_node_name(dev), dev->type);
> > +
> 
> Is the printk really necessary? It will a spurious log on platform where
> ranges is not empty (midway, arndale, foundation model...).

If the ranges is present and non-empty it's not impossible that we need
to be doing something with it. I'd rather try and figure out how to
whitelist such nodes, perhaps they lack a dev_type completely?

> In general, we should not handle ranges by default as we may overlap the
> mapping done during the domain creation (such as the GIC).
> 
> > +    /* Lets not worry for now... */
> > +    return 0;
> > +}
> > +
> > +static int map_interrupt_to_domain(const struct dt_device_node *dev,
> > +                                   const struct dt_raw_irq *dt_raw_irq,
> > +                                   void *data)
> > +{
> > +    struct domain *d = data;
> > +    struct dt_irq dt_irq;
> > +    int res;
> > +
> > +    res = dt_irq_translate(dt_raw_irq, &dt_irq);
> > +    if ( res < 0 )
> > +    {
> > +        printk(XENLOG_ERR "%s: Failed to translate IRQ: %d\n",
> > +               dt_node_name(dev), res);
> > +        return res;
> > +    }
> > +
> > +    if ( dt_irq.irq < NR_LOCAL_IRQS )
> > +    {
> > +        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
> > +               dt_node_name(dev), dt_irq.irq);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Setup the IRQ type */
> > +    res = irq_set_spi_type(dt_irq.irq, dt_irq.type);
> > +    if ( res )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
> > +               dt_node_name(dev), dt_irq.irq, d->domain_id);
> > +        return res;
> > +    }
> > +
> > +    res = route_irq_to_guest(d, dt_irq.irq, dt_node_name(dev));
> > +    if ( res < 0 )
> > +    {
> > +        printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> > +               dt_irq.irq, d->domain_id);
> > +        return res;
> > +    }
> > +
> > +    DPRINT("PCI IRQ %u mapped to guest", dt_irq.irq);
> > +
> > +    return 0;
> > +}
> > +
> > +static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
> > +{
> > +
> > +    if ( !dt_property_read_bool(dev, "interrupt-map") )
> 
> It's strange to use dt_property_read_bool here and dt_get_property above
> to check the emptiness.
> 
> I prefer the dt_get_property version which is less confusing.
> 
> Anyway, why do you need to check interrupt-map. Can't your new helper
> deal with empty property?

It can, but the code below would log for any non-pci device, which is
undesirable if there is no interrupt-map present.


> 
> > +        return 0; /* No interrupt map to handle */
> > +
> > +    if ( dt_device_type_is_equal(dev, "pci") )
> > +        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
> > +
> > +    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
> > +           dt_node_name(dev), dev->type);
> > +
> > +    /* Lets not worry for now... */
> > +    return 0;
> > +}
> > +
> > +
> >  /* Map the device in the domain */
> >  static int map_device(struct domain *d, struct dt_device_node *dev)
> >  {
> > @@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
> >          }
> >      }
> >  
> > +    res = map_device_ranges(d, dev);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = map_device_interrupts(d, dev);
> 
> The name of the function doesn't make much sense. We already map the
> interrupt above (see platform get_irq).

child_interrupt perhaps?

Ian.

Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Julien Grall]

On 16/03/15 16:22, Ian Campbell wrote:
>>> +    if ( dt_device_type_is_equal(dev, "pci") )
>>> +        return map_pci_device_ranges(d, dev, ranges, len);
>>> +
>>> +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
>>> +           dt_node_name(dev), dev->type);
>>> +
>>
>> Is the printk really necessary? It will a spurious log on platform where
>> ranges is not empty (midway, arndale, foundation model...).
> 
> If the ranges is present and non-empty it's not impossible that we need
> to be doing something with it. I'd rather try and figure out how to
> whitelist such nodes, perhaps they lack a dev_type completely?

Why would we compute the range? Any usable MMIO should be describe the
child.

We have to compute the ranges for PCI because we want to map everything
at boot time and the PCI devices are not discoverable via the device tree...

AFAICT the parsing of the range is only required when the child are not
discoverable via device tree. So the lack of device_type is not a good
check.

IHMO all those stuff could be removed by doing the parsing in
PHYSDEVOP_pci_device_add.

Anyway this is a different approach which require Xen PCI support in
Linux...

>>> +static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
>>> +{
>>> +
>>> +    if ( !dt_property_read_bool(dev, "interrupt-map") )
>>
>> It's strange to use dt_property_read_bool here and dt_get_property above
>> to check the emptiness.
>>
>> I prefer the dt_get_property version which is less confusing.
>>
>> Anyway, why do you need to check interrupt-map. Can't your new helper
>> deal with empty property?
> 
> It can, but the code below would log for any non-pci device, which is
> undesirable if there is no interrupt-map present.

It would be undesirable when interrupt-map is present for non-pci device...

We are able to go further down and map the interrupt so you will add
more worrying log on normal platform.

It's like the ranges, if we try to compute interrupt-map on non-pci we
may have some trouble and route IRQ we should not.

>>
>>> +        return 0; /* No interrupt map to handle */
>>> +
>>> +    if ( dt_device_type_is_equal(dev, "pci") )
>>> +        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
>>> +
>>> +    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
>>> +           dt_node_name(dev), dev->type);
>>> +
>>> +    /* Lets not worry for now... */
>>> +    return 0;
>>> +}
>>> +
>>> +
>>>  /* Map the device in the domain */
>>>  static int map_device(struct domain *d, struct dt_device_node *dev)
>>>  {
>>> @@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>>          }
>>>      }
>>>  
>>> +    res = map_device_ranges(d, dev);
>>> +    if ( res )
>>> +        return res;
>>> +
>>> +    res = map_device_interrupts(d, dev);
>>
>> The name of the function doesn't make much sense. We already map the
>> interrupt above (see platform get_irq).
> 
> child_interrupt perhaps?

Sounds better. Although, with my point above those 2 functions is PCI
related.

Regards,

-- 
Julien Grall

Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Ian Campbell]

On Mon, 2015-03-16 at 16:38 +0000, Julien Grall wrote:
> On 16/03/15 16:22, Ian Campbell wrote:
> >>> +    if ( dt_device_type_is_equal(dev, "pci") )
> >>> +        return map_pci_device_ranges(d, dev, ranges, len);
> >>> +
> >>> +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
> >>> +           dt_node_name(dev), dev->type);
> >>> +
> >>
> >> Is the printk really necessary? It will a spurious log on platform where
> >> ranges is not empty (midway, arndale, foundation model...).
> > 
> > If the ranges is present and non-empty it's not impossible that we need
> > to be doing something with it. I'd rather try and figure out how to
> > whitelist such nodes, perhaps they lack a dev_type completely?
> 
> Why would we compute the range? Any usable MMIO should be describe the
> child.
> 
> We have to compute the ranges for PCI because we want to map everything
> at boot time and the PCI devices are not discoverable via the device tree...

The same would be true for any similar discoverable bus.

Which I suppose is uncommon on ARM. I think I'll make this a debug level
print, or even a #if DT_DEBUG. (Same for the interrupt case)

Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Julien Grall]

On 16/03/15 16:46, Ian Campbell wrote:
> On Mon, 2015-03-16 at 16:38 +0000, Julien Grall wrote:
>> On 16/03/15 16:22, Ian Campbell wrote:
>>>>> +    if ( dt_device_type_is_equal(dev, "pci") )
>>>>> +        return map_pci_device_ranges(d, dev, ranges, len);
>>>>> +
>>>>> +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
>>>>> +           dt_node_name(dev), dev->type);
>>>>> +
>>>>
>>>> Is the printk really necessary? It will a spurious log on platform where
>>>> ranges is not empty (midway, arndale, foundation model...).
>>>
>>> If the ranges is present and non-empty it's not impossible that we need
>>> to be doing something with it. I'd rather try and figure out how to
>>> whitelist such nodes, perhaps they lack a dev_type completely?
>>
>> Why would we compute the range? Any usable MMIO should be describe the
>> child.
>>
>> We have to compute the ranges for PCI because we want to map everything
>> at boot time and the PCI devices are not discoverable via the device tree...
> 
> The same would be true for any similar discoverable bus.
> 
> Which I suppose is uncommon on ARM. I think I'll make this a debug level
> print, or even a #if DT_DEBUG. (Same for the interrupt case)

I'm ok with this solution. With Maybe adding a comment explaining that
we may need to parse it for discoverable bus?

Regards,

-- 
Julien Grall

Re: [PATCH v2 0/3] xen: arm: Parse PCI DT nodes' ranges and interrupt-map

[Ian Campbell]

On Thu, 2015-03-12 at 17:16 +0000, Ian Campbell wrote:
> This series adds parsing of the DT ranges and interrupt-map properties
> for PCI devices, these contain the MMIOs and IRQs used by children on
> the bus. This replaces the specific mapping stuff on xgene.

Somehow I managed to completely miss sending out the first patch here
(thanks Chen Baozi!)...

This should be inserted at the head of the series.

>From d0a024dd49ca6f67b0ec0342fd2d819b750a52a4 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 6 Mar 2015 11:13:30 +0000
Subject: [PATCH] xen: dt: add dt_translate_address to translate a raw address

A future patch is going to want to translate an address which is not
part of the reg property so the existing dt_device_get_address is not
suitable.

This is the same function as Linux's of_translate_address but with the
names changed to fit our context and the dev parameter constified.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/common/device_tree.c      |    5 +++++
 xen/include/xen/device_tree.h |    2 ++
 2 files changed, 7 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index d1c716f..89491b2 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -682,6 +682,11 @@ bail:
     return result;
 }
 
+u64 dt_translate_address(const struct dt_device_node *dev, const __be32 *in_addr)
+{
+        return __dt_translate_address(dev, in_addr, "ranges");
+}
+
 /* dt_device_address - Translate device tree address and return it */
 int dt_device_get_address(const struct dt_device_node *dev, int index,
                           u64 *addr, u64 *size)
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index c8a0375..b7455cd 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -464,6 +464,8 @@ struct dt_device_node *dt_find_node_by_path(const char *path);
  */
 const struct dt_device_node *dt_get_parent(const struct dt_device_node *node);
 
+u64 dt_translate_address(const struct dt_device_node *np, const __be32 *addr);
+
 /**
  * dt_device_get_address - Resolve an address for a device
  * @device: the device whose address is to be resolved
-- 
1.7.10.4

Re: [PATCH v2 0/3] xen: arm: Parse PCI DT nodes' ranges and interrupt-map

[Chen Baozi]

On Tue, Apr 14, 2015 at 09:43:07AM +0100, Ian Campbell wrote:
> On Thu, 2015-03-12 at 17:16 +0000, Ian Campbell wrote:
> > This series adds parsing of the DT ranges and interrupt-map properties
> > for PCI devices, these contain the MMIOs and IRQs used by children on
> > the bus. This replaces the specific mapping stuff on xgene.
> 
> Somehow I managed to completely miss sending out the first patch here
> (thanks Chen Baozi!)...
> 
> This should be inserted at the head of the series.
> 
> From d0a024dd49ca6f67b0ec0342fd2d819b750a52a4 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Fri, 6 Mar 2015 11:13:30 +0000
> Subject: [PATCH] xen: dt: add dt_translate_address to translate a raw address
> 
> A future patch is going to want to translate an address which is not
> part of the reg property so the existing dt_device_get_address is not
> suitable.
> 
> This is the same function as Linux's of_translate_address but with the
> names changed to fit our context and the dev parameter constified.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

These 4 patches works for me.

Tested-by: Chen Baozi <baozich@gmail.com>

> ---
>  xen/common/device_tree.c      |    5 +++++
>  xen/include/xen/device_tree.h |    2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index d1c716f..89491b2 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -682,6 +682,11 @@ bail:
>      return result;
>  }
>  
> +u64 dt_translate_address(const struct dt_device_node *dev, const __be32 *in_addr)
> +{
> +        return __dt_translate_address(dev, in_addr, "ranges");
> +}
> +
>  /* dt_device_address - Translate device tree address and return it */
>  int dt_device_get_address(const struct dt_device_node *dev, int index,
>                            u64 *addr, u64 *size)
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index c8a0375..b7455cd 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -464,6 +464,8 @@ struct dt_device_node *dt_find_node_by_path(const char *path);
>   */
>  const struct dt_device_node *dt_get_parent(const struct dt_device_node *node);
>  
> +u64 dt_translate_address(const struct dt_device_node *np, const __be32 *addr);
> +
>  /**
>   * dt_device_get_address - Resolve an address for a device
>   * @device: the device whose address is to be resolved
> -- 
> 1.7.10.4
> 
> 
> 
>