Message-ID: <862e74d9-6390-f0ab-3708-039f987e7a1e@quicinc.com>
Date:   Mon, 19 Sep 2022 10:09:04 +0530
From:   Souradeep Chowdhury <quic_schowdhu@...cinc.com>
To:     Bjorn Andersson <andersson@...nel.org>
CC:     Andy Gross <agross@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        "Alex Elder" <elder@...e.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        Sai Prakash Ranjan <quic_saipraka@...cinc.com>,
        Sibi Sankar <quic_sibis@...cinc.com>,
        Rajendra Nayak <quic_rjendra@...cinc.com>, <vkoul@...nel.org>
Subject: Re: [PATCH V11 2/7] soc: qcom: dcc: Add driver support for Data
 Capture and Compare unit(DCC)


On 9/17/2022 5:46 AM, Bjorn Andersson wrote:
> On Fri, Sep 16, 2022 at 02:20:14PM +0530, 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 user inputs via the debugfs interface. The user gives
>> addresses as inputs and these addresses are stored in the
>> dcc sram. In case of a system crash or a manual software
>> trigger by the user through the debugfs interface,
>> the dcc captures and stores the values at these addresses.
>> This patch contains the driver which has all the methods
>> pertaining to the debugfs interface, auxiliary functions to
>> support all the four fundamental operations of dcc namely
>> read, write, read/modify/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 software
>> triggers as well based on user inputs.
>>
>> Also added the documentation for debugfs entries and explained
>> the functionalities of each debugfs file that has been created
>> for dcc.
>>
>> The following is the justification of using debugfs interface
>> over the other alternatives like sysfs/ioctls
>>
>> i) As can be seen from the debugfs attribute descriptions,
>> some of the debugfs attribute files here contains multiple
>> arguments which needs to be accepted from the user. This goes
>> against the design style of sysfs.
>>
>> ii) The user input patterns have been made simple and convenient
>> in this case with the use of debugfs interface as user doesn't
>> need to shuffle between different files to execute one instruction
>> as was the case on using other alternatives.
>>
> Thanks for fixing up the checkpatch warnings.
>
> I did unfortunately find a few more things as I was looking through this
> revision.
>
>> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
>> new file mode 100644
>> index 0000000..7e07f1a
>> --- /dev/null
>> +++ b/drivers/soc/qcom/dcc.c
>> @@ -0,0 +1,1341 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copneight (c) 2015-2021, The Linex Fnendation. All rights reserved.
> Something funny happened to Copneight, Linuex and Fnendation.
>
> Should you perhaps also bump the year to 2022?
Ack
>
>> + */
> [..]
>> +/**
>> + * 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
>> + * @ram_base:         Base address for the SRAM dedicated for the dcc device
> @ram_size missing
Ack
>
>> + * @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
>> + * @sram_dev:	      Micellaneous device equivalent of dcc SRAM
>> + * @cfg_head:	      Points to the head of the linked list of addresses
>> + * @dbg_dir:          The dcc debugfs directory under which all the debugfs files are placed
>> + * @nr_link_list:     Total number of linkedlists supported by the DCC configuration
>> + * @loopoff:          Loop offset bits range for the addresses
>> + * @enable:           This contains an array of linkedlist enable flags
>> + */
>> +struct dcc_drvdata {
>> +	void __iomem		*base;
>> +	struct device		*dev;
>> +	struct mutex		mutex;
>> +	void __iomem		*ram_base;
>> +	size_t			ram_size;
>> +	size_t			ram_offset;
>> +	int			mem_map_ver;
>> +	phys_addr_t		ram_cfg;
>> +	phys_addr_t		ram_start;
>> +	struct miscdevice	sram_dev;
>> +	struct list_head	*cfg_head;
>> +	struct dentry		*dbg_dir;
>> +	size_t			nr_link_list;
>> +	u8			loopoff;
>> +	bool                    *enable;
>> +};
> [..]
>> +static int dcc_read_and_clear(struct dcc_drvdata *drvdata)
>> +{
>> +	int i;
>> +	u32 bus_status;
>> +	u32 ll_cfg;
>> +	u32 tmp_ll_cfg;
>> +
>> +	for (i = 0; i < drvdata->nr_link_list; i++) {
>> +		if (!drvdata->enable[i])
>> +			continue;
>> +
>> +		bus_status = dcc_ll_readl(drvdata, i, DCC_LL_BUS_ACCESS_STATUS);
>> +		if (!bus_status)
>> +			continue;
>> +
>> +		dev_err(drvdata->dev,
>> +			"Read access error for list %d err: 0x%x.\n", i, bus_status);
> This can be unwrapped, or it would look more natural if you wrapped it
> after the string.
>
> You should also drop the '.' at the end of the message.
>
> If you want to shorten it down a little bit more, you could rename
> bus_status to status, as there's no ambiguity here.
Ack
>
>> +		ll_cfg = dcc_ll_readl(drvdata, i, DCC_LL_CFG);
>> +		tmp_ll_cfg = ll_cfg & ~DCC_TRIGGER_MASK;
>> +		dcc_ll_writel(drvdata, tmp_ll_cfg, i, DCC_LL_CFG);
>> +		dcc_ll_writel(drvdata, DCC_STATUS_MASK, i,
>> +			      DCC_LL_BUS_ACCESS_STATUS);
> This can be unwrapped.
Ack
>
>> +		dcc_ll_writel(drvdata, ll_cfg, i, DCC_LL_CFG);
>> +		return -ENODATA;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dcc_sw_trigger(struct dcc_drvdata *drvdata)
>> +{
>> +	void __iomem *addr;
>> +	int ret;
>> +	int i;
>> +	u32 ll_cfg;
>> +	u32 tmp_ll_cfg;
>> +	u32 val;
>> +
>> +	mutex_lock(&drvdata->mutex);
>> +
>> +	for (i = 0; i < drvdata->nr_link_list; i++) {
>> +		if (!drvdata->enable[i])
>> +			continue;
>> +		ll_cfg = dcc_ll_readl(drvdata, i, DCC_LL_CFG);
>> +		tmp_ll_cfg = ll_cfg & ~DCC_TRIGGER_MASK;
>> +		dcc_ll_writel(drvdata, tmp_ll_cfg, i, DCC_LL_CFG);
>> +		dcc_ll_writel(drvdata, 1, i, DCC_LL_SW_TRIGGER);
>> +		dcc_ll_writel(drvdata, ll_cfg, i, DCC_LL_CFG);
>> +	}
>> +
>> +	addr = drvdata->base + DCC_STATUS(drvdata->mem_map_ver);
>> +	if (readl_poll_timeout(addr, val, (FIELD_GET(DCC_STATUS_MASK, val) == 0),
>> +			       1, STATUS_READY_TIMEOUT)) {
>> +		dev_err(drvdata->dev,
>> +			"DCC is busy after receiving sw tigger.\n");
> Unwrapp this please, and drop the '.' at the end.
Ack
>
>> +		ret = -EBUSY;
>> +		goto out_unlock;
>> +	}
>> +
>> +	ret = dcc_read_and_clear(drvdata);
>> +
>> +out_unlock:
>> +	mutex_unlock(&drvdata->mutex);
>> +	return ret;
>> +}
>> +
>> +static void _dcc_ll_cfg_reset_link(struct dcc_cfg_attr *cfg)
>> +{
>> +	cfg->addr = 0x00;
>> +	cfg->link = 0;
>> +	cfg->prev_off = 0;
>> +	cfg->prev_addr = cfg->addr;
>> +}
>> +
>> +static void _dcc_emit_read_write(struct dcc_drvdata *drvdata,
>> +				 struct dcc_config_entry *entry,
>> +				 struct dcc_cfg_attr *cfg)
>> +{
>> +	if (cfg->link) {
>> +		/*
>> +		 * write new offset = 1 to continue
>> +		 * processing the list
>> +		 */
>> +
>> +		dcc_sram_write_auto(drvdata, cfg->link, &cfg->sram_offset);
>> +
>> +		/* Reset link and prev_off */
>> +		_dcc_ll_cfg_reset_link(cfg);
>> +	}
>> +
>> +	cfg->addr = DCC_RD_MOD_WR_DESCRIPTOR;
>> +	dcc_sram_write_auto(drvdata, cfg->addr, &cfg->sram_offset);
>> +
>> +	dcc_sram_write_auto(drvdata, entry->mask, &cfg->sram_offset);
>> +
>> +	dcc_sram_write_auto(drvdata, entry->write_val, &cfg->sram_offset);
>> +
>> +	cfg->addr = 0;
>> +}
>> +
>> +static void _dcc_emit_loop(struct dcc_drvdata *drvdata, struct dcc_config_entry *entry,
>> +			   struct dcc_cfg_attr *cfg,
>> +			   struct dcc_cfg_loop_attr *cfg_loop,
>> +			   u32 *total_len)
>> +{
>> +	/* Check if we need to write link of prev entry */
>> +	if (cfg->link)
>> +		dcc_sram_write_auto(drvdata, cfg->link, &cfg->sram_offset);
>> +
>> +	if (cfg_loop->loop_start) {
>> +		cfg_loop->loop = (cfg->sram_offset - cfg_loop->loop_off) / 4;
>> +		cfg_loop->loop |= (cfg_loop->loop_cnt << drvdata->loopoff) &
>> +		GENMASK(DCC_ADDR_LIMIT, drvdata->loopoff);
> Odd line wrap. Please indent better.
Ack
>
>> +		cfg_loop->loop |= DCC_LOOP_DESCRIPTOR;
>> +		*total_len += (*total_len - cfg_loop->loop_len) * cfg_loop->loop_cnt;
>> +
>> +		dcc_sram_write_auto(drvdata, cfg_loop->loop, &cfg->sram_offset);
>> +
>> +		cfg_loop->loop_start = false;
>> +		cfg_loop->loop_len = 0;
>> +		cfg_loop->loop_off = 0;
>> +	} else {
>> +		cfg_loop->loop_start = true;
>> +		cfg_loop->loop_cnt = entry->loop_cnt - 1;
>> +		cfg_loop->loop_len = *total_len;
>> +		cfg_loop->loop_off = cfg->sram_offset;
>> +	}
>> +
>> +	/* Reset link and prev_off */
>> +	_dcc_ll_cfg_reset_link(cfg);
>> +}
>> +
>> +static void _dcc_emit_write(struct dcc_drvdata *drvdata,
>> +			    struct dcc_config_entry *entry,
>> +			    struct dcc_cfg_attr *cfg,
>> +			    u32 *total_len)
>> +{
>> +	u32 off;
>> +
>> +	if (cfg->link) {
>> +		/*
>> +		 * write new offset = 1 to continue
>> +		 * processing the list
>> +		 */
>> +		dcc_sram_write_auto(drvdata, cfg->link, &cfg->sram_offset);
>> +
>> +		/* Reset link and prev_off */
>> +		cfg->addr = 0x00;
>> +		cfg->prev_off = 0;
>> +		cfg->prev_addr = cfg->addr;
>> +	}
>> +
>> +	off = entry->offset / 4;
>> +	/* write new offset-length pair to correct position */
>> +	cfg->link |= ((off & GENMASK(7, 0)) | BIT(15) | ((entry->len << 8) & GENMASK(14, 8)));
> Would it be possible to get some defines for these masks and bits?
> Perhaps use FIELD_PREP() instead of manually mask and shift the values.
Ack
>
>> +	cfg->link |= DCC_LINK_DESCRIPTOR;
>> +
>> +	/* Address type */
>> +	cfg->addr = (entry->base >> 4) & GENMASK(DCC_ADDR_LIMIT, 0);
>> +	if (entry->apb_bus)
>> +		cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_WRITE_IND | DCC_APB_IND;
>> +	else
>> +		cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_WRITE_IND | DCC_AHB_IND;
>> +	dcc_sram_write_auto(drvdata, cfg->addr, &cfg->sram_offset);
>> +
>> +	dcc_sram_write_auto(drvdata, cfg->link, &cfg->sram_offset);
>> +
>> +	dcc_sram_write_auto(drvdata, entry->write_val, &cfg->sram_offset);
>> +
>> +	cfg->addr = 0x00;
>> +	cfg->link = 0;
>> +}
> [..]
>> +static ssize_t dcc_config_add_write(struct dcc_drvdata *drvdata, char *buf, int curr_list)
>> +{
>> +	int ret, bus;
>> +	int nval;
>> +	unsigned int addr, write_val;
>> +	char apb_bus[4];
>> +
>> +	nval = sscanf(buf, "%x %x %s", &addr, &write_val, apb_bus);
>> +
>> +	if (nval <= 1 || nval > 3)
>> +		return -EINVAL;
>> +
>> +	if (nval == 3) {
>> +		if (!strcmp("apb", apb_bus))
>> +			bus = 1;
>> +		else if (!strcmp("apb", apb_bus))
>> +			bus = 0;
>> +		else
>> +			return -EINVAL;
>> +	}
>> +
>> +	ret = dcc_add_write(drvdata, addr, write_val, bus, curr_list);
> return dcc_add_write(); and you can drop the local variable.
Ack
>
>> +
>> +	return ret;
>> +}
>> +
>> +static ssize_t config_read(struct file *filp, char __user *userbuf,
>> +			   size_t count, loff_t *ppos)
>> +{
>> +	struct dcc_drvdata *drvdata = filp->private_data;
>> +	struct dcc_config_entry *entry;
>> +	char local_buf[64], buf[100] = "\0";
>> +	int len = 0, tot_len = 0, index = 0, curr_list;
>> +
>> +	mutex_lock(&drvdata->mutex);
>> +	curr_list = dcc_filp_curr_list(filp);
>> +	if (curr_list < 0)
>> +		return curr_list;
>> +
>> +	list_for_each_entry(entry,
>> +			    &drvdata->cfg_head[curr_list], list) {
>> +		index++;
>> +		switch (entry->desc_type) {
>> +		case DCC_READ_WRITE_TYPE:
>> +			len = snprintf(local_buf, 64,
>> +				       "Index: 0x%x, mask: 0x%x, val: 0x%x\n",
> Wouldn't it be nice if this matches the format used to specify the
> instructions?
>
> I.e. "RW <addr> <value> <mask>"
Ack
>
>> +				       index, entry->mask, entry->write_val);
>> +			break;
>> +		case DCC_LOOP_TYPE:
>> +			len = snprintf(local_buf, 64, "Index: 0x%x, Loop: %d\n",
>> +				       index, entry->loop_cnt);
>> +			break;
>> +		case DCC_WRITE_TYPE:
>> +			len = snprintf(local_buf, 64,
>> +				       "Write Index: 0x%x, Base: 0x%x, Offset: 0x%x, len: 0x%x APB: %d\n",
>> +				       index, entry->base, entry->offset, entry->len,
>> +				       entry->apb_bus);
>> +			break;
>> +		case DCC_READ_TYPE:
>> +			len = snprintf(local_buf, 64,
>> +				       "Read Index: 0x%x, Base: 0x%x, Offset: 0x%x, len: 0x%x APB: %d\n",
> Can't this generate something like:
>    Read Index: 0x1, Base: 0xaabbccdd, Offset: 0xaa, len: 0xaa APB: 1
> Afaict that's 65 bytes.
>
> If that's the case you will end up with tot_len > strlen(buf). You
> probably meant to use scnprintf(), which would return 63.
>
> But either way, the tail would be chopped of.
Ack
>
>> +				       index, entry->base, entry->offset,
>> +				       entry->len, entry->apb_bus);
>> +		}
>> +		tot_len += len;
>> +		strlcat(buf, local_buf, sizeof(local_buf));
> The limit should be the destination buffer, i.e. sizeof(buf).
>
> But why is sizeof(buf) only 100? If you have a RMW in there you will fit
> 1.5 entries in the buffer.
>
>
> I think it would be better to use single_open() and use the seq_file()
> interface for generating your data. That will handle the buffer
> management for you, among other things.
Ack
>
>> +	}
>> +	mutex_unlock(&drvdata->mutex);
>> +	return simple_read_from_buffer(userbuf, count, ppos, buf, tot_len);
>> +}
>> +
>> +static ssize_t config_write(struct file *filp,
>> +			    const char __user *user_buf, size_t count,
>> +			    loff_t *ppos)
>> +{
>> +	int ret, curr_list;
>> +	char *token, buf[50];
>> +	char *delim = " ";
>> +	struct dcc_drvdata *drvdata = filp->private_data;
>> +
>> +	ret = copy_from_user(buf, user_buf, count);
> count might be > sizeof(buf).
Ack
>
>> +	if (ret)
>> +		return -EFAULT;
>> +
>> +	curr_list = dcc_filp_curr_list(filp);
>> +	if (curr_list < 0)
>> +		return curr_list;
>> +
>> +	if (buf[count - 1] == '\n')
> count can be 0.
Ack
>
>> +		buf[count - 1] = '\0';
> buf[] might not be \n or NUL-terminated.
Ack
>
>> +
>> +	token = strsep((char **)&buf, delim);
>> +
>> +	if (!strcmp("R", token)) {
>> +		ret = dcc_config_add_read(drvdata, buf, curr_list);
>> +	} else if (!strcmp("W", token)) {
>> +		ret = dcc_config_add_write(drvdata, buf, curr_list);
>> +	} else if (!strcmp("RW", token)) {
>> +		ret = dcc_config_add_read_write(drvdata, buf, curr_list);
>> +	} else if (!strcmp("L", token)) {
>> +		ret = dcc_config_add_loop(drvdata, buf, curr_list);
>> +	} else {
>> +		dev_err(drvdata->dev, "%s is not a correct input\n", token);
>> +		return -EINVAL;
>> +	}
> [..]
>> +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 = container_of(file->private_data,
>> +		struct dcc_drvdata,
>> +		sram_dev);
>> +
>> +	/* EOF check */
>> +	if (*ppos > drvdata->ram_size)
>> +		return 0;
>> +
>> +	if ((*ppos + len) > drvdata->ram_size)
>> +		len = (drvdata->ram_size - *ppos);
> If *ppos == drvdata->ram_size, the EOF check will miss that and len will
> be 0.
Ack
>
>> +
>> +	buf = kzalloc(len, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	memcpy_fromio(buf, drvdata->ram_base + *ppos, len);
>> +
>> +	if (copy_to_user(data, buf, len)) {
>> +		kfree(buf);
>> +		return -EFAULT;
>> +	}
>> +
>> +	*ppos += len;
>> +
>> +	kfree(buf);
>> +
>> +	return len;
>> +}
>> +
> Regards,
> Bjorn

Powered by blists - more mailing lists