From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755882AbeCSP2N (ORCPT ); Mon, 19 Mar 2018 11:28:13 -0400 Received: from mail-it0-f66.google.com ([209.85.214.66]:40502 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755773AbeCSP2F (ORCPT ); Mon, 19 Mar 2018 11:28:05 -0400 X-Google-Smtp-Source: AG47ELu5n3fGspgPZ+flQi1ORU+hKUezp/JRsmYj2wkJbbjleQ+xYkgVdfa4o0YsSLck05WGbSJmp32qJIp+n1CJy34= MIME-Version: 1.0 In-Reply-To: References: From: Arnd Bergmann Date: Mon, 19 Mar 2018 23:28:03 +0800 X-Google-Sender-Auth: no3h05YwrL6maGiq1U_bVcgUR7k Message-ID: Subject: Re: [PATCH 16/19] csky: Device tree To: Guo Ren Cc: linux-arch , Linux Kernel Mailing List , Thomas Gleixner , Daniel Lezcano , Jason Cooper , c-sky_gcc_upstream@c-sky.com, gnu-csky@mentor.com, thomas.petazzoni@bootlin.com, wbx@uclibc-ng.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 19, 2018 at 3:51 AM, Guo Ren wrote: > Signed-off-by: Guo Ren Please add a changelog text to each patch, and send patches that add .dts files or binding documents to the devicetree mailing list. > --- > arch/csky/boot/dts/gx6605s.dts | 159 +++++++++++++++++++++++++++++++++ > arch/csky/boot/dts/include/dt-bindings | 1 + > arch/csky/boot/dts/qemu.dts | 87 ++++++++++++++++++ > 3 files changed, 247 insertions(+) > create mode 100644 arch/csky/boot/dts/gx6605s.dts > create mode 120000 arch/csky/boot/dts/include/dt-bindings > create mode 100644 arch/csky/boot/dts/qemu.dts > > diff --git a/arch/csky/boot/dts/gx6605s.dts b/arch/csky/boot/dts/gx6605s.dts > new file mode 100644 > index 0000000..0d34d22 > --- /dev/null > +++ b/arch/csky/boot/dts/gx6605s.dts > @@ -0,0 +1,159 @@ > +/dts-v1/; > +#include > +#include It is usually better for an SoC based board to split the SoC specific into a separate .dtsi file that gets included by the board .dts file. > +/ { > + model = "Nationalchip gx6605s ck610"; > + compatible = "nationalchip,gx6605s,ck610"; Is ck610 the name of the CPU core? The general convention is to have the top-level "compatible" property list first the name of the board, then the name of the soc, but not the name of the CPU core. > + #address-cells = <1>; > + #size-cells = <1>; > + > + memory { > + device_type = "memory"; > + reg = <0x10000000 0x04000000>; > + }; > + > + cpus { > + #address-cells = <0>; > + #size-cells = <0>; > + > + cpu { > + device_type = "cpu"; > + ccr = <0x7d>; > + hint = <0x1c>; Here you should list the specific type of CPU in the compatible property and document the binding for that string in the Documentations/devicetree/bindings hierarchy. Without a binding, the 'ccr' and 'hint' properties make no sense. If there is any chance that you could have SMP systems in the future, it would be better to start with #address-cells=<1>, with appropriate reg properties. > + soc { > + #address-cells = <1>; > + #size-cells = <1>; > + compatible = "simple-bus"; > + ranges; > + > + intc: interrupt-controller { > + compatible = "nationalchip,intc-v1,ave"; > + reg = <0x00500000 0x400>; Each node with a register property also needs the address in the node name, e.g. "interrupt-controller@500000" Try building the dtb file with 'make W=1' to get warnings about when you got that wrong. > + interrupt-controller; > + #interrupt-cells = <1>; > + }; > + > + timer0 { > + compatible = "nationalchip,timer-v1"; This should be "timer@400" Also, each device node should have a binding documentation to explain the binding associated with that "compatible" string. > + reg = <0x0020a000 0x400>; > + clock-frequency = <1000000>; > + interrupts = <10>; > + interrupt-parent = <&intc>; > + }; > + > + ehci: ehci-hcd { > + compatible = "generic-ehci"; > + reg = <0x00900000 0x400>; > + interrupt-parent = <&intc>; > + interrupts = <59>; > + }; > + > + ohci0: ohci-hcd0 { The names here should be "usb@...", not "ehci-hcd" > + chosen { > + bootargs = "console=ttyS0,115200 rdinit=/sbin/init root=/dev/ram0"; > + }; The bootargs should not be in the dts file normally, they should come from the boot loader. For the console, use the "stdout-path" property. Arnd