[PATCH v2] remoteproc: qcom: Venus firmware loader support for msm8996

[PATCH v2] remoteproc: qcom: Venus firmware loader support for msm8996

[Avaneesh Kumar Dwivedi]

This Patch is based on 
        https://patchwork.kernel.org/patch/9415627/
        https://patchwork.kernel.org/patch/9415651/

This patch add necessary routine and data structure to support standalone venus 
firmware load on msm8996. Below are brief of changes. 

1- Add private data structure which provide string-name and rate of clock
   on msm8996 platform for venus.
2- Provide clock initialization and enable/disable functionality.

below is console log on msm8996 platform with above change, this is standalone test log 
without video driver enablement.

[    2.612011]  remoteproc0: soc:vidc_tzpil@0 is available
[    2.616939]  remoteproc0: Note: remoteproc is still under development and considered experimental.
[    2.621963]  remoteproc0: THE BINARY FORMAT IS NOT YET FINALIZED, and backward compatibility isn't yet guaranteed.
[    2.631037] qcom-tz-pil soc:vidc_tzpil@0: Venus rproc probe done
[    2.641463]  remoteproc0: powering up soc:vidc_tzpil@0
[    2.641468]  remoteproc0: Booting fw image venus.mdt, size 6812
[    2.698127]  remoteproc0: remote processor soc:vidc_tzpil@0 is now up

Changes w.r.t. patchset 1
	reorganization and cleanup of added code.
Avaneesh Kumar Dwivedi (1):
  remoteproc: qcom: Add venus rproc support on msm8996 platform.

 .../devicetree/bindings/remoteproc/qcom,venus.txt  |   3 +-
 drivers/remoteproc/qcom_venus_pil.c                | 115 ++++++++++++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Avaneesh Kumar Dwivedi]

This patch is based on
	https://patchwork.kernel.org/patch/9415627/
	https://patchwork.kernel.org/patch/9415651/

This patch add clock initialization, enable and disable support.
Required resource name string and rating are differentiated based
on compatible string. Also added documentation for venus pil on
msm8996.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,venus.txt  |   3 +-
 drivers/remoteproc/qcom_venus_pil.c                | 115 ++++++++++++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
index 2d73ba1..c986f52 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
@@ -6,7 +6,8 @@ on the Qualcomm Venus remote processor core.
 - compatible:
 	Usage: required
 	Value type: <string>
-	Definition: must contain "qcom,venus-pil"
+	Definition: must contain "qcom,venus-pil" or
+				"qcom,venus-8996-pil"
 
 - memory-region:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_venus_pil.c b/drivers/remoteproc/qcom_venus_pil.c
index 6d4e55b..23b7e99 100644
--- a/drivers/remoteproc/qcom_venus_pil.c
+++ b/drivers/remoteproc/qcom_venus_pil.c
@@ -19,8 +19,10 @@
 #include <linux/module.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
+#include <linux/clk.h>
 #include <linux/qcom_scm.h>
 #include <linux/remoteproc.h>
+#include <linux/of_device.h>
 
 #include "qcom_mdt_loader.h"
 #include "remoteproc_internal.h"
@@ -30,6 +32,11 @@
 #define VENUS_PAS_ID			9
 #define VENUS_FW_MEM_SIZE		SZ_8M
 
+struct venus_rproc_res {
+	char **venus_clks;
+	int venus_clk_rate[4];
+};
+
 struct qcom_venus {
 	struct device *dev;
 	struct rproc *rproc;
@@ -37,6 +44,8 @@ struct qcom_venus {
 	phys_addr_t mem_phys;
 	void *mem_va;
 	size_t mem_size;
+	struct clk *venus_clks[4];
+	int clk_count;
 };
 
 static int venus_load(struct rproc *rproc, const struct firmware *fw)
@@ -78,11 +87,49 @@ static int venus_load(struct rproc *rproc, const struct firmware *fw)
 	.load = venus_load,
 };
 
+static int qcom_venus_clk_enable(struct device *dev, struct clk **clks,
+								int clk_count)
+{
+	int rc = 0;
+	int i;
+
+	for (i = 0; i < clk_count; i++) {
+		rc = clk_prepare_enable(clks[i]);
+		if (rc) {
+			dev_err(dev, "Clock enable failed\n");
+			goto err;
+		}
+	}
+
+	return 0;
+err:
+	for (i--; i >= 0; i--)
+		clk_disable_unprepare(clks[i]);
+
+	return rc;
+}
+
+static void qcom_venus_clk_disable(struct qcom_venus *venus)
+{
+	int i;
+	struct clk **clks = venus->venus_clks;
+
+	for (i = 0; i < venus->clk_count; i++)
+		clk_disable_unprepare(clks[i]);
+}
+
 static int venus_start(struct rproc *rproc)
 {
 	struct qcom_venus *venus = rproc->priv;
 	int ret;
 
+	ret = qcom_venus_clk_enable(venus->dev, venus->venus_clks,
+			venus->clk_count);
+	if (ret) {
+		dev_err(venus->dev, "failed to enable venus_clk\n");
+		return ret;
+	}
+
 	ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
 	if (ret)
 		dev_err(venus->dev,
@@ -101,6 +148,8 @@ static int venus_stop(struct rproc *rproc)
 	if (ret)
 		dev_err(venus->dev, "failed to shutdown: %d\n", ret);
 
+	qcom_venus_clk_disable(venus);
+
 	return ret;
 }
 
@@ -123,13 +172,58 @@ static void *venus_da_to_va(struct rproc *rproc, u64 da, int len)
 	.da_to_va = venus_da_to_va,
 };
 
+static int qcom_venus_init_clocks(struct device *dev, struct clk **clks,
+		char **clk_str, const int *rate)
+{
+	int clk_count = 0, i;
+
+	if (!clk_str)
+		return 0;
+
+	while (clk_str[clk_count] != NULL)
+		clk_count++;
+
+	if (!clk_count)
+		return clk_count;
+
+	if (!clks)
+		return -ENOMEM;
+
+	for (i = 0; i < clk_count; i++) {
+		const char *clock_name;
+
+		clock_name = clk_str[i];
+		clks[i] = devm_clk_get(dev, clock_name);
+		if (IS_ERR(clks[i])) {
+
+			int rc = PTR_ERR(clks[i]);
+
+			if (rc != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get %s clock\n",
+					clock_name);
+			return rc;
+		}
+		clk_set_rate(clks[i], clk_round_rate(clks[i], rate[i]));
+
+	}
+
+	return clk_count;
+}
+
+
+
 static int venus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct qcom_venus *venus;
 	struct rproc *rproc;
+	const struct venus_rproc_res *desc;
 	int ret;
 
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
 	if (!qcom_scm_is_available())
 		return -EPROBE_DEFER;
 
@@ -158,6 +252,14 @@ static int venus_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, venus);
 
+	ret = qcom_venus_init_clocks(&pdev->dev, venus->venus_clks,
+			desc->venus_clks, desc->venus_clk_rate);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to setup venus clocks.\n");
+		return ret;
+	}
+	venus->clk_count = ret;
+
 	venus->mem_va = dma_alloc_coherent(dev, venus->mem_size,
 					   &venus->mem_phys, GFP_KERNEL);
 	if (!venus->mem_va) {
@@ -194,8 +296,19 @@ static int venus_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct venus_rproc_res venus_8996_res = {
+	.venus_clks = (char*[]){"mx", "cx", "pll", NULL},
+	.venus_clk_rate = {19200000, 19200000, 19200000, 80000000},
+};
+
+static const struct venus_rproc_res venus_8916_res = {
+	.venus_clks = NULL,
+	.venus_clk_rate = {0},
+};
+
 static const struct of_device_id venus_of_match[] = {
-	{ .compatible = "qcom,venus-pil" },
+	{ .compatible = "qcom,venus-8996-pil", .data = &venus_8996_res },
+	{ .compatible = "qcom,venus-pil", .data = &venus_8916_res},
 	{ },
 };
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Stephen Boyd]

On 11/25, Avaneesh Kumar Dwivedi wrote:
> This patch is based on
> 	https://patchwork.kernel.org/patch/9415627/
> 	https://patchwork.kernel.org/patch/9415651/
> 
> This patch add clock initialization, enable and disable support.
> Required resource name string and rating are differentiated based
> on compatible string. Also added documentation for venus pil on
> msm8996.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---

Why isn't Stan Cced on this patch?

>  .../devicetree/bindings/remoteproc/qcom,venus.txt  |   3 +-
>  drivers/remoteproc/qcom_venus_pil.c                | 115 ++++++++++++++++++++-
>  2 files changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> index 2d73ba1..c986f52 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> @@ -6,7 +6,8 @@ on the Qualcomm Venus remote processor core.
>  - compatible:
>  	Usage: required
>  	Value type: <string>
> -	Definition: must contain "qcom,venus-pil"
> +	Definition: must contain "qcom,venus-pil" or
> +				"qcom,venus-8996-pil"
>  
>  - memory-region:
>  	Usage: required

No addition of clocks or clock-names properties in this document?

> diff --git a/drivers/remoteproc/qcom_venus_pil.c b/drivers/remoteproc/qcom_venus_pil.c
> index 6d4e55b..23b7e99 100644
> --- a/drivers/remoteproc/qcom_venus_pil.c
> +++ b/drivers/remoteproc/qcom_venus_pil.c
> @@ -194,8 +296,19 @@ static int venus_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct venus_rproc_res venus_8996_res = {
> +	.venus_clks = (char*[]){"mx", "cx", "pll", NULL},
> +	.venus_clk_rate = {19200000, 19200000, 19200000, 80000000},

I'm very much lost. I don't really understand why we're adding
clock control here. Perhaps that is the responsibility of the
video driver itself and isn't supposed to be in the remoteproc
portion? Testing things standalone without the video driver seems
like a unit test, which is not too useful of a test if it doesn't
mirror real use cases.

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

Re: [PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Dwivedi, Avaneesh Kumar (avani)]

On 11/29/2016 7:06 AM, Stephen Boyd wrote:
> On 11/25, Avaneesh Kumar Dwivedi wrote:
>> This patch is based on
>> 	https://patchwork.kernel.org/patch/9415627/
>> 	https://patchwork.kernel.org/patch/9415651/
>>
>> This patch add clock initialization, enable and disable support.
>> Required resource name string and rating are differentiated based
>> on compatible string. Also added documentation for venus pil on
>> msm8996.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
> Why isn't Stan Cced on this patch?
my bad will resend patch with Stan in CC
>
>>   .../devicetree/bindings/remoteproc/qcom,venus.txt  |   3 +-
>>   drivers/remoteproc/qcom_venus_pil.c                | 115 ++++++++++++++++++++-
>>   2 files changed, 116 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> index 2d73ba1..c986f52 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> @@ -6,7 +6,8 @@ on the Qualcomm Venus remote processor core.
>>   - compatible:
>>   	Usage: required
>>   	Value type: <string>
>> -	Definition: must contain "qcom,venus-pil"
>> +	Definition: must contain "qcom,venus-pil" or
>> +				"qcom,venus-8996-pil"
>>   
>>   - memory-region:
>>   	Usage: required
> No addition of clocks or clock-names properties in this document?
Ok, Yes will do it.
>
>> diff --git a/drivers/remoteproc/qcom_venus_pil.c b/drivers/remoteproc/qcom_venus_pil.c
>> index 6d4e55b..23b7e99 100644
>> --- a/drivers/remoteproc/qcom_venus_pil.c
>> +++ b/drivers/remoteproc/qcom_venus_pil.c
>> @@ -194,8 +296,19 @@ static int venus_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct venus_rproc_res venus_8996_res = {
>> +	.venus_clks = (char*[]){"mx", "cx", "pll", NULL},
>> +	.venus_clk_rate = {19200000, 19200000, 19200000, 80000000},
> I'm very much lost. I don't really understand why we're adding
> clock control here. Perhaps that is the responsibility of the
> video driver itself and isn't supposed to be in the remoteproc
> portion? Testing things standalone without the video driver seems
> like a unit test, which is not too useful of a test if it doesn't
> mirror real use cases.
There was a discussion with Bjorn<bjorn.andersson@linaro.org>, where he 
mention
"It only works if some other driver (the venus v4l driver) has enabled
    a set of clocks and the gdsc. I do not like the concept of having a
    driver that depends on the state of another driver to success..."

That is why this patch to handle clock resources within venus loader. As 
far as gdsc is concerned, change to enable gdsc lies only in DT Node 
where one need to define power-domain property. let me know if i have 
misunderstood Bjorn, then i can drop this patch.
>

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.