| Message ID | 1349714816-12618-3-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
> -----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
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.
> -----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
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.
> -----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
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
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");
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(-)