linux-kernel - Re: [RFC PATCH v3 36/37] kvx: dts: DeviceTree for qemu emulated Coolidge SoC

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: <f032dd83-8e5d-4062-b8a7-ef98924ac9b4@kernel.org>
Date: Wed, 31 Jul 2024 18:57:27 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Yann Sionneau <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 31/07/2024 17:38, Yann Sionneau wrote:

>>
>>> +	#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.

That's just sanity and common (maybe except Tegra) style... Almost every
source uses this, because it gives you code which is independent of node
ordering, node names and unit addresses.

What if I change in next patch axi->soc, how usually we code it?

Override by label/phandle was mentioned many times on mailing lists as
preferred.

>>
>>> +	};
>>> +
>>> +	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?

Just go with virtio.

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

Coding style, emails, all new DTS since some years or talks on numerous
conferences... All this code looks like you took some vendor downstream
code and sent it. That's not how it works. You take recent, reviewed DTS
and use it as template. Qualcomm sm8650 or x1e8010 are good examples.

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


Best regards,
Krzysztof