On 2/25/2025 12:55 AM, Tomeu Vizoso wrote:
diff --git a/Documentation/accel/rocket/index.rst b/Documentation/accel/rocket/index.rst
new file mode 100644
index 0000000000000000000000000000000000000000..ad33194dec0325d0dab362768fd349e8dc286970
--- /dev/null
+++ b/Documentation/accel/rocket/index.rst
@@ -0,0 +1,19 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=====================================
+ accel/rocket Rockchip NPU driver
+=====================================
+
+The accel/rocket driver supports the Neural Processing Units (NPUs) inside some Rockchip SoCs such as the RK3588. Rockchip calls it RKNN and sometimes RKNPU.
+
+This NPU is closely based on the NVDLA IP released by NVIDIA as open hardware in 2018, along with open source kernel and userspace drivers.
+
+The frontend unit in Rockchip's NPU though is completely different from that in the open source IP, so this kernel driver is specific to Rockchip's version.
+
+The hardware is described in chapter 36 in the RK3588 TRM.
+
+This driver just powers the hardware on and off, allocates and maps buffers to the device and submits jobs to the frontend unit. Everything else is done in userspace, as a Gallium driver that is part of the Mesa3D project.
Nit: name the specific Gallium driver here?
diff --git a/drivers/accel/rocket/Kconfig b/drivers/accel/rocket/Kconfig
new file mode 100644
index 0000000000000000000000000000000000000000..83a401129ab2dc2847ccc30c6364e8802f43648d
--- /dev/null
+++ b/drivers/accel/rocket/Kconfig
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: GPL-2.0
I'm curious, is there a specific reason for "GPL-2.0"? This tag is
depreciated and everywhere else in the accel subsystem "GPL-2.0-only" is
used.
Same comment for the Makefile and everything else in this patch.
+
+config DRM_ACCEL_ROCKET
+ tristate "Rocket (support for Rockchip NPUs)"
+ depends on DRM
+ depends on ARM64 || COMPILE_TEST
+ depends on MMU
+ select DRM_SCHED
+ select IOMMU_SUPPORT
+ select IOMMU_IO_PGTABLE_LPAE
+ select DRM_GEM_SHMEM_HELPER
+ help
+ Choose this option if you have a Rockchip SoC that contains a
+ compatible Neural Processing Unit (NPU), such as the RK3588. Called by
+ Rockchip either RKNN or RKNPU, it accelerates inference of neural
+ networks.
+
+ The interface exposed to userspace is described in
+ include/uapi/drm/rocket_accel.h and is used by the userspace driver in
+ Mesa3D.
Nit: name the specific Mesa driver here?
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called rocket.
diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
new file mode 100644
index 0000000000000000000000000000000000000000..09d966c826b5b1090a18cb24b3aa4aba286a12d4
--- /dev/null
+++ b/drivers/accel/rocket/rocket_core.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2024 Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx> */
+
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
This list of includes seems insufficient. You use PTR_ERR but don't have
the include for it (linux/err.h). Same for dev_err/dev_info. I'm
guessing this comment applies elsewhere.
+#include "rocket_core.h"
+#include "rocket_registers.h"
+
+static int rocket_clk_init(struct rocket_core *core)
+{
+ struct device *dev = core->dev;
+ int err;
+
+ core->a_clk = devm_clk_get(dev, "aclk");
+ if (IS_ERR(core->a_clk)) {
+ err = PTR_ERR(core->a_clk);
+ dev_err(dev, "devm_clk_get_enabled failed %d for core %d\n", err, core->index);
Do you think it would be useful to mention "aclk" in the message? Seems
like if this or the hclk get fails, you won't know just from the kernel log.
+ return err;
+ }
+
+ core->h_clk = devm_clk_get(dev, "hclk");
+ if (IS_ERR(core->h_clk)) {
+ err = PTR_ERR(core->h_clk);
+ dev_err(dev, "devm_clk_get_enabled failed %d for core %d\n", err, core->index);
+ clk_disable_unprepare(core->a_clk);
+ return err;
+ }
+
+ return 0;
+}
+
+int rocket_core_init(struct rocket_core *core)
+{
+ struct device *dev = core->dev;
+ struct platform_device *pdev = to_platform_device(dev);
+ uint32_t version;
+ int err = 0;
+
+ err = rocket_clk_init(core);
+ if (err) {
+ dev_err(dev, "clk init failed %d\n", err);
This feels redundant since rocket_clk_init() already printed an error
message with more details.
+ return err;
+ }
+
+ core->iomem = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(core->iomem))
+ return PTR_ERR(core->iomem);
+
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_set_autosuspend_delay(dev, 50); /* ~3 frames */
Frames? That feels like a rendering term, but this driver is not doing
rendering, right? Is there a better way to describe this?
+ pm_runtime_enable(dev);
+
+ err = pm_runtime_get_sync(dev);
+
+ version = rocket_read(core, REG_PC_VERSION);
+ version += rocket_read(core, REG_PC_VERSION_NUM) & 0xffff;
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ dev_info(dev, "Rockchip NPU core %d version: %d\n", core->index, version);
+
+ return 0;
+}
+
diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
new file mode 100644
index 0000000000000000000000000000000000000000..ce3b533f15c1011d8a7a23dd8132e907cc334c58
--- /dev/null
+++ b/drivers/accel/rocket/rocket_device.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2024 Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx> */
+
+#include <linux/clk.h>
+
+#include "rocket_device.h"
+
+int rocket_device_init(struct rocket_device *rdev)
+{
+ struct device *dev = rdev->cores[0].dev;
+ int err;
+
+ rdev->clk_npu = devm_clk_get(dev, "npu");
+ rdev->pclk = devm_clk_get(dev, "pclk");
Are these optional? It would appear that error handling is missing.
+
+ /* Initialize core 0 (top) */
+ err = rocket_core_init(&rdev->cores[0]);
+ if (err) {
+ rocket_device_fini(rdev);
+ return err;
+ }
+
+ return 0;
+}
+
+static void rocket_drm_unbind(struct device *dev)
+{
+ struct rocket_device *rdev = dev_get_drvdata(dev);
+ struct drm_device *ddev = &rdev->ddev;
+
+ drm_dev_unregister(ddev);
You allocated this with devm, shouldn't that make this unnecessary?
+
+ component_unbind_all(dev, rdev);
+
+ rocket_device_fini(rdev);
+}
