Re: [PATCH 11/14] arm64: dts: Add initial device tree support for EXYNOS7

[Olof Johansson <olof@lixom.net>]

Hi,

On Wed, Aug 27, 2014 at 03:14:18PM +0530, Naveen Krishna Chatradhi wrote:
> Add initial device tree nodes for EXYNOS7 SoC.
> Also, includes the dt-binding definitions for clock ids.

Uh, no -- it just adds the dtsi.

> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Cc: Thomas Abraham <thomas.ab@samsung.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/boot/dts/exynos7.dtsi |  553 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 553 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/exynos7.dtsi
> 
> diff --git a/arch/arm64/boot/dts/exynos7.dtsi b/arch/arm64/boot/dts/exynos7.dtsi
> new file mode 100644
> index 0000000..6b9eaf4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos7.dtsi

Let's not make the same mistake as on 32-bit, and go with a directory
hierarchy here from day one.

So, please create a exynos subdirectory for this file. You also need
a Makefile when you add a board dts.

> @@ -0,0 +1,553 @@
> +/*
> + * SAMSUNG EXYNOS7 SoC device tree source
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * SAMSUNG EXYNOS7 SoC device nodes are listed in this file.
> + * EXYNOS7 based board files can include this file and provide
> + * values for board specfic bindings.
> + *
> + * 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 <dt-bindings/clock/exynos7-clk.h>
> +
> +/ {
> +	compatible = "samsung,exynos7";
> +	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;

You should probably use address-cells/size-cells 2/2 on a 64-bit platform.

> +	aliases {
> +		pinctrl0 = &pinctrl_0;
> +		pinctrl1 = &pinctrl_1;
> +		pinctrl2 = &pinctrl_2;
> +		pinctrl3 = &pinctrl_3;
> +		pinctrl4 = &pinctrl_4;
> +		pinctrl5 = &pinctrl_5;
> +		pinctrl6 = &pinctrl_6;
> +		pinctrl7 = &pinctrl_7;
> +		pinctrl8 = &pinctrl_8;
> +		pinctrl9 = &pinctrl_9;
> +		mshc0 = &mmc_0;
> +		mshc2 = &mmc_2;
> +	};
> +
> +	chipid@10000000 {
> +		compatible = "samsung,exynos4210-chipid";
> +		reg = <0x10000000 0x100>;
> +	};
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;

Why size-cells=2? Can you not fit a cpuid in 32 bits?

> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a57", "arm,armv8";
> +			reg = <0x0 0x0>;
> +		};
> +	};
> +
> +	fin_pll: xxti {
> +		compatible = "fixed-clock";
> +		clock-frequency = <24000000>;
> +		clock-output-names = "fin_pll";
> +		#clock-cells = <0>;
> +	};
> +
> +	gic: interrupt-controller@11001000 {
> +		compatible = "arm,gic-400";
> +		#interrupt-cells = <3>;
> +		#address-cells = <0>;
> +		interrupt-controller;
> +		reg =	<0x11001000 0x1000>,
> +			<0x11002000 0x1000>,
> +			<0x11004000 0x2000>,
> +			<0x11006000 0x2000>;
> +	};
> +
> +	hsi2c_0: hsi2c@13640000 {
> +		compatible = "samsung,exynos7-hsi2c";

Is the i2c controller here completely new?

Also, please use 'i2c' for node name on these nodes.

> +		reg = <0x13640000 0x1000>;
> +		interrupts = <0 441 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&hs_i2c0_bus>;
> +		clocks = <&clock_peric0 PCLK_HSI2C0>;
> +		clock-names = "hsi2c";
> +		status = "disabled";
> +	};
> +
> +	hsi2c_1: hsi2c@13650000 {
> +		compatible = "samsung,exynos7-hsi2c";
> +		reg = <0x13650000 0x1000>;
> +		interrupts = <0 442 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&hs_i2c1_bus>;
> +		clocks = <&clock_peric0 PCLK_HSI2C1>;
> +		clock-names = "hsi2c";
> +		status = "disabled";
> +	};
> +
> +	hsi2c_2: hsi2c@14E60000 {

I much prefer lowercase hex in unit addresses (and reg entries) below. I
know 32-bit uses uppercase, but let's switch going forward here.

> +	mct@101C0000 {
> +		compatible = "samsung,exynos4210-mct";

Please just do away with MCT here, and use architected timers going
forward. There really shouldn't be a need to keep supporting MCT any
more -- it's a construct from before arch timers on Cortex-A9.

> +	mmc_0: mmc@15740000 {
> +		compatible = "samsung,exynos7-dw-mshc-smu";

Is this controller backwards compatible with exynos5 ones?

> +	/* The Clock nodes are ordered as per the usermanual. */

"The clock"

"user manual"

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

These two properties are not standard, and I would expect any 64-bit
platform to come with PSCI such that you have a way to initialize the
virtual timers.


-Olof