On 11/17/2011 02:56 PM, Peter Maydell wrote: > 2011/11/17 Benoît Canet <benoit.ca...@gmail.com>: > > Signed-off-by: Benoit Canet <benoit.ca...@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. -- error compiling committee.c: too many arguments to function
