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

[RESEND 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  |  26 ++++-
 drivers/remoteproc/qcom_venus_pil.c                | 116 ++++++++++++++++++++-
 2 files changed, 140 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.

[RESEND 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  |  26 ++++-
 drivers/remoteproc/qcom_venus_pil.c                | 116 ++++++++++++++++++++-
 2 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
index 2d73ba1..417026b 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
@@ -6,13 +6,30 @@ 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-msm8996-pil"
 
 - memory-region:
 	Usage: required
 	Value type: <phandle>
 	Definition: a phandle to a node describing reserved memory
 
+- clocks:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: reference to the core, iface and bus and maxi clocks to be held on
+		    behalf of the booting of the venus core
+
+- clock-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: should be "core_clk", "iface_clk", "bus_clk", "maxi_clk"
+
+- power-domains:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: reference to the venus gdsc to be turned on before booting venus core
+
 * An example
 	reserved-memory {
 		#address-cells = <2>;
@@ -29,5 +46,12 @@ on the Qualcomm Venus remote processor core.
 
 	rproc_venus@0 {
 		compatible = "qcom,venus-pil";
+		clocks = <&mmcc VIDEO_CORE_CLK>,
+				<&mmcc VIDEO_AHB_CLK>,
+				<&mmcc VIDEO_AXI_CLK>,
+				<&mmcc VIDEO_MAXI_CLK>;
+
+		clock-names = "core_clk", "iface_clk", "bus_clk", "maxi_clk";
+		power-domains = <&mmcc VENUS_GDSC>;
 		memory-region = <&venus_mem>;
 	};
diff --git a/drivers/remoteproc/qcom_venus_pil.c b/drivers/remoteproc/qcom_venus_pil.c
index 6d4e55b..f91f873 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,20 @@ static int venus_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct venus_rproc_res venus_msm8996_res = {
+	.venus_clks = (char*[]){"core_clk", "iface_clk", "bus_clk",
+		"maxi_clk", 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-msm8996-pil", .data = &venus_msm8996_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: [RESEND PATCH v2] remoteproc: qcom: Venus firmware loader support for msm8996

[Stephen Boyd]

On 11/29, Avaneesh Kumar Dwivedi wrote:
> 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.

For one patch we typically don't send cover letters. These sorts
of details just go below the dashed lines after the commit text
of the single patch.

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

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

[Stephen Boyd]

On 11/29, 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>
> ---
>  .../devicetree/bindings/remoteproc/qcom,venus.txt  |  26 ++++-
>  drivers/remoteproc/qcom_venus_pil.c                | 116 ++++++++++++++++++++-
>  2 files changed, 140 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> index 2d73ba1..417026b 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
> @@ -6,13 +6,30 @@ 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-msm8996-pil"
>  
>  - memory-region:
>  	Usage: required
>  	Value type: <phandle>
>  	Definition: a phandle to a node describing reserved memory
>  
> +- clocks:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: reference to the core, iface and bus and maxi clocks to be held on
> +		    behalf of the booting of the venus core
> +
> +- clock-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: should be "core_clk", "iface_clk", "bus_clk", "maxi_clk"

Please drop _clk from all clock names.

> +
> +- power-domains:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: reference to the venus gdsc to be turned on before booting venus core

All these new properties can't be required if the original
compatible is used, right?

> +
>  * An example
>  	reserved-memory {
>  		#address-cells = <2>;
> @@ -29,5 +46,12 @@ on the Qualcomm Venus remote processor core.
>  
>  	rproc_venus@0 {
>  		compatible = "qcom,venus-pil";
> +		clocks = <&mmcc VIDEO_CORE_CLK>,
> +				<&mmcc VIDEO_AHB_CLK>,
> +				<&mmcc VIDEO_AXI_CLK>,
> +				<&mmcc VIDEO_MAXI_CLK>;
> +
> +		clock-names = "core_clk", "iface_clk", "bus_clk", "maxi_clk";
> +		power-domains = <&mmcc VENUS_GDSC>;
>  		memory-region = <&venus_mem>;
>  	};

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

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

[Dwivedi, Avaneesh Kumar (avani)]

On 11/30/2016 12:54 AM, Stephen Boyd wrote:
> On 11/29, Avaneesh Kumar Dwivedi wrote:
>> 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.
> For one patch we typically don't send cover letters. These sorts
> of details just go below the dashed lines after the commit text
> of the single patch.
OK

-- 
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: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Dwivedi, Avaneesh Kumar (avani)]

On 11/30/2016 12:57 AM, Stephen Boyd wrote:
> On 11/29, 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>
>> ---
>>   .../devicetree/bindings/remoteproc/qcom,venus.txt  |  26 ++++-
>>   drivers/remoteproc/qcom_venus_pil.c                | 116 ++++++++++++++++++++-
>>   2 files changed, 140 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> index 2d73ba1..417026b 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>> @@ -6,13 +6,30 @@ 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-msm8996-pil"
>>   
>>   - memory-region:
>>   	Usage: required
>>   	Value type: <phandle>
>>   	Definition: a phandle to a node describing reserved memory
>>   
>> +- clocks:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: reference to the core, iface and bus and maxi clocks to be held on
>> +		    behalf of the booting of the venus core
>> +
>> +- clock-names:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: should be "core_clk", "iface_clk", "bus_clk", "maxi_clk"
> Please drop _clk from all clock names.
OK
>
>> +
>> +- power-domains:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: reference to the venus gdsc to be turned on before booting venus core
> All these new properties can't be required if the original
> compatible is used, right?
gdsc were earlier handled via regulator framework, but now gdsc's are 
being handled via power domain framework.
so any driver which require gdsc to be enabled need to use this property 
on mainline kernel.
so these properties will even be applicable for 8916 venus rproc on 
mainline kernel.

>
>> +
>>   * An example
>>   	reserved-memory {
>>   		#address-cells = <2>;
>> @@ -29,5 +46,12 @@ on the Qualcomm Venus remote processor core.
>>   
>>   	rproc_venus@0 {
>>   		compatible = "qcom,venus-pil";
>> +		clocks = <&mmcc VIDEO_CORE_CLK>,
>> +				<&mmcc VIDEO_AHB_CLK>,
>> +				<&mmcc VIDEO_AXI_CLK>,
>> +				<&mmcc VIDEO_MAXI_CLK>;
>> +
>> +		clock-names = "core_clk", "iface_clk", "bus_clk", "maxi_clk";
>> +		power-domains = <&mmcc VENUS_GDSC>;
>>   		memory-region = <&venus_mem>;
>>   	};

-- 
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: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Stanimir Varbanov]

Hi,

On 11/30/2016 07:24 AM, Dwivedi, Avaneesh Kumar (avani) wrote:
> 
> 
> On 11/30/2016 12:57 AM, Stephen Boyd wrote:
>> On 11/29, 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>
>>> ---
>>>   .../devicetree/bindings/remoteproc/qcom,venus.txt  |  26 ++++-
>>>   drivers/remoteproc/qcom_venus_pil.c                | 116
>>> ++++++++++++++++++++-
>>>   2 files changed, 140 insertions(+), 2 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>> b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>> index 2d73ba1..417026b 100644
>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>> @@ -6,13 +6,30 @@ 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-msm8996-pil"
>>>     - memory-region:
>>>       Usage: required
>>>       Value type: <phandle>
>>>       Definition: a phandle to a node describing reserved memory
>>>   +- clocks:
>>> +    Usage: required
>>> +    Value type: <prop-encoded-array>
>>> +    Definition: reference to the core, iface and bus and maxi clocks
>>> to be held on
>>> +            behalf of the booting of the venus core
>>> +
>>> +- clock-names:
>>> +    Usage: required
>>> +    Value type: <stringlist>
>>> +    Definition: should be "core_clk", "iface_clk", "bus_clk",
>>> "maxi_clk"
>> Please drop _clk from all clock names.
> OK
>>
>>> +
>>> +- power-domains:
>>> +    Usage: required
>>> +    Value type: <prop-encoded-array>
>>> +    Definition: reference to the venus gdsc to be turned on before
>>> booting venus core
>> All these new properties can't be required if the original
>> compatible is used, right?
> gdsc were earlier handled via regulator framework, but now gdsc's are
> being handled via power domain framework.
> so any driver which require gdsc to be enabled need to use this property
> on mainline kernel.
> so these properties will even be applicable for 8916 venus rproc on
> mainline kernel.

Then I'd propose to add compatible string for msm8916 too. We will need
to distinguish between SoCs in order to select proper Venus firmware
file too.

I think using power domain in remoteproc driver will break power
management of the v4l2 venus driver. Presently I use runtime pm to
switch ON/OFF GDSC and enable/disable Venus clocks.

When the .probe of venus remoteproc driver is called the power domain
will become ON and never switched OFF (except when platform driver
.remove method is called). One possible solution would be to add runtime
pm in venus remoreproc driver.

IMO we should think more about that.

Bjorn, Stephen what are your opinions?


-- 
regards,
Stan

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

[Dwivedi, Avaneesh Kumar (avani)]

On 11/30/2016 2:52 PM, Stanimir Varbanov wrote:
> Hi,
>
> On 11/30/2016 07:24 AM, Dwivedi, Avaneesh Kumar (avani) wrote:
>>
>> On 11/30/2016 12:57 AM, Stephen Boyd wrote:
>>> On 11/29, 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>
>>>> ---
>>>>    .../devicetree/bindings/remoteproc/qcom,venus.txt  |  26 ++++-
>>>>    drivers/remoteproc/qcom_venus_pil.c                | 116
>>>> ++++++++++++++++++++-
>>>>    2 files changed, 140 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>> b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>> index 2d73ba1..417026b 100644
>>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>> @@ -6,13 +6,30 @@ 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-msm8996-pil"
>>>>      - memory-region:
>>>>        Usage: required
>>>>        Value type: <phandle>
>>>>        Definition: a phandle to a node describing reserved memory
>>>>    +- clocks:
>>>> +    Usage: required
>>>> +    Value type: <prop-encoded-array>
>>>> +    Definition: reference to the core, iface and bus and maxi clocks
>>>> to be held on
>>>> +            behalf of the booting of the venus core
>>>> +
>>>> +- clock-names:
>>>> +    Usage: required
>>>> +    Value type: <stringlist>
>>>> +    Definition: should be "core_clk", "iface_clk", "bus_clk",
>>>> "maxi_clk"
>>> Please drop _clk from all clock names.
>> OK
>>>> +
>>>> +- power-domains:
>>>> +    Usage: required
>>>> +    Value type: <prop-encoded-array>
>>>> +    Definition: reference to the venus gdsc to be turned on before
>>>> booting venus core
>>> All these new properties can't be required if the original
>>> compatible is used, right?
>> gdsc were earlier handled via regulator framework, but now gdsc's are
>> being handled via power domain framework.
>> so any driver which require gdsc to be enabled need to use this property
>> on mainline kernel.
>> so these properties will even be applicable for 8916 venus rproc on
>> mainline kernel.
> Then I'd propose to add compatible string for msm8916 too. We will need
> to distinguish between SoCs in order to select proper Venus firmware
> file too.
>
> I think using power domain in remoteproc driver will break power
> management of the v4l2 venus driver. Presently I use runtime pm to
> switch ON/OFF GDSC and enable/disable Venus clocks.
>
> When the .probe of venus remoteproc driver is called the power domain
> will become ON and never switched OFF (except when platform driver
> .remove method is called). One possible solution would be to add runtime
> pm in venus remoreproc driver.
>
> IMO we should think more about that.
>
> Bjorn, Stephen what are your opinions?
>
Shall I go ahead and add runtime PM API calls during shutdown/start call 
along with other changes to address other comments? i was waiting any 
comment from Sboyed and Bjorn.

-- 
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: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Stanimir Varbanov]

On 12/05/2016 11:46 AM, Dwivedi, Avaneesh Kumar (avani) wrote:
> 
> 
> On 11/30/2016 2:52 PM, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 11/30/2016 07:24 AM, Dwivedi, Avaneesh Kumar (avani) wrote:
>>>
>>> On 11/30/2016 12:57 AM, Stephen Boyd wrote:
>>>> On 11/29, 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>
>>>>> ---
>>>>>    .../devicetree/bindings/remoteproc/qcom,venus.txt  |  26 ++++-
>>>>>    drivers/remoteproc/qcom_venus_pil.c                | 116
>>>>> ++++++++++++++++++++-
>>>>>    2 files changed, 140 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>> b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>> index 2d73ba1..417026b 100644
>>>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt
>>>>> @@ -6,13 +6,30 @@ 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-msm8996-pil"
>>>>>      - memory-region:
>>>>>        Usage: required
>>>>>        Value type: <phandle>
>>>>>        Definition: a phandle to a node describing reserved memory
>>>>>    +- clocks:
>>>>> +    Usage: required
>>>>> +    Value type: <prop-encoded-array>
>>>>> +    Definition: reference to the core, iface and bus and maxi clocks
>>>>> to be held on
>>>>> +            behalf of the booting of the venus core
>>>>> +
>>>>> +- clock-names:
>>>>> +    Usage: required
>>>>> +    Value type: <stringlist>
>>>>> +    Definition: should be "core_clk", "iface_clk", "bus_clk",
>>>>> "maxi_clk"
>>>> Please drop _clk from all clock names.
>>> OK
>>>>> +
>>>>> +- power-domains:
>>>>> +    Usage: required
>>>>> +    Value type: <prop-encoded-array>
>>>>> +    Definition: reference to the venus gdsc to be turned on before
>>>>> booting venus core
>>>> All these new properties can't be required if the original
>>>> compatible is used, right?
>>> gdsc were earlier handled via regulator framework, but now gdsc's are
>>> being handled via power domain framework.
>>> so any driver which require gdsc to be enabled need to use this property
>>> on mainline kernel.
>>> so these properties will even be applicable for 8916 venus rproc on
>>> mainline kernel.
>> Then I'd propose to add compatible string for msm8916 too. We will need
>> to distinguish between SoCs in order to select proper Venus firmware
>> file too.
>>
>> I think using power domain in remoteproc driver will break power
>> management of the v4l2 venus driver. Presently I use runtime pm to
>> switch ON/OFF GDSC and enable/disable Venus clocks.
>>
>> When the .probe of venus remoteproc driver is called the power domain
>> will become ON and never switched OFF (except when platform driver
>> .remove method is called). One possible solution would be to add runtime
>> pm in venus remoreproc driver.
>>
>> IMO we should think more about that.
>>
>> Bjorn, Stephen what are your opinions?
>>
> Shall I go ahead and add runtime PM API calls during shutdown/start call
> along with other changes to address other comments? i was waiting any
> comment from Sboyed and Bjorn.
> 

IMO runtime should be added in remoteproc core, so you can start with an
RFC patch to see how is going.

-- 
regards,
Stan

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

[Bjorn Andersson]

On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
[..]
> 
> Then I'd propose to add compatible string for msm8916 too. We will need
> to distinguish between SoCs in order to select proper Venus firmware
> file too.
> 

This does make sense.

> I think using power domain in remoteproc driver will break power
> management of the v4l2 venus driver. Presently I use runtime pm to
> switch ON/OFF GDSC and enable/disable Venus clocks.
> 
> When the .probe of venus remoteproc driver is called the power domain
> will become ON and never switched OFF (except when platform driver
> .remove method is called). One possible solution would be to add runtime
> pm in venus remoreproc driver.
> 
> IMO we should think more about that.
> 
> Bjorn, Stephen what are your opinions?
> 

Sorry for not getting back to you on this after our earlier discussion
on the topic.

Modelling the remoteproc and codec drivers as completely separate
entities (with only the rproc phandle) gives us issues with the timing
between the two. As I pulled down and started playing with [1] I noticed
that I was not able to get the codec driver to probe unless I compiled
it as a module which I insmoded after my /lib/firmware became available.

In addition to this, there's no way for the remoteproc driver to inform
the codec driver when it's actually shutting down and booting back up
during a crash recovery (an async operation).

And lastly when I talked to Rob about the DT binding the obvious
question of why one piece of hardware have two disjunct nodes in the DT.


So I believe we should represent the codec driver as a child of the
remoteproc driver, utilizing the newly introduced "remoteproc
subdevices" to probe and remove the codec driver based on the state of
the remoteproc driver. This relationship will also allow us to tie
certain resources (e.g. the clocks and power-domain) to the remoteproc
driver and use pm_runtime in either driver to make sure the resources
are enabled appropriately.

I did backport some patches from my v4.10 remoteproc pull request into
[2] and merged this into [1] and made a prototype of this. You can find
it here [3], I did test that I can boot the remoteproc and run
v4l2-compliance, shut it down, boot it back up and re-run
v4l2-compliance, and it seems to work fine.


With these patches you can compile the remoteproc driver builtin to the
kernel and once your system is booted you can issue:

  echo start > /sys/class/remoteproc/remoteproc0/state

and venus should boot and the codec should be probed.

  echo stop > /sys/class/remoteproc/remoteproc0/state

will shut down the core after removing the codec.


Please let me know if you think this is a viable solution.

(This is only tested on db410c so far, looks like we might need a few
more clocks for 8996 and we should probably be explicit about the noc
and its resources)

[1] https://git.linaro.org/people/stanimir.varbanov/linux.git/log/?h=release/qcomlt-4.9-venus-wip
[2] https://github.com/andersson/kernel/commits/qcomlt/v4.9-rproc-backport
[3] https://github.com/andersson/kernel/commits/qcomlt/venus-wip

Regards,
Bjorn

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

[Stephen Boyd]

On 12/12, Bjorn Andersson wrote:
> On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
> [..]
> 
> > I think using power domain in remoteproc driver will break power
> > management of the v4l2 venus driver. Presently I use runtime pm to
> > switch ON/OFF GDSC and enable/disable Venus clocks.
> > 
> > When the .probe of venus remoteproc driver is called the power domain
> > will become ON and never switched OFF (except when platform driver
> > .remove method is called). One possible solution would be to add runtime
> > pm in venus remoreproc driver.
> > 
> > IMO we should think more about that.
> > 
> > Bjorn, Stephen what are your opinions?
> > 
> 
> Sorry for not getting back to you on this after our earlier discussion
> on the topic.
> 
> Modelling the remoteproc and codec drivers as completely separate
> entities (with only the rproc phandle) gives us issues with the timing
> between the two. As I pulled down and started playing with [1] I noticed
> that I was not able to get the codec driver to probe unless I compiled
> it as a module which I insmoded after my /lib/firmware became available.
> 
> In addition to this, there's no way for the remoteproc driver to inform
> the codec driver when it's actually shutting down and booting back up
> during a crash recovery (an async operation).
> 
> And lastly when I talked to Rob about the DT binding the obvious
> question of why one piece of hardware have two disjunct nodes in the DT.
> 
> 
> So I believe we should represent the codec driver as a child of the
> remoteproc driver, utilizing the newly introduced "remoteproc
> subdevices" to probe and remove the codec driver based on the state of
> the remoteproc driver. This relationship will also allow us to tie
> certain resources (e.g. the clocks and power-domain) to the remoteproc
> driver and use pm_runtime in either driver to make sure the resources
> are enabled appropriately.
> 
> I did backport some patches from my v4.10 remoteproc pull request into
> [2] and merged this into [1] and made a prototype of this. You can find
> it here [3], I did test that I can boot the remoteproc and run
> v4l2-compliance, shut it down, boot it back up and re-run
> v4l2-compliance, and it seems to work fine.
> 

>From a DT perspective this seems backwards. We shouldn't be
putting something with a reg property underneath a "virtual" node
that doesn't have a reg property. It's already causing pain,
evident by these sorts of patches, so something seems wrong.

Are we gaining anything by having a remoteproc driver and device
for the video hardware here? Why can't the video driver
load/unload firmware with the mdt loader code by itself when it
wants to boot the processor? That seems like a much simpler
design and it nicely avoids this DT confusion.

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

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

[Bjorn Andersson]

On Fri 16 Dec 16:56 PST 2016, Stephen Boyd wrote:

> On 12/12, Bjorn Andersson wrote:
> > On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
> > [..]
> > 
> > > I think using power domain in remoteproc driver will break power
> > > management of the v4l2 venus driver. Presently I use runtime pm to
> > > switch ON/OFF GDSC and enable/disable Venus clocks.
> > > 
> > > When the .probe of venus remoteproc driver is called the power domain
> > > will become ON and never switched OFF (except when platform driver
> > > .remove method is called). One possible solution would be to add runtime
> > > pm in venus remoreproc driver.
> > > 
> > > IMO we should think more about that.
> > > 
> > > Bjorn, Stephen what are your opinions?
> > > 
> > 
> > Sorry for not getting back to you on this after our earlier discussion
> > on the topic.
> > 
> > Modelling the remoteproc and codec drivers as completely separate
> > entities (with only the rproc phandle) gives us issues with the timing
> > between the two. As I pulled down and started playing with [1] I noticed
> > that I was not able to get the codec driver to probe unless I compiled
> > it as a module which I insmoded after my /lib/firmware became available.
> > 
> > In addition to this, there's no way for the remoteproc driver to inform
> > the codec driver when it's actually shutting down and booting back up
> > during a crash recovery (an async operation).
> > 
> > And lastly when I talked to Rob about the DT binding the obvious
> > question of why one piece of hardware have two disjunct nodes in the DT.
> > 
> > 
> > So I believe we should represent the codec driver as a child of the
> > remoteproc driver, utilizing the newly introduced "remoteproc
> > subdevices" to probe and remove the codec driver based on the state of
> > the remoteproc driver. This relationship will also allow us to tie
> > certain resources (e.g. the clocks and power-domain) to the remoteproc
> > driver and use pm_runtime in either driver to make sure the resources
> > are enabled appropriately.
> > 
> > I did backport some patches from my v4.10 remoteproc pull request into
> > [2] and merged this into [1] and made a prototype of this. You can find
> > it here [3], I did test that I can boot the remoteproc and run
> > v4l2-compliance, shut it down, boot it back up and re-run
> > v4l2-compliance, and it seems to work fine.
> > 
> 
> From a DT perspective this seems backwards. We shouldn't be
> putting something with a reg property underneath a "virtual" node
> that doesn't have a reg property. It's already causing pain,
> evident by these sorts of patches, so something seems wrong.
> 

I agree.

> Are we gaining anything by having a remoteproc driver and device
> for the video hardware here?

After discussing the matter with Avaneesh I realized that the downstream
driver powers the venus core up and down based on the presence of
clients. I expect that this means we have to do the same to be able to
meet our power KPIs.

With this in mind the remoteproc driver becomes a wrapper for a mdt
loader and the scm pas interface - and brings with it a bunch of other
things, expectations and challenges.

> Why can't the video driver load/unload firmware with the mdt loader
> code by itself when it wants to boot the processor?

Providing a saner api around the mdt-loader and pas would allow venus
and Adreno to reuse the code, without the remoteproc bloat.

> That seems like a much simpler design and it nicely avoids this DT
> confusion.
> 

Agreed.

I was under the impression that the venus core was always on, but after
re-reading the downstream venus code a few more times I think this would
be much cleaner to do so, both implementation and binding wise.

Regards,
Bjorn

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

[Dwivedi, Avaneesh Kumar (avani)]

On 12/17/2016 12:41 PM, Bjorn Andersson wrote:
> On Fri 16 Dec 16:56 PST 2016, Stephen Boyd wrote:
>
>> On 12/12, Bjorn Andersson wrote:
>>> On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
>>> [..]
>>>
>>>> I think using power domain in remoteproc driver will break power
>>>> management of the v4l2 venus driver. Presently I use runtime pm to
>>>> switch ON/OFF GDSC and enable/disable Venus clocks.
>>>>
>>>> When the .probe of venus remoteproc driver is called the power domain
>>>> will become ON and never switched OFF (except when platform driver
>>>> .remove method is called). One possible solution would be to add runtime
>>>> pm in venus remoreproc driver.
>>>>
>>>> IMO we should think more about that.
>>>>
>>>> Bjorn, Stephen what are your opinions?
>>>>
>>> Sorry for not getting back to you on this after our earlier discussion
>>> on the topic.
>>>
>>> Modelling the remoteproc and codec drivers as completely separate
>>> entities (with only the rproc phandle) gives us issues with the timing
>>> between the two. As I pulled down and started playing with [1] I noticed
>>> that I was not able to get the codec driver to probe unless I compiled
>>> it as a module which I insmoded after my /lib/firmware became available.
>>>
>>> In addition to this, there's no way for the remoteproc driver to inform
>>> the codec driver when it's actually shutting down and booting back up
>>> during a crash recovery (an async operation).
>>>
>>> And lastly when I talked to Rob about the DT binding the obvious
>>> question of why one piece of hardware have two disjunct nodes in the DT.
>>>
>>>
>>> So I believe we should represent the codec driver as a child of the
>>> remoteproc driver, utilizing the newly introduced "remoteproc
>>> subdevices" to probe and remove the codec driver based on the state of
>>> the remoteproc driver. This relationship will also allow us to tie
>>> certain resources (e.g. the clocks and power-domain) to the remoteproc
>>> driver and use pm_runtime in either driver to make sure the resources
>>> are enabled appropriately.
>>>
>>> I did backport some patches from my v4.10 remoteproc pull request into
>>> [2] and merged this into [1] and made a prototype of this. You can find
>>> it here [3], I did test that I can boot the remoteproc and run
>>> v4l2-compliance, shut it down, boot it back up and re-run
>>> v4l2-compliance, and it seems to work fine.
>>>
>>  From a DT perspective this seems backwards. We shouldn't be
>> putting something with a reg property underneath a "virtual" node
>> that doesn't have a reg property. It's already causing pain,
>> evident by these sorts of patches, so something seems wrong.
>>
> I agree.
>
>> Are we gaining anything by having a remoteproc driver and device
>> for the video hardware here?
> After discussing the matter with Avaneesh I realized that the downstream
> driver powers the venus core up and down based on the presence of
> clients. I expect that this means we have to do the same to be able to
> meet our power KPIs.
>
> With this in mind the remoteproc driver becomes a wrapper for a mdt
> loader and the scm pas interface - and brings with it a bunch of other
> things, expectations and challenges.
>
>> Why can't the video driver load/unload firmware with the mdt loader
>> code by itself when it wants to boot the processor?
> Providing a saner api around the mdt-loader and pas would allow venus
> and Adreno to reuse the code, without the remoteproc bloat.
>
>> That seems like a much simpler design and it nicely avoids this DT
>> confusion.
>>
> Agreed.
>
> I was under the impression that the venus core was always on, but after
> re-reading the downstream venus code a few more times I think this would
> be much cleaner to do so, both implementation and binding wise.

Does It mean that, you are thinking to abolish venus rproc driver? and 
video driver will use mdt loader and scm interface to boot venus core as 
clients make call?
effectively avoiding any requirement for separate DT node for venus rproc?
>
> Regards,
> Bjorn

-- 
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: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Stanimir Varbanov]

Hi Bjorn,

On 12/17/2016 09:11 AM, Bjorn Andersson wrote:
> On Fri 16 Dec 16:56 PST 2016, Stephen Boyd wrote:
> 
>> On 12/12, Bjorn Andersson wrote:
>>> On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
>>> [..]
>>>
>>>> I think using power domain in remoteproc driver will break power
>>>> management of the v4l2 venus driver. Presently I use runtime pm to
>>>> switch ON/OFF GDSC and enable/disable Venus clocks.
>>>>
>>>> When the .probe of venus remoteproc driver is called the power domain
>>>> will become ON and never switched OFF (except when platform driver
>>>> .remove method is called). One possible solution would be to add runtime
>>>> pm in venus remoreproc driver.
>>>>
>>>> IMO we should think more about that.
>>>>
>>>> Bjorn, Stephen what are your opinions?
>>>>
>>>
>>> Sorry for not getting back to you on this after our earlier discussion
>>> on the topic.
>>>
>>> Modelling the remoteproc and codec drivers as completely separate
>>> entities (with only the rproc phandle) gives us issues with the timing
>>> between the two. As I pulled down and started playing with [1] I noticed
>>> that I was not able to get the codec driver to probe unless I compiled
>>> it as a module which I insmoded after my /lib/firmware became available.
>>>
>>> In addition to this, there's no way for the remoteproc driver to inform
>>> the codec driver when it's actually shutting down and booting back up
>>> during a crash recovery (an async operation).
>>>
>>> And lastly when I talked to Rob about the DT binding the obvious
>>> question of why one piece of hardware have two disjunct nodes in the DT.
>>>
>>>
>>> So I believe we should represent the codec driver as a child of the
>>> remoteproc driver, utilizing the newly introduced "remoteproc
>>> subdevices" to probe and remove the codec driver based on the state of
>>> the remoteproc driver. This relationship will also allow us to tie
>>> certain resources (e.g. the clocks and power-domain) to the remoteproc
>>> driver and use pm_runtime in either driver to make sure the resources
>>> are enabled appropriately.
>>>
>>> I did backport some patches from my v4.10 remoteproc pull request into
>>> [2] and merged this into [1] and made a prototype of this. You can find
>>> it here [3], I did test that I can boot the remoteproc and run
>>> v4l2-compliance, shut it down, boot it back up and re-run
>>> v4l2-compliance, and it seems to work fine.
>>>
>>
>> From a DT perspective this seems backwards. We shouldn't be
>> putting something with a reg property underneath a "virtual" node
>> that doesn't have a reg property. It's already causing pain,
>> evident by these sorts of patches, so something seems wrong.
>>
> 
> I agree.
> 
>> Are we gaining anything by having a remoteproc driver and device
>> for the video hardware here?
> 
> After discussing the matter with Avaneesh I realized that the downstream
> driver powers the venus core up and down based on the presence of
> clients. I expect that this means we have to do the same to be able to
> meet our power KPIs.
> 
> With this in mind the remoteproc driver becomes a wrapper for a mdt
> loader and the scm pas interface - and brings with it a bunch of other
> things, expectations and challenges.
> 
>> Why can't the video driver load/unload firmware with the mdt loader
>> code by itself when it wants to boot the processor?
> 
> Providing a saner api around the mdt-loader and pas would allow venus
> and Adreno to reuse the code, without the remoteproc bloat.

Does it make sense if we move mdt parser in drivers/firmware ?

-- 
regards,
Stan