On 22/07/2024 11:41, ysionneau@xxxxxxxxxxxxx wrote:
> From: Yann Sionneau <ysionneau@xxxxxxxxxxxxx>
>
> Add device tree for QEMU that emulates a Coolidge V1 SoC.
>
> Signed-off-by: Yann Sionneau <ysionneau@xxxxxxxxxxxxx>
> ---
>
> Notes:
>
> V2 -> V3: New patch
> ---
> arch/kvx/boot/dts/Makefile | 1 +
> arch/kvx/boot/dts/coolidge-qemu.dts | 444 ++++++++++++++++++++++++++++
> 2 files changed, 445 insertions(+)
> create mode 100644 arch/kvx/boot/dts/Makefile
> create mode 100644 arch/kvx/boot/dts/coolidge-qemu.dts
>
> diff --git a/arch/kvx/boot/dts/Makefile b/arch/kvx/boot/dts/Makefile
> new file mode 100644
> index 0000000000000..cd27ceb7a6cce
> --- /dev/null
> +++ b/arch/kvx/boot/dts/Makefile
> @@ -0,0 +1 @@
> +dtb-y += coolidge-qemu.dtb
> diff --git a/arch/kvx/boot/dts/coolidge-qemu.dts b/arch/kvx/boot/dts/coolidge-qemu.dts
> new file mode 100644
> index 0000000000000..1d5af0d2e687d
> --- /dev/null
> +++ b/arch/kvx/boot/dts/coolidge-qemu.dts
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/dts-v1/;
> +/*
> + * Copyright (C) 2024, Kalray Inc.
> + */
> +
> +/ {
> + model = "Kalray Coolidge processor (QEMU)";
> + compatible = "kalray,coolidge-qemu";
> + #address-cells = <0x02>;
That's not a hex, so just <2>
> + #size-cells = <0x02>;
> +
> + chosen {
> + stdout-path = "/axi/serial@20210000";
No, use phandle/label.
> + };
> +
> + memory@100000000 {
> + phandle = <0x40>;
> + reg = <0x01 0x00 0x00 0x8000000>;
> + device_type = "memory";
> + };
> +
> + axi {
> + compatible = "simple-bus";
> + #address-cells = <0x02>;
Same problem.
> + #size-cells = <0x02>;
> + ranges;
> +
> + virtio-mmio@30003c00 {
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
> + compatible = "virtio,mmio";
> + reg = <0x00 0x30003c00 0x00 0x200>;
> + interrupt-parent = <&itgen0>;
> + interrupts = <0x9e 0x04>;
> + };
> +
> + virtio-mmio@30003e00 {
> + compatible = "virtio,mmio";
> + reg = <0x00 0x30003e00 0x00 0x200>;
> + interrupt-parent = <&itgen0>;
> + interrupts = <0x9f 0x04>;
> + };
> +
> + itgen0: itgen_soc_periph0@27000000 {
Please follow DTS coding style.
> + compatible = "kalray,coolidge-itgen";
> + reg = <0x00 0x27000000 0x00 0x1104>;
> + msi-parent = <&apic_mailbox>;
> + #interrupt-cells = <0x02>;
> + interrupt-controller;
> + };
> +
> + serial@20210000 {
> + reg-shift = <0x02>;
> + reg-io-width = <0x04>;
Sorry, but width and shift are rarely hex values. Make your code
readable. Adhere to existing coding style.
> + clocks = <&ref_clk>;
> + interrupts = <0x29 0x04>;
> + interrupt-parent = <&itgen0>;
> + reg = <0x00 0x20210000 0x00 0x100>;
> + compatible = "snps,dw-apb-uart";
Follow DTS coding style - order the properties correctly.
> + };
> +
> + serial@20211000 {
> + reg-shift = <0x02>;
> + reg-io-width = <0x04>;
> + phandle = <0x3c>;
> + clocks = <&ref_clk>;
> + interrupts = <0x2a 0x04>;
> + interrupt-parent = <&itgen0>;
> + reg = <0x00 0x20211000 0x00 0x100>;
> + compatible = "snps,dw-apb-uart";
> + };
> +
> + serial@20212000 {
> + reg-shift = <0x02>;
> + reg-io-width = <0x04>;
> + phandle = <0x3b>;
> + clocks = <&ref_clk>;
> + interrupts = <0x2b 0x04>;
> + interrupt-parent = <&itgen0>;
> + reg = <0x00 0x20212000 0x00 0x100>;
> + compatible = "snps,dw-apb-uart";
> + };
> +
> + serial@20213000 {
> + reg-shift = <0x02>;
> + reg-io-width = <0x04>;
> + phandle = <0x3a>;
> + clocks = <&ref_clk>;
> + interrupts = <0x2c 0x04>;
> + interrupt-parent = <&itgen0>;
> + reg = <0x00 0x20213000 0x00 0x100>;
> + compatible = "snps,dw-apb-uart";
> + };
> +
> + serial@20214000 {
> + reg-shift = <0x02>;
> + reg-io-width = <0x04>;
> + phandle = <0x39>;
> + clocks = <&ref_clk>;
> + interrupts = <0x2d 0x04>;
> + interrupt-parent = <&itgen0>;
> + reg = <0x00 0x20214000 0x00 0x100>;
> + compatible = "snps,dw-apb-uart";
> + };
> +
> + serial@20215000 {
> + reg-shift = <0x02>;
> + reg-io-width = <0x04>;
> + phandle = <0x38>;
> + clocks = <&ref_clk>;
> + interrupts = <0x2e 0x04>;
> + interrupt-parent = <&itgen0>;
> + reg = <0x00 0x20215000 0x00 0x100>;
> + compatible = "snps,dw-apb-uart";
> + };
> + };
> +
> + memory@0 {
Why memory is in multiple places?
> + device_type = "memory";
> + reg = <0x00 0x00 0x00 0x400000>;
> + };
> +
> + apic_mailbox: apic_mailbox@a00000 {
Why this is outside of SoC? Where is the SoC anyway?
> + compatible = "kalray,coolidge-apic-mailbox";
Your compatibles are confusing. What is the soc name? In other binding
you entirely omitted coolidge. See writing bindings (or any other recent
DTS which passed review) - it has rationale behind it.
> + reg = <0x00 0xa00000 0x00 0xea00>;
> + #interrupt-cells = <0x00>;
> + #address-cells = <0>;
And this is not <0x0>? It's like random coding style.
I stopped reviewing here. Rest of the DTS does not look better.
Best regards,
Krzysztof
