[PATCH v1 0/8] Add EcoNet EN751221 MIPS platform support

[PATCH v1 0/8] Add EcoNet EN751221 MIPS platform support

[Caleb James DeLisle]

EcoNet MIPS SoCs are big endian machines based on 34Kc and 1004Kc
processors. They are found in xDSL and xPON modems, and contain PCM
(VoIP), Ethernet, USB, GPIO, I2C, SPI (Flash), UART, and PCIe.

The EcoNet MIPS SoCs are divided broadly into two families, the
EN751221 family based on the 34Kc, and the EN751627 family based on
the 1004Kc. Individual SoCs within a family are roughly the same, but
with different peripherals.

This patchset adds very basic "boots to a console" support for the
EN751221 family. EN751627 support and additional drivers will come in
a future patchset.

Note that while EcoNet is similar to Airoha (AN7513, AN7581) in terms
of peripherals, it is different because Airoha is based on ARM.

This patchset is against mips-next.

Caleb James DeLisle (8):
  dt-bindings: vendor-prefixes: Add EcoNet
  dt-bindings: interrupt-controller: Add EcoNet EN751221 INTC
  irqchip: Add EcoNet EN751221 INTC
  dt-bindings: timer: Add EcoNet HPT CPU Timer
  clocksource/drivers: Add EcoNet Timer HPT driver
  dt-bindings: mips: Add EcoNet platform binding
  mips: Add EcoNet MIPS platform support
  MAINTAINERS: Add EcoNet MIPS platform entry

 .../econet,en751221-intc.yaml                 |  77 +++++
 .../devicetree/bindings/mips/econet.yaml      |  27 ++
 .../bindings/timer/econet,timer-hpt.yaml      |  58 ++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |  12 +
 arch/mips/Kbuild.platforms                    |   1 +
 arch/mips/Kconfig                             |  25 ++
 arch/mips/boot/compressed/uart-16550.c        |   5 +
 arch/mips/boot/dts/Makefile                   |   1 +
 arch/mips/boot/dts/econet/Makefile            |   2 +
 arch/mips/boot/dts/econet/en751221.dtsi       |  62 ++++
 .../boot/dts/econet/en751221_test_image.dts   |  19 ++
 arch/mips/econet/Kconfig                      |  42 +++
 arch/mips/econet/Makefile                     |   2 +
 arch/mips/econet/Platform                     |   5 +
 arch/mips/econet/init.c                       |  78 +++++
 drivers/clocksource/Kconfig                   |   8 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/timer-econet-hpt.c        | 221 ++++++++++++++
 drivers/irqchip/Kconfig                       |   5 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-econet-en751221.c         | 280 ++++++++++++++++++
 22 files changed, 934 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
 create mode 100644 Documentation/devicetree/bindings/mips/econet.yaml
 create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
 create mode 100644 arch/mips/boot/dts/econet/Makefile
 create mode 100644 arch/mips/boot/dts/econet/en751221.dtsi
 create mode 100644 arch/mips/boot/dts/econet/en751221_test_image.dts
 create mode 100644 arch/mips/econet/Kconfig
 create mode 100644 arch/mips/econet/Makefile
 create mode 100644 arch/mips/econet/Platform
 create mode 100644 arch/mips/econet/init.c
 create mode 100644 drivers/clocksource/timer-econet-hpt.c
 create mode 100644 drivers/irqchip/irq-econet-en751221.c

-- 
2.30.2

[PATCH v1 1/8] dt-bindings: vendor-prefixes: Add EcoNet

[Caleb James DeLisle]

Add the "econet" vendor prefix for EcoNet devices, to be used in upcoming
device tree bindings.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 5079ca6ce1d1..4cd050e50743 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -416,6 +416,8 @@ patternProperties:
     description: dServe Technology B.V.
   "^dynaimage,.*":
     description: Dyna-Image
+  "^econet,.*":
+    description: EcoNet (HK) Limited
   "^ea,.*":
     description: Embedded Artists AB
   "^ebang,.*":
-- 
2.30.2

[PATCH v1 2/8] dt-bindings: interrupt-controller: Add EcoNet EN751221 INTC

[Caleb James DeLisle]

Document the device tree binding for the interrupt controller in the
EcoNet EN751221 MIPS SoC.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
If anyone is aware of a standard name for this "shadow interrupt" pattern,
please let me know and I will re-send with updated naming.
---
 .../econet,en751221-intc.yaml                 | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
new file mode 100644
index 000000000000..1b0f262c9630
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/econet,en751221-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: EcoNet EN751221 Interrupt Controller
+
+maintainers:
+  - Caleb James DeLisle <cjd@cjdns.fr>
+
+description: |
+  The EcoNet EN751221 Interrupt Controller is a simple interrupt controller
+  designed for the MIPS 34Kc MT SMP processor with 2 VPEs. Each interrupt can
+  be routed to either VPE but not both, so to support per-CPU interrupts, a
+  secondary IRQ number is allocated to control masking/unmasking on VPE#1. For
+  lack of a better term we call these "shadow interrupts". The assignment of
+  shadow interrupts is defined by the SoC integrator when wiring the interrupt
+  lines, so they are configurable in the device tree.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    const: econet,en751221-intc
+
+  reg:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 1
+
+  interrupt-controller: true
+
+  interrupts:
+    maxItems: 1
+    description: Interrupt line connecting this controller to its parent.
+
+  econet,shadow-interrupts:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: |
+      An array of interrupt number pairs where each pair represents a shadow
+      interrupt relationship. The first number in each pair is the primary IRQ,
+      and the second is its shadow IRQ used for VPE#1 control. For example,
+      <8 3> means IRQ 8 is shadowed by IRQ 3, so IRQ 3 cannot be mapped, but
+      when VPE#1 requests IRQ 8, it will use manipulate the IRQ 3 mask bit.
+    maxItems: 40
+    items:
+      minimum: 0
+      maximum: 40
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - "#interrupt-cells"
+  - interrupt-parent
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    intc: interrupt-controller@1fb40000 {
+        compatible = "econet,en751221-intc";
+        reg = <0x1fb40000 0x100>;
+
+        interrupt-controller;
+        #interrupt-cells = <1>;
+
+        interrupt-parent = <&cpuintc>;
+        interrupts = <2>;
+
+        econet,shadow-interrupts = <7 2>, <8 3>, <13 12>, <30 29>;
+    };
+...
-- 
2.30.2

Re: [PATCH v1 2/8] dt-bindings: interrupt-controller: Add EcoNet EN751221 INTC

[Rob Herring (Arm)]

On Fri, 21 Mar 2025 13:46:27 +0000, Caleb James DeLisle wrote:
> Document the device tree binding for the interrupt controller in the
> EcoNet EN751221 MIPS SoC.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> ---
> If anyone is aware of a standard name for this "shadow interrupt" pattern,
> please let me know and I will re-send with updated naming.
> ---
>  .../econet,en751221-intc.yaml                 | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.example.dtb: interrupt-controller@1fb40000: 'interrupt-parent' is a required property
	from schema $id: http://devicetree.org/schemas/interrupt-controller/econet,en751221-intc.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250321134633.2155141-3-cjd@cjdns.fr

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 2/8] dt-bindings: interrupt-controller: Add EcoNet EN751221 INTC

[Caleb James DeLisle]

On 21/03/2025 16:52, Rob Herring (Arm) wrote:
> On Fri, 21 Mar 2025 13:46:27 +0000, Caleb James DeLisle wrote:
>> Document the device tree binding for the interrupt controller in the
>> EcoNet EN751221 MIPS SoC.
>>
>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>> ---
>> If anyone is aware of a standard name for this "shadow interrupt" pattern,
>> please let me know and I will re-send with updated naming.
>> ---
>>   .../econet,en751221-intc.yaml                 | 77 +++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
>>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.example.dtb: interrupt-controller@1fb40000: 'interrupt-parent' is a required property
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/econet,en751221-intc.yaml#
>

Reproduced and fixed, thank you.

In the interest of limiting mailing list noise, I plan to send a v2 sometime
around Monday, ideally to aggregate a few more reviews in the process.


Thanks,

Caleb

Re: [PATCH v1 2/8] dt-bindings: interrupt-controller: Add EcoNet EN751221 INTC

[Rob Herring]

On Fri, Mar 21, 2025 at 01:46:27PM +0000, Caleb James DeLisle wrote:
> Document the device tree binding for the interrupt controller in the
> EcoNet EN751221 MIPS SoC.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> ---
> If anyone is aware of a standard name for this "shadow interrupt" pattern,
> please let me know and I will re-send with updated naming.
> ---
>  .../econet,en751221-intc.yaml                 | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
> new file mode 100644
> index 000000000000..1b0f262c9630
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/econet,en751221-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EcoNet EN751221 Interrupt Controller
> +
> +maintainers:
> +  - Caleb James DeLisle <cjd@cjdns.fr>
> +
> +description: |

Don't need '|' if no formatting.

> +  The EcoNet EN751221 Interrupt Controller is a simple interrupt controller
> +  designed for the MIPS 34Kc MT SMP processor with 2 VPEs. Each interrupt can
> +  be routed to either VPE but not both, so to support per-CPU interrupts, a
> +  secondary IRQ number is allocated to control masking/unmasking on VPE#1. For
> +  lack of a better term we call these "shadow interrupts". The assignment of
> +  shadow interrupts is defined by the SoC integrator when wiring the interrupt
> +  lines, so they are configurable in the device tree.
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: econet,en751221-intc
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#interrupt-cells":
> +    const: 1
> +
> +  interrupt-controller: true
> +
> +  interrupts:
> +    maxItems: 1
> +    description: Interrupt line connecting this controller to its parent.
> +
> +  econet,shadow-interrupts:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array

Looks like uint32-matrix to me as it pairs of u32's.

> +    description: |
> +      An array of interrupt number pairs where each pair represents a shadow
> +      interrupt relationship. The first number in each pair is the primary IRQ,
> +      and the second is its shadow IRQ used for VPE#1 control. For example,
> +      <8 3> means IRQ 8 is shadowed by IRQ 3, so IRQ 3 cannot be mapped, but
> +      when VPE#1 requests IRQ 8, it will use manipulate the IRQ 3 mask bit.
> +    maxItems: 40
> +    items:
> +      minimum: 0
> +      maximum: 40

Then this would be:

minItems: 1
maxItems: 40
items:
  items:
    - description: primary IRQ
    - description: shadow IRQ

(Feel free to expand the descriptions)

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - interrupt-parent

Generally, interrupt-parent is never required. It can be in a parent 
node for example.

> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    intc: interrupt-controller@1fb40000 {

Drop unused labels (intc).

> +        compatible = "econet,en751221-intc";
> +        reg = <0x1fb40000 0x100>;
> +
> +        interrupt-controller;
> +        #interrupt-cells = <1>;
> +
> +        interrupt-parent = <&cpuintc>;
> +        interrupts = <2>;
> +
> +        econet,shadow-interrupts = <7 2>, <8 3>, <13 12>, <30 29>;
> +    };
> +...
> -- 
> 2.30.2
> 

Re: [PATCH v1 2/8] dt-bindings: interrupt-controller: Add EcoNet EN751221 INTC

[Caleb James DeLisle]

On 21/03/2025 22:17, Rob Herring wrote:
> On Fri, Mar 21, 2025 at 01:46:27PM +0000, Caleb James DeLisle wrote:
>> Document the device tree binding for the interrupt controller in the
>> EcoNet EN751221 MIPS SoC.
>>
>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>> ---
>> If anyone is aware of a standard name for this "shadow interrupt" pattern,
>> please let me know and I will re-send with updated naming.
>> ---
>>   .../econet,en751221-intc.yaml                 | 77 +++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
>> new file mode 100644
>> index 000000000000..1b0f262c9630
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/interrupt-controller/econet,en751221-intc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: EcoNet EN751221 Interrupt Controller
>> +
>> +maintainers:
>> +  - Caleb James DeLisle <cjd@cjdns.fr>
>> +
>> +description: |
> Don't need '|' if no formatting.
Got it, thanks.
>
>> +  The EcoNet EN751221 Interrupt Controller is a simple interrupt controller
>> +  designed for the MIPS 34Kc MT SMP processor with 2 VPEs. Each interrupt can
>> +  be routed to either VPE but not both, so to support per-CPU interrupts, a
>> +  secondary IRQ number is allocated to control masking/unmasking on VPE#1. For
>> +  lack of a better term we call these "shadow interrupts". The assignment of
>> +  shadow interrupts is defined by the SoC integrator when wiring the interrupt
>> +  lines, so they are configurable in the device tree.
>> +
>> +allOf:
>> +  - $ref: /schemas/interrupt-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: econet,en751221-intc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#interrupt-cells":
>> +    const: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Interrupt line connecting this controller to its parent.
>> +
>> +  econet,shadow-interrupts:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> Looks like uint32-matrix to me as it pairs of u32's.
Thanks for the pointer, will update.
>
>> +    description: |
>> +      An array of interrupt number pairs where each pair represents a shadow
>> +      interrupt relationship. The first number in each pair is the primary IRQ,
>> +      and the second is its shadow IRQ used for VPE#1 control. For example,
>> +      <8 3> means IRQ 8 is shadowed by IRQ 3, so IRQ 3 cannot be mapped, but
>> +      when VPE#1 requests IRQ 8, it will use manipulate the IRQ 3 mask bit.
>> +    maxItems: 40
>> +    items:
>> +      minimum: 0
>> +      maximum: 40
> Then this would be:
>
> minItems: 1
> maxItems: 40
> items:
>    items:
>      - description: primary IRQ
>      - description: shadow IRQ
>
> (Feel free to expand the descriptions)
Yes, much nicer.
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupt-controller
>> +  - "#interrupt-cells"
>> +  - interrupt-parent
> Generally, interrupt-parent is never required. It can be in a parent
> node for example.
Removed, thanks.
>
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    intc: interrupt-controller@1fb40000 {
> Drop unused labels (intc).

Ok.


Thank you very much for your time, hope to see you soon in v2.

Thanks,

Caleb


>
>> +        compatible = "econet,en751221-intc";
>> +        reg = <0x1fb40000 0x100>;
>> +
>> +        interrupt-controller;
>> +        #interrupt-cells = <1>;
>> +
>> +        interrupt-parent = <&cpuintc>;
>> +        interrupts = <2>;
>> +
>> +        econet,shadow-interrupts = <7 2>, <8 3>, <13 12>, <30 29>;
>> +    };
>> +...
>> -- 
>> 2.30.2
>>

[PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Caleb James DeLisle]

Add a driver for the interrupt controller in the EcoNet EN751221 MIPS SoC.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this
device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU
is not prepared to handle. If anybody knows how to either disable this
behavior, or handle vectored interrupts without ugly code that breaks
cascading, please let me know and I will implement that and add
MIPS_MT_SMP in a future patchset.
---
 drivers/irqchip/Kconfig               |   5 +
 drivers/irqchip/Makefile              |   1 +
 drivers/irqchip/irq-econet-en751221.c | 280 ++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/irqchip/irq-econet-en751221.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c11b9965c4ad..a591ad3156dc 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -147,6 +147,11 @@ config DW_APB_ICTL
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN_HIERARCHY
 
+config ECONET_EN751221_INTC
+	bool
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
+
 config FARADAY_FTINTC010
 	bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 25e9ad29b8c4..1ee83823928d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
 obj-$(CONFIG_ARCH_ACTIONS)		+= irq-owl-sirq.o
 obj-$(CONFIG_DAVINCI_CP_INTC)		+= irq-davinci-cp-intc.o
 obj-$(CONFIG_EXYNOS_IRQ_COMBINER)	+= exynos-combiner.o
+obj-$(CONFIG_ECONET_EN751221_INTC)	+= irq-econet-en751221.o
 obj-$(CONFIG_FARADAY_FTINTC010)		+= irq-ftintc010.o
 obj-$(CONFIG_ARCH_HIP04)		+= irq-hip04.o
 obj-$(CONFIG_ARCH_LPC32XX)		+= irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-econet-en751221.c b/drivers/irqchip/irq-econet-en751221.c
new file mode 100644
index 000000000000..edbb8a3d6d51
--- /dev/null
+++ b/drivers/irqchip/irq-econet-en751221.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * EN751221 Interrupt Controller Driver.
+ *
+ * Copyright (C) 2025 Caleb James DeLisle <cjd@cjdns.fr>
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+
+#define INTC_IRQ_COUNT		40
+
+#define INTC_NO_SHADOW		0xff
+#define INTC_IS_SHADOW		0xfe
+
+#define REG_MASK0		0x04
+#define REG_MASK1		0x50
+#define REG_PENDING0		0x08
+#define REG_PENDING1		0x54
+
+static const struct econet_intc {
+	const struct irq_chip chip;
+
+	const struct irq_domain_ops domain_ops;
+} econet_intc;
+
+static struct {
+	void __iomem *membase;
+	u8 shadow_interrupts[INTC_IRQ_COUNT];
+} econet_intc_rai __ro_after_init;
+
+static DEFINE_RAW_SPINLOCK(irq_lock);
+
+static void econet_wreg(u32 reg, u32 val, u32 mask)
+{
+	unsigned long flags;
+	u32 v;
+
+	raw_spin_lock_irqsave(&irq_lock, flags);
+
+	v = ioread32(econet_intc_rai.membase + reg);
+	v &= ~mask;
+	v |= val & mask;
+	iowrite32(v, econet_intc_rai.membase + reg);
+
+	raw_spin_unlock_irqrestore(&irq_lock, flags);
+}
+
+static void econet_chmask(u32 hwirq, bool unmask)
+{
+	u32 reg;
+	u32 mask;
+	u32 bit;
+	u8 shadow;
+
+	shadow = econet_intc_rai.shadow_interrupts[hwirq];
+	if (WARN_ON_ONCE(shadow == INTC_IS_SHADOW))
+		return;
+	else if (shadow < INTC_NO_SHADOW && smp_processor_id() > 0)
+		hwirq = shadow;
+
+	if (hwirq >= 32) {
+		reg = REG_MASK1;
+		mask = BIT(hwirq - 32);
+	} else {
+		reg = REG_MASK0;
+		mask = BIT(hwirq);
+	}
+	bit = (unmask) ? mask : 0;
+
+	econet_wreg(reg, bit, mask);
+}
+
+static void econet_intc_mask(struct irq_data *d)
+{
+	econet_chmask(d->hwirq, false);
+}
+
+static void econet_intc_unmask(struct irq_data *d)
+{
+	econet_chmask(d->hwirq, true);
+}
+
+static void econet_mask_all(void)
+{
+	econet_wreg(REG_MASK0, 0, ~0);
+	econet_wreg(REG_MASK1, 0, ~0);
+}
+
+static void econet_intc_handle_pending(struct irq_domain *d, u32 pending, u32 offset)
+{
+	int hwirq;
+
+	while (pending) {
+		hwirq = fls(pending) - 1;
+		generic_handle_domain_irq(d, hwirq + offset);
+		pending &= ~BIT(hwirq);
+	}
+}
+
+static void econet_intc_from_parent(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irq_domain *domain;
+	u32 pending0;
+	u32 pending1;
+
+	chained_irq_enter(chip, desc);
+
+	pending0 = ioread32(econet_intc_rai.membase + REG_PENDING0);
+	pending1 = ioread32(econet_intc_rai.membase + REG_PENDING1);
+
+	if (unlikely(!(pending0 | pending1))) {
+		spurious_interrupt();
+		goto out;
+	}
+
+	domain = irq_desc_get_handler_data(desc);
+
+	econet_intc_handle_pending(domain, pending0, 0);
+	econet_intc_handle_pending(domain, pending1, 32);
+
+out:
+	chained_irq_exit(chip, desc);
+}
+
+static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq)
+{
+	int ret;
+
+	if (hwirq >= INTC_IRQ_COUNT) {
+		pr_err("%s: hwirq %lu out of range\n", __func__, hwirq);
+		return -EINVAL;
+	} else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) {
+		pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n",
+		       __func__, hwirq);
+		return -EINVAL;
+	}
+	if (econet_intc_rai.shadow_interrupts[hwirq] != INTC_NO_SHADOW) {
+		irq_set_chip_and_handler(
+			irq, &econet_intc.chip, handle_percpu_devid_irq);
+		ret = irq_set_percpu_devid(irq);
+		if (ret) {
+			pr_warn("%s: Failed irq_set_percpu_devid for %u: %d\n",
+				d->name, irq, ret);
+		}
+	} else {
+		irq_set_chip_and_handler(
+			irq, &econet_intc.chip, handle_level_irq);
+	}
+	irq_set_chip_data(irq, NULL);
+	return 0;
+}
+
+static const struct econet_intc econet_intc = {
+	.chip = {
+		.name		= "en751221-intc",
+		.irq_unmask	= econet_intc_unmask,
+		.irq_mask	= econet_intc_mask,
+		.irq_mask_ack	= econet_intc_mask,
+	},
+	.domain_ops = {
+		.xlate = irq_domain_xlate_onecell,
+		.map = econet_intc_map,
+	},
+};
+
+static int __init get_shadow_interrupts(struct device_node *node)
+{
+	const char *field = "econet,shadow-interrupts";
+	int n_shadow_interrupts;
+	u32 *shadow_interrupts;
+
+	n_shadow_interrupts = of_property_count_u32_elems(node, field);
+	memset(econet_intc_rai.shadow_interrupts, INTC_NO_SHADOW,
+	       sizeof(econet_intc_rai.shadow_interrupts));
+	if (n_shadow_interrupts <= 0) {
+		return 0;
+	} else if (n_shadow_interrupts % 2) {
+		pr_err("%pOF: %s count is odd, ignoring\n", node, field);
+		return 0;
+	}
+	shadow_interrupts = kmalloc_array(n_shadow_interrupts, sizeof(u32),
+					  GFP_KERNEL);
+	if (!shadow_interrupts)
+		return -ENOMEM;
+	if (of_property_read_u32_array(node, field,
+				       shadow_interrupts, n_shadow_interrupts)
+	) {
+		pr_err("%pOF: Failed to read %s\n", node, field);
+		kfree(shadow_interrupts);
+		return -EINVAL;
+	}
+	for (int i = 0; i < n_shadow_interrupts; i += 2) {
+		u32 shadow = shadow_interrupts[i + 1];
+		u32 target = shadow_interrupts[i];
+
+		if (shadow > INTC_IRQ_COUNT) {
+			pr_err("%pOF: %s[%d] shadow(%d) out of range\n",
+			       node, field, i, shadow);
+			continue;
+		}
+		if (target >= INTC_IRQ_COUNT) {
+			pr_err("%pOF: %s[%d] target(%d) out of range\n",
+			       node, field, i + 1, target);
+			continue;
+		}
+		econet_intc_rai.shadow_interrupts[target] = shadow;
+		econet_intc_rai.shadow_interrupts[shadow] = INTC_IS_SHADOW;
+	}
+	kfree(shadow_interrupts);
+	return 0;
+}
+
+static int __init econet_intc_of_init(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+	int irq;
+	struct resource res;
+	struct irq_domain *domain;
+
+	ret = get_shadow_interrupts(node);
+	if (ret)
+		return ret;
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq) {
+		pr_err("%pOF: DT: Failed to get IRQ from 'interrupts'\n", node);
+		return -EINVAL;
+	}
+
+	if (of_address_to_resource(node, 0, &res)) {
+		pr_err("%pOF: DT: Failed to get 'reg'\n", node);
+		ret = -EINVAL;
+		goto err_dispose_mapping;
+	}
+
+	if (!request_mem_region(res.start, resource_size(&res), res.name)) {
+		pr_err("%pOF: Failed to request memory\n", node);
+		ret = -EBUSY;
+		goto err_dispose_mapping;
+	}
+
+	econet_intc_rai.membase = ioremap(res.start, resource_size(&res));
+	if (!econet_intc_rai.membase) {
+		pr_err("%pOF: Failed to remap membase\n", node);
+		ret = -ENOMEM;
+		goto err_release;
+	}
+
+	econet_mask_all();
+
+	domain = irq_domain_add_linear(
+		node, INTC_IRQ_COUNT,
+		&econet_intc.domain_ops, NULL);
+	if (!domain) {
+		pr_err("%pOF: Failed to add irqdomain\n", node);
+		ret = -ENOMEM;
+		goto err_unmap;
+	}
+
+	irq_set_chained_handler_and_data(irq, econet_intc_from_parent, domain);
+
+	return 0;
+
+err_unmap:
+	iounmap(econet_intc_rai.membase);
+err_release:
+	release_mem_region(res.start, resource_size(&res));
+err_dispose_mapping:
+	irq_dispose_mapping(irq);
+	return ret;
+}
+
+IRQCHIP_DECLARE(econet_en751221_intc, "econet,en751221-intc", econet_intc_of_init);
-- 
2.30.2

Re: [PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Thomas Gleixner]

Caleb!

On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote:
> ---
> If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this
> device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU
> is not prepared to handle. If anybody knows how to either disable this
> behavior, or handle vectored interrupts without ugly code that breaks
> cascading, please let me know and I will implement that and add
> MIPS_MT_SMP in a future patchset.

This must be addressed before this driver can be merged, but that's a
topic for the MIPS wizards and out of my area of expertise, except for
the obvious:

    For a start you can exclude this platform from being enabled in
    Kconfig when the EI/VI muck is enabled. That's what 'depends on' is
    for,

So this patch clearly should have been tagged with 'RFC'.

> +static const struct econet_intc {
> +	const struct irq_chip chip;
> +
> +	const struct irq_domain_ops domain_ops;
> +} econet_intc;

Please see
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Aside of the coding style issues, what's the actual value of this
struct? Is there anything which can't be done with:

static const struct irq_chip econet_chip = {
	.name	= ....
};

static const struct irq_domain_ops econet_domain_ops = {
	.xlate	= ....
};

Which avoids the above forward struct declaration completely and does
not need any forward declaration at all, neither for the chip nor for
the domain.

> +static struct {
> +	void __iomem *membase;
> +	u8 shadow_interrupts[INTC_IRQ_COUNT];
> +} econet_intc_rai __ro_after_init;
> +
> +static DEFINE_RAW_SPINLOCK(irq_lock);
> +
> +static void econet_wreg(u32 reg, u32 val, u32 mask)
> +{
> +	unsigned long flags;
> +	u32 v;
> +
> +	raw_spin_lock_irqsave(&irq_lock, flags);

Please use

       guard(raw_spinlock)(&irq_lock);

You don't need irqsave when invoked from mask/unmask as the caller
guarantees to have interrupts disabled. Then you only need to disable
interrupts across the invocation from mask_all().

> +
> +	v = ioread32(econet_intc_rai.membase + reg);
> +	v &= ~mask;
> +	v |= val & mask;
> +	iowrite32(v, econet_intc_rai.membase + reg);
> +
> +	raw_spin_unlock_irqrestore(&irq_lock, flags);
> +}
> +
> +static void econet_chmask(u32 hwirq, bool unmask)
> +{
> +	u32 reg;
> +	u32 mask;
> +	u32 bit;
> +	u8 shadow;

Search the same document for local variables.

> +	shadow = econet_intc_rai.shadow_interrupts[hwirq];
> +	if (WARN_ON_ONCE(shadow == INTC_IS_SHADOW))
> +		return;
> +	else if (shadow < INTC_NO_SHADOW && smp_processor_id() > 0)
> +		hwirq = shadow;

This is completely undocumented voodoo. Please add comments which
explain this properly.

> +	if (hwirq >= 32) {
> +		reg = REG_MASK1;
> +		mask = BIT(hwirq - 32);
> +	} else {
> +		reg = REG_MASK0;
> +		mask = BIT(hwirq);
> +	}
> +	bit = (unmask) ? mask : 0;
> +	econet_wreg(reg, bit, mask);

        econet_wreg(reg, unmask ? mask : 0, mask);

> +}
> +
> +static void econet_intc_mask(struct irq_data *d)
> +{
> +	econet_chmask(d->hwirq, false);
> +}
> +
> +static void econet_intc_unmask(struct irq_data *d)
> +{
> +	econet_chmask(d->hwirq, true);
> +}
> +
> +static void econet_mask_all(void)
> +{

with a

     guard(irq)();

added here you spare the irqsave in the write function.

> +static void econet_intc_from_parent(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct irq_domain *domain;
> +	u32 pending0;
> +	u32 pending1;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	pending0 = ioread32(econet_intc_rai.membase + REG_PENDING0);
> +	pending1 = ioread32(econet_intc_rai.membase + REG_PENDING1);
> +
> +	if (unlikely(!(pending0 | pending1))) {
> +		spurious_interrupt();
> +		goto out;
> +	}
> +
> +	domain = irq_desc_get_handler_data(desc);
> +
> +	econet_intc_handle_pending(domain, pending0, 0);
> +	econet_intc_handle_pending(domain, pending1, 32);

	if (likely(pending0 | pending1) {
             domain = ...
             ...
        } else {
             spurious_interrupt();
        }

Makes the goto go away _and_ sets the focus on the likely path and not
on the visual clutter of the unlikely one.

> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq)
> +{
> +	int ret;
> +
> +	if (hwirq >= INTC_IRQ_COUNT) {
> +		pr_err("%s: hwirq %lu out of range\n", __func__, hwirq);
> +		return -EINVAL;
> +	} else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) {
> +		pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n",
> +		       __func__, hwirq);

No newline

> +		return -EINVAL;
> +	}

Please put a newline here for readability instead.

> +	if (econet_intc_rai.shadow_interrupts[hwirq] != INTC_NO_SHADOW) {

This INTC_IS_SHADOW and INTC_NO_SHADOW logic is beyond confusing without
comments. Three month down the road you will ask yourself what the hell
this means.

> +		irq_set_chip_and_handler(
> +			irq, &econet_intc.chip, handle_percpu_devid_irq);

This line break is unreadable. See documentation.

If at all this wants to be:

		irq_set_chip_and_handler(irq, &econet_intc.chip,
                                         handle_percpu_devid_irq);

But this fits nicely within 100 characters, so get rid of it completely.

> +		ret = irq_set_percpu_devid(irq);

And please add a comment which explains why this magic shadow thing maps
to percpu devid interrupts.

> +		if (ret) {
> +			pr_warn("%s: Failed irq_set_percpu_devid for %u: %d\n",
> +				d->name, irq, ret);
> +		}
> +	} else {
> +		irq_set_chip_and_handler(
> +			irq, &econet_intc.chip, handle_level_irq);

Same here.

> +	}
> +	irq_set_chip_data(irq, NULL);
> +	return 0;
> +}
> +
> +static const struct econet_intc econet_intc = {
> +	.chip = {
> +		.name		= "en751221-intc",
> +		.irq_unmask	= econet_intc_unmask,
> +		.irq_mask	= econet_intc_mask,
> +		.irq_mask_ack	= econet_intc_mask,
> +	},
> +	.domain_ops = {
> +		.xlate = irq_domain_xlate_onecell,
> +		.map = econet_intc_map,

See documention.

> +	},
> +};
> +
> +static int __init get_shadow_interrupts(struct device_node *node)
> +{
> +	const char *field = "econet,shadow-interrupts";
> +	int n_shadow_interrupts;
> +	u32 *shadow_interrupts;
> +
> +	n_shadow_interrupts = of_property_count_u32_elems(node, field);
> +	memset(econet_intc_rai.shadow_interrupts, INTC_NO_SHADOW,
> +	       sizeof(econet_intc_rai.shadow_interrupts));
> +	if (n_shadow_interrupts <= 0) {
> +		return 0;
> +	} else if (n_shadow_interrupts % 2) {
> +		pr_err("%pOF: %s count is odd, ignoring\n", node, field);
> +		return 0;
> +	}
> +	shadow_interrupts = kmalloc_array(n_shadow_interrupts, sizeof(u32),
> +					  GFP_KERNEL);

	u32 *shadow_interrupts __free(kfree) =
        	kmalloc_array(n_shadow_interrupts, sizeof(u32), GFP_KERNEL);                                  

Then the return paths don't have to care about this allocation at all.

> +	if (!shadow_interrupts)
> +		return -ENOMEM;
> +	if (of_property_read_u32_array(node, field,
> +				       shadow_interrupts, n_shadow_interrupts)
> +	) {

Your random choices of coding style and lack of visual seperation by
empty newlines really make this hard to digest.

> +		pr_err("%pOF: Failed to read %s\n", node, field);
> +		kfree(shadow_interrupts);
> +		return -EINVAL;
> +	}

The __free() above will reduce this to

	if (of_property_read_u32_array(node, field, shadow_interrupts, n_shadow_interrupts)) {
		pr_err("%pOF: Failed to read %s\n", node, field);
                return -EINVAL;
	}

and removes the kfree() at the end of the function.

> +	for (int i = 0; i < n_shadow_interrupts; i += 2) {
> +		u32 shadow = shadow_interrupts[i + 1];
> +		u32 target = shadow_interrupts[i];
> +
> +		if (shadow > INTC_IRQ_COUNT) {
> +			pr_err("%pOF: %s[%d] shadow(%d) out of range\n",
> +			       node, field, i, shadow);

No line break.

> +			continue;
> +		}

Newline

> +		if (target >= INTC_IRQ_COUNT) {
> +			pr_err("%pOF: %s[%d] target(%d) out of range\n",
> +			       node, field, i + 1, target);

No line break.

> +			continue;
> +		}
> +		econet_intc_rai.shadow_interrupts[target] = shadow;
> +		econet_intc_rai.shadow_interrupts[shadow] = INTC_IS_SHADOW;

What the heck does any of this mean? This whole shadow magic is
hideously incomprehensible. It's amazing that this whole file does not
contain a single line of comment.

Aside of that how is any of this sanity checked, i.e. so that there are
no existing entries overwritten? I assume this blindly relies on the
device tree being correct. Fine, but then please document it.

> +	}
> +	kfree(shadow_interrupts);
> +	return 0;
> +}
> +
> +static int __init econet_intc_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +	int irq;
> +	struct resource res;
> +	struct irq_domain *domain;

Sigh.

> +
> +	domain = irq_domain_add_linear(
> +		node, INTC_IRQ_COUNT,
> +		&econet_intc.domain_ops, NULL);

Finally my eyes bleed and my mental code pattern matching engine threw a
garbage-overload exception.

Seriously. Consistent coding style _and_ comments explaining the
non-obvious parts of the code are not optional.

You want me and others to review your code, so please have the courtesy
to provide it in a digestable form.

That spares us to point out the obvious, which can be looked up in
documentation, and the frustration of staring at incomprehensible
undocumented logic. And it spares you the frustration of getting your
submission ripped into bits and pieces.

Thanks,

        tglx

Re: [PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Caleb James DeLisle]

Thank you for this review.

On 21/03/2025 21:26, Thomas Gleixner wrote:
> Caleb!
>
> On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote:
>> ---
>> If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this
>> device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU
>> is not prepared to handle. If anybody knows how to either disable this
>> behavior, or handle vectored interrupts without ugly code that breaks
>> cascading, please let me know and I will implement that and add
>> MIPS_MT_SMP in a future patchset.
> This must be addressed before this driver can be merged, but that's a
> topic for the MIPS wizards and out of my area of expertise, except for
> the obvious:
>
>      For a start you can exclude this platform from being enabled in
>      Kconfig when the EI/VI muck is enabled. That's what 'depends on' is
>      for,


Maybe my message was misleading everything has been tested and works correctly
on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering
this question will allow me to enable them, thus also getting MIPS_MT_SMP.

I could look at forbidding them in the driver, but I'm not sure that's appropriate as this
seems like more of an SoC issue than an INTC issue. But I'll follow your guidance.


>
> So this patch clearly should have been tagged with 'RFC'.

Given the patchset works correctly in testing, does this comment stand?

>
>> +static const struct econet_intc {
>> +	const struct irq_chip chip;
>> +
>> +	const struct irq_domain_ops domain_ops;
>> +} econet_intc;
> Please see
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Thank you, I've been reading a lot of documentation but did not find that one.

>
> Aside of the coding style issues, what's the actual value of this
> struct? Is there anything which can't be done with:
>
> static const struct irq_chip econet_chip = {
> 	.name	= ....
> };
>
> static const struct irq_domain_ops econet_domain_ops = {
> 	.xlate	= ....
> };
>
> Which avoids the above forward struct declaration completely and does
> not need any forward declaration at all, neither for the chip nor for
> the domain.

My normal instinct lead me to minimize symbols on the top level scope and I
didn't realize I was doing it and should check what is typical in kernel code.
Will refactor.

>
>> +static struct {
>> +	void __iomem *membase;
>> +	u8 shadow_interrupts[INTC_IRQ_COUNT];
>> +} econet_intc_rai __ro_after_init;
>> +
>> +static DEFINE_RAW_SPINLOCK(irq_lock);
>> +
>> +static void econet_wreg(u32 reg, u32 val, u32 mask)
>> +{
>> +	unsigned long flags;
>> +	u32 v;
>> +
>> +	raw_spin_lock_irqsave(&irq_lock, flags);
> Please use
>
>         guard(raw_spinlock)(&irq_lock);
>
> You don't need irqsave when invoked from mask/unmask as the caller
> guarantees to have interrupts disabled. Then you only need to disable
> interrupts across the invocation from mask_all().
Thank you very much, I would not have thought of this.
>
>> +
>> +	v = ioread32(econet_intc_rai.membase + reg);
>> +	v &= ~mask;
>> +	v |= val & mask;
>> +	iowrite32(v, econet_intc_rai.membase + reg);
>> +
>> +	raw_spin_unlock_irqrestore(&irq_lock, flags);
>> +}
>> +
>> +static void econet_chmask(u32 hwirq, bool unmask)
>> +{
>> +	u32 reg;
>> +	u32 mask;
>> +	u32 bit;
>> +	u8 shadow;
> Search the same document for local variables.
Ok
>
>> +	shadow = econet_intc_rai.shadow_interrupts[hwirq];
>> +	if (WARN_ON_ONCE(shadow == INTC_IS_SHADOW))
>> +		return;
>> +	else if (shadow < INTC_NO_SHADOW && smp_processor_id() > 0)
>> +		hwirq = shadow;
> This is completely undocumented voodoo. Please add comments which
> explain this properly.

Sure thing, I often write and then remove comments because I don't want to be
judged for over-explaining. In fact, I love explaining.

>
>> +	if (hwirq >= 32) {
>> +		reg = REG_MASK1;
>> +		mask = BIT(hwirq - 32);
>> +	} else {
>> +		reg = REG_MASK0;
>> +		mask = BIT(hwirq);
>> +	}
>> +	bit = (unmask) ? mask : 0;
>> +	econet_wreg(reg, bit, mask);
>          econet_wreg(reg, unmask ? mask : 0, mask);
Ok
>
>> +}
>> +
>> +static void econet_intc_mask(struct irq_data *d)
>> +{
>> +	econet_chmask(d->hwirq, false);
>> +}
>> +
>> +static void econet_intc_unmask(struct irq_data *d)
>> +{
>> +	econet_chmask(d->hwirq, true);
>> +}
>> +
>> +static void econet_mask_all(void)
>> +{
> with a
>
>       guard(irq)();
>
> added here you spare the irqsave in the write function.
Ok, thanks.
>
>> +static void econet_intc_from_parent(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	struct irq_domain *domain;
>> +	u32 pending0;
>> +	u32 pending1;
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	pending0 = ioread32(econet_intc_rai.membase + REG_PENDING0);
>> +	pending1 = ioread32(econet_intc_rai.membase + REG_PENDING1);
>> +
>> +	if (unlikely(!(pending0 | pending1))) {
>> +		spurious_interrupt();
>> +		goto out;
>> +	}
>> +
>> +	domain = irq_desc_get_handler_data(desc);
>> +
>> +	econet_intc_handle_pending(domain, pending0, 0);
>> +	econet_intc_handle_pending(domain, pending1, 32);
> 	if (likely(pending0 | pending1) {
>               domain = ...
>               ...
>          } else {
>               spurious_interrupt();
>          }
>
> Makes the goto go away _and_ sets the focus on the likely path and not
> on the visual clutter of the unlikely one.
Indeed, will fix.
>
>> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq)
>> +{
>> +	int ret;
>> +
>> +	if (hwirq >= INTC_IRQ_COUNT) {
>> +		pr_err("%s: hwirq %lu out of range\n", __func__, hwirq);
>> +		return -EINVAL;
>> +	} else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) {
>> +		pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n",
>> +		       __func__, hwirq);
> No newline
If I understand correctly, you prefer:
.....interrupt\n", __func__, hwirq);
for a 96 char line?
>
>> +		return -EINVAL;
>> +	}
> Please put a newline here for readability instead.
Sure thing
>
>> +	if (econet_intc_rai.shadow_interrupts[hwirq] != INTC_NO_SHADOW) {
> This INTC_IS_SHADOW and INTC_NO_SHADOW logic is beyond confusing without
> comments. Three month down the road you will ask yourself what the hell
> this means.
I'll add comments and also try to make the code a little more self-evident in v2.
>
>> +		irq_set_chip_and_handler(
>> +			irq, &econet_intc.chip, handle_percpu_devid_irq);
> This line break is unreadable. See documentation.
>
> If at all this wants to be:
>
> 		irq_set_chip_and_handler(irq, &econet_intc.chip,
>                                           handle_percpu_devid_irq);
>
> But this fits nicely within 100 characters, so get rid of it completely.
My apologies. It was my intention, but instincts are sneaky and for some reason
checkpatch didn't catch it.
>
>> +		ret = irq_set_percpu_devid(irq);
> And please add a comment which explains why this magic shadow thing maps
> to percpu devid interrupts.
Sure thing
>
>> +		if (ret) {
>> +			pr_warn("%s: Failed irq_set_percpu_devid for %u: %d\n",
>> +				d->name, irq, ret);
>> +		}
>> +	} else {
>> +		irq_set_chip_and_handler(
>> +			irq, &econet_intc.chip, handle_level_irq);
> Same here.
Ok.
>
>> +	}
>> +	irq_set_chip_data(irq, NULL);
>> +	return 0;
>> +}
>> +
>> +static const struct econet_intc econet_intc = {
>> +	.chip = {
>> +		.name		= "en751221-intc",
>> +		.irq_unmask	= econet_intc_unmask,
>> +		.irq_mask	= econet_intc_mask,
>> +		.irq_mask_ack	= econet_intc_mask,
>> +	},
>> +	.domain_ops = {
>> +		.xlate = irq_domain_xlate_onecell,
>> +		.map = econet_intc_map,
> See documention.
I suppose this is tab alignment, but I will in any case make a point
of reading it all carefully.
>
>> +	},
>> +};
>> +
>> +static int __init get_shadow_interrupts(struct device_node *node)
>> +{
>> +	const char *field = "econet,shadow-interrupts";
>> +	int n_shadow_interrupts;
>> +	u32 *shadow_interrupts;
>> +
>> +	n_shadow_interrupts = of_property_count_u32_elems(node, field);
>> +	memset(econet_intc_rai.shadow_interrupts, INTC_NO_SHADOW,
>> +	       sizeof(econet_intc_rai.shadow_interrupts));
>> +	if (n_shadow_interrupts <= 0) {
>> +		return 0;
>> +	} else if (n_shadow_interrupts % 2) {
>> +		pr_err("%pOF: %s count is odd, ignoring\n", node, field);
>> +		return 0;
>> +	}
>> +	shadow_interrupts = kmalloc_array(n_shadow_interrupts, sizeof(u32),
>> +					  GFP_KERNEL);
> 	u32 *shadow_interrupts __free(kfree) =
>          	kmalloc_array(n_shadow_interrupts, sizeof(u32), GFP_KERNEL);
>
> Then the return paths don't have to care about this allocation at all.
Very nice, thank you.
>
>> +	if (!shadow_interrupts)
>> +		return -ENOMEM;
>> +	if (of_property_read_u32_array(node, field,
>> +				       shadow_interrupts, n_shadow_interrupts)
>> +	) {
> Your random choices of coding style and lack of visual seperation by
> empty newlines really make this hard to digest.
Apologies, will restructure.
>
>> +		pr_err("%pOF: Failed to read %s\n", node, field);
>> +		kfree(shadow_interrupts);
>> +		return -EINVAL;
>> +	}
> The __free() above will reduce this to
>
> 	if (of_property_read_u32_array(node, field, shadow_interrupts, n_shadow_interrupts)) {
> 		pr_err("%pOF: Failed to read %s\n", node, field);
>                  return -EINVAL;
> 	}
>
> and removes the kfree() at the end of the function.
Yes, I wasn't aware of __free() until now. Generally I love this type of thing
and use them wherever possible.
>
>> +	for (int i = 0; i < n_shadow_interrupts; i += 2) {
>> +		u32 shadow = shadow_interrupts[i + 1];
>> +		u32 target = shadow_interrupts[i];
>> +
>> +		if (shadow > INTC_IRQ_COUNT) {
>> +			pr_err("%pOF: %s[%d] shadow(%d) out of range\n",
>> +			       node, field, i, shadow);
> No line break.
Ok
>
>> +			continue;
>> +		}
> Newline
Ok (I will check for tightly packed if statements and fix all)
>
>> +		if (target >= INTC_IRQ_COUNT) {
>> +			pr_err("%pOF: %s[%d] target(%d) out of range\n",
>> +			       node, field, i + 1, target);
> No line break.
Ok
>
>> +			continue;
>> +		}
>> +		econet_intc_rai.shadow_interrupts[target] = shadow;
>> +		econet_intc_rai.shadow_interrupts[shadow] = INTC_IS_SHADOW;
> What the heck does any of this mean? This whole shadow magic is
> hideously incomprehensible. It's amazing that this whole file does not
> contain a single line of comment.
>
> Aside of that how is any of this sanity checked, i.e. so that there are
> no existing entries overwritten? I assume this blindly relies on the
> device tree being correct. Fine, but then please document it.
I had a nice comment on the oddities of these "shadow interrupts" which I
then moved into the DT binding and removed from the source. I'll happily
add it back and more.

As I said, I actually like explaining, I perhaps erroneously tried to match the
terseness of other kernel drivers.
>
>> +	}
>> +	kfree(shadow_interrupts);
>> +	return 0;
>> +}
>> +
>> +static int __init econet_intc_of_init(struct device_node *node, struct device_node *parent)
>> +{
>> +	int ret;
>> +	int irq;
>> +	struct resource res;
>> +	struct irq_domain *domain;
> Sigh.
Ok I see, reverse fir tree order.
>
>> +
>> +	domain = irq_domain_add_linear(
>> +		node, INTC_IRQ_COUNT,
>> +		&econet_intc.domain_ops, NULL);
> Finally my eyes bleed and my mental code pattern matching engine threw a
> garbage-overload exception.
>
> Seriously. Consistent coding style _and_ comments explaining the
> non-obvious parts of the code are not optional.
>
> You want me and others to review your code, so please have the courtesy
> to provide it in a digestable form.
>
> That spares us to point out the obvious, which can be looked up in
> documentation, and the frustration of staring at incomprehensible
> undocumented logic. And it spares you the frustration of getting your
> submission ripped into bits and pieces.

In case of any doubt, I wasn't trying to sneak bad code past you. I read a lot of
documentation, though clearly not enough / the right stuff.
When I sent this, I didn't know what was left that could be improved.


I'm going to read the documentation you linked, then re-read all of this patchset
with your comments in mind, and hopefully come back with something much
better. Lastly, I very much appreciate your taking the time.

Thanks,
Caleb

>
> Thanks,
>
>          tglx
>
>

Re: [PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Thomas Gleixner]

On Fri, Mar 21 2025 at 23:20, Caleb James DeLisle wrote:
> On 21/03/2025 21:26, Thomas Gleixner wrote:
>> Caleb!
>>
>> On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote:
>>> ---
>>> If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this
>>> device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU
>>> is not prepared to handle. If anybody knows how to either disable this
>>> behavior, or handle vectored interrupts without ugly code that breaks
>>> cascading, please let me know and I will implement that and add
>>> MIPS_MT_SMP in a future patchset.
>> This must be addressed before this driver can be merged, but that's a
>> topic for the MIPS wizards and out of my area of expertise, except for
>> the obvious:
>>
>>      For a start you can exclude this platform from being enabled in
>>      Kconfig when the EI/VI muck is enabled. That's what 'depends on' is
>>      for,
>
> Maybe my message was misleading everything has been tested and works correctly
> on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering
> this question will allow me to enable them, thus also getting
> MIPS_MT_SMP.

It does not select it, but it can be enabled independently or through
some other magic config knob, right? And if it gets enabled, then it
does not work, right?

> I could look at forbidding them in the driver, but I'm not sure that's
> appropriate as this seems like more of an SoC issue than an INTC
> issue. But I'll follow your guidance.

What's not appropriate? If it does not work, then it's very appropriate
to do

   config ECONET
          depends on !EI && !VI

on the principle of least surprise, no?

>> So this patch clearly should have been tagged with 'RFC'.
>
> Given the patchset works correctly in testing, does this comment
> stand?

Until the EI/VI issue is resolved so that it either works or cannot
happen.

>>> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (hwirq >= INTC_IRQ_COUNT) {
>>> +		pr_err("%s: hwirq %lu out of range\n", __func__, hwirq);
>>> +		return -EINVAL;
>>> +	} else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) {
>>> +		pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n",
>>> +		       __func__, hwirq);
>> No newline
> If I understand correctly, you prefer:
> .....interrupt\n", __func__, hwirq);
> for a 96 char line?

You have 100 characters in drivers/irqchip/

>>> +	.domain_ops = {
>>> +		.xlate = irq_domain_xlate_onecell,
>>> +		.map = econet_intc_map,
>> See documention.
> I suppose this is tab alignment, but I will in any case make a point
> of reading it all carefully.

Yes. The aligned tabular view is way simpler to read and parse. Reading
is based on pattern recognition. Irregular patterns disturb the reading
flow, which means the focus is shifted from understanding to decoding
the irregular pattern.

> In case of any doubt, I wasn't trying to sneak bad code past you.

I did not assume malice here at all.

Thanks,

        tglx

Re: [PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Caleb James DeLisle]

[snip]
>> Maybe my message was misleading everything has been tested and works correctly
>> on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering
>> this question will allow me to enable them, thus also getting
>> MIPS_MT_SMP.
> It does not select it, but it can be enabled independently or through
> some other magic config knob, right? And if it gets enabled, then it
> does not work, right?

Not really true on either point.

Firstly, it's not something you can select in the kernel menuconfig, it's
selected by the SoC or some core feature of the SoC (e.g. SMP_MT_MIPS).

Secondly it does work, it's just that it does what it says it does, and you
need to handle those vectored interrupts - no irqchip driver does this.

>> I could look at forbidding them in the driver, but I'm not sure that's
>> appropriate as this seems like more of an SoC issue than an INTC
>> issue. But I'll follow your guidance.
> What's not appropriate? If it does not work, then it's very appropriate
> to do
>
>     config ECONET
>            depends on !EI && !VI
>
> on the principle of least surprise, no?

I've spent quite a bit of time studying this, so with respect for your time,
let me try to give you a brief summary of what I know and why I
submitted this as I did:

1. EI/VI is supported by the intc but it's really a feature of MIPS32r2.
In MIPS32r2, the CPU<->intc wire interface allows the CPU to send its
interrupts to the intc, and then allows the intc to fire any of of up to
64 interrupts back to the CPU.

2. When enabled, the CPU's internal intc sends its 7 interrupts to the
external intc who prioritizes them, renumbers them, and sends them
back along with their own.

3. When they come back, the CPU tries to be helpful by dispatching to an
offset within a vector table depending on the interrupt number.

4. The real problem with this is IRQ_MIPS_CPU no longer gets its 7
interrupts because they've been renumbered.

5. MIPS 1004Kc uses a standard intc (IRQ_GIC), and they solved this by
not really using the feature. Despite having 64 lines, they only send on
one and they make the driver poll to find out what's pending. I believe
they also return the CPU's 7 interrupts without renumbering.

6. But in 34Kc world there is no standard intc, and AFAICT many (most?)
of them fully use the EI/VI feature.

7. If you don't set EI/VI, the processor goes into / stays in legacy mode,
so it doesn't send anything to the intc, and everything the intc sends to
it is converted to a hit on line 2 - so as long as the intc has some
kind of pending register, chaining works.

8. But without EI/VI you can't have MT_SMP so you only get one thread.

9. In every 34Kc SoC I've found in Linux or OpenWRT, EI/VI is
conspicuously missing (with one exception). Clearly they had a
compelling reason for doing it, and I *think* that reason is because
they all faced the same issue as me and solved it the same way.

10. The exception is irq-realtek-rtl, which via an out-of-tree patch[1]
was able to enable EI/VI and I have no idea what they're doing, but it
appears that their intc hardware is participating, like with GIC.

11. I did implement an EI/VI dispatcher myself and had it working with
SMP, but I shelved because it's complex and it's not tightly coupled to
the intc driver itself so I concluded that it should be a separate
component that works with any intc. The complexity comes from the
fact that you need to either route the software interrupts back to
IRQ_MIPS_CPU's domain and fix the renumbering, or else implement
your own IPI subsystem.


So it's my belief that what I'm doing here is standard for 34Kc.

The reason I asked the question in the beginning was because I wanted
to check my assumptions and know if there's any way I can get SMP
without writing this dispatcher.


>>> So this patch clearly should have been tagged with 'RFC'.
>> Given the patchset works correctly in testing, does this comment
>> stand?
> Until the EI/VI issue is resolved so that it either works or cannot
> happen.

All said, if "depends on !EI && !VI" makes you happy then I'm OK to add it.

Just what I'm afraid of is being asked to find an authoritative answer to my
question before merging, because if nobody decides to jump in with one
then this could just be blocked indefinitely.


Thanks,

Caleb


[1]: 
https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/patches-6.6/314-irqchip-irq-realtek-rtl-add-VPE-support.patch

>>>> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (hwirq >= INTC_IRQ_COUNT) {
>>>> +		pr_err("%s: hwirq %lu out of range\n", __func__, hwirq);
>>>> +		return -EINVAL;
>>>> +	} else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) {
>>>> +		pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n",
>>>> +		       __func__, hwirq);
>>> No newline
>> If I understand correctly, you prefer:
>> .....interrupt\n", __func__, hwirq);
>> for a 96 char line?
> You have 100 characters in drivers/irqchip/
>
>>>> +	.domain_ops = {
>>>> +		.xlate = irq_domain_xlate_onecell,
>>>> +		.map = econet_intc_map,
>>> See documention.
>> I suppose this is tab alignment, but I will in any case make a point
>> of reading it all carefully.
> Yes. The aligned tabular view is way simpler to read and parse. Reading
> is based on pattern recognition. Irregular patterns disturb the reading
> flow, which means the focus is shifted from understanding to decoding
> the irregular pattern.
>
>> In case of any doubt, I wasn't trying to sneak bad code past you.
> I did not assume malice here at all.
>
> Thanks,
>
>          tglx

Re: [PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Thomas Gleixner]

On Sun, Mar 23 2025 at 04:06, Caleb James DeLisle wrote:
> So it's my belief that what I'm doing here is standard for 34Kc.
>
> The reason I asked the question in the beginning was because I wanted
> to check my assumptions and know if there's any way I can get SMP
> without writing this dispatcher.

Fair enough. If it just works as is then I don't have any objections and
the question vs. SMP has to answered by the MIPS wizards.

>>>> So this patch clearly should have been tagged with 'RFC'.
>>> Given the patchset works correctly in testing, does this comment
>>> stand?
>> Until the EI/VI issue is resolved so that it either works or cannot
>> happen.
>
> All said, if "depends on !EI && !VI" makes you happy then I'm OK to add it.

It's not about making me happy. I just want to avoid a situation where
this causes hard to diagnose issues.

> Just what I'm afraid of is being asked to find an authoritative answer to my
> question before merging, because if nobody decides to jump in with one
> then this could just be blocked indefinitely.

Nah. If it works the way you implemented it and you can arguably exclude
EI/VI interaction, then there is no reason to delay anything.

Thanks,

        tglx

[PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Caleb James DeLisle]

Add device tree binding documentation for the high-precision timer (HPT)
in the EcoNet EN751221 SoC.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 .../bindings/timer/econet,timer-hpt.yaml      | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml

diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
new file mode 100644
index 000000000000..8b7ff9bce947
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: EcoNet High Precision Timer (HPT)
+
+maintainers:
+  - Calev James DeLisle <cjd@cjdns.fr>
+
+description: |
+  The EcoNet High Precision Timer (HPT) is a timer peripheral found in various
+  EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE
+  count/compare registers and a per-CPU control register, with a single interrupt
+  line using a percpu-devid interrupt mechanism.
+
+properties:
+  compatible:
+    const: econet,timer-hpt
+
+  reg:
+    minItems: 1
+    maxItems: 2
+    description: |
+      Physical base address and size of the timer's register space. On 34Kc
+      processors, a single region is used. On 1004Kc processors, two regions are
+      used, one for each core.
+
+  interrupts:
+    maxItems: 1
+    description: |
+      The interrupt number for the timer. This is a percpu-devid interrupt shared
+      across CPUs.
+
+  clocks:
+    maxItems: 1
+    description: |
+      A clock to get the frequency of the timer.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    timer_hpt@1fbf0400 {
+        compatible = "econet,timer-hpt";
+        reg = <0x1fbf0400 0x100>;
+        interrupt-parent = <&intc>;
+        interrupts = <30>;
+        clocks = <&hpt_clock>;
+    };
+...
-- 
2.30.2

Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Krzysztof Kozlowski]

On 21/03/2025 14:46, Caleb James DeLisle wrote:
> Add device tree binding documentation for the high-precision timer (HPT)
> in the EcoNet EN751221 SoC.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>

Previous patch was not tested, so was this one tested?

> ---
>  .../bindings/timer/econet,timer-hpt.yaml      | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
> new file mode 100644
> index 000000000000..8b7ff9bce947
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EcoNet High Precision Timer (HPT)
> +
> +maintainers:
> +  - Calev James DeLisle <cjd@cjdns.fr>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The EcoNet High Precision Timer (HPT) is a timer peripheral found in various
> +  EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE
> +  count/compare registers and a per-CPU control register, with a single interrupt
> +  line using a percpu-devid interrupt mechanism.
> +
> +properties:
> +  compatible:
> +    const: econet,timer-hpt

Soc components must have soc-based compatible and then filename matching
whatever you use as fallback.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2

No, list items instead.

> +    description: |
> +      Physical base address and size of the timer's register space. On 34Kc
> +      processors, a single region is used. On 1004Kc processors, two regions are
> +      used, one for each core.

So different hardware, different compatible. That's why you need
soc-based compatibles. Follow standard SoC upstreaming rules and examples.

> +
> +  interrupts:
> +    maxItems: 1
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      The interrupt number for the timer.

Drop, redundant.


> This is a percpu-devid interrupt shared
> +      across CPUs.
> +
> +  clocks:
> +    maxItems: 1
> +    description: |
> +      A clock to get the frequency of the timer.

Drop description, redundant

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    timer_hpt@1fbf0400 {

No underscores

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Look how other SoCs are calling this.

> +        compatible = "econet,timer-hpt";
> +        reg = <0x1fbf0400 0x100>;
> +        interrupt-parent = <&intc>;
> +        interrupts = <30>;
> +        clocks = <&hpt_clock>;
> +    };
> +...


Best regards,
Krzysztof

Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Caleb James DeLisle]

Thank you for the review.

On 21/03/2025 21:56, Krzysztof Kozlowski wrote:
> On 21/03/2025 14:46, Caleb James DeLisle wrote:
>> Add device tree binding documentation for the high-precision timer (HPT)
>> in the EcoNet EN751221 SoC.
>>
>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> Previous patch was not tested, so was this one tested?

Yes, all of this has been tested on multiple devices, I believe I was
unclear in the question I added in patch 3.

>
>> ---
>>   .../bindings/timer/econet,timer-hpt.yaml      | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>> new file mode 100644
>> index 000000000000..8b7ff9bce947
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: EcoNet High Precision Timer (HPT)
>> +
>> +maintainers:
>> +  - Calev James DeLisle <cjd@cjdns.fr>
>> +
>> +description: |
> Do not need '|' unless you need to preserve formatting.
Ok
>
>> +  The EcoNet High Precision Timer (HPT) is a timer peripheral found in various
>> +  EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE
>> +  count/compare registers and a per-CPU control register, with a single interrupt
>> +  line using a percpu-devid interrupt mechanism.
>> +
>> +properties:
>> +  compatible:
>> +    const: econet,timer-hpt
> Soc components must have soc-based compatible and then filename matching
> whatever you use as fallback.

I have so far been unable to find good documentation on writing DT bindings
specifically for SoC devices. If you have anything to point me to, I will read it.
If not, even a good example of someone else doing it right is helpful.

Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence
of any other advice, I can try to do what they do.

>
>> +
>> +  reg:
>> +    minItems: 1
>> +    maxItems: 2
> No, list items instead.
I see qcom,pdc.yaml using items: with per-item description so can follow that.
>
>> +    description: |
>> +      Physical base address and size of the timer's register space. On 34Kc
>> +      processors, a single region is used. On 1004Kc processors, two regions are
>> +      used, one for each core.
> So different hardware, different compatible. That's why you need
> soc-based compatibles. Follow standard SoC upstreaming rules and examples.
I presume this should ideally be with If: statements to further validate the DT (?)
>
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: |
> Do not need '|' unless you need to preserve formatting.
Ok
>
>> +      The interrupt number for the timer.
> Drop, redundant.
Ok
>
>
>> This is a percpu-devid interrupt shared
>> +      across CPUs.
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: |
>> +      A clock to get the frequency of the timer.
> Drop description, redundant
Ok
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    timer_hpt@1fbf0400 {
> No underscores
I knew that, my mistake.
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Thank you, this is useful.
>
> Look how other SoCs are calling this.
As said, any documentation link or example of someone who does this right
is much appreciated. In any case, thank you very much for your time and I
will address these points in v2.

Thanks,
Caleb

>
>> +        compatible = "econet,timer-hpt";
>> +        reg = <0x1fbf0400 0x100>;
>> +        interrupt-parent = <&intc>;
>> +        interrupts = <30>;
>> +        clocks = <&hpt_clock>;
>> +    };
>> +...
>
> Best regards,
> Krzysztof

Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Krzysztof Kozlowski]

On 22/03/2025 00:21, Caleb James DeLisle wrote:
> Thank you for the review.
> 
> On 21/03/2025 21:56, Krzysztof Kozlowski wrote:
>> On 21/03/2025 14:46, Caleb James DeLisle wrote:
>>> Add device tree binding documentation for the high-precision timer (HPT)
>>> in the EcoNet EN751221 SoC.
>>>
>>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>> Previous patch was not tested, so was this one tested?
> 
> Yes, all of this has been tested on multiple devices, I believe I was
> unclear in the question I added in patch 3.

Hm? How can you test a binding on a device? I meant here bindings - they
were not tested.

> 
>>
>>> ---
>>>   .../bindings/timer/econet,timer-hpt.yaml      | 58 +++++++++++++++++++
>>>   1 file changed, 58 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>> new file mode 100644
>>> index 000000000000..8b7ff9bce947
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>> @@ -0,0 +1,58 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: EcoNet High Precision Timer (HPT)
>>> +
>>> +maintainers:
>>> +  - Calev James DeLisle <cjd@cjdns.fr>
>>> +
>>> +description: |
>> Do not need '|' unless you need to preserve formatting.
> Ok
>>
>>> +  The EcoNet High Precision Timer (HPT) is a timer peripheral found in various
>>> +  EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE
>>> +  count/compare registers and a per-CPU control register, with a single interrupt
>>> +  line using a percpu-devid interrupt mechanism.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: econet,timer-hpt
>> Soc components must have soc-based compatible and then filename matching
>> whatever you use as fallback.
> 
> I have so far been unable to find good documentation on writing DT bindings
> specifically for SoC devices. If you have anything to point me to, I will read it.
> If not, even a good example of someone else doing it right is helpful.
> 
> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence
> of any other advice, I can try to do what they do.

Just don't use generic fallback.

> 
>>
>>> +
>>> +  reg:
>>> +    minItems: 1
>>> +    maxItems: 2
>> No, list items instead.
> I see qcom,pdc.yaml using items: with per-item description so can follow that.
>>
>>> +    description: |
>>> +      Physical base address and size of the timer's register space. On 34Kc
>>> +      processors, a single region is used. On 1004Kc processors, two regions are
>>> +      used, one for each core.
>> So different hardware, different compatible. That's why you need
>> soc-based compatibles. Follow standard SoC upstreaming rules and examples.
> I presume this should ideally be with If: statements to further validate the DT (?)

Yes

>>
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +    description: |
>> Do not need '|' unless you need to preserve formatting.
> Ok
>>
>>> +      The interrupt number for the timer.
>> Drop, redundant.
> Ok
>>
>>
>>> This is a percpu-devid interrupt shared
>>> +      across CPUs.
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +    description: |
>>> +      A clock to get the frequency of the timer.
>> Drop description, redundant
> Ok
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    timer_hpt@1fbf0400 {
>> No underscores
> I knew that, my mistake.
>>
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> Thank you, this is useful.
>>
>> Look how other SoCs are calling this.
> As said, any documentation link or example of someone who does this right
> is much appreciated. In any case, thank you very much for your time and I
> will address these points in v2.

I gave one link above. Other could be one of my talks... or maybe what
elinux.org has, but I did not verify it.

Best regards,
Krzysztof

Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Caleb James DeLisle]

On 23/03/2025 13:39, Krzysztof Kozlowski wrote:
> On 22/03/2025 00:21, Caleb James DeLisle wrote:
>> Thank you for the review.
>>
>> On 21/03/2025 21:56, Krzysztof Kozlowski wrote:
>>> On 21/03/2025 14:46, Caleb James DeLisle wrote:
>>>> Add device tree binding documentation for the high-precision timer (HPT)
>>>> in the EcoNet EN751221 SoC.
>>>>
>>>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>>> Previous patch was not tested, so was this one tested?
>> Yes, all of this has been tested on multiple devices, I believe I was
>> unclear in the question I added in patch 3.
> Hm? How can you test a binding on a device? I meant here bindings - they
> were not tested.


I see. For bindings I ran `make dt_binding_check` and assumed it good because
it ran to completion. I now know that isn't reliable, but re-checked that it didn't
log any errors (warnings?) about econet,timer-hpt.yaml


>
>>>> ---
>>>>    .../bindings/timer/econet,timer-hpt.yaml      | 58 +++++++++++++++++++
>>>>    1 file changed, 58 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>>> new file mode 100644
>>>> index 000000000000..8b7ff9bce947
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
>>>> @@ -0,0 +1,58 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/timer/econet,timer-hpt.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: EcoNet High Precision Timer (HPT)
>>>> +
>>>> +maintainers:
>>>> +  - Calev James DeLisle <cjd@cjdns.fr>
>>>> +
>>>> +description: |
>>> Do not need '|' unless you need to preserve formatting.
>> Ok
>>>> +  The EcoNet High Precision Timer (HPT) is a timer peripheral found in various
>>>> +  EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE
>>>> +  count/compare registers and a per-CPU control register, with a single interrupt
>>>> +  line using a percpu-devid interrupt mechanism.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: econet,timer-hpt
>>> Soc components must have soc-based compatible and then filename matching
>>> whatever you use as fallback.
>> I have so far been unable to find good documentation on writing DT bindings
>> specifically for SoC devices. If you have anything to point me to, I will read it.
>> If not, even a good example of someone else doing it right is helpful.
>>
>> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence
>> of any other advice, I can try to do what they do.
> Just don't use generic fallback.


Ok I watched your "Accepted in Less Than 10 Iterations" lecture (I'm doing my
homework). If I understand this correctly, you prefer that I use something specific
like econet,en751221-timer as the fallback case, so for example on EN751627,
it would be:

compatible = "econet,en751627-timer", "econet,en751221-timer";

The reason why I didn't do this is because this timer seems to show up in a lot of
places. Vendor code says that it's older than EN751221, and (if my reading is
correct) it has found it's way into chips branded TrendChip, MediaTek and Ralink
as well as EcoNet.

Now that I'll be adding strict checks on the number of register blocks, this way
also has the advantage of allowing a case for users of the timer in SoCs we don't
know about:

// Only valid with 2 register blocks
compatible = "econet,en751627-timer", "econet,timer-hpt";

// Only valid with 1 register block
compatible = "econet,en751612-timer", "econet,timer-hpt";

// No restriction because we don't know how many timers the SoC has
compatible = "econet,timer-hpt";


That said, I'm fine to do it however you want as long as I'm clear on what you're
asking for and you have all of the context behind my original decision.


Thanks,

Caleb


>
>>>> +
>>>> +  reg:
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>> No, list items instead.
>> I see qcom,pdc.yaml using items: with per-item description so can follow that.
>>>> +    description: |
>>>> +      Physical base address and size of the timer's register space. On 34Kc
>>>> +      processors, a single region is used. On 1004Kc processors, two regions are
>>>> +      used, one for each core.
>>> So different hardware, different compatible. That's why you need
>>> soc-based compatibles. Follow standard SoC upstreaming rules and examples.
>> I presume this should ideally be with If: statements to further validate the DT (?)
> Yes
>
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +    description: |
>>> Do not need '|' unless you need to preserve formatting.
>> Ok
>>>> +      The interrupt number for the timer.
>>> Drop, redundant.
>> Ok
>>>
>>>> This is a percpu-devid interrupt shared
>>>> +      across CPUs.
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +    description: |
>>>> +      A clock to get the frequency of the timer.
>>> Drop description, redundant
>> Ok
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - clocks
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    timer_hpt@1fbf0400 {
>>> No underscores
>> I knew that, my mistake.
>>> Node names should be generic. See also an explanation and list of
>>> examples (not exhaustive) in DT specification:
>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>> Thank you, this is useful.
>>> Look how other SoCs are calling this.
>> As said, any documentation link or example of someone who does this right
>> is much appreciated. In any case, thank you very much for your time and I
>> will address these points in v2.
> I gave one link above. Other could be one of my talks... or maybe what
> elinux.org has, but I did not verify it.
>
> Best regards,
> Krzysztof

Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Krzysztof Kozlowski]

On 24/03/2025 00:53, Caleb James DeLisle wrote:
>>>>> +  compatible:
>>>>> +    const: econet,timer-hpt
>>>> Soc components must have soc-based compatible and then filename matching
>>>> whatever you use as fallback.
>>> I have so far been unable to find good documentation on writing DT bindings
>>> specifically for SoC devices. If you have anything to point me to, I will read it.
>>> If not, even a good example of someone else doing it right is helpful.
>>>
>>> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence
>>> of any other advice, I can try to do what they do.
>> Just don't use generic fallback.
> 
> 
> Ok I watched your "Accepted in Less Than 10 Iterations" lecture (I'm doing my
> homework). If I understand this correctly, you prefer that I use something specific
> like econet,en751221-timer as the fallback case, so for example on EN751627,
> it would be:
> 
> compatible = "econet,en751627-timer", "econet,en751221-timer";

Yes

> 
> The reason why I didn't do this is because this timer seems to show up in a lot of
> places. Vendor code says that it's older than EN751221, and (if my reading is

Just like every other SoC component for every other SoC.

> correct) it has found it's way into chips branded TrendChip, MediaTek and Ralink
> as well as EcoNet.
> 
> Now that I'll be adding strict checks on the number of register blocks, this way
> also has the advantage of allowing a case for users of the timer in SoCs we don't
> know about:
> 
> // Only valid with 2 register blocks
> compatible = "econet,en751627-timer", "econet,timer-hpt";
> 
> // Only valid with 1 register block
> compatible = "econet,en751612-timer", "econet,timer-hpt";

Above do not differ...

> 
> // No restriction because we don't know how many timers the SoC has
> compatible = "econet,timer-hpt";

How can you not know? This is strictly defined on given hardware.

> 
> 
> That said, I'm fine to do it however you want as long as I'm clear on what you're
> asking for and you have all of the context behind my original decision.

Generic compatible as fallback is accepted if you have evidence that it
is the same IP, with same programming interface, across all these SoCs.
Or if its version is discoverable. If you do not know about other SoC
implementations it is rather a proof that above statement cannot be
fulfilled - you just don't know how it is implemented.


Best regards,
Krzysztof

Re: [PATCH v1 4/8] dt-bindings: timer: Add EcoNet HPT CPU Timer

[Caleb James DeLisle]

On 24/03/2025 08:13, Krzysztof Kozlowski wrote:
> On 24/03/2025 00:53, Caleb James DeLisle wrote:
>>>>>> +  compatible:
>>>>>> +    const: econet,timer-hpt
>>>>> Soc components must have soc-based compatible and then filename matching
>>>>> whatever you use as fallback.
>>>> I have so far been unable to find good documentation on writing DT bindings
>>>> specifically for SoC devices. If you have anything to point me to, I will read it.
>>>> If not, even a good example of someone else doing it right is helpful.
>>>>
>>>> Currently, I see qcom,pdc.yaml appears to do what you say, so I in absence
>>>> of any other advice, I can try to do what they do.
>>> Just don't use generic fallback.
>>
>> Ok I watched your "Accepted in Less Than 10 Iterations" lecture (I'm doing my
>> homework). If I understand this correctly, you prefer that I use something specific
>> like econet,en751221-timer as the fallback case, so for example on EN751627,
>> it would be:
>>
>> compatible = "econet,en751627-timer", "econet,en751221-timer";
> Yes
>
>> The reason why I didn't do this is because this timer seems to show up in a lot of
>> places. Vendor code says that it's older than EN751221, and (if my reading is
> Just like every other SoC component for every other SoC.
>
>> correct) it has found it's way into chips branded TrendChip, MediaTek and Ralink
>> as well as EcoNet.
>>
>> Now that I'll be adding strict checks on the number of register blocks, this way
>> also has the advantage of allowing a case for users of the timer in SoCs we don't
>> know about:
>>
>> // Only valid with 2 register blocks
>> compatible = "econet,en751627-timer", "econet,timer-hpt";
>>
>> // Only valid with 1 register block
>> compatible = "econet,en751612-timer", "econet,timer-hpt";
> Above do not differ...
>
>> // No restriction because we don't know how many timers the SoC has
>> compatible = "econet,timer-hpt";
> How can you not know? This is strictly defined on given hardware.
>
I mean I don't know, the person writing the DTS for that SoC needs to know.


Per your preference, I'll do the following:


// 2 blocks accepted

compatible = "econet,en751627-timer", "econet,en751221-timer";

// 1 block accepted

compatible = "econet,en751221-timer";


If someone has an SoC with more than 2 timers, it is not supported so they
should update the binding, or (in downstream) they might write an invalid
DTS. FWIW I have no evidence of any >2 core processor which uses this, so
2 timers is probably the maximum.


Lastly I'll change the driver name to timer-econet-en751221.c to avoid the
proliferation of different names.


Thanks,

Caleb

[PATCH v1 5/8] clocksource/drivers: Add EcoNet Timer HPT driver

[Caleb James DeLisle]

Introduce a clocksource driver for the so-called high-precision timer (HPT)
in the EcoNet EN751221 MIPS SoC.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
The name HPT derives from a log line "Using XXX.XX MHz high precision timer"
which originated in mips/kernel/time.c and was copied into the vendor driver.
Subsequent generations of vendor and 3rd party out-of-tree drivers preserved
the log line long after it had been removed from the kernel proper. This
device began to be known as "the HPT", so we preserve the naming and log line.
---
 drivers/clocksource/Kconfig            |   8 +
 drivers/clocksource/Makefile           |   1 +
 drivers/clocksource/timer-econet-hpt.c | 221 +++++++++++++++++++++++++
 3 files changed, 230 insertions(+)
 create mode 100644 drivers/clocksource/timer-econet-hpt.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 487c85259967..1a7a9b4f16f9 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -73,6 +73,14 @@ config DW_APB_TIMER_OF
 	select DW_APB_TIMER
 	select TIMER_OF
 
+config ECONET_TIMER_HPT
+	bool "EcoNet High Precision Timer" if COMPILE_TEST
+	depends on HAS_IOMEM
+	select CLKSRC_MMIO
+	select TIMER_OF
+	help
+	  Support for CPU timer found on EcoNet MIPS based SoCs.
+
 config FTTMR010_TIMER
 	bool "Faraday Technology timer driver" if COMPILE_TEST
 	depends on HAS_IOMEM
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 43ef16a4efa6..a3dd2a9d2a37 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
 obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
 obj-$(CONFIG_DAVINCI_TIMER)	+= timer-davinci.o
 obj-$(CONFIG_DIGICOLOR_TIMER)	+= timer-digicolor.o
+obj-$(CONFIG_ECONET_TIMER_HPT)	+= timer-econet-hpt.o
 obj-$(CONFIG_OMAP_DM_TIMER)	+= timer-ti-dm.o
 obj-$(CONFIG_OMAP_DM_SYSTIMER)	+= timer-ti-dm-systimer.o
 obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
diff --git a/drivers/clocksource/timer-econet-hpt.c b/drivers/clocksource/timer-econet-hpt.c
new file mode 100644
index 000000000000..defd797426c5
--- /dev/null
+++ b/drivers/clocksource/timer-econet-hpt.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Timer present on EcoNet EN75xx MIPS based SoCs.
+ *
+ * Copyright (C) 2025 by Caleb James DeLisle <cjd@cjdns.fr>
+ */
+
+#include <linux/io.h>
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/sched_clock.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/cpuhotplug.h>
+#include <linux/clk.h>
+
+#define ECONET_BITS			32
+#define ECONET_MIN_DELTA		0x00001000
+#define ECONET_MAX_DELTA		GENMASK(ECONET_BITS - 2, 0)
+/* 34Kc hardware has 1 block and 1004Kc has 2. */
+#define ECONET_NUM_BLOCKS		DIV_ROUND_UP(NR_CPUS, 2)
+
+static struct {
+	void __iomem *membase[ECONET_NUM_BLOCKS];
+	u32 freq_hz;
+} econet_timer __ro_after_init;
+
+static DEFINE_PER_CPU(struct clock_event_device, econet_timer_pcpu_m);
+
+/* Each memory block has 2 timers, the order of registers is:
+ * CTL, CMR0, CNT0, CMR1, CNT1
+ */
+static inline void __iomem *reg_ctl(u32 timer_n)
+{
+	return econet_timer.membase[timer_n >> 1];
+}
+
+static inline void __iomem *reg_compare(u32 timer_n)
+{
+	return econet_timer.membase[timer_n >> 1] + (timer_n & 1) * 0x08 + 0x04;
+}
+
+static inline void __iomem *reg_count(u32 timer_n)
+{
+	return econet_timer.membase[timer_n >> 1] + (timer_n & 1) * 0x08 + 0x08;
+}
+
+static inline u32 ctl_bit_enabled(u32 timer_n)
+{
+	return 1U << (timer_n & 1);
+}
+
+static inline u32 ctl_bit_pending(u32 timer_n)
+{
+	return 1U << ((timer_n & 1) + 16);
+}
+
+static bool cevt_is_pending(int cpu_id)
+{
+	return ioread32(reg_ctl(cpu_id)) & ctl_bit_pending(cpu_id);
+}
+
+static irqreturn_t cevt_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *dev = this_cpu_ptr(&econet_timer_pcpu_m);
+	int cpu = cpumask_first(dev->cpumask);
+
+	if (!cevt_is_pending(cpu)) {
+		pr_debug("%s IRQ %d on CPU %d is not pending\n", __func__, irq, cpu);
+		return IRQ_NONE;
+	}
+
+	iowrite32(ioread32(reg_count(cpu)), reg_compare(cpu));
+	dev->event_handler(dev);
+	return IRQ_HANDLED;
+}
+
+static int cevt_set_next_event(ulong delta, struct clock_event_device *dev)
+{
+	int cpu;
+	u32 next;
+
+	cpu = cpumask_first(dev->cpumask);
+	next = ioread32(reg_count(cpu)) + delta;
+	iowrite32(next, reg_compare(cpu));
+
+	if ((s32)(next - ioread32(reg_count(cpu))) < ECONET_MIN_DELTA / 2)
+		return -ETIME;
+
+	return 0;
+}
+
+static int cevt_init_cpu(uint cpu)
+{
+	u32 reg;
+	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu_m, cpu);
+
+	pr_info("%s: Setting up clockevent for CPU %d\n", cd->name, cpu);
+
+	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
+	iowrite32(reg, reg_ctl(cpu));
+
+	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
+
+	/* Do this last because it synchronously configures the timer */
+	clockevents_config_and_register(
+		cd, econet_timer.freq_hz,
+		ECONET_MIN_DELTA, ECONET_MAX_DELTA);
+
+	return 0;
+}
+
+static u64 notrace sched_clock_read(void)
+{
+	/* Always read from clock zero no matter the CPU */
+	return (u64)ioread32(reg_count(0));
+}
+
+/* Init */
+
+static void __init cevt_dev_init(uint cpu)
+{
+	iowrite32(0, reg_count(cpu));
+	iowrite32(U32_MAX, reg_compare(cpu));
+}
+
+static int __init cevt_init(struct device_node *np)
+{
+	int i;
+	int irq;
+	int ret;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq <= 0) {
+		pr_err("%pOFn: irq_of_parse_and_map failed", np);
+		return -EINVAL;
+	}
+
+	ret = request_percpu_irq(
+		irq, cevt_interrupt,
+		np->name, &econet_timer_pcpu_m);
+
+	if (ret < 0) {
+		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np, irq, ret);
+		goto err_unmap_irq;
+	}
+
+	for_each_possible_cpu(i) {
+		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu_m, i);
+
+		cd->rating		= 310,
+		cd->features		= CLOCK_EVT_FEAT_ONESHOT |
+					  CLOCK_EVT_FEAT_C3STOP |
+					  CLOCK_EVT_FEAT_PERCPU;
+		cd->set_next_event	= cevt_set_next_event;
+		cd->irq			= irq;
+		cd->cpumask		= cpumask_of(i);
+		cd->name		= np->name;
+
+		cevt_dev_init(i);
+	}
+
+	cpuhp_setup_state(CPUHP_AP_MIPS_GIC_TIMER_STARTING,
+			  "clockevents/en75/timer:starting",
+			  cevt_init_cpu, NULL);
+	return 0;
+
+err_unmap_irq:
+	irq_dispose_mapping(irq);
+	return ret;
+}
+
+static int __init timer_init(struct device_node *np)
+{
+	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
+	struct clk *clk;
+	int ret;
+
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk)) {
+		pr_err("%pOFn: Failed to get CPU clock from DT %ld\n", np,
+		       PTR_ERR(clk));
+		return PTR_ERR(clk);
+	}
+
+	econet_timer.freq_hz = clk_get_rate(clk);
+
+	for (int i = 0; i < num_blocks; i++) {
+		econet_timer.membase[i] = of_iomap(np, i);
+		if (!econet_timer.membase[i]) {
+			pr_err("%pOFn: failed to map register [%d]\n", np, i);
+			return -ENXIO;
+		}
+	}
+
+	/* For clocksource purposes always read clock zero, whatever the CPU */
+	ret = clocksource_mmio_init(reg_count(0), np->name,
+				    econet_timer.freq_hz, 301, ECONET_BITS,
+				    clocksource_mmio_readl_up);
+	if (ret) {
+		pr_err("%pOFn: clocksource_mmio_init failed: %d", np, ret);
+		return ret;
+	}
+
+	ret = cevt_init(np);
+	if (ret < 0)
+		return ret;
+
+	sched_clock_register(sched_clock_read, ECONET_BITS,
+			     econet_timer.freq_hz);
+
+	pr_info("%pOFn: using %u.%03u MHz high precision timer\n", np,
+		econet_timer.freq_hz / 1000000,
+		(econet_timer.freq_hz / 1000) % 1000);
+
+	return 0;
+}
+
+TIMER_OF_DECLARE(econet_timer_hpt, "econet,timer-hpt", timer_init);
-- 
2.30.2

[PATCH v1 6/8] dt-bindings: mips: Add EcoNet platform binding

[Caleb James DeLisle]

Document the top-level device tree binding for EcoNet MIPS-based SoCs.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 .../devicetree/bindings/mips/econet.yaml      | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mips/econet.yaml

diff --git a/Documentation/devicetree/bindings/mips/econet.yaml b/Documentation/devicetree/bindings/mips/econet.yaml
new file mode 100644
index 000000000000..44da78dc1e29
--- /dev/null
+++ b/Documentation/devicetree/bindings/mips/econet.yaml
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mips/econet.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: EcoNet MIPS SoCs
+
+maintainers:
+  - Caleb James DeLisle <cjd@cjdns.fr>
+
+description:
+  Boards with an EcoNet SoC shall have the following properties.
+
+properties:
+  $nodename:
+    const: '/'
+
+  compatible:
+    oneOf:
+      - description: Boards with EcoNet EN751221 family SoC
+        items:
+          - const: econet,en751221
+
+additionalProperties: true
+
+...
-- 
2.30.2

Re: [PATCH v1 6/8] dt-bindings: mips: Add EcoNet platform binding

[Krzysztof Kozlowski]

On 21/03/2025 14:46, Caleb James DeLisle wrote:
> Document the top-level device tree binding for EcoNet MIPS-based SoCs.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> ---
>  .../devicetree/bindings/mips/econet.yaml      | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mips/econet.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mips/econet.yaml b/Documentation/devicetree/bindings/mips/econet.yaml
> new file mode 100644
> index 000000000000..44da78dc1e29
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/econet.yaml
> @@ -0,0 +1,27 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mips/econet.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EcoNet MIPS SoCs
> +
> +maintainers:
> +  - Caleb James DeLisle <cjd@cjdns.fr>
> +
> +description:
> +  Boards with an EcoNet SoC shall have the following properties.

Drop description, useless and not even correct.

> +
> +properties:
> +  $nodename:
> +    const: '/'
> +
> +  compatible:
> +    oneOf:
> +      - description: Boards with EcoNet EN751221 family SoC
> +        items:
> +          - const: econet,en751221

That's a soc. You need board compatible as well. Look at other
upstreamed code.



Best regards,
Krzysztof

[PATCH v1 7/8] mips: Add EcoNet MIPS platform support

[Caleb James DeLisle]

Add platform support for EcoNet MIPS SoCs.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
As is typical of embedded platforms, it's not realistic to imagine building
a fully functioning system from kernel sources alone. In the interest of
providing something without external dependencies, I have included build and
device tree for a minimal testing / PoC image that can be booted from memory
on these devices.
---
 arch/mips/Kbuild.platforms                    |  1 +
 arch/mips/Kconfig                             | 25 ++++++
 arch/mips/boot/compressed/uart-16550.c        |  5 ++
 arch/mips/boot/dts/Makefile                   |  1 +
 arch/mips/boot/dts/econet/Makefile            |  2 +
 arch/mips/boot/dts/econet/en751221.dtsi       | 62 +++++++++++++++
 .../boot/dts/econet/en751221_test_image.dts   | 19 +++++
 arch/mips/econet/Kconfig                      | 42 ++++++++++
 arch/mips/econet/Makefile                     |  2 +
 arch/mips/econet/Platform                     |  5 ++
 arch/mips/econet/init.c                       | 78 +++++++++++++++++++
 11 files changed, 242 insertions(+)
 create mode 100644 arch/mips/boot/dts/econet/Makefile
 create mode 100644 arch/mips/boot/dts/econet/en751221.dtsi
 create mode 100644 arch/mips/boot/dts/econet/en751221_test_image.dts
 create mode 100644 arch/mips/econet/Kconfig
 create mode 100644 arch/mips/econet/Makefile
 create mode 100644 arch/mips/econet/Platform
 create mode 100644 arch/mips/econet/init.c

diff --git a/arch/mips/Kbuild.platforms b/arch/mips/Kbuild.platforms
index bca37ddf974b..41a00fa860c1 100644
--- a/arch/mips/Kbuild.platforms
+++ b/arch/mips/Kbuild.platforms
@@ -11,6 +11,7 @@ platform-$(CONFIG_CAVIUM_OCTEON_SOC)	+= cavium-octeon/
 platform-$(CONFIG_EYEQ)			+= mobileye/
 platform-$(CONFIG_MIPS_COBALT)		+= cobalt/
 platform-$(CONFIG_MACH_DECSTATION)	+= dec/
+platform-$(CONFIG_ECONET)		+= econet/
 platform-$(CONFIG_MIPS_GENERIC)		+= generic/
 platform-$(CONFIG_MACH_JAZZ)		+= jazz/
 platform-$(CONFIG_LANTIQ)		+= lantiq/
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 1924f2d83932..593ff00855b6 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -390,6 +390,30 @@ config MACH_DECSTATION
 
 	  otherwise choose R3000.
 
+config ECONET
+	bool "EcoNet MIPS family"
+	select BOOT_RAW
+	select CPU_BIG_ENDIAN
+	select DEBUG_ZBOOT
+	select EARLY_PRINTK_8250
+	select ECONET_TIMER_HPT
+	select SERIAL_OF_PLATFORM
+	select SYS_SUPPORTS_BIG_ENDIAN
+	select SYS_HAS_CPU_MIPS32_R1
+	select SYS_HAS_CPU_MIPS32_R2
+	select SYS_HAS_EARLY_PRINTK
+	select SYS_SUPPORTS_32BIT_KERNEL
+	select SYS_SUPPORTS_MIPS16
+	select SYS_SUPPORTS_ZBOOT_UART16550
+	select USE_GENERIC_EARLY_PRINTK_8250
+	select USE_OF
+	help
+	  EcoNet EN75xx MIPS devices are big endian MIPS machines used
+	  in XPON (fiber) and DSL applications. They have SPI, PCI, USB,
+	  GPIO, and Ethernet, with optional XPON, DSL, and VoIP DSP cores.
+	  Don't confuse these with the Airoha ARM devices sometimes referred
+	  to as "EcoNet", this family is for MIPS based devices only.
+
 config MACH_JAZZ
 	bool "Jazz family of machines"
 	select ARC_MEMORY
@@ -1019,6 +1043,7 @@ source "arch/mips/ath79/Kconfig"
 source "arch/mips/bcm47xx/Kconfig"
 source "arch/mips/bcm63xx/Kconfig"
 source "arch/mips/bmips/Kconfig"
+source "arch/mips/econet/Kconfig"
 source "arch/mips/generic/Kconfig"
 source "arch/mips/ingenic/Kconfig"
 source "arch/mips/jazz/Kconfig"
diff --git a/arch/mips/boot/compressed/uart-16550.c b/arch/mips/boot/compressed/uart-16550.c
index db618e72a0c4..529e77a6487c 100644
--- a/arch/mips/boot/compressed/uart-16550.c
+++ b/arch/mips/boot/compressed/uart-16550.c
@@ -20,6 +20,11 @@
 #define PORT(offset) (CKSEG1ADDR(INGENIC_UART_BASE_ADDR) + (4 * offset))
 #endif
 
+#ifdef CONFIG_ECONET
+#define EN75_UART_BASE	0x1fbf0003
+#define PORT(offset)	(CKSEG1ADDR(EN75_UART_BASE) + (4 * (offset)))
+#endif
+
 #ifndef IOTYPE
 #define IOTYPE char
 #endif
diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
index ff468439a8c4..7375c6ced82b 100644
--- a/arch/mips/boot/dts/Makefile
+++ b/arch/mips/boot/dts/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 subdir-$(CONFIG_BMIPS_GENERIC)		+= brcm
 subdir-$(CONFIG_CAVIUM_OCTEON_SOC)	+= cavium-octeon
+subdir-$(CONFIG_ECONET)			+= econet
 subdir-$(CONFIG_EYEQ)			+= mobileye
 subdir-$(CONFIG_FIT_IMAGE_FDT_MARDUK)   += img
 subdir-$(CONFIG_FIT_IMAGE_FDT_BOSTON)	+= img
diff --git a/arch/mips/boot/dts/econet/Makefile b/arch/mips/boot/dts/econet/Makefile
new file mode 100644
index 000000000000..524ba10ce750
--- /dev/null
+++ b/arch/mips/boot/dts/econet/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_SOC_ECONET_EN751221_TEST_IMAGE)	+= en751221_test_image.dtb
diff --git a/arch/mips/boot/dts/econet/en751221.dtsi b/arch/mips/boot/dts/econet/en751221.dtsi
new file mode 100644
index 000000000000..e4404aed5705
--- /dev/null
+++ b/arch/mips/boot/dts/econet/en751221.dtsi
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+/ {
+	compatible = "econet,en751221";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	hpt_clock: hpt_clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <200000000>;  /* 200 MHz */
+	};
+
+	cpus: cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "mips,mips24KEc";
+			reg = <0>;
+		};
+	};
+
+	cpuintc: interrupt-controller {
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+		interrupt-controller;
+		compatible = "mti,cpu-interrupt-controller";
+	};
+
+	intc: interrupt-controller@1fb40000 {
+		compatible = "econet,en751221-intc";
+		reg = <0x1fb40000 0x100>;
+		interrupt-parent = <&cpuintc>;
+		interrupts = <2>;
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		econet,shadow-interrupts = <7 2>, <8 3>, <13 12>, <30 29>;
+	};
+
+	uart: serial@1fbf0000 {
+		compatible = "ns16550";
+		reg = <0x1fbf0000 0x30>;
+		reg-io-width = <4>;
+		reg-shift = <2>;
+		interrupt-parent = <&intc>;
+		interrupts = <0>;
+		clock-frequency = <1843200>;
+	};
+
+	timer_hpt: timer_hpt@1fbf0400 {
+		compatible = "econet,timer-hpt";
+		reg = <0x1fbf0400 0x100>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <30>;
+		clocks = <&hpt_clock>;
+	};
+};
diff --git a/arch/mips/boot/dts/econet/en751221_test_image.dts b/arch/mips/boot/dts/econet/en751221_test_image.dts
new file mode 100644
index 000000000000..bc140c4043b2
--- /dev/null
+++ b/arch/mips/boot/dts/econet/en751221_test_image.dts
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+#include "en751221.dtsi"
+
+/ {
+	model = "Generic EN751221";
+
+	memory@0 {
+		/* We hope at least 64MB will be available wherever we are run */
+		device_type = "memory";
+		reg = <0x00000000 0x4000000>;
+	};
+
+	chosen {
+		bootargs = "console=ttyS0,115200";
+		linux,usable-memory-range = <0x00020000 0x3fe0000>;
+	};
+};
diff --git a/arch/mips/econet/Kconfig b/arch/mips/econet/Kconfig
new file mode 100644
index 000000000000..12f85d638e47
--- /dev/null
+++ b/arch/mips/econet/Kconfig
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0
+if ECONET
+
+config SOC_ECONET_EN751221
+	bool
+	select COMMON_CLK
+	select ECONET_EN751221_INTC
+	select IRQ_MIPS_CPU
+	select SMP
+	select SMP_UP
+	select SYS_SUPPORTS_SMP
+
+choice
+	prompt "EcoNet SoC selection"
+	default SOC_ECONET_EN751221_FAMILY
+	help
+	  Select EcoNet MIPS SoC type. Individual SoCs within a family are
+	  similar enough that is it enough to select the right family, and
+	  then customize to the specific SoC using the device tree only.
+
+	config SOC_ECONET_EN751221_FAMILY
+		bool "EN751221 family"
+		select SOC_ECONET_EN751221
+		help
+		  The EN751221 family includes EN7512, RN7513, EN7521, EN7526.
+		  They are based on single core MIPS 34Kc processors. To boot
+		  this kernel, you will need a device tree such as
+		  MIPS_RAW_APPENDED_DTB=y, and a root filesystem.
+
+	config SOC_ECONET_EN751221_TEST_IMAGE
+		bool "EN751221 test image"
+		select SOC_ECONET_EN751221
+		select BUILTIN_DTB
+		help
+		  Build a minimal kernel that will boot on any EN751221 board
+		  with at least 64MB of memory. This has a builtin device tree
+		  so it can boot with nothing more than an appended initramfs.
+		  This is good for validating that a given SoC is EN751221
+		  compatible, or for regression testing.
+endchoice
+
+endif
diff --git a/arch/mips/econet/Makefile b/arch/mips/econet/Makefile
new file mode 100644
index 000000000000..7e4529e7d3d7
--- /dev/null
+++ b/arch/mips/econet/Makefile
@@ -0,0 +1,2 @@
+
+obj-y := init.o
diff --git a/arch/mips/econet/Platform b/arch/mips/econet/Platform
new file mode 100644
index 000000000000..bb659876d855
--- /dev/null
+++ b/arch/mips/econet/Platform
@@ -0,0 +1,5 @@
+# To address a 7.2MB kernel size limit in the EcoNet SDK bootloader,
+# we put the load address well above where the bootloader loads and then use
+# zboot. So please set CONFIG_ZBOOT_LOAD_ADDRESS to the address where your
+# bootloader actually places the kernel.
+load-$(CONFIG_ECONET)	+= 0xffffffff81000000
\ No newline at end of file
diff --git a/arch/mips/econet/init.c b/arch/mips/econet/init.c
new file mode 100644
index 000000000000..6f43ffb209cb
--- /dev/null
+++ b/arch/mips/econet/init.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * EcoNet setup code
+ *
+ * Copyright (C) 2025 Caleb James DeLisle <cjd@cjdns.fr>
+ */
+
+#include <linux/init.h>
+#include <linux/of_clk.h>
+#include <linux/irqchip.h>
+
+#include <asm/addrspace.h>
+#include <asm/io.h>
+#include <asm/bootinfo.h>
+#include <asm/time.h>
+#include <asm/prom.h>
+#include <asm/smp-ops.h>
+#include <asm/reboot.h>
+
+#define CR_AHB_RSTCR		((void __iomem *)CKSEG1ADDR(0x1fb00040))
+#define RESET			BIT(31)
+
+#define UART_BASE		CKSEG1ADDR(0x1fbf0003)
+#define UART_REG_SHIFT		2
+
+static void hw_reset(char *command)
+{
+	iowrite32(RESET, CR_AHB_RSTCR);
+}
+
+/* 1. Bring up early printk. */
+void __init prom_init(void)
+{
+	setup_8250_early_printk_port(UART_BASE, UART_REG_SHIFT, 0);
+	_machine_restart = hw_reset;
+}
+
+/* 2. Parse the DT and find memory */
+void __init plat_mem_setup(void)
+{
+	void *dtb;
+
+	set_io_port_base(KSEG1);
+
+	dtb = get_fdt();
+	if (!dtb)
+		panic("no dtb found");
+
+	__dt_setup_arch(dtb);
+
+	early_init_dt_scan_memory();
+}
+
+/* 3. Overload __weak device_tree_init(), add SMP_UP ops */
+void __init device_tree_init(void)
+{
+	unflatten_and_copy_device_tree();
+
+	register_up_smp_ops();
+}
+
+const char *get_system_type(void)
+{
+	return "EcoNet-EN75xx";
+}
+
+/* 4. Initialize the IRQ subsystem */
+void __init arch_init_irq(void)
+{
+	irqchip_init();
+}
+
+/* 5. Timers */
+void __init plat_time_init(void)
+{
+	of_clk_init(NULL);
+	timer_probe();
+}
-- 
2.30.2

Re: [PATCH v1 7/8] mips: Add EcoNet MIPS platform support

[Krzysztof Kozlowski]

On 21/03/2025 14:46, Caleb James DeLisle wrote:
> Add platform support for EcoNet MIPS SoCs.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> ---
> As is typical of embedded platforms, it's not realistic to imagine building
> a fully functioning system from kernel sources alone. In the interest of
> providing something without external dependencies, I have included build and
> device tree for a minimal testing / PoC image that can be booted from memory
> on these devices.
> ---
>  arch/mips/Kbuild.platforms                    |  1 +
>  arch/mips/Kconfig                             | 25 ++++++
>  arch/mips/boot/compressed/uart-16550.c        |  5 ++
>  arch/mips/boot/dts/Makefile                   |  1 +
>  arch/mips/boot/dts/econet/Makefile            |  2 +
>  arch/mips/boot/dts/econet/en751221.dtsi       | 62 +++++++++++++++
>  .../boot/dts/econet/en751221_test_image.dts   | 19 +++++
>  arch/mips/econet/Kconfig                      | 42 ++++++++++
>  arch/mips/econet/Makefile                     |  2 +
>  arch/mips/econet/Platform                     |  5 ++
>  arch/mips/econet/init.c                       | 78 +++++++++++++++++++
>  11 files changed, 242 insertions(+)
>  create mode 100644 arch/mips/boot/dts/econet/Makefile
>  create mode 100644 arch/mips/boot/dts/econet/en751221.dtsi
>  create mode 100644 arch/mips/boot/dts/econet/en751221_test_image.dts

DTS are always, always separate patches. See also DTS coding style and
submitting patches in the bindings, which already covers this.



...


> new file mode 100644
> index 000000000000..e4404aed5705
> --- /dev/null
> +++ b/arch/mips/boot/dts/econet/en751221.dtsi
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +/ {
> +	compatible = "econet,en751221";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	hpt_clock: hpt_clock {

No underscores in node names.

Follow DTS coding style.

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <200000000>;  /* 200 MHz */
> +	};
> +
> +	cpus: cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "mips,mips24KEc";
> +			reg = <0>;
> +		};
> +	};
> +
> +	cpuintc: interrupt-controller {
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +		interrupt-controller;
> +		compatible = "mti,cpu-interrupt-controller";
> +	};
> +
> +	intc: interrupt-controller@1fb40000 {
> +		compatible = "econet,en751221-intc";
> +		reg = <0x1fb40000 0x100>;
> +		interrupt-parent = <&cpuintc>;
> +		interrupts = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		econet,shadow-interrupts = <7 2>, <8 3>, <13 12>, <30 29>;
> +	};
> +
> +	uart: serial@1fbf0000 {
> +		compatible = "ns16550";
> +		reg = <0x1fbf0000 0x30>;
> +		reg-io-width = <4>;
> +		reg-shift = <2>;
> +		interrupt-parent = <&intc>;
> +		interrupts = <0>;
> +		clock-frequency = <1843200>;
> +	};
> +
> +	timer_hpt: timer_hpt@1fbf0400 {

Same problem as with binding.

> +		compatible = "econet,timer-hpt";
> +		reg = <0x1fbf0400 0x100>;
> +
> +		interrupt-parent = <&intc>;
> +		interrupts = <30>;
> +		clocks = <&hpt_clock>;
> +	};
> +};
> diff --git a/arch/mips/boot/dts/econet/en751221_test_image.dts b/arch/mips/boot/dts/econet/en751221_test_image.dts
> new file mode 100644
> index 000000000000..bc140c4043b2
> --- /dev/null
> +++ b/arch/mips/boot/dts/econet/en751221_test_image.dts

Does not look like a board. We do not take some testing/debugging
thingies. Please upstream *real* board.

> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +#include "en751221.dtsi"
> +
> +/ {
> +	model = "Generic EN751221";

Missing compatible.

> +
> +	memory@0 {
> +		/* We hope at least 64MB will be available wherever we are run */
> +		device_type = "memory";
> +		reg = <0x00000000 0x4000000>;
> +	};
> +
> +	chosen {
> +		bootargs = "console=ttyS0,115200";

Drop bootargs and use standard property - stdout.

See how all other platforms are doing it (and not some ancient MIPS, but
the most recent arm64 or riscv).


> +		linux,usable-memory-range = <0x00020000 0x3fe0000>;
> +	};
> +};
> diff --git a/arch/mips/econet/Kconfig b/arch/mips/econet/Kconfig
> new file mode 100644
> index 000000000000..12f85d638e47
> --- /dev/null
> +++ b/arch/mips/econet/Kconfig
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0
> +if ECONET
> +
> +config SOC_ECONET_EN751221
> +	bool
> +	select COMMON_CLK
> +	select ECONET_EN751221_INTC
> +	select IRQ_MIPS_CPU
> +	select SMP
> +	select SMP_UP
> +	select SYS_SUPPORTS_SMP
> +
> +choice
> +	prompt "EcoNet SoC selection"
> +	default SOC_ECONET_EN751221_FAMILY
> +	help
> +	  Select EcoNet MIPS SoC type. Individual SoCs within a family are
> +	  similar enough that is it enough to select the right family, and
> +	  then customize to the specific SoC using the device tree only.
> +
> +	config SOC_ECONET_EN751221_FAMILY
> +		bool "EN751221 family"
> +		select SOC_ECONET_EN751221
> +		help
> +		  The EN751221 family includes EN7512, RN7513, EN7521, EN7526.
> +		  They are based on single core MIPS 34Kc processors. To boot
> +		  this kernel, you will need a device tree such as
> +		  MIPS_RAW_APPENDED_DTB=y, and a root filesystem.
> +
> +	config SOC_ECONET_EN751221_TEST_IMAGE
> +		bool "EN751221 test image"
> +		select SOC_ECONET_EN751221
> +		select BUILTIN_DTB
> +		help
> +		  Build a minimal kernel that will boot on any EN751221 board
> +		  with at least 64MB of memory. This has a builtin device tree
> +		  so it can boot with nothing more than an appended initramfs.
> +		  This is good for validating that a given SoC is EN751221
> +		  compatible, or for regression testing.
> +endchoice
> +
> +endif
> diff --git a/arch/mips/econet/Makefile b/arch/mips/econet/Makefile
> new file mode 100644
> index 000000000000..7e4529e7d3d7
> --- /dev/null
> +++ b/arch/mips/econet/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-y := init.o
> diff --git a/arch/mips/econet/Platform b/arch/mips/econet/Platform
> new file mode 100644
> index 000000000000..bb659876d855
> --- /dev/null
> +++ b/arch/mips/econet/Platform
> @@ -0,0 +1,5 @@
> +# To address a 7.2MB kernel size limit in the EcoNet SDK bootloader,
> +# we put the load address well above where the bootloader loads and then use
> +# zboot. So please set CONFIG_ZBOOT_LOAD_ADDRESS to the address where your
> +# bootloader actually places the kernel.
> +load-$(CONFIG_ECONET)	+= 0xffffffff81000000
> \ No newline at end of file

You have patch warnings

Best regards,
Krzysztof

Re: [PATCH v1 7/8] mips: Add EcoNet MIPS platform support

[Caleb James DeLisle]

On 21/03/2025 22:00, Krzysztof Kozlowski wrote:
> On 21/03/2025 14:46, Caleb James DeLisle wrote:
>> Add platform support for EcoNet MIPS SoCs.
>>
>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>> ---
>> As is typical of embedded platforms, it's not realistic to imagine building
>> a fully functioning system from kernel sources alone. In the interest of
>> providing something without external dependencies, I have included build and
>> device tree for a minimal testing / PoC image that can be booted from memory
>> on these devices.
>> ---
>>   arch/mips/Kbuild.platforms                    |  1 +
>>   arch/mips/Kconfig                             | 25 ++++++
>>   arch/mips/boot/compressed/uart-16550.c        |  5 ++
>>   arch/mips/boot/dts/Makefile                   |  1 +
>>   arch/mips/boot/dts/econet/Makefile            |  2 +
>>   arch/mips/boot/dts/econet/en751221.dtsi       | 62 +++++++++++++++
>>   .../boot/dts/econet/en751221_test_image.dts   | 19 +++++
>>   arch/mips/econet/Kconfig                      | 42 ++++++++++
>>   arch/mips/econet/Makefile                     |  2 +
>>   arch/mips/econet/Platform                     |  5 ++
>>   arch/mips/econet/init.c                       | 78 +++++++++++++++++++
>>   11 files changed, 242 insertions(+)
>>   create mode 100644 arch/mips/boot/dts/econet/Makefile
>>   create mode 100644 arch/mips/boot/dts/econet/en751221.dtsi
>>   create mode 100644 arch/mips/boot/dts/econet/en751221_test_image.dts
> DTS are always, always separate patches. See also DTS coding style and
> submitting patches in the bindings, which already covers this.
Ok got it, somehow I though arch/mips/* made it different.
>
>
> ...
>
>
>> new file mode 100644
>> index 000000000000..e4404aed5705
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/econet/en751221.dtsi
>> @@ -0,0 +1,62 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +
>> +/ {
>> +	compatible = "econet,en751221";
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +
>> +	hpt_clock: hpt_clock {
> No underscores in node names.
>
> Follow DTS coding style.
Right, I know this, my fault.
>
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <200000000>;  /* 200 MHz */
>> +	};
>> +
>> +	cpus: cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		cpu@0 {
>> +			device_type = "cpu";
>> +			compatible = "mips,mips24KEc";
>> +			reg = <0>;
>> +		};
>> +	};
>> +
>> +	cpuintc: interrupt-controller {
>> +		#address-cells = <0>;
>> +		#interrupt-cells = <1>;
>> +		interrupt-controller;
>> +		compatible = "mti,cpu-interrupt-controller";
>> +	};
>> +
>> +	intc: interrupt-controller@1fb40000 {
>> +		compatible = "econet,en751221-intc";
>> +		reg = <0x1fb40000 0x100>;
>> +		interrupt-parent = <&cpuintc>;
>> +		interrupts = <2>;
>> +
>> +		interrupt-controller;
>> +		#interrupt-cells = <1>;
>> +		econet,shadow-interrupts = <7 2>, <8 3>, <13 12>, <30 29>;
>> +	};
>> +
>> +	uart: serial@1fbf0000 {
>> +		compatible = "ns16550";
>> +		reg = <0x1fbf0000 0x30>;
>> +		reg-io-width = <4>;
>> +		reg-shift = <2>;
>> +		interrupt-parent = <&intc>;
>> +		interrupts = <0>;
>> +		clock-frequency = <1843200>;
>> +	};
>> +
>> +	timer_hpt: timer_hpt@1fbf0400 {
> Same problem as with binding.
Will fix.
>
>> +		compatible = "econet,timer-hpt";
>> +		reg = <0x1fbf0400 0x100>;
>> +
>> +		interrupt-parent = <&intc>;
>> +		interrupts = <30>;
>> +		clocks = <&hpt_clock>;
>> +	};
>> +};
>> diff --git a/arch/mips/boot/dts/econet/en751221_test_image.dts b/arch/mips/boot/dts/econet/en751221_test_image.dts
>> new file mode 100644
>> index 000000000000..bc140c4043b2
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/econet/en751221_test_image.dts
> Does not look like a board. We do not take some testing/debugging
> thingies. Please upstream *real* board.
No problem, will swap this out for a cheap available modem that I like to dev on.
>
>> @@ -0,0 +1,19 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +
>> +#include "en751221.dtsi"
>> +
>> +/ {
>> +	model = "Generic EN751221";
> Missing compatible.
>
>> +
>> +	memory@0 {
>> +		/* We hope at least 64MB will be available wherever we are run */
>> +		device_type = "memory";
>> +		reg = <0x00000000 0x4000000>;
>> +	};
>> +
>> +	chosen {
>> +		bootargs = "console=ttyS0,115200";
> Drop bootargs and use standard property - stdout.
>
> See how all other platforms are doing it (and not some ancient MIPS, but
> the most recent arm64 or riscv).
Sure thing.
>
>
>> +		linux,usable-memory-range = <0x00020000 0x3fe0000>;
>> +	};
>> +};
>> diff --git a/arch/mips/econet/Kconfig b/arch/mips/econet/Kconfig
>> new file mode 100644
>> index 000000000000..12f85d638e47
>> --- /dev/null
>> +++ b/arch/mips/econet/Kconfig
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +if ECONET
>> +
>> +config SOC_ECONET_EN751221
>> +	bool
>> +	select COMMON_CLK
>> +	select ECONET_EN751221_INTC
>> +	select IRQ_MIPS_CPU
>> +	select SMP
>> +	select SMP_UP
>> +	select SYS_SUPPORTS_SMP
>> +
>> +choice
>> +	prompt "EcoNet SoC selection"
>> +	default SOC_ECONET_EN751221_FAMILY
>> +	help
>> +	  Select EcoNet MIPS SoC type. Individual SoCs within a family are
>> +	  similar enough that is it enough to select the right family, and
>> +	  then customize to the specific SoC using the device tree only.
>> +
>> +	config SOC_ECONET_EN751221_FAMILY
>> +		bool "EN751221 family"
>> +		select SOC_ECONET_EN751221
>> +		help
>> +		  The EN751221 family includes EN7512, RN7513, EN7521, EN7526.
>> +		  They are based on single core MIPS 34Kc processors. To boot
>> +		  this kernel, you will need a device tree such as
>> +		  MIPS_RAW_APPENDED_DTB=y, and a root filesystem.
>> +
>> +	config SOC_ECONET_EN751221_TEST_IMAGE
>> +		bool "EN751221 test image"
>> +		select SOC_ECONET_EN751221
>> +		select BUILTIN_DTB
>> +		help
>> +		  Build a minimal kernel that will boot on any EN751221 board
>> +		  with at least 64MB of memory. This has a builtin device tree
>> +		  so it can boot with nothing more than an appended initramfs.
>> +		  This is good for validating that a given SoC is EN751221
>> +		  compatible, or for regression testing.
>> +endchoice
>> +
>> +endif
>> diff --git a/arch/mips/econet/Makefile b/arch/mips/econet/Makefile
>> new file mode 100644
>> index 000000000000..7e4529e7d3d7
>> --- /dev/null
>> +++ b/arch/mips/econet/Makefile
>> @@ -0,0 +1,2 @@
>> +
>> +obj-y := init.o
>> diff --git a/arch/mips/econet/Platform b/arch/mips/econet/Platform
>> new file mode 100644
>> index 000000000000..bb659876d855
>> --- /dev/null
>> +++ b/arch/mips/econet/Platform
>> @@ -0,0 +1,5 @@
>> +# To address a 7.2MB kernel size limit in the EcoNet SDK bootloader,
>> +# we put the load address well above where the bootloader loads and then use
>> +# zboot. So please set CONFIG_ZBOOT_LOAD_ADDRESS to the address where your
>> +# bootloader actually places the kernel.
>> +load-$(CONFIG_ECONET)	+= 0xffffffff81000000
>> \ No newline at end of file
> You have patch warnings

"No newline at end of file" - that's embarrassing, I'll try to figure out how that got into the patch.


Thank you very much for your time.


Thanks,

Caleb


>
> Best regards,
> Krzysztof

[PATCH v1 8/8] MAINTAINERS: Add EcoNet MIPS platform entry

[Caleb James DeLisle]

Add a MAINTAINERS entry for the EcoNet EN751221 platform, covering its
device tree bindings, platform code, interrupt controller, and timer
drivers.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index efee40ea589f..fcb1c49ee54e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8182,6 +8182,18 @@ W:	https://linuxtv.org
 Q:	http://patchwork.linuxtv.org/project/linux-media/list/
 F:	drivers/media/dvb-frontends/ec100*
 
+ECONET MIPS PLATFORM
+M:	Caleb James DeLisle <cjd@cjdns.fr>
+S:	Maintained
+F:	Documentation/devicetree/bindings/interrupt-controller/econet,en751221-intc.yaml
+F:	Documentation/devicetree/bindings/mips/econet.yaml
+F:	Documentation/devicetree/bindings/timer/econet,timer-hpt.yaml
+F:	Documentation/devicetree/bindings/vendor-prefixes.yaml
+F:	arch/mips/boot/dts/econet/
+F:	arch/mips/econet/
+F:	drivers/clocksource/timer-econet-hpt.c
+F:	drivers/irqchip/irq-econet-en751221.c
+
 ECRYPT FILE SYSTEM
 M:	Tyler Hicks <code@tyhicks.com>
 L:	ecryptfs@vger.kernel.org
-- 
2.30.2