Re: [Qemu-devel] [PATCH 1/2] e500: Adding CCSR memory region

[Alexander Graf]

On 03.10.2012, at 13:49, Bharat Bhushan wrote:

> 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 <address@hidden>
> ---
> hw/ppc/e500.c |   51 +++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 5bab340..197411d 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -46,14 +46,23 @@
> /* 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_MPIC_REGS_BASE     (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_MPIC_REGS_OFFSET)
> +#define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
> +#define MPC8544_SERIAL0_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_SERIAL0_REGS_OFFSET)
> +#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
> +#define MPC8544_SERIAL1_REGS_BASE  (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_SERIAL1_REGS_OFFSET)
> +#define MPC8544_PCI_REGS_OFFSET    0x8000ULL
> +#define MPC8544_PCI_REGS_BASE      (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_PCI_REGS_OFFSET)

You don't use any of the bases anymore, right? Please remove the respective 
#define's.

The rest of the patch looks very nice.


Alex

> #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_UTIL_BASE          (MPC8544_CCSRBAR_BASE + 
> MPC8544_UTIL_OFFSET)
> #define MPC8544_SPIN_BASE          0xEF000000ULL
> 
> struct boot_info
> @@ -419,6 +428,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) {
> @@ -474,8 +485,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) {
> @@ -484,25 +499,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[0], DEVICE_BIG_ENDIAN);
> +                       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");
> -- 
> 1.7.0.4
> 
>