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

[Naveen Krishna Ch <naveenkrishna.ch@gmail.com>]

Hi Olof,

On 28 August 2014 09:26, Olof Johansson <olof@lixom.net> wrote:
> 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.

Ok. Will fix.

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

Ok. Will fix.

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

Ok.

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

It is almost the same as in Exynos5 but few register bits have been
changed. So we have added a new compatible string.

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

Ok.

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

Ok. Will fix.

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

Ok.

>
>> +     mmc_0: mmc@15740000 {
>> +             compatible = "samsung,exynos7-dw-mshc-smu";
>
> Is this controller backwards compatible with exynos5 ones?

The dwmmc controller in Exynos7 is not fully backward compatible with
Exynos5. Specifically, it requires 64-bit related changes for IDMAC
and handling the changes in register offsets.

>
>> +     /* The Clock nodes are ordered as per the usermanual. */
>
> "The clock"
>
> "user manual"

Ok.

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

Ok.

>
>
> -Olof


Thanks for your comments.

-- 
Shine bright,
(: Nav :)