Message-ID: <7c189355ca6c472b05151673d27481c3@codeaurora.org>
Date:   Thu, 11 Mar 2021 15:36:47 +0530
From:   schowdhu@...eaurora.org
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>, Andy Gross <agross@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        sibis@...eaurora.org, saiprakash.ranjan@...eaurora.org,
        Rajendra Nayak <rnayak@...eaurora.org>, vkoul@...nel.org
Subject: Re: [PATCH V1 2/6] soc: qcom: dcc: Add driver support for Data
 Capture and Compare unit(DCC)

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@...eaurora.org>
>> ---
>>  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
>> 

Powered by blists - more mailing lists