[PATCH V5 0/2] qcom aoss qmp_get and debugfs support patches

[PATCH V5 0/2] qcom aoss qmp_get and debugfs support patches

[Deepak Kumar Singh]

[Changes from V4]
Revert back to get_device/put_device based approach.
Remove refcount and orphan variables in qmp handle.
Use devres api for qmp handle allocation as removing refcount based approach

Deepak Kumar Singh (2):
  soc: qcom: aoss: Expose send for generic usecase
  soc: qcom: aoss: Add debugfs entry

 drivers/soc/qcom/qcom_aoss.c       | 86 +++++++++++++++++++++++++++++++++++++-
 include/linux/soc/qcom/qcom_aoss.h | 38 +++++++++++++++++
 2 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/soc/qcom/qcom_aoss.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V5 1/2] soc: qcom: aoss: Expose send for generic usecase

[Deepak Kumar Singh]

Not all upcoming usecases will have an interface to allow the aoss
driver to hook onto. Expose the send api and create a get function to
enable drivers to send their own messages to aoss.

Signed-off-by: Chris Lew <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/soc/qcom/qcom_aoss.c       | 46 +++++++++++++++++++++++++++++++++++++-
 include/linux/soc/qcom/qcom_aoss.h | 38 +++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/soc/qcom/qcom_aoss.h

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 934fcc4..b84cb31 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -8,10 +8,12 @@
 #include <linux/io.h>
 #include <linux/mailbox_client.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/thermal.h>
 #include <linux/slab.h>
+#include <linux/soc/qcom/qcom_aoss.h>
 
 #define QMP_DESC_MAGIC			0x0
 #define QMP_DESC_VERSION		0x4
@@ -223,11 +225,14 @@ static bool qmp_message_empty(struct qmp *qmp)
  *
  * Return: 0 on success, negative errno on failure
  */
-static int qmp_send(struct qmp *qmp, const void *data, size_t len)
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
 {
 	long time_left;
 	int ret;
 
+	if (WARN_ON(IS_ERR_OR_NULL(qmp) || !data))
+		return -EINVAL;
+
 	if (WARN_ON(len + sizeof(u32) > qmp->size))
 		return -EINVAL;
 
@@ -261,6 +266,7 @@ static int qmp_send(struct qmp *qmp, const void *data, size_t len)
 
 	return ret;
 }
+EXPORT_SYMBOL(qmp_send);
 
 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {
@@ -515,6 +521,43 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
 		thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
 }
 
+/**
+ * qmp_get() - get a qmp handle from a device
+ * @dev: client device pointer
+ *
+ * Return: handle to qmp device on success, ERR_PTR() on failure
+ */
+struct qmp *qmp_get(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+	struct qmp *qmp;
+
+	if (!dev || !dev->of_node)
+		return ERR_PTR(-EINVAL);
+
+	np = of_parse_phandle(dev->of_node, "qcom,qmp", 0);
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+	if (!pdev)
+		return ERR_PTR(-EINVAL);
+
+	qmp = platform_get_drvdata(pdev);
+
+	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL(qmp_get);
+
+void qmp_put(struct qmp *qmp)
+{
+	if (!IS_ERR_OR_NULL(qmp))
+		put_device(qmp->dev);
+}
+EXPORT_SYMBOL(qmp_put);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -610,6 +653,7 @@ static struct platform_driver qmp_driver = {
 	.driver = {
 		.name		= "qcom_aoss_qmp",
 		.of_match_table	= qmp_dt_match,
+		.suppress_bind_attrs = true,
 	},
 	.probe = qmp_probe,
 	.remove	= qmp_remove,
diff --git a/include/linux/soc/qcom/qcom_aoss.h b/include/linux/soc/qcom/qcom_aoss.h
new file mode 100644
index 0000000..3c2a82e
--- /dev/null
+++ b/include/linux/soc/qcom/qcom_aoss.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_AOSS_H__
+#define __QCOM_AOSS_H__
+
+#include <linux/err.h>
+#include <linux/device.h>
+
+struct qmp;
+
+#if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len);
+struct qmp *qmp_get(struct device *dev);
+void qmp_put(struct qmp *qmp);
+
+#else
+
+static inline int qmp_send(struct qmp *qmp, const void *data, size_t len)
+{
+	return -ENODEV;
+}
+
+static inline struct qmp *qmp_get(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline void qmp_put(struct qmp *qmp)
+{
+}
+
+#endif
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V5 1/2] soc: qcom: aoss: Expose send for generic usecase

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-08-05 04:12:53)
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 934fcc4..b84cb31 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -515,6 +521,43 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
>                 thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
>  }
>
> +/**
> + * qmp_get() - get a qmp handle from a device
> + * @dev: client device pointer
> + *
> + * Return: handle to qmp device on success, ERR_PTR() on failure
> + */
> +struct qmp *qmp_get(struct device *dev)
> +{
> +       struct platform_device *pdev;
> +       struct device_node *np;
> +       struct qmp *qmp;
> +
> +       if (!dev || !dev->of_node)
> +               return ERR_PTR(-EINVAL);
> +
> +       np = of_parse_phandle(dev->of_node, "qcom,qmp", 0);
> +       if (!np)
> +               return ERR_PTR(-ENODEV);
> +
> +       pdev = of_find_device_by_node(np);
> +       of_node_put(np);
> +       if (!pdev)
> +               return ERR_PTR(-EINVAL);
> +
> +       qmp = platform_get_drvdata(pdev);
> +
> +       return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL(qmp_get);
> +
> +void qmp_put(struct qmp *qmp)
> +{
> +       if (!IS_ERR_OR_NULL(qmp))
> +               put_device(qmp->dev);

Where is the corresponding get_device() call?

> +}
> +EXPORT_SYMBOL(qmp_put);
> +
>  static int qmp_probe(struct platform_device *pdev)
>  {
>         struct resource *res;

Re: [PATCH V5 1/2] soc: qcom: aoss: Expose send for generic usecase

[Deepak Kumar Singh]

On 8/6/2021 12:04 AM, Stephen Boyd wrote:
> Quoting Deepak Kumar Singh (2021-08-05 04:12:53)
>> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
>> index 934fcc4..b84cb31 100644
>> --- a/drivers/soc/qcom/qcom_aoss.c
>> +++ b/drivers/soc/qcom/qcom_aoss.c
>> @@ -515,6 +521,43 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
>>                  thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
>>   }
>>
>> +/**
>> + * qmp_get() - get a qmp handle from a device
>> + * @dev: client device pointer
>> + *
>> + * Return: handle to qmp device on success, ERR_PTR() on failure
>> + */
>> +struct qmp *qmp_get(struct device *dev)
>> +{
>> +       struct platform_device *pdev;
>> +       struct device_node *np;
>> +       struct qmp *qmp;
>> +
>> +       if (!dev || !dev->of_node)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       np = of_parse_phandle(dev->of_node, "qcom,qmp", 0);
>> +       if (!np)
>> +               return ERR_PTR(-ENODEV);
>> +
>> +       pdev = of_find_device_by_node(np);
>> +       of_node_put(np);
>> +       if (!pdev)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       qmp = platform_get_drvdata(pdev);
>> +
>> +       return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
>> +}
>> +EXPORT_SYMBOL(qmp_get);
>> +
>> +void qmp_put(struct qmp *qmp)
>> +{
>> +       if (!IS_ERR_OR_NULL(qmp))
>> +               put_device(qmp->dev);
> Where is the corresponding get_device() call?
of_find_device_by_node() internally calls get_device(), so no need to 
call get_device() explicitly.
>
>> +}
>> +EXPORT_SYMBOL(qmp_put);
>> +
>>   static int qmp_probe(struct platform_device *pdev)
>>   {
>>          struct resource *res;

Re: [PATCH V5 1/2] soc: qcom: aoss: Expose send for generic usecase

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-08-09 02:57:26)
>
> On 8/6/2021 12:04 AM, Stephen Boyd wrote:
> > Quoting Deepak Kumar Singh (2021-08-05 04:12:53)
> >> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> >> index 934fcc4..b84cb31 100644
> >> --- a/drivers/soc/qcom/qcom_aoss.c
> >> +++ b/drivers/soc/qcom/qcom_aoss.c
> >> @@ -515,6 +521,43 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
> >>                  thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
> >>   }
> >>
> >> +/**
> >> + * qmp_get() - get a qmp handle from a device
> >> + * @dev: client device pointer
> >> + *
> >> + * Return: handle to qmp device on success, ERR_PTR() on failure
> >> + */
> >> +struct qmp *qmp_get(struct device *dev)
> >> +{
> >> +       struct platform_device *pdev;
> >> +       struct device_node *np;
> >> +       struct qmp *qmp;
> >> +
> >> +       if (!dev || !dev->of_node)
> >> +               return ERR_PTR(-EINVAL);
> >> +
> >> +       np = of_parse_phandle(dev->of_node, "qcom,qmp", 0);
> >> +       if (!np)
> >> +               return ERR_PTR(-ENODEV);
> >> +
> >> +       pdev = of_find_device_by_node(np);
> >> +       of_node_put(np);
> >> +       if (!pdev)
> >> +               return ERR_PTR(-EINVAL);
> >> +
> >> +       qmp = platform_get_drvdata(pdev);
> >> +
> >> +       return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
> >> +}
> >> +EXPORT_SYMBOL(qmp_get);
> >> +
> >> +void qmp_put(struct qmp *qmp)
> >> +{
> >> +       if (!IS_ERR_OR_NULL(qmp))
> >> +               put_device(qmp->dev);
> > Where is the corresponding get_device() call?
> of_find_device_by_node() internally calls get_device(), so no need to
> call get_device() explicitly.

Got it. Can you add a comment here so we remember?

	/*
	 * Match get_device() inside of_find_device_by_node() in
	 * qmp_get()
	 */

and it would be super useful if we could update the documentation on
that API, and the one it calls, so callers know about this.

[PATCH V5 2/2] soc: qcom: aoss: Add debugfs entry

[Deepak Kumar Singh]

It can be useful to control the different power states of various
parts of hardware for device testing. Add a debugfs node for qmp so
messages can be sent to aoss for debugging and testing purposes.

Signed-off-by: Chris Lew <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/soc/qcom/qcom_aoss.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index b84cb31..16e3d65 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -4,6 +4,7 @@
  */
 #include <dt-bindings/power/qcom-aoss-qmp.h>
 #include <linux/clk-provider.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/mailbox_client.h>
@@ -86,6 +87,9 @@ struct qmp {
 	struct clk_hw qdss_clk;
 	struct genpd_onecell_data pd_data;
 	struct qmp_cooling_device *cooling_devs;
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	struct dentry *debugfs_file;
+#endif /* CONFIG_DEBUG_FS */
 };
 
 struct qmp_pd {
@@ -558,6 +562,33 @@ void qmp_put(struct qmp *qmp)
 }
 EXPORT_SYMBOL(qmp_put);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static ssize_t aoss_dbg_write(struct file *file, const char __user *userstr,
+			      size_t len, loff_t *pos)
+{
+	struct qmp *qmp = file->private_data;
+	char buf[QMP_MSG_LEN] = {};
+	int ret;
+
+	if (!len || len >= QMP_MSG_LEN)
+		return -EINVAL;
+
+	ret  = copy_from_user(buf, userstr, len);
+	if (ret) {
+		return -EFAULT;
+	}
+
+	ret = qmp_send(qmp, buf, QMP_MSG_LEN);
+
+	return ret ? ret : len;
+}
+
+static const struct file_operations aoss_dbg_fops = {
+	.open = simple_open,
+	.write = aoss_dbg_write,
+};
+#endif /* CONFIG_DEBUG_FS */
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -612,6 +643,11 @@ static int qmp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, qmp);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	qmp->debugfs_file = debugfs_create_file("aoss_send_message", 0220, NULL,
+						qmp, &aoss_dbg_fops);
+#endif /* CONFIG_DEBUG_FS */
+
 	return 0;
 
 err_remove_qdss_clk:
@@ -628,6 +664,10 @@ static int qmp_remove(struct platform_device *pdev)
 {
 	struct qmp *qmp = platform_get_drvdata(pdev);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	debugfs_remove(qmp->debugfs_file);
+#endif /* CONFIG_DEBUG_FS */
+
 	qmp_qdss_clk_remove(qmp);
 	qmp_pd_remove(qmp);
 	qmp_cooling_devices_remove(qmp);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V5 2/2] soc: qcom: aoss: Add debugfs entry

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-08-05 04:12:54)
> It can be useful to control the different power states of various
> parts of hardware for device testing. Add a debugfs node for qmp so
> messages can be sent to aoss for debugging and testing purposes.

Is it ever useful after device testing? I'd prefer we not apply this
patch as it looks like testing code that won't ever be used after
developing this driver.

Re: [PATCH V5 2/2] soc: qcom: aoss: Add debugfs entry

[Deepak Kumar Singh]

On 8/5/2021 11:58 PM, Stephen Boyd wrote:
> Quoting Deepak Kumar Singh (2021-08-05 04:12:54)
>> It can be useful to control the different power states of various
>> parts of hardware for device testing. Add a debugfs node for qmp so
>> messages can be sent to aoss for debugging and testing purposes.
> Is it ever useful after device testing? I'd prefer we not apply this
> patch as it looks like testing code that won't ever be used after
> developing this driver.

This is not only for testing. Some user space clients can also use this 
to send messages to aoss.

One such example is setting higher ddr frequency during boot and 
reducing it post boot from user space.

Re: [PATCH V5 2/2] soc: qcom: aoss: Add debugfs entry

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-08-30 04:46:53)
>
> On 8/5/2021 11:58 PM, Stephen Boyd wrote:
> > Quoting Deepak Kumar Singh (2021-08-05 04:12:54)
> >> It can be useful to control the different power states of various
> >> parts of hardware for device testing. Add a debugfs node for qmp so
> >> messages can be sent to aoss for debugging and testing purposes.
> > Is it ever useful after device testing? I'd prefer we not apply this
> > patch as it looks like testing code that won't ever be used after
> > developing this driver.
>
> This is not only for testing. Some user space clients can also use this
> to send messages to aoss.
>
> One such example is setting higher ddr frequency during boot and
> reducing it post boot from user space.
>

The debugfs file system should not be used by userspace to do things
like that. It's a debugging file system, not a configuration file
system. If you want to expose userspace control for this it needs to be
done in a different way.