[PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

This patch adds a generic devicetree board file and a dtsi for
Realtek rtd1619 SoC.

Signed-off-by: james tai <james.tai@realtek.com>
---
 arch/arm64/boot/dts/realtek/Makefile          |  3 +
 .../boot/dts/realtek/rtd1619-mjolnir.dts      | 31 +++++++
 arch/arm64/boot/dts/realtek/rtd1619.dtsi      | 91 +++++++++++++++++++
 arch/arm64/boot/dts/realtek/rtd16xx.dtsi      | 66 ++++++++++++++
 4 files changed, 191 insertions(+)
 create mode 100644 arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts
 create mode 100644 arch/arm64/boot/dts/realtek/rtd1619.dtsi
 create mode 100644 arch/arm64/boot/dts/realtek/rtd16xx.dtsi

diff --git a/arch/arm64/boot/dts/realtek/Makefile b/arch/arm64/boot/dts/realtek/Makefile
index 555638ada721..a58353a1d99a 100644
--- a/arch/arm64/boot/dts/realtek/Makefile
+++ b/arch/arm64/boot/dts/realtek/Makefile
@@ -7,3 +7,6 @@ dtb-$(CONFIG_ARCH_REALTEK) += rtd1295-probox2-ava.dtb
 dtb-$(CONFIG_ARCH_REALTEK) += rtd1295-zidoo-x9s.dtb
 
 dtb-$(CONFIG_ARCH_REALTEK) += rtd1296-ds418.dtb
+
+dtb-$(CONFIG_ARCH_REALTEK) += rtd1619-mjolnir.dtb
+
diff --git a/arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts b/arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts
new file mode 100644
index 000000000000..def5964c7715
--- /dev/null
+++ b/arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Copyright (c) 2019 Realtek Semiconductor Corp.
+ */
+
+/dts-v1/;
+
+#include "rtd1619.dtsi"
+
+/ {
+	compatible = "realtek,rtd1619";
+	model= "Realtek Mjolnir EVB";
+
+	memory@0 {
+		device_type = "memory";
+		reg = <0x0 0x0 0x0 0x80000000>;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	aliases {
+		serial0 = &uart0;
+	};
+};
+
+&uart0 {
+	status = "okay";
+};
+
diff --git a/arch/arm64/boot/dts/realtek/rtd1619.dtsi b/arch/arm64/boot/dts/realtek/rtd1619.dtsi
new file mode 100644
index 000000000000..ac5c737b04db
--- /dev/null
+++ b/arch/arm64/boot/dts/realtek/rtd1619.dtsi
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek RTD1619 SoC
+ *
+ * Copyright (c) 2019 Realtek Semiconductor Corp.
+ */
+
+#include "rtd16xx.dtsi"
+
+/ {
+	compatible = "realtek,rtd1619";
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x000>;
+			next-level-cache = <&l2>;
+		};
+
+		cpu1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x100>;
+			enable-method = "psci";
+			next-level-cache = <&l3>;
+		};
+
+		cpu2: cpu@2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x200>;
+			enable-method = "psci";
+			next-level-cache = <&l3>;
+		};
+
+		cpu3: cpu@3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x300>;
+			enable-method = "psci";
+			next-level-cache = <&l3>;
+		};
+
+		cpu4: cpu@4 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x400>;
+			enable-method = "psci";
+			next-level-cache = <&l3>;
+		};
+
+		cpu5: cpu@5 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a55";
+			reg = <0x500>;
+			enable-method = "psci";
+			next-level-cache = <&l3>;
+		};
+
+		l2: l2-cache {
+			compatible = "cache";
+			next-level-cache = <&l3>;
+
+		};
+
+		l3: l3-cache {
+			compatible = "cache";
+		};
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13
+			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 14
+			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 11
+			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 10
+			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+};
+
+&arm_pmu {
+	interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>,
+		<&cpu3>, <&cpu4>, <&cpu5>;
+};
diff --git a/arch/arm64/boot/dts/realtek/rtd16xx.dtsi b/arch/arm64/boot/dts/realtek/rtd16xx.dtsi
new file mode 100644
index 000000000000..ef56c6ed6663
--- /dev/null
+++ b/arch/arm64/boot/dts/realtek/rtd16xx.dtsi
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek RTD1619 SoC
+ *
+ * Copyright (c) 2019 Realtek Semiconductor Corp.
+ */
+
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/{
+	compatible = "realtek,rtd1619";
+	interrupt-parent = <&gic>;
+	#address-cells = <0x2>;
+	#size-cells = <0x2>;
+
+	arm_pmu: pmu {
+		compatible = "arm,armv8-pmuv3";
+		interrupts = <GIC_PPI 7
+			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+
+	osc27M: osc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <27000000>;
+		clock-output-names = "osc27M";
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+	};
+
+	soc {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		uart0: serial0@98007800 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x98007800 0x0 0x400>,
+				<0x0 0x98007000 0x0 0x100>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			interrupts = <0 68 4>;
+			clock-frequency = <27000000>;
+			status = "disabled";
+		};
+
+		gic: interrupt-controller@ff100000 {
+			compatible = "arm,gic-v3";
+			#interrupt-cells = <3>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			interrupt-controller;
+			redistributor-stride = <0x0 0x20000>;
+			#redistributor-regions = <1>;
+			reg = <0x0 0xff100000 0x0 0x10000>,
+				<0x0 0xff140000 0x0 0x200000>;
+			interrupts = <GIC_PPI 9 4>;
+		};
+	};
+};
-- 
2.17.1

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Hi James,

Thanks for your patch.

$subject: "RTD1619", "Mjolnir". (I can fix up such spelling nits myself
when applying, but there's more change requests below, so please do.)

In theory "Realtek RTD1619" is redundant with "realtek:". Compare recent
patches on linux-realtek-soc or
`git log --oneline -- arch/arm64/boot/dts/realtek/` on linux-next.git.
Not wrong obviously.

Am 05.11.19 um 09:00 schrieb James Tai:
> This patch adds a generic devicetree board file and a dtsi for
> Realtek rtd1619 SoC.

There is no such thing as a "generic devicetree board file"! It is
specific to that one board. If you create a second eval board or when
your ODM customers design their own boards they should only be reusing
the rtd1619.dtsi, as seen with the various rtd1295-*.dts files.

Also, once a patch gets applied to Git, it becomes a commit, so avoid
the term "patch" in the commit message. What about the following:

"Add Device Trees for Realtek RTD1619 SoC family, RTD1619 SoC and
Realtek's Mjolnir evaluation board." (or "This adds ...")

> 
> Signed-off-by: james tai <james.tai@realtek.com>

You've already fixed this from "james.tai", but can you please adjust
your config again to match the regular Western spelling of "James Tai"
in From? Thanks. (In theory UTF-8 would also allow you to add your
Chinese name, like you did in an earlier From, if you wanted to.)

> ---
>  arch/arm64/boot/dts/realtek/Makefile          |  3 +
>  .../boot/dts/realtek/rtd1619-mjolnir.dts      | 31 +++++++
>  arch/arm64/boot/dts/realtek/rtd1619.dtsi      | 91 +++++++++++++++++++
>  arch/arm64/boot/dts/realtek/rtd16xx.dtsi      | 66 ++++++++++++++
>  4 files changed, 191 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts
>  create mode 100644 arch/arm64/boot/dts/realtek/rtd1619.dtsi
>  create mode 100644 arch/arm64/boot/dts/realtek/rtd16xx.dtsi
> 
> diff --git a/arch/arm64/boot/dts/realtek/Makefile b/arch/arm64/boot/dts/realtek/Makefile
> index 555638ada721..a58353a1d99a 100644
> --- a/arch/arm64/boot/dts/realtek/Makefile
> +++ b/arch/arm64/boot/dts/realtek/Makefile
> @@ -7,3 +7,6 @@ dtb-$(CONFIG_ARCH_REALTEK) += rtd1295-probox2-ava.dtb
>  dtb-$(CONFIG_ARCH_REALTEK) += rtd1295-zidoo-x9s.dtb
>  
>  dtb-$(CONFIG_ARCH_REALTEK) += rtd1296-ds418.dtb
> +
> +dtb-$(CONFIG_ARCH_REALTEK) += rtd1619-mjolnir.dtb
> +

Please don't add trailing newlines, here and elsewhere.

> diff --git a/arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts b/arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts
> new file mode 100644
> index 000000000000..def5964c7715
> --- /dev/null
> +++ b/arch/arm64/boot/dts/realtek/rtd1619-mjolnir.dts
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +/*
> + * Copyright (c) 2019 Realtek Semiconductor Corp.
> + */
> +
> +/dts-v1/;
> +
> +#include "rtd1619.dtsi"
> +
> +/ {
> +	compatible = "realtek,rtd1619";

Please run ./scripts/checkpatch.pl before submitting:

You're not allowed to use compatible strings without defining them first
in a separate "dt-bindings: ..." patch, in this case against
Documentation/devicetree/bindings/arm/realtek.yaml.

Please also define a suitable compatible string specific to this board:
"realtek,mjolnir", "realtek,rtd1619"?

So v2 should be a two-patch series with a cover letter please.

> +	model= "Realtek Mjolnir EVB";
> +
> +	memory@0 {
> +		device_type = "memory";
> +		reg = <0x0 0x0 0x0 0x80000000>;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	aliases {
> +		serial0 = &uart0;
> +	};
> +};
> +
> +&uart0 {
> +	status = "okay";
> +};
> +

Trailing newline.

> diff --git a/arch/arm64/boot/dts/realtek/rtd1619.dtsi b/arch/arm64/boot/dts/realtek/rtd1619.dtsi
> new file mode 100644
> index 000000000000..ac5c737b04db
> --- /dev/null
> +++ b/arch/arm64/boot/dts/realtek/rtd1619.dtsi
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +/*
> + * Realtek RTD1619 SoC
> + *
> + * Copyright (c) 2019 Realtek Semiconductor Corp.
> + */
> +
> +#include "rtd16xx.dtsi"
> +
> +/ {
> +	compatible = "realtek,rtd1619";
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x000>;

Missing enable-method = "psci"?

> +			next-level-cache = <&l2>;
> +		};
> +
> +		cpu1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x100>;

reg value and node unit address need to match, i.e., cpu@100 if that's
the correct value. The label can stay with the intuitive number (cpu1).

> +			enable-method = "psci";
> +			next-level-cache = <&l3>;
> +		};
> +
> +		cpu2: cpu@2 {

Ditto.

> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x200>;
> +			enable-method = "psci";
> +			next-level-cache = <&l3>;
> +		};
> +
> +		cpu3: cpu@3 {

Ditto.

> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x300>;
> +			enable-method = "psci";
> +			next-level-cache = <&l3>;
> +		};
> +
> +		cpu4: cpu@4 {

Ditto.

> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x400>;
> +			enable-method = "psci";
> +			next-level-cache = <&l3>;
> +		};
> +
> +		cpu5: cpu@5 {

Ditto.

> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x500>;
> +			enable-method = "psci";
> +			next-level-cache = <&l3>;
> +		};

Just to be sure: This is one cluster of 6 CPUs? Or is it some 4+2
big.LITTLE, DynamiQ or whatever multi-cluster configuration with
different frequencies, power domains or something?

> +
> +		l2: l2-cache {
> +			compatible = "cache";
> +			next-level-cache = <&l3>;
> +
> +		};
> +
> +		l3: l3-cache {
> +			compatible = "cache";
> +		};

Caches look weird - only cpu0 uses L2 and all others use L3 directly?

> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13
> +			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 14
> +			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 11
> +			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 10
> +			(GIC_CPU_MASK_RAW(0xf) | IRQ_TYPE_LEVEL_LOW)>;

Generic question also applying to my RTD1295/RTD1195 patches: Are you
sure about GIC_CPU_MASK_RAW(0xf) or could this be GIC_CPU_MASK_SIMPLE(6)
in this case? This here would seem equivalent of GIC_CPU_MASK_SIMPLE(8).

> +	};
> +};
> +
> +&arm_pmu {
> +	interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>,
> +		<&cpu3>, <&cpu4>, <&cpu5>;
> +};

Just to be sure: You expect there to be later rtd16*.dtsi files that
would have a different number of CPUs than 6? Otherwise both the cpus
node and the interrupt-affinity should go into rtd16xx.dtsi and only
diverging things should go here. For RTD1295 this was refactored due to
dual-core RTD1293 vs. quad-core RTD1295/96, so just verifying that
you're not unintentionally copying its design pattern.

> diff --git a/arch/arm64/boot/dts/realtek/rtd16xx.dtsi b/arch/arm64/boot/dts/realtek/rtd16xx.dtsi
> new file mode 100644
> index 000000000000..ef56c6ed6663
> --- /dev/null
> +++ b/arch/arm64/boot/dts/realtek/rtd16xx.dtsi
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +/*
> + * Realtek RTD1619 SoC

Hmm, my rtd129x.dtsi has the exact list of SoCs it applies to:
"Realtek RTD1293/RTD1295/RTD1296 SoC"
Copying that pattern here leads to identical description in rtd16xx.dtsi
and rtd1619.dtsi - make that "SoC family" maybe, for distinction?

> + *
> + * Copyright (c) 2019 Realtek Semiconductor Corp.
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>

Please swap these for alphabetic ordering, so that the next contributor
can add further #includes in a well-defined place.

> +
> +/{
> +	compatible = "realtek,rtd1619";

Copy&paste? Suggest to drop it here. You still have it in rtd1619.dtsi.

> +	interrupt-parent = <&gic>;
> +	#address-cells = <0x2>;
> +	#size-cells = <0x2>;

Please always use decimal numbers for these two properties.

And double-check whether you actually need <2> - compare rtd129x.dtsi
using <1> because nothing went beyond 32-bit address space. It was a
review request back then. Can RTD1619 have more than 2 GiB RAM, with a
second RAM region in high mem, requiring two cells for memory nodes?

> +
> +	arm_pmu: pmu {
> +		compatible = "arm,armv8-pmuv3";
> +		interrupts = <GIC_PPI 7
> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;

Here you use GIC_CPU_MASK_SIMPLE(4) but rtd1619.dtsi with 6 CPUs does
not override it with 6 - are you sure about 4 here?

> +	};
> +
> +	osc27M: osc {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;

Order this property last please - it only affects references to this node.

> +		clock-frequency = <27000000>;
> +		clock-output-names = "osc27M";
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";

Lorenzo, should this be "arm,psci-1.0", "arm,psci-0.2"? The YAML schema
allows either, without documenting which one is preferred for new DTs.

> +		method = "smc";
> +	};
> +
> +	soc {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;

Please specify the ranges property in a safe way. Compare RTD1295 (which
you can probably copy from?) and my RTD1195 patches. One of the text
files in Documentation/devicetree/ defines the syntax for ranges.

In addition please use #address-cells and #size-cells of 1 here, if
there are no registers beyond 32-bit address space.

> +
> +		uart0: serial0@98007800 {
> +			compatible = "snps,dw-apb-uart";
> +			reg = <0x0 0x98007800 0x0 0x400>,
> +				<0x0 0x98007000 0x0 0x100>;

This looks wrong... What is the second reg entry for, have you run "make
dtbs_check"? My pending irqchip driver avoids the need to extend each
and every driver with the region for acknowledging their interrupts.

> +			reg-shift = <2>;
> +			reg-io-width = <4>;
> +			interrupts = <0 68 4>;

Please use symbolic names for first and last cell.

Is the UART no longer behind an IRQ mux on RTD1619, or is the above the
IRQ mux interrupt as a workaround for lack of in-tree irqchip driver?

> +			clock-frequency = <27000000>;
> +			status = "disabled";
> +		};

My suggestion here would be to refactor as follows:

	rbus: r-bus@98000000 {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0x98000000 0x0 rbussize>;

		uart0: serial@7800 {
			compatible = ...
			reg = <0x7800 0x400>;
			...
		};
	};

If we do the same for RTD1295 and RTD1195 as proposed earlier, we would
have neatly comparable register offsets independent of {98,18}000000.

> +
> +		gic: interrupt-controller@ff100000 {
> +			compatible = "arm,gic-v3";
> +			#interrupt-cells = <3>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +			interrupt-controller;
> +			redistributor-stride = <0x0 0x20000>;
> +			#redistributor-regions = <1>;
> +			reg = <0x0 0xff100000 0x0 0x10000>,
> +				<0x0 0xff140000 0x0 0x200000>;
> +			interrupts = <GIC_PPI 9 4>;
> +		};

This is really hard to read, please clean up the property order:

reg goes immediately after compatible, so that we have it close to the
node's unit address.

There are no child nodes here or in derived .dts[i] - can we drop
#address-cells, #size-cells and ranges? Otherwise please place them last.

Also please place #interrupt-cells after interrupt-controller - compare
other GICv3 examples for whether they should go after or before
[#]redistributor-*. If we get more such pairs you might use a whiteline
for grouping to aid in reading.

Are you sure you don't have GICC, GICH, GICV and IRQ? No MBI support?

For RTD1295 I extended the GICv2 node during review, and KVM initialized
fine, although I'm not sure whether I've run an actual guest yet, given
how many drivers were missing still.

(I'd also appreciate Realtek to review my RTD1195 patch's GIC [1] for
whether we should have all four regions and some interrupt there - the
OEM's U-Boot doesn't boot in HYP mode, so I can't test myself.)

> +	};
> +};

Thanks,
Andreas

[1] https://patchwork.kernel.org/patch/11221609/

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

Hi Andreas,

Thank you for your review.

[...]
> Just to be sure: This is one cluster of 6 CPUs? Or is it some 4+2 big.LITTLE,
> DynamiQ or whatever multi-cluster configuration with different frequencies,
> power domains or something?
> 
Yes, it is one cluster of 6 CPUs.

[...]
> > +
> > +		l2: l2-cache {
> > +			compatible = "cache";
> > +			next-level-cache = <&l3>;
> > +
> > +		};
> > +
> > +		l3: l3-cache {
> > +			compatible = "cache";
> > +		};
> 
> Caches look weird - only cpu0 uses L2 and all others use L3 directly?
> 
Yes, only cpu0 uses L2 and others use L3 directly.

[...]
> Generic question also applying to my RTD1295/RTD1195 patches: Are you sure
> about GIC_CPU_MASK_RAW(0xf) or could this be GIC_CPU_MASK_SIMPLE(6)
> in this case? This here would seem equivalent of GIC_CPU_MASK_SIMPLE(8).
>
The GICv3 does not have affinity bitmap in the binding for PPI
interrupts. So remove the GIC_CPU_MASK_RAW() macro.

[...] 
> 
> And double-check whether you actually need <2> - compare rtd129x.dtsi using
> <1> because nothing went beyond 32-bit address space. It was a review
> request back then. Can RTD1619 have more than 2 GiB RAM, with a second
> RAM region in high mem, requiring two cells for memory nodes?
> 
The RTD1619 can support more than 2 GiB RAM.

[...]
> 
> Is the UART no longer behind an IRQ mux on RTD1619, or is the above the IRQ
> mux interrupt as a workaround for lack of in-tree irqchip driver?
> 
Yes, the UART no longer behind an IRQ mux on RTD1619.

[...] 
> Are you sure you don't have GICC, GICH, GICV and IRQ? No MBI support?
> 
The RTD1619 don't have GICC, GICH, GICV and no support MBI.

> For RTD1295 I extended the GICv2 node during review, and KVM initialized
> fine, although I'm not sure whether I've run an actual guest yet, given how
> many drivers were missing still.
> 
> (I'd also appreciate Realtek to review my RTD1195 patch's GIC [1] for whether
> we should have all four regions and some interrupt there - the OEM's U-Boot
> doesn't boot in HYP mode, so I can't test myself.)
> 
I will help with review.

[...]

Regards,
James

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Hi James,

Am 08.11.19 um 16:36 schrieb James Tai:
>> And double-check whether you actually need <2> - compare rtd129x.dtsi using
>> <1> because nothing went beyond 32-bit address space. It was a review
>> request back then. Can RTD1619 have more than 2 GiB RAM, with a second
>> RAM region in high mem, requiring two cells for memory nodes?
>>
> The RTD1619 can support more than 2 GiB RAM.

How much? More than 0x98000000? The RTD1395 datasheet says up to 4 GB -
does that mean it continues in a second region beyond 0xffffffff? Those
locations should be excluded in the soc node ranges (which you sadly
appear to have dropped in v2).

I'll try to post a patch for RTD1295 soon to demonstrate, it's just a
little time-consuming with the 100+ commits on top of linux-next that
need to be rebased then... RTD1195 may be quicker.

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

Hi Andreas,

> How much? More than 0x98000000? The RTD1395 datasheet says up to 4 GB -
> does that mean it continues in a second region beyond 0xffffffff? Those
> locations should be excluded in the soc node ranges (which you sadly appear to
> have dropped in v2).
> 

Sorry for my misunderstanding. The RAM region don't require 
two cells for memory nodes, so I'll fix it in v3 patch.

Regards,
James

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Am 08.11.19 um 18:17 schrieb Andreas Färber:
> Hi James,
> 
> Am 08.11.19 um 16:36 schrieb James Tai:
>>> And double-check whether you actually need <2> - compare rtd129x.dtsi using
>>> <1> because nothing went beyond 32-bit address space. It was a review
>>> request back then. Can RTD1619 have more than 2 GiB RAM, with a second
>>> RAM region in high mem, requiring two cells for memory nodes?
>>>
>> The RTD1619 can support more than 2 GiB RAM.
> 
> How much? More than 0x98000000? The RTD1395 datasheet says up to 4 GB -
> does that mean it continues in a second region beyond 0xffffffff? Those
> locations should be excluded in the soc node ranges (which you sadly
> appear to have dropped in v2).
> 
> I'll try to post a patch for RTD1295 soon to demonstrate, it's just a
> little time-consuming with the 100+ commits on top of linux-next that
> need to be rebased then... RTD1195 may be quicker.

I've finished both and included patches for both in the RTD1395 series
just posted, along with other follow-up cleanups recently discussed.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Hi James,

Am 11.11.19 um 03:58 schrieb James Tai:
>> How much? More than 0x98000000? The RTD1395 datasheet says up to 4 GB -
>> does that mean it continues in a second region beyond 0xffffffff? Those
>> locations should be excluded in the soc node ranges (which you sadly appear to
>> have dropped in v2).
> 
> Sorry for my misunderstanding. The RAM region don't require 
> two cells for memory nodes, so I'll fix it in v3 patch.

Should I then also change RTD1395 to use only one cell, or does it
support more RAM than RTD1619?

By my calculation 0x98000000 is less than 2.4 GiB! So, does RAM continue
between r-bus and GIC, similar to how it does on RTD1195? Then we need
to exclude those RAM ranges from the SoC node (adjusting 0x68000000).

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

Hi Andreas,

> > Sorry for my misunderstanding. The RAM region don't require two cells
> > for memory nodes, so I'll fix it in v3 patch.
> 
> Should I then also change RTD1395 to use only one cell, or does it support
> more RAM than RTD1619?

Yes, you can. The memory capacity of RTD1395 and RTD1619 are the same.

> By my calculation 0x98000000 is less than 2.4 GiB! So, does RAM continue
> between r-bus and GIC, similar to how it does on RTD1195? Then we need to
> exclude those RAM ranges from the SoC node (adjusting 0x68000000).

We need to reserve memory address for r-bus and GIC and exclude those RAM range from the SoC node.

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Hi James,

Am 08.11.19 um 16:36 schrieb James Tai:
>> Is the UART no longer behind an IRQ mux on RTD1619, or is the above the IRQ
>> mux interrupt as a workaround for lack of in-tree irqchip driver?
>>
> Yes, the UART no longer behind an IRQ mux on RTD1619.

This conflicts with what I see in BSP irq mux code here:
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/drivers/irqchip/irq-rtd16xx.h

That does show UR0 as bit 2 for the iso irq mux, as for previous SoCs.
Is that code wrong, or does the same UART0 IP block have two alternative
interrupts for backwards compatibility? I therefore held back RTD1619
irq mux patches from my irqchip v4 series [1].

The BSP DT does assign non-mux interrupts to the UART node like you did:
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm64/boot/dts/realtek/rtd16xx/rtd-16xx.dtsi
And I obviously trust that you tested your DT to produce serial output.


Also note how there are UR1_TO and UR2_TO (TO = timeout?) in addition to
regular UR1 and UR2 interrupts in the mux above, just as for RTD1295 and
RTD1195 (UR1/UR1_TO only). From my irqmux v4 series posted last night I
had to drop those additional interrupts property values from the DT [2],
as they violate mainline's DesignWare DT schema's maxItems 1 and would
require a new compatible string (and a driver patch to make use of it).

https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/drivers/irqchip/irq-rtd119x.h
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/drivers/irqchip/irq-rtd129x.h
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/drivers/irqchip/irq-rtd139x.h

Regards,
Andreas

[1] https://patchwork.kernel.org/cover/11250643/
[2] https://patchwork.kernel.org/patch/11250645/

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

Hi Andreas,

> 
> This conflicts with what I see in BSP irq mux code here:
> https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/drivers/ir
> qchip/irq-rtd16xx.h
> 
> That does show UR0 as bit 2 for the iso irq mux, as for previous SoCs.
> Is that code wrong, or does the same UART0 IP block have two alternative
> interrupts for backwards compatibility? I therefore held back RTD1619 irq mux
> patches from my irqchip v4 series [1].
> 
It is code wrong. The UR0 should remove from "irq-rtd16xx.h".

> The BSP DT does assign non-mux interrupts to the UART node like you did:
> https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm
> 64/boot/dts/realtek/rtd16xx/rtd-16xx.dtsi
> And I obviously trust that you tested your DT to produce serial output.
> 

> Also note how there are UR1_TO and UR2_TO (TO = timeout?) in addition to
> regular UR1 and UR2 interrupts in the mux above, just as for RTD1295 and
> RTD1195 (UR1/UR1_TO only). From my irqmux v4 series posted last night I had
> to drop those additional interrupts property values from the DT [2], as they
> violate mainline's DesignWare DT schema's maxItems 1 and would require a
> new compatible string (and a driver patch to make use of it).
> 
Yes, TO is interrupt timeout.


Regards,
James

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Hi James,

Am 20.11.19 um 08:58 schrieb James Tai:
>> This conflicts with what I see in BSP irq mux code here:
>> https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/drivers/ir
>> qchip/irq-rtd16xx.h
>>
>> That does show UR0 as bit 2 for the iso irq mux, as for previous SoCs.
>> Is that code wrong, or does the same UART0 IP block have two alternative
>> interrupts for backwards compatibility? I therefore held back RTD1619 irq mux
>> patches from my irqchip v4 series [1].
>>
> It is code wrong. The UR0 should remove from "irq-rtd16xx.h".

Actually, I just tested that UR0 works! (rev A01) So we shouldn't remove
it from the irqchip driver, given the mapping changes requested for v5.

RTD1619 driver support and DT nodes pushed to my rtd1295-next branch.

>> The BSP DT does assign non-mux interrupts to the UART node like you did:
>> https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm
>> 64/boot/dts/realtek/rtd16xx/rtd-16xx.dtsi
>> And I obviously trust that you tested your DT to produce serial output.

We should obviously leave the new GIC interrupts in the DT.

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Marc Zyngier]

On 2019-11-17 15:39, James Tai wrote:
> Hi Andreas,
>
>> > Sorry for my misunderstanding. The RAM region don't require two 
>> cells
>> > for memory nodes, so I'll fix it in v3 patch.
>>
>> Should I then also change RTD1395 to use only one cell, or does it 
>> support
>> more RAM than RTD1619?
>
> Yes, you can. The memory capacity of RTD1395 and RTD1619 are the 
> same.
>
>> By my calculation 0x98000000 is less than 2.4 GiB! So, does RAM 
>> continue
>> between r-bus and GIC, similar to how it does on RTD1195? Then we 
>> need to
>> exclude those RAM ranges from the SoC node (adjusting 0x68000000).
>
> We need to reserve memory address for r-bus and GIC and exclude those
> RAM range from the SoC node.

Memory for the GIC? For what purpose?

         M.
-- 
Who you jivin' with that Cosmik Debris?

Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[Andreas Färber]

Am 22.11.19 um 10:43 schrieb Marc Zyngier:
> On 2019-11-17 15:39, James Tai wrote:
>> Hi Andreas,
>>
>>> > Sorry for my misunderstanding. The RAM region don't require two cells
>>> > for memory nodes, so I'll fix it in v3 patch.
>>>
>>> Should I then also change RTD1395 to use only one cell, or does it
>>> support
>>> more RAM than RTD1619?
>>
>> Yes, you can. The memory capacity of RTD1395 and RTD1619 are the same.
>>
>>> By my calculation 0x98000000 is less than 2.4 GiB! So, does RAM continue
>>> between r-bus and GIC, similar to how it does on RTD1195? Then we
>>> need to
>>> exclude those RAM ranges from the SoC node (adjusting 0x68000000).
>>
>> We need to reserve memory address for r-bus and GIC and exclude those
>> RAM range from the SoC node.
> 
> Memory for the GIC? For what purpose?

MMIO, for GICD and GICR. It's about fixing the ranges of the /soc node:

My proposed
ranges = <0x98000000 0x98000000 0x68000000>;
needs to be split, with a gap between r-bus and GIC for continued RAM.

https://github.com/afaerber/linux/commit/1884ec6a533c9d5c2b6ca40ee138ff7e8312b6c8

This goes back to Rob's review of RTD1295 [1], where we then for lack of
memory space documentation assumed that everything beyond 2 GiB would be
potential register space. Here we're dealing with up to 4 GiB though.


James, are you planning to send a fix-up patch here? If not, you'll need
to tell me what values to use, e.g., is there a NOR flash region on
RTD1619, and does RAM continue also in between and after GIC, or is
there some timer register behind it, like on RTD1195?

ranges = <0x00000000 0x00000000 0x00030000>, // ??? boot ROM size
         <0x98000000 0x98000000 0x00200000>, // r-bus
         // anything here? e.g., NOR flash?
         <0xff100000 0xff100000 0x00010000>, // GICD
         <0xff140000 0xff140000 0x000c0000>; // GICR
         // anything here? e.g., timer enable?

ranges = <0x00000000 0x00000000 0x00030000>,
         <0x98000000 0x98000000 0x00200000>,
         <0xff100000 0xff100000 0x00100000>; // whole GIC?

Regards,
Andreas

[1] https://patchwork.kernel.org/patch/9588611/

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

Hi Andreas,

> 
> MMIO, for GICD and GICR. It's about fixing the ranges of the /soc node:
> 
> My proposed
> ranges = <0x98000000 0x98000000 0x68000000>; needs to be split, with a gap
> between r-bus and GIC for continued RAM.
> 
> https://github.com/afaerber/linux/commit/1884ec6a533c9d5c2b6ca40ee138ff
> 7e8312b6c8
> 
> This goes back to Rob's review of RTD1295 [1], where we then for lack of
> memory space documentation assumed that everything beyond 2 GiB would
> be potential register space. Here we're dealing with up to 4 GiB though.
> 
> 
> James, are you planning to send a fix-up patch here? If not, you'll need to tell
> me what values to use, e.g., is there a NOR flash region on RTD1619, and does
> RAM continue also in between and after GIC, or is there some timer register
> behind it, like on RTD1195?
> 
> ranges = <0x00000000 0x00000000 0x00030000>, // ??? boot ROM size
>          <0x98000000 0x98000000 0x00200000>, // r-bus
>          // anything here? e.g., NOR flash?
>          <0xff100000 0xff100000 0x00010000>, // GICD
>          <0xff140000 0xff140000 0x000c0000>; // GICR
>          // anything here? e.g., timer enable?
> 
> ranges = <0x00000000 0x00000000 0x00030000>,
>          <0x98000000 0x98000000 0x00200000>,
>          <0xff100000 0xff100000 0x00100000>; // whole GIC?
> 

Yes, I'll send a fix-up patch.


Regards,
James

RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir

[James Tai]

Hi Andreas,

> >> back RTD1619 irq mux patches from my irqchip v4 series [1].
> >>
> > It is code wrong. The UR0 should remove from "irq-rtd16xx.h".
> 
> Actually, I just tested that UR0 works! (rev A01) So we shouldn't remove it from
> the irqchip driver, given the mapping changes requested for v5.

The UR0 has been removed from the RTD1619 specification. 
I'll check it again.


Regards,
James