| 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
| ||
|
Message-ID: <434c5199-613e-4765-af44-d7404a4554dc@kalrayinc.com>
Date: Wed, 31 Jul 2024 17:38:45 +0200
From: Yann Sionneau <ysionneau@...rayinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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
Hello Krzysztof,
On 22/07/2024 11:55, Krzysztof Kozlowski wrote:
> 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>
Ack, I will fix this.
>
>> + #size-cells = <0x02>;
>> +
>> + chosen {
>> + stdout-path = "/axi/serial@...10000";
> No, use phandle/label.
Ack, I will fix this. However can you point me to where this is documented? In https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt I can see a path is used as example and not a phandle/label.
>
>> + };
>> +
>> + memory@...000000 {
>> + phandle = <0x40>;
>> + reg = <0x01 0x00 0x00 0x8000000>;
>> + device_type = "memory";
>> + };
>> +
>> + axi {
>> + compatible = "simple-bus";
>> + #address-cells = <0x02>;
> Same problem.
Ack.
>
>
>> + #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
I fail to understand what I should put even after reading the link above. This node is kind of "generic" and could be used either for a virtio-block device or a virtio-net device.
Could you elaborate on this please?
>
>
>> + 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.
Oops, ack, I will fix this and replace "_" with "-" in node/property names.
>
>> + 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.
Ack, I will fix this.
>
>
>> + 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.
Oops, ack, I will fix this.
>
>
>> + };
>> +
>> + 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?
I should put all memory nodes one after another? Ok I will do this.
>
>> + device_type = "memory";
>> + reg = <0x00 0x00 0x00 0x400000>;
>> + };
>> +
>> + apic_mailbox: apic_mailbox@...000 {
> Why this is outside of SoC? Where is the SoC anyway?
Oops, I didn't know it was mandatory to put a soc { } in the DT, I've browsed the DT spec and the "soc" node is not formally described as something special. Maybe this needs to be documented somewhere?
I reckon it's a nice way to separate what's on the board (PCB) and what's in the SoC.
I'll add a `soc { [...] };` in the next patch iteration that will contain what's in the SoC.
>
>> + 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.
SoC name is "Coolidge" and the "APIC Mailbox" hw is in the SoC, it is memory mapped.
But I guess this point is now already more clear since my last emails answering the "core intc" reviews.
>
>> + reg = <0x00 0xa00000 0x00 0xea00>;
>> + #interrupt-cells = <0x00>;
>> + #address-cells = <0>;
> And this is not <0x0>? It's like random coding style.
Oops, I will clean this up for next iteration.
>
> I stopped reviewing here. Rest of the DTS does not look better.
I'm sorry if it looks like a mess here, I am new to submitting DT upstream and it looks like I failed to apply all the rules.
I thank you for taking the time to review it nonetheless and I hope my next patch iterations will be better and easier to review.
Regards,
--
Yann
Powered by blists - more mailing lists