Message-ID: <YElUCaBUOx7hEuIh@builder.lan>
Date:   Wed, 10 Mar 2021 17:19:37 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Souradeep Chowdhury <schowdhu@...eaurora.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 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?

> 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.

> +
> +/* 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.

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

> +{
> +	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.

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

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

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

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

> +	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"...

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

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

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

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

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

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

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

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

Add a newline between each check and the subsequent allocation, or do
all the allocation then do a
	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.

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

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

> +
> +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"?


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

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