Re: [PATCH v1 35/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)

[Mark Cave-Ayland]

On 19/04/2022 02:50, yangxiaojuan wrote:

> On 2022/4/18 下午4:57, Mark Cave-Ayland wrote:
> > On 18/04/2022 04:48, Richard Henderson wrote:
> >
> > > On 4/15/22 02:40, Xiaojuan Yang wrote:
> > > > + memory_region_init(&s->mmio[cpu], OBJECT(s),
> > > > +                           "loongarch_extioi", EXTIOI_SIZE);
> > > > +
> > > > +        memory_region_init_io(&s->mmio_nodetype[cpu], OBJECT(s),
> > > > +                              &extioi_nodetype_ops, s,
> > > > +                              EXTIOI_LINKNAME(.nodetype),
> > > > +                              IPMAP_OFFSET - APIC_BASE);
> > > > +        memory_region_add_subregion(&s->mmio[cpu], 0, &s->mmio_nodetype[cpu]);
> > > > +
> > > > + memory_region_init_io(&s->mmio_ipmap_enable[cpu], OBJECT(s),
> > > > +                              &extioi_ipmap_enable_ops, s,
> > > > +                              EXTIOI_LINKNAME(.ipmap_enable),
> > > > +                              BOUNCE_OFFSET - IPMAP_OFFSET);
> > > > +        memory_region_add_subregion(&s->mmio[cpu], IPMAP_OFFSET - APIC_BASE,
> > > > + &s->mmio_ipmap_enable[cpu]);
> > > > +
> > > > + memory_region_init_io(&s->mmio_bounce_coreisr[cpu], OBJECT(s),
> > > > +                              &extioi_bounce_coreisr_ops, s,
> > > > + EXTIOI_LINKNAME(.bounce_coreisr),
> > > > +                              COREMAP_OFFSET - BOUNCE_OFFSET);
> > > > +        memory_region_add_subregion(&s->mmio[cpu], BOUNCE_OFFSET - APIC_BASE,
> > > > + &s->mmio_bounce_coreisr[cpu]);
> > > > +
> > > > +        memory_region_init_io(&s->mmio_coremap[cpu], OBJECT(s),
> > > > +                              &extioi_coremap_ops, s,
> > > > +                              EXTIOI_LINKNAME(.coremap),
> > > > +                              EXTIOI_COREMAP_END);
> > > > +        memory_region_add_subregion(&s->mmio[cpu], COREMAP_OFFSET - APIC_BASE,
> > > > + &s->mmio_coremap[cpu]);
> > >
> > > Why are these separate memory regions, instead of one region? They're certainly 
> > > described in a single table in section 11.2 of the 3A5000 manual...
> >
> > The reason it was done like this is because there were different access sizes in 
> > the relevant _ops definitions. Certainly when I looked at the patches previously I 
> > wasn't able to easily see how these could be consolidated without digging deeper 
> > into the documentation.
> Would it be better to keep consistent with the content of the 3A5000 manual 
> document？ And only one memory region is used to represent the extioi iocsr region

The reason that different memory regions are required is because you've specified 
different access size requirements for each region:

static const MemoryRegionOps extioi_nodetype_ops = {
    .read = extioi_nodetype_readw,
    .write = extioi_nodetype_writew,
    .impl.min_access_size = 4,
    .impl.max_access_size = 4,
    .valid.min_access_size = 4,
    .valid.max_access_size = 8,
    .endianness = DEVICE_LITTLE_ENDIAN,
};

static const MemoryRegionOps extioi_ipmap_enable_ops = {
    .read = extioi_ipmap_enable_read,
    .write = extioi_ipmap_enable_write,
    .impl.min_access_size = 1,
    .impl.max_access_size = 1,
    .valid.min_access_size = 1,
    .valid.max_access_size = 8,
    .endianness = DEVICE_LITTLE_ENDIAN,
};

static const MemoryRegionOps extioi_bounce_coreisr_ops = {
    .read = extioi_bounce_coreisr_readw,
    .write = extioi_bounce_coreisr_writew,
    .impl.min_access_size = 4,
    .impl.max_access_size = 4,
    .valid.min_access_size = 4,
    .valid.max_access_size = 8,
    .endianness = DEVICE_LITTLE_ENDIAN,
};

static const MemoryRegionOps extioi_coremap_ops = {
    .read = extioi_coremap_read,
    .write = extioi_coremap_write,
    .impl.min_access_size = 1,
    .impl.max_access_size = 1,
    .valid.min_access_size = 1,
    .valid.max_access_size = 8,
    .endianness = DEVICE_LITTLE_ENDIAN,
};

Certainly this is unusual (and for a given device I'd expect that its registers would 
all have the same access requirements), but it's not something that can be tested 
without access to real hardware.

As Richard suggests though, if it is found that all of the device registers have the 
same access requirements then they could potentially be collapsed into a single 
memory region.


ATB,

Mark.