| Message ID | 1321994102-28263-8-git-send-email-benoit.canet@gmail.com |
|---|---|
| State | New |
| Headers | show |
2011/11/22 Benoît Canet <benoit.canet@gmail.com>: > static int bonito_initfn(PCIDevice *dev) > { > PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev); > + SysBusDevice *sysbus = sysbus_from_qdev(&dev->qdev); This looks odd. The device here is a PCIBonitoState, which is-a PCIDevice, which is-a DeviceState. It's not a SysBusDevice and merely casting doesn't make it one. I'm not sure what should be being done here, but I'm pretty sure this won't work... -- PMM
2011/11/23 Peter Maydell <peter.maydell@linaro.org> > 2011/11/22 Benoît Canet <benoit.canet@gmail.com>: > > static int bonito_initfn(PCIDevice *dev) > > { > > PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev); > > + SysBusDevice *sysbus = sysbus_from_qdev(&dev->qdev); > > This looks odd. The device here is a PCIBonitoState, which > is-a PCIDevice, which is-a DeviceState. It's not a SysBusDevice > and merely casting doesn't make it one. > > I'm not sure what should be being done here, but I'm pretty > sure this won't work... > It would work using memory_region_add_subregion() and get_system_memory() but Avi previously noticed me that using get_system_memory() in devices is wrong because it goes against one of the goals of the memory API : avoiding global knowledge. I think I need Avi's advices on this one.
2011/11/23 Benoît Canet <benoit.canet@gmail.com>: > It would work using memory_region_add_subregion() and get_system_memory() > but Avi previously noticed me that using get_system_memory() in devices > is wrong because it goes against one of the goals of the memory API : > avoiding global knowledge. Yes. It's a bit difficult to tell without hardware docs for this chip, but I suspect that at least some of these memory regions should actually be registered as part of the init of the "Bonito-pcihost" sysbus device and then mapped by bonito_init(). But I don't know enough about PCI or the Bonito to know just how much of bonito_initfn() ought to move into bonito_pcihost_initfn(). -- PMM
On 11/23/2011 01:03 AM, Peter Maydell wrote: > 2011/11/22 Benoît Canet <benoit.canet@gmail.com>: > > static int bonito_initfn(PCIDevice *dev) > > { > > PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev); > > + SysBusDevice *sysbus = sysbus_from_qdev(&dev->qdev); > > This looks odd. The device here is a PCIBonitoState, which > is-a PCIDevice, which is-a DeviceState. It's not a SysBusDevice > and merely casting doesn't make it one. > > I'm not sure what should be being done here, but I'm pretty > sure this won't work... s->pcihost.busdev gives you a SysBusDevice (and s->pcihost.confmem/datamem gives you a few MemoryRegions that you need later in bonito_initfn.
Thanks On Thu, Nov 24, 2011 at 9:40 AM, Avi Kivity <avi@redhat.com> wrote: > On 11/23/2011 01:03 AM, Peter Maydell wrote: > > 2011/11/22 Benoît Canet <benoit.canet@gmail.com>: > > > static int bonito_initfn(PCIDevice *dev) > > > { > > > PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev); > > > + SysBusDevice *sysbus = sysbus_from_qdev(&dev->qdev); > > > > This looks odd. The device here is a PCIBonitoState, which > > is-a PCIDevice, which is-a DeviceState. It's not a SysBusDevice > > and merely casting doesn't make it one. > > > > I'm not sure what should be being done here, but I'm pretty > > sure this won't work... > > s->pcihost.busdev gives you a SysBusDevice (and > s->pcihost.confmem/datamem gives you a few MemoryRegions that you need > later in bonito_initfn. > > -- > error compiling committee.c: too many arguments to function > >
diff --git a/hw/bonito.c b/hw/bonito.c index fdb8198..9260848 100644 --- a/hw/bonito.c +++ b/hw/bonito.c @@ -201,9 +201,7 @@ typedef struct PCIBonitoState } boncop; /* Bonito registers */ - target_phys_addr_t bonito_reg_start; - target_phys_addr_t bonito_reg_length; - int bonito_reg_handle; + MemoryRegion iomem; target_phys_addr_t bonito_pciconf_start; target_phys_addr_t bonito_pciconf_length; @@ -233,7 +231,8 @@ typedef struct PCIBonitoState PCIBonitoState * bonito_state; -static void bonito_writel(void *opaque, target_phys_addr_t addr, uint32_t val) +static void bonito_writel(void *opaque, target_phys_addr_t addr, + uint64_t val, unsigned size) { PCIBonitoState *s = opaque; uint32_t saddr; @@ -295,7 +294,8 @@ static void bonito_writel(void *opaque, target_phys_addr_t addr, uint32_t val) } } -static uint32_t bonito_readl(void *opaque, target_phys_addr_t addr) +static uint64_t bonito_readl(void *opaque, target_phys_addr_t addr, + unsigned size) { PCIBonitoState *s = opaque; uint32_t saddr; @@ -311,16 +311,14 @@ static uint32_t bonito_readl(void *opaque, target_phys_addr_t addr) } } -static CPUWriteMemoryFunc * const bonito_write[] = { - NULL, - NULL, - bonito_writel, -}; - -static CPUReadMemoryFunc * const bonito_read[] = { - NULL, - NULL, - bonito_readl, +static const MemoryRegionOps bonito_ops = { + .read = bonito_readl, + .write = bonito_writel, + .endianness = DEVICE_NATIVE_ENDIAN, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + }, }; static void bonito_pciconf_writel(void *opaque, target_phys_addr_t addr, @@ -690,17 +688,16 @@ static int bonito_pcihost_initfn(SysBusDevice *dev) static int bonito_initfn(PCIDevice *dev) { PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev); + SysBusDevice *sysbus = sysbus_from_qdev(&dev->qdev); /* Bonito North Bridge, built on FPGA, VENDOR_ID/DEVICE_ID are "undefined" */ pci_config_set_prog_interface(dev->config, 0x00); /* set the north bridge register mapping */ - s->bonito_reg_handle = cpu_register_io_memory(bonito_read, bonito_write, s, - DEVICE_NATIVE_ENDIAN); - s->bonito_reg_start = BONITO_INTERNAL_REG_BASE; - s->bonito_reg_length = BONITO_INTERNAL_REG_SIZE; - cpu_register_physical_memory(s->bonito_reg_start, s->bonito_reg_length, - s->bonito_reg_handle); + memory_region_init_io(&s->iomem, &bonito_ops, s, + "north-bridge-register", BONITO_INTERNAL_REG_SIZE); + sysbus_init_mmio_region(sysbus, &s->iomem); + sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE); /* set the north bridge pci configure mapping */ s->bonito_pciconf_handle = cpu_register_io_memory(bonito_pciconf_read,
Signed-off-by: Benoît Canet <benoit.canet@gmail.com> --- hw/bonito.c | 39 ++++++++++++++++++--------------------- 1 files changed, 18 insertions(+), 21 deletions(-)