lkml.org 
[lkml]   [2022]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V11 2/7] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)
From

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

\
 
 \ /
  Last update: 2022-09-19 06:40    [W:0.045 / U:0.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site