Re: [PATCH 16/19] csky: Device tree

[Guo Ren <ren_guo@c-sky.com>]

Hi Arnd,

On Mon, Mar 19, 2018 at 11:28:03PM +0800, Arnd Bergmann wrote:
> Please add a changelog text to each patch, and send patches that add
> .dts files or
> binding documents to the devicetree mailing list.
Ok

> 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.
Ok

> > +/ {
> > +       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.
Yes, ck610 is a cpu core of abiv1. and I will change it like this:
	model = "Zhuxian-sword gx6605s Development Board";
	compatible = "zhuxian-sword,gx6605s";
ref: https://c-sky.github.io/docs/gx6605s.html

> 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.
Sorry, I forget remove the cpu region. It's no use now.

> 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.
Ok

> Each node with a register property also needs the address in the
> node name, e.g. "interrupt-controller@500000"
Ok

> Try building the dtb file with 'make W=1' to get warnings about
> when you got that wrong.
Thx

> > +               timer0 {
> > +                       compatible = "nationalchip,timer-v1";
> 
> This should be "timer@400"
Ok

> Also, each device node should have a binding documentation
> to explain the binding associated with that "compatible"
> string.
Ok, I will add them.

> > +               ohci0: ohci-hcd0 {
> The names here should be "usb@...", not "ehci-hcd"
Ok

> > +       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.
I want to keep bootargs in dts, because the bootloader only pass the dtb to kernel. 

> For the console, use the "stdout-path" property.
Ok

Best Regards
  Guo Ren