| Message ID | 1321532700-8929-5-git-send-email-benoit.canet@gmail.com |
|---|---|
| State | New |
| Headers | show |
2011/11/17 Benoît Canet <benoit.canet@gmail.com>: > Signed-off-by: Benoit Canet <benoit.canet@gmail.com> > --- a/hw/sh7750.c > +++ b/hw/sh7750.c > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem) > "cache-and-tlb", 0x08000000); > memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem); > > - sh_intc_init(&s->intc, NR_SOURCES, > + sh_intc_init(sysmem, &s->intc, NR_SOURCES, > _INTC_ARRAY(mask_registers), > _INTC_ARRAY(prio_registers)); This would be nicer as a sysbus device so we didn't have to hand it the sysmem, but that can be done later if we care enough I guess. > + iomem = &desc->iomem; > + iomem_p4 = desc->iomem_aliases + index; > + iomem_a7 = iomem_p4 + 1; > + > +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s" > + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4"); > + memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4); > + memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4); > + > + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7"); > + memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4); > + memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7); > +#undef SH_INTC_IOMEM_FORMAT > + > + /* used to increment aliases index */ > + return 2; This is going to give us 6 * 2 * 2 = 24 four-byte memory regions, incidentally. That should be OK, but one memory region per register is an interesting arrangement. > @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc, > desc->nr_mask_regs = nr_mask_regs; > desc->prio_regs = prio_regs; > desc->nr_prio_regs = nr_prio_regs; > + /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */ > + desc->iomem_aliases = g_new0(MemoryRegion, > + (nr_mask_regs + nr_prio_regs) * 4); This should be exactly the right size... > + /* free unused MemoryRegions */ > + desc->iomem_aliases = g_realloc(desc->iomem_aliases, > + sizeof(MemoryRegion)*j); making this realloc unnecessary; or am I missing something? -- PMM
On 11/17/2011 02:56 PM, Peter Maydell wrote: > 2011/11/17 Benoît Canet <benoit.canet@gmail.com>: > > Signed-off-by: Benoit Canet <benoit.canet@gmail.com> > > --- a/hw/sh7750.c > > +++ b/hw/sh7750.c > > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem) > > "cache-and-tlb", 0x08000000); > > memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem); > > > > - sh_intc_init(&s->intc, NR_SOURCES, > > + sh_intc_init(sysmem, &s->intc, NR_SOURCES, > > _INTC_ARRAY(mask_registers), > > _INTC_ARRAY(prio_registers)); > > This would be nicer as a sysbus device so we didn't have to hand > it the sysmem, but that can be done later if we care enough I guess. Later, yes. > > > + iomem = &desc->iomem; > > + iomem_p4 = desc->iomem_aliases + index; > > + iomem_a7 = iomem_p4 + 1; > > + > > +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s" > > + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4"); > > + memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4); > > + memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4); > > + > > + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7"); > > + memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4); > > + memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7); > > +#undef SH_INTC_IOMEM_FORMAT > > + > > + /* used to increment aliases index */ > > + return 2; > > This is going to give us 6 * 2 * 2 = 24 four-byte memory regions, > incidentally. That should be OK, but one memory region per register > is an interesting arrangement. In fact if we introduce a Register class there's no reason it won't be a MemoryRegion. So any Register would be a MemoryRegion, we could have thousands in a system. I don't see anything wrong with it, do you? > > > @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc, > > desc->nr_mask_regs = nr_mask_regs; > > desc->prio_regs = prio_regs; > > desc->nr_prio_regs = nr_prio_regs; > > + /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */ > > + desc->iomem_aliases = g_new0(MemoryRegion, > > + (nr_mask_regs + nr_prio_regs) * 4); > > This should be exactly the right size... > > > + /* free unused MemoryRegions */ > > + desc->iomem_aliases = g_realloc(desc->iomem_aliases, > > + sizeof(MemoryRegion)*j); > > making this realloc unnecessary; or am I missing something? > Not all calls to sh_intc_register() return 2. However, calling realloc() in a MemoryRegion array is not a good idea, since the pointers may leak (memory_region_add_subregion() does this). It's true that a size-reducing realloc doesn't change the pointer, yet it makes me uncomfortable.
Yes, allocating the exact size would make the realloc unnecessary. But it involve more code to walk one more time through the mask_reg and prio_reg array before allocating. I made it this way to make code shorter. The freed MemoryRegion are not initialized at all. However as realloc seems to be a bad idea I need something to compute the exact size in a minimum of code. 2011/11/17 Peter Maydell <peter.maydell@linaro.org> > 2011/11/17 Benoît Canet <benoit.canet@gmail.com>: > > Signed-off-by: Benoit Canet <benoit.canet@gmail.com> > > --- a/hw/sh7750.c > > +++ b/hw/sh7750.c > > @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, > MemoryRegion *sysmem) > > "cache-and-tlb", 0x08000000); > > memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem); > > > > - sh_intc_init(&s->intc, NR_SOURCES, > > + sh_intc_init(sysmem, &s->intc, NR_SOURCES, > > _INTC_ARRAY(mask_registers), > > _INTC_ARRAY(prio_registers)); > > This would be nicer as a sysbus device so we didn't have to hand > it the sysmem, but that can be done later if we care enough I guess. > > > + iomem = &desc->iomem; > > + iomem_p4 = desc->iomem_aliases + index; > > + iomem_a7 = iomem_p4 + 1; > > + > > +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s" > > + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, > "p4"); > > + memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), > 4); > > + memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4); > > + > > + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, > "a7"); > > + memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), > 4); > > + memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7); > > +#undef SH_INTC_IOMEM_FORMAT > > + > > + /* used to increment aliases index */ > > + return 2; > > This is going to give us 6 * 2 * 2 = 24 four-byte memory regions, > incidentally. That should be OK, but one memory region per register > is an interesting arrangement. > > > @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc, > > desc->nr_mask_regs = nr_mask_regs; > > desc->prio_regs = prio_regs; > > desc->nr_prio_regs = nr_prio_regs; > > + /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */ > > + desc->iomem_aliases = g_new0(MemoryRegion, > > + (nr_mask_regs + nr_prio_regs) * 4); > > This should be exactly the right size... > > > + /* free unused MemoryRegions */ > > + desc->iomem_aliases = g_realloc(desc->iomem_aliases, > > + sizeof(MemoryRegion)*j); > > making this realloc unnecessary; or am I missing something? > > -- PMM >
diff --git a/hw/sh7750.c b/hw/sh7750.c index e181305..930d212 100644 --- a/hw/sh7750.c +++ b/hw/sh7750.c @@ -756,7 +756,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu, MemoryRegion *sysmem) "cache-and-tlb", 0x08000000); memory_region_add_subregion(sysmem, 0xf0000000, &s->mmct_iomem); - sh_intc_init(&s->intc, NR_SOURCES, + sh_intc_init(sysmem, &s->intc, NR_SOURCES, _INTC_ARRAY(mask_registers), _INTC_ARRAY(prio_registers)); diff --git a/hw/sh_intc.c b/hw/sh_intc.c index e07424f..38cefc9 100644 --- a/hw/sh_intc.c +++ b/hw/sh_intc.c @@ -219,7 +219,8 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id, #endif } -static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset) +static uint64_t sh_intc_read(void *opaque, target_phys_addr_t offset, + unsigned size) { struct intc_desc *desc = opaque; intc_enum *enum_ids = NULL; @@ -238,7 +239,7 @@ static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset) } static void sh_intc_write(void *opaque, target_phys_addr_t offset, - uint32_t value) + uint64_t value, unsigned size) { struct intc_desc *desc = opaque; intc_enum *enum_ids = NULL; @@ -282,16 +283,10 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset, #endif } -static CPUReadMemoryFunc * const sh_intc_readfn[] = { - sh_intc_read, - sh_intc_read, - sh_intc_read -}; - -static CPUWriteMemoryFunc * const sh_intc_writefn[] = { - sh_intc_write, - sh_intc_write, - sh_intc_write +static const struct MemoryRegionOps sh_intc_ops = { + .read = sh_intc_read, + .write = sh_intc_write, + .endianness = DEVICE_NATIVE_ENDIAN, }; struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id) @@ -302,15 +297,36 @@ struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id) return NULL; } -static void sh_intc_register(struct intc_desc *desc, - unsigned long address) +static unsigned int sh_intc_register(MemoryRegion *sysmem, + struct intc_desc *desc, + const unsigned long address, + const char *type, + const char *action, + const unsigned int index) { - if (address) { - cpu_register_physical_memory_offset(P4ADDR(address), 4, - desc->iomemtype, INTC_A7(address)); - cpu_register_physical_memory_offset(A7ADDR(address), 4, - desc->iomemtype, INTC_A7(address)); + char name[60]; + MemoryRegion *iomem, *iomem_p4, *iomem_a7; + + if (!address) { + return 0; } + + iomem = &desc->iomem; + iomem_p4 = desc->iomem_aliases + index; + iomem_a7 = iomem_p4 + 1; + +#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s" + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4"); + memory_region_init_alias(iomem_p4, name, iomem, INTC_A7(address), 4); + memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4); + + snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7"); + memory_region_init_alias(iomem_a7, name, iomem, INTC_A7(address), 4); + memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7); +#undef SH_INTC_IOMEM_FORMAT + + /* used to increment aliases index */ + return 2; } static void sh_intc_register_source(struct intc_desc *desc, @@ -415,14 +431,15 @@ void sh_intc_register_sources(struct intc_desc *desc, } } -int sh_intc_init(struct intc_desc *desc, +int sh_intc_init(MemoryRegion *sysmem, + struct intc_desc *desc, int nr_sources, struct intc_mask_reg *mask_regs, int nr_mask_regs, struct intc_prio_reg *prio_regs, int nr_prio_regs) { - unsigned int i; + unsigned int i, j; desc->pending = 0; desc->nr_sources = nr_sources; @@ -430,7 +447,11 @@ int sh_intc_init(struct intc_desc *desc, desc->nr_mask_regs = nr_mask_regs; desc->prio_regs = prio_regs; desc->nr_prio_regs = nr_prio_regs; + /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases). */ + desc->iomem_aliases = g_new0(MemoryRegion, + (nr_mask_regs + nr_prio_regs) * 4); + j = 0; i = sizeof(struct intc_source) * nr_sources; desc->sources = g_malloc0(i); @@ -442,15 +463,19 @@ int sh_intc_init(struct intc_desc *desc, desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources); - desc->iomemtype = cpu_register_io_memory(sh_intc_readfn, - sh_intc_writefn, desc, - DEVICE_NATIVE_ENDIAN); + memory_region_init_io(&desc->iomem, &sh_intc_ops, desc, + "interrupt-controller", 0x100000000ULL); + +#define INT_REG_PARAMS(reg_struct, type, action, j) \ + reg_struct->action##_reg, #type, #action, j if (desc->mask_regs) { for (i = 0; i < desc->nr_mask_regs; i++) { struct intc_mask_reg *mr = desc->mask_regs + i; - sh_intc_register(desc, mr->set_reg); - sh_intc_register(desc, mr->clr_reg); + j += sh_intc_register(sysmem, desc, + INT_REG_PARAMS(mr, mask, set, j)); + j += sh_intc_register(sysmem, desc, + INT_REG_PARAMS(mr, mask, clr, j)); } } @@ -458,10 +483,16 @@ int sh_intc_init(struct intc_desc *desc, for (i = 0; i < desc->nr_prio_regs; i++) { struct intc_prio_reg *pr = desc->prio_regs + i; - sh_intc_register(desc, pr->set_reg); - sh_intc_register(desc, pr->clr_reg); + j += sh_intc_register(sysmem, desc, + INT_REG_PARAMS(pr, prio, set, j)); + j += sh_intc_register(sysmem, desc, + INT_REG_PARAMS(pr, prio, clr, j)); } } +#undef INT_REG_PARAMS + /* free unused MemoryRegions */ + desc->iomem_aliases = g_realloc(desc->iomem_aliases, + sizeof(MemoryRegion)*j); return 0; } diff --git a/hw/sh_intc.h b/hw/sh_intc.h index c117d6f..8916e8c 100644 --- a/hw/sh_intc.h +++ b/hw/sh_intc.h @@ -3,6 +3,7 @@ #include "qemu-common.h" #include "irq.h" +#include "exec-memory.h" typedef unsigned char intc_enum; @@ -46,6 +47,8 @@ struct intc_source { }; struct intc_desc { + MemoryRegion iomem; + MemoryRegion *iomem_aliases; qemu_irq *irqs; struct intc_source *sources; int nr_sources; @@ -53,7 +56,6 @@ struct intc_desc { int nr_mask_regs; struct intc_prio_reg *prio_regs; int nr_prio_regs; - int iomemtype; int pending; /* number of interrupt sources that has pending set */ }; @@ -68,7 +70,8 @@ void sh_intc_register_sources(struct intc_desc *desc, struct intc_group *groups, int nr_groups); -int sh_intc_init(struct intc_desc *desc, +int sh_intc_init(MemoryRegion *sysmem, + struct intc_desc *desc, int nr_sources, struct intc_mask_reg *mask_regs, int nr_mask_regs, diff --git a/target-sh4/helper.c b/target-sh4/helper.c index 5a1e15e..f4dda48 100644 --- a/target-sh4/helper.c +++ b/target-sh4/helper.c @@ -24,7 +24,10 @@ #include <signal.h> #include "cpu.h" + +#if !defined(CONFIG_USER_ONLY) #include "hw/sh_intc.h" +#endif #if defined(CONFIG_USER_ONLY)
Signed-off-by: Benoit Canet <benoit.canet@gmail.com> --- hw/sh7750.c | 2 +- hw/sh_intc.c | 87 ++++++++++++++++++++++++++++++++++---------------- hw/sh_intc.h | 7 +++- target-sh4/helper.c | 3 ++ 4 files changed, 68 insertions(+), 31 deletions(-)