Patchwork [2/3] e500: Adding CCSR memory region

login
register
mail settings
Submitter Bharat Bhushan
Date Oct. 8, 2012, 4:46 p.m.
Message ID <1349714816-12618-3-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/190062/
State New
Headers show

Comments

Bharat Bhushan - Oct. 8, 2012, 4:46 p.m.
All devices are also placed under CCSR memory region.
The CCSR memory region is exported to pci device. The MSI interrupt
generation is the main reason to export the CCSR region to PCI device.
This put the requirement to move mpic under CCSR region, but logically
all devices should be under CCSR. So this patch places all emulated
devices under ccsr region.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 hw/ppc/e500.c |   61 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 38 insertions(+), 23 deletions(-)
Andreas Färber - Oct. 8, 2012, 5:21 p.m.
Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> All devices are also placed under CCSR memory region.
> The CCSR memory region is exported to pci device. The MSI interrupt
> generation is the main reason to export the CCSR region to PCI device.
> This put the requirement to move mpic under CCSR region, but logically
> all devices should be under CCSR. So this patch places all emulated
> devices under ccsr region.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  hw/ppc/e500.c |   61 +++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 1949c81..b3e6a1e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -46,14 +46,16 @@
>  /* TODO: parameterize */
>  #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> -#define MPC8544_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + 0x40000ULL)
> -#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4500ULL)
> -#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4600ULL)
> -#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + 0x8000ULL)
> +#define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
> +#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_PCI_REGS_OFFSET)
>  #define MPC8544_PCI_REGS_SIZE      0x1000ULL
>  #define MPC8544_PCI_IO             0xE1000000ULL
>  #define MPC8544_PCI_IOLEN          0x10000ULL
> -#define MPC8544_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 0xe0000ULL)
> +#define MPC8544_UTIL_OFFSET        0xe0000ULL
>  #define MPC8544_SPIN_BASE          0xEF000000ULL
>  
>  struct boot_info
> @@ -268,13 +270,12 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>      /* XXX should contain a reasonable value */
>      qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
>  
> -    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc,
> -             MPC8544_MPIC_REGS_BASE - MPC8544_CCSRBAR_BASE);
> +    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
>      qemu_devtree_add_subnode(fdt, mpic);
>      qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
>      qemu_devtree_setprop_string(fdt, mpic, "compatible", "chrp,open-pic");
> -    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_BASE -
> -                               MPC8544_CCSRBAR_BASE, 0x40000);
> +    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
> +                               0x40000);
>      qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
>      qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
>      mpic_ph = qemu_devtree_alloc_phandle(fdt);
> @@ -287,17 +288,16 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>       * device it finds in the dt as serial output device. And we generate
>       * devices in reverse order to the dt.
>       */
> -    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE,
> +    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
>                       soc, mpic, "serial1", 1, false);
> -    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE,
> +    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
>                       soc, mpic, "serial0", 0, true);
>  
>      snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
> -             MPC8544_UTIL_BASE - MPC8544_CCSRBAR_BASE);
> +             MPC8544_UTIL_OFFSET);
>      qemu_devtree_add_subnode(fdt, gutil);
>      qemu_devtree_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
> -    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_BASE -
> -                               MPC8544_CCSRBAR_BASE, 0x1000);
> +    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
>      qemu_devtree_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
>  
>      snprintf(pci, sizeof(pci), "/pci@%llx", MPC8544_PCI_REGS_BASE);
> @@ -423,6 +423,8 @@ void ppce500_init(PPCE500Params *params)
>      qemu_irq **irqs, *mpic;
>      DeviceState *dev;
>      CPUPPCState *firstenv = NULL;
> +    MemoryRegion *ccsr;
> +    SysBusDevice *s;
>  
>      /* Setup CPUs */
>      if (params->cpu_model == NULL) {
> @@ -451,7 +453,8 @@ void ppce500_init(PPCE500Params *params)
>          irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>          irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>          env->spr[SPR_BOOKE_PIR] = env->cpu_index = i;
> -        env->mpic_cpu_base = MPC8544_MPIC_REGS_BASE + 0x20000;
> +        env->mpic_cpu_base = MPC8544_CCSRBAR_BASE +
> +                              MPC8544_MPIC_REGS_OFFSET + 0x20000;
>  
>          ppc_booke_timers_init(env, 400000000, PPC_TIMER_E500);
>  
> @@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(address_space_mem, 0, ram);
>  
> +    ccsr = g_malloc0(sizeof(MemoryRegion));
> +    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
> +    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
> +
>      /* MPIC */
> -    mpic = mpic_init(address_space_mem, MPC8544_MPIC_REGS_BASE,
> +    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
>                       smp_cpus, irqs, NULL);
>  
>      if (!mpic) {
> @@ -488,25 +495,33 @@ void ppce500_init(PPCE500Params *params)
>  
>      /* Serial */
>      if (serial_hds[0]) {
> -        serial_mm_init(address_space_mem, MPC8544_SERIAL0_REGS_BASE,
> +        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
>                         0, mpic[12+26], 399193,
>                         serial_hds[0], DEVICE_BIG_ENDIAN);
>      }
>  
>      if (serial_hds[1]) {
> -        serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
> +        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
>                         0, mpic[12+26], 399193,
>                         serial_hds[1], DEVICE_BIG_ENDIAN);
>      }
>  
>      /* General Utility device */
> -    sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
> +    dev = qdev_create(NULL, "mpc8544-guts");
> +    qdev_init_nofail(dev);
> +    s = sysbus_from_qdev(dev);

s = SYS_BUS_DEVICE(dev);

> +    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET, s->mmio[0].memory);

Hmm ...

>  
>      /* PCI */
> -    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
> -                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
> -                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
> -                                NULL);
> +    dev = qdev_create(NULL, "e500-pcihost");
> +    qdev_init_nofail(dev);
> +    s = sysbus_from_qdev(dev);

(dito)

> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);

... I wonder if fiddling with SysBus MMIO is a good idea.
s->mmio[0].addr is not getting assigned this way, which is checked as
condition for deleting the subregion. But sysbus_mmio_map() only adds to
/ deletes from get_system_memory().
The alternative would be using a custom field rather than the
SysBus-internal one. Avi/Alex?

> +
>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>      if (!pci_bus)
>          printf("couldn't create PCI controller!\n");
> 

Andreas
Andreas Färber - Oct. 8, 2012, 5:26 p.m.
Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 1949c81..b3e6a1e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
[...]
> @@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(address_space_mem, 0, ram);
>  
> +    ccsr = g_malloc0(sizeof(MemoryRegion));
> +    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);

"e500-ccsr" :) Got fixed in 3/3 only.

Andreas
Alexander Graf - Oct. 8, 2012, 6:58 p.m.
On 08.10.2012, at 19:21, Andreas Färber wrote:

> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>> All devices are also placed under CCSR memory region.
>> The CCSR memory region is exported to pci device. The MSI interrupt
>> generation is the main reason to export the CCSR region to PCI device.
>> This put the requirement to move mpic under CCSR region, but logically
>> all devices should be under CCSR. So this patch places all emulated
>> devices under ccsr region.
>> 
>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>> ---
>> hw/ppc/e500.c |   61 +++++++++++++++++++++++++++++++++++---------------------
>> 1 files changed, 38 insertions(+), 23 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 1949c81..b3e6a1e 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -46,14 +46,16 @@
>> /* TODO: parameterize */
>> #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>> #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> -#define MPC8544_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + 0x40000ULL)
>> -#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4500ULL)
>> -#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4600ULL)
>> -#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + 0x8000ULL)
>> +#define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
>> +#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
>> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
>> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
>> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
>> +                                    MPC8544_PCI_REGS_OFFSET)
>> #define MPC8544_PCI_REGS_SIZE      0x1000ULL
>> #define MPC8544_PCI_IO             0xE1000000ULL
>> #define MPC8544_PCI_IOLEN          0x10000ULL
>> -#define MPC8544_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 0xe0000ULL)
>> +#define MPC8544_UTIL_OFFSET        0xe0000ULL
>> #define MPC8544_SPIN_BASE          0xEF000000ULL
>> 
>> struct boot_info
>> @@ -268,13 +270,12 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>>     /* XXX should contain a reasonable value */
>>     qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
>> 
>> -    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc,
>> -             MPC8544_MPIC_REGS_BASE - MPC8544_CCSRBAR_BASE);
>> +    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
>>     qemu_devtree_add_subnode(fdt, mpic);
>>     qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
>>     qemu_devtree_setprop_string(fdt, mpic, "compatible", "chrp,open-pic");
>> -    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_BASE -
>> -                               MPC8544_CCSRBAR_BASE, 0x40000);
>> +    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
>> +                               0x40000);
>>     qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
>>     qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
>>     mpic_ph = qemu_devtree_alloc_phandle(fdt);
>> @@ -287,17 +288,16 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>>      * device it finds in the dt as serial output device. And we generate
>>      * devices in reverse order to the dt.
>>      */
>> -    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE,
>> +    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
>>                      soc, mpic, "serial1", 1, false);
>> -    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE,
>> +    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
>>                      soc, mpic, "serial0", 0, true);
>> 
>>     snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
>> -             MPC8544_UTIL_BASE - MPC8544_CCSRBAR_BASE);
>> +             MPC8544_UTIL_OFFSET);
>>     qemu_devtree_add_subnode(fdt, gutil);
>>     qemu_devtree_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
>> -    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_BASE -
>> -                               MPC8544_CCSRBAR_BASE, 0x1000);
>> +    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
>>     qemu_devtree_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
>> 
>>     snprintf(pci, sizeof(pci), "/pci@%llx", MPC8544_PCI_REGS_BASE);
>> @@ -423,6 +423,8 @@ void ppce500_init(PPCE500Params *params)
>>     qemu_irq **irqs, *mpic;
>>     DeviceState *dev;
>>     CPUPPCState *firstenv = NULL;
>> +    MemoryRegion *ccsr;
>> +    SysBusDevice *s;
>> 
>>     /* Setup CPUs */
>>     if (params->cpu_model == NULL) {
>> @@ -451,7 +453,8 @@ void ppce500_init(PPCE500Params *params)
>>         irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>>         irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>>         env->spr[SPR_BOOKE_PIR] = env->cpu_index = i;
>> -        env->mpic_cpu_base = MPC8544_MPIC_REGS_BASE + 0x20000;
>> +        env->mpic_cpu_base = MPC8544_CCSRBAR_BASE +
>> +                              MPC8544_MPIC_REGS_OFFSET + 0x20000;
>> 
>>         ppc_booke_timers_init(env, 400000000, PPC_TIMER_E500);
>> 
>> @@ -478,8 +481,12 @@ void ppce500_init(PPCE500Params *params)
>>     vmstate_register_ram_global(ram);
>>     memory_region_add_subregion(address_space_mem, 0, ram);
>> 
>> +    ccsr = g_malloc0(sizeof(MemoryRegion));
>> +    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
>> +    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
>> +
>>     /* MPIC */
>> -    mpic = mpic_init(address_space_mem, MPC8544_MPIC_REGS_BASE,
>> +    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
>>                      smp_cpus, irqs, NULL);
>> 
>>     if (!mpic) {
>> @@ -488,25 +495,33 @@ void ppce500_init(PPCE500Params *params)
>> 
>>     /* Serial */
>>     if (serial_hds[0]) {
>> -        serial_mm_init(address_space_mem, MPC8544_SERIAL0_REGS_BASE,
>> +        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
>>                        0, mpic[12+26], 399193,
>>                        serial_hds[0], DEVICE_BIG_ENDIAN);
>>     }
>> 
>>     if (serial_hds[1]) {
>> -        serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
>> +        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
>>                        0, mpic[12+26], 399193,
>>                        serial_hds[1], DEVICE_BIG_ENDIAN);
>>     }
>> 
>>     /* General Utility device */
>> -    sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
>> +    dev = qdev_create(NULL, "mpc8544-guts");
>> +    qdev_init_nofail(dev);
>> +    s = sysbus_from_qdev(dev);
> 
> s = SYS_BUS_DEVICE(dev);

Mind to explain why? The rest of the code uses the above helper :).

> 
>> +    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET, s->mmio[0].memory);
> 
> Hmm ...
> 
>> 
>>     /* PCI */
>> -    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
>> -                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
>> -                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
>> -                                NULL);
>> +    dev = qdev_create(NULL, "e500-pcihost");
>> +    qdev_init_nofail(dev);
>> +    s = sysbus_from_qdev(dev);
> 
> (dito)
> 
>> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
>> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
>> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
>> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
> 
> ... I wonder if fiddling with SysBus MMIO is a good idea.
> s->mmio[0].addr is not getting assigned this way, which is checked as
> condition for deleting the subregion. But sysbus_mmio_map() only adds to
> / deletes from get_system_memory().
> The alternative would be using a custom field rather than the
> SysBus-internal one. Avi/Alex?

I honestly don't mind either way. We could

  a) add a new sysbus helper for maps in memory region containers
  b) not use sysbus at all
  c) do it this way

Anything else wouldn't improve the situation. I really don't care which way we go. To me, c) is fine.


Alex
Avi Kivity - Oct. 9, 2012, 9:04 a.m.
On 10/08/2012 07:21 PM, Andreas Färber wrote:
> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>> All devices are also placed under CCSR memory region.
>> The CCSR memory region is exported to pci device. The MSI interrupt
>> generation is the main reason to export the CCSR region to PCI device.
>> This put the requirement to move mpic under CCSR region, but logically
>> all devices should be under CCSR. So this patch places all emulated
>> devices under ccsr region.
>> 
>> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
>> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
>> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
>> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
> 
> ... I wonder if fiddling with SysBus MMIO is a good idea.
> s->mmio[0].addr is not getting assigned this way, which is checked as
> condition for deleting the subregion. But sysbus_mmio_map() only adds to
> / deletes from get_system_memory().
> The alternative would be using a custom field rather than the
> SysBus-internal one. Avi/Alex?

IMO yes.  Or not use sysbus at all.
Bharat Bhushan - Oct. 9, 2012, 4:45 p.m.
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Tuesday, October 09, 2012 2:35 PM
> To: Andreas Färber
> Cc: Bhushan Bharat-R65777; agraf@suse.de; qemu-ppc@nongnu.org; qemu-
> devel@nongnu.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> 
> On 10/08/2012 07:21 PM, Andreas Färber wrote:
> > Am 08.10.2012 18:46, schrieb Bharat Bhushan:
> >> All devices are also placed under CCSR memory region.
> >> The CCSR memory region is exported to pci device. The MSI interrupt
> >> generation is the main reason to export the CCSR region to PCI device.
> >> This put the requirement to move mpic under CCSR region, but
> >> logically all devices should be under CCSR. So this patch places all
> >> emulated devices under ccsr region.
> >>
> >> +    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> >> +    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
> >> +    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
> >> +    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
> >> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET,
> >> + s->mmio[0].memory);
> >
> > ... I wonder if fiddling with SysBus MMIO is a good idea.
> > s->mmio[0].addr is not getting assigned this way, which is checked as
> > condition for deleting the subregion. But sysbus_mmio_map() only adds
> > to / deletes from get_system_memory().
> > The alternative would be using a custom field rather than the
> > SysBus-internal one. Avi/Alex?
> 
> IMO yes.  Or not use sysbus at all.

What about adding a API:
void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
                           MemoryRegion *mr)
{
    assert(n >= 0 && n < dev->num_mmio);

    if (dev->mmio[n].addr == addr) {
        /* ??? region already mapped here.  */
        return;
    }
    if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
        /* Unregister previous mapping.  */
        memory_region_del_subregion(mr, dev->mmio[n].memory);
    }
    dev->mmio[n].addr = addr;
    memory_region_add_subregion(mr, addr, dev->mmio[n].memory);
}

Thanks
-Bharat

> 
> 
> --
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 9, 2012, 4:53 p.m.
On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
> 
> What about adding a API:
> void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
>                            MemoryRegion *mr)
> {
>     assert(n >= 0 && n < dev->num_mmio);
> 
>     if (dev->mmio[n].addr == addr) {
>         /* ??? region already mapped here.  */
>         return;
>     }
>     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
>         /* Unregister previous mapping.  */
>         memory_region_del_subregion(mr, dev->mmio[n].memory);
>     }
>     dev->mmio[n].addr = addr;
>     memory_region_add_subregion(mr, addr, dev->mmio[n].memory);
> }
> 

I think you can just use sysbus_mmio_get_region().  There are plenty of
other users, so there's precedent.
Bharat Bhushan - Oct. 9, 2012, 4:57 p.m.
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Tuesday, October 09, 2012 10:24 PM
> To: Bhushan Bharat-R65777
> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> 
> On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
> >
> > What about adding a API:
> > void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
> >                            MemoryRegion *mr) {
> >     assert(n >= 0 && n < dev->num_mmio);
> >
> >     if (dev->mmio[n].addr == addr) {
> >         /* ??? region already mapped here.  */
> >         return;
> >     }
> >     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
> >         /* Unregister previous mapping.  */
> >         memory_region_del_subregion(mr, dev->mmio[n].memory);
> >     }
> >     dev->mmio[n].addr = addr;
> >     memory_region_add_subregion(mr, addr, dev->mmio[n].memory); }
> >
> 
> I think you can just use sysbus_mmio_get_region().  There are plenty of other
> users, so there's precedent.

You mean something like this : memory_region_add_subregion(mr, addr, sysbus_mmio_get_region(dev, 0);

Ok, but this will still not resolve the issue of not setting the "dev->mmio[n].addr", no ?

Thanks
-Bharat

> 
> 
> --
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 9, 2012, 5:01 p.m.
On 10/09/2012 06:57 PM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Tuesday, October 09, 2012 10:24 PM
>> To: Bhushan Bharat-R65777
>> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
>> 
>> On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
>> >
>> > What about adding a API:
>> > void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t addr,
>> >                            MemoryRegion *mr) {
>> >     assert(n >= 0 && n < dev->num_mmio);
>> >
>> >     if (dev->mmio[n].addr == addr) {
>> >         /* ??? region already mapped here.  */
>> >         return;
>> >     }
>> >     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
>> >         /* Unregister previous mapping.  */
>> >         memory_region_del_subregion(mr, dev->mmio[n].memory);
>> >     }
>> >     dev->mmio[n].addr = addr;
>> >     memory_region_add_subregion(mr, addr, dev->mmio[n].memory); }
>> >
>> 
>> I think you can just use sysbus_mmio_get_region().  There are plenty of other
>> users, so there's precedent.
> 
> You mean something like this : memory_region_add_subregion(mr, addr, sysbus_mmio_get_region(dev, 0);
> 
> Ok, but this will still not resolve the issue of not setting the "dev->mmio[n].addr", no ?

Correct.  But there are 20 uses already, so it can't matter much.  If
someone wants to fix them, they can write a new API and do a sweep.

But really, sysbus just needs to go away.  It's pointless to give it
more and more features.
Bharat Bhushan - Oct. 9, 2012, 5:05 p.m.
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Tuesday, October 09, 2012 10:31 PM
> To: Bhushan Bharat-R65777
> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> 
> On 10/09/2012 06:57 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Avi Kivity [mailto:avi@redhat.com]
> >> Sent: Tuesday, October 09, 2012 10:24 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: Andreas Färber; agraf@suse.de; qemu-ppc@nongnu.org;
> >> qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 2/3] e500: Adding CCSR memory region
> >>
> >> On 10/09/2012 06:45 PM, Bhushan Bharat-R65777 wrote:
> >> >
> >> > What about adding a API:
> >> > void sysbus_mmio_map_to_mr(SysBusDevice *dev, int n, target_phys_addr_t
> addr,
> >> >                            MemoryRegion *mr) {
> >> >     assert(n >= 0 && n < dev->num_mmio);
> >> >
> >> >     if (dev->mmio[n].addr == addr) {
> >> >         /* ??? region already mapped here.  */
> >> >         return;
> >> >     }
> >> >     if (dev->mmio[n].addr != (target_phys_addr_t)-1) {
> >> >         /* Unregister previous mapping.  */
> >> >         memory_region_del_subregion(mr, dev->mmio[n].memory);
> >> >     }
> >> >     dev->mmio[n].addr = addr;
> >> >     memory_region_add_subregion(mr, addr, dev->mmio[n].memory); }
> >> >
> >>
> >> I think you can just use sysbus_mmio_get_region().  There are plenty
> >> of other users, so there's precedent.
> >
> > You mean something like this : memory_region_add_subregion(mr, addr,
> > sysbus_mmio_get_region(dev, 0);
> >
> > Ok, but this will still not resolve the issue of not setting the "dev-
> >mmio[n].addr", no ?
> 
> Correct.  But there are 20 uses already, so it can't matter much.  If someone
> wants to fix them, they can write a new API and do a sweep.
> 
> But really, sysbus just needs to go away.  It's pointless to give it more and
> more features.

Ok

Thanks
-Bharat

> 
> 
> --
> error compiling committee.c: too many arguments to function
Andreas Färber - Oct. 9, 2012, 5:23 p.m.
Am 08.10.2012 20:58, schrieb Alexander Graf:
> 
> On 08.10.2012, at 19:21, Andreas Färber wrote:
> 
>> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>>> +    s = sysbus_from_qdev(dev);
>>
>> s = SYS_BUS_DEVICE(dev);
> 
> Mind to explain why? The rest of the code uses the above helper :).

SYS_BUS_DEVICE() is the QOM cast macro, relying on binary struct layout.

sysbus_from_qdev() is an old qdev helper, relying on container_of() and
field naming.

I'm lobbying for using the new constructs in new code. You can argue of
course that SysBus will go away some day so it doesn't matter much here.
But we had issues with PCIHostState not adhering to QOM layout
requirements (e.g., pSeries), so using the cast consistently helps to
avoid/detect such issues.

>>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
>>
>> ... I wonder if fiddling with SysBus MMIO is a good idea.
>> s->mmio[0].addr is not getting assigned this way, which is checked as
>> condition for deleting the subregion. But sysbus_mmio_map() only adds to
>> / deletes from get_system_memory().
>> The alternative would be using a custom field rather than the
>> SysBus-internal one. Avi/Alex?
> 
> I honestly don't mind either way. We could
> 
>   a) add a new sysbus helper for maps in memory region containers
>   b) not use sysbus at all
>   c) do it this way
> 
> Anything else wouldn't improve the situation. I really don't care which way we go. To me, c) is fine.

b) is not that easily possible for the PHB, since we still haven't seen
a new round of i440fx patches (PCIHostState is based on SysBusDevice, so
all PHBs would need to get converted at once).

I'd also be in favor of c) with Avi's suggestion of using the accessor
function to obtain the MemoryRegion* (data caging).

Andreas

Patch

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 1949c81..b3e6a1e 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -46,14 +46,16 @@ 
 /* TODO: parameterize */
 #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
 #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
-#define MPC8544_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + 0x40000ULL)
-#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4500ULL)
-#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + 0x4600ULL)
-#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + 0x8000ULL)
+#define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
+#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
+#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
+#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
+#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
+                                    MPC8544_PCI_REGS_OFFSET)
 #define MPC8544_PCI_REGS_SIZE      0x1000ULL
 #define MPC8544_PCI_IO             0xE1000000ULL
 #define MPC8544_PCI_IOLEN          0x10000ULL
-#define MPC8544_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 0xe0000ULL)
+#define MPC8544_UTIL_OFFSET        0xe0000ULL
 #define MPC8544_SPIN_BASE          0xEF000000ULL
 
 struct boot_info
@@ -268,13 +270,12 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     /* XXX should contain a reasonable value */
     qemu_devtree_setprop_cell(fdt, soc, "bus-frequency", 0);
 
-    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc,
-             MPC8544_MPIC_REGS_BASE - MPC8544_CCSRBAR_BASE);
+    snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
     qemu_devtree_add_subnode(fdt, mpic);
     qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
     qemu_devtree_setprop_string(fdt, mpic, "compatible", "chrp,open-pic");
-    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_BASE -
-                               MPC8544_CCSRBAR_BASE, 0x40000);
+    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
+                               0x40000);
     qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
     qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
     mpic_ph = qemu_devtree_alloc_phandle(fdt);
@@ -287,17 +288,16 @@  static int ppce500_load_device_tree(CPUPPCState *env,
      * device it finds in the dt as serial output device. And we generate
      * devices in reverse order to the dt.
      */
-    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_BASE - MPC8544_CCSRBAR_BASE,
+    dt_serial_create(fdt, MPC8544_SERIAL1_REGS_OFFSET,
                      soc, mpic, "serial1", 1, false);
-    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_BASE - MPC8544_CCSRBAR_BASE,
+    dt_serial_create(fdt, MPC8544_SERIAL0_REGS_OFFSET,
                      soc, mpic, "serial0", 0, true);
 
     snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,
-             MPC8544_UTIL_BASE - MPC8544_CCSRBAR_BASE);
+             MPC8544_UTIL_OFFSET);
     qemu_devtree_add_subnode(fdt, gutil);
     qemu_devtree_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
-    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_BASE -
-                               MPC8544_CCSRBAR_BASE, 0x1000);
+    qemu_devtree_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
     qemu_devtree_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
 
     snprintf(pci, sizeof(pci), "/pci@%llx", MPC8544_PCI_REGS_BASE);
@@ -423,6 +423,8 @@  void ppce500_init(PPCE500Params *params)
     qemu_irq **irqs, *mpic;
     DeviceState *dev;
     CPUPPCState *firstenv = NULL;
+    MemoryRegion *ccsr;
+    SysBusDevice *s;
 
     /* Setup CPUs */
     if (params->cpu_model == NULL) {
@@ -451,7 +453,8 @@  void ppce500_init(PPCE500Params *params)
         irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
         irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
         env->spr[SPR_BOOKE_PIR] = env->cpu_index = i;
-        env->mpic_cpu_base = MPC8544_MPIC_REGS_BASE + 0x20000;
+        env->mpic_cpu_base = MPC8544_CCSRBAR_BASE +
+                              MPC8544_MPIC_REGS_OFFSET + 0x20000;
 
         ppc_booke_timers_init(env, 400000000, PPC_TIMER_E500);
 
@@ -478,8 +481,12 @@  void ppce500_init(PPCE500Params *params)
     vmstate_register_ram_global(ram);
     memory_region_add_subregion(address_space_mem, 0, ram);
 
+    ccsr = g_malloc0(sizeof(MemoryRegion));
+    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
+    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, ccsr);
+
     /* MPIC */
-    mpic = mpic_init(address_space_mem, MPC8544_MPIC_REGS_BASE,
+    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
                      smp_cpus, irqs, NULL);
 
     if (!mpic) {
@@ -488,25 +495,33 @@  void ppce500_init(PPCE500Params *params)
 
     /* Serial */
     if (serial_hds[0]) {
-        serial_mm_init(address_space_mem, MPC8544_SERIAL0_REGS_BASE,
+        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
                        0, mpic[12+26], 399193,
                        serial_hds[0], DEVICE_BIG_ENDIAN);
     }
 
     if (serial_hds[1]) {
-        serial_mm_init(address_space_mem, MPC8544_SERIAL1_REGS_BASE,
+        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
                        0, mpic[12+26], 399193,
                        serial_hds[1], DEVICE_BIG_ENDIAN);
     }
 
     /* General Utility device */
-    sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
+    dev = qdev_create(NULL, "mpc8544-guts");
+    qdev_init_nofail(dev);
+    s = sysbus_from_qdev(dev);
+    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET, s->mmio[0].memory);
 
     /* PCI */
-    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
-                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
-                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
-                                NULL);
+    dev = qdev_create(NULL, "e500-pcihost");
+    qdev_init_nofail(dev);
+    s = sysbus_from_qdev(dev);
+    sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
+    sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
+    sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
+    sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
+    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, s->mmio[0].memory);
+
     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
     if (!pci_bus)
         printf("couldn't create PCI controller!\n");