On Wed, Sep 20, 2017 at 08:53:03PM +0000, Rob Herring wrote:

On Sun, Sep 17, 2017 at 03:23:27AM -0500, Marty E. Plummer wrote:

quoted

Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
marketed under the name Samsung SDR-B74301N

Signed-off-by: Marty E. Plummer <redacted>
---
 arch/arm/boot/dts/Makefile              |   2 +
 arch/arm/boot/dts/hi3521a-rs-dm290e.dts |  52 ++++++
 arch/arm/boot/dts/hi3521a.dtsi          | 310 ++++++++++++++++++++++++++++++++
 3 files changed, 364 insertions(+)
 create mode 100644 arch/arm/boot/dts/hi3521a-rs-dm290e.dts
 create mode 100644 arch/arm/boot/dts/hi3521a.dtsi

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index faf46abaa4a2..e7b9b5dde20f 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile

@@ -189,6 +189,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
 	gemini-sq201.dtb \
 	gemini-wbd111.dtb \
 	gemini-wbd222.dtb
+dtb-$(CONFIG_ARCH_HI3521A) += \
+	hi3521a-rs-dm290e.dtb
 dtb-$(CONFIG_ARCH_HI3xxx) += \
 	hi3620-hi4511.dtb
 dtb-$(CONFIG_ARCH_HIGHBANK) += \

diff --git a/arch/arm/boot/dts/hi3521a-rs-dm290e.dts b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
new file mode 100644
index 000000000000..b32c8392c93f
--- /dev/null
+++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts

@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2017 Marty Plummer <hanetzer@startmail.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.

Should be version 2 or later? Doesn't really matter to me from a DT 
perspective, but it is in the kernel tree.

You can use SPDX tags if you want.

Oh, that's a good idea. I hadn't seen any SPDX tags in the tree that I
noticed before. I ended up just using the :Gpl command from neovim. 

quoted

+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/dts-v1/;
+#include "hi3521a.dtsi"
+
+/ {
+	model = "RaySharp RS-DM-290E DVR Board";
+	compatible = "hisilicon,hi3521a";

Needs a board compatible too.

Something like `compatible = "hisilicon,hi3521a", "raysharp,rs-dm-290e";` ? 

quoted

+
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+	};
+
+	memory {

Needs a unit-address.

Could you explain what you mean here? As in, memory@someaddr? What would
I use here? 

quoted

+		device_type = "memory";
+		reg = <0x80000000 0xf00000>;
+	};
+};
+
+&hi_sfc {
+	status = "okay";
+	spi-nor@0 {
+		compatible = "jedec,spi-nor";

I don't remember offhand, but I think this should have a device specific 
compatible too.

Instead of "jedec,spi-nor" ? Specific to the SPI chip? 

quoted

+		reg = <0>;
+		spi-max-frequency = <104000000>;
+	};
+};
+
+&uart0 {
+	status = "okay";
+};
+
+&dual_timer0 {
+	status = "okay";
+};

diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
new file mode 100644
index 000000000000..2af746fdec46
--- /dev/null
+++ b/arch/arm/boot/dts/hi3521a.dtsi

@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2017 Marty Plummer <hanetzer@startmail.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <dt-bindings/clock/hi3521a-clock.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	chosen { };
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0>;
+		};
+	};
+
+	hi_sfc: spi-nor-controller@10000000 {
+		compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x10000000 0x10000>, <0x14000000 0x1000000>;
+		reg-names = "control", "memory";
+		clocks = <&crg HI3521A_FMC_CLK>;
+		status = "disabled";
+	};
+
+	gic: interrupt-controller@10300000 {
+		compatible = "arm,pl390";
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
+	};
+
+	clk_3m: clk_3m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <3000000>;
+	};
+
+	crg: clock-reset-controller@12040000 {
+		compatible = "hisilicon,hi3521a-crg";
+		#clock-cells = <1>;
+		#reset-cells = <2>;
+		reg = <0x12040000 0x10000>;
+	};

These memory mapped peripherals should be under a bus node.

Crap, will fix. 

quoted

+
+	soc {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&gic>;
+		ranges;

It is preferred to have a value here and limit the range of the bus 
addresses.

Yeah, I think I've seen that before, I don't quite grok how that works.

quoted

+
+		dmac: dma@10060000 {

dma-controller@...

Will fix.

quoted

+			compatible = "arm,pl080", "arm,primecell";
+			reg = <0x10060000 0x1000>;
+			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";

I wouldn't think enabling dma would be a per board decision.

I've just noticed that in general dtsi files just lay it all out and are
mostly "disabled", though if you think this should be explicitly enabled
thats fine by me.

quoted

+		};
+
+		dual_timer0: timer@12000000 {
+			compatible = "arm,sp804", "arm,primecell";
+			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x12000000 0x1000>;
+			clocks = <&clk_3m>;
+			clock-names = "apb_pclk";

IIRC, it is deprecated to have a single clock here. The h/w has 2 clock 
inputs.

Are you meaning for the 0x0 index and 0x20 index clocks?

Where's the ARM architected timer?

Unsure tbqf, just doing my best to translate a datasheet into code. Do
all ARM soc's have one?

quoted

-- 
2.14.1