Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir
@ 2019-11-05  8:00 James Tai
  2019-11-06  8:28 ` Andreas Färber
  0 siblings, 1 reply; 6+ messages in thread
From: James Tai @ 2019-11-05  8:00 UTC (permalink / raw)
  To: linux-realtek-soc
  Cc: Mark Rutland, Rob Herring, \'linux-kernel, linux-arm-kernel,
	Andreas Färber

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


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir
  2019-11-05  8:00 [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir James Tai
@ 2019-11-06  8:28 ` Andreas Färber
  2019-11-08 15:36   ` James Tai
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Färber @ 2019-11-06  8:28 UTC (permalink / raw)
  To: James Tai
  Cc: Mark Rutland, Lorenzo Pieralisi, linux-realtek-soc, Marc Zyngier,
	linux-kernel, Rob Herring, linux-arm-kernel

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)

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir
  2019-11-06  8:28 ` Andreas Färber
@ 2019-11-08 15:36   ` James Tai
  2019-11-08 17:17     ` Andreas Färber
  0 siblings, 1 reply; 6+ messages in thread
From: James Tai @ 2019-11-08 15:36 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Mark Rutland, Lorenzo Pieralisi, linux-realtek-soc, Marc Zyngier,
	linux-kernel, Rob Herring, linux-arm-kernel

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


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir
  2019-11-08 15:36   ` James Tai
@ 2019-11-08 17:17     ` Andreas Färber
  2019-11-11  2:58       ` James Tai
  2019-11-11  3:09       ` Andreas Färber
  0 siblings, 2 replies; 6+ messages in thread
From: Andreas Färber @ 2019-11-08 17:17 UTC (permalink / raw)
  To: James Tai
  Cc: Mark Rutland, Rob Herring, linux-kernel, linux-arm-kernel,
	linux-realtek-soc

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)

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir
  2019-11-08 17:17     ` Andreas Färber
@ 2019-11-11  2:58       ` James Tai
  2019-11-11  3:09       ` Andreas Färber
  1 sibling, 0 replies; 6+ messages in thread
From: James Tai @ 2019-11-11  2:58 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Mark Rutland, Rob Herring, linux-kernel, linux-arm-kernel,
	linux-realtek-soc

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


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir
  2019-11-08 17:17     ` Andreas Färber
  2019-11-11  2:58       ` James Tai
@ 2019-11-11  3:09       ` Andreas Färber
  1 sibling, 0 replies; 6+ messages in thread
From: Andreas Färber @ 2019-11-11  3:09 UTC (permalink / raw)
  To: James Tai
  Cc: Mark Rutland, Rob Herring, linux-kernel, linux-arm-kernel,
	linux-realtek-soc

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)

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05  8:00 [PATCH] arm64: dts: realtek: Add Realtek rtd1619 and mjolnir James Tai
2019-11-06  8:28 ` Andreas Färber
2019-11-08 15:36   ` James Tai
2019-11-08 17:17     ` Andreas Färber
2019-11-11  2:58       ` James Tai
2019-11-11  3:09       ` Andreas Färber

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git