Re: [PATCH 2/3] arm64: dts: freescale: add i.MX95 basic dtsi

From: Krzysztof Kozlowski
Date: Mon Feb 19 2024 - 03:21:38 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH 3/3] arm64: dts: freescale: add i.MX95 19x19 EVK minimal board dts"
Previous message: Weizhao Ouyang: "Re: [PATCH v2 2/2] dt-bindings: arm: rockchip: Add Toybrick TB-RK3588X"
In reply to: Peng Fan (OSS): "[PATCH 2/3] arm64: dts: freescale: add i.MX95 basic dtsi"
Next in thread: Peng Fan (OSS): "[PATCH 3/3] arm64: dts: freescale: add i.MX95 19x19 EVK minimal board dts"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 18/02/2024 07:38, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> i.MX95 features 6 A55 Cores, ARM Mali GPU, ISP, ML acceleration NPU,
> and Edgelock secure enclave security. This patch is to add a minimal
> dtsi, with cpu cores, coresight, scmi, gic, uart, mu, sdhc, lpi2c added.
>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
> arch/arm64/boot/dts/freescale/imx95-clock.h | 187 +++++
> arch/arm64/boot/dts/freescale/imx95-power.h | 55 ++
> arch/arm64/boot/dts/freescale/imx95.dtsi | 1215 +++++++++++++++++++++++++++
> 3 files changed, 1457 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx95-clock.h b/arch/arm64/boot/dts/freescale/imx95-clock.h
> new file mode 100644
> index 000000000000..939655f0414e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-clock.h

What is this?

> @@ -0,0 +1,187 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#ifndef __DT_BINDINGS_CLOCK_IMX95_H

NAK, bindings do not go to DTS!

> +#endif /* __DT_BINDINGS_CLOCK_IMX95_H */
> diff --git a/arch/arm64/boot/dts/freescale/imx95-power.h b/arch/arm64/boot/dts/freescale/imx95-power.h
> new file mode 100644
> index 000000000000..17be8a4cb96b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-power.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#ifndef __DT_BINDINGS_IMX95_POWER_H__
> +#define __DT_BINDINGS_IMX95_POWER_H__

NAK, bindings do not go to DTS.

> diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
> new file mode 100644
> index 000000000000..5b5b29a9aff5
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
> @@ -0,0 +1,1215 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

Weird license. We do not want GPL v3 or v4.

> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/thermal/thermal.h>
> +
> +#include "imx95-clock.h"
> +#include "imx95-power.h"
> +
> +/ {
> + interrupt-parent = <&gic>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + aliases {
> + gpio0 = &gpio1;
> + gpio1 = &gpio2;
> + gpio2 = &gpio3;
> + gpio3 = &gpio4;
> + gpio4 = &gpio5;
> + i2c0 = &lpi2c1;
> + i2c1 = &lpi2c2;
> + i2c2 = &lpi2c3;
> + i2c3 = &lpi2c4;
> + i2c4 = &lpi2c5;
> + i2c5 = &lpi2c6;
> + i2c6 = &lpi2c7;
> + i2c7 = &lpi2c8;

None of these are properties of the SoC.

> + mmc0 = &usdhc1;
> + mmc1 = &usdhc2;
> + mmc2 = &usdhc3;
> + serial0 = &lpuart1;
> + serial1 = &lpuart2;
> + serial2 = &lpuart3;
> + serial3 = &lpuart4;
> + serial4 = &lpuart5;
> + serial5 = &lpuart6;
> + serial6 = &lpuart7;
> + serial7 = &lpuart8;

Neither these.

> + };
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + idle-states {
> + entry-method = "psci";
> +
> + cpu_pd_wait: cpu-pd-wait {
> + compatible = "arm,idle-state";
> + arm,psci-suspend-param = <0x0010033>;
> + local-timer-stop;
> + entry-latency-us = <10000>;
> + exit-latency-us = <7000>;
> + min-residency-us = <27000>;
> + wakeup-latency-us = <15000>;
> + status = "disabled";
> + };
> + };
> +
> + A55_0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x0>;
> + enable-method = "psci";
> + #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_pd_wait>;
> + power-domains = <&scmi_devpd IMX95_PERF_A55>;
> + power-domain-names = "perf";
> + i-cache-size = <32768>;
> + i-cache-line-size = <64>;
> + i-cache-sets = <128>;
> + d-cache-size = <32768>;
> + d-cache-line-size = <64>;
> + d-cache-sets = <128>;
> + next-level-cache = <&l2_cache_l0>;
> + };
> +
> + A55_1: cpu@100 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x100>;
> + enable-method = "psci";
> + #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_pd_wait>;
> + power-domains = <&scmi_devpd IMX95_PERF_A55>;
> + power-domain-names = "perf";
> + i-cache-size = <32768>;
> + i-cache-line-size = <64>;
> + i-cache-sets = <128>;
> + d-cache-size = <32768>;
> + d-cache-line-size = <64>;
> + d-cache-sets = <128>;
> + next-level-cache = <&l2_cache_l1>;
> + };
> +
> + A55_2: cpu@200 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x200>;
> + enable-method = "psci";
> + #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_pd_wait>;
> + power-domains = <&scmi_devpd IMX95_PERF_A55>;
> + power-domain-names = "perf";
> + i-cache-size = <32768>;
> + i-cache-line-size = <64>;
> + i-cache-sets = <128>;
> + d-cache-size = <32768>;
> + d-cache-line-size = <64>;
> + d-cache-sets = <128>;
> + next-level-cache = <&l2_cache_l2>;
> + };
> +
> + A55_3: cpu@300 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x300>;
> + enable-method = "psci";
> + #cooling-cells = <2>;
> + power-domains = <&scmi_devpd IMX95_PERF_A55>;
> + power-domain-names = "perf";
> + i-cache-size = <32768>;
> + i-cache-line-size = <64>;
> + i-cache-sets = <128>;
> + d-cache-size = <32768>;
> + d-cache-line-size = <64>;
> + d-cache-sets = <128>;
> + next-level-cache = <&l2_cache_l3>;
> + };
> +
> + A55_4: cpu@400 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x400>;
> + power-domains = <&scmi_devpd IMX95_PERF_A55>;
> + power-domain-names = "perf";
> + enable-method = "psci";
> + #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_pd_wait>;
> + i-cache-size = <32768>;
> + i-cache-line-size = <64>;
> + i-cache-sets = <128>;
> + d-cache-size = <32768>;
> + d-cache-line-size = <64>;
> + d-cache-sets = <128>;
> + next-level-cache = <&l2_cache_l4>;
> + };
> +
> + A55_5: cpu@500 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x500>;
> + power-domains = <&scmi_devpd IMX95_PERF_A55>;
> + power-domain-names = "perf";
> + enable-method = "psci";
> + #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_pd_wait>;
> + i-cache-size = <32768>;
> + i-cache-line-size = <64>;
> + i-cache-sets = <128>;
> + d-cache-size = <32768>;
> + d-cache-line-size = <64>;
> + d-cache-sets = <128>;
> + next-level-cache = <&l2_cache_l5>;
> + };
> +
> + l2_cache_l0: l2-cache-l0 {
> + compatible = "cache";
> + cache-size = <65536>;
> + cache-line-size = <64>;
> + cache-sets = <256>;
> + cache-level = <2>;
> + cache-unified;
> + next-level-cache = <&l3_cache>;
> + };
> +
> + l2_cache_l1: l2-cache-l1 {
> + compatible = "cache";
> + cache-size = <65536>;
> + cache-line-size = <64>;
> + cache-sets = <256>;
> + cache-level = <2>;
> + cache-unified;
> + next-level-cache = <&l3_cache>;
> + };
> +
> + l2_cache_l2: l2-cache-l2 {
> + compatible = "cache";
> + cache-size = <65536>;
> + cache-line-size = <64>;
> + cache-sets = <256>;
> + cache-level = <2>;
> + cache-unified;
> + next-level-cache = <&l3_cache>;
> + };
> +
> + l2_cache_l3: l2-cache-l3 {
> + compatible = "cache";
> + cache-size = <65536>;
> + cache-line-size = <64>;
> + cache-sets = <256>;
> + cache-level = <2>;
> + cache-unified;
> + next-level-cache = <&l3_cache>;
> + };
> +
> + l2_cache_l4: l2-cache-l4 {
> + compatible = "cache";
> + cache-size = <65536>;
> + cache-line-size = <64>;
> + cache-sets = <256>;
> + cache-level = <2>;
> + cache-unified;
> + next-level-cache = <&l3_cache>;
> + };
> +
> + l2_cache_l5: l2-cache-l5 {
> + compatible = "cache";
> + cache-size = <65536>;
> + cache-line-size = <64>;
> + cache-sets = <256>;
> + cache-level = <2>;
> + cache-unified;
> + next-level-cache = <&l3_cache>;
> + };
> +
> + l3_cache: l3-cache {
> + compatible = "cache";
> + cache-size = <524288>;
> + cache-line-size = <64>;
> + cache-sets = <1024>;
> + cache-level = <3>;
> + cache-unified;
> + };
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&A55_0>;
> + };
> +
> + core1 {
> + cpu = <&A55_1>;
> + };
> +
> + core2 {
> + cpu = <&A55_2>;
> + };
> +
> + core3 {
> + cpu = <&A55_3>;
> + };
> +
> + core4 {
> + cpu = <&A55_4>;
> + };
> +
> + core5 {
> + cpu = <&A55_5>;
> + };
> + };
> + };
> + };
> +
> + clk_ext1: clock-ext1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <133000000>;
> + clock-output-names = "clk_ext1";
> + };
> +
> + dummy: clk-dummy {

Nothing explains why do you add dummy clocks. Dummy is fake, so don't
add fake clocks to DTS.

> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <0>;
> + clock-output-names = "dummy";
> + };
> +
> + sai1_mclk: sai-mclk1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency= <0>;
> + clock-output-names = "sai1_mclk";
> + };
> +
> + sai2_mclk: sai-mclk2 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency= <0>;
> + clock-output-names = "sai2_mclk";
> + };
> +
> + sai3_mclk: sai-mclk3 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency= <0>;
> + clock-output-names = "sai3_mclk";
> + };
> +
> + sai4_mclk: sai-mclk4 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency= <0>;
> + clock-output-names = "sai4_mclk";
> + };
> +
> + sai5_mclk: sai-mclk5 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency= <0>;
> + clock-output-names = "sai5_mclk";
> + };
> +
> + osc_24m: osc-24m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24000000>;
> + clock-output-names = "osc_24m";
> + };
> +
> + sram1: sram@20480000 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> + compatible = "mmio-sram";
> + reg = <0x0 0x204c0000 0x0 0x18000>;
> + };
> +
> + firmware {
> + scmi {
> + compatible = "arm,scmi";
> + mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> + shmem = <&scmi_buf0>, <&scmi_buf1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + scmi_devpd: protocol@11 {
> + reg = <0x11>;
> + #power-domain-cells = <1>;
> + };
> +
> + scmi_sys_power: protocol@12 {
> + reg = <0x12>;
> + };
> +
> + scmi_perf: protocol@13 {
> + reg = <0x13>;
> + #clock-cells = <1>;
> + #power-domain-cells = <1>;
> + };
> +
> + scmi_clk: protocol@14 {
> + reg = <0x14>;
> + #clock-cells = <1>;
> + };
> +
> + scmi_sensor: protocol@15 {
> + reg = <0x15>;
> + #thermal-sensor-cells = <1>;
> + };
> + };
> + };
> +
> + pmu {
> + compatible = "arm,cortex-a55-pmu";
> + interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_HIGH)>;
> + };
> +
> + thermal-zones {
> + a55 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> + polling-delay-passive = <250>;
> + polling-delay = <2000>;
> + thermal-sensors = <&scmi_sensor 1>;
> +
> + trips {
> + cpu_alert0: trip0 {
> + temperature = <85000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_crit0: trip1 {
> + temperature = <95000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
> +
> + cooling-maps {
> + map0 {
> + trip = <&cpu_alert0>;
> + cooling-device =
> + <&A55_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A55_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A55_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A55_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A55_4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&A55_5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> + };
> + };
> + };
> +

> + etm0: etm@40840000 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

I see you did not test it at all. No need to review. First fix all the
issues found by tooling.

This applies to all your future submissions as well. You must first run
your patches through tooling before asking people to review. Otherwise
it is huge waste of our time and quite disrespectful to that time...
> + compatible = "arm,coresight-etm4x", "arm,primecell";
> + reg = <0x0 0x40840000 0x0 0x10000>;
> + arm,primecell-periphid = <0xbb95d>;
> + cpu = <&A55_0>;
> + clocks = <&scmi_clk IMX95_CLK_A55PERIPH>;
> + clock-names = "apb_pclk";
> + status = "disabled";
> +

..

> +
> + elemu5: mailbox@47570000 {
> + compatible = "fsl,imx95-mu-ele";
> + reg = <0x0 0x47570000 0x0 0x10000>;
> + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> + #mbox-cells = <2>;
> + status = "disabled";
> + };
> +
> + v2x_mu: v2x-mu@47350000 {
> + compatible = "fsl,imx95-mu-v2x";
> + reg = <0x0 0x47350000 0x0 0x10000>;
> + interrupts = <GIC_SPI 255 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "tx", "rx";
> + #mbox-cells = <2>;
> + status = "okay";

Why? Drop. This applies everywhere.

Best regards,
Krzysztof

Next message: Krzysztof Kozlowski: "Re: [PATCH 3/3] arm64: dts: freescale: add i.MX95 19x19 EVK minimal board dts"
Previous message: Weizhao Ouyang: "Re: [PATCH v2 2/2] dt-bindings: arm: rockchip: Add Toybrick TB-RK3588X"
In reply to: Peng Fan (OSS): "[PATCH 2/3] arm64: dts: freescale: add i.MX95 basic dtsi"
Next in thread: Peng Fan (OSS): "[PATCH 3/3] arm64: dts: freescale: add i.MX95 19x19 EVK minimal board dts"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]