Patchwork [v3,07/11] bonito: convert north bridge register mapping to memory API

login
register
mail settings
Submitter Benoit Canet
Date Nov. 22, 2011, 8:34 p.m.
Message ID <1321994102-28263-8-git-send-email-benoit.canet@gmail.com>
Download mbox | patch
Permalink /patch/127171/
State New
Headers show

Comments

Benoit Canet - Nov. 22, 2011, 8:34 p.m.
Signed-off-by: Benoît Canet <benoit.canet@gmail.com>
---
 hw/bonito.c |   39 ++++++++++++++++++---------------------
 1 files changed, 18 insertions(+), 21 deletions(-)
Peter Maydell - Nov. 22, 2011, 11:03 p.m.
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
Benoit Canet - Nov. 23, 2011, 5:55 p.m.
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.
Peter Maydell - Nov. 23, 2011, 6:25 p.m.
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
Avi Kivity - Nov. 24, 2011, 8:40 a.m.
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.
Benoit Canet - Nov. 24, 2011, 8:47 a.m.
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
>
>

Patch

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,