Re: [PATCH V1 2/6] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)
From: schowdhu
Date: Thu Mar 11 2021 - 05:07:54 EST
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