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

[Richard Henderson]

On 5/9/22 04:38, yangxiaojuan wrote:
> > You are not considering CSR[0x420][49], which changes the format of this 
> > mapping.
> Thanks very much, I will consider the mapping format by read iocsr[0x420][49] 
> like this:
> static uint64_t map_format(void)
> {
>      LoongArchCPU *cpu;
>      CPULoongArchState *env;
>      uint64_t val;
>
>      cpu = LOONGARCH_CPU(current_cpu);
>      env = &(cpu->env);
>
>      val = address_space_ldq(&env->address_space_iocsr, 0x420,
>                               MEMTXATTRS_UNSPECIFIED, NULL);
>      val &= 1 << 49;
>      return val;
> }

I'm not 100% sure how this "Other configuration control register" should be handled, but 
definitely not like this.

I see you're putting control of this register into loongarch_qemu_read in 
target/loongarch/cpu.c.  Which, I suppose is fair, because this is documented as part of 
the 3A5000 cpu documentation.  But then you split out all of the devices which are *also* 
documented as part of the cpu into the board configuration.

This reminds me of the memory-mapped interface that the armv7m cpu has with its own 
registers.  I believe that you need to model this similarly, where you will have a device 
that represents the cpu, and then instantiates all of the devices that are listed in the 
Loongson 3A5000 TRM -- call this ls3a to match the ls7a name you have for the 7A1000 
bridge device.

When there is a write to the ls3a "Other function configuration register", the ls3a will 
need to communicate the changes to the various bits to its various sub-devices.  I do not 
think it unreasonable to have direct function calls between the components.

Peter, do you have any advice from the armv7m side?


> > > +    case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
> > > +        index = (offset - EXTIOI_IPMAP_START) >> 2;
> > > +        s->ipmap[index] = val;
> > > +        break;
> >
> > Do you need to recompute the entire interrupt map when ipmap changes?
> Sorry, could you explain it in more detail? i can not understand the meanning of 'the 
> entire interrupt map'?

I mean, ipmap[*] and coremap[*] controls how isr[*] maps to the various cpus, as 
coreisr[*].  If either ipmap or coremap changes, do you need to re-compute coreisr[*] from 
the input isr[*]?


r~