Add driver support for Data Capture and Compare Engine(DCC) for SM8150,SC7280,SC7180,SDM845

Documentation for Data Capture and Compare(DCC) device tree bindings in yaml format.

Reviewed-by Rob Herring <[email protected]>

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  .../devicetree/bindings/arm/msm/qcom,dcc.yaml      | 43 ++++++++++++++++++++++
  1 file changed, 43 insertions(+)

create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml

Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml new file +43 -0

new file mode 100644
index 0000000..b7a6619
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/msm/qcom,dcc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Data Capture and Compare
+
+maintainers:
+  - Souradeep Chowdhury <[email protected]>
+
+description: |
+    DCC (Data Capture and Compare) is a DMA engine which is used to save
+    configuration data or system memory contents during catastrophic failure
+    or SW trigger. DCC is used to capture and store data for debugging purpose
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,sm8150-dcc
+          - qcom,sc7280-dcc
+          - qcom,sc7180-dcc
+          - qcom,sdm845-dcc
+      - const: qcom,dcc
+
+  reg:
+    items:
+      - description: DCC base register region
+      - description: DCC RAM base register region
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    dma@10a2000{
+                compatible = "qcom,sm8150-dcc","qcom,dcc";
+                reg = <0x010a2000  0x1000>,
+                      <0x010ad000  0x2000>;
+    };

The DCC is a DMA Engine designed to capture and store data during system crash or software triggers.The DCC operates based on user inputs via the sysfs interface.The user gives addresses as inputs and these addresses are stored in the form of linkedlists.In case of a system crash or a manual software trigger by the user through the sysfs interface, the dcc captures and stores the values at these addresses. This patch contains the driver which has all the methods

pertaining to the sysfs interface, auxiliary functions to support all the four fundamental operations of dcc namely read, write, first read then write and loop.The probe method here instantiates all the resources necessary for dcc to operate mainly the dedicated dcc sram where it stores the values.The DCC driver can be used for debugging purposes without going for a reboot since it can perform manual triggers.

Also added the documentation for sysfs entries and explained the functionalities of each sysfs file that has been created for dcc.

The following is the justification of using sysfs interface over the other alternatives like ioctls

i) As can be seen from the sysfs attribute descriptions, most of it does basic hardware manipulations like dcc_enable, dcc_disable, config reset etc. As a result sysfs is preferred over ioctl as we just need to enter a 0 or 1.

ii) Existing similar debug hardwares are there for which drivers have been written using sysfs interface.One such example is the coresight-etm-trace driver.A closer analog can also be the watchdog subsystems though it is ioctls based.

Documentation/ABI/testing/sysfs-driver-dcc new file +10 -0

+
+What:           /sys/bus/platform/devices/.../config_write
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file allows user to write a value to the register
+		address given as argument.The values are entered in the
+		form of <register_address> <value>.The reason for this
+		feature of dcc is that for accessing certain registers
+		it is necessary to set some bits of soe other register.

drivers/soc/qcom/dcc.c new file +10 -0

new file mode 100644
index 0000000..daf4388
@@ -0,0 +1,1549 @@
+// 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>

drivers/soc/qcom/dcc.c new file +10 -0

+#define dcc_writel(drvdata, val, off)					\
+	writel((val), drvdata->base + dcc_offset_conv(drvdata, off))
+#define dcc_readl(drvdata, off)						\
+	readl(drvdata->base + dcc_offset_conv(drvdata, off))
+
+#define DCC_SRAM_NODE "dcc_sram"
+
+/* DCC registers */
+#define DCC_HW_INFO			0x04
+#define DCC_LL_NUM_INFO			0x10

drivers/soc/qcom/dcc.c new file +34 -0

+#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_READ			0
+#define DCC_WRITE			1
+#define DCC_LOOP			2
+#define DCC_READ_WRITE			3
+
+#define MAX_DCC_OFFSET			GENMASK(9, 2)
+#define MAX_DCC_LEN			GENMASK(6, 0)
+#define MAX_LOOP_CNT			GENMASK(7, 0)
+
+#define DCC_ADDR_DESCRIPTOR		0x00
+#define DCC_ADDR_LIMIT			27
+#define DCC_ADDR_OFF_RANGE		8
+#define DCC_ADDR_RANGE			GENMASK(31, 4)
+#define DCC_LOOP_DESCRIPTOR		BIT(30)
+#define DCC_RD_MOD_WR_DESCRIPTOR	BIT(31)
+#define DCC_LINK_DESCRIPTOR		GENMASK(31, 30)
+
+#define DCC_READ_IND			0x00
+#define DCC_WRITE_IND			(BIT(28))
+
+#define DCC_AHB_IND			0x00
+#define DCC_APB_IND			BIT(29)

drivers/soc/qcom/dcc.c new file +4 -0

+	int dcc_ram_offset;
+};
+
+enum dcc_descriptor_type {

drivers/soc/qcom/dcc.c new file +16 -0

+
+}
+
+static int dcc_read_status(struct dcc_drvdata *drvdata)
+{
+	int curr_list;
+	u32 bus_status;
+	u32 ll_cfg;
+	u32 tmp_ll_cfg;
+
+	for (curr_list = 0; curr_list < drvdata->nr_link_list; curr_list++) {
+		if (!drvdata->enable[curr_list])
+			continue;
+
+		bus_status = dcc_readl(drvdata, DCC_LL_BUS_ACCESS_STATUS(curr_list));
+

drivers/soc/qcom/dcc.c new file +12 -0

+			dcc_writel(drvdata, ll_cfg, DCC_LL_CFG(curr_list));
+			return -ENODATA;
+		}
+	}
+
+	return 0;
+}
+
+static int dcc_sw_trigger(struct dcc_drvdata *drvdata)
+{
+	int ret;
+	int curr_list;

drivers/soc/qcom/dcc.c new file +6 -0

+	}
+
+	if (!dcc_ready(drvdata)) {
+		dev_err(drvdata->dev,
+			"DCC is busy after receiving sw tigger.\n");
+		ret = -EBUSY;

drivers/soc/qcom/dcc.c new file +24 -0

+static int __dcc_ll_cfg(struct dcc_drvdata *drvdata, int curr_list)
+{
+	int ret = 0;
+	u32 total_len, pos;
+	struct dcc_config_entry *entry;
+	struct dcc_cfg_attr cfg;
+	struct dcc_cfg_loop_attr cfg_loop;
+
+	memset(&cfg, 0, sizeof(cfg));
+	memset(&cfg_loop, 0, sizeof(cfg_loop));
+	cfg.sram_offset = drvdata->ram_cfg * 4;
+	total_len = 0;
+
+	list_for_each_entry(entry, &drvdata->cfg_head[curr_list], list) {
+		switch (entry->desc_type) {
+		case DCC_READ_WRITE_TYPE:
+			ret = _dcc_ll_cfg_read_write(drvdata, entry, &cfg);
+			if (ret)
+				goto overstep;
+			break;
+
+		case DCC_LOOP_TYPE:
+			ret = _dcc_ll_cfg_loop(drvdata, entry, &cfg, &cfg_loop, &total_len);
+			if (ret)

drivers/soc/qcom/dcc.c new file +12 -0

+		ret = -EINVAL;
+		goto err;
+	}
+
+	dcc_enable = is_dcc_enabled(drvdata);
+
+	ret = scnprintf(buf, PAGE_SIZE, "%u\n",
+				(unsigned int)dcc_enable > +err:
+	mutex_unlock(&drvdata->mutex);
+	return ret;
+}
+

drivers/soc/qcom/dcc.c new file +28 -0

+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (nval == 3 && apb_bus != 0)
+		apb_bus = 1;
+
+	ret = dcc_add_write(drvdata, addr, write_val, apb_bus);
+	if (ret)
+		goto err;
+
+	mutex_unlock(&drvdata->mutex);
+	return size;
+err:
+	mutex_unlock(&drvdata->mutex);
+	return ret;
+}
+
+static DEVICE_ATTR_WO(config_write);
+
+static const struct device_attribute *dcc_attrs[] = {
+	&dev_attr_trigger,
+	&dev_attr_enable,
+	&dev_attr_config,
+	&dev_attr_config_reset,
+	&dev_attr_ready,
+	&dev_attr_interrupt_disable,
+	&dev_attr_loop,

drivers/soc/qcom/dcc.c new file +32 -0

+	for (i = 0; attrs[i] != NULL; i++) {
+		ret = device_create_file(dev, attrs[i]);
+		if (ret) {
+			dev_err(dev, "DCC: Couldn't create sysfs attribute: %s\n",
+				attrs[i]->attr.name);
+			break;
+		}
+	}
+	return ret;
+}
+
+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)
+		return 0;
+
+	if ((*ppos + len) > drvdata->ram_size)

drivers/soc/qcom/dcc.c new file +7 -0

+		kfree(buf);
+		return -EFAULT;
+	}
+
+	*ppos += len;
+
+	kfree(buf);

drivers/soc/qcom/dcc.c new file +73 -0

+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, DCC_SRAM_NODE);
+	if (ret)
+		goto err_alloc;
+
+	cdev_init(&drvdata->sram_dev, &dcc_sram_fops);
+
+	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, DCC_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,
+						DCC_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;
+
+	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)
+{
+	u32 val;
+	int ret = 0, i, enable_size, nr_config_size, cfg_head_size;
+	struct device *dev = &pdev->dev;
+	struct dcc_drvdata *dcc;
+	struct resource *res;

drivers/soc/qcom/dcc.c new file +4 -0

+	if (FIELD_GET(BIT(DCC_VER_INFO_BIT), val)) {
+		dcc->mem_map_ver = DCC_MEM_MAP_VER3;
+		dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
+		if (dcc->nr_link_list == 0)

drivers/soc/qcom/dcc.c new file +17 -0

+		dcc->loopoff = DCC_FIX_LOOP_OFFSET;
+	else
+		dcc->loopoff = get_bitmask_order((dcc->ram_size +
+				dcc->ram_offset) / 4 - 1);
+
+	mutex_init(&dcc->mutex);
+	/* Allocate space for all entries at once */
+	enable_size = dcc->nr_link_list * sizeof(bool);
+	nr_config_size = dcc->nr_link_list * sizeof(size_t);
+	cfg_head_size = dcc->nr_link_list * sizeof(struct list_head);
+
+	dcc->enable = devm_kzalloc(dev, enable_size + nr_config_size + cfg_head_size, GFP_KERNEL);
+	if (!dcc->enable)
+		return -ENOMEM;
+
+	dcc->nr_config  = (size_t *)(dcc->enable + dcc->nr_link_list);
+	dcc->cfg_head = (struct list_head *)(dcc->nr_config + dcc->nr_link_list);

drivers/soc/qcom/dcc.c new file +5 -0

+}
+
+static int dcc_remove(struct platform_device *pdev)
+{
+	struct dcc_drvdata *drvdata = platform_get_drvdata(pdev);

drivers/soc/qcom/dcc.c new file +37 -0

+};
+
+static const struct qcom_dcc_config sc7280_cfg = {
+	.dcc_ram_offset = 0x12000,
+};
+
+static const struct qcom_dcc_config sc7180_cfg = {
+	.dcc_ram_offset = 0x6000,
+};
+
+static const struct qcom_dcc_config sdm845_cfg = {
+	.dcc_ram_offset = 0x6000,
+};
+
+static const struct of_device_id dcc_match_table[] = {
+	{ .compatible = "qcom,sm8150-dcc", .data = &sm8150_cfg },
+	{ .compatible = "qcom,sc7280-dcc", .data = &sc7280_cfg },
+	{ .compatible = "qcom,sc7180-dcc", .data = &sc7180_cfg },
+	{ .compatible = "qcom,sdm845-dcc", .data = &sdm845_cfg },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, dcc_match_table);
+
+static struct platform_driver dcc_driver = {
+	.probe = dcc_probe,
+	.remove	= dcc_remove,
+	.driver	= {
+		.name = "qcom-dcc",
+		.of_match_table	= dcc_match_table,
+	},
+};
+
+module_platform_driver(dcc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. DCC driver");
+

Quoted 8 lines · Souradeep Chowdhury

The DCC is a DMA Engine designed to capture and store data during system crash or software triggers.The DCC operates based on user inputs via the sysfs interface.The user gives addresses as inputs and these addresses are stored in the form of linkedlists.In case of a system crash or a manual software trigger by the user through the sysfs interface, the dcc captures and stores the values at these addresses. This patch contains the driver which has all the methods

Quoted 8 lines

pertaining to the sysfs interface, auxiliary functions to support all the four fundamental operations of dcc namely read, write, first read then write and loop.The probe method here instantiates all the resources necessary for dcc to operate mainly the dedicated dcc sram where it stores the values.The DCC driver can be used for debugging purposes without going for a reboot since it can perform manual triggers.

I don't understand what you're trying to say with the last sentence above.

Quoted 3 lines

Also added the documentation for sysfs entries and explained the functionalities of each sysfs file that has been created for dcc.

Could these files all land in debugfs, at least initially? I guess if you get it right it's fine in sysfs, but unlike sysfs, debugfs is not considered a stable user space API.

Quoted 8 lines

The following is the justification of using sysfs interface over the other alternatives like ioctls

i) As can be seen from the sysfs attribute descriptions, most of it does basic hardware manipulations like dcc_enable, dcc_disable, config reset etc. As a result sysfs is preferred over ioctl as we just need to enter a 0 or 1.

Quoted 4 lines

ii) Existing similar debug hardwares are there for which drivers have been written using sysfs interface.One such example is the coresight-etm-trace driver.A closer analog can also be the watchdog subsystems though it is ioctls based.

Even if it eventually becomes a sysfs interface, it might be better to initially land it in debugfs.  (Maybe others disagree.)

Generally, it looks to me like this always reads memory in 32-bit units.  I might have missed it, but if that is not documented it should be.  Also, is it OK to provide an address that is not aligned on a 32-bit boundary?  I presume memory is interpreted as CPU endian; does that produce the most meaningful results if the offset isn't aligned?

More comments, below.

                    -Alex

Documentation/ABI/testing/sysfs-driver-dcc

Quoted 16 lines

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  Documentation/ABI/testing/sysfs-driver-dcc |  114 ++ drivers/soc/qcom/Kconfig                   |    8 + drivers/soc/qcom/Makefile                  |    1 + drivers/soc/qcom/dcc.c                     | 1549
++++++++++++++++++++++++++++

  4 files changed, 1672 insertions(+)

  create mode 100644 Documentation/ABI/testing/sysfs-driver-dcc create mode 100644 drivers/soc/qcom/dcc.c

Documentation/ABI/testing/sysfs-driver-dcc new file +0 -0

new file mode 100644
index 0000000..05d24f0

Throughout this file, add a space or two after each period.

Documentation/ABI/testing/sysfs-driver-dcc line 2

Quoted 3 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +2 -0

@@ -0,0 +1,114 @@
+What:           /sys/bus/platform/devices/.../trigger
+Date:           March 2021

Probably update these dates when the code is close to getting accepted.

Documentation/ABI/testing/sysfs-driver-dcc line 5

Quoted 4 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +3 -0

+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+        This is the sysfs interface for manual software

Try to reword each of the descriptions so they don't talk about "this sysfs interface."  Each one of these *is* describing a sysfs interface file, so that can be omitted from the description.  For example:

    This file is used to perform a manual trigger operation, causing all lists of memory operations to be executed.

Or something along those lines.

Documentation/ABI/testing/sysfs-driver-dcc line 17

Quoted 13 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +12 -0

+        triggers.The user can simply enter a 1 against
+        the sysfs file and enable a manual trigger.
+        Example:
+        echo  1 > /sys/bus/platform/devices/.../trigger
+
+What:           /sys/bus/platform/devices/.../enable
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+        This sysfs interface is used for enabling the
+        the dcc hardware.Without this being set to 1,
+        the dcc hardware ceases to function.

This could/should use a standard Boolean interface.  That is, parse the input in your code using kstrtobool() on the buffer passed to your store function.

Quoted 5 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +2 -0

+        Example:
+        echo  0 > /sys/bus/platform/devices/.../enable

Documentation/ABI/testing/sysfs-driver-dcc line 29

Quoted 10 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +10 -0

+        (disable interface)
+        echo  1 > /sys/bus/platform/devices/.../enable
+        (enable interface)
+
+What:           /sys/bus/platform/devices/.../config
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+        This is the most commonly used sysfs interface
+        file and this basically stores the addresses of

It doesn't matter if this is the "most commonly used" file. Just describe what it does.

If I understand it right, you write a memory offset and an (optional) number of consecutive memory addresses to be read.

Documentation/ABI/testing/sysfs-driver-dcc line 36

Quoted 8 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +7 -0

+        the registers which needs to be read in case of
+        a hardware crash or manual software triggers.
+        Example:
+        echo  0x80000010 10 > /sys/bus/platform/devices/../config
+        This specifies that 10 words starting from address
+        0x80000010 is to be read.In case there are no words to be
+        specified we can simply enter the address.

I don't know what "in case there are no words to be specified" means.  Does that just mean memory at exactly one offset is read is implied if a count is not specified?

Quoted 19 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +13 -0

+
+What:           /sys/bus/platform/devices/.../config_write
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+        This file allows user to write a value to the register
+        address given as argument.The values are entered in the
+        form of <register_address> <value>.The reason for this
+        feature of dcc is that for accessing certain registers
+        it is necessary to set some bits of soe other register.
+        That is achievable by giving DCC this privelege.
+        Example:
+        echo 0x80000000 0xFF > 

/sys/bus/platform/devices/.../config_write

Documentation/ABI/testing/sysfs-driver-dcc new file +3 -0

+
+What:           /sys/bus/platform/devices/.../config_reset
+Date:           March 2021

Documentation/ABI/testing/sysfs-driver-dcc line 56

Quoted 4 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +4 -0

+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+        This file is used to reset the configuration of
+        a dcc driver to the default configuration.

What does it mean to "reset the driver to the default"? Does that mean all lists of operations supplied previously are forgotten, and the lists become empty and disabled?

Documentation/ABI/testing/sysfs-driver-dcc line 63

Quoted 8 lines

+        Example:
+        echo  1 > /sys/bus/platform/devices/.../config_reset

Documentation/ABI/testing/sysfs-driver-dcc new file +5 -0

+
+What:           /sys/bus/platform/devices/.../loop
+Date:        March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:

Can looping be done for a write or read/modify/write operation, or is it only used for reads?

Now skipping ahead to the code.

Quoted 3 lines

+        This file is used to enter the loop count as dcc +        driver gives the option to loop multiple times on +        the same register and store the values for each

. . .

Quoted 22 lines

drivers/soc/qcom/dcc.c new file +16 -0

new file mode 100644
index 0000000..daf4388
@@ -0,0 +1,1549 @@
+// 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>

drivers/soc/qcom/dcc.c line 20

Quoted 4 lines

drivers/soc/qcom/dcc.c new file +4 -0

+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define TIMEOUT_US        5000

This timeout is only used when polling the STATUS register to determine when the DCC hardware is "ready".  Maybe the name could reflect that (STATUS_READY_TIMEOUT?).

I think the next two might be better implemented as functions. It would allow the compiler to do some additional type checking.

Quoted 12 lines

drivers/soc/qcom/dcc.c new file +10 -0

+#define dcc_writel(drvdata, val, off)                    \
+    writel((val), drvdata->base + dcc_offset_conv(drvdata, off))
+#define dcc_readl(drvdata, off)                        \
+    readl(drvdata->base + dcc_offset_conv(drvdata, off))
+
+#define DCC_SRAM_NODE "dcc_sram"
+
+/* DCC registers */
+#define DCC_HW_INFO            0x04
+#define DCC_LL_NUM_INFO            0x10

drivers/soc/qcom/dcc.c line 41

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +10 -0

+#define DCC_STATUS            0x1C
+#define DCC_LL_LOCK(m)            (0x34 + 0x80 * m)
+#define DCC_LL_CFG(m)            (0x38 + 0x80 * m)
+#define DCC_LL_BASE(m)            (0x3c + 0x80 * m)
+#define DCC_FD_BASE(m)            (0x40 + 0x80 * m)
+#define DCC_LL_TIMEOUT(m)        (0x44 + 0x80 * m)
+#define DCC_LL_INT_ENABLE(m)        (0x4C + 0x80 * m)
+#define DCC_LL_INT_STATUS(m)        (0x50 + 0x80 * m)
+#define DCC_LL_SW_TRIGGER(m)        (0x60 + 0x80 * m)
+#define DCC_LL_BUS_ACCESS_STATUS(m)    (0x64 + 0x80 * m)

The MAP_LEVEL and MAP_OFFSET definitions below, together with the function dcc_offset_conv(), are a clever way to have the above set of offsets "work" by mapping them to (at least) three distinct "actual" addresses.  That is:

    if (map_ver == 1)

        if (0 <= offset < MAP_LEVEL_1)

            actual_offset = offset

        else if (MAP_LEVEL_1 <= offset < MAP_LEVEL_2)

            actual_offset = offset - MAP_OFFSET_1

        else if (MAP_LEVEL_2 <= offset < MAP_LEVEL_3)

            actual_offset = offset - MAP_OFFSET_2

        else

            actual_offset = offset - MAP_OFFSET_3

    else
 if (map_ver == 2)
        if (0 <= offset < MAP_LEVEL_1)

            actual_offset = offset

        else

            actual_offset = offset - MAP_OFFSET_4

    else
            actual_offset = offset

This is a bit messy, but other ways of doing it aren't likely to be any cleaner.  Regardless, this whole mapping process should be explained in comments.

Quoted 39 lines

drivers/soc/qcom/dcc.c new file +38 -0

+#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_READ            0
+#define DCC_WRITE            1
+#define DCC_LOOP            2
+#define DCC_READ_WRITE            3
+
+#define MAX_DCC_OFFSET            GENMASK(9, 2)
+#define MAX_DCC_LEN            GENMASK(6, 0)
+#define MAX_LOOP_CNT            GENMASK(7, 0)
+
+#define DCC_ADDR_DESCRIPTOR        0x00
+#define DCC_ADDR_LIMIT            27
+#define DCC_ADDR_OFF_RANGE        8
+#define DCC_ADDR_RANGE            GENMASK(31, 4)
+#define DCC_LOOP_DESCRIPTOR        BIT(30)
+#define DCC_RD_MOD_WR_DESCRIPTOR    BIT(31)
+#define DCC_LINK_DESCRIPTOR        GENMASK(31, 30)
+
+#define DCC_READ_IND            0x00
+#define DCC_WRITE_IND            (BIT(28))
+
+#define DCC_AHB_IND            0x00
+#define DCC_APB_IND            BIT(29)
+
+#define DCC_MAX_LINK_LIST        8
+#define DCC_INVALID_LINK_LIST        GENMASK(7, 0)
+

drivers/soc/qcom/dcc.c line 86

Quoted 6 lines

drivers/soc/qcom/dcc.c new file +6 -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 {

No need for "qcom_" in the type name here.

Quoted 13 lines

drivers/soc/qcom/dcc.c new file +11 -0

+    int dcc_ram_offset;
+};
+
+enum dcc_descriptor_type {
+    DCC_ADDR_TYPE,
+    DCC_LOOP_TYPE,
+    DCC_READ_WRITE_TYPE,
+    DCC_WRITE_TYPE
+};
+
+enum dcc_mem_map_ver {

drivers/soc/qcom/dcc.c line 100

Quoted 3 lines

drivers/soc/qcom/dcc.c new file +3 -0

+    DCC_MEM_MAP_VER1 = 1,
+    DCC_MEM_MAP_VER2 = 2,
+    DCC_MEM_MAP_VER3 = 3

This enumerated type doesn't really add any value.  Just use 1, 2, and 3 for your version numbers in the few places this is used in code. If it gets more complicated than this one-to-one mapping in the future, add a type like this.

drivers/soc/qcom/dcc.c line 103

Quoted 4 lines

drivers/soc/qcom/dcc.c new file +3 -0

+};
+
+struct dcc_config_entry {

. . .

Quoted 6 lines

drivers/soc/qcom/dcc.c new file +5 -0

+static bool dcc_ready(struct dcc_drvdata *drvdata)
+{
+    u32 val;
+
+    return !readl_poll_timeout((drvdata->base + 

dcc_offset_conv(drvdata, DCC_STATUS)),

Use dcc_readl(drvdata, offset) here.

Quoted 3 lines

+                val, (FIELD_GET(GENMASK(1, 0), val) == 0), 1, TIMEOUT_US);

Use !FIELD_GET(...) here.

Is the 1 microsecond delay required?

Quoted 26 lines

drivers/soc/qcom/dcc.c new file +11 -0

+
+}
+
+static int dcc_read_status(struct dcc_drvdata *drvdata)
+{
+    int curr_list;
+    u32 bus_status;
+    u32 ll_cfg;
+    u32 tmp_ll_cfg;
+
+    for (curr_list = 0; curr_list < drvdata->nr_link_list; 

curr_list++) {
+        if (!drvdata->enable[curr_list])
+            continue;
+
+        bus_status = dcc_readl(drvdata,
DCC_LL_BUS_ACCESS_STATUS(curr_list));

drivers/soc/qcom/dcc.c new file +7 -0

+
+        if (bus_status) {
+            dev_err(drvdata->dev,
+                "Read access error for list %d err: 0x%x.\n",
+                curr_list, bus_status);
+
+            ll_cfg = dcc_readl(drvdata, DCC_LL_CFG(curr_list));

Quoted 4 lines

+            tmp_ll_cfg = ll_cfg & ~BIT(9);
+            dcc_writel(drvdata, tmp_ll_cfg, DCC_LL_CFG(curr_list)); +            dcc_writel(drvdata, 0x3,
+                DCC_LL_BUS_ACCESS_STATUS(curr_list));

What does writing 1 to the bottom two bits of the STATUS register do?  Can you represent that with a mask, which defines the register field?

Quoted 19 lines

+            dcc_writel(drvdata, ll_cfg, DCC_LL_CFG(curr_list)); +            return -ENODATA;
+        }
+    }

drivers/soc/qcom/dcc.c new file +14 -0

+
+    return 0;
+}
+
+static int dcc_sw_trigger(struct dcc_drvdata *drvdata)
+{
+    int ret;
+    int curr_list;
+    u32 ll_cfg;
+    u32 tmp_ll_cfg;
+
+    mutex_lock(&drvdata->mutex);
+
+    for (curr_list = 0; curr_list < drvdata->nr_link_list; 

Quoted 5 lines

curr_list++) {
+        if (!drvdata->enable[curr_list])
+            continue;
+        ll_cfg = dcc_readl(drvdata, DCC_LL_CFG(curr_list)); +        tmp_ll_cfg = ll_cfg & ~BIT(9);

What does bit 9 of this register represent?  Why is it turned off here?

Quoted 3 lines

+        dcc_writel(drvdata, tmp_ll_cfg, DCC_LL_CFG(curr_list)); +        dcc_writel(drvdata, 1, DCC_LL_SW_TRIGGER(curr_list)); +        dcc_writel(drvdata, ll_cfg, DCC_LL_CFG(curr_list));

Does this assume that bit 9 of the register was originally set?

Quoted 8 lines

+    }

drivers/soc/qcom/dcc.c new file +5 -0

+
+    if (!dcc_ready(drvdata)) {
+        dev_err(drvdata->dev,
+            "DCC is busy after receiving sw tigger.\n");
+        ret = -EBUSY;

drivers/soc/qcom/dcc.c line 273

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +10 -0

+        goto err;
+    }
+
+    ret = dcc_read_status(drvdata);
+
+err:
+    mutex_unlock(&drvdata->mutex);
+    return ret;
+}
+

. . .

Quoted 30 lines

drivers/soc/qcom/dcc.c new file +23 -0

+static int __dcc_ll_cfg(struct dcc_drvdata *drvdata, int curr_list)
+{
+    int ret = 0;
+    u32 total_len, pos;
+    struct dcc_config_entry *entry;
+    struct dcc_cfg_attr cfg;
+    struct dcc_cfg_loop_attr cfg_loop;
+
+    memset(&cfg, 0, sizeof(cfg));
+    memset(&cfg_loop, 0, sizeof(cfg_loop));
+    cfg.sram_offset = drvdata->ram_cfg * 4;
+    total_len = 0;
+
+    list_for_each_entry(entry, &drvdata->cfg_head[curr_list], list) {
+        switch (entry->desc_type) {
+        case DCC_READ_WRITE_TYPE:
+            ret = _dcc_ll_cfg_read_write(drvdata, entry, &cfg);
+            if (ret)
+                goto overstep;
+            break;
+
+        case DCC_LOOP_TYPE:
+            ret = _dcc_ll_cfg_loop(drvdata, entry, &cfg, &cfg_loop, 

&total_len);
+            if (ret)
+                goto overstep;
+            break;
+
+        case DCC_WRITE_TYPE:
+            ret = _dcc_ll_cfg_write(drvdata, entry, &cfg, &total_len);

Quoted 5 lines

+            if (ret)
+                goto overstep;
+            break;
+
+        default:

Use this instead of "default":
        case DCC_ADDR_TYPE:

Quoted 15 lines

+            ret = _dcc_ll_cfg_default(drvdata, entry, &cfg, &pos, &total_len);
+            if (ret)
+                goto overstep;
+            break;
+        }
+    }
+
+    if (cfg.link) {
+        ret = dcc_sram_writel(drvdata, cfg.link, cfg.sram_offset); +        if (ret)
+            goto overstep;
+        cfg.sram_offset += 4;
+    }

. . .

drivers/soc/qcom/dcc.c line 802

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +10 -0

+static ssize_t enable_show(struct device *dev,
+    struct device_attribute *attr, char *buf)
+{
+    int ret;
+    bool dcc_enable = false;
+    struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
+
+    mutex_lock(&drvdata->mutex);
+    if (drvdata->curr_list >= drvdata->nr_link_list) {
+        dev_err(dev, "Select link list to program using curr_list\n");

Can this actually happen?  Isn't curr_list initially 0? And can't you constrain the store function for curr_list so it never exceeds nr_link_list?

Quoted 17 lines

+        ret = -EINVAL;
+        goto err;
+    }

drivers/soc/qcom/dcc.c new file +13 -0

+
+    dcc_enable = is_dcc_enabled(drvdata);
+
+    ret = scnprintf(buf, PAGE_SIZE, "%u\n",
+                (unsigned int)dcc_enable > +err:
+    mutex_unlock(&drvdata->mutex);
+    return ret;
+}
+
+static ssize_t enable_store(struct device *dev,
+                struct device_attribute *attr,
+                const char *buf, size_t size)
+{

Quoted 6 lines

+    int ret = 0;
+    unsigned long val;
+    struct dcc_drvdata *drvdata = dev_get_drvdata(dev); +
+    if (kstrtoul(buf, 16, &val))
+        return -EINVAL;

I recommend using kstrtobool() here.

Quoted 4 lines

drivers/soc/qcom/dcc.c new file +2 -0

+
+    if (val)

drivers/soc/qcom/dcc.c line 837

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +10 -0

+        ret = dcc_enable(drvdata);
+    else
+        dcc_disable(drvdata);
+
+    if (!ret)
+        ret = size;
+
+    return ret;
+
+}

. . .

drivers/soc/qcom/dcc.c line 1246

Quoted 13 lines

drivers/soc/qcom/dcc.c new file +13 -0

+static ssize_t config_write_store(struct device *dev,
+                        struct device_attribute *attr,
+                        const char *buf, size_t size)
+{
+    int ret;
+    int nval;
+    unsigned int addr, write_val;
+    int apb_bus = 0;
+    struct dcc_drvdata *drvdata = dev_get_drvdata(dev);
+
+    mutex_lock(&drvdata->mutex);
+
+    nval = sscanf(buf, "%x %x %d", &addr, &write_val, &apb_bus);

You didn't document in the sysfs documentation the optional third argument here, which specify APB rather than AHB bus.

Quoted 9 lines

+    if (nval <= 1 || nval > 3) {
+        ret = -EINVAL;
+        goto err;
+    }
+
+    if (drvdata->curr_list >= drvdata->nr_link_list) {
+        dev_err(dev, "Select link list to program using curr_list\n");

Here again (and everywhere else), avoid this possible error condition by guaranteeing it will never be assigned a value that's larger than nr_link_list.

Quoted 35 lines

+        ret = -EINVAL;
+        goto err;
+    }

drivers/soc/qcom/dcc.c new file +30 -0

+
+    if (nval == 3 && apb_bus != 0)
+        apb_bus = 1;
+
+    ret = dcc_add_write(drvdata, addr, write_val, apb_bus);
+    if (ret)
+        goto err;
+
+    mutex_unlock(&drvdata->mutex);
+    return size;
+err:
+    mutex_unlock(&drvdata->mutex);
+    return ret;
+}
+
+static DEVICE_ATTR_WO(config_write);
+
+static const struct device_attribute *dcc_attrs[] = {
+    &dev_attr_trigger,
+    &dev_attr_enable,
+    &dev_attr_config,
+    &dev_attr_config_reset,
+    &dev_attr_ready,
+    &dev_attr_interrupt_disable,
+    &dev_attr_loop,
+    &dev_attr_rd_mod_wr,
+    &dev_attr_curr_list,
+    &dev_attr_config_write,
+    NULL,
+};

drivers/soc/qcom/dcc.c line 1292

Quoted 5 lines

drivers/soc/qcom/dcc.c new file +5 -0

+
+static int dcc_create_files(struct device *dev,
+                    const struct device_attribute **attrs)
+{
+    int ret = 0, i;

Cant these be initialized automatically as an attribute group or something?  Maybe I'm getting confused.

Quoted 41 lines

+    for (i = 0; attrs[i] != NULL; i++) {
+        ret = device_create_file(dev, attrs[i]);
+        if (ret) {
+            dev_err(dev, "DCC: Couldn't create sysfs attribute: %s\n", +                attrs[i]->attr.name);
+            break;
+        }
+    }
+    return ret;

drivers/soc/qcom/dcc.c new file +30 -0

+}
+
+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)
+        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);

Quoted 3 lines

+
+    if (copy_to_user(data, buf, len)) {
+        dev_err(drvdata->dev, "DCC: Couldn't copy all data to user\n");

Don't allow user input (i.e., providing a bad buffer pointer in this case) lead to spamming the log.  The EFAULT error is enough to explain what the problem is.  In generaly, I don't think you should call dev_err() in these sysfs functions.

Quoted 16 lines

+        kfree(buf);
+        return -EFAULT;
+    }

drivers/soc/qcom/dcc.c new file +11 -0

+
+    *ppos += len;
+
+    kfree(buf);
+
+    return len;
+}
+
+static const struct file_operations dcc_sram_fops = {
+    .owner        = THIS_MODULE,
+    .open        = dcc_sram_open,

Quoted 3 lines

+    .read        = dcc_sram_read,
+    .llseek        = no_llseek,
+};

Since dcc_sram_dev_init() does nothing but call this function, why not just incorporate one into the other?  Same thing goes for dcc_sram_dev_exit() and dcc_sram_dev_deregister().

Quoted 80 lines

drivers/soc/qcom/dcc.c new file +78 -0

+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, DCC_SRAM_NODE);
+    if (ret)
+        goto err_alloc;
+
+    cdev_init(&drvdata->sram_dev, &dcc_sram_fops);
+
+    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, DCC_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,
+                        DCC_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;
+
+    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)
+{
+    u32 val;
+    int ret = 0, i, enable_size, nr_config_size, cfg_head_size;
+    struct device *dev = &pdev->dev;
+    struct dcc_drvdata *dcc;
+    struct resource *res;
+    const struct qcom_dcc_config *cfg;
+
+    dcc = devm_kzalloc(dev, sizeof(*dcc), GFP_KERNEL);
+    if (!dcc)
+        return -ENOMEM;

drivers/soc/qcom/dcc.c line 1436

Quoted 5 lines

drivers/soc/qcom/dcc.c new file +5 -0

+
+    dcc->dev = &pdev->dev;
+    platform_set_drvdata(pdev, dcc);
+
+    dcc->base = devm_platform_ioremap_resource(pdev, 0);

I mentioned earlier, it might be nice to give these memory ranges a name more meaningful than 0 and 1.

Quoted 11 lines

+    if (IS_ERR(dcc->base))
+        return PTR_ERR(dcc->base);
+
+    dcc->ram_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
+    if (IS_ERR(dcc->ram_base))
+        return PTR_ERR(dcc->ram_base);
+
+    dcc->ram_size = resource_size(res);

Did you already take care of remapping with the call just above?

Quoted 5 lines

+    dcc->ram_base = devm_ioremap(dev, res->start, resource_size(res)); +    if (!dcc->ram_base)
+        return -ENOMEM;

In any case, I think this second memory region is more like memory space than I/O space.  If so, memremapping it might be the right thing to do.

Quoted 6 lines

+    cfg = of_device_get_match_data(&pdev->dev);
+    dcc->ram_offset = cfg->dcc_ram_offset;
+
+    val = dcc_readl(dcc, DCC_HW_INFO);

Can you provide a short block of code that explains in English what the next set of if statements are doing, and why?

Quoted 15 lines

+    if (FIELD_GET(BIT(DCC_VER_INFO_BIT), val)) {
+        dcc->mem_map_ver = DCC_MEM_MAP_VER3;
+        dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO); +        if (dcc->nr_link_list == 0)
+            return    -EINVAL;
+    } else if ((val & DCC_VER_MASK2) == DCC_VER_MASK2) { +        dcc->mem_map_ver = DCC_MEM_MAP_VER2;
+        dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO); +        if (dcc->nr_link_list == 0)
+            return    -EINVAL;
+    } else {
+        dcc->mem_map_ver = DCC_MEM_MAP_VER1;
+        dcc->nr_link_list = DCC_MAX_LINK_LIST;
+    }

What does bit 6 in the HW_INFO register represent?

Quoted 3 lines

+    if ((val & BIT(6)) == BIT(6))

    if (val & BIT(6))

Quoted 24 lines

+        dcc->loopoff = DCC_FIX_LOOP_OFFSET;
+    else
+        dcc->loopoff = get_bitmask_order((dcc->ram_size + +                dcc->ram_offset) / 4 - 1);

drivers/soc/qcom/dcc.c new file +8 -0

+
+    mutex_init(&dcc->mutex);
+    /* Allocate space for all entries at once */
+    enable_size = dcc->nr_link_list * sizeof(bool);
+    nr_config_size = dcc->nr_link_list * sizeof(size_t);
+    cfg_head_size = dcc->nr_link_list * sizeof(struct list_head);
+
+    dcc->enable = devm_kzalloc(dev, enable_size + nr_config_size + 

cfg_head_size, GFP_KERNEL);
+    if (!dcc->enable)
+        return -ENOMEM;
+
+    dcc->nr_config  = (size_t *)(dcc->enable + dcc->nr_link_list); +    dcc->cfg_head = (struct list_head *)(dcc->nr_config + dcc->nr_link_list);

drivers/soc/qcom/dcc.c new file +5 -0

+
+    for (i = 0; i < dcc->nr_link_list; i++)
+        INIT_LIST_HEAD(&dcc->cfg_head[i]);
+
+    dcc->curr_list = DCC_INVALID_LINK_LIST;

Quoted 5 lines

+    ret = dcc_sram_dev_init(dcc);
+    if (ret)
+        return ret;
+
+    return dcc_create_files(dev, dcc_attrs);

If dcc_create_files() returns an error, you are not calling dcc_sram_dev_exit() to clean things up.

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +5 -0

+}
+
+static int dcc_remove(struct platform_device *pdev)
+{
+    struct dcc_drvdata *drvdata = platform_get_drvdata(pdev);

drivers/soc/qcom/dcc.c line 1512

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +10 -0

+
+    dcc_sram_dev_exit(drvdata);
+
+    dcc_config_reset(drvdata);
+
+    return 0;
+}
+
+static const struct qcom_dcc_config sm8150_cfg = {
+    .dcc_ram_offset    = 0x5000,

If you know you'll be adding more fields there's nothing wrong with this, but if the only thing you're storing is the RAM offset, there's no need to define that within a structure.  Just do something like:

    { .compatible = "qcom,sm8150-dcc", .data = (void *)0x5000 },

Quoted 38 lines

drivers/soc/qcom/dcc.c new file +37 -0

+};
+
+static const struct qcom_dcc_config sc7280_cfg = {
+    .dcc_ram_offset = 0x12000,
+};
+
+static const struct qcom_dcc_config sc7180_cfg = {
+    .dcc_ram_offset = 0x6000,
+};
+
+static const struct qcom_dcc_config sdm845_cfg = {
+    .dcc_ram_offset = 0x6000,
+};
+
+static const struct of_device_id dcc_match_table[] = {
+    { .compatible = "qcom,sm8150-dcc", .data = &sm8150_cfg },
+    { .compatible = "qcom,sc7280-dcc", .data = &sc7280_cfg },
+    { .compatible = "qcom,sc7180-dcc", .data = &sc7180_cfg },
+    { .compatible = "qcom,sdm845-dcc", .data = &sdm845_cfg },
+    { }
+};
+MODULE_DEVICE_TABLE(of, dcc_match_table);
+
+static struct platform_driver dcc_driver = {
+    .probe = dcc_probe,
+    .remove    = dcc_remove,
+    .driver    = {
+        .name = "qcom-dcc",
+        .of_match_table    = dcc_match_table,
+    },
+};
+
+module_platform_driver(dcc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm Technologies Inc. DCC driver");
+

The DCC is a DMA Engine designed to capture and store data during system crash or software triggers.The DCC operates based on user inputs via the sysfs interface.The user gives addresses as inputs and these addresses are stored in the form of linkedlists.

In case of a system crash or a manual

software trigger by the user through the sysfs interface, the dcc captures and stores the values at these addresses.

This patch contains the driver which has all the methods pertaining to the sysfs interface, auxiliary functions to support all the four fundamental operations of dcc namely read, write, first read then write and loop.The probe method here instantiates all the resources necessary for dcc to operate mainly the dedicated dcc sram where it stores the

values.The DCC driver can be used for debugging purposes without going for a reboot since it can perform manual triggers.

Also added the documentation for sysfs entries and explained the functionalities of each sysfs file that has been created for dcc.

The following is the justification of using sysfs interface over the other alternatives like ioctls

i) As can be seen from the sysfs attribute descriptions, most of it does basic hardware manipulations like dcc_enable, dcc_disable, config reset etc. As a result sysfs is preferred over ioctl as we just need to enter a 0 or 1.

ii) Existing similar debug hardwares are there for which drivers have been written using sysfs interface.One such example is the coresight-etm-trace driver.A closer analog can also be the watchdog subsystems though it is ioctls based.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 Documentation/ABI/testing/sysfs-driver-dcc |  114 ++
 drivers/soc/qcom/Kconfig                   |    8 +
 drivers/soc/qcom/Makefile                  |    1 +
 drivers/soc/qcom/dcc.c                     | 1549 ++++++++++++++++++++++++++++
 4 files changed, 1672 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-dcc
 create mode 100644 drivers/soc/qcom/dcc.c

Documentation/ABI/testing/sysfs-driver-dcc new file +0 -0

new file mode 100644
index 0000000..05d24f0

Documentation/ABI/testing/sysfs-driver-dcc new file +8 -0

+		the dcc hardware ceases to function.
+		Example:
+		echo  0 > /sys/bus/platform/devices/.../enable
+		(disable interface)
+		echo  1 > /sys/bus/platform/devices/.../enable
+		(enable interface)
+
+What:           /sys/bus/platform/devices/.../config

Documentation/ABI/testing/sysfs-driver-dcc new file +12 -0

+		Example:
+		echo  1 > /sys/bus/platform/devices/.../config_reset
+
+What:           /sys/bus/platform/devices/.../loop
+Date:		March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to enter the loop count as dcc
+		driver gives the option to loop multiple times on
+		the same register and store the values for each
+		loop.This is done to capture the changing values
+		of a register with time which comes handy for

Documentation/ABI/testing/sysfs-driver-dcc new file +4 -0

+		Example:
+		echo 0x80000000 > /sys/bus/platform/devices/.../config
+		(Set the address in config file)
+		echo 0xF 0xA > /sys/bus/platform/devices/.../rd_mod_wr

drivers/soc/qcom/Kconfig +8 -0

index 79b568f..5101912 100644
@@ -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(DCC) 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

drivers/soc/qcom/Makefile +1 -0

index ad675a6..0aaf82b 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
 obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
+obj-$(CONFIG_QCOM_DCC) += dcc.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o

drivers/soc/qcom/dcc.c new file +22 -0

new file mode 100644
index 0000000..daf4388
@@ -0,0 +1,1549 @@
+// 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 TIMEOUT_US		5000
+
+#define dcc_writel(drvdata, val, off)					\

drivers/soc/qcom/dcc.c new file +45 -0

+#define DCC_LL_LOCK(m)			(0x34 + 0x80 * m)
+#define DCC_LL_CFG(m)			(0x38 + 0x80 * m)
+#define DCC_LL_BASE(m)			(0x3c + 0x80 * m)
+#define DCC_FD_BASE(m)			(0x40 + 0x80 * m)
+#define DCC_LL_TIMEOUT(m)		(0x44 + 0x80 * m)
+#define DCC_LL_INT_ENABLE(m)		(0x4C + 0x80 * m)
+#define DCC_LL_INT_STATUS(m)		(0x50 + 0x80 * m)
+#define DCC_LL_SW_TRIGGER(m)		(0x60 + 0x80 * m)
+#define DCC_LL_BUS_ACCESS_STATUS(m)	(0x64 + 0x80 * m)
+
+#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_READ			0
+#define DCC_WRITE			1
+#define DCC_LOOP			2
+#define DCC_READ_WRITE			3
+
+#define MAX_DCC_OFFSET			GENMASK(9, 2)
+#define MAX_DCC_LEN			GENMASK(6, 0)
+#define MAX_LOOP_CNT			GENMASK(7, 0)
+
+#define DCC_ADDR_DESCRIPTOR		0x00
+#define DCC_ADDR_LIMIT			27
+#define DCC_ADDR_OFF_RANGE		8
+#define DCC_ADDR_RANGE			GENMASK(31, 4)
+#define DCC_LOOP_DESCRIPTOR		BIT(30)
+#define DCC_RD_MOD_WR_DESCRIPTOR	BIT(31)
+#define DCC_LINK_DESCRIPTOR		GENMASK(31, 30)
+
+#define DCC_READ_IND			0x00
+#define DCC_WRITE_IND			(BIT(28))
+
+#define DCC_AHB_IND			0x00
+#define DCC_APB_IND			BIT(29)
+

drivers/soc/qcom/dcc.c new file +23 -0

+};
+
+enum dcc_descriptor_type {
+	DCC_ADDR_TYPE,
+	DCC_LOOP_TYPE,
+	DCC_READ_WRITE_TYPE,
+	DCC_WRITE_TYPE
+};
+
+enum dcc_mem_map_ver {
+	DCC_MEM_MAP_VER1 = 1,
+	DCC_MEM_MAP_VER2 = 2,
+	DCC_MEM_MAP_VER3 = 3
+};
+
+struct dcc_config_entry {
+	u32				base;
+	u32				offset;
+	u32				len;
+	u32				index;
+	u32				loop_cnt;
+	u32				write_val;
+	u32				mask;

drivers/soc/qcom/dcc.c new file +10 -0

+ * @sram_dev:	      Character device equivalent of dcc SRAM
+ * @sram_class:	      Class equivalent of the DCC SRAM device
+ * @cfg_head:	      Points to the head of the linked list of addresses
+ * @nr_config:        Stores the number of addresses currently configured for a linkedlist
+ * @nr_link_list:     Total number of linkedlists supported by the DCC configuration
+ * @curr_list:        The index of the current linklist with which the driver is working
+ * @loopoff:          Loop offset bits range for the addresses
+ */
+struct dcc_drvdata {
+	void __iomem		*base;

drivers/soc/qcom/dcc.c new file +6 -0

+		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)

Quoted 5 lines · Souradeep Chowdhury

The DCC is a DMA Engine designed to capture and store data during system crash or software triggers.The DCC operates based on user inputs via the sysfs interface.The user gives addresses as inputs and these addresses are stored in the form of linkedlists.

The linked lists are present in the hardware, right? You should state it explicitly.

Quoted 2 lines

In case of a system crash or a manual

So what does a crash here means? How does DCC detect it?

Quoted 3 lines

software trigger by the user through the sysfs interface, the dcc captures and stores the values at these addresses.

This could be reworded a bit:

"DCC captures and stores the current values of the provided addresses in SRAM"

Quoted 7 lines

This patch contains the driver which has all the methods pertaining to the sysfs interface, auxiliary functions to support all the four fundamental operations of dcc namely read, write, first read then write and loop.The probe method here instantiates all the resources necessary for dcc to operate mainly the dedicated dcc sram where it stores the

Please use "DCC" everywhere.

Quoted 5 lines

values.The DCC driver can be used for debugging purposes without going for a reboot since it can perform manual triggers.

It allows users to perform software triggers for capturing values manually.

Quoted 8 lines

Also added the documentation for sysfs entries and explained the functionalities of each sysfs file that has been created for dcc.

The following is the justification of using sysfs interface over the other alternatives like ioctls

Quoted 5 lines

i) As can be seen from the sysfs attribute descriptions, most of it does basic hardware manipulations like dcc_enable, dcc_disable, config reset etc. As a result sysfs is preferred over ioctl as we just need to enter a 0 or 1.

This is not an apt reason to use sysfs. The one-value-per-file is a rule of sysfs not a criteria to use it.

Quoted 6 lines

ii) Existing similar debug hardwares are there for which drivers have been written using sysfs interface.One such example is the coresight-etm-trace driver.A closer analog can also be the watchdog subsystems though it is ioctls based.

Initially I thought that this driver could use debugfs interface, but going through it I feel that sysfs is more suited. Reason is, debugfs interface is used by drivers to expose debugging information additional to the function they do. But the sole usage of this driver depends on the configuration exported through the attributes and they looks to be an ABI to me.

Quoted 11 lines

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  Documentation/ABI/testing/sysfs-driver-dcc |  114 ++
  drivers/soc/qcom/Kconfig                   |    8 +
  drivers/soc/qcom/Makefile                  |    1 +
  drivers/soc/qcom/dcc.c                     | 1549 ++++++++++++++++++++++++++++
  4 files changed, 1672 insertions(+)

create mode 100644 Documentation/ABI/testing/sysfs-driver-dcc create mode 100644 drivers/soc/qcom/dcc.c

Documentation/ABI/testing/sysfs-driver-dcc line 2

Quoted 8 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +2 -0

new file mode 100644
index 0000000..05d24f0
@@ -0,0 +1,114 @@
+What:           /sys/bus/platform/devices/.../trigger
+Date:           March 2021

Please fix the dates of all attributes.

Documentation/ABI/testing/sysfs-driver-dcc line 5

Quoted 4 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +3 -0

+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This is the sysfs interface for manual software

All entries should be aligned properly.

Documentation/ABI/testing/sysfs-driver-dcc line 9

Quoted 5 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +4 -0

+		triggers.The user can simply enter a 1 against
+		the sysfs file and enable a manual trigger.
+		Example:
+		echo  1 > /sys/bus/platform/devices/.../trigger

Why 2 spaces after echo?

Documentation/ABI/testing/sysfs-driver-dcc line 16

Quoted 8 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +7 -0

+
+What:           /sys/bus/platform/devices/.../enable
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This sysfs interface is used for enabling the
+		the dcc hardware.Without this being set to 1,

DCC here also. And a space after "." everywhere.

Quoted 12 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +11 -0

+		the dcc hardware ceases to function.
+		Example:
+		echo  0 > /sys/bus/platform/devices/.../enable
+		(disable interface)
+		echo  1 > /sys/bus/platform/devices/.../enable
+		(enable interface)
+
+What:           /sys/bus/platform/devices/.../config
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:

Documentation/ABI/testing/sysfs-driver-dcc line 34

Quoted 7 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +7 -0

+		This is the most commonly used sysfs interface
+		file and this basically stores the addresses of
+		the registers which needs to be read in case of
+		a hardware crash or manual software triggers.
+		Example:
+		echo  0x80000010 10 > /sys/bus/platform/devices/../config
+		This specifies that 10 words starting from address

words are of width?

Documentation/ABI/testing/sysfs-driver-dcc line 36

Quoted 3 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +2 -0

+		0x80000010 is to be read.In case there are no words to be
+		specified we can simply enter the address.

No word to be read? So what's the purpose then?

Documentation/ABI/testing/sysfs-driver-dcc line 46

Quoted 11 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +10 -0

+
+What:           /sys/bus/platform/devices/.../config_write
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file allows user to write a value to the register
+		address given as argument.The values are entered in the
+		form of <register_address> <value>.The reason for this
+		feature of dcc is that for accessing certain registers
+		it is necessary to set some bits of soe other register.

soe?

Documentation/ABI/testing/sysfs-driver-dcc line 47

Quoted 2 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +1 -0

+		That is achievable by giving DCC this privelege.

privilege

Documentation/ABI/testing/sysfs-driver-dcc line 56

Quoted 10 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +9 -0

+		Example:
+		echo 0x80000000 0xFF > /sys/bus/platform/devices/.../config_write
+
+What:           /sys/bus/platform/devices/.../config_reset
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to reset the configuration of
+		a dcc driver to the default configuration.

s/"a dcc driver"/"the DCC driver"/g

Quoted 16 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +15 -0

+		Example:
+		echo  1 > /sys/bus/platform/devices/.../config_reset
+
+What:           /sys/bus/platform/devices/.../loop
+Date:		March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to enter the loop count as dcc
+		driver gives the option to loop multiple times on
+		the same register and store the values for each
+		loop.This is done to capture the changing values
+		of a register with time which comes handy for
+		debugging purposes.
+		Example:
+		echo 10 > /sys/bus/platform/devices/10a2000.dcc/loop

Documentation/ABI/testing/sysfs-driver-dcc line 78

Quoted 7 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +7 -0

+		(Setting the loop count to 10)
+		echo  0x80000010 10 > /sys/bus/platform/devices/.../config
+                (Read 10 words starting from address 0x80000010O)
+		echo 1 > /sys/bus/platform/devices/.../loop
+		(Terminate the loop by writing a count of 1 to the loop sysfs node)
+
+What:           /sys/bus/platform/devices/.../rd_mod_wr

Can you come up with a better name? Like config_read_write?

Documentation/ABI/testing/sysfs-driver-dcc line 87

Quoted 10 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +9 -0

+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to read the value of the register
+		and then write the value given as an argument to the
+		register address in config.The address argument should
+		be given of the form <mask> <value>.For debugging
+		purposes sometimes we need to first read from a register
+		and then set some values to the register.

Reading this description gives an impression that this file is used to read the value of a register. But it is write only and the value will be read only during the trigger or crash. So this should be stated explicitly.

Quoted 5 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +4 -0

+		Example:
+		echo 0x80000000 > /sys/bus/platform/devices/.../config
+		(Set the address in config file)
+		echo 0xF 0xA > /sys/bus/platform/devices/.../rd_mod_wr

Documentation/ABI/testing/sysfs-driver-dcc line 101

Quoted 10 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +10 -0

+		(Provide the mask and the value to write)
+
+What:           /sys/bus/platform/devices/.../ready
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to check the status of the dcc
+		hardware if it's ready to take the inputs.
+		Example:
+		cat /sys/bus/platform/devices/.../ready

Document the value read also.

Quoted 4 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +3 -0

+
+What:		/sys/bus/platform/devices/.../curr_list
+Date:		February 2021

Documentation/ABI/testing/sysfs-driver-dcc line 114

Quoted 10 lines

Documentation/ABI/testing/sysfs-driver-dcc new file +10 -0

+Contact:	Souradeep Chowdhury <[email protected]>
+Description:
+		This attribute is used to enter the linklist to be
+		used while appending addresses.The range of values
+		for this can be from 0 to 3.This feature is given in
+		order to use certain linkedlist for certain debugging
+		purposes.
+		Example:
+		echo 0 > /sys/bus/platform/devices/10a2000.dcc/curr_list
+

How does a user will know the contents of the linked list? Basis on what criteria, user will know what value to write?

Quoted 63 lines

drivers/soc/qcom/Kconfig +8 -0

index 79b568f..5101912 100644
@@ -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(DCC) 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

drivers/soc/qcom/Makefile +1 -0

index ad675a6..0aaf82b 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
  obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
  obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
  obj-$(CONFIG_QCOM_CPR)		+= cpr.o
+obj-$(CONFIG_QCOM_DCC) += dcc.o
  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
  obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
  obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o

drivers/soc/qcom/dcc.c new file +25 -0

new file mode 100644
index 0000000..daf4388
@@ -0,0 +1,1549 @@
+// 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 TIMEOUT_US		5000
+
+#define dcc_writel(drvdata, val, off)					\
+	writel((val), drvdata->base + dcc_offset_conv(drvdata, off))
+#define dcc_readl(drvdata, off)						\
+	readl(drvdata->base + dcc_offset_conv(drvdata, off))

drivers/soc/qcom/dcc.c line 32

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +7 -0

+
+#define DCC_SRAM_NODE "dcc_sram"
+
+/* DCC registers */
+#define DCC_HW_INFO			0x04
+#define DCC_LL_NUM_INFO			0x10
+#define DCC_STATUS			0x1C

Please use lower case for register offsets. Also, align the defines properly.

Quoted 50 lines

drivers/soc/qcom/dcc.c new file +49 -0

+#define DCC_LL_LOCK(m)			(0x34 + 0x80 * m)
+#define DCC_LL_CFG(m)			(0x38 + 0x80 * m)
+#define DCC_LL_BASE(m)			(0x3c + 0x80 * m)
+#define DCC_FD_BASE(m)			(0x40 + 0x80 * m)
+#define DCC_LL_TIMEOUT(m)		(0x44 + 0x80 * m)
+#define DCC_LL_INT_ENABLE(m)		(0x4C + 0x80 * m)
+#define DCC_LL_INT_STATUS(m)		(0x50 + 0x80 * m)
+#define DCC_LL_SW_TRIGGER(m)		(0x60 + 0x80 * m)
+#define DCC_LL_BUS_ACCESS_STATUS(m)	(0x64 + 0x80 * m)
+
+#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_READ			0
+#define DCC_WRITE			1
+#define DCC_LOOP			2
+#define DCC_READ_WRITE			3
+
+#define MAX_DCC_OFFSET			GENMASK(9, 2)
+#define MAX_DCC_LEN			GENMASK(6, 0)
+#define MAX_LOOP_CNT			GENMASK(7, 0)
+
+#define DCC_ADDR_DESCRIPTOR		0x00
+#define DCC_ADDR_LIMIT			27
+#define DCC_ADDR_OFF_RANGE		8
+#define DCC_ADDR_RANGE			GENMASK(31, 4)
+#define DCC_LOOP_DESCRIPTOR		BIT(30)
+#define DCC_RD_MOD_WR_DESCRIPTOR	BIT(31)
+#define DCC_LINK_DESCRIPTOR		GENMASK(31, 30)
+
+#define DCC_READ_IND			0x00
+#define DCC_WRITE_IND			(BIT(28))
+
+#define DCC_AHB_IND			0x00
+#define DCC_APB_IND			BIT(29)
+
+#define DCC_MAX_LINK_LIST		8
+#define DCC_INVALID_LINK_LIST		GENMASK(7, 0)
+
+#define DCC_VER_MASK1			GENMASK(6, 0)

drivers/soc/qcom/dcc.c line 87

Quoted 6 lines

drivers/soc/qcom/dcc.c new file +6 -0

+#define DCC_VER_MASK2			GENMASK(5, 0)
+
+#define DCC_RD_MOD_WR_ADDR              0xC105E
+
+struct qcom_dcc_config {
+	int dcc_ram_offset;

Are you planning to expand this structure? If not, please use "dcc_ram_offset" directly.

Quoted 27 lines

drivers/soc/qcom/dcc.c new file +26 -0

+};
+
+enum dcc_descriptor_type {
+	DCC_ADDR_TYPE,
+	DCC_LOOP_TYPE,
+	DCC_READ_WRITE_TYPE,
+	DCC_WRITE_TYPE
+};
+
+enum dcc_mem_map_ver {
+	DCC_MEM_MAP_VER1 = 1,
+	DCC_MEM_MAP_VER2 = 2,
+	DCC_MEM_MAP_VER3 = 3
+};
+
+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;

drivers/soc/qcom/dcc.c line 120

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +7 -0

+};
+
+/**
+ * struct dcc_drvdata - configuration information related to a dcc device
+ * @base:	      Base Address of the dcc device
+ * @dev:	      The device attached to the driver data
+ * @mutex:	      Lock to protect access and manipulation of dcc_drvdata

What? Are you trying to protect the whole structure or some fields?

drivers/soc/qcom/dcc.c line 126

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +6 -0

+ * @ram_base:         Base address for the SRAM dedicated for the dcc device
+ * @ram_offset:       Offset to the SRAM dedicated for dcc device
+ * @mem_map_ver:      Memory map version of DCC hardware
+ * @ram_cfg:          Used for address limit calculation for dcc
+ * @ram_start:        Starting address of DCC SRAM
+ * @enable:	      Flag to check if DCC linked list is enabled

This contains an array of linked list enable flags.

drivers/soc/qcom/dcc.c line 127

Quoted 2 lines

drivers/soc/qcom/dcc.c new file +1 -0

+ * @interrupt_disable:Flag to enable/disable interrupts

For simplicity, just use a space after colon.

Quoted 14 lines

drivers/soc/qcom/dcc.c new file +13 -0

+ * @sram_dev:	      Character device equivalent of dcc SRAM
+ * @sram_class:	      Class equivalent of the DCC SRAM device
+ * @cfg_head:	      Points to the head of the linked list of addresses
+ * @nr_config:        Stores the number of addresses currently configured for a linkedlist
+ * @nr_link_list:     Total number of linkedlists supported by the DCC configuration
+ * @curr_list:        The index of the current linklist with which the driver is working
+ * @loopoff:          Loop offset bits range for the addresses
+ */
+struct dcc_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct mutex		mutex;
+	void __iomem		*ram_base;

drivers/soc/qcom/dcc.c line 147

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +7 -0

+	phys_addr_t		ram_size;
+	phys_addr_t		ram_offset;
+	enum dcc_mem_map_ver	mem_map_ver;
+	phys_addr_t		ram_cfg;
+	phys_addr_t		ram_start;
+	bool			*enable;
+	bool			interrupt_disable;

Move this to end to avoid holes.

drivers/soc/qcom/dcc.c line 155

Quoted 9 lines

drivers/soc/qcom/dcc.c new file +8 -0

+	struct cdev		sram_dev;
+	struct class		*sram_class;
+	struct list_head	*cfg_head;
+	size_t			*nr_config;
+	size_t			nr_link_list;
+	u8			curr_list;
+	u8			loopoff;
+};

Is it possible to move the linked list specific members to a different struct?

drivers/soc/qcom/dcc.c line 167

Quoted 13 lines

drivers/soc/qcom/dcc.c new file +12 -0

+
+struct dcc_cfg_attr {
+	u32	addr;
+	u32	prev_addr;
+	u32	prev_off;
+	u32	link;
+	u32	sram_offset;
+};
+
+struct dcc_cfg_loop_attr {
+	u32	loop;
+	bool	loop_start;

Move this to the end

drivers/soc/qcom/dcc.c line 173

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +6 -0

+	u32	loop_cnt;
+	u32	loop_len;
+	u32	loop_off;
+};
+
+static size_t dcc_offset_conv(struct dcc_drvdata *drvdata, size_t off)

Add comment on what this function does.

drivers/soc/qcom/dcc.c line 176

Quoted 4 lines

drivers/soc/qcom/dcc.c new file +3 -0

+{
+	if (drvdata->mem_map_ver == DCC_MEM_MAP_VER1) {
+		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL3)

Use FIELD_* macros where applicable.

drivers/soc/qcom/dcc.c line 177

Quoted 2 lines

drivers/soc/qcom/dcc.c new file +1 -0

+			return off - DCC_MAP_OFFSET3;

Use brackets ():

return (off - DCC_MAP_OFFSET3);

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +9 -0

+		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;
+	}
+

drivers/soc/qcom/dcc.c line 193

Quoted 7 lines

drivers/soc/qcom/dcc.c new file +7 -0

+	return off;
+}
+
+static int dcc_sram_writel(struct dcc_drvdata *drvdata,
+					u32 val, u32 off)
+{
+	if (unlikely(off > (drvdata->ram_size - 4)))

A comment here would be good too.

Quoted 4 lines

drivers/soc/qcom/dcc.c new file +3 -0

+		return -EINVAL;
+
+	writel(val, drvdata->ram_base + off);

drivers/soc/qcom/dcc.c line 206

Quoted 10 lines

drivers/soc/qcom/dcc.c new file +10 -0

+
+	return 0;
+}
+
+static bool dcc_ready(struct dcc_drvdata *drvdata)
+{
+	u32 val;
+
+	return !readl_poll_timeout((drvdata->base + dcc_offset_conv(drvdata, DCC_STATUS)),
+				val, (FIELD_GET(GENMASK(1, 0), val) == 0), 1, TIMEOUT_US);

"FIELD_GET(GENMASK(1, 0)" could be wrapped in a define.

drivers/soc/qcom/dcc.c line 218

Quoted 13 lines

drivers/soc/qcom/dcc.c new file +12 -0

+
+}
+
+static int dcc_read_status(struct dcc_drvdata *drvdata)
+{
+	int curr_list;
+	u32 bus_status;
+	u32 ll_cfg;
+	u32 tmp_ll_cfg;
+
+	for (curr_list = 0; curr_list < drvdata->nr_link_list; curr_list++) {
+		if (!drvdata->enable[curr_list])

This looks wrong to me. Why can't you simply allocate N number of linked list structures based on the value read from hardware and do all list manipulations with it? Like,

struct dcc_list {
	struct list_head cfg_head;
	bool enable;
	...
};

struct dcc_drvdata {
	...
	struct dcc_list *lists;
	...
};

/* List allocation */
dcc->lists = devm_kzalloc();

...

/* List initialization */

for (i = 0; i < dcc->nr_link_list; i++) {
	list = dcc->lists[i];
	INIT_LIST_HEAD(&list->cfg_head);
}

...

/* List manipulation */
list = dcc->lists[i];
list->enable = true;

drivers/soc/qcom/dcc.c line 222

Quoted 5 lines

drivers/soc/qcom/dcc.c new file +4 -0

+			continue;
+
+		bus_status = dcc_readl(drvdata, DCC_LL_BUS_ACCESS_STATUS(curr_list));
+

No new line needed here.

I'm stopping here. For the next revision, please split this patch into multiple ones based on the functionality added. It is hard to review 1.3k LOC in a single patch.

Thanks,
Mani

The DCC is a DMA Engine designed to capture and store data during system crash or software triggers.The DCC operates

based on user inputs via the sysfs interface.The user gives addresses as inputs and these addresses are stored in the form of linkedlists.In case of a system crash or a manual

software trigger by the user through the sysfs interface, the dcc captures and stores the values at these addresses. This patch contains the driver which has all the methods pertaining to the sysfs interface, auxiliary functions to support all the four fundamental operations of dcc namely read, write, first read then write and loop.The probe method

here instantiates all the resources necessary for dcc to operate mainly the dedicated dcc sram where it stores the values.The DCC driver can be used for debugging purposes without going for a reboot since it can perform manual triggers.

Also added the documentation for sysfs entries and explained the functionalities of each sysfs file that has been created for dcc.

The following is the justification of using sysfs interface over the other alternatives like ioctls

i) As can be seen from the sysfs attribute descriptions, most of it does basic hardware manipulations like dcc_enable, dcc_disable, config reset etc. As a result sysfs is preferred over ioctl as we just need to enter a 0 or 1.

ii) Existing similar debug hardwares are there for which drivers have been written using sysfs interface.One such example is the coresight-etm-trace driver.

A closer analog can also be the watchdog
subsystems though it is ioctls based.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 Documentation/ABI/testing/sysfs-driver-dcc |  114 ++
 drivers/soc/qcom/Kconfig                   |    8 +
 drivers/soc/qcom/Makefile                  |    1 +
 drivers/soc/qcom/dcc.c                     | 1549 ++++++++++++++++++++++++++++
 4 files changed, 1672 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-dcc
 create mode 100644 drivers/soc/qcom/dcc.c

Documentation/ABI/testing/sysfs-driver-dcc new file +18 -0

new file mode 100644
index 0000000..05d24f0
@@ -0,0 +1,114 @@
+What:           /sys/bus/platform/devices/.../trigger
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This is the sysfs interface for manual software
+		triggers.The user can simply enter a 1 against
+		the sysfs file and enable a manual trigger.
+		Example:
+		echo  1 > /sys/bus/platform/devices/.../trigger
+
+What:           /sys/bus/platform/devices/.../enable
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This sysfs interface is used for enabling the
+		the dcc hardware.Without this being set to 1,
+		the dcc hardware ceases to function.
+		Example:

Documentation/ABI/testing/sysfs-driver-dcc new file +43 -0

+		Example:
+		echo  0x80000010 10 > /sys/bus/platform/devices/../config
+		This specifies that 10 words starting from address
+		0x80000010 is to be read.In case there are no words to be
+		specified we can simply enter the address.
+
+What:           /sys/bus/platform/devices/.../config_write
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file allows user to write a value to the register
+		address given as argument.The values are entered in the
+		form of <register_address> <value>.The reason for this
+		feature of dcc is that for accessing certain registers
+		it is necessary to set some bits of soe other register.
+		That is achievable by giving DCC this privelege.
+		Example:
+		echo 0x80000000 0xFF > /sys/bus/platform/devices/.../config_write
+
+What:           /sys/bus/platform/devices/.../config_reset
+Date:           March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to reset the configuration of
+		a dcc driver to the default configuration.
+		Example:
+		echo  1 > /sys/bus/platform/devices/.../config_reset
+
+What:           /sys/bus/platform/devices/.../loop
+Date:		March 2021
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		This file is used to enter the loop count as dcc
+		driver gives the option to loop multiple times on
+		the same register and store the values for each
+		loop.This is done to capture the changing values
+		of a register with time which comes handy for
+		debugging purposes.
+		Example:
+		echo 10 > /sys/bus/platform/devices/10a2000.dcc/loop
+		(Setting the loop count to 10)
+		echo  0x80000010 10 > /sys/bus/platform/devices/.../config
+                (Read 10 words starting from address 0x80000010O)

Documentation/ABI/testing/sysfs-driver-dcc new file +6 -0

+		The address argument should
+		be given of the form <mask> <value>.For debugging
+		purposes sometimes we need to first read from a register
+		and then set some values to the register.
+		Example:
+		echo 0x80000000 > /sys/bus/platform/devices/.../config

Documentation/ABI/testing/sysfs-driver-dcc new file +7 -0

+		used while appending addresses.The range of values
+		for this can be from 0 to 3.This feature is given in
+		order to use certain linkedlist for certain debugging
+		purposes.
+		Example:
+		echo 0 > /sys/bus/platform/devices/10a2000.dcc/curr_list
+

drivers/soc/qcom/Kconfig +8 -0

index 79b568f..5101912 100644
@@ -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(DCC) 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

drivers/soc/qcom/Makefile +1 -0

index ad675a6..0aaf82b 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
 obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
+obj-$(CONFIG_QCOM_DCC) += dcc.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o

drivers/soc/qcom/dcc.c new file +120 -0

new file mode 100644
index 0000000..daf4388
@@ -0,0 +1,1549 @@
+// 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 TIMEOUT_US		5000
+
+#define dcc_writel(drvdata, val, off)					\
+	writel((val), drvdata->base + dcc_offset_conv(drvdata, off))
+#define dcc_readl(drvdata, off)						\
+	readl(drvdata->base + dcc_offset_conv(drvdata, off))
+
+#define DCC_SRAM_NODE "dcc_sram"
+
+/* DCC registers */
+#define DCC_HW_INFO			0x04
+#define DCC_LL_NUM_INFO			0x10
+#define DCC_STATUS			0x1C
+#define DCC_LL_LOCK(m)			(0x34 + 0x80 * m)
+#define DCC_LL_CFG(m)			(0x38 + 0x80 * m)
+#define DCC_LL_BASE(m)			(0x3c + 0x80 * m)
+#define DCC_FD_BASE(m)			(0x40 + 0x80 * m)
+#define DCC_LL_TIMEOUT(m)		(0x44 + 0x80 * m)
+#define DCC_LL_INT_ENABLE(m)		(0x4C + 0x80 * m)
+#define DCC_LL_INT_STATUS(m)		(0x50 + 0x80 * m)
+#define DCC_LL_SW_TRIGGER(m)		(0x60 + 0x80 * m)
+#define DCC_LL_BUS_ACCESS_STATUS(m)	(0x64 + 0x80 * m)
+
+#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_READ			0
+#define DCC_WRITE			1
+#define DCC_LOOP			2
+#define DCC_READ_WRITE			3
+
+#define MAX_DCC_OFFSET			GENMASK(9, 2)
+#define MAX_DCC_LEN			GENMASK(6, 0)
+#define MAX_LOOP_CNT			GENMASK(7, 0)
+
+#define DCC_ADDR_DESCRIPTOR		0x00
+#define DCC_ADDR_LIMIT			27
+#define DCC_ADDR_OFF_RANGE		8
+#define DCC_ADDR_RANGE			GENMASK(31, 4)
+#define DCC_LOOP_DESCRIPTOR		BIT(30)
+#define DCC_RD_MOD_WR_DESCRIPTOR	BIT(31)
+#define DCC_LINK_DESCRIPTOR		GENMASK(31, 30)
+
+#define DCC_READ_IND			0x00
+#define DCC_WRITE_IND			(BIT(28))
+
+#define DCC_AHB_IND			0x00
+#define DCC_APB_IND			BIT(29)
+
+#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 {
+	int dcc_ram_offset;
+};
+
+enum dcc_descriptor_type {
+	DCC_ADDR_TYPE,
+	DCC_LOOP_TYPE,
+	DCC_READ_WRITE_TYPE,
+	DCC_WRITE_TYPE
+};
+
+enum dcc_mem_map_ver {
+	DCC_MEM_MAP_VER1 = 1,
+	DCC_MEM_MAP_VER2 = 2,
+	DCC_MEM_MAP_VER3 = 3
+};
+
+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 - configuration information related to a dcc device
+ * @base:	      Base Address of the dcc device
+ * @dev:	      The device attached to the driver data
+ * @mutex:	      Lock to protect access and manipulation of dcc_drvdata