On Fri, Nov 28, 2014 at 01:18:25PM +0000, Chanwoo Choi wrote:

Dear Mark,

On 11/27/2014 08:18 PM, Mark Rutland wrote:

quoted

On Thu, Nov 27, 2014 at 07:35:13AM +0000, Chanwoo Choi wrote:

quoted

This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).

Cc: Kukjin Kim <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Geunsik Lim <redacted>
---
 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 +++++++++++++++++++++
 arch/arm64/boot/dts/exynos/exynos5433.dtsi         | 523 +++++++++++++++
 2 files changed, 1221 insertions(+)
 create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
 create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi

[...]

quoted

quoted

+       cpus {
+               #address-cells = <2>;
+               #size-cells = <0>;
+
+               cpu0: cpu at 100 {
+                       device_type = "cpu";
+                       compatible = "arm,cortex-a53", "arm,armv8";
+                       enable-method = "psci";

While the CPU nodes have enable-methods, I didn't spot a PSCI node
anywhere, so this dts cannot possibly have been used to bring up an SMP
system.

How has this dts been tested?

What PSCI revision have you implemented? Have have you tested it?

My mistake,
Exynos5433 supports PSCI v0.1. I'll add following PSCI nodes:
I tested the boot of secondary cpu.

        psci {
                compatible = "arm,psci";
                method = "smc";
                cpu_off = <0x84000002>;
                cpu_on = <0xC4000003>;
        };

Ok. I take it _any_ CPU may be hotplugged (including CPU0), given that
you don't have MIGRATE_INFO_TYPE from PSCI 0.2 to tell you that this is
not possible? If not, attempting to hotplug CPU0 will result in a BUG()
and the kernel will explode.

Has that been tested? 

Do all CPUs enter the kernel at EL2?

quoted

quoted

+       soc: soc {
+               compatible = "simple-bus";
+               #address-cells = <1>;
+               #size-cells = <1>;
+               ranges;
+
+               fixed-rate-clocks {
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+
+                       xusbxti: clock at 0 {
+                               compatible = "fixed-clock";
+                               clock-output-names = "xusbxti";
+                               #clock-cells = <0>;
+                       };
+               };

Get rid of the fixed-rate-clocks container node. It's pointless and
messy. Given you only have one there's no need for the bogus
unit-address either.

OK, I'll remove unneeded code and will add following dt node for fin_pll.

        fin_pll: xxti {
                compatible = "fixed-clock";
                clock-output-names = "fin_pll";
                #clock-cells = <0>;
        };

That looks fine to me.

[...]

quoted

quoted

+               mct at 101c0000 {
+                       compatible = "samsung,exynos4210-mct";
+                       reg = <0x101c0000 0x800>;
+                       interrupts = <0 102 0>, <0 103 0>, <0 104 0>, <0 105 0>,
+                               <0 106 0>, <0 107 0>, <0 108 0>, <0 109>,
+                               <0 110 0>, <0 111 0>, <0 112 0>, <0 113 0>;
+                       clocks = <&cmu_top CLK_FIN_PLL>, <&cmu_peris CLK_PCLK_MCT>;
+                       clock-names = "fin_pll", "mct";
+               };

Hase this block had no changes whatsoever since its use in Exynos4210?
Do we not need a "samsung,exynos5433-mct" comaptible string too?

The type of Exynos5433's MCT(Multi-Core Timer) IP is the same with the type of Exynos4210 MCT.
Just Exynos5433 have eight local timer for Octa cores.

So "samsung,exynos4210-mct" should appear in the list. I'm just
wondering if it's worth having:

	compatible = "samsung,exynos5433-mct", "samsung,exynos4210-mct";

Just in case we need to special-case the 5433 MCT for some reason later.

Why not make both of these depend on CONFIG_ARM, rather than
!CONFIG_ARM64? We care about the presence of the delay_timer struct and
functions, which (from grepping around) exist in arch/arm and nowhere
else.

quoted

quoted

+               gic:interrupt-controller at 11001000 {
+                       compatible = "arm,cortex-a15-gic";

Given this is multi-cluster, surely this is an external GIC-400, for
which we have a supported compatible string?

So this should at least be:

      compatible = "arm,gic-400", "arm,cortex-a15-gic";

Exynos5433 used GIC-400. I'll modify it as following:

        compatible = "arm,gic-400";

Ok. The former variant (with "arm,cortex-a15-gic" later in the list) has
the benefit of working with KVM today (which doesn't currently look for
"arm,gic-400").

quoted

quoted

+                       #interrupt-cells = <3>;
+                       interrupt-controller;
+                       reg =   <0x11001000 0x1000>,
+                               <0x11002000 0x1000>,
+                               <0x11004000 0x2000>,
+                               <0x11006000 0x2000>;

As far as I am aware, the GICC size is 8KiB. Regardless of whether we
currently use the second page of registers, they should be described.

The GICC (CPU Interface Register) register of Exynos5433 is range of 0x1100_2000 ~ 0x1100_2100.

That does not sound right. Per the GICv2 architecture, GICC is at least
0x1004 bytes long (as GICC_DIR lives at offset 0x1000).

But, I'll modify GICC size from 4KiB to 8KiB as following according to your comment:
        <0x11002000 0x1000> -> <0x11002000 0x2000>

To clarify: is GICC_DIR accessible in Exynos5433 systems, or is
everything past offset 0x100 not physically mapped?

quoted

quoted

+                       interrupts = <1 9 0xf04>;
+               };
+
+               serial_0: serial at 14C10000 {

Nit: Please be consistent with capitalisation of hex. IMO it's better
to leave it all lower-case.

I'll use the lower-case for all base address.

Thanks.

quoted

[...]

quoted

+               timer {
+                       compatible = "arm,armv8-timer";
+                       interrupts = <1 13 0xff01>,
+                                    <1 14 0xff01>,
+                                    <1 11 0xff01>,
+                                    <1 10 0xff01>;
+                       clock-frequency = <24000000>;
+                       use-clocksource-only;
+                       use-physical-timer;

As Marc said, NAK for these last three properties.

There is no excuse for not setting CNTFRQ_EL0, especially given a PSCI
implementation. The last two properties have never been supported in
mainline, and shouldn't be necessary regardless.

OK, I'll remove last three properties.

Thanks.

Mark.