Re: [PATCH 3/5] MIPS: dt: xilfpga: Add xilfpga device tree files.

[James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4469 bytes --]

On Wed, Oct 14, 2015 at 01:51:55PM +0100, Zubair Lutfullah Kakakhel wrote:
> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Any chance of a little more explanation, other than the subject line?

> ---
>  arch/mips/boot/dts/Makefile                |  1 +
>  arch/mips/boot/dts/xilfpga/Makefile        |  9 ++++++
>  arch/mips/boot/dts/xilfpga/microAptiv.dtsi | 21 +++++++++++++
>  arch/mips/boot/dts/xilfpga/nexys4ddr.dts   | 47 ++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+)
>  create mode 100644 arch/mips/boot/dts/xilfpga/Makefile
>  create mode 100644 arch/mips/boot/dts/xilfpga/microAptiv.dtsi
>  create mode 100644 arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> 
> diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
> index 778a340..0571ef7 100644
> --- a/arch/mips/boot/dts/Makefile
> +++ b/arch/mips/boot/dts/Makefile
> @@ -6,6 +6,7 @@ dts-dirs	+= mti
>  dts-dirs	+= netlogic
>  dts-dirs	+= qca
>  dts-dirs	+= ralink
> +dts-dirs	+= xilfpga
>  
>  obj-y		:= $(addsuffix /, $(dts-dirs))
>  
> diff --git a/arch/mips/boot/dts/xilfpga/Makefile b/arch/mips/boot/dts/xilfpga/Makefile
> new file mode 100644
> index 0000000..913a752
> --- /dev/null
> +++ b/arch/mips/boot/dts/xilfpga/Makefile
> @@ -0,0 +1,9 @@
> +dtb-$(CONFIG_XILFPGA_NEXYS4DDR)	+= nexys4ddr.dtb
> +
> +obj-y				+= $(patsubst %.dtb, %.dtb.o, $(dtb-y))
> +
> +# Force kbuild to make empty built-in.o if necessary
> +obj-				+= dummy.o
> +
> +always				:= $(dtb-y)
> +clean-files	:= *.dtb *.dtb.S
> diff --git a/arch/mips/boot/dts/xilfpga/microAptiv.dtsi b/arch/mips/boot/dts/xilfpga/microAptiv.dtsi
> new file mode 100644
> index 0000000..81d518e
> --- /dev/null
> +++ b/arch/mips/boot/dts/xilfpga/microAptiv.dtsi
> @@ -0,0 +1,21 @@
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "img,xilfpga";
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "mips,m14Kc";
> +			clocks	= <&ext>;
> +			reg = <0>;
> +		};
> +	};
> +
> +	ext: ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +};
> diff --git a/arch/mips/boot/dts/xilfpga/nexys4ddr.dts b/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> new file mode 100644
> index 0000000..e225ae7
> --- /dev/null
> +++ b/arch/mips/boot/dts/xilfpga/nexys4ddr.dts
> @@ -0,0 +1,47 @@
> +/dts-v1/;
> +
> +#include "microAptiv.dtsi"
> +
> +/ {
> +	compatible = "img,xilfpga";

You can have more than one compatible string, separated by commas. Does
it make sense to have one that describes the specific system first
(nexys4ddr??) in addition to the generic xilfpga one? That way software
can still distinguish if it becomes necessary.

> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x0 0x07ffffff>;

I think the second cell is size, not end address, so it should
presumably be 0x08000000?

> +	};
> +
> +	cpuintc: interrupt-controller@0 {
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +		interrupt-controller;
> +		compatible = "mti,cpu-interrupt-controller";
> +	};
> +
> +	axi_gpio: gpio@10600000 {
> +		#gpio-cells = <1>;
> +		compatible = "xlnx,xps-gpio-1.00.a";
> +		gpio-controller;
> +		reg = < 0x10600000 0x10000 >;

Inconsistent whitespace between < > compared to other properties.

> +		xlnx,all-inputs = <0x0>;
> +		xlnx,dout-default = <0x0>;
> +		xlnx,gpio-width = <0x16>;
> +		xlnx,interrupt-present = <0x0>;
> +		xlnx,is-dual = <0x0>;
> +		xlnx,tri-default = <0xffffffff>;
> +	} ;
> +
> +	axi_uart16550: serial@10400000 {
> +		compatible = "ns16550a";
> +		reg = <0x10400000 0x10000>;
> +
> +		reg-shift = <2>;
> +		reg-offset = <0x1000>;
> +
> +		clock-frequency = <50000000>;

I guess the binding for that device already exists, but is it worth
extending it to take a clock node reference instead of a hardcoded
frequency (I'm guessing ext by the frequency and choice of clocks)?

> +		status = "okay";

I don't think you normally need that unless you describe e.g. SoC
peripheral hardware that might not be brought out by the board, in which
case you'd say its disabled in the SoC dtsi file, but enable it at the
board level, which doesn't seem to be happening here.

Cheers
James

> +	};
> +};
> +
> +&ext {
> +	clock-frequency = <50000000>;
> +};
> -- 
> 1.9.1
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]