From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH 11/14] arm64: dts: Add initial device tree support for EXYNOS7 Date: Wed, 27 Aug 2014 11:42:31 +0100 Message-ID: <20140827104231.GB7773@leverpostej> References: <1409132660-1898-1-git-send-email-ch.naveen@samsung.com> <1409132660-1898-3-git-send-email-ch.naveen@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1409132660-1898-3-git-send-email-ch.naveen@samsung.com> Sender: linux-samsung-soc-owner@vger.kernel.org To: Naveen Krishna Chatradhi Cc: Catalin Marinas , "naveenkrishna.ch@gmail.com" , "linux-arm-kernel@lists.infradead.org" , "linux-samsung-soc@vger.kernel.org" , "devicetree@vger.kernel.org" , "cpgs@samsung.com" , Thomas Abraham , Rob Herring List-Id: devicetree@vger.kernel.org Hi Naveen, On Wed, Aug 27, 2014 at 10:44:18AM +0100, Naveen Krishna Chatradhi wrote: > Add initial device tree nodes for EXYNOS7 SoC. > Also, includes the dt-binding definitions for clock ids. Fallout from a rebase? That latter part doesn't seem to be relevant. > Signed-off-by: Naveen Krishna Chatradhi > Cc: Thomas Abraham > Cc: Rob Herring > Cc: Catalin Marinas > --- > 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 > @@ -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 > + > +/ { > + compatible = "samsung,exynos7"; > + interrupt-parent = <&gic>; > + #address-cells = <1>; > + #size-cells = <1>; Can we guarantee everything going to live within 0x0 - 0xffffffff for all boards using the SoC? I suspect that we can't, so the addresses and sizes at the top level should be two cells. At some point there is bound to be something above 4GB that we'll need to map, so to save us from a painful dts refactoring we should have the dts organised to support that from the outside. [...] > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu@0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a57", "arm,armv8"; > + reg = <0x0 0x0>; > + }; > + }; Only UP? [...] > + 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>; > + }; Nice to see GICV and GICH. [...] > + mct@101C0000 { > + compatible = "samsung,exynos4210-mct"; > + reg = <0x101C0000 0x800>; > + interrupt-controller; > + #interrupt-cells = <1>; > + interrupt-parent = <&mct_map>; > + interrupts = <0>, <1>, <2>, <3>, > + <4>, <5>, <6>, <7>, > + <8>, <9>, <10>, <11>; > + clocks = <&fin_pll>, <&clock_peris PCLK_MCT>; > + clock-names = "fin_pll", "mct"; > + > + mct_map: mct-map { > + #interrupt-cells = <1>; > + #address-cells = <0>; > + #size-cells = <0>; > + interrupt-map = <0 &gic 0 112 0>, > + <1 &gic 0 113 0>, > + <2 &gic 0 114 0>, > + <3 &gic 0 115 0>, > + <4 &gic 0 116 0>, > + <5 &gic 0 117 0>, > + <6 &gic 0 118 0>, > + <7 &gic 0 119 0>, > + <8 &gic 0 120 0>, > + <9 &gic 0 121 0>, > + <10 &gic 0 122 0>, > + <11 &gic 0 123 0>; > + }; > + }; I don't see why need the map here. Why can't we describe the GIC interrupts directly? [...] > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <1 13 0xff01>, > + <1 14 0xff01>, > + <1 11 0xff01>, > + <1 10 0xff01>; > + clock-frequency = <24000000>; Your firmware/bootloader should configure CNTFRQ, and this shouldn't be necessary. The clock-frequency property is an incomplete workaround for broken firmware that in an ideal world we could kill off. > + use-clocksource-only; > + use-physical-timer; Neither of these properties were introduced by this series, and no rationale was given. What are these properties for, and why do you believe they are necessary? Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Wed, 27 Aug 2014 11:42:31 +0100 Subject: [PATCH 11/14] arm64: dts: Add initial device tree support for EXYNOS7 In-Reply-To: <1409132660-1898-3-git-send-email-ch.naveen@samsung.com> References: <1409132660-1898-1-git-send-email-ch.naveen@samsung.com> <1409132660-1898-3-git-send-email-ch.naveen@samsung.com> Message-ID: <20140827104231.GB7773@leverpostej> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Naveen, On Wed, Aug 27, 2014 at 10:44:18AM +0100, Naveen Krishna Chatradhi wrote: > Add initial device tree nodes for EXYNOS7 SoC. > Also, includes the dt-binding definitions for clock ids. Fallout from a rebase? That latter part doesn't seem to be relevant. > Signed-off-by: Naveen Krishna Chatradhi > Cc: Thomas Abraham > Cc: Rob Herring > Cc: Catalin Marinas > --- > 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 > @@ -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 > + > +/ { > + compatible = "samsung,exynos7"; > + interrupt-parent = <&gic>; > + #address-cells = <1>; > + #size-cells = <1>; Can we guarantee everything going to live within 0x0 - 0xffffffff for all boards using the SoC? I suspect that we can't, so the addresses and sizes at the top level should be two cells. At some point there is bound to be something above 4GB that we'll need to map, so to save us from a painful dts refactoring we should have the dts organised to support that from the outside. [...] > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu at 0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a57", "arm,armv8"; > + reg = <0x0 0x0>; > + }; > + }; Only UP? [...] > + gic: interrupt-controller at 11001000 { > + compatible = "arm,gic-400"; > + #interrupt-cells = <3>; > + #address-cells = <0>; > + interrupt-controller; > + reg = <0x11001000 0x1000>, > + <0x11002000 0x1000>, > + <0x11004000 0x2000>, > + <0x11006000 0x2000>; > + }; Nice to see GICV and GICH. [...] > + mct at 101C0000 { > + compatible = "samsung,exynos4210-mct"; > + reg = <0x101C0000 0x800>; > + interrupt-controller; > + #interrupt-cells = <1>; > + interrupt-parent = <&mct_map>; > + interrupts = <0>, <1>, <2>, <3>, > + <4>, <5>, <6>, <7>, > + <8>, <9>, <10>, <11>; > + clocks = <&fin_pll>, <&clock_peris PCLK_MCT>; > + clock-names = "fin_pll", "mct"; > + > + mct_map: mct-map { > + #interrupt-cells = <1>; > + #address-cells = <0>; > + #size-cells = <0>; > + interrupt-map = <0 &gic 0 112 0>, > + <1 &gic 0 113 0>, > + <2 &gic 0 114 0>, > + <3 &gic 0 115 0>, > + <4 &gic 0 116 0>, > + <5 &gic 0 117 0>, > + <6 &gic 0 118 0>, > + <7 &gic 0 119 0>, > + <8 &gic 0 120 0>, > + <9 &gic 0 121 0>, > + <10 &gic 0 122 0>, > + <11 &gic 0 123 0>; > + }; > + }; I don't see why need the map here. Why can't we describe the GIC interrupts directly? [...] > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <1 13 0xff01>, > + <1 14 0xff01>, > + <1 11 0xff01>, > + <1 10 0xff01>; > + clock-frequency = <24000000>; Your firmware/bootloader should configure CNTFRQ, and this shouldn't be necessary. The clock-frequency property is an incomplete workaround for broken firmware that in an ideal world we could kill off. > + use-clocksource-only; > + use-physical-timer; Neither of these properties were introduced by this series, and no rationale was given. What are these properties for, and why do you believe they are necessary? Thanks, Mark.