Re: [PATCH v15 10/10] ARM: dts: Add Sunplus SP7021-Demo-V3 board device tree

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/05/2022 08:31, Qin Jian wrote:
> Add the basic support for Sunplus SP7021-Demo-V3 board.
> 
> Signed-off-by: Qin Jian <qinjian@cqplus1.com>
> ---
>  MAINTAINERS                                  |   1 +
>  arch/arm/boot/dts/sunplus-sp7021-achip.dtsi  |  85 +++++
>  arch/arm/boot/dts/sunplus-sp7021-demo-v3.dts |  27 ++
>  arch/arm/boot/dts/sunplus-sp7021.dtsi        | 369 +++++++++++++++++++
>  4 files changed, 482 insertions(+)
>  create mode 100644 arch/arm/boot/dts/sunplus-sp7021-achip.dtsi
>  create mode 100644 arch/arm/boot/dts/sunplus-sp7021-demo-v3.dts
>  create mode 100644 arch/arm/boot/dts/sunplus-sp7021.dtsi
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9cf30e776..b55ec0768 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2747,6 +2747,7 @@ F:	Documentation/devicetree/bindings/arm/sunplus,sp7021.yaml
>  F:	Documentation/devicetree/bindings/clock/sunplus,sp7021-clkc.yaml
>  F:	Documentation/devicetree/bindings/interrupt-controller/sunplus,sp7021-intc.yaml
>  F:	Documentation/devicetree/bindings/reset/sunplus,reset.yaml
> +F:	arch/arm/boot/dts/sunplus-sp7021*.dts*
>  F:	arch/arm/configs/sp7021_*defconfig
>  F:	arch/arm/mach-sunplus/
>  F:	drivers/clk/clk-sp7021.c
> diff --git a/arch/arm/boot/dts/sunplus-sp7021-achip.dtsi b/arch/arm/boot/dts/sunplus-sp7021-achip.dtsi
> new file mode 100644
> index 000000000..1560c95d9
> --- /dev/null
> +++ b/arch/arm/boot/dts/sunplus-sp7021-achip.dtsi
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for Sunplus SP7021
> + *
> + * Copyright (C) 2021 Sunplus Technology Co.
> + */
> +
> +#include "sunplus-sp7021.dtsi"
> +
> +/ {
> +	compatible = "sunplus,sp7021-achip";

This does not match your bindings.

> +	model = "Sunplus SP7021 (CA7)";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	interrupt-parent = <&gic>;
> +
> +	clocks {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		extclk: clk@osc0 {

This is not a valid device tree. Please run make dtbs_check and compile
dtbs with W=1.

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <27000000>;
> +			clock-output-names = "extclk";
> +		};
> +
> +		divextclk: clk@0 {

How is it suppose to pass any automated checks if there is no unit address?

> +			compatible = "fixed-factor-clock";
> +			#clock-cells = <0>;
> +			clocks  = <&extclk>;
> +			clock-mult = <1>;
> +			clock-div = <2>;
> +			clock-output-names = "extdivclk";
> +		};
> +
> +		A_pll0: clk@A_pll0 {

This is not a valid device tree.

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <2000000000>;
> +			clock-output-names = "A_pll0";
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			compatible = "arm,cortex-a7";
> +			device_type = "cpu";
> +			reg = <0>;
> +			clock-frequency = <931000000>;
> +		};
> +		cpu1: cpu@1 {
> +			compatible = "arm,cortex-a7";
> +			device_type = "cpu";
> +			reg = <1>;
> +			clock-frequency = <931000000>;
> +		};
> +		cpu2: cpu@2 {
> +			compatible = "arm,cortex-a7";
> +			device_type = "cpu";
> +			reg = <2>;
> +			clock-frequency = <931000000>;
> +		};
> +		cpu3: cpu@3 {
> +			compatible = "arm,cortex-a7";
> +			device_type = "cpu";
> +			reg = <3>;
> +			clock-frequency = <931000000>;
> +		};
> +	};
> +
> +	arm-pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupts = <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 220 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 222 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
> +	};
> +
> +};
> diff --git a/arch/arm/boot/dts/sunplus-sp7021-demo-v3.dts b/arch/arm/boot/dts/sunplus-sp7021-demo-v3.dts
> new file mode 100644
> index 000000000..05e164115
> --- /dev/null
> +++ b/arch/arm/boot/dts/sunplus-sp7021-demo-v3.dts
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for Sunplus SP7021 Demo V3 SBC board
> + *
> + * Copyright (C) Sunplus Technology Co.
> + */
> +
> +/dts-v1/;
> +
> +#include "sunplus-sp7021-achip.dtsi"
> +
> +/ {
> +	compatible = "sunplus,sp7021-demo-v3";

This does not match your bindings.

Please run make dtbs_check.

> +	model = "SP7021/CA7/Demo_V3";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	chosen {
> +		bootargs = "console=ttyS0,115200 loglevel=8 earlycon";

No bootargs.

I'll stop reviewing. This either does not compile, does not work or does
not pass automated checks. There is no point to use reviewers time if
the tools are doing the same job, so use the tools and then submit DTS.

Best regards,
Krzysztof