Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

On Thu, Nov 27, 2014 at 07:35:13AM +0000, Chanwoo Choi wrote:
> 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 <kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
> Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Geunsik Lim <geunsik.lim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  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

[...]

> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> new file mode 100644
> index 0000000..3d8b576
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> @@ -0,0 +1,523 @@
> +/*
> + * Samsung's Exynos5433 SoC device tree source
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + *             http://www.samsung.com
> + *
> + * Samsung's Exynos5433 SoC device nodes are listed in this file. Exynos5433
> + * based board files can include this file and provide values for board specfic
> + * bindings.
> + *
> + * Note: This file does not include device nodes for all the controllers in
> + * Exynos5433 SoC. As device tree coverage for Exynos5433 increases, additional
> + * nodes can be added to this file.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "skeleton.dtsi"
> +#include <dt-bindings/clock/exynos5433.h>
> +

Just to check: no memory reservations required for any reason?

There also don't appear to be any memory nodes. Typically if that's
filled in by the bootloader/FW we'd have an empty node (or one with a
zero size entry) and a comment regarding the FW.

> +/ {
> +       compatible = "samsung,exynos5433";
> +       #address-cells = <1>;
> +       #size-cells = <1>;

Not two, on both counts? The CPUs can address more than 32 bits.

Is there nothing in the physical address space above 0xffffffff?

[...]

> +       cpus {
> +               #address-cells = <2>;
> +               #size-cells = <0>;
> +
> +               cpu0: cpu@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?

I take it from the presence of GICH/GICV in the gic node that CPUs enter
the kernel at EL2?

> +                       reg = <0x0 0x100>;
> +                       clock-frequency = <1050000000>;

What uses this?

> +               };

[...]

> +       soc: soc {
> +               compatible = "simple-bus";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               fixed-rate-clocks {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       xusbxti: clock@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.

> +
> +               cmu_top: clock-controller@0x10030000{

s/@0x/@/ -- a unit-address should not have the leading '0x'. Please
apply that to the rest of the file.

> +                       compatible = "samsung,exynos5433-cmu-top";
> +                       reg = <0x10030000 0x0c04>;
> +                       #clock-cells = <1>;
> +               };

[...]

> +               mct@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?

> +
> +               gic:interrupt-controller@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";

> +                       #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.

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

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

[...]

> +               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.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html