Re: [PATCH 2/6] ARM: dts: nuc900: Add nuc970 dts files

Arnd Bergmann
2016-06-29T15:24:10Z

On Saturday, June 25, 2016 6:37:18 PM CEST Wan Zongshun wrote:
> This patch is to add dts support for nuc970 platform.
>
> Signed-off-by: Wan Zongshun <mcuos.com-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> .../devicetree/bindings/arm/nuvoton/nuc970.txt | 30 +++
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/nuc970-evb.dts | 20 ++
> arch/arm/boot/dts/nuc970.dtsi | 93 ++++++++
> include/dt-bindings/clock/nuc970-clock.h | 233 +++++++++++++++++++++
> 5 files changed, 377 insertions(+)

I'd suggest splitting this into multiple patches: the binding, the dts files and the header.

> +Boards with the NUC970 SoC shall have the following properties:
> +
> +Root node required properties:
> +- compatible: Should be "nuvoton,nuc970evb", "nuvoton,nuc970"

Better don't mention "nuvoton,nuc970evb", as that is the board specific
identifier: we don't want to have to list every single board.

> +Timer required properties:
> +- compatible: Should be "nuvoton,tmr"
> +- reg: Should contain registers location and length
> +- interrupts: hwirq is direct mapping to irq number
> +- clocks: phandle to input clock.
> +
> +Clock required properties:
> +- compatible: Should be "nuvoton,clk"
> +- reg: Should contain registers location and length
> +
> +Interrupt-controller required properties
> +- compatible: Should be "nuvoton,aic"
> +- reg: Should contain registers location and length
> +- interrupt-cells: set to 1
> +
> +GCR register required properties:
> +- compatible: Should be "nuvoton,gcr"
> +- reg: Should contain registers location and length

These compatible strings are all very generic, and should contain the
SoC name.

> diff --git a/arch/arm/boot/dts/nuc970.dtsi b/arch/arm/boot/dts/nuc970.dtsi
> new file mode 100644
> index 0000000..8a6c225
> --- /dev/null
> +++ b/arch/arm/boot/dts/nuc970.dtsi
> @@ -0,0 +1,93 @@
> +/*
> + * Copyright 2016 Wan Zongshun <mcuos.com-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include "skeleton.dtsi"
> +#include <dt-bindings/clock/nuc970-clock.h>
> +
> +/ {
> + compatible = "nuvoton,nuc970evb", "nuvoton,nuc970";
> +
> + interrupt-parent = <&aic>;
> +
> + aliases {
> + serial0 = &uart0;
> + };
> +
> + memory {
> + reg = <0x00000000 0x04000000>;
> + };

Better split this into SoC-specific and board-specific contents, the
aliases and memory should go into the board.dts file, along with the
alias for the serial port.

> + ahb@B0000000 {

Don't capatilize the hexadecimal numbers.

> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xB0000000 0x9000>;
> + ranges;

A simple-bus should not have a 'reg' property. You can have a 'ranges'
though, to translate the addresses in the child nodes.

Arnd