Re: [PATCH V1 2/6] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-03-11 04:49, Bjorn Andersson wrote:
On Wed 10 Mar 10:46 CST 2021, Souradeep Chowdhury wrote:

The DCC is a DMA Engine designed to capture and store data
during system crash or software triggers. The DCC operates
based on link list entries which provides it with data and
addresses and the function it needs to perform. These
functions are read, write and loop. Added the basic driver
in this patch which contains a probe method which instantiates
the resources needed by the driver. DCC has it's own SRAM which
needs to be instantiated at probe time as well.


So to summarize, the DCC will upon a crash copy the configured region
into the dcc-ram, where it can be retrieved either by dumping the memory
over USB or from sysfs on the next boot?

Replied by Sai


Signed-off-by: Souradeep Chowdhury <schowdhu@xxxxxxxxxxxxxx>
---
 drivers/soc/qcom/Kconfig  |   8 +
 drivers/soc/qcom/Makefile |   1 +
drivers/soc/qcom/dcc.c | 388 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 397 insertions(+)
 create mode 100644 drivers/soc/qcom/dcc.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79b568f..8819e0b 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -69,6 +69,14 @@ config QCOM_LLCC
 	  SDM845. This provides interfaces to clients that use the LLCC.
 	  Say yes here to enable LLCC slice driver.

+config QCOM_DCC
+ tristate "Qualcomm Technologies, Inc. Data Capture and Compare engine driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+ This option enables driver for Data Capture and Compare engine. DCC
+	  driver provides interface to configure DCC block and read back
+	  captured data from DCC's internal SRAM.
+
 config QCOM_KRYO_L2_ACCESSORS
 	bool
 	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ad675a6..1b00870 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -26,3 +26,4 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
 obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
 obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
+obj-$(CONFIG_QCOM_DCC) += dcc.o
diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
new file mode 100644
index 0000000..89816bf
--- /dev/null
+++ b/drivers/soc/qcom/dcc.c
@@ -0,0 +1,388 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define dcc_readl(drvdata, off)						\
+	readl(drvdata->base + dcc_offset_conv(drvdata, off))

This is only used in probe, please just inline it, and use (a) local
variable(s) to avoid the lengthy lines.

This has been used at a lot of places in the subsequent patch. I will be sharing the
combined patch in the next version for better clarity.


+
+/* DCC registers */
+#define DCC_HW_INFO			0x04
+#define DCC_LL_NUM_INFO			0x10
+
+#define DCC_MAP_LEVEL1			0x18
+#define DCC_MAP_LEVEL2			0x34
+#define DCC_MAP_LEVEL3			0x4C
+
+#define DCC_MAP_OFFSET1			0x10
+#define DCC_MAP_OFFSET2			0x18
+#define DCC_MAP_OFFSET3			0x1C
+#define DCC_MAP_OFFSET4			0x8
+
+#define DCC_FIX_LOOP_OFFSET		16
+#define DCC_VER_INFO_BIT		9
+
+#define DCC_MAX_LINK_LIST		8
+#define DCC_INVALID_LINK_LIST		GENMASK(7, 0)
+
+#define DCC_VER_MASK1			GENMASK(6, 0)
+#define DCC_VER_MASK2			GENMASK(5, 0)
+
+#define DCC_RD_MOD_WR_ADDR		0xC105E
+
+struct qcom_dcc_config {
+	const int dcc_ram_offset;
+};
+
+enum dcc_mem_map_ver {
+	DCC_MEM_MAP_VER1,

As these are just integers, I would suggest skipping them. If you really
like them I would like to see VER1 = 1, to make any future debugging
easier.

Ack


+	DCC_MEM_MAP_VER2,
+	DCC_MEM_MAP_VER3
+};
+
+enum dcc_descriptor_type {
+	DCC_ADDR_TYPE,
+	DCC_LOOP_TYPE,
+	DCC_READ_WRITE_TYPE,
+	DCC_WRITE_TYPE
+};
+
+struct dcc_config_entry {
+	u32				base;
+	u32				offset;
+	u32				len;
+	u32				index;
+	u32				loop_cnt;
+	u32				write_val;
+	u32				mask;
+	bool				apb_bus;
+	enum dcc_descriptor_type	desc_type;
+	struct list_head		list;
+};
+
+struct dcc_drvdata {
+	void __iomem		*base;
+	u32			reg_size;
+	struct device		*dev;
+	struct mutex		mutex;
+	void __iomem		*ram_base;
+	u32			ram_size;
+	u32			ram_offset;
+	enum dcc_mem_map_ver	mem_map_ver;
+	u32			ram_cfg;
+	u32			ram_start;
+	bool			*enable;
+	bool			*configured;
+	bool			interrupt_disable;
+	char			*sram_node;
+	struct cdev		sram_dev;
+	struct class		*sram_class;
+	struct list_head	*cfg_head;
+	u32			*nr_config;
+	u32			nr_link_list;
+	u8			curr_list;
+	u8			loopoff;
+};
+
+static u32 dcc_offset_conv(struct dcc_drvdata *drvdata, u32 off)

I don't see a reason for specifying that the parameter and return value
are 32-bit unsigned ints, please use a generic data type...Like size_t

Ack.

+{
+	if (drvdata->mem_map_ver == DCC_MEM_MAP_VER1) {
+		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL3)
+			return (off - DCC_MAP_OFFSET3);
+		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL2)
+			return (off - DCC_MAP_OFFSET2);
+		else if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL1)
+			return (off - DCC_MAP_OFFSET1);
+	} else if (drvdata->mem_map_ver == DCC_MEM_MAP_VER2) {
+		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL2)
+			return (off - DCC_MAP_OFFSET4);
+	}
+	return off;
+}
+
+static void dcc_config_reset(struct dcc_drvdata *drvdata)
+{
+	struct dcc_config_entry *entry, *temp;
+	int curr_list;
+
+	mutex_lock(&drvdata->mutex);
+
+ for (curr_list = 0; curr_list < drvdata->nr_link_list; curr_list++) {
+

Unnecessary newline.

Ack.


+		list_for_each_entry_safe(entry, temp,
+			&drvdata->cfg_head[curr_list], list) {
+			list_del(&entry->list);
+			devm_kfree(drvdata->dev, entry);
+			drvdata->nr_config[curr_list]--;
+		}
+	}
+	drvdata->ram_start = 0;
+	drvdata->ram_cfg = 0;
+	mutex_unlock(&drvdata->mutex);
+}
+
+static int dcc_sram_open(struct inode *inode, struct file *file)
+{
+	struct dcc_drvdata *drvdata = container_of(inode->i_cdev,
+		struct dcc_drvdata,
+		sram_dev);
+	file->private_data = drvdata;
+
+	return	0;
+}
+
+static ssize_t dcc_sram_read(struct file *file, char __user *data,
+						size_t len, loff_t *ppos)
+{
+	unsigned char *buf;
+	struct dcc_drvdata *drvdata = file->private_data;
+
+	/* EOF check */
+	if (drvdata->ram_size <= *ppos)

"Is ppos beyond the EOF" is better expressed as:

	if (*ppos >= drvdata->ram_size)


Ack

+		return 0;
+
+	if ((*ppos + len) > drvdata->ram_size)
+		len = (drvdata->ram_size - *ppos);
+
+	buf = kzalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	memcpy_fromio(buf, (drvdata->ram_base + *ppos), len);

Parenthesis are unnecessary.

Ack



+
+	if (copy_to_user(data, buf, len)) {
+		dev_err(drvdata->dev, "DCC: Couldn't copy all data to user\n");
+		kfree(buf);
+		return -EFAULT;
+	}
+
+	*ppos += len;
+
+	kfree(buf);
+
+	return len;
+}
+
+static const struct file_operations dcc_sram_fops = {
+	.owner		= THIS_MODULE,
+	.open		= dcc_sram_open,
+	.read		= dcc_sram_read,
+	.llseek		= no_llseek,
+};
+
+static int dcc_sram_dev_register(struct dcc_drvdata *drvdata)
+{
+	int ret;
+	struct device *device;
+	dev_t dev;
+
+	ret = alloc_chrdev_region(&dev, 0, 1, drvdata->sram_node);
+	if (ret)
+		goto err_alloc;
+
+	cdev_init(&drvdata->sram_dev, &dcc_sram_fops);

How about implementing this using pstore instead of exposing it through
a custom /dev/dcc_sram (if I read the code correclty)

Replied by Sai


+
+	drvdata->sram_dev.owner = THIS_MODULE;
+	ret = cdev_add(&drvdata->sram_dev, dev, 1);
+	if (ret)
+		goto err_cdev_add;
+
+	drvdata->sram_class = class_create(THIS_MODULE, drvdata->sram_node);
+	if (IS_ERR(drvdata->sram_class)) {
+		ret = PTR_ERR(drvdata->sram_class);
+		goto err_class_create;
+	}
+
+	device = device_create(drvdata->sram_class, NULL,
+						drvdata->sram_dev.dev, drvdata,
+						drvdata->sram_node);
+	if (IS_ERR(device)) {
+		ret = PTR_ERR(device);
+		goto err_dev_create;
+	}
+
+	return 0;
+err_dev_create:
+	class_destroy(drvdata->sram_class);
+err_class_create:
+	cdev_del(&drvdata->sram_dev);
+err_cdev_add:
+	unregister_chrdev_region(drvdata->sram_dev.dev, 1);
+err_alloc:
+	return ret;
+}
+
+static void dcc_sram_dev_deregister(struct dcc_drvdata *drvdata)
+{
+	device_destroy(drvdata->sram_class, drvdata->sram_dev.dev);
+	class_destroy(drvdata->sram_class);
+	cdev_del(&drvdata->sram_dev);
+	unregister_chrdev_region(drvdata->sram_dev.dev, 1);
+}
+
+static int dcc_sram_dev_init(struct dcc_drvdata *drvdata)
+{
+	int ret = 0;
+	size_t node_size;
+	char *node_name = "dcc_sram";
+	struct device *dev = drvdata->dev;
+
+	node_size = strlen(node_name) + 1;
+
+	drvdata->sram_node = devm_kzalloc(dev, node_size, GFP_KERNEL);

kzalloc + strlcpy can be replaced by kstrdup(), but that said...all this
does seems to be to copy a const string to the heap and lugging it
around. Use a define instead.

Ack


+	if (!drvdata->sram_node)
+		return -ENOMEM;
+
+	strlcpy(drvdata->sram_node, node_name, node_size);
+	ret = dcc_sram_dev_register(drvdata);
+	if (ret)
+		dev_err(drvdata->dev, "DCC: sram node not registered.\n");
+
+	return ret;
+}
+
+static void dcc_sram_dev_exit(struct dcc_drvdata *drvdata)
+{
+	dcc_sram_dev_deregister(drvdata);
+}
+
+static int dcc_probe(struct platform_device *pdev)
+{
+	int ret = 0, i;
+	struct device *dev = &pdev->dev;
+	struct dcc_drvdata *drvdata;

I think "dcc" would be a better name than "drvdata"...

Ack

+	struct resource *res;
+	const struct qcom_dcc_config *cfg;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &pdev->dev;
+	platform_set_drvdata(pdev, drvdata);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dcc-base");

platform_get_resource_byname() + devm_ioremap() is done in one go using
devm_platform_ioremap_resource_byname()

Ack


+	if (!res)
+		return -EINVAL;
+
+	drvdata->reg_size = resource_size(res);

reg_size is unused outside this assignment, no need to lug it around in
drvdata.

Ack


+	drvdata->base = devm_ioremap(dev, res->start, resource_size(res));

drvdata->base is only accessed from this function, use a local variable
instead.

drvdata->base has been used in the subsequent patch at other places.
Will share the combined patch in next version for better clarity.


+	if (!drvdata->base)
+		return -ENOMEM;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+							"dcc-ram-base");

Here you're actually carrying the resource size, so it makes sense.

But it's okay not to wrap this line.

Ack

+	if (!res)
+		return -EINVAL;
+
+	drvdata->ram_size = resource_size(res);
+ drvdata->ram_base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!drvdata->ram_base)
+		return -ENOMEM;
+	cfg = of_device_get_match_data(&pdev->dev);
+	drvdata->ram_offset = cfg->dcc_ram_offset;
+
+ if (FIELD_GET(BIT(DCC_VER_INFO_BIT), dcc_readl(drvdata, DCC_HW_INFO))) {
+		drvdata->mem_map_ver = DCC_MEM_MAP_VER3;
+		drvdata->nr_link_list = dcc_readl(drvdata, DCC_LL_NUM_INFO);
+		if (drvdata->nr_link_list == 0)
+			return	-EINVAL;

Replace the \t in the middle

Ack

+ } else if ((dcc_readl(drvdata, DCC_HW_INFO) & DCC_VER_MASK2) == DCC_VER_MASK2) {
+		drvdata->mem_map_ver = DCC_MEM_MAP_VER2;
+		drvdata->nr_link_list = dcc_readl(drvdata, DCC_LL_NUM_INFO);
+		if (drvdata->nr_link_list == 0)
+			return	-EINVAL;
+	} else {
+		drvdata->mem_map_ver = DCC_MEM_MAP_VER1;
+		drvdata->nr_link_list = DCC_MAX_LINK_LIST;
+	}
+
+	if ((dcc_readl(drvdata, DCC_HW_INFO) & BIT(6)) == BIT(6))
+		drvdata->loopoff = DCC_FIX_LOOP_OFFSET;
+	else
+		drvdata->loopoff = get_bitmask_order((drvdata->ram_size +
+				drvdata->ram_offset) / 4 - 1);
+	mutex_init(&drvdata->mutex);
+
+	drvdata->enable = devm_kzalloc(dev, drvdata->nr_link_list *
+			sizeof(bool), GFP_KERNEL);

kzalloc(, items * sizeof(), ...) is kcalloc()

Ack

+	if (!drvdata->enable)
+		return -ENOMEM;

Add a newline between each check and the subsequent allocation, or do
all the allocation then do a

Ack

	if (!dcc->enable || !dcc->configured ...)
		return -ENOMEM;

+	drvdata->configured = devm_kzalloc(dev, drvdata->nr_link_list *
+			sizeof(bool), GFP_KERNEL);
+	if (!drvdata->configured)
+		return -ENOMEM;
+	drvdata->nr_config = devm_kzalloc(dev, drvdata->nr_link_list *
+			sizeof(u32), GFP_KERNEL);
+	if (!drvdata->nr_config)
+		return -ENOMEM;
+	drvdata->cfg_head = devm_kzalloc(dev, drvdata->nr_link_list *
+			sizeof(struct list_head), GFP_KERNEL);
+	if (!drvdata->cfg_head)
+		return -ENOMEM;
+
+	for (i = 0; i < drvdata->nr_link_list; i++) {
+		INIT_LIST_HEAD(&drvdata->cfg_head[i]);
+		drvdata->nr_config[i] = 0;

kzalloc() already initialized nr_config.

Ack

+	}
+
+	memset_io(drvdata->ram_base, 0, drvdata->ram_size);
+
+	drvdata->curr_list = DCC_INVALID_LINK_LIST;
+
+	ret = dcc_sram_dev_init(drvdata);
+	if (ret)
+		goto err;
+
+	return ret;

We know ret is 0 here, but if you rename "err" to "out" and move it
above this line you can reuse return.

Ack


+err:
+	return ret;
+}
+
+static int dcc_remove(struct platform_device *pdev)
+{
+	struct dcc_drvdata *drvdata = platform_get_drvdata(pdev);
+
+	dcc_sram_dev_exit(drvdata);
+
+	dcc_config_reset(drvdata);
+
+	return 0;
+}
+
+static const struct qcom_dcc_config sm8150_cfg = {
+	.dcc_ram_offset                         = 0x5000,
+};
+
+static const struct of_device_id dcc_match_table[] = {
+	{ .compatible = "qcom,sm8150-dcc", .data = &sm8150_cfg },
+};

MODULE_DEVICE_TABLE(of, dcc_match_table);

Ack


+
+static struct platform_driver dcc_driver = {
+	.probe					= dcc_probe,

The indentation here is excessive, feel free to use a single space.

+	.remove					= dcc_remove,
+	.driver					= {
+		.name		= "msm-dcc",

We tend to not use "msm" anymore, so how about "qcom-dcc"?

Ack



PS. It's hard to comment on some of the things in this patch, as they
are not used until the next patch...

Ack. As mentioned above I will be sharing the combined patch in the next version for better clarity.


Regards,
Bjorn

+		.of_match_table	= dcc_match_table,
+	},
+};
+
+module_platform_driver(dcc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. DCC driver");
+
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux