[PATCH 0/2] Hi3521a support.

[PATCH 0/2] Hi3521a support.

[Marty E. Plummer]

Hey folks. Its been a while. Finally got back on the kernel dev train.
This *mostly* seems to work, but I've ran into an issue which may
require some upstream support.

Basic gist, I was attempting to boot a buildroot-built intramfs, was
mostly working, but it would not give me a login prompt no matter how
much I badgered it, so I decided to try a flash boot. That also failed,
and after much banging my head on the desk and annoying people on irc,
we finally came across *why* it was failing, or at least part of it.
The kernel parser could produce the mtdblockN partitions based on the
devicetree, but for whatever reason, when the hisilicon,fmc-spi-nor read
its superblock, instead of the magic 0x73717368 (sqsh), it read back 0x73717360.
At first I thought it was a mistake of mine, as we had tried modifying
the header, and 0x60 is a `, so it could be a hex editing issue, but nope.
Reading the data in u-boot (2010.06, vendor fork, would like to get mainline
running on it at some point) showed the correct magic, so its something
with the controller driver, I guess. CC'ing the people associated with
that as well, hopefully we can get to the bottom of this.

Marty E. Plummer (2):
  clk: hisilicon: add CRG driver Hi3521a SoC
  arm: hisi: enable Hi3521a soc

 arch/arm/boot/dts/Makefile                |   2 +
 arch/arm/mach-hisi/Kconfig                |   9 ++
 drivers/clk/hisilicon/Kconfig             |   8 ++
 drivers/clk/hisilicon/Makefile            |   1 +
 drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
 include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
 6 files changed, 195 insertions(+)
 create mode 100644 drivers/clk/hisilicon/crg-hi3521a.c
 create mode 100644 include/dt-bindings/clock/hi3521a-clock.h

-- 
2.35.1

[PATCH 0/2] Hi3521a support.

[Marty E. Plummer]

Hey folks. Its been a while. Finally got back on the kernel dev train.
This *mostly* seems to work, but I've ran into an issue which may
require some upstream support.

Basic gist, I was attempting to boot a buildroot-built intramfs, was
mostly working, but it would not give me a login prompt no matter how
much I badgered it, so I decided to try a flash boot. That also failed,
and after much banging my head on the desk and annoying people on irc,
we finally came across *why* it was failing, or at least part of it.
The kernel parser could produce the mtdblockN partitions based on the
devicetree, but for whatever reason, when the hisilicon,fmc-spi-nor read
its superblock, instead of the magic 0x73717368 (sqsh), it read back 0x73717360.
At first I thought it was a mistake of mine, as we had tried modifying
the header, and 0x60 is a `, so it could be a hex editing issue, but nope.
Reading the data in u-boot (2010.06, vendor fork, would like to get mainline
running on it at some point) showed the correct magic, so its something
with the controller driver, I guess. CC'ing the people associated with
that as well, hopefully we can get to the bottom of this.

Marty E. Plummer (2):
  clk: hisilicon: add CRG driver Hi3521a SoC
  arm: hisi: enable Hi3521a soc

 arch/arm/boot/dts/Makefile                |   2 +
 arch/arm/mach-hisi/Kconfig                |   9 ++
 drivers/clk/hisilicon/Kconfig             |   8 ++
 drivers/clk/hisilicon/Makefile            |   1 +
 drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
 include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
 6 files changed, 195 insertions(+)
 create mode 100644 drivers/clk/hisilicon/crg-hi3521a.c
 create mode 100644 include/dt-bindings/clock/hi3521a-clock.h

-- 
2.35.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[RFC v2 0/2] Hi3521a support.

[Marty E. Plummer]

Resend RFC.

Changes in v2:
- Actually include the dts files.
- DT Bindings still missing, as the the driver is not quite complete
  (need to add the reset controller bindings, have't quite figured that
  out yet.)

Marty E. Plummer (2):
  clk: hisilicon: add CRG driver Hi3521a SoC
  arm: hisi: enable Hi3521a soc

 arch/arm/boot/dts/Makefile                |   2 +
 arch/arm/boot/dts/hi3521a-rs-dm290e.dts   | 134 +++++++
 arch/arm/boot/dts/hi3521a.dtsi            | 423 ++++++++++++++++++++++
 arch/arm/mach-hisi/Kconfig                |   9 +
 drivers/clk/hisilicon/Kconfig             |   8 +
 drivers/clk/hisilicon/Makefile            |   1 +
 drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++
 include/dt-bindings/clock/hi3521a-clock.h |  34 ++
 8 files changed, 752 insertions(+)
 create mode 100644 arch/arm/boot/dts/hi3521a-rs-dm290e.dts
 create mode 100644 arch/arm/boot/dts/hi3521a.dtsi
 create mode 100644 drivers/clk/hisilicon/crg-hi3521a.c
 create mode 100644 include/dt-bindings/clock/hi3521a-clock.h

-- 
2.35.1

[RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
generates clock and reset signals used by other module blocks on SoC.

Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
---
 drivers/clk/hisilicon/Kconfig             |   8 ++
 drivers/clk/hisilicon/Makefile            |   1 +
 drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
 include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/clk/hisilicon/crg-hi3521a.c
 create mode 100644 include/dt-bindings/clock/hi3521a-clock.h

diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index c1ec75aa4ccd..72435c06bf4d 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -15,6 +15,14 @@ config COMMON_CLK_HI3519
 	help
 	  Build the clock driver for hi3519.
 
+config COMMON_CLK_HI3521A
+	tristate "Hi3521a Clock Driver"
+	depends on ARCH_HISI || COMPILE_TEST
+	select RESET_HISI
+	default ARCH_HISI
+	help
+	  Build the clock driver for hi3521a.
+
 config COMMON_CLK_HI3559A
 	bool "Hi3559A Clock Driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index 2978e56cb876..dc27acc5b885 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ARCH_HIP04)	+= clk-hip04.o
 obj-$(CONFIG_ARCH_HIX5HD2)	+= clk-hix5hd2.o
 obj-$(CONFIG_COMMON_CLK_HI3516CV300)	+= crg-hi3516cv300.o
 obj-$(CONFIG_COMMON_CLK_HI3519)	+= clk-hi3519.o
+obj-$(CONFIG_COMMON_CLK_HI3521A)	+= crg-hi3521a.o
 obj-$(CONFIG_COMMON_CLK_HI3559A)	+= clk-hi3559a.o
 obj-$(CONFIG_COMMON_CLK_HI3660) += clk-hi3660.o
 obj-$(CONFIG_COMMON_CLK_HI3670) += clk-hi3670.o
diff --git a/drivers/clk/hisilicon/crg-hi3521a.c b/drivers/clk/hisilicon/crg-hi3521a.c
new file mode 100644
index 000000000000..42d8ff440f07
--- /dev/null
+++ b/drivers/clk/hisilicon/crg-hi3521a.c
@@ -0,0 +1,141 @@
+/* SPDX-License-Identifier:	GPL-2.0-or-later */
+/*
+ * Copyright (C) 2017-2022 Marty E. Plummer <hanetzer@startmail.com>
+ */
+#include <linux/clk-provider.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/clock/hi3521a-clock.h>
+
+#include "clk.h"
+#include "crg.h"
+#include "reset.h"
+
+#define HI3521A_INNER_CLK_OFFSET	64
+#define HI3521A_FIXED_2M			65
+#define HI3521A_FIXED_24M			66
+#define HI3521A_FIXED_50M			67
+#define HI3521A_FIXED_83M			68
+#define HI3521A_FIXED_100M			69
+#define HI3521A_FIXED_150M			70
+#define HI3521A_FIXED_202P5M		71
+#define HI3521A_FIXED_250M			72
+#define HI3521A_SYSAXI_MUX			73
+#define HI3521A_FMC_MUX				74
+#define HI3521A_UART_MUX			75
+#define HI3521A_SP804_CLK			76
+
+#define HI3521A_NR_CLKS				128
+
+static const struct hisi_fixed_rate_clock hi3521a_fixed_rate_clks[] = {
+	{ HI3521A_FIXED_2M,         "2m", NULL, 0,   2000000, },
+	{ HI3521A_FIXED_3M,         "3m", NULL, 0,   3000000, },
+	{ HI3521A_FIXED_24M,       "24m", NULL, 0,  24000000, },
+	{ HI3521A_FIXED_50M,       "50m", NULL, 0,  50000000, },
+	{ HI3521A_FIXED_83M,       "83m", NULL, 0,  83000000, },
+	{ HI3521A_FIXED_100M,     "100m", NULL, 0, 100000000, },
+	{ HI3521A_FIXED_150M,     "150m", NULL, 0, 150000000, },
+	{ HI3521A_FIXED_202P5M, "202p5m", NULL, 0, 202500000, },
+	{ HI3521A_FIXED_250M,     "250m", NULL, 0, 250000000, },
+};
+
+static const char *const sysaxi_mux_p[] = { "24m", "250m", "202p5m", };
+static const char *const uart_mux_p[] = { "apb", "2m", "24m", };
+static const char *const fmc_mux_p[] = { "24m", "83m", "150m", };
+static const char *const timer_mux_p[] = { "3m", "clk_sp804", };
+
+static const u32 sysaxi_mux_table[] = {0, 1, 2};
+static const u32 uart_mux_table[] = {0, 1, 2};
+static const u32 fmc_mux_table[] = {0, 1, 2};
+static const u32 timer_mux_table[] = {0, 1};
+
+static const struct hisi_mux_clock hi3521a_mux_clks[] = {
+	{ HI3521A_APB_CLK, "apb", sysaxi_mux_p, ARRAY_SIZE(sysaxi_mux_p),
+		CLK_SET_RATE_PARENT, 0x34, 12, 2, 0, sysaxi_mux_table, },
+	{ HI3521A_UART_MUX, "uart_mux", uart_mux_p, ARRAY_SIZE(uart_mux_p),
+		CLK_SET_RATE_PARENT, 0x84, 18, 2, 0, uart_mux_table, },
+	{ HI3521A_FMC_MUX, "fmc_mux", fmc_mux_p, ARRAY_SIZE(fmc_mux_p),
+		CLK_SET_RATE_PARENT, 0x74, 2, 2, 0, fmc_mux_table, },
+};
+
+static const struct hisi_gate_clock hi3521a_gate_clks[] = {
+	{ HI3521A_FMC_CLK, "clk_fmc", "fmc_mux", CLK_SET_RATE_PARENT,
+		0x74, 1, 0, },
+	{ HI3521A_ETH_CLK, "clk_eth", NULL, 0, 0x78, 1, 0, },
+	{ HI3521A_ETH_MACIF_CLK, "clk_eth_macif", NULL, 0x78, 3, 0, },
+	{ HI3521A_DMAC_CLK, "clk_dmac", NULL, 0, 0x80, 5, 0, },
+	{ HI3521A_UART0_CLK, "clk_uart0", "uart_mux", CLK_SET_RATE_PARENT,
+		0x84, 15, 0, },
+	{ HI3521A_UART1_CLK, "clk_uart1", "uart_mux", CLK_SET_RATE_PARENT,
+		0x84, 16, 0, },
+	{ HI3521A_UART2_CLK, "clk_uart2", "uart_mux", CLK_SET_RATE_PARENT,
+		0x84, 17, 0, },
+	{ HI3521A_SPI0_CLK, "clk_spi0", "50m", CLK_SET_RATE_PARENT,
+		0x84, 13, 0, },
+};
+
+static const struct hisi_fixed_factor_clock hi3521a_fixed_factor_clks[] = {
+	{ HI3521A_SP804_CLK, "clk_sp804", "apb", 1, 4, CLK_SET_RATE_PARENT },
+};
+
+static void __init hi3521a_crg_init(struct device_node *np)
+{
+	struct hisi_clock_data *clk_data;
+
+	clk_data = hisi_clk_init(np, HI3521A_NR_CLKS);
+	if (!clk_data)
+		return;
+
+	hisi_clk_register_fixed_rate(hi3521a_fixed_rate_clks,
+				ARRAY_SIZE(hi3521a_fixed_rate_clks),
+				clk_data);
+	hisi_clk_register_mux(hi3521a_mux_clks,
+				ARRAY_SIZE(hi3521a_mux_clks),
+				clk_data);
+	hisi_clk_register_gate(hi3521a_gate_clks,
+				ARRAY_SIZE(hi3521a_gate_clks),
+				clk_data);
+	hisi_clk_register_fixed_factor(hi3521a_fixed_factor_clks,
+				ARRAY_SIZE(hi3521a_fixed_factor_clks),
+				clk_data);
+}
+CLK_OF_DECLARE(hi3521a_clk, "hisilicon,hi3521a-crg", hi3521a_crg_init);
+
+#define HI3521A_SYSCTRL_NR_CLKS 16
+
+static const struct hisi_mux_clock hi3521a_sysctrl_mux_clks[] = {
+	{ HI3521A_TIMER0_CLK, "clk_timer0", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 16, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER1_CLK, "clk_timer1", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 18, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER2_CLK, "clk_timer2", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 20, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER3_CLK, "clk_timer3", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 22, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER4_CLK, "clk_timer4", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 25, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER5_CLK, "clk_timer5", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 27, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER6_CLK, "clk_timer6", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 29, 1, 0, timer_mux_table, },
+	{ HI3521A_TIMER7_CLK, "clk_timer7", timer_mux_p, ARRAY_SIZE(timer_mux_p),
+		CLK_SET_RATE_PARENT, 0, 31, 1, 0, timer_mux_table, },
+};
+
+static void __init hi3521a_sysctrl_init(struct device_node *np)
+{
+	struct hisi_clock_data *clk_data;
+
+	clk_data = hisi_clk_init(np, HI3521A_SYSCTRL_NR_CLKS);
+	if (!clk_data)
+		return;
+
+	hisi_clk_register_mux(hi3521a_sysctrl_mux_clks,
+				ARRAY_SIZE(hi3521a_sysctrl_mux_clks),
+				clk_data);
+}
+CLK_OF_DECLARE(hi3521a_sysctrl, "hisilicon,hi3521a-sysctrl", hi3521a_sysctrl_init);
diff --git a/include/dt-bindings/clock/hi3521a-clock.h b/include/dt-bindings/clock/hi3521a-clock.h
new file mode 100644
index 000000000000..416a08079002
--- /dev/null
+++ b/include/dt-bindings/clock/hi3521a-clock.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier:	GPL-2.0-or-later */
+/*
+ * Copyright (C) 2017-2022 Marty E. Plummer <hanetzer@startmail.com>
+ */
+
+#ifndef __DTS_HI3521A_CLOCK_H
+#define __DTS_HI3521A_CLOCK_H
+
+/* clocks provided by the crg */
+#define HI3521A_FIXED_3M		1
+#define HI3521A_FMC_CLK			2
+#define HI3521A_SPI0_CLK		3
+#define HI3521A_UART0_CLK		4
+#define HI3521A_UART1_CLK		5
+#define HI3521A_UART2_CLK		6
+#define HI3521A_DMAC_CLK		7
+#define HI3521A_IR_CLK			8
+#define HI3521A_ETH_CLK			9
+#define HI3521A_ETH_MACIF_CLK	10
+#define HI3521A_USB2_BUS_CLK	11
+#define HI3521A_USB2_PORT_CLK	12
+#define HI3521A_APB_CLK			13
+
+/* clocks provided by the sysctrl */
+#define HI3521A_TIMER0_CLK		1
+#define HI3521A_TIMER1_CLK		2
+#define HI3521A_TIMER2_CLK		3
+#define HI3521A_TIMER3_CLK		4
+#define HI3521A_TIMER4_CLK		5
+#define HI3521A_TIMER5_CLK		6
+#define HI3521A_TIMER6_CLK		7
+#define HI3521A_TIMER7_CLK		8
+
+#endif /* __DTS_HI3521A_CLK_H */
-- 
2.35.1

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> generates clock and reset signals used by other module blocks on SoC.
> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  drivers/clk/hisilicon/Kconfig             |   8 ++
>  drivers/clk/hisilicon/Makefile            |   1 +
>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++

Bindings go to separate patch. Your patchset is unmerge'able.

Best regards,
Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> generates clock and reset signals used by other module blocks on SoC.
> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  drivers/clk/hisilicon/Kconfig             |   8 ++
>  drivers/clk/hisilicon/Makefile            |   1 +
>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++

Bindings go to separate patch. Your patchset is unmerge'able.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> generates clock and reset signals used by other module blocks on SoC.
> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  drivers/clk/hisilicon/Kconfig             |   8 ++
>  drivers/clk/hisilicon/Makefile            |   1 +
>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++

Bindings go to separate patch. Your patchset is unmerge'able.

Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> > generates clock and reset signals used by other module blocks on SoC.
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  drivers/clk/hisilicon/Kconfig             |   8 ++
> >  drivers/clk/hisilicon/Makefile            |   1 +
> >  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> 
> Bindings go to separate patch. Your patchset is unmerge'able.
> 
So, assuming I have the following patches:
1: +include/dt-bindings/clock/hi3521a-clock.h
2: +drivers/clk/hisilicon/crg-hi3521a.c
3: +Documentation/devicetree/bindings/whatever

In what order should they be applied?
> Best regards,
> Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> > generates clock and reset signals used by other module blocks on SoC.
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  drivers/clk/hisilicon/Kconfig             |   8 ++
> >  drivers/clk/hisilicon/Makefile            |   1 +
> >  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> 
> Bindings go to separate patch. Your patchset is unmerge'able.
> 
So, assuming I have the following patches:
1: +include/dt-bindings/clock/hi3521a-clock.h
2: +drivers/clk/hisilicon/crg-hi3521a.c
3: +Documentation/devicetree/bindings/whatever

In what order should they be applied?
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> > generates clock and reset signals used by other module blocks on SoC.
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  drivers/clk/hisilicon/Kconfig             |   8 ++
> >  drivers/clk/hisilicon/Makefile            |   1 +
> >  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> 
> Bindings go to separate patch. Your patchset is unmerge'able.
> 
So, assuming I have the following patches:
1: +include/dt-bindings/clock/hi3521a-clock.h
2: +drivers/clk/hisilicon/crg-hi3521a.c
3: +Documentation/devicetree/bindings/whatever

In what order should they be applied?
> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 12:58, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>> generates clock and reset signals used by other module blocks on SoC.
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>
>> Bindings go to separate patch. Your patchset is unmerge'able.
>>
> So, assuming I have the following patches:
> 1: +include/dt-bindings/clock/hi3521a-clock.h
> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> 3: +Documentation/devicetree/bindings/whatever
> 
> In what order should they be applied?

Applied or sent? The maintainer will apply them in proper order, this is
bisectable.


Best regards,
Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 12:58, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>> generates clock and reset signals used by other module blocks on SoC.
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>
>> Bindings go to separate patch. Your patchset is unmerge'able.
>>
> So, assuming I have the following patches:
> 1: +include/dt-bindings/clock/hi3521a-clock.h
> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> 3: +Documentation/devicetree/bindings/whatever
> 
> In what order should they be applied?

Applied or sent? The maintainer will apply them in proper order, this is
bisectable.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 12:58, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>> generates clock and reset signals used by other module blocks on SoC.
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>
>> Bindings go to separate patch. Your patchset is unmerge'able.
>>
> So, assuming I have the following patches:
> 1: +include/dt-bindings/clock/hi3521a-clock.h
> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> 3: +Documentation/devicetree/bindings/whatever
> 
> In what order should they be applied?

Applied or sent? The maintainer will apply them in proper order, this is
bisectable.


Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 12:58, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>> generates clock and reset signals used by other module blocks on SoC.
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>
> >> Bindings go to separate patch. Your patchset is unmerge'able.
> >>
> > So, assuming I have the following patches:
> > 1: +include/dt-bindings/clock/hi3521a-clock.h
> > 2: +drivers/clk/hisilicon/crg-hi3521a.c
> > 3: +Documentation/devicetree/bindings/whatever
> > 
> > In what order should they be applied?
> 
> Applied or sent? The maintainer will apply them in proper order, this is
> bisectable.
> 
> 
Either or. Whatever makes the workload easier is what I'm looking for.
> Best regards,
> Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 12:58, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>> generates clock and reset signals used by other module blocks on SoC.
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>
> >> Bindings go to separate patch. Your patchset is unmerge'able.
> >>
> > So, assuming I have the following patches:
> > 1: +include/dt-bindings/clock/hi3521a-clock.h
> > 2: +drivers/clk/hisilicon/crg-hi3521a.c
> > 3: +Documentation/devicetree/bindings/whatever
> > 
> > In what order should they be applied?
> 
> Applied or sent? The maintainer will apply them in proper order, this is
> bisectable.
> 
> 
Either or. Whatever makes the workload easier is what I'm looking for.
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 12:58, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>> generates clock and reset signals used by other module blocks on SoC.
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>
> >> Bindings go to separate patch. Your patchset is unmerge'able.
> >>
> > So, assuming I have the following patches:
> > 1: +include/dt-bindings/clock/hi3521a-clock.h
> > 2: +drivers/clk/hisilicon/crg-hi3521a.c
> > 3: +Documentation/devicetree/bindings/whatever
> > 
> > In what order should they be applied?
> 
> Applied or sent? The maintainer will apply them in proper order, this is
> bisectable.
> 
> 
Either or. Whatever makes the workload easier is what I'm looking for.
> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 13:06, Marty E. Plummer wrote:
> On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 12:58, Marty E. Plummer wrote:
>>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>>>> generates clock and reset signals used by other module blocks on SoC.
>>>>>
>>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>> ---
>>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>>>
>>>> Bindings go to separate patch. Your patchset is unmerge'able.
>>>>
>>> So, assuming I have the following patches:
>>> 1: +include/dt-bindings/clock/hi3521a-clock.h
>>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
>>> 3: +Documentation/devicetree/bindings/whatever
>>>
>>> In what order should they be applied?
>>
>> Applied or sent? The maintainer will apply them in proper order, this is
>> bisectable.
>>
>>
> Either or. Whatever makes the workload easier is what I'm looking for.

Sorry, you need to be more specific. Apply is not a job for you, for the
patch submitter.

Then you miss here important piece - which is the first patch. DTS goes
always via separate branch (or even tree) from driver changes. That's
why bindings are always separate first patches.

Best regards,
Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 13:06, Marty E. Plummer wrote:
> On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 12:58, Marty E. Plummer wrote:
>>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>>>> generates clock and reset signals used by other module blocks on SoC.
>>>>>
>>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>> ---
>>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>>>
>>>> Bindings go to separate patch. Your patchset is unmerge'able.
>>>>
>>> So, assuming I have the following patches:
>>> 1: +include/dt-bindings/clock/hi3521a-clock.h
>>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
>>> 3: +Documentation/devicetree/bindings/whatever
>>>
>>> In what order should they be applied?
>>
>> Applied or sent? The maintainer will apply them in proper order, this is
>> bisectable.
>>
>>
> Either or. Whatever makes the workload easier is what I'm looking for.

Sorry, you need to be more specific. Apply is not a job for you, for the
patch submitter.

Then you miss here important piece - which is the first patch. DTS goes
always via separate branch (or even tree) from driver changes. That's
why bindings are always separate first patches.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 13:06, Marty E. Plummer wrote:
> On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 12:58, Marty E. Plummer wrote:
>>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>>>> generates clock and reset signals used by other module blocks on SoC.
>>>>>
>>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>> ---
>>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>>>
>>>> Bindings go to separate patch. Your patchset is unmerge'able.
>>>>
>>> So, assuming I have the following patches:
>>> 1: +include/dt-bindings/clock/hi3521a-clock.h
>>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
>>> 3: +Documentation/devicetree/bindings/whatever
>>>
>>> In what order should they be applied?
>>
>> Applied or sent? The maintainer will apply them in proper order, this is
>> bisectable.
>>
>>
> Either or. Whatever makes the workload easier is what I'm looking for.

Sorry, you need to be more specific. Apply is not a job for you, for the
patch submitter.

Then you miss here important piece - which is the first patch. DTS goes
always via separate branch (or even tree) from driver changes. That's
why bindings are always separate first patches.

Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:09:28PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 13:06, Marty E. Plummer wrote:
> > On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/06/2022 12:58, Marty E. Plummer wrote:
> >>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>>>> generates clock and reset signals used by other module blocks on SoC.
> >>>>>
> >>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>>>> ---
> >>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>>>
> >>>> Bindings go to separate patch. Your patchset is unmerge'able.
> >>>>
> >>> So, assuming I have the following patches:
> >>> 1: +include/dt-bindings/clock/hi3521a-clock.h
> >>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> >>> 3: +Documentation/devicetree/bindings/whatever
> >>>
> >>> In what order should they be applied?
> >>
> >> Applied or sent? The maintainer will apply them in proper order, this is
> >> bisectable.
> >>
> >>
> > Either or. Whatever makes the workload easier is what I'm looking for.
> 
> Sorry, you need to be more specific. Apply is not a job for you, for the
> patch submitter.
> 
> Then you miss here important piece - which is the first patch. DTS goes
> always via separate branch (or even tree) from driver changes. That's
> why bindings are always separate first patches.
> 
So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
to the above list, or should those be the same patch as well?

> Best regards,
> Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:09:28PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 13:06, Marty E. Plummer wrote:
> > On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/06/2022 12:58, Marty E. Plummer wrote:
> >>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>>>> generates clock and reset signals used by other module blocks on SoC.
> >>>>>
> >>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>>>> ---
> >>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>>>
> >>>> Bindings go to separate patch. Your patchset is unmerge'able.
> >>>>
> >>> So, assuming I have the following patches:
> >>> 1: +include/dt-bindings/clock/hi3521a-clock.h
> >>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> >>> 3: +Documentation/devicetree/bindings/whatever
> >>>
> >>> In what order should they be applied?
> >>
> >> Applied or sent? The maintainer will apply them in proper order, this is
> >> bisectable.
> >>
> >>
> > Either or. Whatever makes the workload easier is what I'm looking for.
> 
> Sorry, you need to be more specific. Apply is not a job for you, for the
> patch submitter.
> 
> Then you miss here important piece - which is the first patch. DTS goes
> always via separate branch (or even tree) from driver changes. That's
> why bindings are always separate first patches.
> 
So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
to the above list, or should those be the same patch as well?

> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:09:28PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 13:06, Marty E. Plummer wrote:
> > On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/06/2022 12:58, Marty E. Plummer wrote:
> >>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>>>> generates clock and reset signals used by other module blocks on SoC.
> >>>>>
> >>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>>>> ---
> >>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>>>
> >>>> Bindings go to separate patch. Your patchset is unmerge'able.
> >>>>
> >>> So, assuming I have the following patches:
> >>> 1: +include/dt-bindings/clock/hi3521a-clock.h
> >>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> >>> 3: +Documentation/devicetree/bindings/whatever
> >>>
> >>> In what order should they be applied?
> >>
> >> Applied or sent? The maintainer will apply them in proper order, this is
> >> bisectable.
> >>
> >>
> > Either or. Whatever makes the workload easier is what I'm looking for.
> 
> Sorry, you need to be more specific. Apply is not a job for you, for the
> patch submitter.
> 
> Then you miss here important piece - which is the first patch. DTS goes
> always via separate branch (or even tree) from driver changes. That's
> why bindings are always separate first patches.
> 
So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
to the above list, or should those be the same patch as well?

> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 20:24, Marty E. Plummer wrote:

>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>
>> Sorry, you need to be more specific. Apply is not a job for you, for the
>> patch submitter.
>>
>> Then you miss here important piece - which is the first patch. DTS goes
>> always via separate branch (or even tree) from driver changes. That's
>> why bindings are always separate first patches.
>>
> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> to the above list, or should those be the same patch as well?

For me does not matter, sub architecture maintainer might have preference.

Best regards,
Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 20:24, Marty E. Plummer wrote:

>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>
>> Sorry, you need to be more specific. Apply is not a job for you, for the
>> patch submitter.
>>
>> Then you miss here important piece - which is the first patch. DTS goes
>> always via separate branch (or even tree) from driver changes. That's
>> why bindings are always separate first patches.
>>
> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> to the above list, or should those be the same patch as well?

For me does not matter, sub architecture maintainer might have preference.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 20:24, Marty E. Plummer wrote:

>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>
>> Sorry, you need to be more specific. Apply is not a job for you, for the
>> patch submitter.
>>
>> Then you miss here important piece - which is the first patch. DTS goes
>> always via separate branch (or even tree) from driver changes. That's
>> why bindings are always separate first patches.
>>
> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> to the above list, or should those be the same patch as well?

For me does not matter, sub architecture maintainer might have preference.

Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 20:24, Marty E. Plummer wrote:
> 
> >>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>
> >> Sorry, you need to be more specific. Apply is not a job for you, for the
> >> patch submitter.
> >>
> >> Then you miss here important piece - which is the first patch. DTS goes
> >> always via separate branch (or even tree) from driver changes. That's
> >> why bindings are always separate first patches.
> >>
> > So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> > to the above list, or should those be the same patch as well?
> 
> For me does not matter, sub architecture maintainer might have preference.
> 
Fair enough. That being said, for the dt-bindings patch, is it
permissible to include #define CLOCK_FOO 1337 and so on for clocks which
haven't been wired up in the driver yet? As in, you know they're there,
and are important enough to model, but you haven't gotten to that point
yet?
> Best regards,
> Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 20:24, Marty E. Plummer wrote:
> 
> >>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>
> >> Sorry, you need to be more specific. Apply is not a job for you, for the
> >> patch submitter.
> >>
> >> Then you miss here important piece - which is the first patch. DTS goes
> >> always via separate branch (or even tree) from driver changes. That's
> >> why bindings are always separate first patches.
> >>
> > So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> > to the above list, or should those be the same patch as well?
> 
> For me does not matter, sub architecture maintainer might have preference.
> 
Fair enough. That being said, for the dt-bindings patch, is it
permissible to include #define CLOCK_FOO 1337 and so on for clocks which
haven't been wired up in the driver yet? As in, you know they're there,
and are important enough to model, but you haven't gotten to that point
yet?
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 20:24, Marty E. Plummer wrote:
> 
> >>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>
> >> Sorry, you need to be more specific. Apply is not a job for you, for the
> >> patch submitter.
> >>
> >> Then you miss here important piece - which is the first patch. DTS goes
> >> always via separate branch (or even tree) from driver changes. That's
> >> why bindings are always separate first patches.
> >>
> > So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> > to the above list, or should those be the same patch as well?
> 
> For me does not matter, sub architecture maintainer might have preference.
> 
Fair enough. That being said, for the dt-bindings patch, is it
permissible to include #define CLOCK_FOO 1337 and so on for clocks which
haven't been wired up in the driver yet? As in, you know they're there,
and are important enough to model, but you haven't gotten to that point
yet?
> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 03/06/2022 13:22, Marty E. Plummer wrote:
> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>
>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>
>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>> patch submitter.
>>>>
>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>> always via separate branch (or even tree) from driver changes. That's
>>>> why bindings are always separate first patches.
>>>>
>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>> to the above list, or should those be the same patch as well?
>>
>> For me does not matter, sub architecture maintainer might have preference.
>>
> Fair enough. That being said, for the dt-bindings patch, is it
> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> haven't been wired up in the driver yet? As in, you know they're there,
> and are important enough to model, but you haven't gotten to that point
> yet?

What would be the benefit to include them now? I imagine that if you
plan to add such clocks to the driver in next week or something, and you
need to use them in DTS, then it's fine. If that's not the case,
probably there is little sense in defining them upfront...


Best regards,
Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 03/06/2022 13:22, Marty E. Plummer wrote:
> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>
>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>
>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>> patch submitter.
>>>>
>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>> always via separate branch (or even tree) from driver changes. That's
>>>> why bindings are always separate first patches.
>>>>
>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>> to the above list, or should those be the same patch as well?
>>
>> For me does not matter, sub architecture maintainer might have preference.
>>
> Fair enough. That being said, for the dt-bindings patch, is it
> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> haven't been wired up in the driver yet? As in, you know they're there,
> and are important enough to model, but you haven't gotten to that point
> yet?

What would be the benefit to include them now? I imagine that if you
plan to add such clocks to the driver in next week or something, and you
need to use them in DTS, then it's fine. If that's not the case,
probably there is little sense in defining them upfront...


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 03/06/2022 13:22, Marty E. Plummer wrote:
> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>
>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>
>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>> patch submitter.
>>>>
>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>> always via separate branch (or even tree) from driver changes. That's
>>>> why bindings are always separate first patches.
>>>>
>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>> to the above list, or should those be the same patch as well?
>>
>> For me does not matter, sub architecture maintainer might have preference.
>>
> Fair enough. That being said, for the dt-bindings patch, is it
> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> haven't been wired up in the driver yet? As in, you know they're there,
> and are important enough to model, but you haven't gotten to that point
> yet?

What would be the benefit to include them now? I imagine that if you
plan to add such clocks to the driver in next week or something, and you
need to use them in DTS, then it's fine. If that's not the case,
probably there is little sense in defining them upfront...


Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> On 03/06/2022 13:22, Marty E. Plummer wrote:
>> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>>
>>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>>
>>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>>> patch submitter.
>>>>>
>>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>>> always via separate branch (or even tree) from driver changes. That's
>>>>> why bindings are always separate first patches.
>>>>>
>>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>>> to the above list, or should those be the same patch as well?
>>>
>>> For me does not matter, sub architecture maintainer might have preference.
>>>
>> Fair enough. That being said, for the dt-bindings patch, is it
>> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
>> haven't been wired up in the driver yet? As in, you know they're there,
>> and are important enough to model, but you haven't gotten to that point
>> yet?
> 
> What would be the benefit to include them now? I imagine that if you
> plan to add such clocks to the driver in next week or something, and you
> need to use them in DTS, then it's fine. If that's not the case,
> probably there is little sense in defining them upfront...

Actually I see one more benefit - since IDs should be incremented by
one, you can define all of them upfront thus having some
logical/alphabetical order/grouping. If you extend the bindings header
with new IDs later, they must go to the end of the list, thus maybe
ordering will not be that nice.

If you want, go ahead with all IDs. Just remeber that these must be IDs,
not register values or some programming offsets.

Best regards,
Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> On 03/06/2022 13:22, Marty E. Plummer wrote:
>> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>>
>>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>>
>>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>>> patch submitter.
>>>>>
>>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>>> always via separate branch (or even tree) from driver changes. That's
>>>>> why bindings are always separate first patches.
>>>>>
>>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>>> to the above list, or should those be the same patch as well?
>>>
>>> For me does not matter, sub architecture maintainer might have preference.
>>>
>> Fair enough. That being said, for the dt-bindings patch, is it
>> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
>> haven't been wired up in the driver yet? As in, you know they're there,
>> and are important enough to model, but you haven't gotten to that point
>> yet?
> 
> What would be the benefit to include them now? I imagine that if you
> plan to add such clocks to the driver in next week or something, and you
> need to use them in DTS, then it's fine. If that's not the case,
> probably there is little sense in defining them upfront...

Actually I see one more benefit - since IDs should be incremented by
one, you can define all of them upfront thus having some
logical/alphabetical order/grouping. If you extend the bindings header
with new IDs later, they must go to the end of the list, thus maybe
ordering will not be that nice.

If you want, go ahead with all IDs. Just remeber that these must be IDs,
not register values or some programming offsets.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> On 03/06/2022 13:22, Marty E. Plummer wrote:
>> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>>
>>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>>
>>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>>> patch submitter.
>>>>>
>>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>>> always via separate branch (or even tree) from driver changes. That's
>>>>> why bindings are always separate first patches.
>>>>>
>>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>>> to the above list, or should those be the same patch as well?
>>>
>>> For me does not matter, sub architecture maintainer might have preference.
>>>
>> Fair enough. That being said, for the dt-bindings patch, is it
>> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
>> haven't been wired up in the driver yet? As in, you know they're there,
>> and are important enough to model, but you haven't gotten to that point
>> yet?
> 
> What would be the benefit to include them now? I imagine that if you
> plan to add such clocks to the driver in next week or something, and you
> need to use them in DTS, then it's fine. If that's not the case,
> probably there is little sense in defining them upfront...

Actually I see one more benefit - since IDs should be incremented by
one, you can define all of them upfront thus having some
logical/alphabetical order/grouping. If you extend the bindings header
with new IDs later, they must go to the end of the list, thus maybe
ordering will not be that nice.

If you want, go ahead with all IDs. Just remeber that these must be IDs,
not register values or some programming offsets.

Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Mon, Jun 06, 2022 at 09:29:59AM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> > On 03/06/2022 13:22, Marty E. Plummer wrote:
> >> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> >>> On 01/06/2022 20:24, Marty E. Plummer wrote:
> >>>
> >>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>>>>
> >>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
> >>>>> patch submitter.
> >>>>>
> >>>>> Then you miss here important piece - which is the first patch. DTS goes
> >>>>> always via separate branch (or even tree) from driver changes. That's
> >>>>> why bindings are always separate first patches.
> >>>>>
> >>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> >>>> to the above list, or should those be the same patch as well?
> >>>
> >>> For me does not matter, sub architecture maintainer might have preference.
> >>>
> >> Fair enough. That being said, for the dt-bindings patch, is it
> >> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> >> haven't been wired up in the driver yet? As in, you know they're there,
> >> and are important enough to model, but you haven't gotten to that point
> >> yet?
> > 
> > What would be the benefit to include them now? I imagine that if you
> > plan to add such clocks to the driver in next week or something, and you
> > need to use them in DTS, then it's fine. If that's not the case,
> > probably there is little sense in defining them upfront...
> 
> Actually I see one more benefit - since IDs should be incremented by
> one, you can define all of them upfront thus having some
> logical/alphabetical order/grouping. If you extend the bindings header
> with new IDs later, they must go to the end of the list, thus maybe
> ordering will not be that nice.
> 
> If you want, go ahead with all IDs. Just remeber that these must be IDs,
> not register values or some programming offsets.
> 
Yeah, this was my intent. There are a number of non-standard,
proprietary IP blocks on this soc who's 'organized clock number' come
'in between' the more standard bits, depending on how you decide to
organize them (based on their parent clocks, based on the order they
appear in the crg register block, whatever). I *do* intend on hopefully
putting together drivers for these as well, though that's a long-term
stretch goal.
> Best regards,
> Krzysztof

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Mon, Jun 06, 2022 at 09:29:59AM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> > On 03/06/2022 13:22, Marty E. Plummer wrote:
> >> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> >>> On 01/06/2022 20:24, Marty E. Plummer wrote:
> >>>
> >>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>>>>
> >>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
> >>>>> patch submitter.
> >>>>>
> >>>>> Then you miss here important piece - which is the first patch. DTS goes
> >>>>> always via separate branch (or even tree) from driver changes. That's
> >>>>> why bindings are always separate first patches.
> >>>>>
> >>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> >>>> to the above list, or should those be the same patch as well?
> >>>
> >>> For me does not matter, sub architecture maintainer might have preference.
> >>>
> >> Fair enough. That being said, for the dt-bindings patch, is it
> >> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> >> haven't been wired up in the driver yet? As in, you know they're there,
> >> and are important enough to model, but you haven't gotten to that point
> >> yet?
> > 
> > What would be the benefit to include them now? I imagine that if you
> > plan to add such clocks to the driver in next week or something, and you
> > need to use them in DTS, then it's fine. If that's not the case,
> > probably there is little sense in defining them upfront...
> 
> Actually I see one more benefit - since IDs should be incremented by
> one, you can define all of them upfront thus having some
> logical/alphabetical order/grouping. If you extend the bindings header
> with new IDs later, they must go to the end of the list, thus maybe
> ordering will not be that nice.
> 
> If you want, go ahead with all IDs. Just remeber that these must be IDs,
> not register values or some programming offsets.
> 
Yeah, this was my intent. There are a number of non-standard,
proprietary IP blocks on this soc who's 'organized clock number' come
'in between' the more standard bits, depending on how you decide to
organize them (based on their parent clocks, based on the order they
appear in the crg register block, whatever). I *do* intend on hopefully
putting together drivers for these as well, though that's a long-term
stretch goal.
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Mon, Jun 06, 2022 at 09:29:59AM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> > On 03/06/2022 13:22, Marty E. Plummer wrote:
> >> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> >>> On 01/06/2022 20:24, Marty E. Plummer wrote:
> >>>
> >>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>>>>
> >>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
> >>>>> patch submitter.
> >>>>>
> >>>>> Then you miss here important piece - which is the first patch. DTS goes
> >>>>> always via separate branch (or even tree) from driver changes. That's
> >>>>> why bindings are always separate first patches.
> >>>>>
> >>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> >>>> to the above list, or should those be the same patch as well?
> >>>
> >>> For me does not matter, sub architecture maintainer might have preference.
> >>>
> >> Fair enough. That being said, for the dt-bindings patch, is it
> >> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> >> haven't been wired up in the driver yet? As in, you know they're there,
> >> and are important enough to model, but you haven't gotten to that point
> >> yet?
> > 
> > What would be the benefit to include them now? I imagine that if you
> > plan to add such clocks to the driver in next week or something, and you
> > need to use them in DTS, then it's fine. If that's not the case,
> > probably there is little sense in defining them upfront...
> 
> Actually I see one more benefit - since IDs should be incremented by
> one, you can define all of them upfront thus having some
> logical/alphabetical order/grouping. If you extend the bindings header
> with new IDs later, they must go to the end of the list, thus maybe
> ordering will not be that nice.
> 
> If you want, go ahead with all IDs. Just remeber that these must be IDs,
> not register values or some programming offsets.
> 
Yeah, this was my intent. There are a number of non-standard,
proprietary IP blocks on this soc who's 'organized clock number' come
'in between' the more standard bits, depending on how you decide to
organize them (based on their parent clocks, based on the order they
appear in the crg register block, whatever). I *do* intend on hopefully
putting together drivers for these as well, though that's a long-term
stretch goal.
> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
security systems and ipcameras. The arm core is a Cortex A7.

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 <hanetzer@startmail.com>
---
 arch/arm/boot/dts/Makefile              |   2 +
 arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
 arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
 arch/arm/mach-hisi/Kconfig              |   9 +
 4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
 	gemini-ssi1328.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..b24fcf2ca85e
--- /dev/null
+++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
+ */
+
+#include "hi3521a.dtsi"
+
+/ {
+	model = "RaySharp RS-DM-290E DVR Board";
+	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
+
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	memory@800000000 {
+		device_type = "memory";
+		reg = <0x80000000 0xf00000>;
+	};
+};
+
+&hi_sfc {
+	status = "okay";
+	spi-nor@0 {
+		// compatible = "jedec,spi-nor";
+		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
+		reg = <0>;
+		spi-max-frequency = <150000000>;
+		// spi-rx-bus-width = <1>;
+		// spi-tx-bus-width = <1>;
+		// m25p,default-addr-width = <4>;
+		// spi-max-frequency = <160000000>;
+		m25p,fast-read;
+
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			
+			u-boot@0 {
+				label = "u-boot";
+				reg = <0x0 0x50000>;
+				read-only;
+			};
+			u-boot-env@50000 {
+				label = "u-boot-env";
+				reg = <0x50000 0x20000>;
+			};
+			kernel@70000 {
+				label = "kernel";
+				reg = <0x70000 0x700000>;
+			};
+			rootfs@770000 {
+				label = "rootfs";
+				reg = <0x800000 0x300000>;
+				read-only;
+			};
+			extra@b00000 {
+				label = "extra";
+				reg = <0xb00000 0x1500000>;
+			};
+		};
+	};
+};
+
+&dual_timer0 {
+	status = "okay";
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
+	status = "okay";
+};
+
+&pmx0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&sfc_pmx_func>;
+	uart0_pmx_func: uart0_pmx_func {
+		pinctrl-single,pins = <
+			0x00e8 0x0
+			0x00ec 0x0
+		>;
+	};
+
+	sfc_pmx_func: sfc_pmx_func {
+		pinctrl-single,pins = <
+			0x00c4 0x1
+			0x00c8 0x1
+			0x00cc 0x1
+			0x00d0 0x1
+			0x00d4 0x1
+		>;
+	};
+};
+
+&pmx1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&sfc_cfg_func>;
+	uart0_cfg_func: uart0_cfg_func {
+		pinctrl-single,pins = <
+			0x00e8 0x0
+			0x00ec 0x0
+		>;
+	};
+	sfc_cfg_func: sfc_cfg_func {
+		pinctrl-single,pins = <
+			0x00c4 0x58
+			0x00c8 0x28
+			0x00cc 0x38
+			0x00d0 0x38
+			0x00d4 0x38
+		>;
+	};
+};
+
+/* &gmac0 { */
+/* 	#address-cells = <1>; */
+/* 	#size-cells = <0>; */
+/* 	phy-handle = <&phy3>; */
+/* 	phy-mode = "rgmii"; */
+/* 	mac-address = [00 00 00 00 00 00]; */
+/* 	status = "okay"; */
+/**/
+/* 	phy3: ethernet-phy@3 { */
+/* 		reg = <3>; */
+/* 	}; */
+/* }; */
diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
new file mode 100644
index 000000000000..53993a32fd5d
--- /dev/null
+++ b/arch/arm/boot/dts/hi3521a.dtsi
@@ -0,0 +1,423 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
+ */
+
+#include <dt-bindings/clock/hi3521a-clock.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+/dts-v1/;
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0>;
+			clock-frequency = <1100000000>;
+		};
+	};
+
+	timer {
+		compatible = "arm,armv7-timer";
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_PPI 13 0xf08>,
+			     <GIC_PPI 14 0xf08>,
+			     <GIC_PPI 11 0xf08>,
+			     <GIC_PPI 10 0xf08>;
+		clock-frequency = <24000000>;
+	};
+
+	pmu {
+		compatible = "arm,cortex-a7-pmu";
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	gic: interrupt-controller@10301000 {
+		compatible = "arm,pl390";
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
+	};
+
+	xtal24m: xtal24m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+	};
+
+	clk_3m: clk_3m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <3000000>;
+	};
+
+	soc {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&gic>;
+		ranges;
+
+		hi_sfc: spi@10000000 {
+			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
+			reg-names = "control", "memory";
+			clocks = <&crg HI3521A_FMC_CLK>;
+			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		/* usb0: usb@10030000 { */
+		/* 		compatible = "generic-ohci"; */
+		/* 		reg = <0x10030000 0x1000>; */
+		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
+		/* 		clocks = <&crg > */
+		/* 	}; */
+		dmac: dma-controller@10060000 {
+			compatible = "arm,pl080", "arm,primecell";
+			arm,primecell-periphid = <0x00041080>;
+			reg = <0x10060000 0x1000>;
+			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_DMAC_CLK>;
+			clock-names = "apb_pclk";
+			lli-bus-interface-ahb1;
+			lli-bus-interface-ahb2;
+			mem-bus-interface-ahb1;
+			mem-bus-interface-ahb2;
+			memcpy-burst-size = <256>;
+			memcpy-bus-width = <32>;
+			#dma-cells = <2>;
+			status = "okay";
+		};
+
+		gmac0: ethernet@100a0000 {
+			compatible = "hisilicon,hisi-gmac-v1";
+			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
+			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
+			clock-names = "mac_core", "mac_ifc";
+			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
+			/* reset-names = "mac_core", "mac_ifc", "phy"; */
+			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */
+			status = "disabled";
+		};
+
+		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 = <&sysctrl HI3521A_TIMER0_CLK>,
+				 <&sysctrl HI3521A_TIMER1_CLK>,
+				 <&crg HI3521A_APB_CLK>;
+			clock-names = "timer0clk", "timer1clk", "apb_pclk";
+			status = "disabled";
+		};
+
+		dual_timer1: timer@12010000 {
+			compatible = "arm,sp804", "arm,primecell";
+			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x12010000 0x1000>;
+			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
+				 <&sysctrl HI3521A_TIMER3_CLK>,
+				 <&crg HI3521A_APB_CLK>;
+			clock-names = "timer0clk", "timer1clk", "apb_pclk";
+			status = "disabled";
+		};
+
+		dual_timer2: timer@12020000 {
+			compatible = "arm,sp804", "arm,primecell";
+			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x12020000 0x1000>;
+			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
+				 <&sysctrl HI3521A_TIMER5_CLK>,
+				 <&crg HI3521A_APB_CLK>;
+			clock-names = "timer0clk", "timer1clk", "apb_pclk";
+			status = "disabled";
+		};
+
+		dual_timer3: timer@12030000 {
+			compatible = "arm,sp804", "arm,primecell";
+			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x12030000 0x1000>;
+			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
+				 <&sysctrl HI3521A_TIMER7_CLK>,
+				 <&crg HI3521A_APB_CLK>;
+			clock-names = "timer0clk", "timer1clk", "apb_pclk";
+			status = "disabled";
+		};
+
+		crg: clock-reset-controller@12040000 {
+			compatible = "hisilicon,hi3521a-crg";
+			#clock-cells = <1>;
+			#reset-cells = <2>;
+			reg = <0x12040000 0x1000>;
+		};
+
+		sysctrl: system-controller@12050000 {
+			compatible = "hisilicon,hi3521a-sysctrl", "syscon";
+			reg = <0x12050000 0x1000>;
+			#clock-cells = <1>;
+			#reset-cells = <2>;
+		};
+
+		reboot {
+			compatible = "syscon-reboot";
+			regmap = <&sysctrl>;
+			offset = <0x4>;
+			mask = <0xdeadbeef>;
+		};
+
+		wdt0: watchdog@12070000 {
+			compatible = "arm,sp805", "arm,primecell";
+			arm,primecell-periphid = <0x00141805>;
+			reg = <0x12070000 0x1000>;
+			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_FIXED_3M>;
+			clock-names = "apb_pclk";
+		};
+
+		uart0: serial@12080000 {
+			compatible = "arm,pl011", "arm,primecell";
+			reg = <0x12080000 0x1000>;
+			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
+			clock-names = "uartclk", "apb_pclk";
+			dmas = <&dmac 0 1>, <&dmac 1 2>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		uart1: serial@12090000 {
+			compatible = "arm,pl011", "arm,primecell";
+			reg = <0x12090000 0x1000>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_UART1_CLK>;
+			clock-names = "apb_pclk";
+			dmas = <&dmac 2 1>, <&dmac 3 2>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		uart2: serial@120a0000 {
+			compatible = "arm,pl011", "arm,primecell";
+			reg = <0x120a0000 0x1000>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_UART2_CLK>;
+			clock-names = "apb_pclk";
+			dmas = <&dmac 4 1>, <&dmac 5 2>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi_bus0: spi@120d0000 {
+			compatible = "arm,pl022", "arm,primecell";
+			reg = <0x120d0000 0x1000>;
+			arm,primecell-periphid = <0x00041022>;
+			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&crg HI3521A_SPI0_CLK>;
+			clock-names = "apb_pclk";
+			dmas = <&dmac 6 1>, <&dmac 7 2>;
+			dma-names = "rx", "tx";
+			num-cs = <2>;
+			status = "disabled";
+		};
+
+		pmx0: pinmux@120f0000 {
+			compatible = "pinctrl-single";
+			reg = <0x120f0000 0x188>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			#pinctrl-cells = <1>;
+			#gpio-range-cells = <3>;
+			ranges;
+
+			pinctrl-single,register-width = <32>;
+			pinctrl-single,function-mask = <7>;
+			/* pin base, nr pins & gpio function */
+			pinctrl-single,gpio-range = <
+				&range 58 4 0
+			>;
+
+			range: gpio-range {
+				#pinctrl-single,gpio-range-cells = <3>;
+			};
+		};
+
+		pmx1: pinmux@120f0800 {
+			compatible = "pinconf-single";
+			reg = <0x120f0800 0x1d4>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			#pinctrl-cells = <1>;
+			ranges;
+
+			pinctrl-single,register-width = <32>;
+		};
+
+		gpio0: gpio@12150000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12150000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio1: gpio@12160000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12160000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio2: gpio@12170000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12170000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio3: gpio@12180000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12180000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio4: gpio@12190000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12190000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio5: gpio@121a0000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x121a0000 0x1000>;
+			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio6: gpio@121b0000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x121b0000 0x1000>;
+			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio7: gpio@121c0000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x121c0000 0x1000>;
+			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio8: gpio@121d0000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x121d0000 0x1000>;
+			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio9: gpio@121e0000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x121e0000 0x1000>;
+			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio10: gpio@121f0000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x121f0000 0x1000>;
+			interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio11: gpio@12200000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12200000 0x1000>;
+			interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio12: gpio@12210000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12210000 0x1000>;
+			interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio13: gpio@12220000 {
+			compatible = "arm,pl061", "arm,primecell";
+			reg = <0x12220000 0x1000>;
+			interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			status = "disabled";
+		};
+	};
+};
diff --git a/arch/arm/mach-hisi/Kconfig b/arch/arm/mach-hisi/Kconfig
index 2e980f834a6a..165ffb972157 100644
--- a/arch/arm/mach-hisi/Kconfig
+++ b/arch/arm/mach-hisi/Kconfig
@@ -13,6 +13,15 @@ if ARCH_HISI
 
 menu "Hisilicon platform type"
 
+config ARCH_HI3521A
+	bool "Hisilicon Hi3521a/Hi3520dcv300 family"
+	depends on ARCH_MULTI_V7
+	select CACHE_L2X0
+	select PINCTRL
+	select PINCTRL_SINGLE
+	help
+	  Hisilicon Hi3521a/Hi3520dcv300 family
+
 config ARCH_HI3xxx
 	bool "Hisilicon Hi36xx family"
 	depends on ARCH_MULTI_V7
-- 
2.35.1

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> security systems and ipcameras. The arm core is a Cortex A7.
> 
> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> marketed under the name Samsung SDR-B74301N.

Thank you for your patch. There is something to discuss/improve.

> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  arch/arm/boot/dts/Makefile              |   2 +
>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++

DTSes go to separate patches.

>  arch/arm/mach-hisi/Kconfig              |   9 +
>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>  	gemini-ssi1328.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..b24fcf2ca85e
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include "hi3521a.dtsi"
> +
> +/ {
> +	model = "RaySharp RS-DM-290E DVR Board";
> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";

Please run checkpatch and fix the warnings.

> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	memory@800000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0xf00000>;
> +	};
> +};
> +
> +&hi_sfc {
> +	status = "okay";
> +	spi-nor@0 {

Looks like wrong node name. You need to run "dtbs_check" and fix
warnings related to your DTS.

> +		// compatible = "jedec,spi-nor";

No dead code in Linux kernel. Commented out stuff sometimes is okay if
it is explained with comments.

> +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <150000000>;
> +		// spi-rx-bus-width = <1>;
> +		// spi-tx-bus-width = <1>;
> +		// m25p,default-addr-width = <4>;
> +		// spi-max-frequency = <160000000>;

No dead code.

> +		m25p,fast-read;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			
> +			u-boot@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x50000>;
> +				read-only;
> +			};
> +			u-boot-env@50000 {
> +				label = "u-boot-env";
> +				reg = <0x50000 0x20000>;
> +			};
> +			kernel@70000 {
> +				label = "kernel";
> +				reg = <0x70000 0x700000>;
> +			};
> +			rootfs@770000 {
> +				label = "rootfs";
> +				reg = <0x800000 0x300000>;
> +				read-only;
> +			};
> +			extra@b00000 {
> +				label = "extra";
> +				reg = <0xb00000 0x1500000>;
> +			};
> +		};
> +	};
> +};
> +
> +&dual_timer0 {
> +	status = "okay";
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> +	status = "okay";
> +};
> +
> +&pmx0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_pmx_func>;
> +	uart0_pmx_func: uart0_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};
> +
> +	sfc_pmx_func: sfc_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x1
> +			0x00c8 0x1
> +			0x00cc 0x1
> +			0x00d0 0x1
> +			0x00d4 0x1
> +		>;
> +	};
> +};
> +
> +&pmx1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_cfg_func>;
> +	uart0_cfg_func: uart0_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};

Blank line

> +	sfc_cfg_func: sfc_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x58
> +			0x00c8 0x28
> +			0x00cc 0x38
> +			0x00d0 0x38
> +			0x00d4 0x38
> +		>;
> +	};
> +};
> +
> +/* &gmac0 { */
> +/* 	#address-cells = <1>; */
> +/* 	#size-cells = <0>; */
> +/* 	phy-handle = <&phy3>; */
> +/* 	phy-mode = "rgmii"; */
> +/* 	mac-address = [00 00 00 00 00 00]; */
> +/* 	status = "okay"; */
> +/**/
> +/* 	phy3: ethernet-phy@3 { */
> +/* 		reg = <3>; */
> +/* 	}; */
> +/* }; */


? What's this?

> diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> new file mode 100644
> index 000000000000..53993a32fd5d
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a.dtsi
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include <dt-bindings/clock/hi3521a-clock.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +/dts-v1/;

Blank line.

> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0>;
> +			clock-frequency = <1100000000>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv7-timer";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_PPI 13 0xf08>,
> +			     <GIC_PPI 14 0xf08>,
> +			     <GIC_PPI 11 0xf08>,
> +			     <GIC_PPI 10 0xf08>;

Use macros for these interrupts.

> +		clock-frequency = <24000000>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	gic: interrupt-controller@10301000 {
> +		compatible = "arm,pl390";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;

Put reg just after compatible. This applies to each other node as well
(if it comes with reg).

> +	};
> +
> +	xtal24m: xtal24m {

Generic node names, so one of: "clock-0" "clock-xtal24m"

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	clk_3m: clk_3m {

No underscores in node names, generic node name (see above).

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <3000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		interrupt-parent = <&gic>;
> +		ranges;
> +
> +		hi_sfc: spi@10000000 {
> +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> +			reg-names = "control", "memory";
> +			clocks = <&crg HI3521A_FMC_CLK>;
> +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		/* usb0: usb@10030000 { */
> +		/* 		compatible = "generic-ohci"; */
> +		/* 		reg = <0x10030000 0x1000>; */
> +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> +		/* 		clocks = <&crg > */
> +		/* 	}; */

No dead code please.

> +		dmac: dma-controller@10060000 {
> +			compatible = "arm,pl080", "arm,primecell";
> +			arm,primecell-periphid = <0x00041080>;
> +			reg = <0x10060000 0x1000>;
> +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_DMAC_CLK>;
> +			clock-names = "apb_pclk";
> +			lli-bus-interface-ahb1;
> +			lli-bus-interface-ahb2;
> +			mem-bus-interface-ahb1;
> +			mem-bus-interface-ahb2;
> +			memcpy-burst-size = <256>;
> +			memcpy-bus-width = <32>;
> +			#dma-cells = <2>;
> +			status = "okay";

No need for status okay, it's by default.

> +		};
> +
> +		gmac0: ethernet@100a0000 {
> +			compatible = "hisilicon,hisi-gmac-v1";
> +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> +			clock-names = "mac_core", "mac_ifc";
> +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */

No dead code.

> +			status = "disabled";
> +		};
> +
> +		dual_timer0: timer@12000000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;

A bit weird interrupts... the same?

> +			reg = <0x12000000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> +				 <&sysctrl HI3521A_TIMER1_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer1: timer@12010000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12010000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> +				 <&sysctrl HI3521A_TIMER3_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer2: timer@12020000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12020000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> +				 <&sysctrl HI3521A_TIMER5_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer3: timer@12030000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12030000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> +				 <&sysctrl HI3521A_TIMER7_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		crg: clock-reset-controller@12040000 {
> +			compatible = "hisilicon,hi3521a-crg";

Undocumented compatible.  Didn't checkpatch warn about it?

> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +			reg = <0x12040000 0x1000>;
> +		};
> +
> +		sysctrl: system-controller@12050000 {
> +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";

Same.

> +			reg = <0x12050000 0x1000>;
> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +		};
> +
> +		reboot {
> +			compatible = "syscon-reboot";
> +			regmap = <&sysctrl>;
> +			offset = <0x4>;
> +			mask = <0xdeadbeef>;
> +		};
> +
> +		wdt0: watchdog@12070000 {
> +			compatible = "arm,sp805", "arm,primecell";
> +			arm,primecell-periphid = <0x00141805>;
> +			reg = <0x12070000 0x1000>;
> +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_FIXED_3M>;
> +			clock-names = "apb_pclk";
> +		};
> +
> +		uart0: serial@12080000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12080000 0x1000>;
> +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> +			clock-names = "uartclk", "apb_pclk";
> +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart1: serial@12090000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12090000 0x1000>;
> +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART1_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart2: serial@120a0000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x120a0000 0x1000>;
> +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART2_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		spi_bus0: spi@120d0000 {
> +			compatible = "arm,pl022", "arm,primecell";
> +			reg = <0x120d0000 0x1000>;
> +			arm,primecell-periphid = <0x00041022>;
> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_SPI0_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> +			dma-names = "rx", "tx";
> +			num-cs = <2>;
> +			status = "disabled";
> +		};
> +
> +		pmx0: pinmux@120f0000 {
> +			compatible = "pinctrl-single";
> +			reg = <0x120f0000 0x188>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			#gpio-range-cells = <3>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +			pinctrl-single,function-mask = <7>;
> +			/* pin base, nr pins & gpio function */
> +			pinctrl-single,gpio-range = <
> +				&range 58 4 0
> +			>;
> +
> +			range: gpio-range {
> +				#pinctrl-single,gpio-range-cells = <3>;
> +			};
> +		};
> +
> +		pmx1: pinmux@120f0800 {
> +			compatible = "pinconf-single";
> +			reg = <0x120f0800 0x1d4>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +		};
> +
> +		gpio0: gpio@12150000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12150000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio1: gpio@12160000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12160000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio2: gpio@12170000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12170000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio3: gpio@12180000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12180000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio4: gpio@12190000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12190000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio5: gpio@121a0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121a0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio6: gpio@121b0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121b0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;

Same interrupts as for other nodes? Is it correct?

> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio7: gpio@121c0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121c0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +_MULTI_V7


Best regards,
Krzysztof

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> security systems and ipcameras. The arm core is a Cortex A7.
> 
> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> marketed under the name Samsung SDR-B74301N.

Thank you for your patch. There is something to discuss/improve.

> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  arch/arm/boot/dts/Makefile              |   2 +
>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++

DTSes go to separate patches.

>  arch/arm/mach-hisi/Kconfig              |   9 +
>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>  	gemini-ssi1328.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..b24fcf2ca85e
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include "hi3521a.dtsi"
> +
> +/ {
> +	model = "RaySharp RS-DM-290E DVR Board";
> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";

Please run checkpatch and fix the warnings.

> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	memory@800000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0xf00000>;
> +	};
> +};
> +
> +&hi_sfc {
> +	status = "okay";
> +	spi-nor@0 {

Looks like wrong node name. You need to run "dtbs_check" and fix
warnings related to your DTS.

> +		// compatible = "jedec,spi-nor";

No dead code in Linux kernel. Commented out stuff sometimes is okay if
it is explained with comments.

> +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <150000000>;
> +		// spi-rx-bus-width = <1>;
> +		// spi-tx-bus-width = <1>;
> +		// m25p,default-addr-width = <4>;
> +		// spi-max-frequency = <160000000>;

No dead code.

> +		m25p,fast-read;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			
> +			u-boot@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x50000>;
> +				read-only;
> +			};
> +			u-boot-env@50000 {
> +				label = "u-boot-env";
> +				reg = <0x50000 0x20000>;
> +			};
> +			kernel@70000 {
> +				label = "kernel";
> +				reg = <0x70000 0x700000>;
> +			};
> +			rootfs@770000 {
> +				label = "rootfs";
> +				reg = <0x800000 0x300000>;
> +				read-only;
> +			};
> +			extra@b00000 {
> +				label = "extra";
> +				reg = <0xb00000 0x1500000>;
> +			};
> +		};
> +	};
> +};
> +
> +&dual_timer0 {
> +	status = "okay";
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> +	status = "okay";
> +};
> +
> +&pmx0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_pmx_func>;
> +	uart0_pmx_func: uart0_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};
> +
> +	sfc_pmx_func: sfc_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x1
> +			0x00c8 0x1
> +			0x00cc 0x1
> +			0x00d0 0x1
> +			0x00d4 0x1
> +		>;
> +	};
> +};
> +
> +&pmx1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_cfg_func>;
> +	uart0_cfg_func: uart0_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};

Blank line

> +	sfc_cfg_func: sfc_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x58
> +			0x00c8 0x28
> +			0x00cc 0x38
> +			0x00d0 0x38
> +			0x00d4 0x38
> +		>;
> +	};
> +};
> +
> +/* &gmac0 { */
> +/* 	#address-cells = <1>; */
> +/* 	#size-cells = <0>; */
> +/* 	phy-handle = <&phy3>; */
> +/* 	phy-mode = "rgmii"; */
> +/* 	mac-address = [00 00 00 00 00 00]; */
> +/* 	status = "okay"; */
> +/**/
> +/* 	phy3: ethernet-phy@3 { */
> +/* 		reg = <3>; */
> +/* 	}; */
> +/* }; */


? What's this?

> diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> new file mode 100644
> index 000000000000..53993a32fd5d
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a.dtsi
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include <dt-bindings/clock/hi3521a-clock.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +/dts-v1/;

Blank line.

> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0>;
> +			clock-frequency = <1100000000>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv7-timer";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_PPI 13 0xf08>,
> +			     <GIC_PPI 14 0xf08>,
> +			     <GIC_PPI 11 0xf08>,
> +			     <GIC_PPI 10 0xf08>;

Use macros for these interrupts.

> +		clock-frequency = <24000000>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	gic: interrupt-controller@10301000 {
> +		compatible = "arm,pl390";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;

Put reg just after compatible. This applies to each other node as well
(if it comes with reg).

> +	};
> +
> +	xtal24m: xtal24m {

Generic node names, so one of: "clock-0" "clock-xtal24m"

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	clk_3m: clk_3m {

No underscores in node names, generic node name (see above).

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <3000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		interrupt-parent = <&gic>;
> +		ranges;
> +
> +		hi_sfc: spi@10000000 {
> +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> +			reg-names = "control", "memory";
> +			clocks = <&crg HI3521A_FMC_CLK>;
> +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		/* usb0: usb@10030000 { */
> +		/* 		compatible = "generic-ohci"; */
> +		/* 		reg = <0x10030000 0x1000>; */
> +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> +		/* 		clocks = <&crg > */
> +		/* 	}; */

No dead code please.

> +		dmac: dma-controller@10060000 {
> +			compatible = "arm,pl080", "arm,primecell";
> +			arm,primecell-periphid = <0x00041080>;
> +			reg = <0x10060000 0x1000>;
> +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_DMAC_CLK>;
> +			clock-names = "apb_pclk";
> +			lli-bus-interface-ahb1;
> +			lli-bus-interface-ahb2;
> +			mem-bus-interface-ahb1;
> +			mem-bus-interface-ahb2;
> +			memcpy-burst-size = <256>;
> +			memcpy-bus-width = <32>;
> +			#dma-cells = <2>;
> +			status = "okay";

No need for status okay, it's by default.

> +		};
> +
> +		gmac0: ethernet@100a0000 {
> +			compatible = "hisilicon,hisi-gmac-v1";
> +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> +			clock-names = "mac_core", "mac_ifc";
> +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */

No dead code.

> +			status = "disabled";
> +		};
> +
> +		dual_timer0: timer@12000000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;

A bit weird interrupts... the same?

> +			reg = <0x12000000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> +				 <&sysctrl HI3521A_TIMER1_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer1: timer@12010000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12010000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> +				 <&sysctrl HI3521A_TIMER3_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer2: timer@12020000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12020000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> +				 <&sysctrl HI3521A_TIMER5_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer3: timer@12030000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12030000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> +				 <&sysctrl HI3521A_TIMER7_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		crg: clock-reset-controller@12040000 {
> +			compatible = "hisilicon,hi3521a-crg";

Undocumented compatible.  Didn't checkpatch warn about it?

> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +			reg = <0x12040000 0x1000>;
> +		};
> +
> +		sysctrl: system-controller@12050000 {
> +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";

Same.

> +			reg = <0x12050000 0x1000>;
> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +		};
> +
> +		reboot {
> +			compatible = "syscon-reboot";
> +			regmap = <&sysctrl>;
> +			offset = <0x4>;
> +			mask = <0xdeadbeef>;
> +		};
> +
> +		wdt0: watchdog@12070000 {
> +			compatible = "arm,sp805", "arm,primecell";
> +			arm,primecell-periphid = <0x00141805>;
> +			reg = <0x12070000 0x1000>;
> +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_FIXED_3M>;
> +			clock-names = "apb_pclk";
> +		};
> +
> +		uart0: serial@12080000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12080000 0x1000>;
> +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> +			clock-names = "uartclk", "apb_pclk";
> +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart1: serial@12090000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12090000 0x1000>;
> +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART1_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart2: serial@120a0000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x120a0000 0x1000>;
> +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART2_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		spi_bus0: spi@120d0000 {
> +			compatible = "arm,pl022", "arm,primecell";
> +			reg = <0x120d0000 0x1000>;
> +			arm,primecell-periphid = <0x00041022>;
> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_SPI0_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> +			dma-names = "rx", "tx";
> +			num-cs = <2>;
> +			status = "disabled";
> +		};
> +
> +		pmx0: pinmux@120f0000 {
> +			compatible = "pinctrl-single";
> +			reg = <0x120f0000 0x188>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			#gpio-range-cells = <3>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +			pinctrl-single,function-mask = <7>;
> +			/* pin base, nr pins & gpio function */
> +			pinctrl-single,gpio-range = <
> +				&range 58 4 0
> +			>;
> +
> +			range: gpio-range {
> +				#pinctrl-single,gpio-range-cells = <3>;
> +			};
> +		};
> +
> +		pmx1: pinmux@120f0800 {
> +			compatible = "pinconf-single";
> +			reg = <0x120f0800 0x1d4>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +		};
> +
> +		gpio0: gpio@12150000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12150000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio1: gpio@12160000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12160000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio2: gpio@12170000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12170000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio3: gpio@12180000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12180000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio4: gpio@12190000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12190000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio5: gpio@121a0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121a0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio6: gpio@121b0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121b0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;

Same interrupts as for other nodes? Is it correct?

> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio7: gpio@121c0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121c0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +_MULTI_V7


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> security systems and ipcameras. The arm core is a Cortex A7.
> 
> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> marketed under the name Samsung SDR-B74301N.

Thank you for your patch. There is something to discuss/improve.

> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  arch/arm/boot/dts/Makefile              |   2 +
>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++

DTSes go to separate patches.

>  arch/arm/mach-hisi/Kconfig              |   9 +
>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>  	gemini-ssi1328.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..b24fcf2ca85e
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include "hi3521a.dtsi"
> +
> +/ {
> +	model = "RaySharp RS-DM-290E DVR Board";
> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";

Please run checkpatch and fix the warnings.

> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	memory@800000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0xf00000>;
> +	};
> +};
> +
> +&hi_sfc {
> +	status = "okay";
> +	spi-nor@0 {

Looks like wrong node name. You need to run "dtbs_check" and fix
warnings related to your DTS.

> +		// compatible = "jedec,spi-nor";

No dead code in Linux kernel. Commented out stuff sometimes is okay if
it is explained with comments.

> +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <150000000>;
> +		// spi-rx-bus-width = <1>;
> +		// spi-tx-bus-width = <1>;
> +		// m25p,default-addr-width = <4>;
> +		// spi-max-frequency = <160000000>;

No dead code.

> +		m25p,fast-read;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			
> +			u-boot@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x50000>;
> +				read-only;
> +			};
> +			u-boot-env@50000 {
> +				label = "u-boot-env";
> +				reg = <0x50000 0x20000>;
> +			};
> +			kernel@70000 {
> +				label = "kernel";
> +				reg = <0x70000 0x700000>;
> +			};
> +			rootfs@770000 {
> +				label = "rootfs";
> +				reg = <0x800000 0x300000>;
> +				read-only;
> +			};
> +			extra@b00000 {
> +				label = "extra";
> +				reg = <0xb00000 0x1500000>;
> +			};
> +		};
> +	};
> +};
> +
> +&dual_timer0 {
> +	status = "okay";
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> +	status = "okay";
> +};
> +
> +&pmx0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_pmx_func>;
> +	uart0_pmx_func: uart0_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};
> +
> +	sfc_pmx_func: sfc_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x1
> +			0x00c8 0x1
> +			0x00cc 0x1
> +			0x00d0 0x1
> +			0x00d4 0x1
> +		>;
> +	};
> +};
> +
> +&pmx1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_cfg_func>;
> +	uart0_cfg_func: uart0_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};

Blank line

> +	sfc_cfg_func: sfc_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x58
> +			0x00c8 0x28
> +			0x00cc 0x38
> +			0x00d0 0x38
> +			0x00d4 0x38
> +		>;
> +	};
> +};
> +
> +/* &gmac0 { */
> +/* 	#address-cells = <1>; */
> +/* 	#size-cells = <0>; */
> +/* 	phy-handle = <&phy3>; */
> +/* 	phy-mode = "rgmii"; */
> +/* 	mac-address = [00 00 00 00 00 00]; */
> +/* 	status = "okay"; */
> +/**/
> +/* 	phy3: ethernet-phy@3 { */
> +/* 		reg = <3>; */
> +/* 	}; */
> +/* }; */


? What's this?

> diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> new file mode 100644
> index 000000000000..53993a32fd5d
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a.dtsi
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include <dt-bindings/clock/hi3521a-clock.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +/dts-v1/;

Blank line.

> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0>;
> +			clock-frequency = <1100000000>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv7-timer";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_PPI 13 0xf08>,
> +			     <GIC_PPI 14 0xf08>,
> +			     <GIC_PPI 11 0xf08>,
> +			     <GIC_PPI 10 0xf08>;

Use macros for these interrupts.

> +		clock-frequency = <24000000>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	gic: interrupt-controller@10301000 {
> +		compatible = "arm,pl390";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;

Put reg just after compatible. This applies to each other node as well
(if it comes with reg).

> +	};
> +
> +	xtal24m: xtal24m {

Generic node names, so one of: "clock-0" "clock-xtal24m"

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	clk_3m: clk_3m {

No underscores in node names, generic node name (see above).

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <3000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		interrupt-parent = <&gic>;
> +		ranges;
> +
> +		hi_sfc: spi@10000000 {
> +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> +			reg-names = "control", "memory";
> +			clocks = <&crg HI3521A_FMC_CLK>;
> +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		/* usb0: usb@10030000 { */
> +		/* 		compatible = "generic-ohci"; */
> +		/* 		reg = <0x10030000 0x1000>; */
> +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> +		/* 		clocks = <&crg > */
> +		/* 	}; */

No dead code please.

> +		dmac: dma-controller@10060000 {
> +			compatible = "arm,pl080", "arm,primecell";
> +			arm,primecell-periphid = <0x00041080>;
> +			reg = <0x10060000 0x1000>;
> +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_DMAC_CLK>;
> +			clock-names = "apb_pclk";
> +			lli-bus-interface-ahb1;
> +			lli-bus-interface-ahb2;
> +			mem-bus-interface-ahb1;
> +			mem-bus-interface-ahb2;
> +			memcpy-burst-size = <256>;
> +			memcpy-bus-width = <32>;
> +			#dma-cells = <2>;
> +			status = "okay";

No need for status okay, it's by default.

> +		};
> +
> +		gmac0: ethernet@100a0000 {
> +			compatible = "hisilicon,hisi-gmac-v1";
> +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> +			clock-names = "mac_core", "mac_ifc";
> +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */

No dead code.

> +			status = "disabled";
> +		};
> +
> +		dual_timer0: timer@12000000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;

A bit weird interrupts... the same?

> +			reg = <0x12000000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> +				 <&sysctrl HI3521A_TIMER1_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer1: timer@12010000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12010000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> +				 <&sysctrl HI3521A_TIMER3_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer2: timer@12020000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12020000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> +				 <&sysctrl HI3521A_TIMER5_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer3: timer@12030000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12030000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> +				 <&sysctrl HI3521A_TIMER7_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		crg: clock-reset-controller@12040000 {
> +			compatible = "hisilicon,hi3521a-crg";

Undocumented compatible.  Didn't checkpatch warn about it?

> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +			reg = <0x12040000 0x1000>;
> +		};
> +
> +		sysctrl: system-controller@12050000 {
> +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";

Same.

> +			reg = <0x12050000 0x1000>;
> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +		};
> +
> +		reboot {
> +			compatible = "syscon-reboot";
> +			regmap = <&sysctrl>;
> +			offset = <0x4>;
> +			mask = <0xdeadbeef>;
> +		};
> +
> +		wdt0: watchdog@12070000 {
> +			compatible = "arm,sp805", "arm,primecell";
> +			arm,primecell-periphid = <0x00141805>;
> +			reg = <0x12070000 0x1000>;
> +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_FIXED_3M>;
> +			clock-names = "apb_pclk";
> +		};
> +
> +		uart0: serial@12080000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12080000 0x1000>;
> +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> +			clock-names = "uartclk", "apb_pclk";
> +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart1: serial@12090000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12090000 0x1000>;
> +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART1_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart2: serial@120a0000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x120a0000 0x1000>;
> +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART2_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		spi_bus0: spi@120d0000 {
> +			compatible = "arm,pl022", "arm,primecell";
> +			reg = <0x120d0000 0x1000>;
> +			arm,primecell-periphid = <0x00041022>;
> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_SPI0_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> +			dma-names = "rx", "tx";
> +			num-cs = <2>;
> +			status = "disabled";
> +		};
> +
> +		pmx0: pinmux@120f0000 {
> +			compatible = "pinctrl-single";
> +			reg = <0x120f0000 0x188>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			#gpio-range-cells = <3>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +			pinctrl-single,function-mask = <7>;
> +			/* pin base, nr pins & gpio function */
> +			pinctrl-single,gpio-range = <
> +				&range 58 4 0
> +			>;
> +
> +			range: gpio-range {
> +				#pinctrl-single,gpio-range-cells = <3>;
> +			};
> +		};
> +
> +		pmx1: pinmux@120f0800 {
> +			compatible = "pinconf-single";
> +			reg = <0x120f0800 0x1d4>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +		};
> +
> +		gpio0: gpio@12150000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12150000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio1: gpio@12160000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12160000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio2: gpio@12170000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12170000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio3: gpio@12180000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12180000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio4: gpio@12190000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12190000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio5: gpio@121a0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121a0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio6: gpio@121b0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121b0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;

Same interrupts as for other nodes? Is it correct?

> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio7: gpio@121c0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121c0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +_MULTI_V7


Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> > hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> > security systems and ipcameras. The arm core is a Cortex A7.
> > 
> > Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> > marketed under the name Samsung SDR-B74301N.
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  arch/arm/boot/dts/Makefile              |   2 +
> >  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
> >  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
> 
> DTSes go to separate patches.
Do you mean dts and dtsi need to be separate patches?
> 
> >  arch/arm/mach-hisi/Kconfig              |   9 +
> >  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
> >  	gemini-ssi1328.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..b24fcf2ca85e
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include "hi3521a.dtsi"
> > +
> > +/ {
> > +	model = "RaySharp RS-DM-290E DVR Board";
> > +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
> 
> Please run checkpatch and fix the warnings.
> 
sunova. I could have sworn I had my editor setup right for whitespace
and such.
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	memory@800000000 {
> > +		device_type = "memory";
> > +		reg = <0x80000000 0xf00000>;
> > +	};
> > +};
> > +
> > +&hi_sfc {
> > +	status = "okay";
> > +	spi-nor@0 {
> 
> Looks like wrong node name. You need to run "dtbs_check" and fix
> warnings related to your DTS.
> 
Ah, that's a good thing to know. dt-doc-validate isn't found, doesn't
seem packaged for gentoo either. I'll see about that asap.
> > +		// compatible = "jedec,spi-nor";
> 
> No dead code in Linux kernel. Commented out stuff sometimes is okay if
> it is explained with comments.
> 
As mentioned in the title, just an RFC, not final.
> > +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> > +		reg = <0>;
> > +		spi-max-frequency = <150000000>;
> > +		// spi-rx-bus-width = <1>;
> > +		// spi-tx-bus-width = <1>;
> > +		// m25p,default-addr-width = <4>;
> > +		// spi-max-frequency = <160000000>;
> 
> No dead code.
> 
> > +		m25p,fast-read;
> > +
> > +		partitions {
> > +			compatible = "fixed-partitions";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			
> > +			u-boot@0 {
> > +				label = "u-boot";
> > +				reg = <0x0 0x50000>;
> > +				read-only;
> > +			};
> > +			u-boot-env@50000 {
> > +				label = "u-boot-env";
> > +				reg = <0x50000 0x20000>;
> > +			};
> > +			kernel@70000 {
> > +				label = "kernel";
> > +				reg = <0x70000 0x700000>;
> > +			};
> > +			rootfs@770000 {
> > +				label = "rootfs";
> > +				reg = <0x800000 0x300000>;
> > +				read-only;
> > +			};
> > +			extra@b00000 {
> > +				label = "extra";
> > +				reg = <0xb00000 0x1500000>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&dual_timer0 {
> > +	status = "okay";
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> > +	status = "okay";
> > +};
> > +
> > +&pmx0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_pmx_func>;
> > +	uart0_pmx_func: uart0_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> > +
> > +	sfc_pmx_func: sfc_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x1
> > +			0x00c8 0x1
> > +			0x00cc 0x1
> > +			0x00d0 0x1
> > +			0x00d4 0x1
> > +		>;
> > +	};
> > +};
> > +
> > +&pmx1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_cfg_func>;
> > +	uart0_cfg_func: uart0_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> 
> Blank line
> 
> > +	sfc_cfg_func: sfc_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x58
> > +			0x00c8 0x28
> > +			0x00cc 0x38
> > +			0x00d0 0x38
> > +			0x00d4 0x38
> > +		>;
> > +	};
> > +};
> > +
> > +/* &gmac0 { */
> > +/* 	#address-cells = <1>; */
> > +/* 	#size-cells = <0>; */
> > +/* 	phy-handle = <&phy3>; */
> > +/* 	phy-mode = "rgmii"; */
> > +/* 	mac-address = [00 00 00 00 00 00]; */
> > +/* 	status = "okay"; */
> > +/**/
> > +/* 	phy3: ethernet-phy@3 { */
> > +/* 		reg = <3>; */
> > +/* 	}; */
> > +/* }; */
> 
> 
> ? What's this?
> 
ethernet port. its giving me a bit of trouble at the moment, but it does
work and has a phy at 3.
> > diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> > new file mode 100644
> > index 000000000000..53993a32fd5d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a.dtsi
> > @@ -0,0 +1,423 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include <dt-bindings/clock/hi3521a-clock.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +/dts-v1/;
> 
> Blank line.
> 
> > +/ {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu0: cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a7";
> > +			reg = <0>;
> > +			clock-frequency = <1100000000>;
> > +		};
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv7-timer";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_PPI 13 0xf08>,
> > +			     <GIC_PPI 14 0xf08>,
> > +			     <GIC_PPI 11 0xf08>,
> > +			     <GIC_PPI 10 0xf08>;
> 
> Use macros for these interrupts.
> 
moot point, turns out enabling this timer is causing the system to lock
up. Something's weird there.
> > +		clock-frequency = <24000000>;
> > +	};
> > +
> > +	pmu {
> > +		compatible = "arm,cortex-a7-pmu";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > +	};
> > +
> > +	gic: interrupt-controller@10301000 {
> > +		compatible = "arm,pl390";
> > +		#interrupt-cells = <3>;
> > +		interrupt-controller;
> > +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
> 
> Put reg just after compatible. This applies to each other node as well
> (if it comes with reg).
> 
Ah gotcha.
> > +	};
> > +
> > +	xtal24m: xtal24m {
> 
> Generic node names, so one of: "clock-0" "clock-xtal24m"
> 
Will do.
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> 
> This does not look like property of the SoC, so should be defined by boards.
> 
SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
present regardless.
> > +	};
> > +
> > +	clk_3m: clk_3m {
> 
> No underscores in node names, generic node name (see above).
> 
early debugging clock, will be removed.
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <3000000>;
> 
> This does not look like property of the SoC, so should be defined by boards.
> 
> > +	};
> > +
> > +	soc {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "simple-bus";
> > +		interrupt-parent = <&gic>;
> > +		ranges;
> > +
> > +		hi_sfc: spi@10000000 {
> > +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> > +			reg-names = "control", "memory";
> > +			clocks = <&crg HI3521A_FMC_CLK>;
> > +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > +			status = "disabled";
> > +		};
> > +
> > +		/* usb0: usb@10030000 { */
> > +		/* 		compatible = "generic-ohci"; */
> > +		/* 		reg = <0x10030000 0x1000>; */
> > +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> > +		/* 		clocks = <&crg > */
> > +		/* 	}; */
> 
> No dead code please.
> 
> > +		dmac: dma-controller@10060000 {
> > +			compatible = "arm,pl080", "arm,primecell";
> > +			arm,primecell-periphid = <0x00041080>;
> > +			reg = <0x10060000 0x1000>;
> > +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_DMAC_CLK>;
> > +			clock-names = "apb_pclk";
> > +			lli-bus-interface-ahb1;
> > +			lli-bus-interface-ahb2;
> > +			mem-bus-interface-ahb1;
> > +			mem-bus-interface-ahb2;
> > +			memcpy-burst-size = <256>;
> > +			memcpy-bus-width = <32>;
> > +			#dma-cells = <2>;
> > +			status = "okay";
> 
> No need for status okay, it's by default.
> 
ah, good to know.
> > +		};
> > +
> > +		gmac0: ethernet@100a0000 {
> > +			compatible = "hisilicon,hisi-gmac-v1";
> > +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> > +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> > +			clock-names = "mac_core", "mac_ifc";
> > +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> > +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> > +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */
> 
> No dead code.
> 
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer0: timer@12000000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> 
> A bit weird interrupts... the same?
> 
Yes, though I am aware that some sp804 timers do have a separate
interrupts per pair.
> > +			reg = <0x12000000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> > +				 <&sysctrl HI3521A_TIMER1_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer1: timer@12010000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12010000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> > +				 <&sysctrl HI3521A_TIMER3_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer2: timer@12020000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12020000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> > +				 <&sysctrl HI3521A_TIMER5_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer3: timer@12030000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12030000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> > +				 <&sysctrl HI3521A_TIMER7_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		crg: clock-reset-controller@12040000 {
> > +			compatible = "hisilicon,hi3521a-crg";
> 
> Undocumented compatible.  Didn't checkpatch warn about it?
> 
As mentioned, RFC. I'm more concerned with getting it to work consistently
at the moment.
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +			reg = <0x12040000 0x1000>;
> > +		};
> > +
> > +		sysctrl: system-controller@12050000 {
> > +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";
> 
> Same.
> 
> > +			reg = <0x12050000 0x1000>;
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +		};
> > +
> > +		reboot {
> > +			compatible = "syscon-reboot";
> > +			regmap = <&sysctrl>;
> > +			offset = <0x4>;
> > +			mask = <0xdeadbeef>;
> > +		};
> > +
> > +		wdt0: watchdog@12070000 {
> > +			compatible = "arm,sp805", "arm,primecell";
> > +			arm,primecell-periphid = <0x00141805>;
> > +			reg = <0x12070000 0x1000>;
> > +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_FIXED_3M>;
> > +			clock-names = "apb_pclk";
> > +		};
> > +
> > +		uart0: serial@12080000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12080000 0x1000>;
> > +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> > +			clock-names = "uartclk", "apb_pclk";
> > +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart1: serial@12090000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12090000 0x1000>;
> > +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART1_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart2: serial@120a0000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x120a0000 0x1000>;
> > +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART2_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		spi_bus0: spi@120d0000 {
> > +			compatible = "arm,pl022", "arm,primecell";
> > +			reg = <0x120d0000 0x1000>;
> > +			arm,primecell-periphid = <0x00041022>;
> > +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_SPI0_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> > +			dma-names = "rx", "tx";
> > +			num-cs = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pmx0: pinmux@120f0000 {
> > +			compatible = "pinctrl-single";
> > +			reg = <0x120f0000 0x188>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			#gpio-range-cells = <3>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +			pinctrl-single,function-mask = <7>;
> > +			/* pin base, nr pins & gpio function */
> > +			pinctrl-single,gpio-range = <
> > +				&range 58 4 0
> > +			>;
> > +
> > +			range: gpio-range {
> > +				#pinctrl-single,gpio-range-cells = <3>;
> > +			};
> > +		};
> > +
> > +		pmx1: pinmux@120f0800 {
> > +			compatible = "pinconf-single";
> > +			reg = <0x120f0800 0x1d4>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +		};
> > +
> > +		gpio0: gpio@12150000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12150000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio1: gpio@12160000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12160000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio2: gpio@12170000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12170000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio3: gpio@12180000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12180000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio4: gpio@12190000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12190000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio5: gpio@121a0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121a0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio6: gpio@121b0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121b0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> 
> Same interrupts as for other nodes? Is it correct?
> 
Yep, there are three interrupts for the gpios and they're shared among a
few each.
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio7: gpio@121c0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121c0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +_MULTI_V7
> 
> 
> Best regards,
> Krzysztof

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> > hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> > security systems and ipcameras. The arm core is a Cortex A7.
> > 
> > Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> > marketed under the name Samsung SDR-B74301N.
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  arch/arm/boot/dts/Makefile              |   2 +
> >  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
> >  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
> 
> DTSes go to separate patches.
Do you mean dts and dtsi need to be separate patches?
> 
> >  arch/arm/mach-hisi/Kconfig              |   9 +
> >  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
> >  	gemini-ssi1328.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..b24fcf2ca85e
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include "hi3521a.dtsi"
> > +
> > +/ {
> > +	model = "RaySharp RS-DM-290E DVR Board";
> > +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
> 
> Please run checkpatch and fix the warnings.
> 
sunova. I could have sworn I had my editor setup right for whitespace
and such.
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	memory@800000000 {
> > +		device_type = "memory";
> > +		reg = <0x80000000 0xf00000>;
> > +	};
> > +};
> > +
> > +&hi_sfc {
> > +	status = "okay";
> > +	spi-nor@0 {
> 
> Looks like wrong node name. You need to run "dtbs_check" and fix
> warnings related to your DTS.
> 
Ah, that's a good thing to know. dt-doc-validate isn't found, doesn't
seem packaged for gentoo either. I'll see about that asap.
> > +		// compatible = "jedec,spi-nor";
> 
> No dead code in Linux kernel. Commented out stuff sometimes is okay if
> it is explained with comments.
> 
As mentioned in the title, just an RFC, not final.
> > +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> > +		reg = <0>;
> > +		spi-max-frequency = <150000000>;
> > +		// spi-rx-bus-width = <1>;
> > +		// spi-tx-bus-width = <1>;
> > +		// m25p,default-addr-width = <4>;
> > +		// spi-max-frequency = <160000000>;
> 
> No dead code.
> 
> > +		m25p,fast-read;
> > +
> > +		partitions {
> > +			compatible = "fixed-partitions";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			
> > +			u-boot@0 {
> > +				label = "u-boot";
> > +				reg = <0x0 0x50000>;
> > +				read-only;
> > +			};
> > +			u-boot-env@50000 {
> > +				label = "u-boot-env";
> > +				reg = <0x50000 0x20000>;
> > +			};
> > +			kernel@70000 {
> > +				label = "kernel";
> > +				reg = <0x70000 0x700000>;
> > +			};
> > +			rootfs@770000 {
> > +				label = "rootfs";
> > +				reg = <0x800000 0x300000>;
> > +				read-only;
> > +			};
> > +			extra@b00000 {
> > +				label = "extra";
> > +				reg = <0xb00000 0x1500000>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&dual_timer0 {
> > +	status = "okay";
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> > +	status = "okay";
> > +};
> > +
> > +&pmx0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_pmx_func>;
> > +	uart0_pmx_func: uart0_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> > +
> > +	sfc_pmx_func: sfc_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x1
> > +			0x00c8 0x1
> > +			0x00cc 0x1
> > +			0x00d0 0x1
> > +			0x00d4 0x1
> > +		>;
> > +	};
> > +};
> > +
> > +&pmx1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_cfg_func>;
> > +	uart0_cfg_func: uart0_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> 
> Blank line
> 
> > +	sfc_cfg_func: sfc_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x58
> > +			0x00c8 0x28
> > +			0x00cc 0x38
> > +			0x00d0 0x38
> > +			0x00d4 0x38
> > +		>;
> > +	};
> > +};
> > +
> > +/* &gmac0 { */
> > +/* 	#address-cells = <1>; */
> > +/* 	#size-cells = <0>; */
> > +/* 	phy-handle = <&phy3>; */
> > +/* 	phy-mode = "rgmii"; */
> > +/* 	mac-address = [00 00 00 00 00 00]; */
> > +/* 	status = "okay"; */
> > +/**/
> > +/* 	phy3: ethernet-phy@3 { */
> > +/* 		reg = <3>; */
> > +/* 	}; */
> > +/* }; */
> 
> 
> ? What's this?
> 
ethernet port. its giving me a bit of trouble at the moment, but it does
work and has a phy at 3.
> > diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> > new file mode 100644
> > index 000000000000..53993a32fd5d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a.dtsi
> > @@ -0,0 +1,423 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include <dt-bindings/clock/hi3521a-clock.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +/dts-v1/;
> 
> Blank line.
> 
> > +/ {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu0: cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a7";
> > +			reg = <0>;
> > +			clock-frequency = <1100000000>;
> > +		};
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv7-timer";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_PPI 13 0xf08>,
> > +			     <GIC_PPI 14 0xf08>,
> > +			     <GIC_PPI 11 0xf08>,
> > +			     <GIC_PPI 10 0xf08>;
> 
> Use macros for these interrupts.
> 
moot point, turns out enabling this timer is causing the system to lock
up. Something's weird there.
> > +		clock-frequency = <24000000>;
> > +	};
> > +
> > +	pmu {
> > +		compatible = "arm,cortex-a7-pmu";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > +	};
> > +
> > +	gic: interrupt-controller@10301000 {
> > +		compatible = "arm,pl390";
> > +		#interrupt-cells = <3>;
> > +		interrupt-controller;
> > +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
> 
> Put reg just after compatible. This applies to each other node as well
> (if it comes with reg).
> 
Ah gotcha.
> > +	};
> > +
> > +	xtal24m: xtal24m {
> 
> Generic node names, so one of: "clock-0" "clock-xtal24m"
> 
Will do.
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> 
> This does not look like property of the SoC, so should be defined by boards.
> 
SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
present regardless.
> > +	};
> > +
> > +	clk_3m: clk_3m {
> 
> No underscores in node names, generic node name (see above).
> 
early debugging clock, will be removed.
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <3000000>;
> 
> This does not look like property of the SoC, so should be defined by boards.
> 
> > +	};
> > +
> > +	soc {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "simple-bus";
> > +		interrupt-parent = <&gic>;
> > +		ranges;
> > +
> > +		hi_sfc: spi@10000000 {
> > +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> > +			reg-names = "control", "memory";
> > +			clocks = <&crg HI3521A_FMC_CLK>;
> > +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > +			status = "disabled";
> > +		};
> > +
> > +		/* usb0: usb@10030000 { */
> > +		/* 		compatible = "generic-ohci"; */
> > +		/* 		reg = <0x10030000 0x1000>; */
> > +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> > +		/* 		clocks = <&crg > */
> > +		/* 	}; */
> 
> No dead code please.
> 
> > +		dmac: dma-controller@10060000 {
> > +			compatible = "arm,pl080", "arm,primecell";
> > +			arm,primecell-periphid = <0x00041080>;
> > +			reg = <0x10060000 0x1000>;
> > +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_DMAC_CLK>;
> > +			clock-names = "apb_pclk";
> > +			lli-bus-interface-ahb1;
> > +			lli-bus-interface-ahb2;
> > +			mem-bus-interface-ahb1;
> > +			mem-bus-interface-ahb2;
> > +			memcpy-burst-size = <256>;
> > +			memcpy-bus-width = <32>;
> > +			#dma-cells = <2>;
> > +			status = "okay";
> 
> No need for status okay, it's by default.
> 
ah, good to know.
> > +		};
> > +
> > +		gmac0: ethernet@100a0000 {
> > +			compatible = "hisilicon,hisi-gmac-v1";
> > +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> > +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> > +			clock-names = "mac_core", "mac_ifc";
> > +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> > +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> > +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */
> 
> No dead code.
> 
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer0: timer@12000000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> 
> A bit weird interrupts... the same?
> 
Yes, though I am aware that some sp804 timers do have a separate
interrupts per pair.
> > +			reg = <0x12000000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> > +				 <&sysctrl HI3521A_TIMER1_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer1: timer@12010000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12010000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> > +				 <&sysctrl HI3521A_TIMER3_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer2: timer@12020000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12020000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> > +				 <&sysctrl HI3521A_TIMER5_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer3: timer@12030000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12030000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> > +				 <&sysctrl HI3521A_TIMER7_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		crg: clock-reset-controller@12040000 {
> > +			compatible = "hisilicon,hi3521a-crg";
> 
> Undocumented compatible.  Didn't checkpatch warn about it?
> 
As mentioned, RFC. I'm more concerned with getting it to work consistently
at the moment.
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +			reg = <0x12040000 0x1000>;
> > +		};
> > +
> > +		sysctrl: system-controller@12050000 {
> > +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";
> 
> Same.
> 
> > +			reg = <0x12050000 0x1000>;
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +		};
> > +
> > +		reboot {
> > +			compatible = "syscon-reboot";
> > +			regmap = <&sysctrl>;
> > +			offset = <0x4>;
> > +			mask = <0xdeadbeef>;
> > +		};
> > +
> > +		wdt0: watchdog@12070000 {
> > +			compatible = "arm,sp805", "arm,primecell";
> > +			arm,primecell-periphid = <0x00141805>;
> > +			reg = <0x12070000 0x1000>;
> > +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_FIXED_3M>;
> > +			clock-names = "apb_pclk";
> > +		};
> > +
> > +		uart0: serial@12080000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12080000 0x1000>;
> > +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> > +			clock-names = "uartclk", "apb_pclk";
> > +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart1: serial@12090000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12090000 0x1000>;
> > +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART1_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart2: serial@120a0000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x120a0000 0x1000>;
> > +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART2_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		spi_bus0: spi@120d0000 {
> > +			compatible = "arm,pl022", "arm,primecell";
> > +			reg = <0x120d0000 0x1000>;
> > +			arm,primecell-periphid = <0x00041022>;
> > +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_SPI0_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> > +			dma-names = "rx", "tx";
> > +			num-cs = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pmx0: pinmux@120f0000 {
> > +			compatible = "pinctrl-single";
> > +			reg = <0x120f0000 0x188>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			#gpio-range-cells = <3>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +			pinctrl-single,function-mask = <7>;
> > +			/* pin base, nr pins & gpio function */
> > +			pinctrl-single,gpio-range = <
> > +				&range 58 4 0
> > +			>;
> > +
> > +			range: gpio-range {
> > +				#pinctrl-single,gpio-range-cells = <3>;
> > +			};
> > +		};
> > +
> > +		pmx1: pinmux@120f0800 {
> > +			compatible = "pinconf-single";
> > +			reg = <0x120f0800 0x1d4>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +		};
> > +
> > +		gpio0: gpio@12150000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12150000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio1: gpio@12160000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12160000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio2: gpio@12170000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12170000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio3: gpio@12180000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12180000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio4: gpio@12190000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12190000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio5: gpio@121a0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121a0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio6: gpio@121b0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121b0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> 
> Same interrupts as for other nodes? Is it correct?
> 
Yep, there are three interrupts for the gpios and they're shared among a
few each.
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio7: gpio@121c0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121c0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +_MULTI_V7
> 
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> > hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> > security systems and ipcameras. The arm core is a Cortex A7.
> > 
> > Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> > marketed under the name Samsung SDR-B74301N.
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  arch/arm/boot/dts/Makefile              |   2 +
> >  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
> >  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
> 
> DTSes go to separate patches.
Do you mean dts and dtsi need to be separate patches?
> 
> >  arch/arm/mach-hisi/Kconfig              |   9 +
> >  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
> >  	gemini-ssi1328.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..b24fcf2ca85e
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include "hi3521a.dtsi"
> > +
> > +/ {
> > +	model = "RaySharp RS-DM-290E DVR Board";
> > +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
> 
> Please run checkpatch and fix the warnings.
> 
sunova. I could have sworn I had my editor setup right for whitespace
and such.
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	memory@800000000 {
> > +		device_type = "memory";
> > +		reg = <0x80000000 0xf00000>;
> > +	};
> > +};
> > +
> > +&hi_sfc {
> > +	status = "okay";
> > +	spi-nor@0 {
> 
> Looks like wrong node name. You need to run "dtbs_check" and fix
> warnings related to your DTS.
> 
Ah, that's a good thing to know. dt-doc-validate isn't found, doesn't
seem packaged for gentoo either. I'll see about that asap.
> > +		// compatible = "jedec,spi-nor";
> 
> No dead code in Linux kernel. Commented out stuff sometimes is okay if
> it is explained with comments.
> 
As mentioned in the title, just an RFC, not final.
> > +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> > +		reg = <0>;
> > +		spi-max-frequency = <150000000>;
> > +		// spi-rx-bus-width = <1>;
> > +		// spi-tx-bus-width = <1>;
> > +		// m25p,default-addr-width = <4>;
> > +		// spi-max-frequency = <160000000>;
> 
> No dead code.
> 
> > +		m25p,fast-read;
> > +
> > +		partitions {
> > +			compatible = "fixed-partitions";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			
> > +			u-boot@0 {
> > +				label = "u-boot";
> > +				reg = <0x0 0x50000>;
> > +				read-only;
> > +			};
> > +			u-boot-env@50000 {
> > +				label = "u-boot-env";
> > +				reg = <0x50000 0x20000>;
> > +			};
> > +			kernel@70000 {
> > +				label = "kernel";
> > +				reg = <0x70000 0x700000>;
> > +			};
> > +			rootfs@770000 {
> > +				label = "rootfs";
> > +				reg = <0x800000 0x300000>;
> > +				read-only;
> > +			};
> > +			extra@b00000 {
> > +				label = "extra";
> > +				reg = <0xb00000 0x1500000>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&dual_timer0 {
> > +	status = "okay";
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> > +	status = "okay";
> > +};
> > +
> > +&pmx0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_pmx_func>;
> > +	uart0_pmx_func: uart0_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> > +
> > +	sfc_pmx_func: sfc_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x1
> > +			0x00c8 0x1
> > +			0x00cc 0x1
> > +			0x00d0 0x1
> > +			0x00d4 0x1
> > +		>;
> > +	};
> > +};
> > +
> > +&pmx1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_cfg_func>;
> > +	uart0_cfg_func: uart0_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> 
> Blank line
> 
> > +	sfc_cfg_func: sfc_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x58
> > +			0x00c8 0x28
> > +			0x00cc 0x38
> > +			0x00d0 0x38
> > +			0x00d4 0x38
> > +		>;
> > +	};
> > +};
> > +
> > +/* &gmac0 { */
> > +/* 	#address-cells = <1>; */
> > +/* 	#size-cells = <0>; */
> > +/* 	phy-handle = <&phy3>; */
> > +/* 	phy-mode = "rgmii"; */
> > +/* 	mac-address = [00 00 00 00 00 00]; */
> > +/* 	status = "okay"; */
> > +/**/
> > +/* 	phy3: ethernet-phy@3 { */
> > +/* 		reg = <3>; */
> > +/* 	}; */
> > +/* }; */
> 
> 
> ? What's this?
> 
ethernet port. its giving me a bit of trouble at the moment, but it does
work and has a phy at 3.
> > diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> > new file mode 100644
> > index 000000000000..53993a32fd5d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a.dtsi
> > @@ -0,0 +1,423 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include <dt-bindings/clock/hi3521a-clock.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +/dts-v1/;
> 
> Blank line.
> 
> > +/ {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu0: cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a7";
> > +			reg = <0>;
> > +			clock-frequency = <1100000000>;
> > +		};
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv7-timer";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_PPI 13 0xf08>,
> > +			     <GIC_PPI 14 0xf08>,
> > +			     <GIC_PPI 11 0xf08>,
> > +			     <GIC_PPI 10 0xf08>;
> 
> Use macros for these interrupts.
> 
moot point, turns out enabling this timer is causing the system to lock
up. Something's weird there.
> > +		clock-frequency = <24000000>;
> > +	};
> > +
> > +	pmu {
> > +		compatible = "arm,cortex-a7-pmu";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > +	};
> > +
> > +	gic: interrupt-controller@10301000 {
> > +		compatible = "arm,pl390";
> > +		#interrupt-cells = <3>;
> > +		interrupt-controller;
> > +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
> 
> Put reg just after compatible. This applies to each other node as well
> (if it comes with reg).
> 
Ah gotcha.
> > +	};
> > +
> > +	xtal24m: xtal24m {
> 
> Generic node names, so one of: "clock-0" "clock-xtal24m"
> 
Will do.
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> 
> This does not look like property of the SoC, so should be defined by boards.
> 
SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
present regardless.
> > +	};
> > +
> > +	clk_3m: clk_3m {
> 
> No underscores in node names, generic node name (see above).
> 
early debugging clock, will be removed.
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <3000000>;
> 
> This does not look like property of the SoC, so should be defined by boards.
> 
> > +	};
> > +
> > +	soc {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "simple-bus";
> > +		interrupt-parent = <&gic>;
> > +		ranges;
> > +
> > +		hi_sfc: spi@10000000 {
> > +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> > +			reg-names = "control", "memory";
> > +			clocks = <&crg HI3521A_FMC_CLK>;
> > +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > +			status = "disabled";
> > +		};
> > +
> > +		/* usb0: usb@10030000 { */
> > +		/* 		compatible = "generic-ohci"; */
> > +		/* 		reg = <0x10030000 0x1000>; */
> > +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> > +		/* 		clocks = <&crg > */
> > +		/* 	}; */
> 
> No dead code please.
> 
> > +		dmac: dma-controller@10060000 {
> > +			compatible = "arm,pl080", "arm,primecell";
> > +			arm,primecell-periphid = <0x00041080>;
> > +			reg = <0x10060000 0x1000>;
> > +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_DMAC_CLK>;
> > +			clock-names = "apb_pclk";
> > +			lli-bus-interface-ahb1;
> > +			lli-bus-interface-ahb2;
> > +			mem-bus-interface-ahb1;
> > +			mem-bus-interface-ahb2;
> > +			memcpy-burst-size = <256>;
> > +			memcpy-bus-width = <32>;
> > +			#dma-cells = <2>;
> > +			status = "okay";
> 
> No need for status okay, it's by default.
> 
ah, good to know.
> > +		};
> > +
> > +		gmac0: ethernet@100a0000 {
> > +			compatible = "hisilicon,hisi-gmac-v1";
> > +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> > +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> > +			clock-names = "mac_core", "mac_ifc";
> > +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> > +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> > +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */
> 
> No dead code.
> 
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer0: timer@12000000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> 
> A bit weird interrupts... the same?
> 
Yes, though I am aware that some sp804 timers do have a separate
interrupts per pair.
> > +			reg = <0x12000000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> > +				 <&sysctrl HI3521A_TIMER1_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer1: timer@12010000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12010000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> > +				 <&sysctrl HI3521A_TIMER3_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer2: timer@12020000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12020000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> > +				 <&sysctrl HI3521A_TIMER5_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer3: timer@12030000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12030000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> > +				 <&sysctrl HI3521A_TIMER7_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		crg: clock-reset-controller@12040000 {
> > +			compatible = "hisilicon,hi3521a-crg";
> 
> Undocumented compatible.  Didn't checkpatch warn about it?
> 
As mentioned, RFC. I'm more concerned with getting it to work consistently
at the moment.
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +			reg = <0x12040000 0x1000>;
> > +		};
> > +
> > +		sysctrl: system-controller@12050000 {
> > +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";
> 
> Same.
> 
> > +			reg = <0x12050000 0x1000>;
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +		};
> > +
> > +		reboot {
> > +			compatible = "syscon-reboot";
> > +			regmap = <&sysctrl>;
> > +			offset = <0x4>;
> > +			mask = <0xdeadbeef>;
> > +		};
> > +
> > +		wdt0: watchdog@12070000 {
> > +			compatible = "arm,sp805", "arm,primecell";
> > +			arm,primecell-periphid = <0x00141805>;
> > +			reg = <0x12070000 0x1000>;
> > +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_FIXED_3M>;
> > +			clock-names = "apb_pclk";
> > +		};
> > +
> > +		uart0: serial@12080000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12080000 0x1000>;
> > +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> > +			clock-names = "uartclk", "apb_pclk";
> > +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart1: serial@12090000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12090000 0x1000>;
> > +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART1_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart2: serial@120a0000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x120a0000 0x1000>;
> > +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART2_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		spi_bus0: spi@120d0000 {
> > +			compatible = "arm,pl022", "arm,primecell";
> > +			reg = <0x120d0000 0x1000>;
> > +			arm,primecell-periphid = <0x00041022>;
> > +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_SPI0_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> > +			dma-names = "rx", "tx";
> > +			num-cs = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pmx0: pinmux@120f0000 {
> > +			compatible = "pinctrl-single";
> > +			reg = <0x120f0000 0x188>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			#gpio-range-cells = <3>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +			pinctrl-single,function-mask = <7>;
> > +			/* pin base, nr pins & gpio function */
> > +			pinctrl-single,gpio-range = <
> > +				&range 58 4 0
> > +			>;
> > +
> > +			range: gpio-range {
> > +				#pinctrl-single,gpio-range-cells = <3>;
> > +			};
> > +		};
> > +
> > +		pmx1: pinmux@120f0800 {
> > +			compatible = "pinconf-single";
> > +			reg = <0x120f0800 0x1d4>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +		};
> > +
> > +		gpio0: gpio@12150000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12150000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio1: gpio@12160000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12160000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio2: gpio@12170000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12170000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio3: gpio@12180000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12180000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio4: gpio@12190000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12190000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio5: gpio@121a0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121a0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio6: gpio@121b0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121b0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> 
> Same interrupts as for other nodes? Is it correct?
> 
Yep, there are three interrupts for the gpios and they're shared among a
few each.
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio7: gpio@121c0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121c0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +_MULTI_V7
> 
> 
> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 15:44, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
>>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
>>> security systems and ipcameras. The arm core is a Cortex A7.
>>>
>>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
>>> marketed under the name Samsung SDR-B74301N.
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  arch/arm/boot/dts/Makefile              |   2 +
>>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
>>
>> DTSes go to separate patches.
> Do you mean dts and dtsi need to be separate patches?

I mean that any changes to "arch/arm/boot/dts/" have to be separate from
other changes. These can be still one patch. See other examples on
mailing lists.

>>
>>>  arch/arm/mach-hisi/Kconfig              |   9 +
>>>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>>>  	gemini-ssi1328.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..b24fcf2ca85e
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
>>> @@ -0,0 +1,134 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
>>> + */
>>> +
>>> +#include "hi3521a.dtsi"
>>> +
>>> +/ {
>>> +	model = "RaySharp RS-DM-290E DVR Board";
>>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
>>
>> Please run checkpatch and fix the warnings.
>>
> sunova. I could have sworn I had my editor setup right for whitespace
> and such.

It's not about whitespace but:

WARNING: DT compatible string "raysharp,rs-dm-290e" appears
un-documented -- check ./Documentation/devicetree/bindings/


WARNING: DT compatible string vendor "raysharp" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml


(...)

> Ah gotcha.
>>> +	};
>>> +
>>> +	xtal24m: xtal24m {
>>
>> Generic node names, so one of: "clock-0" "clock-xtal24m"
>>
> Will do.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.
>>
> SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> present regardless.

Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
but on the board, it should be in the board DTSI. Many times such clocks
are put partially in DTSI and only their specific parts - frequency - in
the board DTS, to indicate that implementation is relevant to the board,
not SoC.

>>> +	};
>>> +
>>> +	clk_3m: clk_3m {
>>
>> No underscores in node names, generic node name (see above).
>>
> early debugging clock, will be removed.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <3000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.

(...)

>>
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		dual_timer0: timer@12000000 {
>>> +			compatible = "arm,sp804", "arm,primecell";
>>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>
>> A bit weird interrupts... the same?
>>
> Yes, though I am aware that some sp804 timers do have a separate
> interrupts per pair.

They have also separate interrupts, one combined interrupt or one sole
interrupt. However what you described here is one interrupt line
physically connected to two separate pins on the device yet still not
being somehow shared (shared as "combined interrupt"). I don't think it
is your case...



Best regards,
Krzysztof

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 15:44, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
>>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
>>> security systems and ipcameras. The arm core is a Cortex A7.
>>>
>>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
>>> marketed under the name Samsung SDR-B74301N.
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  arch/arm/boot/dts/Makefile              |   2 +
>>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
>>
>> DTSes go to separate patches.
> Do you mean dts and dtsi need to be separate patches?

I mean that any changes to "arch/arm/boot/dts/" have to be separate from
other changes. These can be still one patch. See other examples on
mailing lists.

>>
>>>  arch/arm/mach-hisi/Kconfig              |   9 +
>>>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>>>  	gemini-ssi1328.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..b24fcf2ca85e
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
>>> @@ -0,0 +1,134 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
>>> + */
>>> +
>>> +#include "hi3521a.dtsi"
>>> +
>>> +/ {
>>> +	model = "RaySharp RS-DM-290E DVR Board";
>>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
>>
>> Please run checkpatch and fix the warnings.
>>
> sunova. I could have sworn I had my editor setup right for whitespace
> and such.

It's not about whitespace but:

WARNING: DT compatible string "raysharp,rs-dm-290e" appears
un-documented -- check ./Documentation/devicetree/bindings/


WARNING: DT compatible string vendor "raysharp" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml


(...)

> Ah gotcha.
>>> +	};
>>> +
>>> +	xtal24m: xtal24m {
>>
>> Generic node names, so one of: "clock-0" "clock-xtal24m"
>>
> Will do.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.
>>
> SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> present regardless.

Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
but on the board, it should be in the board DTSI. Many times such clocks
are put partially in DTSI and only their specific parts - frequency - in
the board DTS, to indicate that implementation is relevant to the board,
not SoC.

>>> +	};
>>> +
>>> +	clk_3m: clk_3m {
>>
>> No underscores in node names, generic node name (see above).
>>
> early debugging clock, will be removed.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <3000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.

(...)

>>
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		dual_timer0: timer@12000000 {
>>> +			compatible = "arm,sp804", "arm,primecell";
>>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>
>> A bit weird interrupts... the same?
>>
> Yes, though I am aware that some sp804 timers do have a separate
> interrupts per pair.

They have also separate interrupts, one combined interrupt or one sole
interrupt. However what you described here is one interrupt line
physically connected to two separate pins on the device yet still not
being somehow shared (shared as "combined interrupt"). I don't think it
is your case...



Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 15:44, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
>>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
>>> security systems and ipcameras. The arm core is a Cortex A7.
>>>
>>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
>>> marketed under the name Samsung SDR-B74301N.
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  arch/arm/boot/dts/Makefile              |   2 +
>>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
>>
>> DTSes go to separate patches.
> Do you mean dts and dtsi need to be separate patches?

I mean that any changes to "arch/arm/boot/dts/" have to be separate from
other changes. These can be still one patch. See other examples on
mailing lists.

>>
>>>  arch/arm/mach-hisi/Kconfig              |   9 +
>>>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>>>  	gemini-ssi1328.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..b24fcf2ca85e
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
>>> @@ -0,0 +1,134 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
>>> + */
>>> +
>>> +#include "hi3521a.dtsi"
>>> +
>>> +/ {
>>> +	model = "RaySharp RS-DM-290E DVR Board";
>>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
>>
>> Please run checkpatch and fix the warnings.
>>
> sunova. I could have sworn I had my editor setup right for whitespace
> and such.

It's not about whitespace but:

WARNING: DT compatible string "raysharp,rs-dm-290e" appears
un-documented -- check ./Documentation/devicetree/bindings/


WARNING: DT compatible string vendor "raysharp" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml


(...)

> Ah gotcha.
>>> +	};
>>> +
>>> +	xtal24m: xtal24m {
>>
>> Generic node names, so one of: "clock-0" "clock-xtal24m"
>>
> Will do.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.
>>
> SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> present regardless.

Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
but on the board, it should be in the board DTSI. Many times such clocks
are put partially in DTSI and only their specific parts - frequency - in
the board DTS, to indicate that implementation is relevant to the board,
not SoC.

>>> +	};
>>> +
>>> +	clk_3m: clk_3m {
>>
>> No underscores in node names, generic node name (see above).
>>
> early debugging clock, will be removed.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <3000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.

(...)

>>
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		dual_timer0: timer@12000000 {
>>> +			compatible = "arm,sp804", "arm,primecell";
>>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>
>> A bit weird interrupts... the same?
>>
> Yes, though I am aware that some sp804 timers do have a separate
> interrupts per pair.

They have also separate interrupts, one combined interrupt or one sole
interrupt. However what you described here is one interrupt line
physically connected to two separate pins on the device yet still not
being somehow shared (shared as "combined interrupt"). I don't think it
is your case...



Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

On Tue, May 03, 2022 at 04:55:23PM +0200, Krzysztof Kozlowski wrote:
> On 03/05/2022 15:44, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> >>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> >>> security systems and ipcameras. The arm core is a Cortex A7.
> >>>
> >>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> >>> marketed under the name Samsung SDR-B74301N.
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  arch/arm/boot/dts/Makefile              |   2 +
> >>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
> >>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
> >>
> >> DTSes go to separate patches.
> > Do you mean dts and dtsi need to be separate patches?
> 
> I mean that any changes to "arch/arm/boot/dts/" have to be separate from
> other changes. These can be still one patch. See other examples on
> mailing lists.
> 
> >>
> >>>  arch/arm/mach-hisi/Kconfig              |   9 +
> >>>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> >>> --- a/arch/arm/boot/dts/Makefile
> >>> +++ b/arch/arm/boot/dts/Makefile
> >>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
> >>>  	gemini-ssi1328.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..b24fcf2ca85e
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> >>> @@ -0,0 +1,134 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>> +/*
> >>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> >>> + */
> >>> +
> >>> +#include "hi3521a.dtsi"
> >>> +
> >>> +/ {
> >>> +	model = "RaySharp RS-DM-290E DVR Board";
> >>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
> >>
> >> Please run checkpatch and fix the warnings.
> >>
> > sunova. I could have sworn I had my editor setup right for whitespace
> > and such.
> 
> It's not about whitespace but:
> 
ah. Well, I'm not certain what we'll even call the board in the end,
kind of a placeholder for now. What do you even name devices which are
generic consumer hardware that they never intended to be tinkered with
this way? the board is whiteboxed by several different vendors.
> WARNING: DT compatible string "raysharp,rs-dm-290e" appears
> un-documented -- check ./Documentation/devicetree/bindings/
> 
> 
> WARNING: DT compatible string vendor "raysharp" appears un-documented --
> check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
> 
> 
> (...)
> 
> > Ah gotcha.
> >>> +	};
> >>> +
> >>> +	xtal24m: xtal24m {
> >>
> >> Generic node names, so one of: "clock-0" "clock-xtal24m"
> >>
> > Will do.
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <24000000>;
> >>
> >> This does not look like property of the SoC, so should be defined by boards.
> >>
> > SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> > present regardless.
> 
> Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
> but on the board, it should be in the board DTSI. Many times such clocks
> are put partially in DTSI and only their specific parts - frequency - in
> the board DTS, to indicate that implementation is relevant to the board,
> not SoC.
> 
Ah ok, that makes sense I guess.
> >>> +	};
> >>> +
> >>> +	clk_3m: clk_3m {
> >>
> >> No underscores in node names, generic node name (see above).
> >>
> > early debugging clock, will be removed.
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <3000000>;
> >>
> >> This does not look like property of the SoC, so should be defined by boards.
> 
> (...)
> 
> >>
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +		dual_timer0: timer@12000000 {
> >>> +			compatible = "arm,sp804", "arm,primecell";
> >>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> >>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >> A bit weird interrupts... the same?
> >>
> > Yes, though I am aware that some sp804 timers do have a separate
> > interrupts per pair.
> 
> They have also separate interrupts, one combined interrupt or one sole
> interrupt. However what you described here is one interrupt line
> physically connected to two separate pins on the device yet still not
> being somehow shared (shared as "combined interrupt"). I don't think it
> is your case...
> 
Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
timers are attached to pins, however.
> 
> 
> Best regards,
> Krzysztof

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

On Tue, May 03, 2022 at 04:55:23PM +0200, Krzysztof Kozlowski wrote:
> On 03/05/2022 15:44, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> >>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> >>> security systems and ipcameras. The arm core is a Cortex A7.
> >>>
> >>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> >>> marketed under the name Samsung SDR-B74301N.
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  arch/arm/boot/dts/Makefile              |   2 +
> >>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
> >>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
> >>
> >> DTSes go to separate patches.
> > Do you mean dts and dtsi need to be separate patches?
> 
> I mean that any changes to "arch/arm/boot/dts/" have to be separate from
> other changes. These can be still one patch. See other examples on
> mailing lists.
> 
> >>
> >>>  arch/arm/mach-hisi/Kconfig              |   9 +
> >>>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> >>> --- a/arch/arm/boot/dts/Makefile
> >>> +++ b/arch/arm/boot/dts/Makefile
> >>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
> >>>  	gemini-ssi1328.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..b24fcf2ca85e
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> >>> @@ -0,0 +1,134 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>> +/*
> >>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> >>> + */
> >>> +
> >>> +#include "hi3521a.dtsi"
> >>> +
> >>> +/ {
> >>> +	model = "RaySharp RS-DM-290E DVR Board";
> >>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
> >>
> >> Please run checkpatch and fix the warnings.
> >>
> > sunova. I could have sworn I had my editor setup right for whitespace
> > and such.
> 
> It's not about whitespace but:
> 
ah. Well, I'm not certain what we'll even call the board in the end,
kind of a placeholder for now. What do you even name devices which are
generic consumer hardware that they never intended to be tinkered with
this way? the board is whiteboxed by several different vendors.
> WARNING: DT compatible string "raysharp,rs-dm-290e" appears
> un-documented -- check ./Documentation/devicetree/bindings/
> 
> 
> WARNING: DT compatible string vendor "raysharp" appears un-documented --
> check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
> 
> 
> (...)
> 
> > Ah gotcha.
> >>> +	};
> >>> +
> >>> +	xtal24m: xtal24m {
> >>
> >> Generic node names, so one of: "clock-0" "clock-xtal24m"
> >>
> > Will do.
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <24000000>;
> >>
> >> This does not look like property of the SoC, so should be defined by boards.
> >>
> > SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> > present regardless.
> 
> Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
> but on the board, it should be in the board DTSI. Many times such clocks
> are put partially in DTSI and only their specific parts - frequency - in
> the board DTS, to indicate that implementation is relevant to the board,
> not SoC.
> 
Ah ok, that makes sense I guess.
> >>> +	};
> >>> +
> >>> +	clk_3m: clk_3m {
> >>
> >> No underscores in node names, generic node name (see above).
> >>
> > early debugging clock, will be removed.
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <3000000>;
> >>
> >> This does not look like property of the SoC, so should be defined by boards.
> 
> (...)
> 
> >>
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +		dual_timer0: timer@12000000 {
> >>> +			compatible = "arm,sp804", "arm,primecell";
> >>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> >>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >> A bit weird interrupts... the same?
> >>
> > Yes, though I am aware that some sp804 timers do have a separate
> > interrupts per pair.
> 
> They have also separate interrupts, one combined interrupt or one sole
> interrupt. However what you described here is one interrupt line
> physically connected to two separate pins on the device yet still not
> being somehow shared (shared as "combined interrupt"). I don't think it
> is your case...
> 
Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
timers are attached to pins, however.
> 
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Marty E. Plummer]

On Tue, May 03, 2022 at 04:55:23PM +0200, Krzysztof Kozlowski wrote:
> On 03/05/2022 15:44, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
> >>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
> >>> security systems and ipcameras. The arm core is a Cortex A7.
> >>>
> >>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
> >>> marketed under the name Samsung SDR-B74301N.
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  arch/arm/boot/dts/Makefile              |   2 +
> >>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
> >>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
> >>
> >> DTSes go to separate patches.
> > Do you mean dts and dtsi need to be separate patches?
> 
> I mean that any changes to "arch/arm/boot/dts/" have to be separate from
> other changes. These can be still one patch. See other examples on
> mailing lists.
> 
> >>
> >>>  arch/arm/mach-hisi/Kconfig              |   9 +
> >>>  4 files changed, 568 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 7c16f8a2b738..535cef3b14ab 100644
> >>> --- a/arch/arm/boot/dts/Makefile
> >>> +++ b/arch/arm/boot/dts/Makefile
> >>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
> >>>  	gemini-ssi1328.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..b24fcf2ca85e
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
> >>> @@ -0,0 +1,134 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>> +/*
> >>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> >>> + */
> >>> +
> >>> +#include "hi3521a.dtsi"
> >>> +
> >>> +/ {
> >>> +	model = "RaySharp RS-DM-290E DVR Board";
> >>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
> >>
> >> Please run checkpatch and fix the warnings.
> >>
> > sunova. I could have sworn I had my editor setup right for whitespace
> > and such.
> 
> It's not about whitespace but:
> 
ah. Well, I'm not certain what we'll even call the board in the end,
kind of a placeholder for now. What do you even name devices which are
generic consumer hardware that they never intended to be tinkered with
this way? the board is whiteboxed by several different vendors.
> WARNING: DT compatible string "raysharp,rs-dm-290e" appears
> un-documented -- check ./Documentation/devicetree/bindings/
> 
> 
> WARNING: DT compatible string vendor "raysharp" appears un-documented --
> check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
> 
> 
> (...)
> 
> > Ah gotcha.
> >>> +	};
> >>> +
> >>> +	xtal24m: xtal24m {
> >>
> >> Generic node names, so one of: "clock-0" "clock-xtal24m"
> >>
> > Will do.
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <24000000>;
> >>
> >> This does not look like property of the SoC, so should be defined by boards.
> >>
> > SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> > present regardless.
> 
> Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
> but on the board, it should be in the board DTSI. Many times such clocks
> are put partially in DTSI and only their specific parts - frequency - in
> the board DTS, to indicate that implementation is relevant to the board,
> not SoC.
> 
Ah ok, that makes sense I guess.
> >>> +	};
> >>> +
> >>> +	clk_3m: clk_3m {
> >>
> >> No underscores in node names, generic node name (see above).
> >>
> > early debugging clock, will be removed.
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <3000000>;
> >>
> >> This does not look like property of the SoC, so should be defined by boards.
> 
> (...)
> 
> >>
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +		dual_timer0: timer@12000000 {
> >>> +			compatible = "arm,sp804", "arm,primecell";
> >>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> >>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >> A bit weird interrupts... the same?
> >>
> > Yes, though I am aware that some sp804 timers do have a separate
> > interrupts per pair.
> 
> They have also separate interrupts, one combined interrupt or one sole
> interrupt. However what you described here is one interrupt line
> physically connected to two separate pins on the device yet still not
> being somehow shared (shared as "combined interrupt"). I don't think it
> is your case...
> 
Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
timers are attached to pins, however.
> 
> 
> Best regards,
> Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 17:51, Marty E. Plummer wrote:
>>>> A bit weird interrupts... the same?
>>>>
>>> Yes, though I am aware that some sp804 timers do have a separate
>>> interrupts per pair.
>>
>> They have also separate interrupts, one combined interrupt or one sole
>> interrupt. However what you described here is one interrupt line
>> physically connected to two separate pins on the device yet still not
>> being somehow shared (shared as "combined interrupt"). I don't think it
>> is your case...
>>
> Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
> timers are attached to pins, however.

So it looks like a combined interrupt, doesn't it?


Best regards,
Krzysztof

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 17:51, Marty E. Plummer wrote:
>>>> A bit weird interrupts... the same?
>>>>
>>> Yes, though I am aware that some sp804 timers do have a separate
>>> interrupts per pair.
>>
>> They have also separate interrupts, one combined interrupt or one sole
>> interrupt. However what you described here is one interrupt line
>> physically connected to two separate pins on the device yet still not
>> being somehow shared (shared as "combined interrupt"). I don't think it
>> is your case...
>>
> Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
> timers are attached to pins, however.

So it looks like a combined interrupt, doesn't it?


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 17:51, Marty E. Plummer wrote:
>>>> A bit weird interrupts... the same?
>>>>
>>> Yes, though I am aware that some sp804 timers do have a separate
>>> interrupts per pair.
>>
>> They have also separate interrupts, one combined interrupt or one sole
>> interrupt. However what you described here is one interrupt line
>> physically connected to two separate pins on the device yet still not
>> being somehow shared (shared as "combined interrupt"). I don't think it
>> is your case...
>>
> Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
> timers are attached to pins, however.

So it looks like a combined interrupt, doesn't it?


Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC v2 0/2] Hi3521a support.

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Resend RFC.
> 
> Changes in v2:
> - Actually include the dts files.
> - DT Bindings still missing, as the the driver is not quite complete
>   (need to add the reset controller bindings, have't quite figured that
>   out yet.)
> 
> Marty E. Plummer (2):
>   clk: hisilicon: add CRG driver Hi3521a SoC
>   arm: hisi: enable Hi3521a soc

Still no bindings for boards/SoC.



Best regards,
Krzysztof

Re: [RFC v2 0/2] Hi3521a support.

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Resend RFC.
> 
> Changes in v2:
> - Actually include the dts files.
> - DT Bindings still missing, as the the driver is not quite complete
>   (need to add the reset controller bindings, have't quite figured that
>   out yet.)
> 
> Marty E. Plummer (2):
>   clk: hisilicon: add CRG driver Hi3521a SoC
>   arm: hisi: enable Hi3521a soc

Still no bindings for boards/SoC.



Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 0/2] Hi3521a support.

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Resend RFC.
> 
> Changes in v2:
> - Actually include the dts files.
> - DT Bindings still missing, as the the driver is not quite complete
>   (need to add the reset controller bindings, have't quite figured that
>   out yet.)
> 
> Marty E. Plummer (2):
>   clk: hisilicon: add CRG driver Hi3521a SoC
>   arm: hisi: enable Hi3521a soc

Still no bindings for boards/SoC.



Best regards,
Krzysztof

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/