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

[maobibo]

在 2022/5/10 02:25, Richard Henderson 写道:
> On 5/9/22 13:04, Peter Maydell wrote:
>> On Mon, 9 May 2022 at 18:56, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>> 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?
>>
>> Nothing concrete. I'm not sure that we'd structure the armv7m stuff the way
>> we have now if we were writing it from scratch, but it's functional enough.
>> (In particular, if MMIO regions were part of Device rather than SysBusDevice
>> then I'd be tempted to suggest that CPUs with MMIO-mapped registers should
>> directly own their MemoryRegions for them. But they aren't, so we can't do
>> that.)
> 
> Having thought about this a little more, I believe that these registers are 
> part of the "cpu package", because they are shared between the 4 cpu cores 
> within the package.  Thus their current placement attached to LoongArchCPU -- 
> as well as the current placement of address_space_iocsr -- is incorrect.

The extioi hardware design is not friend to software developer, local cpu INTC
is mixed with board INTC with extioi/iocsr. Local cpu INTC registers should be 
banked,
address space is space for local cpu review point.

how about put address_space_iocsr as board rather than percpu since there is no 
concept
of "cpu package".

regards
bibo, mao
> 
> 
> r~