Re: [PATCH v3] drivers/coresight: Add Ultrasoc System Memory Buffer driver
From: liuqi (BA)
Date: Wed Nov 24 2021 - 02:12:53 EST
On 2021/11/24 2:30, Mathieu Poirier wrote:
Good morning,
On Thu, Nov 18, 2021 at 07:00:16PM +0800, Qi Liu wrote:
This patch adds driver for Ultrasoc SMB(System Memory Buffer)
device. SMB provides a way to buffer messages from ETM, and
store these CPU instructions in system memory.
SMB is developed by Ultrasoc technology, which is acquired by
Siemens, and we still use "Ultrasoc" to name driver.
Signed-off-by: Qi Liu <liuqi115@xxxxxxxxxx>
Tested-by: JunHao He <hejunhao2@xxxxxxxxxxxxx>
---
Change since v2:
- Move ultrasoc driver to drivers/hwtracing/coresight.
- Link:https://lists.linaro.org/pipermail/coresight/2021-November/007310.html
Change since v1:
- Drop the document of Ultrasoc according to Mathieu's comment.
- Add comments to explain some private hardware settings.
- Address the comments from Mathieu.
- Link: https://lists.linaro.org/pipermail/coresight/2021-August/006842.html
Change since RFC:
- Move ultrasoc driver to drivers/hwtracing/coresight/ultrasoc.
- Remove ultrasoc-axi-com.c, as AXI-COM doesn't need to be configured in
basic tracing function.
- Remove ultrasoc.c as SMB does not need to register with the ultrasoc core.
- Address the comments from Mathieu and Suzuki.
- Link: https://lists.linaro.org/pipermail/coresight/2021-June/006535.html
drivers/hwtracing/coresight/Kconfig | 12 +
drivers/hwtracing/coresight/Makefile | 2 +
drivers/hwtracing/coresight/ultrasoc-smb.c | 607 +++++++++++++++++++++
drivers/hwtracing/coresight/ultrasoc-smb.h | 116 ++++
4 files changed, 737 insertions(+)
create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h
This set is giving me obvious checkpatch warnings, something that is quite
disappointing since 1) you had plenty of time to fix those and 2) someone with
your kind of upstream experience should know better.
Despite the above I have decided to review this patch but next time I will simply
ignore your submission.
Hi Mathieu,
Sorry for my fault, I'll pay attention to these things latter, thanks.
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 514a9b8086e3..d24a5d95153a 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -201,4 +201,16 @@ config CORESIGHT_TRBE
To compile this driver as a module, choose M here: the module will be
called coresight-trbe.
+
+config ULTRASOC_SMB
+ tristate "Ultrasoc system memory buffer drivers"
+ depends on ARM64 && CORESIGHT
You don't need to depend on CORESIGHT since this is already enclosed in "if
CORESIGHT". But like other sink, this should depend on
CORESIGHT_LINKS_AND_SINKS.
Got it, will fix this.
+ help
+ This driver provides support for the Ultrasoc system memory buffer (SMB).
+ SMB is responsible for receiving the trace data from Coresight ETM devices
+ and storing them to a system buffer.
+
+ To compile this driver as a module, choose M here: the module will be
+ called ultrasoc-smb.
+
endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index b6c4a48140ec..2899e5a5d5cd 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -27,3 +27,5 @@ obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o
obj-$(CONFIG_CORESIGHT_TRBE) += coresight-trbe.o
coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \
coresight-cti-sysfs.o
+
Extra newline.
got it, thanks.
+obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
new file mode 100644
index 000000000000..b477fc51fbbb
--- /dev/null
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -0,0 +1,607 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright (C) 2021 HiSilicon Limited.
+ *
+ * Code herein communicates with and accesses proprietary hardware which is
+ * licensed intellectual property (IP) belonging to Siemens Digital Industries
+ * Software Ltd.
+ *
+ * Siemens Digital Industries Software Ltd. asserts and reserves all rights to
+ * their intellectual property. This paragraph may not be removed or modified
+ * in any way without permission from Siemens Digital Industries Software Ltd.
+ */
+
+#include <linux/acpi.h>
+#include <linux/circ_buf.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+
+#include "ultrasoc-smb.h"
+
+DEFINE_CORESIGHT_DEVLIST(sink_devs, "sink_smb");
DEFINE_CORESIGHT_DEVLIST(sink_devs, "smb");
got it, will change this.
More comments at the very bottom of the file.
+
[...]
+static const struct acpi_device_id ultrasoc_smb_acpi_match[] = {
+ {"HISI03A1", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(acpi, ultrasoc_smb_acpi_match);
Please enclose this in #ifdef CONFIG_ACPI as it is done for the funnel and
replicator drivers.
got it, thanks.
+
+static struct platform_driver smb_driver = {
+ .driver = {
+ .name = "ultrasoc,smb",
"ultrasoc-smb"
got it.
+ .acpi_match_table = ACPI_PTR(ultrasoc_smb_acpi_match),
+ .suppress_bind_attrs = true,
+ },
+ .probe = smb_probe,
+ .remove = smb_remove,
+};
+module_platform_driver(smb_driver);
+
+MODULE_DESCRIPTION("Ultrasoc smb driver");
+MODULE_LICENSE("Dual MIT/GPL");
The dual licence is not reflected properly in the SPDX tag at the top of the
file. Please read Documentation/process/license-rules.rst.
I'll fix this next version, thanks.
+MODULE_AUTHOR("Jonathan Zhou <jonathan.zhouwen@xxxxxxxxxx>");
+MODULE_AUTHOR("Qi Liu <liuqi115@xxxxxxxxxx>");
[...]
+/**
+ * struct smb_drv_data - specifics associated to an SMB component
+ * @base: Memory mapped base address for SMB component.
+ * @csdev: Component vitals needed by the framework.
+ * @sdb: Data buffer for SMB.
+ * @miscdev: Specifics to handle "/dev/xyz.tmc" entry.
What tmc entry would this be?
here is a typo, misdev should be an entry to handle /dev/smbx, will fix
this, thanks.
+ * @spinlock: Only one at a time pls.
+ * @reading: Synchronise user space access to SMB buffer.
+ * @pid: Process ID of the process being monitored by the session
+ * that is using this component.
+ * @mode: how this SMB is being used, perf mode or sysfs mode.
+ */
+struct smb_drv_data {
+ void __iomem *base;
+ struct coresight_device *csdev;
+ struct smb_data_buffer sdb;
+ struct miscdevice miscdev;
+ spinlock_t spinlock;
+ local_t reading;
+ pid_t pid;
+ u32 mode;
+};
+
+#define smb_simple_func(type, name, offset) \
+static ssize_t name##_show(struct device *_dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ type *drvdata = dev_get_drvdata(_dev->parent); \
+ u32 val = readl(drvdata->base + offset); \
+ \
+ return sysfs_emit(buf, "0x%x\n", val); \
+} \
+static DEVICE_ATTR_RO(name)
I'm pretty sure you can re-use coresight_tmc_reg() instead of writing
another macro. I suggest to look at coresight_tmc_reg() in coresight-tmc-core.c
for an example.
got it, I've check coresight_tmc_reg and will use this next version.
I am out of time for today - I will continue tomorrow.
Thanks for reviewing this patch.
Qi
Thanks,
Mathieu
+
+#endif
--
2.33.0
.