Re: [PATCH V1 1/6] dt-bindings: Added the yaml bindings for DCC

Re: [PATCH V1 1/6] dt-bindings: Added the yaml bindings for DCC

[Bjorn Andersson]

On Wed 10 Mar 10:46 CST 2021, Souradeep Chowdhury wrote:

> Documentation for Data Capture and Compare(DCC) device tree bindings
> in yaml format.
> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> ---
>  .../devicetree/bindings/arm/msm/qcom,dcc.yaml      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> new file mode 100644
> index 0000000..b8e9998
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> @@ -0,0 +1,49 @@
> +# 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 <schowdhu@codeaurora.org>
> +
> +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
> +      - const: qcom,dcc
> +
> +  reg:
> +    items:
> +      - description: DCC base register region
> +      - description: DCC RAM base register region
> +
> +  reg-names:
> +    items:
> +      - const: dcc-base
> +      - const: dcc-ram-base

Please omit the "-base" suffix from the reg-names.

Regards,
Bjorn

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    dcc@10a2000{
> +                compatible = "qcom,sm8150-dcc","qcom,dcc";
> +                reg = <0x010a2000  0x1000>,
> +                      <0x010ad000  0x2000>;
> +                reg-names = "dcc-base", "dcc-ram-base";
> +    };
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Thu 11 Mar 00:19 CST 2021, Sai Prakash Ranjan wrote:

> Hi Bjorn,
> 
> 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?
> > 
> 
> Not just the next boot, but also for the current boot via /dev/dcc_sram,
> more below.
> 

Interesting!

> > > 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
> > > 
> 
> <snip>...
> 
> > 
> > How about implementing this using pstore instead of exposing it through
> > a custom /dev/dcc_sram (if I read the code correclty)
> > 
> 
> Using pstore is definitely a good suggestion, we have been thinking of
> adding it as a separate change once the basic support for DCC gets in.
> But pstore ram backend again depends on warm reboot which is present only
> in chrome compute platforms but android platforms do not officially support
> warm reboot. Pstore with block devices as a backend would be ideal but it
> is still a work in progress to use mmc as the backend [1].
> 

I was under the impression that we can have multiple pstore
implementations active, so we would have ramoops and dcc presented
side by side after such restart. Perhaps that's a misunderstanding on my
part?

> Now the other reason as to why we need this dev interface in addition to
> pstore,
> 
>  * Pstore contents are retrieved on the next boot, but DCC SRAM contents
>    can be collected via dev interface for the current boot via SW trigger.
>    Lets say we have some non-fatal errors in one of the subsystems and we
>    want to analyze the register values, it becomes as simple as configuring
>    that region, trigger dcc and collect the sram contents and parse it.
> 
>    echo "addr" > /sys/bus/platform/devices/***.dcc/config
>    echo  1 > /sys/bus/platform/devices/***.dcc/trigger
>    cat /dev/dcc_sram > dcc_sram.bin
>    python dcc_parser.py -s dcc_sram.bin --v2 -o output/
> 
> We don't have to reboot at all for SW triggers. This is very useful and
> widely used internally.
> 
> Is the custom /dev/dcc_sram not recommended because of the dependency on
> the userspace component being not available openly? If so, we already have
> the dcc parser upstream which we use to extract the sram contents [2].
> 

My concern is that we come up with a custom chardev implementation for
something that already exists or should exist in a more generic form.


In the runtime sequence above, why don't you have trigger just generate
a devcoredump? But perhaps the answer is that we want a unified
interface between the restart and runtime use cases?


It would be nice to have some more details of how I can use this and how
to operate the sysfs interface to achieve that, would you be able to
elaborate on this?

Regards,
Bjorn

> [1]
> https://lore.kernel.org/lkml/20210120121047.2601-1-bbudiredla@marvell.com/
> [2] https://source.codeaurora.org/quic/la/platform/vendor/qcom-opensource/tools/tree/dcc_parser
> 
> Thanks,
> Sai
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Thu 11 Mar 04:06 CST 2021, schowdhu@codeaurora.org wrote:

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

Thanks Souradeep and Sai, I'm definitely interested in learning more
about what the hardware block can do and how we can use it.

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V1 4/6] DCC: Added the sysfs entries for DCC(Data Capture and Compare) driver

[Bjorn Andersson]

On Wed 10 Mar 10:46 CST 2021, Souradeep Chowdhury wrote:

> The DCC is a DMA engine designed to store register values either in
> case of a system crash or in case of software triggers manually done
> by the user. Using DCC hardware and the sysfs interface of the driver
> the user can exploit various functionalities of DCC. The user can specify
> the register addresses, the values of which is stored by DCC in it's
> dedicated SRAM. The register addresses can be used either to read from,
> write to, first read and store value and then write or to loop. All these
> options can be exploited using the sysfs interface given to the user.
> Following are the sysfs interfaces exposed in DCC driver which are
> documented
> 1)trigger
> 2)config
> 3)config_write
> 4)config_reset
> 5)enable
> 6)rd_mod_wr
> 7)loop
> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> ---
>  Documentation/ABI/testing/sysfs-driver-dcc | 74 ++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-dcc
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-dcc b/Documentation/ABI/testing/sysfs-driver-dcc
> new file mode 100644
> index 0000000..7a855ca
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-dcc
> @@ -0,0 +1,74 @@
> +What:           /sys/bus/platform/devices/.../trigger
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +Description:
> +		This file allows the software trigger to be enabled
> +		by the user through the sysfs interface.Through this
> +		interface the user can manually start a software trigger
> +		in dcc where by the dcc driver stores the current status
> +		of the specified registers in dcc sram.
> +
> +What:           /sys/bus/platform/devices/.../enable
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +Description:
> +		This file allows the user to manually enable or
> +		disable dcc driver.The dcc hardware needs to be
> +		enabled before use.
> +
> +What:           /sys/bus/platform/devices/.../config
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +Description:
> +		This file allows user to configure the register values
> +		along with addresses to the dcc driver.This register
> +		addresses are used to read from,write or loop through.
> +		To enable all these options separate sysfs files have
> +		are created.

Please describe the expected content of this file.

> +
> +What:           /sys/bus/platform/devices/.../config_write
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +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>.

So it's just a generic 'write some user defined data to some user
defined register'? This doesn't sound like the typical way things are
exposed in sysfs.

> +
> +What:           /sys/bus/platform/devices/.../config_reset
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +Description:
> +		This file is used to reset the configuration of
> +		a dcc driver to the default configuration.
> +
> +What:           /sys/bus/platform/devices/.../loop
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +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.
> +
> +What:           /sys/bus/platform/devices/.../rd_mod_wr
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +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>.
> +
> +What:           /sys/bus/platform/devices/.../ready
> +Date:           February 2021
> +Contact:        Souradeep Chowdhury <schowdhu@codeaurora.org>
> +Description:
> +		This file is used to check the status of the dcc
> +		hardware if it's ready to take the inputs.
> +
> +What:		/sys/bus/platform/devices/.../curr_list
> +Date:		February 2021
> +Contact:	Souradeep Chowdhury <schowdhu@codeaurora.org>
> +Description:
> +		This file is used to configure the linkedlist data
> +		to be used while configuring addresses.

Please describe the format of this attr. Is it read/write?

Regards,
Bjorn

> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel