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

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

[Chris Lew]

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>
---
 drivers/soc/qcom/qcom_aoss.c       | 28 +++++++++++++++++++++++++++-
 include/linux/soc/qcom/qcom_aoss.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 60 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 ed2c687c16b3..8f052db1880a 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
@@ -222,12 +224,15 @@ 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;
 	size_t tlen;
 	int ret;
 
+	if (!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_GPL(qmp_send);
 
 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {
@@ -515,6 +521,26 @@ 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 device tree node
+ * @np: of node of qmp device
+ *
+ * Return: handle to qmp device on success, ERR_PTR() on failure
+ */
+struct qmp_device *qmp_get(struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct qmp *qmp;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return ERR_PTR(-EINVAL);
+
+	qmp = platform_get_drvdata(pdev);
+	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
diff --git a/include/linux/soc/qcom/qcom_aoss.h b/include/linux/soc/qcom/qcom_aoss.h
new file mode 100644
index 000000000000..05fc0ed3a10d
--- /dev/null
+++ b/include/linux/soc/qcom/qcom_aoss.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_AOSS_H__
+#define __QCOM_AOSS_H__
+
+#include <linux/err.h>
+#include <linux/of.h>
+
+struct qmp;
+
+#if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len);
+struct qmp_device *qmp_get(struct device_node *np);
+
+#else
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
+{
+	return -ENODEV;
+}
+
+struct qmp *qmp_get(struct device_node *np)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+#endif
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Chris Lew]

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

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

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 8f052db1880a..2fd755d2a92d 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>
@@ -85,6 +86,8 @@ struct qmp {
 	struct clk_hw qdss_clk;
 	struct genpd_onecell_data pd_data;
 	struct qmp_cooling_device *cooling_devs;
+
+	struct dentry *debugfs_fp;
 };
 
 struct qmp_pd {
@@ -541,6 +544,34 @@ struct qmp_device *qmp_get(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(qmp_get);
 
+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 len;
+
+	ret  = copy_from_user(buf, userstr, len);
+	if (ret) {
+		dev_err(qmp->dev, "copy from user failed, ret:%d\n", ret);
+		return len;
+	}
+
+	ret = qmp_send(qmp, buf, QMP_MSG_LEN);
+	if (ret)
+		dev_err(qmp->dev, "debug send failed, ret:%d\n", ret);
+
+	return len;
+}
+
+static const struct file_operations aoss_dbg_fops = {
+	.open = simple_open,
+	.write = aoss_dbg_write,
+};
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -595,6 +626,9 @@ static int qmp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, qmp);
 
+	qmp->debugfs_fp = debugfs_create_file("aoss_send_message", 0220, NULL,
+					      qmp, &aoss_dbg_fops);
+
 	return 0;
 
 err_remove_qdss_clk:
@@ -611,6 +645,8 @@ static int qmp_remove(struct platform_device *pdev)
 {
 	struct qmp *qmp = platform_get_drvdata(pdev);
 
+	debugfs_remove(qmp->debugfs_fp);
+
 	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 1/2] soc: qcom: aoss: Expose send for generic usecase

[Sibi Sankar]

Hey Chris,
Thanks for the patch.

On 2020-11-03 08:49, Chris Lew wrote:
> 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>
> ---
>  drivers/soc/qcom/qcom_aoss.c       | 28 +++++++++++++++++++++++++++-
>  include/linux/soc/qcom/qcom_aoss.h | 33 
> +++++++++++++++++++++++++++++++++
>  2 files changed, 60 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 ed2c687c16b3..8f052db1880a 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
> @@ -222,12 +224,15 @@ 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;
>  	size_t tlen;
>  	int ret;
> 
> +	if (!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_GPL(qmp_send);
> 
>  static int qmp_qdss_clk_prepare(struct clk_hw *hw)
>  {
> @@ -515,6 +521,26 @@ 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 device tree node
> + * @np: of node of qmp device
> + *
> + * Return: handle to qmp device on success, ERR_PTR() on failure
> + */
> +struct qmp_device *qmp_get(struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +	struct qmp *qmp;

Can we use this patch series to determine
the binding the client are expected to use
to point to the qmp phandle and have it
parsed here? This would mean that qmp_get
would take in device as input instead.
Bjorn suggested that clients use "qcom,qmp"
during an offline discussion. Let me know
what you think.

> +
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev)
> +		return ERR_PTR(-EINVAL);
> +
> +	qmp = platform_get_drvdata(pdev);
> +	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL_GPL(qmp_get);
> +
>  static int qmp_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> diff --git a/include/linux/soc/qcom/qcom_aoss.h
> b/include/linux/soc/qcom/qcom_aoss.h
> new file mode 100644
> index 000000000000..05fc0ed3a10d
> --- /dev/null
> +++ b/include/linux/soc/qcom/qcom_aoss.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __QCOM_AOSS_H__
> +#define __QCOM_AOSS_H__
> +
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +struct qmp;
> +
> +#if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
> +
> +int qmp_send(struct qmp *qmp, const void *data, size_t len);
> +struct qmp_device *qmp_get(struct device_node *np);
> +
> +#else
> +
> +int qmp_send(struct qmp *qmp, const void *data, size_t len)
> +{
> +	return -ENODEV;
> +}
> +
> +struct qmp *qmp_get(struct device_node *np)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +#endif
> +
> +#endif

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

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

[Chris Lew]

On 11/11/2020 10:33 AM, Sibi Sankar wrote:
> Hey Chris,
> Thanks for the patch.
> 
> On 2020-11-03 08:49, Chris Lew wrote:
>> 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>
>> ---
>>  drivers/soc/qcom/qcom_aoss.c       | 28 +++++++++++++++++++++++++++-
>>  include/linux/soc/qcom/qcom_aoss.h | 33 
>> +++++++++++++++++++++++++++++++++
>>  2 files changed, 60 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 ed2c687c16b3..8f052db1880a 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
>> @@ -222,12 +224,15 @@ 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;
>>      size_t tlen;
>>      int ret;
>>
>> +    if (!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_GPL(qmp_send);
>>
>>  static int qmp_qdss_clk_prepare(struct clk_hw *hw)
>>  {
>> @@ -515,6 +521,26 @@ 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 device tree node
>> + * @np: of node of qmp device
>> + *
>> + * Return: handle to qmp device on success, ERR_PTR() on failure
>> + */
>> +struct qmp_device *qmp_get(struct device_node *np)
>> +{
>> +    struct platform_device *pdev;
>> +    struct qmp *qmp;
> 
> Can we use this patch series to determine
> the binding the client are expected to use
> to point to the qmp phandle and have it
> parsed here? This would mean that qmp_get
> would take in device as input instead.
> Bjorn suggested that clients use "qcom,qmp"
> during an offline discussion. Let me know
> what you think.
> 

Hey Sibi,

Yea I think that's a better implementation. I'll upload a second 
revision using "qcom,qmp" as the binding.

>> +
>> +    pdev = of_find_device_by_node(np);
>> +    if (!pdev)
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    qmp = platform_get_drvdata(pdev);
>> +    return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
>> +}
>> +EXPORT_SYMBOL_GPL(qmp_get);
>> +
>>  static int qmp_probe(struct platform_device *pdev)
>>  {
>>      struct resource *res;
>> diff --git a/include/linux/soc/qcom/qcom_aoss.h
>> b/include/linux/soc/qcom/qcom_aoss.h
>> new file mode 100644
>> index 000000000000..05fc0ed3a10d
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/qcom_aoss.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef __QCOM_AOSS_H__
>> +#define __QCOM_AOSS_H__
>> +
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +
>> +struct qmp;
>> +
>> +#if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
>> +
>> +int qmp_send(struct qmp *qmp, const void *data, size_t len);
>> +struct qmp_device *qmp_get(struct device_node *np);
>> +
>> +#else
>> +
>> +int qmp_send(struct qmp *qmp, const void *data, size_t len)
>> +{
>> +    return -ENODEV;
>> +}
>> +
>> +struct qmp *qmp_get(struct device_node *np)
>> +{
>> +    return ERR_PTR(-ENODEV);
>> +}
>> +
>> +#endif
>> +
>> +#endif
> 

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

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

[Bjorn Andersson]

On Mon 02 Nov 21:19 CST 2020, Chris Lew wrote:

> It can be useful to control the different power states of various
> parts of hardware for device testing. Add a debugfs node to send
> messages through qmp to aoss for debugging and testing purposes.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
>  drivers/soc/qcom/qcom_aoss.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 8f052db1880a..2fd755d2a92d 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>
> @@ -85,6 +86,8 @@ struct qmp {
>  	struct clk_hw qdss_clk;
>  	struct genpd_onecell_data pd_data;
>  	struct qmp_cooling_device *cooling_devs;
> +
> +	struct dentry *debugfs_fp;
>  };
>  
>  struct qmp_pd {
> @@ -541,6 +544,34 @@ struct qmp_device *qmp_get(struct device_node *np)
>  }
>  EXPORT_SYMBOL_GPL(qmp_get);
>  
> +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 len;
> +
> +	ret  = copy_from_user(buf, userstr, len);
> +	if (ret) {
> +		dev_err(qmp->dev, "copy from user failed, ret:%d\n", ret);
> +		return len;
> +	}
> +
> +	ret = qmp_send(qmp, buf, QMP_MSG_LEN);
> +	if (ret)
> +		dev_err(qmp->dev, "debug send failed, ret:%d\n", ret);

You should propagate this error to the caller, i.e. 

	return ret ? ret : len;

And with that the error print doesn't really add any value, so please
drop it.

> +
> +	return len;
> +}
> +

This will result in a compile warning when compiled without
CONFIG_DEBUG_FS, so either mark it __maybe_unused or wrap the whole hunk
in a #if IS_ENABLED(CONFIG_DEBUG_FS).


PS. Feel free to resubmit this change on its own, as it can be merged
independently from patch 1.

Regards,
Bjorn

> +static const struct file_operations aoss_dbg_fops = {
> +	.open = simple_open,
> +	.write = aoss_dbg_write,
> +};
> +
>  static int qmp_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -595,6 +626,9 @@ static int qmp_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, qmp);
>  
> +	qmp->debugfs_fp = debugfs_create_file("aoss_send_message", 0220, NULL,
> +					      qmp, &aoss_dbg_fops);
> +
>  	return 0;
>  
>  err_remove_qdss_clk:
> @@ -611,6 +645,8 @@ static int qmp_remove(struct platform_device *pdev)
>  {
>  	struct qmp *qmp = platform_get_drvdata(pdev);
>  
> +	debugfs_remove(qmp->debugfs_fp);
> +
>  	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
>