diff mbox

[v3,2/4] remoteproc: qcom: Add additional agree2_clk and px regulator resource.

Message ID 1485788589-21968-3-git-send-email-akdwived@codeaurora.org (mailing list archive)
State Superseded
Headers show

Commit Message

Dwivedi, Avaneesh Kumar (avani) Jan. 30, 2017, 3:03 p.m. UTC
This patch add additional clock and regulator resource which are
initialized based on compatible and has no impact on existing driver
working. This resourse addition enable the existing driver to handle.
low pass sensor processor device also.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_adsp_pil.c | 43 +++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

Comments

Bjorn Andersson Jan. 30, 2017, 9:46 p.m. UTC | #1
On Mon 30 Jan 07:03 PST 2017, Avaneesh Kumar Dwivedi wrote:

> This patch add additional clock and regulator resource which are
> initialized based on compatible and has no impact on existing driver
> working. This resourse addition enable the existing driver to handle.
> low pass sensor processor device also.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>

Applied, with below modification.

> ---
>  drivers/remoteproc/qcom_adsp_pil.c | 43 +++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
[..]
>  static int adsp_init_regulator(struct qcom_adsp *adsp)
>  {
> -	adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
> +	adsp->cx_supply = devm_regulator_get(adsp->dev, "vdd_cx");

We should not change the name of devicetree properties, so I dropped
"vdd_" on both of these.

>  	if (IS_ERR(adsp->cx_supply))
>  		return PTR_ERR(adsp->cx_supply);
>  
>  	regulator_set_load(adsp->cx_supply, 100000);
>  
> +	adsp->px_supply = devm_regulator_get(adsp->dev, "vdd_px");
> +	if (IS_ERR(adsp->px_supply))
> +		return PTR_ERR(adsp->px_supply);

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dwivedi, Avaneesh Kumar (avani) Jan. 31, 2017, 5:58 a.m. UTC | #2
On 1/31/2017 3:16 AM, Bjorn Andersson wrote:
> On Mon 30 Jan 07:03 PST 2017, Avaneesh Kumar Dwivedi wrote:
>
>> This patch add additional clock and regulator resource which are
>> initialized based on compatible and has no impact on existing driver
>> working. This resourse addition enable the existing driver to handle.
>> low pass sensor processor device also.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> Applied, with below modification.
Thanks Bjorn, but please look below inline comment.
>> ---
>>   drivers/remoteproc/qcom_adsp_pil.c | 43 +++++++++++++++++++++++++++++++-------
>>   1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
> [..]
>>   static int adsp_init_regulator(struct qcom_adsp *adsp)
>>   {
>> -	adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
>> +	adsp->cx_supply = devm_regulator_get(adsp->dev, "vdd_cx");
> We should not change the name of devicetree properties, so I dropped
> "vdd_" on both of these.
I observed that giving "cx" or "px" string to devm_regulator_get() was 
returning with dummy regulator, and if i gave "vdd_cx" and "vdd_px" it 
did not print dummy regulator warning.
in device tree these regulators node were defined as "vdd_cx-supply" and 
"vdd_px-supply"
>
>>   	if (IS_ERR(adsp->cx_supply))
>>   		return PTR_ERR(adsp->cx_supply);
>>   
>>   	regulator_set_load(adsp->cx_supply, 100000);
>>   
>> +	adsp->px_supply = devm_regulator_get(adsp->dev, "vdd_px");
>> +	if (IS_ERR(adsp->px_supply))
>> +		return PTR_ERR(adsp->px_supply);
> Regards,
> Bjorn
Bjorn Andersson Jan. 31, 2017, 6:05 a.m. UTC | #3
On Mon 30 Jan 21:58 PST 2017, Dwivedi, Avaneesh Kumar (avani) wrote:

> 
> 
> On 1/31/2017 3:16 AM, Bjorn Andersson wrote:
> > On Mon 30 Jan 07:03 PST 2017, Avaneesh Kumar Dwivedi wrote:
> > 
> > > This patch add additional clock and regulator resource which are
> > > initialized based on compatible and has no impact on existing driver
> > > working. This resourse addition enable the existing driver to handle.
> > > low pass sensor processor device also.
> > > 
> > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> > Applied, with below modification.
> Thanks Bjorn, but please look below inline comment.
> > > ---
> > >   drivers/remoteproc/qcom_adsp_pil.c | 43 +++++++++++++++++++++++++++++++-------
> > >   1 file changed, 36 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
> > [..]
> > >   static int adsp_init_regulator(struct qcom_adsp *adsp)
> > >   {
> > > -	adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
> > > +	adsp->cx_supply = devm_regulator_get(adsp->dev, "vdd_cx");
> > We should not change the name of devicetree properties, so I dropped
> > "vdd_" on both of these.
> I observed that giving "cx" or "px" string to devm_regulator_get() was
> returning with dummy regulator, and if i gave "vdd_cx" and "vdd_px" it did
> not print dummy regulator warning.
> in device tree these regulators node were defined as "vdd_cx-supply" and
> "vdd_px-supply"

They are named "vdd_cx" and "vdd_px" in the downstream dts, I didn't
notice this originally and as we have a few other discrepancies to the
downstream binding I rather stay compatible with the existing upstream
DT binding than the downstream.

So please update your dts.


Btw, forgot to mention that aggre2 definitely is a "bus" and I think it
should be represented separately, but I figured its better to merge the
driver as is and then remove aggre2 once we have figured out how to
represent/reference it properly.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
index f674301..58cc46d 100644
--- a/drivers/remoteproc/qcom_adsp_pil.c
+++ b/drivers/remoteproc/qcom_adsp_pil.c
@@ -36,6 +36,7 @@  struct adsp_data {
 	int crash_reason_smem;
 	const char *firmware_name;
 	int pas_id;
+	bool has_aggre2_clk;
 };
 
 
@@ -53,11 +54,13 @@  struct qcom_adsp {
 	unsigned stop_bit;
 
 	struct clk *xo;
-
+	struct clk *aggre2_clk;
 	struct regulator *cx_supply;
+	struct regulator *px_supply;
 
 	int pas_id;
 	int crash_reason_smem;
+	bool has_aggre2_clk;
 
 	struct completion start_done;
 	struct completion stop_done;
@@ -115,16 +118,22 @@  static int adsp_start(struct rproc *rproc)
 	ret = clk_prepare_enable(adsp->xo);
 	if (ret)
 		return ret;
+	ret = clk_prepare_enable(adsp->aggre2_clk);
+	if (ret)
+		goto disable_xo_clk;
 
 	ret = regulator_enable(adsp->cx_supply);
 	if (ret)
-		goto disable_clocks;
+		goto disable_aggre2_clk;
+	ret = regulator_enable(adsp->px_supply);
+	if (ret)
+		goto disable_cx_supply;
 
 	ret = qcom_scm_pas_auth_and_reset(adsp->pas_id);
 	if (ret) {
 		dev_err(adsp->dev,
 			"failed to authenticate image and release reset\n");
-		goto disable_regulators;
+		goto disable_px_supply;
 	}
 
 	ret = wait_for_completion_timeout(&adsp->start_done,
@@ -133,14 +142,18 @@  static int adsp_start(struct rproc *rproc)
 		dev_err(adsp->dev, "start timed out\n");
 		qcom_scm_pas_shutdown(adsp->pas_id);
 		ret = -ETIMEDOUT;
-		goto disable_regulators;
+		goto disable_px_supply;
 	}
 
 	ret = 0;
 
-disable_regulators:
+disable_px_supply:
+	regulator_disable(adsp->px_supply);
+disable_cx_supply:
 	regulator_disable(adsp->cx_supply);
-disable_clocks:
+disable_aggre2_clk:
+	clk_disable_unprepare(adsp->aggre2_clk);
+disable_xo_clk:
 	clk_disable_unprepare(adsp->xo);
 
 	return ret;
@@ -251,17 +264,31 @@  static int adsp_init_clock(struct qcom_adsp *adsp)
 		return ret;
 	}
 
+	if (adsp->has_aggre2_clk) {
+		adsp->aggre2_clk = devm_clk_get(adsp->dev, "aggre2");
+		if (IS_ERR(adsp->aggre2_clk)) {
+			ret = PTR_ERR(adsp->aggre2_clk);
+			if (ret != -EPROBE_DEFER)
+				dev_err(adsp->dev,
+					"failed to get aggre2 clock");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
 static int adsp_init_regulator(struct qcom_adsp *adsp)
 {
-	adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
+	adsp->cx_supply = devm_regulator_get(adsp->dev, "vdd_cx");
 	if (IS_ERR(adsp->cx_supply))
 		return PTR_ERR(adsp->cx_supply);
 
 	regulator_set_load(adsp->cx_supply, 100000);
 
+	adsp->px_supply = devm_regulator_get(adsp->dev, "vdd_px");
+	if (IS_ERR(adsp->px_supply))
+		return PTR_ERR(adsp->px_supply);
 	return 0;
 }
 
@@ -358,6 +385,7 @@  static int adsp_probe(struct platform_device *pdev)
 
 	adsp->pas_id = desc->pas_id;
 	adsp->crash_reason_smem = desc->crash_reason_smem;
+	adsp->has_aggre2_clk = desc->has_aggre2_clk;
 	ret = adsp_init_clock(adsp);
 	if (ret)
 		goto free_rproc;
@@ -425,6 +453,7 @@  static int adsp_remove(struct platform_device *pdev)
 		.crash_reason_smem = 423,
 		.firmware_name = "adsp.mdt",
 		.pas_id = 1,
+		.has_aggre2_clk = 0,
 };
 static const struct of_device_id adsp_of_match[] = {
 	{ .compatible = "qcom,msm8974-adsp-pil", .data = &adsp_resource_init},