[Qemu-devel] [PATCH 0/5] PPC: e500: Enable more PCI slots for -M ppce500

[Qemu-devel] [PATCH 0/5] PPC: e500: Enable more PCI slots for -M ppce500

[Alexander Graf]

Right now all e500 machines have a limit of 2 PCI devices. This is too little
when one virtio device already eats up one PCI slot.

While we can't change the -M mpc8544ds target on that respect because it
physically only has 2 slots, we can easily loosen that restriction for -M
ppce500.

So this patch set enables us to use up to 31 PCI devices (slots) on -M ppce500
while maintaining the old behavior for -M mpc8544ds

Alexander Graf (5):
  PPC: E500: PCI: Make first slot qdev settable
  PPC: E500: PCI: Make IRQ calculation more generic
  PPC: E500: Generate dt pci irq map dynamically
  PPC: E500: Move PCI slot information into params
  PPC: E500plat: Make a lot of PCI slots available

 hw/ppc/e500.c      |   53 ++++++++++++++++++++++++++++++++-------------------
 hw/ppc/e500.h      |    2 +
 hw/ppc/e500plat.c  |    3 ++
 hw/ppc/mpc8544ds.c |    2 +
 hw/ppce500_pci.c   |   22 ++++++++++----------
 5 files changed, 51 insertions(+), 31 deletions(-)

[Qemu-devel] [PATCH 1/5] PPC: E500: PCI: Make first slot qdev settable

[Alexander Graf]

Today the first slot id in our e500 pci implementation is hardcoded to
0x11. Keep it there as default, but allow users to change the default to
a different id.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppce500_pci.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index e534341..4cd4edc 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -87,6 +87,7 @@ struct PPCE500PCIState {
     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
     uint32_t gasket_time;
     qemu_irq irq[4];
+    uint32_t first_slot;
     /* mmio maps */
     MemoryRegion container;
     MemoryRegion iomem;
@@ -361,7 +362,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
 
     b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
                          mpc85xx_pci_map_irq, s->irq, address_space_mem,
-                         &s->pio, PCI_DEVFN(0x11, 0), 4);
+                         &s->pio, PCI_DEVFN(s->first_slot, 0), 4);
     h->bus = b;
 
     pci_create_simple(b, 0, "e500-host-bridge");
@@ -401,12 +402,18 @@ static const TypeInfo e500_host_bridge_info = {
     .class_init    = e500_host_bridge_class_init,
 };
 
+static Property pcihost_properties[] = {
+    DEFINE_PROP_UINT32("first_slot", PPCE500PCIState, first_slot, 0x11),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void e500_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
     k->init = e500_pcihost_initfn;
+    dc->props = pcihost_properties;
     dc->vmsd = &vmstate_ppce500_pci;
 }
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/5] PPC: E500: PCI: Make IRQ calculation more generic

[Alexander Graf]

The IRQ line calculation is more or less hardcoded today. Instead, let's
write it as an algorithmic function that theoretically allows an arbitrary
number of PCI slots.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppce500_pci.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 4cd4edc..561a776 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -253,17 +253,10 @@ static const MemoryRegionOps e500_pci_reg_ops = {
 
 static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
 {
-    int devno = pci_dev->devfn >> 3, ret = 0;
+    int devno = pci_dev->devfn >> 3;
+    int ret;
 
-    switch (devno) {
-        /* Two PCI slot */
-        case 0x11:
-        case 0x12:
-            ret = (irq_num + devno - 0x10) % 4;
-            break;
-        default:
-            printf("Error:%s:unknown dev number\n", __func__);
-    }
+    ret = (irq_num + devno) % 4;
 
     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
            pci_dev->devfn, irq_num, ret, devno);
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Alexander Graf]

Today we're hardcoding the PCI interrupt map in the e500 machine file.
Instead, let's write it dynamically so that different machine types
can have different slot properties.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc/e500.c |   51 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 1034f93..ebb6d96 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -66,25 +66,33 @@ struct boot_info
     uint32_t entry;
 };
 
-static void pci_map_create(void *fdt, uint32_t *pci_map, uint32_t mpic)
+static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
+                                int nr_slots, int *len)
 {
-    int i;
-    const uint32_t tmp[] = {
-                             /* IDSEL 0x11 J17 Slot 1 */
-                             0x8800, 0x0, 0x0, 0x1, mpic, 0x2, 0x1,
-                             0x8800, 0x0, 0x0, 0x2, mpic, 0x3, 0x1,
-                             0x8800, 0x0, 0x0, 0x3, mpic, 0x4, 0x1,
-                             0x8800, 0x0, 0x0, 0x4, mpic, 0x1, 0x1,
-
-                             /* IDSEL 0x12 J16 Slot 2 */
-                             0x9000, 0x0, 0x0, 0x1, mpic, 0x3, 0x1,
-                             0x9000, 0x0, 0x0, 0x2, mpic, 0x4, 0x1,
-                             0x9000, 0x0, 0x0, 0x3, mpic, 0x2, 0x1,
-                             0x9000, 0x0, 0x0, 0x4, mpic, 0x1, 0x1,
-                           };
-    for (i = 0; i < (7 * 8); i++) {
-        pci_map[i] = cpu_to_be32(tmp[i]);
+    int i = 0;
+    int slot;
+    int pci_irq;
+    int last_slot = first_slot + nr_slots;
+    uint32_t *pci_map;
+
+    *len = nr_slots * 4 * 7 * sizeof(uint32_t);
+    pci_map = g_malloc(*len);
+
+    for (slot = first_slot; slot < last_slot; slot++) {
+        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
+            pci_map[i++] = cpu_to_be32(slot << 11);
+            pci_map[i++] = cpu_to_be32(0x0);
+            pci_map[i++] = cpu_to_be32(0x0);
+            pci_map[i++] = cpu_to_be32(pci_irq + 1);
+            pci_map[i++] = cpu_to_be32(mpic);
+            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
+            pci_map[i++] = cpu_to_be32(0x1);
+        }
     }
+
+    assert((i * sizeof(uint32_t)) == *len);
+
+    return pci_map;
 }
 
 static void dt_serial_create(void *fdt, unsigned long long offset,
@@ -132,7 +140,8 @@ static int ppce500_load_device_tree(CPUPPCState *env,
     char gutil[128];
     char pci[128];
     char msi[128];
-    uint32_t pci_map[7 * 8];
+    uint32_t *pci_map = NULL;
+    int len;
     uint32_t pci_ranges[14] =
         {
             0x2000000, 0x0, 0xc0000000,
@@ -329,8 +338,9 @@ static int ppce500_load_device_tree(CPUPPCState *env,
     qemu_devtree_setprop_string(fdt, pci, "device_type", "pci");
     qemu_devtree_setprop_cells(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
                                0x0, 0x7);
-    pci_map_create(fdt, pci_map, qemu_devtree_get_phandle(fdt, mpic));
-    qemu_devtree_setprop(fdt, pci, "interrupt-map", pci_map, sizeof(pci_map));
+    pci_map = pci_map_create(fdt, qemu_devtree_get_phandle(fdt, mpic),
+                             0x11, 2, &len);
+    qemu_devtree_setprop(fdt, pci, "interrupt-map", pci_map, len);
     qemu_devtree_setprop_phandle(fdt, pci, "interrupt-parent", mpic);
     qemu_devtree_setprop_cells(fdt, pci, "interrupts", 24, 2);
     qemu_devtree_setprop_cells(fdt, pci, "bus-range", 0, 255);
@@ -364,6 +374,7 @@ done:
     ret = fdt_size;
 
 out:
+    g_free(pci_map);
 
     return ret;
 }
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/5] PPC: E500: Move PCI slot information into params

[Alexander Graf]

We have a params struct that allows us to expose differences between
e500 machine models. Include PCI slot information there, so we can have
different machines with different PCI slot topology.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc/e500.c      |    4 +++-
 hw/ppc/e500.h      |    2 ++
 hw/ppc/e500plat.c  |    2 ++
 hw/ppc/mpc8544ds.c |    2 ++
 4 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ebb6d96..564f654 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -339,7 +339,8 @@ static int ppce500_load_device_tree(CPUPPCState *env,
     qemu_devtree_setprop_cells(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
                                0x0, 0x7);
     pci_map = pci_map_create(fdt, qemu_devtree_get_phandle(fdt, mpic),
-                             0x11, 2, &len);
+                             params->pci_first_slot, params->pci_nr_slots,
+                             &len);
     qemu_devtree_setprop(fdt, pci, "interrupt-map", pci_map, len);
     qemu_devtree_setprop_phandle(fdt, pci, "interrupt-parent", mpic);
     qemu_devtree_setprop_cells(fdt, pci, "interrupts", 24, 2);
@@ -569,6 +570,7 @@ void ppce500_init(PPCE500Params *params)
 
     /* PCI */
     dev = qdev_create(NULL, "e500-pcihost");
+    qdev_prop_set_uint32(dev, "first_slot", params->pci_first_slot);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 7ae87f4..f5ff273 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -9,6 +9,8 @@ typedef struct PPCE500Params {
     const char *kernel_cmdline;
     const char *initrd_filename;
     const char *cpu_model;
+    int pci_first_slot;
+    int pci_nr_slots;
 
     /* e500-specific params */
 
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 4cfb940..9365213 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -40,6 +40,8 @@ static void e500plat_init(QEMUMachineInitArgs *args)
         .kernel_cmdline = kernel_cmdline,
         .initrd_filename = initrd_filename,
         .cpu_model = cpu_model,
+        .pci_first_slot = 0x11,
+        .pci_nr_slots = 2,
         .fixup_devtree = e500plat_fixup_devtree,
     };
 
diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
index e651661..7e1761d 100644
--- a/hw/ppc/mpc8544ds.c
+++ b/hw/ppc/mpc8544ds.c
@@ -40,6 +40,8 @@ static void mpc8544ds_init(QEMUMachineInitArgs *args)
         .kernel_cmdline = kernel_cmdline,
         .initrd_filename = initrd_filename,
         .cpu_model = cpu_model,
+        .pci_first_slot = 0x11,
+        .pci_nr_slots = 2,
         .fixup_devtree = mpc8544ds_fixup_devtree,
     };
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/5] PPC: E500plat: Make a lot of PCI slots available

[Alexander Graf]

The ppce500 machine doesn't have to stick to hardware limitations,
as it's defined as being fully device tree based.

Thus we can change the initial PCI slot ID to 0x1 which gives us a
whopping 31 PCI devices we can support with this machine now!

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc/e500plat.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 9365213..2992bd9 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -14,6 +14,7 @@
 #include "e500.h"
 #include "../boards.h"
 #include "device_tree.h"
+#include "hw/pci.h"
 
 static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
 {
@@ -40,8 +41,8 @@ static void e500plat_init(QEMUMachineInitArgs *args)
         .kernel_cmdline = kernel_cmdline,
         .initrd_filename = initrd_filename,
         .cpu_model = cpu_model,
-        .pci_first_slot = 0x11,
-        .pci_nr_slots = 2,
+        .pci_first_slot = 0x1,
+        .pci_nr_slots = PCI_SLOT_MAX - 1,
         .fixup_devtree = e500plat_fixup_devtree,
     };
 
-- 
1.6.0.2

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Scott Wood]

On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
> Today we're hardcoding the PCI interrupt map in the e500 machine file.
> Instead, let's write it dynamically so that different machine types
> can have different slot properties.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ppc/e500.c |   51  
> +++++++++++++++++++++++++++++++--------------------
>  1 files changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 1034f93..ebb6d96 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -66,25 +66,33 @@ struct boot_info
>      uint32_t entry;
>  };
> 
> -static void pci_map_create(void *fdt, uint32_t *pci_map, uint32_t  
> mpic)
> +static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int  
> first_slot,
> +                                int nr_slots, int *len)
>  {
> -    int i;
> -    const uint32_t tmp[] = {
> -                             /* IDSEL 0x11 J17 Slot 1 */
> -                             0x8800, 0x0, 0x0, 0x1, mpic, 0x2, 0x1,
> -                             0x8800, 0x0, 0x0, 0x2, mpic, 0x3, 0x1,
> -                             0x8800, 0x0, 0x0, 0x3, mpic, 0x4, 0x1,
> -                             0x8800, 0x0, 0x0, 0x4, mpic, 0x1, 0x1,
> -
> -                             /* IDSEL 0x12 J16 Slot 2 */
> -                             0x9000, 0x0, 0x0, 0x1, mpic, 0x3, 0x1,
> -                             0x9000, 0x0, 0x0, 0x2, mpic, 0x4, 0x1,
> -                             0x9000, 0x0, 0x0, 0x3, mpic, 0x2, 0x1,
> -                             0x9000, 0x0, 0x0, 0x4, mpic, 0x1, 0x1,
> -                           };
> -    for (i = 0; i < (7 * 8); i++) {
> -        pci_map[i] = cpu_to_be32(tmp[i]);
> +    int i = 0;
> +    int slot;
> +    int pci_irq;
> +    int last_slot = first_slot + nr_slots;
> +    uint32_t *pci_map;
> +
> +    *len = nr_slots * 4 * 7 * sizeof(uint32_t);
> +    pci_map = g_malloc(*len);
> +
> +    for (slot = first_slot; slot < last_slot; slot++) {
> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
> +            pci_map[i++] = cpu_to_be32(slot << 11);
> +            pci_map[i++] = cpu_to_be32(0x0);
> +            pci_map[i++] = cpu_to_be32(0x0);
> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
> +            pci_map[i++] = cpu_to_be32(mpic);
> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
> +            pci_map[i++] = cpu_to_be32(0x1);
> +        }
>      }

It would be nice if the slot-to-IRQ calculation were done in only one  
place rather than duplicated here.

-Scott

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Alexander Graf]

On 12.12.2012, at 19:40, Scott Wood wrote:

> On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
>> Today we're hardcoding the PCI interrupt map in the e500 machine file.
>> Instead, let's write it dynamically so that different machine types
>> can have different slot properties.
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ppc/e500.c |   51 +++++++++++++++++++++++++++++++--------------------
>> 1 files changed, 31 insertions(+), 20 deletions(-)
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 1034f93..ebb6d96 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -66,25 +66,33 @@ struct boot_info
>>     uint32_t entry;
>> };
>> -static void pci_map_create(void *fdt, uint32_t *pci_map, uint32_t mpic)
>> +static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>> +                                int nr_slots, int *len)
>> {
>> -    int i;
>> -    const uint32_t tmp[] = {
>> -                             /* IDSEL 0x11 J17 Slot 1 */
>> -                             0x8800, 0x0, 0x0, 0x1, mpic, 0x2, 0x1,
>> -                             0x8800, 0x0, 0x0, 0x2, mpic, 0x3, 0x1,
>> -                             0x8800, 0x0, 0x0, 0x3, mpic, 0x4, 0x1,
>> -                             0x8800, 0x0, 0x0, 0x4, mpic, 0x1, 0x1,
>> -
>> -                             /* IDSEL 0x12 J16 Slot 2 */
>> -                             0x9000, 0x0, 0x0, 0x1, mpic, 0x3, 0x1,
>> -                             0x9000, 0x0, 0x0, 0x2, mpic, 0x4, 0x1,
>> -                             0x9000, 0x0, 0x0, 0x3, mpic, 0x2, 0x1,
>> -                             0x9000, 0x0, 0x0, 0x4, mpic, 0x1, 0x1,
>> -                           };
>> -    for (i = 0; i < (7 * 8); i++) {
>> -        pci_map[i] = cpu_to_be32(tmp[i]);
>> +    int i = 0;
>> +    int slot;
>> +    int pci_irq;
>> +    int last_slot = first_slot + nr_slots;
>> +    uint32_t *pci_map;
>> +
>> +    *len = nr_slots * 4 * 7 * sizeof(uint32_t);
>> +    pci_map = g_malloc(*len);
>> +
>> +    for (slot = first_slot; slot < last_slot; slot++) {
>> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
>> +            pci_map[i++] = cpu_to_be32(slot << 11);
>> +            pci_map[i++] = cpu_to_be32(0x0);
>> +            pci_map[i++] = cpu_to_be32(0x0);
>> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
>> +            pci_map[i++] = cpu_to_be32(mpic);
>> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
>> +            pci_map[i++] = cpu_to_be32(0x1);
>> +        }
>>     }
> 
> It would be nice if the slot-to-IRQ calculation were done in only one place rather than duplicated here.

Sure, what exactly would you suggest to do? :)

We can move the whole function to ppce500_pci.c. We could export the function(slot, pci_irq) through the header of ppce500_pci.c. We could also try and traverse the pci bus to find the function that is actually called to convert irq numbers internally, so we potentially support other pci host controllers.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Scott Wood]

On 12/12/2012 05:38:32 PM, Alexander Graf wrote:
> 
> On 12.12.2012, at 19:40, Scott Wood wrote:
> 
> > On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
> >> +    for (slot = first_slot; slot < last_slot; slot++) {
> >> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
> >> +            pci_map[i++] = cpu_to_be32(slot << 11);
> >> +            pci_map[i++] = cpu_to_be32(0x0);
> >> +            pci_map[i++] = cpu_to_be32(0x0);
> >> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
> >> +            pci_map[i++] = cpu_to_be32(mpic);
> >> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) +  
> 1);
> >> +            pci_map[i++] = cpu_to_be32(0x1);
> >> +        }
> >>     }
> >
> > It would be nice if the slot-to-IRQ calculation were done in only  
> one place rather than duplicated here.
> 
> Sure, what exactly would you suggest to do? :)

Have a common function to calculate the IRQ given the slot number, and  
call that both from here and from mpc85xx_pci_map_irq().

> We can move the whole function to ppce500_pci.c.
> We could export the function(slot, pci_irq) through the header of  
> ppce500_pci.c.

Either works, though I'd lean towards moving this function into  
ppce500_pci.c.

> We could also try and traverse the pci bus to find the function that  
> is actually called to convert irq numbers internally, so we  
> potentially support other pci host controllers.

Not sure what you mean here.

-Scott

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Alexander Graf]

On 13.12.2012, at 00:43, Scott Wood wrote:

> On 12/12/2012 05:38:32 PM, Alexander Graf wrote:
>> On 12.12.2012, at 19:40, Scott Wood wrote:
>> > On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
>> >> +    for (slot = first_slot; slot < last_slot; slot++) {
>> >> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
>> >> +            pci_map[i++] = cpu_to_be32(slot << 11);
>> >> +            pci_map[i++] = cpu_to_be32(0x0);
>> >> +            pci_map[i++] = cpu_to_be32(0x0);
>> >> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
>> >> +            pci_map[i++] = cpu_to_be32(mpic);
>> >> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
>> >> +            pci_map[i++] = cpu_to_be32(0x1);
>> >> +        }
>> >>     }
>> >
>> > It would be nice if the slot-to-IRQ calculation were done in only one place rather than duplicated here.
>> Sure, what exactly would you suggest to do? :)
> 
> Have a common function to calculate the IRQ given the slot number, and call that both from here and from mpc85xx_pci_map_irq().
> 
>> We can move the whole function to ppce500_pci.c.
>> We could export the function(slot, pci_irq) through the header of ppce500_pci.c.
> 
> Either works, though I'd lean towards moving this function into ppce500_pci.c.

Well, I'm not sure Anthony would be happy about that. He wanted to keep device tree generation inside the machine files. But this one might be an exception, because it's not a generic device.

> 
>> We could also try and traverse the pci bus to find the function that is actually called to convert irq numbers internally, so we potentially support other pci host controllers.
> 
> Not sure what you mean here.

We could call bus->map_irq(...) with an artificially created PCIDevice struct ;). But that's pretty hacky.

So you're indicating you'd like the below patch? Do you think it's worth the additional code for a simple + and & 3?


Alex

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.cindex 564f654..af6b671 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -34,6 +34,7 @@
 #include "hw/sysbus.h"
 #include "exec-memory.h"
 #include "host-utils.h"
+#include "hw/ppce500_pci.h"
 
 #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
 #define UIMAGE_LOAD_BASE           0
@@ -72,6 +73,7 @@ static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
     int i = 0;
     int slot;
     int pci_irq;
+    int host_irq;
     int last_slot = first_slot + nr_slots;
     uint32_t *pci_map;
 
@@ -85,7 +87,8 @@ static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
             pci_map[i++] = cpu_to_be32(0x0);
             pci_map[i++] = cpu_to_be32(pci_irq + 1);
             pci_map[i++] = cpu_to_be32(mpic);
-            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
+            host_irq = ppce500_pci_map_irq_slot(slot, pci_irq);
+            pci_map[i++] = cpu_to_be32(host_irq + 1);
             pci_map[i++] = cpu_to_be32(0x1);
         }
     }
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 561a776..09e3507 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -19,6 +19,7 @@
 #include "pci.h"
 #include "pci_host.h"
 #include "bswap.h"
+#include "ppce500_pci.h"
 
 #ifdef DEBUG_PCI
 #define pci_debug(fmt, ...) fprintf(stderr, fmt, ## __VA_ARGS__)
@@ -256,7 +257,7 @@ static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
     int devno = pci_dev->devfn >> 3;
     int ret;
 
-    ret = (irq_num + devno) % 4;
+    ret = ppce500_pci_map_irq_slot(devno, irq_num);
 
     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
            pci_dev->devfn, irq_num, ret, devno);
agraf@lychee:/home/agraf/release/qemu> git add hw/ppce500_pci.h
agraf@lychee:/home/agraf/release/qemu> git diff HEAD
agraf@lychee:/home/agraf/release/qemu> git diff HEAD | cat
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 564f654..af6b671 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -34,6 +34,7 @@
 #include "hw/sysbus.h"
 #include "exec-memory.h"
 #include "host-utils.h"
+#include "hw/ppce500_pci.h"
 
 #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
 #define UIMAGE_LOAD_BASE           0
@@ -72,6 +73,7 @@ static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
     int i = 0;
     int slot;
     int pci_irq;
+    int host_irq;
     int last_slot = first_slot + nr_slots;
     uint32_t *pci_map;
 
@@ -85,7 +87,8 @@ static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
             pci_map[i++] = cpu_to_be32(0x0);
             pci_map[i++] = cpu_to_be32(pci_irq + 1);
             pci_map[i++] = cpu_to_be32(mpic);
-            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
+            host_irq = ppce500_pci_map_irq_slot(slot, pci_irq);
+            pci_map[i++] = cpu_to_be32(host_irq + 1);
             pci_map[i++] = cpu_to_be32(0x1);
         }
     }
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 561a776..09e3507 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -19,6 +19,7 @@
 #include "pci.h"
 #include "pci_host.h"
 #include "bswap.h"
+#include "ppce500_pci.h"
 
 #ifdef DEBUG_PCI
 #define pci_debug(fmt, ...) fprintf(stderr, fmt, ## __VA_ARGS__)
@@ -256,7 +257,7 @@ static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
     int devno = pci_dev->devfn >> 3;
     int ret;
 
-    ret = (irq_num + devno) % 4;
+    ret = ppce500_pci_map_irq_slot(devno, irq_num);
 
     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
            pci_dev->devfn, irq_num, ret, devno);
diff --git a/hw/ppce500_pci.h b/hw/ppce500_pci.h
new file mode 100644
index 0000000..61f773e
--- /dev/null
+++ b/hw/ppce500_pci.h
@@ -0,0 +1,9 @@
+#ifndef PPCE500_PCI_H
+#define PPCE500_PCI_H
+
+static inline int ppce500_pci_map_irq_slot(int devno, int irq_num)
+{
+    return (devno + irq_num) % 4;
+}
+
+#endif

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Scott Wood]

On 12/12/2012 06:04:11 PM, Alexander Graf wrote:
> 
> On 13.12.2012, at 00:43, Scott Wood wrote:
> 
> > On 12/12/2012 05:38:32 PM, Alexander Graf wrote:
> >> On 12.12.2012, at 19:40, Scott Wood wrote:
> >> > On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
> >> >> +    for (slot = first_slot; slot < last_slot; slot++) {
> >> >> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
> >> >> +            pci_map[i++] = cpu_to_be32(slot << 11);
> >> >> +            pci_map[i++] = cpu_to_be32(0x0);
> >> >> +            pci_map[i++] = cpu_to_be32(0x0);
> >> >> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
> >> >> +            pci_map[i++] = cpu_to_be32(mpic);
> >> >> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4)  
> + 1);
> >> >> +            pci_map[i++] = cpu_to_be32(0x1);
> >> >> +        }
> >> >>     }
> >> >
> >> > It would be nice if the slot-to-IRQ calculation were done in  
> only one place rather than duplicated here.
> >> Sure, what exactly would you suggest to do? :)
> >
> > Have a common function to calculate the IRQ given the slot number,  
> and call that both from here and from mpc85xx_pci_map_irq().
> >
> >> We can move the whole function to ppce500_pci.c.
> >> We could export the function(slot, pci_irq) through the header of  
> ppce500_pci.c.
> >
> > Either works, though I'd lean towards moving this function into  
> ppce500_pci.c.
> 
> Well, I'm not sure Anthony would be happy about that. He wanted to  
> keep device tree generation inside the machine files.

Sigh.  I don't understand the hostility to device tree generation, to  
the point of enforcing unnatural code grouping and possibly even  
duplication.

> But this one might be an exception, because it's not a generic device.

So what happens when we do have a generic device?  Duplicate the code  
in every machine that uses it, or have a parallel "hw/device_dt.c" (or  
maybe some better hidden place) to factor out common code while (sort  
of) complying with Anthony's mandate? :-P

> >> We could also try and traverse the pci bus to find the function  
> that is actually called to convert irq numbers internally, so we  
> potentially support other pci host controllers.
> >
> > Not sure what you mean here.
> 
> We could call bus->map_irq(...) with an artificially created  
> PCIDevice struct ;). But that's pretty hacky.

If we do anything like that, it should probably be to iterate over the  
devices that actually exist and add interrupt-map entries only for  
those.

> So you're indicating you'd like the below patch?

I think you pasted a bit more than one patch, but yes.

> Do you think it's worth the additional code for a simple + and & 3?

It's not about duplicating "+ and & 3" so much as having only one place  
where the relationship is defined, in case someone wants to alter it  
(e.g. for adding some other board where the mapping is done  
differently).

> agraf@lychee:/home/agraf/release/qemu> git add hw/ppce500_pci.h
> agraf@lychee:/home/agraf/release/qemu> git diff HEAD
> agraf@lychee:/home/agraf/release/qemu> git diff HEAD | cat

What does piping through cat get you?

-Scott

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Alexander Graf]

On 13.12.2012, at 01:20, Scott Wood wrote:

> On 12/12/2012 06:04:11 PM, Alexander Graf wrote:
>> On 13.12.2012, at 00:43, Scott Wood wrote:
>> > On 12/12/2012 05:38:32 PM, Alexander Graf wrote:
>> >> On 12.12.2012, at 19:40, Scott Wood wrote:
>> >> > On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
>> >> >> +    for (slot = first_slot; slot < last_slot; slot++) {
>> >> >> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
>> >> >> +            pci_map[i++] = cpu_to_be32(slot << 11);
>> >> >> +            pci_map[i++] = cpu_to_be32(0x0);
>> >> >> +            pci_map[i++] = cpu_to_be32(0x0);
>> >> >> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
>> >> >> +            pci_map[i++] = cpu_to_be32(mpic);
>> >> >> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4) + 1);
>> >> >> +            pci_map[i++] = cpu_to_be32(0x1);
>> >> >> +        }
>> >> >>     }
>> >> >
>> >> > It would be nice if the slot-to-IRQ calculation were done in only one place rather than duplicated here.
>> >> Sure, what exactly would you suggest to do? :)
>> >
>> > Have a common function to calculate the IRQ given the slot number, and call that both from here and from mpc85xx_pci_map_irq().
>> >
>> >> We can move the whole function to ppce500_pci.c.
>> >> We could export the function(slot, pci_irq) through the header of ppce500_pci.c.
>> >
>> > Either works, though I'd lean towards moving this function into ppce500_pci.c.
>> Well, I'm not sure Anthony would be happy about that. He wanted to keep device tree generation inside the machine files.
> 
> Sigh.  I don't understand the hostility to device tree generation, to the point of enforcing unnatural code grouping and possibly even duplication.
> 
>> But this one might be an exception, because it's not a generic device.
> 
> So what happens when we do have a generic device?  Duplicate the code in every machine that uses it, or have a parallel "hw/device_dt.c" (or maybe some better hidden place) to factor out common code while (sort of) complying with Anthony's mandate? :-P

*shrug* I'd say we can try and find out when we hit something we really care about. In this case I doubt we do.

> 
>> >> We could also try and traverse the pci bus to find the function that is actually called to convert irq numbers internally, so we potentially support other pci host controllers.
>> >
>> > Not sure what you mean here.
>> We could call bus->map_irq(...) with an artificially created PCIDevice struct ;). But that's pretty hacky.
> 
> If we do anything like that, it should probably be to iterate over the devices that actually exist and add interrupt-map entries only for those.

Right. Though I'm not sure how pci hotplug slots would look like in that model. I don't think we have PCIDevice structs there yet, but we would still need to keep interrupt maps ready.

> 
>> So you're indicating you'd like the below patch?
> 
> I think you pasted a bit more than one patch, but yes.

Yikes. It's way past midnight after all :).

You mean I messed up and pasted more than I wanted or that I should split the patch? :)

> 
>> Do you think it's worth the additional code for a simple + and & 3?
> 
> It's not about duplicating "+ and & 3" so much as having only one place where the relationship is defined, in case someone wants to alter it (e.g. for adding some other board where the mapping is done differently).

I totally agree with you. I'm just saying that in this particular case the logic is easy enough that I don't necessarily see a big chance for a pitfall. But I'm not objecting to moving the logic to a single place - it certainly makes the relationship situation clearer.

> 
>> agraf@lychee:/home/agraf/release/qemu> git add hw/ppce500_pci.h
>> agraf@lychee:/home/agraf/release/qemu> git diff HEAD
>> agraf@lychee:/home/agraf/release/qemu> git diff HEAD | cat
> 
> What does piping through cat get you?

Piping through cat gets me that I don't get the patch in less, so I can easily copy&paste it from my terminal into the email client even though it's bigger than my terminal window. Also not using less means that formatting stays consistent.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Scott Wood]

On 12/12/2012 06:28:02 PM, Alexander Graf wrote:
> 
> On 13.12.2012, at 01:20, Scott Wood wrote:
> 
> > On 12/12/2012 06:04:11 PM, Alexander Graf wrote:
> >> We could call bus->map_irq(...) with an artificially created  
> PCIDevice struct ;). But that's pretty hacky.
> >
> > If we do anything like that, it should probably be to iterate over  
> the devices that actually exist and add interrupt-map entries only  
> for those.
> 
> Right. Though I'm not sure how pci hotplug slots would look like in  
> that model. I don't think we have PCIDevice structs there yet, but we  
> would still need to keep interrupt maps ready.

Note that if we did limit it to only devices that actually exist, we'd  
be producing a smaller interrupt-map than would be found on real  
hardware for targets like mpc8544ds, if the slots aren't fully  
populated.

> >> So you're indicating you'd like the below patch?
> >
> > I think you pasted a bit more than one patch, but yes.
> 
> Yikes. It's way past midnight after all :).
> 
> You mean I messed up and pasted more than I wanted or that I should  
> split the patch? :)

The former. :-)

> >> agraf@lychee:/home/agraf/release/qemu> git add hw/ppce500_pci.h
> >> agraf@lychee:/home/agraf/release/qemu> git diff HEAD
> >> agraf@lychee:/home/agraf/release/qemu> git diff HEAD | cat
> >
> > What does piping through cat get you?
> 
> Piping through cat gets me that I don't get the patch in less, so I  
> can easily copy&paste it from my terminal into the email client even  
> though it's bigger than my terminal window. Also not using less means  
> that formatting stays consistent.

Ah, forgot about git doing different things based on what stdout is.

-Scott

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Alexander Graf]

On 13.12.2012, at 01:33, Scott Wood wrote:

> On 12/12/2012 06:28:02 PM, Alexander Graf wrote:
>> On 13.12.2012, at 01:20, Scott Wood wrote:
>> > On 12/12/2012 06:04:11 PM, Alexander Graf wrote:
>> >> We could call bus->map_irq(...) with an artificially created PCIDevice struct ;). But that's pretty hacky.
>> >
>> > If we do anything like that, it should probably be to iterate over the devices that actually exist and add interrupt-map entries only for those.
>> Right. Though I'm not sure how pci hotplug slots would look like in that model. I don't think we have PCIDevice structs there yet, but we would still need to keep interrupt maps ready.
> 
> Note that if we did limit it to only devices that actually exist, we'd be producing a smaller interrupt-map than would be found on real hardware for targets like mpc8544ds, if the slots aren't fully populated.

Yeah, I think with the patch as is we're on the safe side. Better be consistent than too smart :).

> 
>> >> So you're indicating you'd like the below patch?
>> >
>> > I think you pasted a bit more than one patch, but yes.
>> Yikes. It's way past midnight after all :).
>> You mean I messed up and pasted more than I wanted or that I should split the patch? :)
> 
> The former. :-)

Ok :). So I'll include the patch in the series without resending the whole set - I don't want to spam the mailing list more than I need to :).


Alex