Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 9 Jan 2023 09:40:25 -0600
From:   Bjorn Andersson <andersson@kernel.org>
To:     Souradeep Chowdhury <quic_schowdhu@quicinc.com>
Cc:     Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Alex Elder <elder@ieee.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org,
        Sai Prakash Ranjan <quic_saipraka@quicinc.com>,
        Sibi Sankar <quic_sibis@quicinc.com>,
        Rajendra Nayak <quic_rjendra@quicinc.com>, vkoul@kernel.org
Subject: Re: [PATCH V1 1/1] soc: qcom: dcc: Add QAD, Ctitrigger and
 Bootconfig support for DCC
Message-ID: <20230109154025.sy5nxgrrzjoon5nc@builder.lan>
References: <cover.1673270769.git.quic_schowdhu@quicinc.com>
 <90a5a60b6f4cc84321370acaa0f2876dcaae7580.1673270769.git.quic_schowdhu@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <90a5a60b6f4cc84321370acaa0f2876dcaae7580.1673270769.git.quic_schowdhu@quicinc.com>
Precedence: bulk

On Mon, Jan 09, 2023 at 07:32:26PM +0530, Souradeep Chowdhury wrote:
> Add the QAD, ctitrigger and bootconfig support for dcc driver.
> The QAD is used for access control of the DCC configurations
> and can be set or unset by writing to DCC_QAD_OUTPUT register.
> The Cti-trigger is used for the cross trigger interface, on
> enabling it the software trigger can be initiated for dcc by
> setting the cti-trigout.
> Bootconfig support for DCC is for configuring register values
> to dcc at boot time, this is needed for debugging crashes that
> happen at boot time.
> Add the debugfs files QAD and CTI-trigger for each list folder.
> Also add hwtrigger debugfs file which needs to be disabled on
> enabling the cti-trigger.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  Documentation/ABI/testing/debugfs-driver-dcc |  24 +++
>  drivers/soc/qcom/dcc.c                       | 284 ++++++++++++++++++++++++++-
>  2 files changed, 304 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/debugfs-driver-dcc b/Documentation/ABI/testing/debugfs-driver-dcc
> index 27ed591..f817a9f 100644
> --- a/Documentation/ABI/testing/debugfs-driver-dcc
> +++ b/Documentation/ABI/testing/debugfs-driver-dcc
> @@ -125,3 +125,27 @@ Description:
>  		on manual or crash induced triggers. Lists must
>  		be configured and enabled sequentially, e.g. list
>  		2 can only be enabled when list 1 have so.
> +
> +What:		/sys/kernel/debug/dcc/.../[list-number]/ctitrigger
> +Date:		January 2023
> +Contact:	Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +Description:
> +		This debugfs interface is used for enabling the
> +		ctitrigger. Ctitrigger can be enabled by writing
> +		a '1' to the file.
> +
> +What:		/sys/kernel/debug/dcc/.../[list-number]/QAD
> +Date:           January 2023
> +Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +Description:
> +		This debugfs interface is used for enabling the
> +		ctitrigger. Ctitrigger can be enabled by writing
> +		a '1' to the file.
> +
> +What:           /sys/kernel/debug/dcc/.../[list-number]/hwtrigger
> +Date:	        January 2023
> +Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +Description:
> +	        This debugfs interface is used for enabling the
> +	        hwtrigger support. Hwtrigger can be enabled by
> +	        writing a '1' to the file.
> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
> index 5b50d63..ea44dbf 100644
> --- a/drivers/soc/qcom/dcc.c
> +++ b/drivers/soc/qcom/dcc.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/bootconfig.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/fs.h>
> @@ -36,6 +37,8 @@
>  #define DCC_LL_INT_STATUS		0x1c
>  #define DCC_LL_SW_TRIGGER		0x2c
>  #define DCC_LL_BUS_ACCESS_STATUS	0x30
> +#define DCC_CTI_TRIG                    0x34
> +#define DCC_QAD_OUTPUT                  0x38
>  
>  /* Default value used if a bit 6 in the HW_INFO register is set. */
>  #define DCC_FIX_LOOP_OFFSET		16
> @@ -114,6 +117,9 @@ struct dcc_config_entry {
>   * @nr_link_list:	Total number of linkedlists supported by the DCC configuration
>   * @loop_shift:	Loop offset bits range for the addresses
>   * @enable_bitmap:	Bitmap to capture the enabled status of each linked list of addresses
> + * @cti_bitmap:		Bitmap to capture the cti-trigger status of each linked list of addresses
> + * @qad_bitmap:		Bitmap to capture the qad status of each linked list of addresses
> + * @hwtrig_bitmap:	Bitmap to capture the hwtrig status of each linked list of addresses
>   */
>  struct dcc_drvdata {
>  	void __iomem		*base;
> @@ -131,6 +137,9 @@ struct dcc_drvdata {
>  	size_t			nr_link_list;
>  	u8			loop_shift;
>  	unsigned long		*enable_bitmap;
> +	unsigned long		*cti_bitmap;
> +	unsigned long           *qad_bitmap;
> +	unsigned long           *hwtrig_bitmap;
>  };
>  
>  struct dcc_cfg_attr {
> @@ -148,6 +157,18 @@ struct dcc_cfg_loop_attr {
>  	bool	loop_start;
>  };
>  
> +static char *replace_char(char *str, char find, char replace)
> +{
> +	char *current_pos = strchr(str, find);
> +
> +	while (current_pos) {
> +		*current_pos = replace;
> +		current_pos = strchr(current_pos, find);
> +	}
> +
> +	return str;
> +}
> +
>  static inline u32 dcc_list_offset(int version)
>  {
>  	return version == 1 ? 0x1c : version == 2 ? 0x2c : 0x34;
> @@ -201,6 +222,10 @@ static int dcc_sw_trigger(struct dcc_drvdata *drvdata)
>  			continue;
>  		ll_cfg = dcc_list_readl(drvdata, i, DCC_LL_CFG);
>  		tmp_ll_cfg = ll_cfg & ~DCC_TRIGGER_MASK;
> +		if (drvdata->mem_map_ver != 3)
> +			tmp_ll_cfg = ll_cfg & ~DCC_TRIGGER_MASK;
> +		else
> +			tmp_ll_cfg = ll_cfg & ~BIT(8);
>  		dcc_list_writel(drvdata, tmp_ll_cfg, i, DCC_LL_CFG);
>  		dcc_list_writel(drvdata, 1, i, DCC_LL_SW_TRIGGER);
>  		dcc_list_writel(drvdata, ll_cfg, i, DCC_LL_CFG);
> @@ -577,6 +602,25 @@ static int dcc_enable(struct dcc_drvdata *drvdata, unsigned int curr_list)
>  	/* 5. Configure trigger */
>  	dcc_list_writel(drvdata, DCC_TRIGGER_MASK,
>  			curr_list, DCC_LL_CFG);
> +	if (drvdata->mem_map_ver == 3) {
> +		dcc_list_writel(drvdata, test_bit(curr_list, drvdata->qad_bitmap), curr_list,
> +				DCC_QAD_OUTPUT);
> +		dcc_list_writel(drvdata, test_bit(curr_list, drvdata->cti_bitmap), curr_list,
> +				DCC_CTI_TRIG);
> +		if (test_bit(curr_list, drvdata->hwtrig_bitmap))
> +			dcc_list_writel(drvdata, BIT(8), curr_list, DCC_LL_CFG);
> +		else
> +			dcc_list_writel(drvdata, (unsigned int)~BIT(8), curr_list, DCC_LL_CFG);
> +	} else {
> +		if (test_bit(curr_list, drvdata->hwtrig_bitmap))
> +			dcc_list_writel(drvdata, DCC_TRIGGER_MASK |
> +					test_bit(curr_list, drvdata->cti_bitmap) << 8,
> +					curr_list, DCC_LL_CFG);
> +		else
> +			dcc_list_writel(drvdata, ~DCC_TRIGGER_MASK &
> +					test_bit(curr_list, drvdata->cti_bitmap) << 8,
> +					curr_list, DCC_LL_CFG);
> +	}
>  
>  out_unlock:
>  	mutex_unlock(&drvdata->mutex);
> @@ -1103,6 +1147,168 @@ static const struct file_operations config_fops = {
>  	.release = single_release,
>  };
>  
> +static ssize_t ctitrigger_read(struct file *filp, char __user *userbuf,
> +			       size_t count, loff_t *ppos)
> +{
> +	char *buf;
> +	int curr_list;
> +	struct dcc_drvdata *drvdata = filp->private_data;
> +
> +	curr_list = dcc_filp_curr_list(filp);
> +
> +	mutex_lock(&drvdata->mutex);
> +
> +	if (test_bit(curr_list, drvdata->cti_bitmap))
> +		buf = "Y\n";
> +	else
> +		buf = "N\n";
> +
> +	mutex_unlock(&drvdata->mutex);
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> +}
> +
> +static ssize_t ctitrigger_write(struct file *filp, const char __user *userbuf,
> +				size_t count, loff_t *ppos)
> +{
> +	int ret = 0, curr_list;
> +	bool val;
> +	struct dcc_drvdata *drvdata = filp->private_data;
> +
> +	curr_list = dcc_filp_curr_list(filp);
> +
> +	if (test_bit(curr_list, drvdata->enable_bitmap))
> +		return -EBUSY;
> +
> +	ret = kstrtobool_from_user(userbuf, count, &val);
> +	if (ret < 0)
> +		return 0;
> +
> +	if (val)
> +		set_bit(curr_list, drvdata->cti_bitmap);
> +	else
> +		clear_bit(curr_list, drvdata->cti_bitmap);
> +
> +	return count;
> +}
> +
> +static const struct file_operations ctitrigger_fops = {
> +	.read = ctitrigger_read,
> +	.write = ctitrigger_write,
> +	.open = simple_open,
> +	.llseek = generic_file_llseek,
> +};
> +
> +static ssize_t qad_read(struct file *filp, char __user *userbuf,
> +			size_t count, loff_t *ppos)
> +{
> +	char *buf;
> +	int curr_list;
> +
> +	struct dcc_drvdata *drvdata = filp->private_data;
> +
> +	curr_list = dcc_filp_curr_list(filp);
> +
> +	mutex_lock(&drvdata->mutex);
> +
> +	if (test_bit(curr_list, drvdata->qad_bitmap))
> +		buf = "Y\n";
> +	else
> +		buf = "N\n";
> +
> +	mutex_unlock(&drvdata->mutex);
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> +}
> +
> +static ssize_t qad_write(struct file *filp, const char __user *userbuf,
> +			 size_t count, loff_t *ppos)
> +{
> +	int ret = 0, curr_list;

First use of ret is an assignment, no need to assign it here.

> +	bool val;
> +	struct dcc_drvdata *drvdata = filp->private_data;
> +
> +	curr_list = dcc_filp_curr_list(filp);
> +
> +	if (drvdata->mem_map_ver != 3) {
> +		dev_err(drvdata->dev, "QAD is not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	if (test_bit(curr_list, drvdata->enable_bitmap))
> +		return -EBUSY;
> +
> +	ret = kstrtobool_from_user(userbuf, count, &val);
> +	if (ret < 0)
> +		return 0;

0 isn't a good return value here.

> +
> +	if (val)
> +		set_bit(curr_list, drvdata->qad_bitmap);
> +	else
> +		clear_bit(curr_list, drvdata->qad_bitmap);
> +
> +	return count;
> +}
> +
> +static const struct file_operations qad_fops = {
> +	.read = qad_read,
> +	.write = qad_write,
> +	.open = simple_open,
> +	.llseek = generic_file_llseek,
> +};
> +
> +static ssize_t hwtrigger_read(struct file *filp, char __user *userbuf,
> +			      size_t count, loff_t *ppos)
> +{
> +	char *buf;
> +	int curr_list;
> +	struct dcc_drvdata *drvdata = filp->private_data;
> +
> +	curr_list = dcc_filp_curr_list(filp);
> +
> +	mutex_lock(&drvdata->mutex);
> +
> +	if (test_bit(curr_list, drvdata->hwtrig_bitmap))
> +		buf = "Y\n";
> +	else
> +		buf = "N\n";
> +
> +	mutex_unlock(&drvdata->mutex);
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> +}
> +
> +static ssize_t hwtrigger_write(struct file *filp, const char __user *userbuf,
> +			       size_t count, loff_t *ppos)
> +{
> +	int ret = 0, curr_list;
> +	bool val;
> +	struct dcc_drvdata *drvdata = filp->private_data;
> +
> +	curr_list = dcc_filp_curr_list(filp);
> +
> +	if (test_bit(curr_list, drvdata->enable_bitmap))
> +		return -EBUSY;
> +
> +	ret = kstrtobool_from_user(userbuf, count, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val)
> +		set_bit(curr_list, drvdata->hwtrig_bitmap);
> +	else
> +		clear_bit(curr_list, drvdata->hwtrig_bitmap);
> +
> +	return count;
> +}
> +
> +static const struct file_operations hwtrigger_fops = {
> +	.read = hwtrigger_read,
> +	.write = hwtrigger_write,
> +	.open = simple_open,
> +	.llseek = generic_file_llseek,
> +};
> +
>  static void dcc_delete_debug_dir(struct dcc_drvdata *drvdata)
>  {
>  	 debugfs_remove_recursive(drvdata->dbg_dir);
> @@ -1126,6 +1332,9 @@ static void dcc_create_debug_dir(struct dcc_drvdata *drvdata)
>  		list = debugfs_create_dir(list_num, drvdata->dbg_dir);
>  		debugfs_create_file("enable", 0600, list, drvdata, &enable_fops);
>  		debugfs_create_file("config", 0600, list, drvdata, &config_fops);
> +		debugfs_create_file("ctitrigger", 0600, list, drvdata, &ctitrigger_fops);
> +		debugfs_create_file("QAD", 0600, list, drvdata, &qad_fops);
> +		debugfs_create_file("hwtrigger", 0600, list, drvdata, &hwtrigger_fops);
>  	}
>  
>  	debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops);
> @@ -1185,13 +1394,60 @@ static void dcc_sram_dev_exit(struct dcc_drvdata *drvdata)
>  	misc_deregister(&drvdata->sram_dev);
>  }
>  
> -static int dcc_probe(struct platform_device *pdev)
> +static int __init dcc_bootconfig_parse(struct dcc_drvdata *drvdata, struct  xbc_node  *dcc_node)
> +{
> +	struct xbc_node *linked_list, *node;
> +	int curr_list, ret;
> +	const char *p;
> +	char *input, *token;
> +	char val[30];
> +
> +	xbc_node_for_each_subkey(dcc_node, linked_list) {
> +		p = xbc_node_find_value(linked_list, "qcom-curr-link-list", &node);
> +		if (p) {

So if I specify 

dcc-config {
	list1 {
		qcom-curr-link-list = 1
	}
	list2 {

	}
}

Then list2 should have &curr_list of 1 as well?

Or did you miss the error handling here?


PS. I think you can skip the "qcom-" prefix. Frankly, you know that
you're in a subnode of dcc_config; use "id" and "items" ("list"?).

> +			ret = kstrtoint(p, 0, &curr_list);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		p = xbc_node_find_value(linked_list, "qcom-link-list", &node);
> +		if (!p)
> +			continue;
> +
> +		xbc_array_for_each_value(node, p) {
> +			snprintf(val, sizeof(val), "%s", p);

strscpy() would be less costly.

> +			input = replace_char(val, '_', ' ');
> +			token = strsep(&input, " ");
> +
> +			if (!strcmp("R", token)) {
> +				ret = dcc_config_add_read(drvdata, input, curr_list);
> +			} else if (!strcmp("W", token)) {
> +				ret = dcc_config_add_write(drvdata, input, curr_list);
> +			} else if (!strcmp("RW", token)) {
> +				ret = dcc_config_add_read_write(drvdata, input, curr_list);
> +			} else if (!strcmp("L", token)) {
> +				ret = dcc_config_add_loop(drvdata, input, curr_list);
> +			} else {
> +				dev_err(drvdata->dev, "%s is not a correct input\n", token);
> +				return -EINVAL;
> +			}
> +			if (ret)
> +				return ret;
> +		}
> +		dcc_enable(drvdata, curr_list);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init dcc_probe(struct platform_device *pdev)
>  {
>  	u32 val;
>  	int ret = 0, i;
>  	struct device *dev = &pdev->dev;
>  	struct dcc_drvdata *drvdata;
>  	struct resource *res;
> +	struct xbc_node *dcc_node;
>  
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata)
> @@ -1247,13 +1503,30 @@ static int dcc_probe(struct platform_device *pdev)
>  	if (!drvdata->enable_bitmap)
>  		return -ENOMEM;
>  
> +	drvdata->cti_bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drvdata->nr_link_list),
> +					   sizeof(*drvdata->cti_bitmap), GFP_KERNEL);

How about devm_bitmap_alloc()?

> +	if (!drvdata->cti_bitmap)
> +		return -ENOMEM;
> +
> +	drvdata->qad_bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drvdata->nr_link_list),
> +					   sizeof(*drvdata->qad_bitmap), GFP_KERNEL);
> +	if (!drvdata->qad_bitmap)
> +		return -ENOMEM;
> +
> +	drvdata->hwtrig_bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drvdata->nr_link_list),
> +					      sizeof(*drvdata->hwtrig_bitmap), GFP_KERNEL);
> +	if (!drvdata->hwtrig_bitmap)
> +		return -ENOMEM;
> +
>  	drvdata->cfg_head = devm_kcalloc(dev, drvdata->nr_link_list,
>  					 sizeof(*drvdata->cfg_head), GFP_KERNEL);
>  	if (!drvdata->cfg_head)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < drvdata->nr_link_list; i++)
> +	for (i = 0; i < drvdata->nr_link_list; i++) {
>  		INIT_LIST_HEAD(&drvdata->cfg_head[i]);
> +		set_bit(i, drvdata->hwtrig_bitmap);
> +	}
>  
>  	ret = dcc_sram_dev_init(drvdata);
>  	if (ret) {
> @@ -1263,6 +1536,10 @@ static int dcc_probe(struct platform_device *pdev)
>  
>  	dcc_create_debug_dir(drvdata);
>  
> +	dcc_node = xbc_find_node("dcc_config");
> +	if (dcc_node)
> +		return dcc_bootconfig_parse(drvdata, dcc_node);

Wouldn't it make sense to still probe dcc, even if the configuration is
rejected?

> +
>  	return 0;
>  }
>  
> @@ -1287,14 +1564,13 @@ static const struct of_device_id dcc_match_table[] = {
>  MODULE_DEVICE_TABLE(of, dcc_match_table);
>  
>  static struct platform_driver dcc_driver = {
> -	.probe = dcc_probe,
>  	.remove	= dcc_remove,
>  	.driver	= {
>  		.name = "qcom-dcc",
>  		.of_match_table	= dcc_match_table,
>  	},
>  };
> -module_platform_driver(dcc_driver);
> +module_platform_driver_probe(dcc_driver, dcc_probe);

This change is unrelated to the others, and I don't see it mentioned
in your commit message.

Regards,
Bjorn

>  
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Qualcomm Technologies Inc. DCC driver");
> -- 
> 2.7.4
>