Re: [PATCH 03/10] arm64: dts: sophgo: Add initial SG2000 SoC device tree

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Alexander Sverdlin <alexander.sverdlin@gmail.com>, soc@lists.linux.dev
Cc: Chen Wang <unicorn_wang@outlook.com>,
	Inochi Amaoto <inochiama@outlook.com>,
	linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
	Haylen Chu <heylenay@outlook.com>,
	linux-arm-kernel@lists.infradead.org,
	Arnd Bergmann <arnd@arndb.de>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>
Subject: Re: [PATCH 03/10] arm64: dts: sophgo: Add initial SG2000 SoC device tree
Date: Tue, 11 Feb 2025 09:07:00 +0100	[thread overview]
Message-ID: <bf8f3ea1-6fb5-40d1-a823-b3bc272a2257@kernel.org> (raw)
In-Reply-To: <03ff407fe68e46df4844e5dd244e7c609331af71.camel@gmail.com>

On 10/02/2025 16:01, Alexander Sverdlin wrote:
> Hi Krzysztof!
> 
> On Mon, 2025-02-10 at 09:45 +0100, Krzysztof Kozlowski wrote:
>> On 09/02/2025 23:06, Alexander Sverdlin wrote:
>>> Add initial device tree for the SG2000 SoC by SOPHGO (from ARM64 PoV).
>>>
>>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
>>> ---
>>>   arch/arm64/boot/dts/sophgo/sg2000.dtsi | 79 ++++++++++++++++++++++++++
>>>   1 file changed, 79 insertions(+)
>>>   create mode 100644 arch/arm64/boot/dts/sophgo/sg2000.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/sophgo/sg2000.dtsi b/arch/arm64/boot/dts/sophgo/sg2000.dtsi
>>> new file mode 100644
>>> index 000000000000..4e520486cbe5
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/sophgo/sg2000.dtsi
>>> @@ -0,0 +1,79 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +
>>> +#define SOC_PERIPHERAL_IRQ(nr)		GIC_SPI (nr)
>>> +
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <riscv/sophgo/cv18xx-periph.dtsi>
>>> +#include <riscv/sophgo/cv181x.dtsi>
>>> +
>>> +/ {
> 
> [...]
> 
>>> +	gic: interrupt-controller@1f01000 {
>>
>> MMIO nodes are always in the soc.
> 
> I think I've looked a wrong example (or a counter-example)...
> $ grep -R -P '^\t\tcompatible = "arm,cortex-a15-gic";' *
> arm/boot/dts/intel/axm/axm55xx.dtsi:		compatible = "arm,cortex-a15-gic";
> arm/boot/dts/ti/omap/dra7.dtsi:		compatible = "arm,cortex-a15-gic";
> arm/boot/dts/ti/omap/omap5.dtsi:		compatible = "arm,cortex-a15-gic";
> arm/boot/dts/nvidia/tegra124.dtsi:		compatible = "arm,cortex-a15-gic";
> arm/boot/dts/nvidia/tegra114.dtsi:		compatible = "arm,cortex-a15-gic";

These are all old.

> arm64/boot/dts/nvidia/tegra132.dtsi:		compatible = "arm,cortex-a15-gic";
> arm64/boot/dts/freescale/s32v234.dtsi:		compatible = "arm,cortex-a15-gic";

Well, it happens.

> arm64/boot/dts/apm/apm-storm.dtsi:		compatible = "arm,cortex-a15-gic";
> arm64/boot/dts/apm/apm-shadowcat.dtsi:		compatible = "arm,cortex-a15-gic";

These are strong anti-patterns. Unmaintained.

> 
> But thanks for clarification!
> 
>>> +		compatible = "arm,cortex-a15-gic";
>>> +		interrupt-controller;
>>> +		#interrupt-cells = <3>;
>>> +		reg = <0x01f01000 0x1000>,
>>> +		      <0x01f02000 0x2000>;
>>> +	};
>>> +
>>> +	soc {
>>
>> Override by phandle/label instead of duplicating.
>>
>>> +		ranges;
>>> +
>>> +		pinctrl: pinctrl@3001000 {
>>> +			compatible = "sophgo,sg2000-pinctrl";
>>> +			reg = <0x03001000 0x1000>,
>>> +			      <0x05027000 0x1000>;
>>> +			reg-names = "sys", "rtc";
>>> +		};
>>> +	};
>>> +};
>>> +
>>> +
>>> +&clk {
>>> +	compatible = "sophgo,sg2000-clk";
>>
>>
>> That's discouraged practice. If you need to define compatible, it means
>> the block is not shared between designs and must not be in common DTSI.
> 
> That doesn't come from my series, that's how original cv18xx.dtsi has been

You can change the other file to match real hardware. But if original
cv18xx.dtsi has incorrect or imprecise compatible, I wonder how does it
work....

> designed. Same question as before: do I need to rework it if I will not be able to
> test the changes (I don't even posess the relevant HW). But if not, I have
> to adapt the same pattern into the new sg2000.dtsi.

Best regards,
Krzysztof

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv