RE: [PATCH 2/2] arm64: dts: realtek: Add RTD1319 SoC and Realtek PymParticle EVB

From: James Tai <james.tai@realtek.com>
To: "Andreas Färber" <afaerber@suse.de>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-realtek-soc@lists.infradead.org" 
	<linux-realtek-soc@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: RE: [PATCH 2/2] arm64: dts: realtek: Add RTD1319 SoC and Realtek PymParticle EVB
Date: Fri, 27 Dec 2019 07:17:25 +0000	[thread overview]
Message-ID: <a673b4e4bb454bad99ad483e1cccbbb1@realtek.com> (raw)
In-Reply-To: <4040ffcf-5c54-fb44-b0a8-ce0c8c21b93f@suse.de>

Hi Andreas,

> This hunk is lacking rtd1395, so is not based on the latest patches I posted. I
> expect you to be developing against linux-next.git tree, and when there's
> relevant in-flight patches, you'll need to either apply my patches via git-am to
> your tree, or for convenience you can use the beginning of my (but better not
> the full experimental) rtd1295-next branch (git-rebase -i, or (careful!) git-reset
> --hard). Yes, neither is super-easy.
> 

I will use your rtd1295-next branch for development.

> Same as with the binding, it would seem better to not add this at the end, even
> if it's your newest family. Consider this: Someone finds an
> RTD1036 in their household and wants to contribute a patch - where would
> they add it? I don't want all newly added stuff to go into the bottom of the file
> (then it'll be hard to find and potentially causes conflicts), so we need a stable
> sort order where I don't need to do historical research of whether 1036 is
> newer or older than 1195/1296 to determine where to insert it in a file.
> Alphanumerical sort order seems simplest to understand and is proven
> elsewhere to reduce merge conflicts.

Thanks for your kind reminder.

> > diff --git a/arch/arm64/boot/dts/realtek/rtd1319-pymparticle.dts
> > b/arch/arm64/boot/dts/realtek/rtd1319-pymparticle.dts
> > new file mode 100644
> > index 000000000000..d8bfe2304b71
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/realtek/rtd1319-pymparticle.dts
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> > +/*
> > + * Copyright (c) 2019 Realtek Semiconductor Corp.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "rtd1319.dtsi"
> > +
> > +/ {
> > +	compatible = "realtek,pymparticle", "realtek,rtd1319";
> 
> Thanks, correct order now.
> 
> > +	model = "Realtek PymParticle EVB";
> > +
> > +	memory@0 {
> > +		device_type = "memory";
> > +		reg = <0x0 0x80000000>;
> > +	};
> 
> No! I understood RTD1319 has the same boot ROM size 184 KiB as RTD1619,
> so please look at the patches I posted, including fix for RTD1619 [1], and fix
> this yourself here. A comment for humans would also be nice.
> 
> In the public BPI-M4-bsp code I see one -pymparticle-1GB.CMAx2.dts file.
> If this board is available with less than 2 GiB RAM then please use the lower
> value to be safe - you can run a 2 GiB board with 1 GiB RAM used, but using
> more RAM than available will break.
> 
> [1] https://patchwork.kernel.org/patch/11268969/
> 

I will fix it in next version patch.

> > +++ b/arch/arm64/boot/dts/realtek/rtd1319.dtsi
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> > +/*
> > + * Realtek RTD1319 SoC
> > + *
> > + * Copyright (c) 2019 Realtek Semiconductor Corp.
> > + */
> > +
> > +#include "rtd13xx.dtsi"
> > +
> > +/ {
> > +	compatible = "realtek,rtd1319";
> > +};
> 
> What other contents are you expecting to add in this file?

I expect add USB, SATA, PCIE and so on.

> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu0: cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a55";
> > +			reg = <0x0>;
> > +			enable-method = "psci";
> > +			next-level-cache = <&l2>;
> > +		};
> > +
> > +		cpu1: cpu@100 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a55";
> > +			reg = <0x100>;
> > +			enable-method = "psci";
> > +			next-level-cache = <&l2>;
> > +		};
> > +
> > +		cpu2: cpu@200 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a55";
> > +			reg = <0x200>;
> > +			enable-method = "psci";
> > +			next-level-cache = <&l2>;
> > +		};
> > +
> > +		cpu3: cpu@300 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a55";
> > +			reg = <0x300>;
> > +			enable-method = "psci";
> > +			next-level-cache = <&l2>;
> > +		};
> > +
> > +		l2: l2-cache {
> > +			compatible = "cache";
> > +		};
> 
> I note this seems a different cache topology than RTD1619?
> 
Yes, It is different cache topology than RTD1619.
The RTD1319 don't have an L3 cache.

> > +	osc27M: osc {
> > +		compatible = "fixed-clock";
> > +		clock-frequency = <27000000>;
> > +		clock-output-names = "osc27M";
> 
> BTW I recall seeing "osc27m" in your clk patchset. We should decide on one
> name and stick with it consistently, and I think it's best to have this as a node
> here in .dtsi (or in .dts), in case OEMs ever choose to have it generated by
> some other non-trivial IC.

I understand.

> > +		#clock-cells = <0>;
> > +	};
> > +
> > +	soc {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges = <0x98000000 0x98000000 0x68000000>;
> 
> No! Lacking a range for boot ROM. And your range is probably too large due to
> high RAM. Please see [1] and fix for both. r-bus ranges below would indicate
> that above soc range should be 0x00200000 long only, plus extra ranges for
> whatever besides r-bus is shadowing RAM (e.g., GIC).
> 

I will fix it in next version patch.

> > +
> > +			uart0: serial0@7800 {
> > +				compatible = "snps,dw-apb-uart";
> > +				reg = <0x7800 0x400>;
> > +				reg-shift = <2>;
> > +				reg-io-width = <4>;
> > +				interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
> > +				clock-frequency = <432000000>;
> > +				status = "disabled";
> > +			};
> > +
> > +			uart1: serial1@1b200 {
> > +				compatible = "snps,dw-apb-uart";
> > +				reg = <0x1b200 0x400>;
> > +				reg-shift = <2>;
> > +				reg-io-width = <4>;
> > +				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > +				clock-frequency = <432000000>;
> > +				status = "disabled";
> > +			};
> > +
> > +			uart2: serial2@1b400 {
> > +				compatible = "snps,dw-apb-uart";
> > +				reg = <0x1b400 0x400>;
> > +				reg-shift = <2>;
> > +				reg-io-width = <4>;
> > +				interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> > +				clock-frequency = <432000000>;
> > +				status = "disabled";
> > +			};
> 
> Here you appear to ignore my patches introducing syscon for ISO & MISC!
> 
> See https://patchwork.kernel.org/cover/11269453/

I will apply the syscon for ISO & MISC in next version patch.

> > +		};
> > +
> > +		gic: interrupt-controller@ff100000 {
> > +			compatible = "arm,gic-v3";
> > +			reg = <0xff100000 0x10000>,
> > +			      <0xff140000 0xc0000>;
> > +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <3>;
> > +		};
> > +	};
> > +};
> 

Regards,
James