[PATCH 0/4 for-4.5] xen: arm: xgene bug fixes + support for McDivitt

[PATCH 0/4 for-4.5] xen: arm: xgene bug fixes + support for McDivitt

[Ian Campbell]

These patches:

      * fix up an off by one bug in the xgene mapping of additional PCI
        bus resources, which would cause an additional extra page to be
        mapped
      * correct the size of the mapped regions to match the docs
      * adds support for the other 4 PCI buses on the chip, which
        enables mcdivitt and presumably most other Xgene based platforms
        which uses PCI buses other than pcie0.
      * adds earlyprintk for the mcdivitt platform

McDivitt is the X-Gene based HP Moonshot cartridge (McDivitt is the code
name, I think the product is called m400, not quite sure).

Other than the bug fixes I'd like to see the mcdivitt support
(specifically the "other 4 PCI buses" one) in 4.5 because Moonshot is an
interesting and exciting platform for arm64. It is also being used for
ongoing work on Xen on ARM on Openstack in Linaro. The earlyprintk patch
is totally harmless unless it's explicitly enabled at compile time, IMHO
if we are taking the rest we may as well throw it in...

The risk here is that we break the existing support for the Mustang
platform, which would be the most likely failure case for the second
patch. I've tested these on a Mustang, including firing up a PCI NIC
device. The new mappings are a superset of the existing ones so the
potential for breakage should be quite small.

I've also successfully tested on a McDivitt.

Ian.

[PATCH for-4.5 1/4] xen: arm: Add earlyprintk for McDivitt.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Rules.mk |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 572d854..ef887a5 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -95,6 +95,12 @@ EARLY_PRINTK_BAUD := 115200
 EARLY_UART_BASE_ADDRESS := 0x1c020000
 EARLY_UART_REG_SHIFT := 2
 endif
+ifeq ($(CONFIG_EARLY_PRINTK), xgene-mcdivitt)
+EARLY_PRINTK_INC := 8250
+EARLY_PRINTK_BAUD := 9600
+EARLY_UART_BASE_ADDRESS := 0x1c021000
+EARLY_UART_REG_SHIFT := 2
+endif
 ifeq ($(CONFIG_EARLY_PRINTK), juno)
 EARLY_PRINTK_INC := pl011
 EARLY_PRINTK_BAUD := 115200
-- 
1.7.10.4

Re: [PATCH for-4.5 1/4] xen: arm: Add earlyprintk for McDivitt.

[Julien Grall]

Hi Ian,

On 11/18/2014 04:44 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/Rules.mk |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 572d854..ef887a5 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -95,6 +95,12 @@ EARLY_PRINTK_BAUD := 115200
>  EARLY_UART_BASE_ADDRESS := 0x1c020000
>  EARLY_UART_REG_SHIFT := 2
>  endif
> +ifeq ($(CONFIG_EARLY_PRINTK), xgene-mcdivitt)
> +EARLY_PRINTK_INC := 8250
> +EARLY_PRINTK_BAUD := 9600

EARLY_PRINTK_BAUD is not necessary as we don't use the initialization
function (EARLY_PRINTK_INIT_UART is not set).

With the EARLY_PRINTK_BAUD dropped, this could be merged with the
xgene-storm  early printk (I didn't really understand why the baud rate
is different). But I don't think it's 4.5 material.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 1/4] xen: arm: Add earlyprintk for McDivitt.

[Ian Campbell]

On Tue, 2014-11-18 at 16:59 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 11/18/2014 04:44 PM, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/Rules.mk |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> > index 572d854..ef887a5 100644
> > --- a/xen/arch/arm/Rules.mk
> > +++ b/xen/arch/arm/Rules.mk
> > @@ -95,6 +95,12 @@ EARLY_PRINTK_BAUD := 115200
> >  EARLY_UART_BASE_ADDRESS := 0x1c020000
> >  EARLY_UART_REG_SHIFT := 2
> >  endif
> > +ifeq ($(CONFIG_EARLY_PRINTK), xgene-mcdivitt)
> > +EARLY_PRINTK_INC := 8250
> > +EARLY_PRINTK_BAUD := 9600
> 
> EARLY_PRINTK_BAUD is not necessary as we don't use the initialization
> function (EARLY_PRINTK_INIT_UART is not set).

Oh yes, oops. Also the baud is not even what is actually used, so it's
not even serving a documentary purpose.

> With the EARLY_PRINTK_BAUD dropped, this could be merged with the
> xgene-storm  early printk

It's at a different base address. Long term I either want to make this
(somewhat) runtime configurable or at least to rationalise the options
into the form <soc/soc-family>-uart<N>, or perhaps even <8250|pl011|
etc>@<address>[,<rate><settings>], if it's not to skanky to arrange to
parse that somewhere in the build system. Not for 4.5 though.

> (I didn't really understand why the baud rate
> is different).

Different hardware might potentially have different baud rates
configured in firmware which we would want to seemlessly follow, but
it's moot since the right thing to do in most cases is leave the
bootloader provided cfg alone.

> But I don't think it's 4.5 material.

You mean the patch generally or the merging?

Ian.

Re: [PATCH for-4.5 1/4] xen: arm: Add earlyprintk for McDivitt.

[Julien Grall]

Hi,

On 19/11/2014 09:54, Ian Campbell wrote:
> On Tue, 2014-11-18 at 16:59 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 11/18/2014 04:44 PM, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>   xen/arch/arm/Rules.mk |    6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
>>> index 572d854..ef887a5 100644
>>> --- a/xen/arch/arm/Rules.mk
>>> +++ b/xen/arch/arm/Rules.mk
>>> @@ -95,6 +95,12 @@ EARLY_PRINTK_BAUD := 115200
>>>   EARLY_UART_BASE_ADDRESS := 0x1c020000
>>>   EARLY_UART_REG_SHIFT := 2
>>>   endif
>>> +ifeq ($(CONFIG_EARLY_PRINTK), xgene-mcdivitt)
>>> +EARLY_PRINTK_INC := 8250
>>> +EARLY_PRINTK_BAUD := 9600
>>
>> EARLY_PRINTK_BAUD is not necessary as we don't use the initialization
>> function (EARLY_PRINTK_INIT_UART is not set).
>
> Oh yes, oops. Also the baud is not even what is actually used, so it's
> not even serving a documentary purpose.
>
>> With the EARLY_PRINTK_BAUD dropped, this could be merged with the
>> xgene-storm  early printk
>
> It's at a different base address. Long term I either want to make this
> (somewhat) runtime configurable or at least to rationalise the options
> into the form <soc/soc-family>-uart<N>, or perhaps even <8250|pl011|
> etc>@<address>[,<rate><settings>], if it's not to skanky to arrange to
> parse that somewhere in the build system. Not for 4.5 though.

> You mean the patch generally or the merging?

I meant rationalise the number of early printk. This patch looks fine 
for me.

Regards,

-- 
Julien Grall

[PATCH for-4.5 2/4] xen: arm: correct off by one in xgene-storm's map_one_mmio

[Ian Campbell]

The callers pass the end as the pfn immediately *after* the last page to be
mapped, therefore adding one is incorrect and causes an additional page to be
mapped.

At the same time correct the printing of the mfn values, zero-padding them to
16 digits as for a paddr when they are frame numbers is just confusing.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/platforms/xgene-storm.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 29c4752..38674cd 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
 {
     int ret;
 
-    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
+    printk("Additional MMIO %lx-%lx (%s)\n",
            start, end, what);
-    ret = map_mmio_regions(d, start, end - start + 1, start);
+    ret = map_mmio_regions(d, start, end - start, start);
     if ( ret )
         printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
                what, start, d->domain_id);
-- 
1.7.10.4

Re: [PATCH for-4.5 2/4] xen: arm: correct off by one in xgene-storm's map_one_mmio

[Julien Grall]

Hi Ian,

On 11/18/2014 04:44 PM, Ian Campbell wrote:
> The callers pass the end as the pfn immediately *after* the last page to be
> mapped, therefore adding one is incorrect and causes an additional page to be
> mapped.
> 
> At the same time correct the printing of the mfn values, zero-padding them to
> 16 digits as for a paddr when they are frame numbers is just confusing.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/platforms/xgene-storm.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 29c4752..38674cd 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
>  {
>      int ret;
>  
> -    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
> +    printk("Additional MMIO %lx-%lx (%s)\n",
>             start, end, what);
> -    ret = map_mmio_regions(d, start, end - start + 1, start);
> +    ret = map_mmio_regions(d, start, end - start, start);
>      if ( ret )
>          printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
>                 what, start, d->domain_id);

As you fixed the previous printf format. I would fix this one too.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 2/4] xen: arm: correct off by one in xgene-storm's map_one_mmio

[Ian Campbell]

On Tue, 2014-11-18 at 17:01 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 11/18/2014 04:44 PM, Ian Campbell wrote:
> > The callers pass the end as the pfn immediately *after* the last page to be
> > mapped, therefore adding one is incorrect and causes an additional page to be
> > mapped.
> > 
> > At the same time correct the printing of the mfn values, zero-padding them to
> > 16 digits as for a paddr when they are frame numbers is just confusing.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/platforms/xgene-storm.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> > index 29c4752..38674cd 100644
> > --- a/xen/arch/arm/platforms/xgene-storm.c
> > +++ b/xen/arch/arm/platforms/xgene-storm.c
> > @@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
> >  {
> >      int ret;
> >  
> > -    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
> > +    printk("Additional MMIO %lx-%lx (%s)\n",
> >             start, end, what);
> > -    ret = map_mmio_regions(d, start, end - start + 1, start);
> > +    ret = map_mmio_regions(d, start, end - start, start);
> >      if ( ret )
> >          printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
> >                 what, start, d->domain_id);
> 
> As you fixed the previous printf format. I would fix this one too.

Yes, good idea.

Ian.

Re: [PATCH for-4.5 2/4] xen: arm: correct off by one in xgene-storm's map_one_mmio

[Konrad Rzeszutek Wilk]

On Tue, Nov 18, 2014 at 04:44:46PM +0000, Ian Campbell wrote:
> The callers pass the end as the pfn immediately *after* the last page to be
> mapped, therefore adding one is incorrect and causes an additional page to be
> mapped.
> 
> At the same time correct the printing of the mfn values, zero-padding them to
> 16 digits as for a paddr when they are frame numbers is just confusing.

HA! I was just looking at that today and thought it was odd.

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/platforms/xgene-storm.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 29c4752..38674cd 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
>  {
>      int ret;
>  
> -    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
> +    printk("Additional MMIO %lx-%lx (%s)\n",
>             start, end, what);
> -    ret = map_mmio_regions(d, start, end - start + 1, start);
> +    ret = map_mmio_regions(d, start, end - start, start);
>      if ( ret )
>          printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
>                 what, start, d->domain_id);
> -- 
> 1.7.10.4
> 

[PATCH for-4.5 3/4] xen: arm: correct specific mappings for PCIE0 on X-Gene

[Ian Campbell]

The region assigned to PCIE0, according to the docs, is 0x0e000000000 to
0x10000000000. They make no distinction between PCI CFG and PCI IO mem within
this range (in fact, I'm not sure that isn't up to the driver).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/platforms/xgene-storm.c |   18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 38674cd..6c432a1 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -89,22 +89,8 @@ static int xgene_storm_specific_mapping(struct domain *d)
     int ret;
 
     /* Map the PCIe bus resources */
-    ret = map_one_mmio(d, "PCI MEM REGION", paddr_to_pfn(0xe000000000UL),
-                                            paddr_to_pfn(0xe010000000UL));
-    if ( ret )
-        goto err;
-
-    ret = map_one_mmio(d, "PCI IO REGION", paddr_to_pfn(0xe080000000UL),
-                                           paddr_to_pfn(0xe080010000UL));
-    if ( ret )
-        goto err;
-
-    ret = map_one_mmio(d, "PCI CFG REGION", paddr_to_pfn(0xe0d0000000UL),
-                                            paddr_to_pfn(0xe0d0200000UL));
-    if ( ret )
-        goto err;
-    ret = map_one_mmio(d, "PCI MSI REGION", paddr_to_pfn(0xe010000000UL),
-                                            paddr_to_pfn(0xe010800000UL));
+    ret = map_one_mmio(d, "PCI MEMORY", paddr_to_pfn(0x0e000000000UL),
+                                        paddr_to_pfn(0x01000000000UL));
     if ( ret )
         goto err;
 
-- 
1.7.10.4

Re: [PATCH for-4.5 3/4] xen: arm: correct specific mappings for PCIE0 on X-Gene

[Julien Grall]

Hi Ian,

On 11/18/2014 04:44 PM, Ian Campbell wrote:
> The region assigned to PCIE0, according to the docs, is 0x0e000000000 to
> 0x10000000000. They make no distinction between PCI CFG and PCI IO mem within
> this range (in fact, I'm not sure that isn't up to the driver).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

[PATCH for-4.5 4/4] xen: arm: Support the other 4 PCI buses on Xgene

[Ian Campbell]

Currently we only establish specific mappings for pcie0, which is
used on the Mustang platform. However at least McDivitt uses pcie3.
So wire up all the others, based on whether the corresponding DT node
is marked as available.

This results in no change for Mustang.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/platforms/xgene-storm.c |   84 ++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 6c432a1..926c325 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -78,35 +78,31 @@ static int map_one_spi(struct domain *d, const char *what,
     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)
+/* Creates MMIO mappings base..end as well as 4 SPIs from the given base. */
+static int xgene_storm_pcie_specific_mapping(struct domain *d,
+                                             paddr_t base, paddr_t end,
+                                             int base_spi)
 {
     int ret;
 
     /* Map the PCIe bus resources */
-    ret = map_one_mmio(d, "PCI MEMORY", paddr_to_pfn(0x0e000000000UL),
-                                        paddr_to_pfn(0x01000000000UL));
+    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", 0xc2, DT_IRQ_TYPE_LEVEL_HIGH);
+    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", 0xc3, DT_IRQ_TYPE_LEVEL_HIGH);
+    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", 0xc4, DT_IRQ_TYPE_LEVEL_HIGH);
+    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", 0xc5, DT_IRQ_TYPE_LEVEL_HIGH);
+    ret = map_one_spi(d, "PCI#INTD", base_spi+3, DT_IRQ_TYPE_LEVEL_HIGH);
     if ( ret )
         goto err;
 
@@ -115,6 +111,68 @@ 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,
+                0x0e000000000UL, 0x10000000000UL, 0xc2);
+            break;
+        case 0x1f2c0000: /* PCIe1 */
+            ret = xgene_storm_pcie_specific_mapping(d,
+                0x0d000000000UL, 0x0e000000000UL, 0xc8);
+            break;
+        case 0x1f2d0000: /* PCIe2 */
+            ret = xgene_storm_pcie_specific_mapping(d,
+                0x09000000000UL, 0x0a000000000UL, 0xce);
+            break;
+        case 0x1f500000: /* PCIe3 */
+            ret = xgene_storm_pcie_specific_mapping(d,
+                0x0a000000000UL, 0x0c000000000UL, 0xd4);
+            break;
+        case 0x1f510000: /* PCIe4 */
+            ret = xgene_storm_pcie_specific_mapping(d,
+                0x0c000000000UL, 0x0d000000000UL, 0xda);
+            break;
+
+        default:
+            /* Ignore unknown PCI busses */
+            ret = 0;
+            break;
+        }
+
+        if ( ret < 0 )
+            return ret;
+
+        printk("Mapped additional regions for PCIe device at 0x%"PRIx64"\n",
+               addr);
+    }
+
+    return 0;
+}
+
 static void xgene_storm_reset(void)
 {
     void __iomem *addr;
-- 
1.7.10.4

Re: [PATCH for-4.5 4/4] xen: arm: Support the other 4 PCI buses on Xgene

[Julien Grall]

Hi Ian,

On 11/18/2014 04:44 PM, Ian Campbell wrote:
> Currently we only establish specific mappings for pcie0, which is
> used on the Mustang platform. However at least McDivitt uses pcie3.
> So wire up all the others, based on whether the corresponding DT node
> is marked as available.
> 
> This results in no change for Mustang.

Hopefully, we will support PCI DT parsing in Xen 4.6!

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> +/*
> + * 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;

NIT: const struct

> +    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,
> +                0x0e000000000UL, 0x10000000000UL, 0xc2);
> +            break;
> +        case 0x1f2c0000: /* PCIe1 */
> +            ret = xgene_storm_pcie_specific_mapping(d,
> +                0x0d000000000UL, 0x0e000000000UL, 0xc8);
> +            break;
> +        case 0x1f2d0000: /* PCIe2 */
> +            ret = xgene_storm_pcie_specific_mapping(d,
> +                0x09000000000UL, 0x0a000000000UL, 0xce);
> +            break;
> +        case 0x1f500000: /* PCIe3 */
> +            ret = xgene_storm_pcie_specific_mapping(d,
> +                0x0a000000000UL, 0x0c000000000UL, 0xd4);
> +            break;
> +        case 0x1f510000: /* PCIe4 */
> +            ret = xgene_storm_pcie_specific_mapping(d,
> +                0x0c000000000UL, 0x0d000000000UL, 0xda);
> +            break;
> +
> +        default:
> +            /* Ignore unknown PCI busses */

I would add a
printk("Ignoring PCI busses %s\n", dt_node_full_name(dev));

> +            ret = 0;
> +            break;

continue? You can't assume the order of the PCI busses in the device tree.

> +        }
> +
> +        if ( ret < 0 )
> +            return ret;
> +
> +        printk("Mapped additional regions for PCIe device at 0x%"PRIx64"\n",
> +               addr);

Printing the device tree path would be more helpful than the address.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 4/4] xen: arm: Support the other 4 PCI buses on Xgene

[Ian Campbell]

On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote:
> > +        default:
> > +            /* Ignore unknown PCI busses */
> 
> I would add a
> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev));
> 
> > +            ret = 0;
> > +            break;
> 
> continue?

Yes, that makes sense (probably the ret = is then unnecessary).

>  You can't assume the order of the PCI busses in the device tree.

But, I don't understand what this has to do with using continue.

> 
> > +        }
> > +
> > +        if ( ret < 0 )
> > +            return ret;
> > +
> > +        printk("Mapped additional regions for PCIe device at 0x%"PRIx64"\n",
> > +               addr);
> 
> Printing the device tree path would be more helpful than the address.

OK.

Re: [PATCH for-4.5 4/4] xen: arm: Support the other 4 PCI buses on Xgene

[Julien Grall]

Hi Ian,

On 19/11/2014 09:56, Ian Campbell wrote:
> On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote:
>>> +        default:
>>> +            /* Ignore unknown PCI busses */
>>
>> I would add a
>> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev));
>>
>>> +            ret = 0;
>>> +            break;
>>
>> continue?
>
> Yes, that makes sense (probably the ret = is then unnecessary).
>
>>   You can't assume the order of the PCI busses in the device tree.
>
> But, I don't understand what this has to do with using continue.

The current xgene-storm DTS has the different PCI busses ordered. So as 
soon as you don't find the PCI range, it means there is no more PCI busses.

Without the continue, this patch gives the impression that you rely on 
the node order on the device tree.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 4/4] xen: arm: Support the other 4 PCI buses on Xgene

[Ian Campbell]

On Wed, 2014-11-19 at 10:06 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 19/11/2014 09:56, Ian Campbell wrote:
> > On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote:
> >>> +        default:
> >>> +            /* Ignore unknown PCI busses */
> >>
> >> I would add a
> >> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev));
> >>
> >>> +            ret = 0;
> >>> +            break;
> >>
> >> continue?
> >
> > Yes, that makes sense (probably the ret = is then unnecessary).
> >
> >>   You can't assume the order of the PCI busses in the device tree.
> >
> > But, I don't understand what this has to do with using continue.
> 
> The current xgene-storm DTS has the different PCI busses ordered. So as 
> soon as you don't find the PCI range, it means there is no more PCI busses.

I don't think it does, the patch iterates over all of the buses, even
ones we don't understand, we don't give up at the first one we don't
grok.

> Without the continue, this patch gives the impression that you rely on 
> the node order on the device tree.



> 
> Regards,
> 

Re: [PATCH for-4.5 4/4] xen: arm: Support the other 4 PCI buses on Xgene

[Julien Grall]

On 19/11/2014 10:18, Ian Campbell wrote:
> On Wed, 2014-11-19 at 10:06 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 19/11/2014 09:56, Ian Campbell wrote:
>>> On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote:
>>>>> +        default:
>>>>> +            /* Ignore unknown PCI busses */
>>>>
>>>> I would add a
>>>> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev));
>>>>
>>>>> +            ret = 0;
>>>>> +            break;
>>>>
>>>> continue?
>>>
>>> Yes, that makes sense (probably the ret = is then unnecessary).
>>>
>>>>    You can't assume the order of the PCI busses in the device tree.
>>>
>>> But, I don't understand what this has to do with using continue.
>>
>> The current xgene-storm DTS has the different PCI busses ordered. So as
>> soon as you don't find the PCI range, it means there is no more PCI busses.
>
> I don't think it does, the patch iterates over all of the buses, even
> ones we don't understand, we don't give up at the first one we don't
> grok.

Hrmm you are right. I don't know why I though the break were bound to 
the loop and not the switch.

Sorry for the noise.

Regards,

-- 
Julien Grall

Re: [PATCH 0/4 for-4.5] xen: arm: xgene bug fixes + support for McDivitt

[Ian Campbell]

On Tue, 2014-11-18 at 16:44 +0000, Ian Campbell wrote:
> These patches:

... which are also at
        git://xenbits.xen.org/people/ianc/xen.git mcdivitt-v1

Ian.

Re: [PATCH 0/4 for-4.5] xen: arm: xgene bug fixes + support for McDivitt

[Konrad Rzeszutek Wilk]

On Tue, Nov 18, 2014 at 04:51:42PM +0000, Ian Campbell wrote:
> On Tue, 2014-11-18 at 16:44 +0000, Ian Campbell wrote:
> > These patches:
> 
> ... which are also at
>         git://xenbits.xen.org/people/ianc/xen.git mcdivitt-v1

I presume you are going to post v2 with Julian's feedback rolled in?

I took a look at the code and it looks Xen 4.5 material so I am
OK with it rolling in, but would appreciate another posting just
to make sure that nothing is amiss.

Thank you!
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0/4 for-4.5] xen: arm: xgene bug fixes + support for McDivitt

[Ian Campbell]

On Wed, 2014-11-19 at 16:18 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 18, 2014 at 04:51:42PM +0000, Ian Campbell wrote:
> > On Tue, 2014-11-18 at 16:44 +0000, Ian Campbell wrote:
> > > These patches:
> > 
> > ... which are also at
> >         git://xenbits.xen.org/people/ianc/xen.git mcdivitt-v1
> 
> I presume you are going to post v2 with Julian's feedback rolled in?

Yes, see <1416410868.29243.39.camel@citrix.com>.

> I took a look at the code and it looks Xen 4.5 material so I am
> OK with it rolling in, but would appreciate another posting just
> to make sure that nothing is amiss.
> 
> Thank you!
> > 
> > Ian.
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel