From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753500AbcHZRte (ORCPT ); Fri, 26 Aug 2016 13:49:34 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:36037 "EHLO s-opensource.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742AbcHZRtc (ORCPT ); Fri, 26 Aug 2016 13:49:32 -0400 Subject: Re: [PATCH v2 5/7] arm64: dts: exynos: Add dts files for Samsung Exynos5433 64bit SoC To: Chanwoo Choi , k.kozlowski@samsung.com, kgene@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, catalin.marinas@arm.com, will.deacon@arm.com, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org References: <1472046551-703-1-git-send-email-cw00.choi@samsung.com> <1472046551-703-6-git-send-email-cw00.choi@samsung.com> From: Javier Martinez Canillas Cc: krzk@kernel.org, jh80.chung@samsung.com, sw0312.kim@samsung.com, jy0922.shim@samsung.com, inki.dae@samsung.com, jonghwa3.lee@samsung.com, beomho.seo@samsung.com, jaewon02.kim@samsung.com, human.hwang@samsung.com, ideal.song@samsung.com, ingi2.kim@samsung.com, m.szyprowski@samsung.com, a.hajda@samsung.com, s.nawrocki@samsung.com, chanwoo@kernel.org Message-ID: <4341f429-64a9-b14c-aeca-d61aac2ee81d@osg.samsung.com> Date: Fri, 26 Aug 2016 13:49:21 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1472046551-703-6-git-send-email-cw00.choi@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Chanwoo, The patch looks good to me, I just have a few minor comments: On 08/24/2016 09:49 AM, Chanwoo Choi wrote: [snip] > + > + reboot: syscon-reboot { > + compatible = "syscon-reboot"; > + regmap = <&pmu_system_controller>; > + offset = <0x400>; Maybe adding a comment that the register is SWRESET to be consistent with the arm32 Exynos node definition in arch/arm/boot/dts/exynos-syscon-restart.dtsi? [snip] > + > + tmu_atlas0: tmu@10060000 { > + compatible = "samsung,exynos5433-tmu"; > + reg = <0x10060000 0x200>; > + interrupts = <0 95 0>; For some interrupts definitions, you are using the GIC interrupt type macros while for others like this you are using the constant numbers. I think would be better to use the macros consistently (i.e: interrupts = ). [snip] > + mipi_phy: video-phy@105C0708 { > + compatible = "samsung,exynos5433-mipi-video-phy"; > + #phy-cells = <1>; > + samsung,pmu-syscon = <&pmu_system_controller>; > + samsung,cam0-sysreg = <&syscon_cam0>; > + samsung,cam1-sysreg = <&syscon_cam1>; > + samsung,disp-sysreg = <&syscon_disp>; > + }; This node has a unit name but it doesn't have a reg property so dtc will warn about the mismatch when executed with W=1. [snip] > + usbdrd30: usb@15400000 { > + compatible = "samsung,exynos5250-dwusb3"; > + clocks = <&cmu_fsys CLK_ACLK_USBDRD30>, > + <&cmu_fsys CLK_SCLK_USBDRD30>; > + clock-names = "usbdrd30", "usbdrd30_susp_clk"; > + assigned-clocks = > + <&cmu_fsys CLK_MOUT_SCLK_USBDRD30_USER>, > + <&cmu_top CLK_MOUT_SCLK_USBDRD30>, > + <&cmu_top CLK_DIV_SCLK_USBDRD30>; > + assigned-clock-parents = > + <&cmu_top CLK_SCLK_USBDRD30_FSYS>, > + <&cmu_top CLK_MOUT_BUS_PLL_USER>; > + assigned-clock-rates = <0>, <0>, <66700000>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + status = "disabled"; > + Ditto, the node has unit address but no reg property. > + dwc3 { > + compatible = "snps,dwc3"; > + reg = <0x15400000 0x10000>; Ditto, the node doesn't have a unit address but has reg property. > + interrupts = <0 231 0>; > + phys = <&usbdrd30_phy 0>, <&usbdrd30_phy 1>; > + phy-names = "usb2-phy", "usb3-phy"; > + }; > + }; > + > + > + usbhost30: usb@15a00000 { > + compatible = "samsung,exynos5250-dwusb3"; > + clocks = <&cmu_fsys CLK_ACLK_USBHOST30>, > + <&cmu_fsys CLK_SCLK_USBHOST30>; > + clock-names = "usbdrd30", "usbdrd30_susp_clk"; > + assigned-clocks = > + <&cmu_fsys CLK_MOUT_SCLK_USBHOST30_USER>, > + <&cmu_top CLK_MOUT_SCLK_USBHOST30>, > + <&cmu_top CLK_DIV_SCLK_USBHOST30>; > + assigned-clock-parents = > + <&cmu_top CLK_SCLK_USBHOST30_FSYS>, > + <&cmu_top CLK_MOUT_BUS_PLL_USER>; > + assigned-clock-rates = <0>, <0>, <66700000>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + status = "disabled"; > + Ditto, the node has unit address but no reg property. > + usbdrd_dwc3_0: dwc3 { > + compatible = "snps,dwc3"; > + reg = <0x15a00000 0x10000>; Ditto, the node doesn't have a unit address but has reg property. > + interrupts = <0 244 0>; > + phys = <&usbhost30_phy 0>, <&usbhost30_phy 1>; > + phy-names = "usb2-phy", "usb3-phy"; > + }; > + }; > + [snip] > + audio-subsystem { > + compatible = "samsung,exynos5433-lpass"; > + reg = <0x11400000 0x100>, <0x11500000 0x08>; Ditto, the node doesn't have a unit address but has reg property. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America