Re: [PATCH V3 1/2] dt-bindings: interrupt-controller: Add Loongson EIOINTC

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 23/04/2023 10:30, Binbin Zhou wrote:
> On Thu, Apr 20, 2023 at 11:52 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 20/04/2023 15:00, Binbin Zhou wrote:
>>>>> +examples:
>>>>> +  - |
>>>>> +    eiointc: interrupt-controller@1fe11600 {
>>>>> +      compatible = "loongson,ls2k0500-eiointc";
>>>>> +      reg = <0x1fe11600 0x10>,
>>>>> +            <0x1fe11700 0x10>,
>>>>> +            <0x1fe11800 0x10>,
>>>>> +            <0x1fe114c0 0x4>;
>>>>
>>>> Binding is OK, but are you sure you want to split the address space like
>>>> this? It looks like two address spaces (enable+clear+status should be
>>>> one). Are you sure this is correct?
>>>>
>>> Hi Krzysztof:
>>>
>>> These registers are all in the range of chip configuration registers,
>>> in the case of LS2K0500, which has a base address of 0x1fe10000.
>>> However, the individual register addresses are not contiguous with
>>> each other, and most are distributed across modules, so I feel that
>>> they should be listed in detail as they are used.
>>
>> Do you want to say that:
>> Between 0x1fe11600 and 0x1fe11700 there are EIOINTC registers and other
>> (independent) module registers?
> 
> No, this section is all EIO-related configuration, but there will be
> undefined space here.
> 
> Throughout the chip configuration space, there are some relatively
> common areas, such as the definition of 0x1fe1_14c0.
> Because our chip supports two interrupt modes, node legacy I/O
> interrupt and extended I/O interrupt, both modes require interrupt
> routing registers.
> Their registers are then defined together: the legacy interrupt I/O
> start address is 0x1fe1_1400, while the extended I/O interrupt start
> address is 0x1fe1_14c0.
> 
> Then I have carefully compared the chip configuration space in
> LS2K0500 and LS2K2000 and can see that:
> 
> 1. The chip configuration space base addresses are different, but they
> both have a size of 64KB;
> 2. The offset addresses of the EIO related registers are the same, for
> example the offset of the enable register is 0x1600.
> 
> Wouldn't it be better to declare the entire configuration space (64KB)
> directly in the dts and use the offsets to access the corresponding
> registers?
> 
> Example:
> reg = <0x1fe10000 0x10000>.

Yes, that's what usually we do.

Best regards,
Krzysztof