Re: [PATCH 1/9] trinity: Add base driver
From: Dafna Hirschfeld
Date: Thu Sep 01 2022 - 15:05:05 EST
On 27.07.2022 15:22, Greg KH wrote:
On Mon, Jul 25, 2022 at 03:53:00PM +0900, Jiho Chu wrote:
It contains the base codes for trinity driver. Minimal codes to load and
probe device is provided. The Trinity Family is controlled by the
Memory-Mapped Registers, the register addresses and offsets are
described. And user api interfaces are presented to control device under
ioctl manner.
Signed-off-by: Jiho Chu <jiho.chu@xxxxxxxxxxx>
Signed-off-by: yelini-jeong <yelini.jeong@xxxxxxxxxxx>
Signed-off-by: Dongju Chae <dongju.chae@xxxxxxxxxxx>
Signed-off-by: Parichay Kapoor <pk.kapoor@xxxxxxxxxxx>
Signed-off-by: Wook Song <wook16.song@xxxxxxxxxxx>
Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
---
drivers/misc/Kconfig | 1 +
drivers/misc/Makefile | 1 +
drivers/misc/trinity/Kconfig | 27 ++
drivers/misc/trinity/Makefile | 7 +
drivers/misc/trinity/trinity.c | 369 ++++++++++++++
drivers/misc/trinity/trinity_common.h | 392 +++++++++++++++
drivers/misc/trinity/trinity_vision2_drv.c | 512 ++++++++++++++++++++
drivers/misc/trinity/trinity_vision2_regs.h | 210 ++++++++
include/uapi/misc/trinity.h | 458 +++++++++++++++++
9 files changed, 1977 insertions(+)
create mode 100644 drivers/misc/trinity/Kconfig
create mode 100644 drivers/misc/trinity/Makefile
create mode 100644 drivers/misc/trinity/trinity.c
create mode 100644 drivers/misc/trinity/trinity_common.h
create mode 100644 drivers/misc/trinity/trinity_vision2_drv.c
create mode 100644 drivers/misc/trinity/trinity_vision2_regs.h
create mode 100644 include/uapi/misc/trinity.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 41d2bb0ae23a..ad0d5f6af291 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -500,4 +500,5 @@ source "drivers/misc/cardreader/Kconfig"
source "drivers/misc/habanalabs/Kconfig"
source "drivers/misc/uacce/Kconfig"
source "drivers/misc/pvpanic/Kconfig"
+source "drivers/misc/trinity/Kconfig"
endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 70e800e9127f..c63f3fc89780 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
obj-$(CONFIG_OPEN_DICE) += open-dice.o
+obj-$(CONFIG_TRINITY) += trinity/
diff --git a/drivers/misc/trinity/Kconfig b/drivers/misc/trinity/Kconfig
new file mode 100644
index 000000000000..ad4bab78f7c6
--- /dev/null
+++ b/drivers/misc/trinity/Kconfig
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config TRINITY
+ bool "Samsung Neural Processing Unit"
+ depends on HAS_IOMEM
+ depends on HAS_DMA
+ default n
The default is 'n', no need to ever say it again.
+ help
+ Select this option to enable driver support for Samsung
+ Neural Processing Unit (NPU).
+
+ This driver works as a base driver of the other drivers
+ for Trinity device family.
+
+ This option should be enabled to support Trinity
+ Vision 2 (TRIV2), and Trinity Audio (TRIA).
+
+config TRINITY_VISION2
+ tristate "Samsung NPU Trinity Vision 2"
What happened to "vision 1"?
+ depends on TRINITY
+ default n
+ help
+ Select this option to enable driver support for a Samsung
+ Neural Processing Unit (NPU), Tinity Vision 2.
+
+ This driver enables userspace system library to access the
+ device via /dev/triv2-N.
What is the module name?
Where is the userspace library code that talks to this? Any
documentation for this interface anywhere?
+#define BASE_DEV_NAME "trinity"
KBUILD_MODNAME?
+/* A global lock for shared static variables such as dev_bitmap */
+static DEFINE_SPINLOCK(trinity_lock);
That's a sign something is wrong, you should not need any module-wide
code variables.
+/* A bitmap to keep track of active Trinity devices */
+static unsigned long dev_bitmap[TRINITY_DEV_END];
Should not be needed, use a simple ida structure if you really want to
name things cleanly.
+
+/**
+ * trinity_release() - A common callback for close() in file_operations for a
+ * Trinity device node. If there are device-specific data to be
+ * cleaned-up, it is required to clean them up before invoke this
+ * callback.
+ *
+ * @inode: Inode to be closed
+ * @file: File to be closed
+ *
+ * Returns 0 on success. Otherwise, returns negative error.
+ */
+int trinity_release(struct inode *inode, struct file *file)
+{
+ struct trinity_driver *drv;
+
+ drv = file->private_data;
+
+ if (drv->verbose)
+ dev_info(drv_to_dev_ptr(drv), "%s\n", "Device closed");
+
+ mutex_lock(&drv->lock);
+ drv->opened = drv->opened - 1;
That will never work, you can't keep track of open/close calls.
Hi, can you explain why this will not work?
Thanks,
Dafna
+ if (drv->opened == 0) {
+ /* wait already submitted requests */
+ if (drv->desc->drain_reqs)
+ drv->desc->drain_reqs(drv);
+
+ drv->desc->set_state(drv, TRINITY_STATE_PAUSE);
+ }
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static bool trinity_is_empty(void)
+{
+ enum trinity_dev_type type;
+ bool empty = true;
+
+ spin_lock(&trinity_lock);
+ for (type = TRINITY_DEV_UNKNOWN, type++; type < TRINITY_DEV_END;
+ type++) {
+ if (find_first_bit(&dev_bitmap[type], TRINITY_DEV_EACH_MAX) !=
+ TRINITY_DEV_EACH_MAX) {
+ empty = false;
+ break;
+ }
+ }
+ spin_unlock(&trinity_lock);
+
+ return empty;
+}
+
+/**
+ * trinity_wait_ready() - Wait until trinity is ready state
+ *
+ * @drv: an instance of trinity driver
+ *
+ * Returns 0 on success. Otherwise, returns negative error.
+ */
+int trinity_wait_ready(struct trinity_driver *drv)
+{
+ const unsigned long time_out = HZ / 100UL; /* 1/100 seconds*/
+ const unsigned int max_retry = 10;
+ unsigned int retry = 0;
+ wait_queue_head_t wq;
+
+ drv->desc->set_state(drv, TRINITY_STATE_READY);
+
+ init_waitqueue_head(&wq);
+ /* try to ensure that NPU is in the ready state */
+ while (wait_event_timeout(
+ wq, drv->desc->get_state(drv) == TRINITY_STATE_READY,
+ time_out) == 0) {
+ /* regarded as failure */
+ if (retry == max_retry)
+ return -ETIMEDOUT;
+ retry++;
+ }
+
+ return 0;
+}
+
+/**
+ * trinity_open() - A common callback for open() in file_operations for a Trinity
+ * device node. If device-specific open() is required, this
+ * callback should be invoked by that open().
+ *
+ * @inode: inode to be opened
+ * @f: file to be opened
+ *
+ * Returns 0 on success. Otherwise, returns negative error.
+ */
+int trinity_open(struct inode *inode, struct file *f)
+{
+ struct miscdevice *miscdev;
+ struct trinity_driver *drv;
+ int ret = 0;
+
+ miscdev = (struct miscdevice *)f->private_data;
Why the cast?
+ drv = container_of(miscdev, struct trinity_driver, mdev);
+ f->private_data = drv;
+
+ mutex_lock(&drv->lock);
+ /** remove PAUSE set on the CP of the NPU */
+ if (drv->opened == 0) {
+ ret = trinity_wait_ready(drv);
+ if (ret != 0)
+ goto out;
+ }
+ drv->opened = drv->opened + 1;
Again, trying to keep track of open/close calls will never work. Just
let the vfs handle that for you (you will note it does that already).
Your driver should never need to worry about it.
+
+ if (drv->verbose)
+ dev_info(drv_to_dev_ptr(drv), "%s\n", "Device opened");
+
+out:
+ mutex_unlock(&drv->lock);
+
+ return 0;
+}
+
+static void trinity_common_init(struct device *dev)
+{
+ if (!trinity_is_empty())
+ return;
+
+ /* Common init codes */
+}
Missing something?
+
+static void trinity_common_exit(void)
+{
+ if (!trinity_is_empty())
+ return;
+
+ /* Common deinit codes */
+}
+
Don't provide empty functions that do nothing please.
+static int trinity_set_device_id(struct trinity_driver *drv)
+{
+ const struct trinity_desc *desc = drv->desc;
+ struct device *dev = drv_to_dev_ptr(drv);
+ int err = -EEXIST;
+
+ spin_lock(&trinity_lock);
+ drv->dev_id =
+ find_first_zero_bit(&dev_bitmap[dev->id], TRINITY_DEV_EACH_MAX);
Again, use an ida structure please.
+ if (drv->dev_id < TRINITY_DEV_EACH_MAX) {
+ set_bit(drv->dev_id, &dev_bitmap[dev->id]);
+ err = 0;
+ }
+ spin_unlock(&trinity_lock);
+
+ if (err == 0) {
+ drv->name = devm_kasprintf(dev, GFP_KERNEL, "%s-%u", desc->type,
+ drv->dev_id);
+ err = IS_ERR_OR_NULL(drv->name) ? -ENOMEM : 0;
Spell out if statements, this just makes things hard to read. And you
just leaked a "bit" if this failed, so are you sure this was ever
tested?
+ }
+
+ return err;
+}
+
+int trinity_create_node(struct trinity_driver *drv)
+{
+ struct device *dev = drv_to_dev_ptr(drv);
+ int err;
+
+ /** register as a misc device */
+ drv->mdev.minor = MISC_DYNAMIC_MINOR;
+ drv->mdev.parent = NULL;
No parent device? Why not? What bus does this device live on? This is
a platform device lower on in this code, please use that, don't just
hang out there at the top of the device tree.
+ drv->mdev.name = drv->name;
+
+ err = misc_register(&drv->mdev);
+ if (err < 0)
+ dev_err(dev, "failed to register as a misc device");
+ else
+ dev_info(dev, "misc device created!");
Again, drivers are quiet if all goes well.
I stopped here.
Also, please remove the layers of abstraction you have in your
structures that you never use, but yet still define in this patch for
some reason...
thanks,
greg k-h