lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d93f93fa-bbc8-4b89-9abc-767486bc443c@kernel.org>
Date: Mon, 22 Jul 2024 11:55:46 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: ysionneau@...rayinc.com, linux-kernel@...r.kernel.org,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: Jonathan Borne <jborne@...rayinc.com>,
 Julian Vetter <jvetter@...rayinc.com>, devicetree@...r.kernel.org
Subject: Re: [RFC PATCH v3 36/37] kvx: dts: DeviceTree for qemu emulated
 Coolidge SoC

On 22/07/2024 11:41, ysionneau@...rayinc.com wrote:
> From: Yann Sionneau <ysionneau@...rayinc.com>
> 
> Add device tree for QEMU that emulates a Coolidge V1 SoC.
> 
> Signed-off-by: Yann Sionneau <ysionneau@...rayinc.com>
> ---
> 
> 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@...10000";

No, use phandle/label.

> +	};
> +
> +	memory@...000000 {
> +		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@...03c00 {

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@...03e00 {
> +			compatible = "virtio,mmio";
> +			reg = <0x00 0x30003e00 0x00 0x200>;
> +			interrupt-parent = <&itgen0>;
> +			interrupts = <0x9f 0x04>;
> +		};
> +
> +		itgen0: itgen_soc_periph0@...00000 {

Please follow DTS coding style.

> +			compatible = "kalray,coolidge-itgen";
> +			reg = <0x00 0x27000000 0x00 0x1104>;
> +			msi-parent = <&apic_mailbox>;
> +			#interrupt-cells = <0x02>;
> +			interrupt-controller;
> +		};
> +
> +		serial@...10000 {
> +			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@...11000 {
> +			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@...12000 {
> +			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@...13000 {
> +			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@...14000 {
> +			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@...15000 {
> +			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@...000 {

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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ