From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753487AbcIBK7h convert rfc822-to-8bit (ORCPT ); Fri, 2 Sep 2016 06:59:37 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:52837 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752962AbcIBK7Y (ORCPT ); Fri, 2 Sep 2016 06:59:24 -0400 X-AuditID: cbfee68d-f79286d000007a9a-21-57c95b893b45 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: 8BIT Message-id: <57C95B88.80909@samsung.com> Date: Fri, 02 Sep 2016 19:59:20 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Javier Martinez Canillas , 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 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 Subject: Re: [PATCH v2 5/7] arm64: dts: exynos: Add dts files for Samsung Exynos5433 64bit SoC References: <1472046551-703-1-git-send-email-cw00.choi@samsung.com> <1472046551-703-6-git-send-email-cw00.choi@samsung.com> <4341f429-64a9-b14c-aeca-d61aac2ee81d@osg.samsung.com> In-reply-to: <4341f429-64a9-b14c-aeca-d61aac2ee81d@osg.samsung.com> X-Brightmail-Tracker: H4sIAAAAAAAAA02Se0iTURjGOd9tn5b05aWOAwslUwot83ZCC6t/FgSVgpdCdNmHk3Tq5rWE BpE6TU0HJWJeyomseWGJVyqby5xiKnYxxWmZteWlMk0ztOa+Av97OM/ze9/zwEvjtiqST8eJ U1iJWBjvQlkTanufZA/5RX3Y4Y31ADTW8JJE/YstPPS19hZAxaOvCFSpM78pSwtJ1LE+CVDb uyUKlUzeJlCbTEeiuXk1hkbXskkkH/iII6NhmECzRi9UND2Lo8HBJh7STL8h0UhHOYVKB59g qF43wUPKt8MYuvlYx0PdczkkKlWYKGT63ksE8QXqCjUQaFRySvCo5rqguWiCJyhsVgHBD82e c9QF68DLbHxcGis5dDzaWrSh1FJJJveMkd8nZWB+bx6woiHjAxWVMwSnd8EhQyOVB6xpW6YO QL26F/wPZTf/wThDCeDYnNFi2DA74arCYKFxxg2WVNzHOW0P89vrSU4fhLXVszgHTwGoaB36 B7vDByurFoBgXGHf3TELQDEH4FPjKLWpdzDO8PXqtDlP0w5MOMzvzdycY890Y3CpZ9EyFGdM GHxo6ONtAnZMNFxcaCe4bXkYfF/aZDGsmFNQu/IN4/qs0lCfe5rbzMCfCi2xuQEyTlDThXMR R/isbpS4DWDZlqJlW4qWbSlatqVoFSBUwIFNikmSXoqVeHlKhQnSVHGsZ0xiggaYT6h/41NB GxjvCtAChgYu223CI/RhtqQwTZqZoAW+5h8V43yHmETz1YlTory8/Y4gXx9f7yP+R/1cdts4 83+F2DKxwhT2CssmsZIoSWo8K9UCjLbiy4C/6+cPgb73zuvcs1qi0qra5RVXEyNj7HpKak3n OluyunOF6S37T3guTWXxQls9voSMTx5bWPfQ6u+8rt5mAAaUsiYSN1R4qMQ3IhKq+hudFPNy cfCjKRGVEZSvDNy3LMy4Nv6iTjZQ83zvcsGMm2NwYXK6oDxH5hjZGYqvnDnrQkhFQq8DuEQq /AucHOLaPQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrPKsWRmVeSWpSXmKPExsVy+t9jAd3O6JPhBh+viFncWneO1eL0p23s Fu+X9TBaTLxxhcVi/hGg2NIZfawWu/7eZ7TYcfMLm8Wk+xNYLHY0HGG1ePN2DZPFjV9trBad Z58wW7y4d5HF4vULQ4v+x6+ZLc6f38BusenxNVaLy7vmsFnMOL+PyWLtkbvsFkuvX2SyaN17 hN3i8Jt2VosZk1+yWbz8eILFQcpjzbw1jB6bVnWyeWxeUu+xpf8uu0ffllWMHp83yQWwRTUw 2mSkJqakFimk5iXnp2TmpdsqeQfHO8ebmhkY6hpaWpgrKeQl5qbaKrn4BOi6ZeYAfaqkUJaY UwoUCkgsLlbSt8M0ITTETdcCpjFC1zckCK7HyAANJKxhzNjSu5qx4Lt6xYsHt1gbGH/LdTFy ckgImEi0bfnPBGGLSVy4t56ti5GLQ0hgKaPErTcvGEESvAKCEj8m32PpYuTgYBaQlzhyKRsk zCygLjFp3iJmiPoHjBKTt1+AqteQWPz9BzOIzSKgKnFq+i1WEJtNQEti/4sbbCA2v4CixNUf jxlBZooKREh0n6gEmSMicJhJ4suxT2BDmQVeMkmsvneKHaRBWCBB4tO7nSwQ27qYJB7O2ACW 4BRwljj0/QPTBEbBWUiOnYVw7Cwkxy5gZF7FKJFakFxQnJSea5iXWq5XnJhbXJqXrpecn7uJ EZy4nkntYDy4y/0QowAHoxIPr4f3yXAh1sSy4srcQ4wSHMxKIryvIoFCvCmJlVWpRfnxRaU5 qcWHGE2Bvp3ILCWanA9Mqnkl8YbGJmZGlkbmhhZGxuZK4ryP/68LExJITyxJzU5NLUgtgulj 4uCUamBcvePW1Efrjx9dESdhpFpkPf9XguyUj1cdF5q6Lc9aNb1F3pnloGrBjN9Leq5IZ/1V YtoetpGXKy/RpUOlZJXmapUETYe5j5fuci0QY5y8i2VWktB9s/qafDkP+QtixaGfV/Y79c46 s2ih3DrZrovRkws/HmpYdCcqNmJBtKlvd1vfba1Ot5lKLMUZiYZazEXFiQBwqJugcgMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Javier, On 2016년 08월 27일 02:49, Javier Martinez Canillas wrote: > 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? OK. I'll add a comment as following: offset = <0x400>; /* SWRESET */ > > [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 = ). OK. I'll use the GIC_SPI at all of them. > > [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. The mipi_phy address are included in the PMU (base 0x105c_0000). There is no memory map for only MIPI_PHY. Instead, the mipi_phy driver[1] uses the regmap (pmu, syscon_cam0/1/disp). [1] drivers/phy/phy-exynos-mipi-video.c don't use Also, the first MIPI_PHY's address is 0x105c0710. I'll fix it. If you want to add the 'reg' property, I can add DT node as following: mipi_phy: video-phy@105c0710 { reg = <0x105c0710 0x0>; compatible = "samsung,exynos5433-mipi-video-phy"; ... }; But it looks strange because the mipi_phy driver[1] never gets the base address with of_iomap(). > > [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. The usbdrd[2] driver don't map the base address. So, this DT node don't include any code calling the of_iomap(). [2] drivers/usb/dwc3/dwc3-exynos.c But, the usbdrd's base address is 0x15400000. > >> + dwc3 { >> + compatible = "snps,dwc3"; >> + reg = <0x15400000 0x10000>; > > Ditto, the node doesn't have a unit address but has reg property. OK. I'll modify it as following: dwc3@15400000 { > >> + 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. This case is same with above 'usbdrd30'. > >> + usbdrd_dwc3_0: dwc3 { >> + compatible = "snps,dwc3"; >> + reg = <0x15a00000 0x10000>; > > Ditto, the node doesn't have a unit address but has reg property. I'll modify it as following: usbdrd_dwc3_0: dwc3@15a00000 { > >> + interrupts = <0 244 0>; >> + phys = <&usbhost30_phy 0>, <&usbhost30_phy 1>; >> + phy-names = "usb2-phy", "usb3-phy"; >> + }; >> + }; >> + > > [snip] > >> + audio-subsystem { I'll modify it as following: audio-subsystem@11400000 { >> + 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 Thanks for your review. -- Best Regards, Chanwoo Choi