mbox series

[V3,0/2] irqchip: loongson-eiointc: Add DT init support

Message ID cover.1681887790.git.zhoubinbin@loongson.cn
Headers show
Series irqchip: loongson-eiointc: Add DT init support | expand

Message

Binbin Zhou April 19, 2023, 7:17 a.m. UTC 
Add EIOINTC irqchip DT support, which is needed for Loongson chips
that are DT-based and support EIOINTC, such as the Loongson-2K0500 chip.

---
V3:
- patch(1/2)
  - Drop quotes;
  - Drop interrupt-names;
  - Drop loongson,eio-num-vecs;
  - SoC-based compatibles instead of version-based compatibles.
- patch (2/2)
  - irq_set_handler_data() is used to get parent irq form DTS;
  - Set vec_count by judging compatibles.

V2:
- Add the dt-bindings file (1/2);
- patch(2/2)
  - Remove forgotten debugging messages;
  - Change compatible string name to "loongson,eiointc-1.0".

Binbin Zhou (2):
  dt-bindings: interrupt-controller: Add Loongson EIOINTC
  irqchip/loongson-eiointc: Add DT init support

 .../loongson,eiointc.yaml                     |  74 ++++++++++
 drivers/irqchip/irq-loongson-eiointc.c        | 129 +++++++++++++-----
 2 files changed, 169 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml

Comments

Binbin Zhou April 20, 2023, 1 p.m. UTC | #1 
On Thu, Apr 20, 2023 at 4:09 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/04/2023 09:17, Binbin Zhou wrote:
> > Add Loongson Extended I/O Interrupt controller binding with DT schema
> > format using json-schema.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../loongson,eiointc.yaml                     | 74 +++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml b/Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml
> > new file mode 100644
> > index 000000000000..4ab4efb061e1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/loongson,eiointc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson Extended I/O Interrupt Controller
> > +
> > +maintainers:
> > +  - Binbin Zhou <zhoubinbin@loongson.cn>
> > +
> > +description: |
> > +  This interrupt controller is found on the Loongson-3 family chips and
> > +  Loongson-2K series chips and is used to distribute interrupts directly to
> > +  individual cores without forwarding them through the HT's interrupt line.
> > +
> > +allOf:
> > +  - $ref: /schemas/interrupt-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - loongson,ls2k0500-eiointc
> > +      - loongson,ls2k2000-eiointc
> > +
> > +  reg:
> > +    items:
> > +      - description: Interrupt enable registers
> > +      - description: Interrupt status registers
> > +      - description: Interrupt clear registers
> > +      - description: Interrupt routing configuration registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: enable
> > +      - const: status
> > +      - const: clear
> > +      - const: route
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - '#interrupt-cells'
> > +
> > +unevaluatedProperties: false
> > +
> > +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.

Thanks.
Binbin

> Best regards,
> Krzysztof
>
>
Krzysztof Kozlowski April 20, 2023, 3:52 p.m. UTC | #2 
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?


Best regards,
Krzysztof
Rob Herring (Arm) April 21, 2023, 7:04 p.m. UTC | #3 
On Thu, Apr 20, 2023 at 09:00:42PM +0800, Binbin Zhou wrote:
> On Thu, Apr 20, 2023 at 4:09 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 19/04/2023 09:17, Binbin Zhou wrote:
> > > Add Loongson Extended I/O Interrupt controller binding with DT schema
> > > format using json-schema.
> > >
> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > ---
> > >  .../loongson,eiointc.yaml                     | 74 +++++++++++++++++++
> > >  1 file changed, 74 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml b/Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml
> > > new file mode 100644
> > > index 000000000000..4ab4efb061e1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/loongson,eiointc.yaml
> > > @@ -0,0 +1,74 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/interrupt-controller/loongson,eiointc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Loongson Extended I/O Interrupt Controller
> > > +
> > > +maintainers:
> > > +  - Binbin Zhou <zhoubinbin@loongson.cn>
> > > +
> > > +description: |
> > > +  This interrupt controller is found on the Loongson-3 family chips and
> > > +  Loongson-2K series chips and is used to distribute interrupts directly to
> > > +  individual cores without forwarding them through the HT's interrupt line.
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/interrupt-controller.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - loongson,ls2k0500-eiointc
> > > +      - loongson,ls2k2000-eiointc
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: Interrupt enable registers
> > > +      - description: Interrupt status registers
> > > +      - description: Interrupt clear registers
> > > +      - description: Interrupt routing configuration registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: enable
> > > +      - const: status
> > > +      - const: clear
> > > +      - const: route
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-controller: true
> > > +
> > > +  '#interrupt-cells':
> > > +    const: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - interrupt-controller
> > > +  - '#interrupt-cells'
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +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.

Where is the schema for this? Either it should be the 
interrupt-controller itself or this binding should be a child node of 
it. Which way really depends on whether the eiointc is reused on 
multiple chips with different register offsets or parent block.

Can't really give better advice without a complete picture of the 'chip 
configuration registers'. So please provide that.

Rob
Binbin Zhou April 23, 2023, 8:30 a.m. UTC | #4 
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>.

Thanks.
Binbin

>
> Best regards,
> Krzysztof
Krzysztof Kozlowski April 24, 2023, 8:45 a.m. UTC | #5 
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