Re: [PATCH v3 01/10] dt-bindings: npu: rockchip,rknn: Add bindings

Krzysztof Kozlowski
2025-05-19T06:14:29Z

On 16/05/2025 18:53, Tomeu Vizoso wrote:
> Add the bindings for the Neural Processing Unit IP from Rockchip.
>
> v2:
> - Adapt to new node structure (one node per core, each with its own
> IOMMU)
> - Several misc. fixes from Sebastian Reichel
>
> v3:
> - Split register block in its constituent subblocks, and only require
> the ones that the kernel would ever use (Nicolas Frattaroli)
> - Group supplies (Rob Herring)
> - Explain the way in which the top core is special (Rob Herring)
>
> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> .../bindings/npu/rockchip,rknn-core.yaml | 162 +++++++++++++++++++++

Nothing here was tested, so limited review.

> 1 file changed, 162 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..4572fb777f1454d0147da29791033fc27c53b8d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml
> @@ -0,0 +1,162 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Neural Processing Unit IP from Rockchip
> +
> +maintainers:
> + - Tomeu Vizoso <tomeu@tomeuvizoso.net>
> +
> +description:
> + Rockchip IP for accelerating inference of neural networks, based on NVIDIA's
> + open source NVDLA IP.
> +
> + There is to be a node per each core in the NPU. In Rockchip's design there
> + will be one core that is special and needs to be powered on before any of the
> + other cores can be used. This special core is called the top core and should
> + have the compatible string that corresponds to top cores.
> +
> +properties:
> + $nodename:
> + pattern: '^npu-core@[a-f0-9]+$'
> +
> + compatible:
> + oneOf:
> + - items:

Drop, just enum

> + - enum:
> + - rockchip,rk3588-rknn-core-top
> + - items:

Drop

> + - enum:

Drop, part of previous enum.

> + - rockchip,rk3588-rknn-core
> +
> + reg:
> + minItems: 3

No, maxItems instead

> +
> + reg-names:
> + minItems: 3

No, drop, makes no sense.

> + items:
> + - const: pc
> + - const: cna
> + - const: core
> +
> + clocks:
> + minItems: 2
> + maxItems: 4
> +
> + clock-names:
> + items:
> + - const: aclk
> + - const: hclk
> + - const: npu
> + - const: pclk
> + minItems: 2
> +
> + interrupts:
> + maxItems: 1
> +
> + iommus:
> + maxItems: 1
> +
> + npu-supply: true
> +
> + power-domains:
> + maxItems: 1
> +
> + resets:
> + maxItems: 2
> +
> + reset-names:
> + items:
> + - const: srst_a
> + - const: srst_h
> +
> + sram-supply: true
> +
> +required:
> + - compatible
> + - reg

reg-names

> + - clocks
> + - clock-names
> + - interrupts
> + - iommus
> + - power-domains
> + - resets
> + - reset-names
> + - npu-supply
> + - sram-supply
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - rockchip,rknn-core-top
> + then:
> + properties:
> + clocks:
> + minItems: 4
> +
> + clock-names:
> + minItems: 4
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - rockchip,rknn-core
> + then:
> + properties:
> + clocks:
> + maxItems: 2
> + clock-names:
> + maxItems: 2
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/power/rk3588-power.h>
> + #include <dt-bindings/reset/rockchip,rk3588-cru.h>
> +
> + bus {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + rknn_core_top: npu-core@fdab0000 {

Drop unused label
npu@

> + compatible = "rockchip,rk3588-rknn-core-top", "rockchip,rknn-core-top";

This makes no sense... If you change compatibles you must change example
DTS and your DTS.

> + reg = <0x0 0xfdab0000 0x0 0x9000>;
> + assigned-clocks = <&scmi_clk SCMI_CLK_NPU>;
> + assigned-clock-rates = <200000000>;
> + clocks = <&cru ACLK_NPU0>, <&cru HCLK_NPU0>,
> + <&scmi_clk SCMI_CLK_NPU>, <&cru PCLK_NPU_ROOT>;
> + clock-names = "aclk", "hclk", "npu", "pclk";
> + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH 0>;
> + iommus = <&rknn_mmu_top>;
> + npu-supply = <&vdd_npu_s0>;
> + power-domains = <&power RK3588_PD_NPUTOP>;
> + resets = <&cru SRST_A_RKNN0>, <&cru SRST_H_RKNN0>;
> + reset-names = "srst_a", "srst_h";
> + sram-supply = <&vdd_npu_mem_s0>;
> + };
> +
> + rknn_core_1: npu-core@fdac0000 {
> + compatible = "rockchip,rk3588-rknn-core", "rockchip,rknn-core";
> + reg = <0x0 0xfdac0000 0x0 0x9000>;
> + clocks = <&cru ACLK_NPU1>, <&cru HCLK_NPU1>;

Difference by one property does not really justify new example. Keep
only one, which would halve the errors you have here. :/

Best regards,
Krzysztof