Re: [PATCH v2 1/7] dt-bindings: npu: rockchip,rknn: Add bindings

On Wed, May 14, 2025 at 7:50 PM Nicolas Frattaroli
[off-list ref] wrote:

On Wednesday, 14 May 2025 17:18:22 Central European Summer Time Tomeu Vizoso wrote:

quoted

Hi Nicolas,

Thanks for looking at this. Some thoughts below:

On Fri, Apr 25, 2025 at 8:50 PM Nicolas Frattaroli
[off-list ref] wrote:

quoted

On Tuesday, 25 February 2025 08:55:47 Central European Summer Time Tomeu Vizoso wrote:

quoted

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

Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
Signed-off-by: Sebastian Reichel <redacted>
---
 .../bindings/npu/rockchip,rknn-core.yaml           | 152 +++++++++++++++++++++
 1 file changed, 152 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..e8d0afe4a7d1c4f166cf13a9f4aa7c1901362a3f
--- /dev/null
+++ b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml

@@ -0,0 +1,152 @@
+# 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.
+
+properties:
+  $nodename:
+    pattern: '^npu-core@[a-f0-9]+$'
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - rockchip,rk3588-rknn-core-top
+          - const: rockchip,rknn-core-top
+      - items:
+          - enum:
+              - rockchip,rk3588-rknn-core
+          - const: rockchip,rknn-core
+
+  reg:
+    maxItems: 1

Hi Tomeu,

as you probably know, RK3576 has quite a similar NPU. This is why I'm currently
poking at this patch series. One of the differences I ran into was that the
IOMMU of each NPU core now sits within the reg address space range of the core
as described by the single reg item binding and assumed by the driver.

But this is not a difference, right?

It is. E.g. on RK3588, you use reg = <0x0 0xfdab0000 0x0 0x9000>; for
rknn_core_top, and rknn_mmu_top then sits at 0xfdab9000, which is just
outside the reg range of the rknn_core_top node. That means acquiring the
iomem as a resource succeeds for you, whereas for me it fails.

Ah, got it now, thanks.

quoted

quoted

This seemed weird to me at first, since I would've guessed the cores would be
exactly the same, but I noticed that they kind of still are; the RK3588's NPU
also has a "hole" between 0x2000 and 0x2fff on each core, which is where RK3576
put its IOMMU.

So this is the same in both RK3576 and RK3588, right?

Yes, both RK3576 and RK3588 have a hole in the same area. RK3562 also has
the same hole. RK3568 doesn't have the offsets for the individual parts of
the NPU in the TRM, making all the relative register offsets the TRM then
goes on to document completely pointless as it omits what those offsets
are based on, so we don't know if it has a hole there. I vaguely recall
that it has the IOMMU either before or past the global range (not sure if
I wrote these findings down anywhere?), so if it has a hole at 0x2000
then it's unused like on the RK3588. I don't have access to the RV1106
Part 2 TRM where the NPU is described, so I don't know whether that has a
hole there unless we dig into the downstream code.

quoted

quoted

This is some information I gleaned from the RK3588 TRM, specifically section
36.4.1 "Internal Address Mapping", which shows where each "part" of the NPU core
has its address space.

Right now we just represent this as a single reg item per core. I've played
with the idea of splitting this up into the distinct ranges the TRM lists and
giving each a reg-names entry, but this would require a major rework of the
driver from what I can tell, including to the auto-generated register header.

For now, my hack on RK3576 is to just ioremap the range defined by resource
start to resource end inside rocket manually if I get -EBUSY trying to ioremap
the resource proper. This is quite an ugly hack though, it means the IOMMU node
still has its address overlapping with another node in the DT, and it also means
we have an unavoidable error message printed into the kernel log. This is also
what the vendor driver seems to do.

What do you reckon is a reg setup in the binding that is both reasonable to
implement in the driver while accurately describing the hardware?

Guess we could go with some smaller granularity and have 3 register
areas per core, instead of 10:

- CORE: PC+CNA (0x0000 ~ 0x1fff)
- AUX: CORE+DPU+PPU+DDMA+SDMA (0x3000 ~ 0x9fff)
- GLOBAL (0xf000 ~ 0xf004)

So the IOMMU on all the known SoCs can have its own regmap. I have
chosen to call the first one CORE because these are the components
that are absolutely needed in any NPU that is oriented towards
convolutional networks (convolutions, basically). I have named the
second AUX because it contains hardware units that are optional and
are used to implement operations that may be common but that aren't as
computational expensive as convolutions and thus might be skipped in
lower-end versions of the IP.

What do you think?

I'm personally fine with this approach. I've floated a two-area approach
to Sebastian Reichel before who, as far as I can recall, expressed his
distaste for  it as it seemed like an arbitrary division. I do concur in
that, it seems very arbitrary, so it's hard to say whether the bindings
maintainers would let us get away with it if they get wind of it.
Unfortunately they are Cc'd on this E-Mail, so the cat is out of the bag
in this regard.

Actually, after thinking a bit more about it I'm leaning towards only
having the PC, CNA and CORE areas in the DT, as those are the only
ones that should be accessible from the CPU.

The registers for the other units should be set by the PC, as it reads
the command stream.

So three register areas that can be set to wherever Rockchip has
placed them, and we just ignore the others in the kernel, as we don't
have any business messing with them ourselves.

What do you think?

Thanks,

Tomeu

What speaks for the 3 register area split is that anything that brings
more holes and doubly mapped things into the AUX area is probably going
to be so radically different it'll ideally have its own binding anyway,
or needs more than just a compatible added to the binding.

I think as far as arbitrary splits goes, the one you propose is probably
the one most closely aligned with reality. Certain register areas do
seem like something they'd never move away from its corresponding
companion, whereas adding parts to the AUX area or removing from it is
probably going to be quite common. So it can essentially be treated as
the area where optional things will most likely land as you pointed out,
which then don't need more bindings fiddling to add those optional things
as explicitly named areas in the bindings as long as we treat it as just
one opaque area s far as the binding is concerned.

Also, unless there's some virtual combined sparse iomem API in the kernel
that I'm not aware of, that's probably the easiest path forward for the
driver as well.

quoted

Regards,

Tomeu

Kind regards,
Nicolas Frattaroli

quoted

quoted

The RK3568, which uses a similar NPU design has the IOMMU at an offset of 0xb000
from the core's start of PC, so probably after any core specifics but before the
global registers if I hazard a guess.

For those without access to the TRM: splitting this up into multiple reg items
per core precisely the way the TRM does it would result in no less than 10 reg
items on RK3588, if I count correctly.

Kind regards,
Nicolas Frattaroli