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

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

[Binbin Zhou]

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

-- 
2.39.1

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

[Binbin Zhou]

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>;
+      reg-names = "enable", "status", "clear", "route";
+
+      interrupt-controller;
+      #interrupt-cells = <1>;
+
+      interrupt-parent = <&cpuintc>;
+      interrupts = <3>;
+    };
+
+...
-- 
2.39.1

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

[Binbin Zhou]

Add EIOINTC irqchip DT support, which is needed for Loongson chips
based on DT and supporting EIOINTC, such as the Loongson-2K0500 SOC.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/irqchip/irq-loongson-eiointc.c | 129 ++++++++++++++++++-------
 1 file changed, 95 insertions(+), 34 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index d15fd38c1756..cfeb40daf1d7 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -39,6 +39,7 @@ static int nr_pics;
 
 struct eiointc_priv {
 	u32			node;
+	u32			vec_count;
 	nodemask_t		node_map;
 	cpumask_t		cpuspan_map;
 	struct fwnode_handle	*domain_handle;
@@ -156,18 +157,18 @@ static int eiointc_router_init(unsigned int cpu)
 	if ((cpu_logical_map(cpu) % CORES_PER_EIO_NODE) == 0) {
 		eiointc_enable();
 
-		for (i = 0; i < VEC_COUNT / 32; i++) {
+		for (i = 0; i < eiointc_priv[0]->vec_count / 32; i++) {
 			data = (((1 << (i * 2 + 1)) << 16) | (1 << (i * 2)));
 			iocsr_write32(data, EIOINTC_REG_NODEMAP + i * 4);
 		}
 
-		for (i = 0; i < VEC_COUNT / 32 / 4; i++) {
+		for (i = 0; i < eiointc_priv[0]->vec_count / 32 / 4; i++) {
 			bit = BIT(1 + index); /* Route to IP[1 + index] */
 			data = bit | (bit << 8) | (bit << 16) | (bit << 24);
 			iocsr_write32(data, EIOINTC_REG_IPMAP + i * 4);
 		}
 
-		for (i = 0; i < VEC_COUNT / 4; i++) {
+		for (i = 0; i < eiointc_priv[0]->vec_count / 4; i++) {
 			/* Route to Node-0 Core-0 */
 			if (index == 0)
 				bit = BIT(cpu_logical_map(0));
@@ -178,7 +179,7 @@ static int eiointc_router_init(unsigned int cpu)
 			iocsr_write32(data, EIOINTC_REG_ROUTE + i * 4);
 		}
 
-		for (i = 0; i < VEC_COUNT / 32; i++) {
+		for (i = 0; i < eiointc_priv[0]->vec_count / 32; i++) {
 			data = 0xffffffff;
 			iocsr_write32(data, EIOINTC_REG_ENABLE + i * 4);
 			iocsr_write32(data, EIOINTC_REG_BOUNCE + i * 4);
@@ -198,7 +199,7 @@ static void eiointc_irq_dispatch(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 
-	for (i = 0; i < VEC_REG_COUNT; i++) {
+	for (i = 0; i < eiointc_priv[0]->vec_count / VEC_COUNT_PER_REG; i++) {
 		pending = iocsr_read64(EIOINTC_REG_ISR + (i << 3));
 		iocsr_write64(pending, EIOINTC_REG_ISR + (i << 3));
 		while (pending) {
@@ -316,7 +317,7 @@ static void eiointc_resume(void)
 	eiointc_router_init(0);
 
 	for (i = 0; i < nr_pics; i++) {
-		for (j = 0; j < VEC_COUNT; j++) {
+		for (j = 0; j < eiointc_priv[0]->vec_count; j++) {
 			desc = irq_resolve_mapping(eiointc_priv[i]->eiointc_domain, j);
 			if (desc && desc->handle_irq && desc->handle_irq != handle_bad_irq) {
 				raw_spin_lock(&desc->lock);
@@ -373,11 +374,44 @@ static int __init acpi_cascade_irqdomain_init(void)
 	return 0;
 }
 
+static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
+			       u64 node_map)
+{
+	int i;
+
+	node_map = node_map ? node_map : -1ULL;
+	for_each_possible_cpu(i) {
+		if (node_map & (1ULL << (cpu_to_eio_node(i)))) {
+			node_set(cpu_to_eio_node(i), priv->node_map);
+			cpumask_or(&priv->cpuspan_map, &priv->cpuspan_map,
+				   cpumask_of(i));
+		}
+	}
+
+	priv->eiointc_domain = irq_domain_create_linear(priv->domain_handle,
+							priv->vec_count,
+							&eiointc_domain_ops,
+							priv);
+	if (!priv->eiointc_domain) {
+		pr_err("loongson-extioi: cannot add IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	eiointc_priv[nr_pics++] = priv;
+	eiointc_router_init(0);
+	irq_set_chained_handler_and_data(parent_irq, eiointc_irq_dispatch, priv);
+	register_syscore_ops(&eiointc_syscore_ops);
+	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_LOONGARCH_STARTING,
+				  "irqchip/loongarch/intc:starting",
+				  eiointc_router_init, NULL);
+
+	return 0;
+}
+
 int __init eiointc_acpi_init(struct irq_domain *parent,
 				     struct acpi_madt_eio_pic *acpi_eiointc)
 {
-	int i, ret, parent_irq;
-	unsigned long node_map;
+	int parent_irq, ret;
 	struct eiointc_priv *priv;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -391,39 +425,20 @@ int __init eiointc_acpi_init(struct irq_domain *parent,
 		goto out_free_priv;
 	}
 
+	priv->vec_count = VEC_COUNT;
 	priv->node = acpi_eiointc->node;
-	node_map = acpi_eiointc->node_map ? : -1ULL;
-
-	for_each_possible_cpu(i) {
-		if (node_map & (1ULL << cpu_to_eio_node(i))) {
-			node_set(cpu_to_eio_node(i), priv->node_map);
-			cpumask_or(&priv->cpuspan_map, &priv->cpuspan_map, cpumask_of(i));
-		}
-	}
-
-	/* Setup IRQ domain */
-	priv->eiointc_domain = irq_domain_create_linear(priv->domain_handle, VEC_COUNT,
-					&eiointc_domain_ops, priv);
-	if (!priv->eiointc_domain) {
-		pr_err("loongson-eiointc: cannot add IRQ domain\n");
-		goto out_free_handle;
-	}
-
-	eiointc_priv[nr_pics++] = priv;
-
-	eiointc_router_init(0);
-
 	parent_irq = irq_create_mapping(parent, acpi_eiointc->cascade);
-	irq_set_chained_handler_and_data(parent_irq, eiointc_irq_dispatch, priv);
 
-	register_syscore_ops(&eiointc_syscore_ops);
-	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_LOONGARCH_STARTING,
-				  "irqchip/loongarch/intc:starting",
-				  eiointc_router_init, NULL);
+	ret = eiointc_init(priv, parent_irq, acpi_eiointc->node_map);
+	if (ret < 0)
+		goto out_free_handle;
 
 	acpi_set_vec_parent(acpi_eiointc->node, priv->eiointc_domain, pch_group);
 	acpi_set_vec_parent(acpi_eiointc->node, priv->eiointc_domain, msi_group);
+
 	ret = acpi_cascade_irqdomain_init();
+	if (ret < 0)
+		goto out_free_handle;
 
 	return ret;
 
@@ -435,3 +450,49 @@ int __init eiointc_acpi_init(struct irq_domain *parent,
 
 	return -ENOMEM;
 }
+
+static int __init eiointc_of_init(struct device_node *of_node,
+				  struct device_node *parent)
+{
+	int parent_irq, ret;
+	struct eiointc_priv *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	parent_irq = irq_of_parse_and_map(of_node, 0);
+	if (parent_irq <= 0) {
+		ret = -ENODEV;
+		goto out_free_priv;
+	}
+
+	ret = irq_set_handler_data(parent_irq, priv);
+	if (ret < 0)
+		goto out_free_priv;
+
+	/*
+	 * In particular, the number of devices supported by the LS2K0500
+	 * extended I/O interrupt vector is 128.
+	 */
+	if (of_device_is_compatible(of_node, "loongson,ls2k0500-eiointc"))
+		priv->vec_count = 128;
+	else
+		priv->vec_count = VEC_COUNT;
+
+	priv->node = 0;
+	priv->domain_handle = of_node_to_fwnode(of_node);
+
+	ret = eiointc_init(priv, parent_irq, 0);
+	if (ret < 0)
+		goto out_free_priv;
+
+	return 0;
+
+out_free_priv:
+	kfree(priv);
+	return ret;
+}
+
+IRQCHIP_DECLARE(loongson_ls2k0500_eiointc, "loongson,ls2k0500-eiointc", eiointc_of_init);
+IRQCHIP_DECLARE(loongson_ls2k2000_eiointc, "loongson,ls2k2000-eiointc", eiointc_of_init);
-- 
2.39.1

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

[Krzysztof Kozlowski]

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?

Best regards,
Krzysztof

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

[Binbin Zhou]

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
>
>

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

[Krzysztof Kozlowski]

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

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

[Rob Herring]

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

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

[Binbin Zhou]

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

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

[Krzysztof Kozlowski]

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