Re: [PATCH v7 06/10] ARM: dts: rockchip: Add Rockchip RV1126 SoC

[Johan Jonker <jbx6244@gmail.com>]

Hi Jagan, Heiko,

Have a look at some comment below.

Johan

On 11/8/22 05:13, Jagan Teki wrote:
> RV1126 is a high-performance vision processor SoC for IPC/CVR,
> especially for AI related application.
> 
> It is based on quad-core ARM Cortex-A7 32-bit core which integrates
> NEON and FPU. There is a 32KB I-cache and 32KB D-cache for each core
> and 512KB unified L2 cache. It has build-in NPU supports INT8/INT16
> hybrid operation and computing power is up to 2.0TOPs.
> 
> This patch add basic core dtsi support.
> 
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> ---
> Changes for v7:
> - fix dtbs_check
> - rearrange nodes
> - remove Edegble in license text
> Changes for v6:
> - add psci node
> Changes for v5:
> - none
> Changes for v4:
> - update i2c0
> - rebase on -next
> Changes for v3:
> - update cru and power file names
> Changes for v2:
> - split pinctrl in separate patch
> 
>  arch/arm/boot/dts/rv1126.dtsi | 438 ++++++++++++++++++++++++++++++++++
>  1 file changed, 438 insertions(+)
>  create mode 100644 arch/arm/boot/dts/rv1126.dtsi
> 
> diff --git a/arch/arm/boot/dts/rv1126.dtsi b/arch/arm/boot/dts/rv1126.dtsi
> new file mode 100644
> index 000000000000..a485420551f5
> --- /dev/null
> +++ b/arch/arm/boot/dts/rv1126.dtsi
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2019 Fuzhou Rockchip Electronics Co., Ltd.
> + */
> +
> +#include <dt-bindings/clock/rockchip,rv1126-cru.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/power/rockchip,rv1126-power.h>
> +#include <dt-bindings/soc/rockchip,boot-mode.h>
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	compatible = "rockchip,rv1126";
> +

[..]

> +	uart0: serial@ff560000 {
> +		compatible = "rockchip,rv1126-uart", "snps,dw-apb-uart";
> +		reg = <0xff560000 0x100>;
> +		interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> +		clock-frequency = <24000000>;
> +		clocks = <&cru SCLK_UART0>, <&cru PCLK_UART0>;
> +		clock-names = "baudclk", "apb_pclk";

> +		dmas = <&dmac 5>, <&dmac 4>;

		dma-names = "tx", "rx";

DT describes hardware.
Maybe add some dma-names ?

===

4 UART0 RX High level
5 UART0 TX High level

6 UART1 RX High level
7 UART1 TX High level

8 UART2 RX High level
9 UART2 TX High level

10 UART3 RX High level
11 UART3 TX High level

12 UART4 RX High level
13 UART4 TX High level

14 UART5 RX High level
15 UART5 TX High level

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&uart0_xfer>;
> +		reg-shift = <2>;
> +		reg-io-width = <4>;
> +		status = "disabled";
> +	};


[..]


> +
> +	timer: timer@ff660000 {

timer0: timer@ff660000 {

This is the first of 6 timers. Change label.

> +		compatible = "rockchip,rv1126-timer", "rockchip,rk3288-timer";
> +		reg = <0xff660000 0x20>;
> +		interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru PCLK_TIMER>, <&cru CLK_TIMER0>;
> +		clock-names = "pclk", "timer";
> +	};

Add possible more timer nodes ?

rv1126 TRM page 727:
descending Timers(Timer0~4)

incremental Timers(Timer5 and STimer0~1)

===

Question for Heiko:
Do we need a different compatible string for Timer5 ?

 "rockchip,rv1126-timer-inc" ??

===

SPI irq addr 56-32=24

56 timer0_int High level
57 timer1_int High level
58 timer2_int High level
59 timer3_int High level
60 timer4_int High level
61 timer5_int High level