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

schowdhu
2021-03-11T10:07:55Z

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