lkml.org 
[lkml]   [2018]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 16/19] csky: Device tree
    On Mon, Mar 19, 2018 at 3:51 AM, Guo Ren <ren_guo@c-sky.com> wrote:
    > Signed-off-by: Guo Ren <ren_guo@c-sky.com>

    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 <dt-bindings/gpio/gpio.h>
    > +#include <dt-bindings/input/input.h>

    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

    \
     
     \ /
      Last update: 2018-03-19 16:29    [W:4.302 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site