[PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Rajendra Nayak]

Once a gdsc is brought in and out of HW control, there is a
power down and up cycle which can take upto 1us. Polling on
the gdsc status immediately after the hw control enable/disable
can mislead software/firmware to belive the gdsc is already either on
or off, while its yet to complete the power cycle.
To avoid this add a 1us delay post a enable/disable of HW control
mode.

Also after the HW control mode is disabled, poll on the status to
check gdsc status reflects its 'on' before force disabling it
in software.

Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---

Stan,
If there was a specific issue you saw with venus because of the missing
delay and poll, can you check if this fixes any of that.

 drivers/clk/qcom/gdsc.c | 58 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 288186c..6fbd6df 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -63,11 +63,26 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
 	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
 }
 
+static int gdsc_poll_status(struct gdsc *sc, unsigned int reg, bool val)
+{
+	ktime_t start;
+
+	start = ktime_get();
+	do {
+		if (gdsc_is_enabled(sc, reg) == val)
+			return 0;
+	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
+
+	if (gdsc_is_enabled(sc, reg) == val)
+		return 0;
+
+	return -ETIMEDOUT;
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
 	int ret;
 	u32 val = en ? 0 : SW_COLLAPSE_MASK;
-	ktime_t start;
 	unsigned int status_reg = sc->gdscr;
 
 	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
@@ -100,16 +115,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 		udelay(1);
 	}
 
-	start = ktime_get();
-	do {
-		if (gdsc_is_enabled(sc, status_reg) == en)
-			return 0;
-	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
-
-	if (gdsc_is_enabled(sc, status_reg) == en)
-		return 0;
-
-	return -ETIMEDOUT;
+	return gdsc_poll_status(sc, status_reg, en);
 }
 
 static inline int gdsc_deassert_reset(struct gdsc *sc)
@@ -188,8 +194,19 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	udelay(1);
 
 	/* Turn on HW trigger mode if supported */
-	if (sc->flags & HW_CTRL)
-		return gdsc_hwctrl(sc, true);
+	if (sc->flags & HW_CTRL) {
+		ret = gdsc_hwctrl(sc, true);
+		/*
+		 * Wait for the GDSC to go through a power down and
+		 * up cycle.  In case a firmware ends up polling status
+		 * bits for the gdsc, it might read an 'on' status before
+		 * the GDSC can finish the power cycle.
+		 * We wait 1us before returning to ensure the firmware
+		 * can't immediately poll the status bits.
+		 */
+		mb();
+		udelay(1);
+	}
 
 	return 0;
 }
@@ -204,9 +221,24 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 
 	/* Turn off HW trigger mode if supported */
 	if (sc->flags & HW_CTRL) {
+		unsigned int reg;
+
 		ret = gdsc_hwctrl(sc, false);
 		if (ret < 0)
 			return ret;
+		/*
+		 * Wait for the GDSC to go through a power down and
+		 * up cycle.  In case we end up polling status
+		 * bits for the gdsc before the power cycle is completed
+		 * it might read an 'on' status wrongly.
+		 */
+		mb();
+		udelay(1);
+
+		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
+		ret = gdsc_poll_status(sc, reg, 0);
+		if (ret)
+			return ret;
 	}
 
 	if (sc->pwrsts & PWRSTS_OFF)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Stanimir Varbanov]

Hi Rajendra,

Thanks for the patch!

On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
> Once a gdsc is brought in and out of HW control, there is a
> power down and up cycle which can take upto 1us. Polling on
> the gdsc status immediately after the hw control enable/disable
> can mislead software/firmware to belive the gdsc is already either on
> or off, while its yet to complete the power cycle.
> To avoid this add a 1us delay post a enable/disable of HW control
> mode.
> 
> Also after the HW control mode is disabled, poll on the status to
> check gdsc status reflects its 'on' before force disabling it
> in software.
> 
> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
> 
> Stan,
> If there was a specific issue you saw with venus because of the missing
> delay and poll, can you check if this fixes any of that.
> 
>  drivers/clk/qcom/gdsc.c | 58 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 288186c..6fbd6df 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -63,11 +63,26 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
>  	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
>  }
>  
> +static int gdsc_poll_status(struct gdsc *sc, unsigned int reg, bool val)

Could you rename 'val' to 'en'.

> +{
> +	ktime_t start;
> +
> +	start = ktime_get();
> +	do {
> +		if (gdsc_is_enabled(sc, reg) == val)
> +			return 0;
> +	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> +
> +	if (gdsc_is_enabled(sc, reg) == val)
> +		return 0;
> +
> +	return -ETIMEDOUT;
> +}
> +
>  static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>  {
>  	int ret;
>  	u32 val = en ? 0 : SW_COLLAPSE_MASK;
> -	ktime_t start;
>  	unsigned int status_reg = sc->gdscr;
>  
>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
> @@ -100,16 +115,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>  		udelay(1);
>  	}
>  
> -	start = ktime_get();
> -	do {
> -		if (gdsc_is_enabled(sc, status_reg) == en)
> -			return 0;
> -	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> -
> -	if (gdsc_is_enabled(sc, status_reg) == en)
> -		return 0;
> -
> -	return -ETIMEDOUT;
> +	return gdsc_poll_status(sc, status_reg, en);
>  }
>  
>  static inline int gdsc_deassert_reset(struct gdsc *sc)
> @@ -188,8 +194,19 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	udelay(1);
>  
>  	/* Turn on HW trigger mode if supported */
> -	if (sc->flags & HW_CTRL)
> -		return gdsc_hwctrl(sc, true);
> +	if (sc->flags & HW_CTRL) {
> +		ret = gdsc_hwctrl(sc, true);
> +		/*
> +		 * Wait for the GDSC to go through a power down and
> +		 * up cycle.  In case a firmware ends up polling status
> +		 * bits for the gdsc, it might read an 'on' status before
> +		 * the GDSC can finish the power cycle.
> +		 * We wait 1us before returning to ensure the firmware
> +		 * can't immediately poll the status bits.
> +		 */
> +		mb();

Missing comment for this memory barrier, although I think it is
pointless because regmap-mmio using readl and writel.


> +		udelay(1);
> +	}
>  
>  	return 0;
>  }
> @@ -204,9 +221,24 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  
>  	/* Turn off HW trigger mode if supported */
>  	if (sc->flags & HW_CTRL) {
> +		unsigned int reg;
> +
>  		ret = gdsc_hwctrl(sc, false);
>  		if (ret < 0)
>  			return ret;
> +		/*
> +		 * Wait for the GDSC to go through a power down and
> +		 * up cycle.  In case we end up polling status
> +		 * bits for the gdsc before the power cycle is completed
> +		 * it might read an 'on' status wrongly.
> +		 */
> +		mb();

Same comment as above one.

> +		udelay(1);
> +
> +		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> +		ret = gdsc_poll_status(sc, reg, 0);

This should be gdsc_poll_status(sc, reg, true) because after disabling
hw_control we expect that the GDSC is in power_on state.

> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (sc->pwrsts & PWRSTS_OFF)
> 

-- 
regards,
Stan

Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Stanimir Varbanov]

Hi Rajendra,

On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
> Once a gdsc is brought in and out of HW control, there is a
> power down and up cycle which can take upto 1us. Polling on
> the gdsc status immediately after the hw control enable/disable
> can mislead software/firmware to belive the gdsc is already either on
> or off, while its yet to complete the power cycle.
> To avoid this add a 1us delay post a enable/disable of HW control
> mode.
> 
> Also after the HW control mode is disabled, poll on the status to
> check gdsc status reflects its 'on' before force disabling it
> in software.
> 
> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
> 
> Stan,
> If there was a specific issue you saw with venus because of the missing
> delay and poll, can you check if this fixes any of that.

Something more about the issue.

I had re-designed venus driver on three platform drivers venus-core,
venus-dec and venus-enc in order to be able to control those three
power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).

After that I abstracted MMAGIC hw on a separate mmagic driver. This
driver just controls mmagic clocks and GDSC in its runtime_suspend and
runtime_resume methods.

The DT nodes looks like:

mmagic_video {
	compatible = "qcom,msm8996-mmagic";
	clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
		 <&mmcc MMSS_MMAGIC_AHB_CLK>,
		 <&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
		 <&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
		 <&mmcc MMSS_MMAGIC_MAXI_CLK>,
		 <&mmcc MMAGIC_VIDEO_AXI_CLK>,
		 <&mmcc SMMU_VIDEO_AHB_CLK>,
		 <&mmcc SMMU_VIDEO_AXI_CLK>;
	power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;

	video-codec {
		compatible = "qcom,msm8996-venus";
		clocks = <&mmcc VIDEO_CORE_CLK>,
			 <&mmcc VIDEO_AHB_CLK>,
			 <&mmcc VIDEO_AXI_CLK>,
			 <&mmcc VIDEO_MAXI_CLK>;
		power-domains = <&mmcc VENUS_GDSC>;
		...
	
		video-decoder {
			compatible = "venus-decoder";
			clocks = "subcore0";
			clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
			power-domains = <&mmcc VENUS_CORE0_GDSC>;
		};

		video-encoder {
			compatible = "venus-encoder";
			clocks = "subcore1";
			clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
			power-domains = <&mmcc VENUS_CORE1_GDSC>;
		};
	};
};

Note that mmagic_video is a parent dt node for smmu_video DT node so
that clocks and mmagic_video gdsc will be enabled once smmu driver is
instantiated by venus-core diriver.

Now when video-dec driver calls pm_runtime_get_sync() the sequence of
enabling is:

MMAGIC_VIDEO_GDSC -> MMAGIC clocks and SMMU clocks -> VENUS_GDSC ->
VIDEO clocks -> VENUS_CORE0_GDSC -> VIDEO subcore0 clock

When video-dec platform driver calls pm_runtime_put_sync() we should
disabling of GDSC and clocks in the reversed oder.

The issue comes when I have ran video decoder, the decoder hw finish
stream decoding and we want to suspend venus core. The issue is that
when I start disabling SMMU_VIDEO_AXI_CLK and/or MMAGIC_VIDEO_AXI_CLK
the system reboots.

I have added a delay (200ms) before disabling mmagic clocks and then
everything is fine again.

Any idea?

-- 
regards,
Stan

RE: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Sricharan]

Hi stan,

>-----Original Message-----
>From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-owner@vger.kernel.org] On Behalf Of Stanimir Varbanov
>Sent: Tuesday, January 10, 2017 10:14 PM
>To: Rajendra Nayak <rnayak@codeaurora.org>; sboyd@codeaurora.org; mturquette@baylibre.com
>Cc: linux-clk@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; sricharan@codeaurora.org
>Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>
>Hi Rajendra,
>
>On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
>> Once a gdsc is brought in and out of HW control, there is a
>> power down and up cycle which can take upto 1us. Polling on
>> the gdsc status immediately after the hw control enable/disable
>> can mislead software/firmware to belive the gdsc is already either on
>> or off, while its yet to complete the power cycle.
>> To avoid this add a 1us delay post a enable/disable of HW control
>> mode.
>>
>> Also after the HW control mode is disabled, poll on the status to
>> check gdsc status reflects its 'on' before force disabling it
>> in software.
>>
>> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>
>> Stan,
>> If there was a specific issue you saw with venus because of the missing
>> delay and poll, can you check if this fixes any of that.
>
>Something more about the issue.
>
>I had re-designed venus driver on three platform drivers venus-core,
>venus-dec and venus-enc in order to be able to control those three
>power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).
>
>After that I abstracted MMAGIC hw on a separate mmagic driver. This
>driver just controls mmagic clocks and GDSC in its runtime_suspend and
>runtime_resume methods.
>
>The DT nodes looks like:
>
>mmagic_video {
>	compatible = "qcom,msm8996-mmagic";
>	clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
>		 <&mmcc MMSS_MMAGIC_AHB_CLK>,
>		 <&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
>		 <&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
>		 <&mmcc MMSS_MMAGIC_MAXI_CLK>,
>		 <&mmcc MMAGIC_VIDEO_AXI_CLK>,
>		 <&mmcc SMMU_VIDEO_AHB_CLK>,
>		 <&mmcc SMMU_VIDEO_AXI_CLK>;
>	power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;
>
>	video-codec {
>		compatible = "qcom,msm8996-venus";
>		clocks = <&mmcc VIDEO_CORE_CLK>,
>			 <&mmcc VIDEO_AHB_CLK>,
>			 <&mmcc VIDEO_AXI_CLK>,
>			 <&mmcc VIDEO_MAXI_CLK>;
>		power-domains = <&mmcc VENUS_GDSC>;
>		...
>
>		video-decoder {
>			compatible = "venus-decoder";
>			clocks = "subcore0";
>			clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
>			power-domains = <&mmcc VENUS_CORE0_GDSC>;
>		};
>
>		video-encoder {
>			compatible = "venus-encoder";
>			clocks = "subcore1";
>			clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
>			power-domains = <&mmcc VENUS_CORE1_GDSC>;
>		};
>	};
>};
>
>Note that mmagic_video is a parent dt node for smmu_video DT node so
>that clocks and mmagic_video gdsc will be enabled once smmu driver is
>instantiated by venus-core diriver.
>

mmagic_video is a parent DT for smmu_video ? , so there are no clocks
populated for the smmu node as such ?

>Now when video-dec driver calls pm_runtime_get_sync() the sequence of
>enabling is:
>
>MMAGIC_VIDEO_GDSC -> MMAGIC clocks and SMMU clocks -> VENUS_GDSC ->
>VIDEO clocks -> VENUS_CORE0_GDSC -> VIDEO subcore0 clock
>
>When video-dec platform driver calls pm_runtime_put_sync() we should
>disabling of GDSC and clocks in the reversed oder.
>
>The issue comes when I have ran video decoder, the decoder hw finish
>stream decoding and we want to suspend venus core. The issue is that
>when I start disabling SMMU_VIDEO_AXI_CLK and/or MMAGIC_VIDEO_AXI_CLK
>the system reboots.
>
>I have added a delay (200ms) before disabling mmagic clocks and then
>everything is fine again.
>
>Any idea?
>

Can you share me a branch, i can have a quick check with a t32
if there is any crash logged in the TZ buffer when the system reboots.

Regards,
 Sricharan

Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Stanimir Varbanov]

Hi Sricharan,

On 01/10/2017 09:29 PM, Sricharan wrote:
> Hi stan,
> 
>> -----Original Message-----
>> From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-owner@vger.kernel.org] On Behalf Of Stanimir Varbanov
>> Sent: Tuesday, January 10, 2017 10:14 PM
>> To: Rajendra Nayak <rnayak@codeaurora.org>; sboyd@codeaurora.org; mturquette@baylibre.com
>> Cc: linux-clk@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; sricharan@codeaurora.org
>> Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>>
>> Hi Rajendra,
>>
>> On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
>>> Once a gdsc is brought in and out of HW control, there is a
>>> power down and up cycle which can take upto 1us. Polling on
>>> the gdsc status immediately after the hw control enable/disable
>>> can mislead software/firmware to belive the gdsc is already either on
>>> or off, while its yet to complete the power cycle.
>>> To avoid this add a 1us delay post a enable/disable of HW control
>>> mode.
>>>
>>> Also after the HW control mode is disabled, poll on the status to
>>> check gdsc status reflects its 'on' before force disabling it
>>> in software.
>>>
>>> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>> ---
>>>
>>> Stan,
>>> If there was a specific issue you saw with venus because of the missing
>>> delay and poll, can you check if this fixes any of that.
>>
>> Something more about the issue.
>>
>> I had re-designed venus driver on three platform drivers venus-core,
>> venus-dec and venus-enc in order to be able to control those three
>> power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).
>>
>> After that I abstracted MMAGIC hw on a separate mmagic driver. This
>> driver just controls mmagic clocks and GDSC in its runtime_suspend and
>> runtime_resume methods.
>>
>> The DT nodes looks like:
>>
>> mmagic_video {
>> 	compatible = "qcom,msm8996-mmagic";
>> 	clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
>> 		 <&mmcc MMSS_MMAGIC_AHB_CLK>,
>> 		 <&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
>> 		 <&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
>> 		 <&mmcc MMSS_MMAGIC_MAXI_CLK>,
>> 		 <&mmcc MMAGIC_VIDEO_AXI_CLK>,
>> 		 <&mmcc SMMU_VIDEO_AHB_CLK>,
>> 		 <&mmcc SMMU_VIDEO_AXI_CLK>;
>> 	power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;
>>
>> 	video-codec {
>> 		compatible = "qcom,msm8996-venus";
>> 		clocks = <&mmcc VIDEO_CORE_CLK>,
>> 			 <&mmcc VIDEO_AHB_CLK>,
>> 			 <&mmcc VIDEO_AXI_CLK>,
>> 			 <&mmcc VIDEO_MAXI_CLK>;
>> 		power-domains = <&mmcc VENUS_GDSC>;
>> 		...
>>
>> 		video-decoder {
>> 			compatible = "venus-decoder";
>> 			clocks = "subcore0";
>> 			clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
>> 			power-domains = <&mmcc VENUS_CORE0_GDSC>;
>> 		};
>>
>> 		video-encoder {
>> 			compatible = "venus-encoder";
>> 			clocks = "subcore1";
>> 			clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
>> 			power-domains = <&mmcc VENUS_CORE1_GDSC>;
>> 		};
>> 	};
>> };
>>
>> Note that mmagic_video is a parent dt node for smmu_video DT node so
>> that clocks and mmagic_video gdsc will be enabled once smmu driver is
>> instantiated by venus-core diriver.
>>
> 
> mmagic_video is a parent DT for smmu_video ? , so there are no clocks
> populated for the smmu node as such ?

Yes, I completely disabled runtime pm in the arm-smmu driver.

> 
>> Now when video-dec driver calls pm_runtime_get_sync() the sequence of
>> enabling is:
>>
>> MMAGIC_VIDEO_GDSC -> MMAGIC clocks and SMMU clocks -> VENUS_GDSC ->
>> VIDEO clocks -> VENUS_CORE0_GDSC -> VIDEO subcore0 clock
>>
>> When video-dec platform driver calls pm_runtime_put_sync() we should
>> disabling of GDSC and clocks in the reversed oder.
>>
>> The issue comes when I have ran video decoder, the decoder hw finish
>> stream decoding and we want to suspend venus core. The issue is that
>> when I start disabling SMMU_VIDEO_AXI_CLK and/or MMAGIC_VIDEO_AXI_CLK
>> the system reboots.
>>
>> I have added a delay (200ms) before disabling mmagic clocks and then
>> everything is fine again.
>>
>> Any idea?
>>
> 
> Can you share me a branch, i can have a quick check with a t32
> if there is any crash logged in the TZ buffer when the system reboots.

I can share a branch but you will need my initramfs too.

I don't think it is tz related, most probably it is MMAGIC sequence
issue or something in GDSC/MMCC.

-- 
regards,
Stan

RE: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Sricharan]

Hi Stan,

>-----Original Message-----
>From: Stanimir Varbanov [mailto:stanimir.varbanov@linaro.org]
>Sent: Wednesday, January 11, 2017 2:25 PM
>To: Sricharan <sricharan@codeaurora.org>; 'Stanimir Varbanov' <stanimir.varbanov@linaro.org>; 'Rajendra Nayak'
><rnayak@codeaurora.org>; sboyd@codeaurora.org; mturquette@baylibre.com
>Cc: linux-clk@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>
>Hi Sricharan,
>
>On 01/10/2017 09:29 PM, Sricharan wrote:
>> Hi stan,
>>
>>> -----Original Message-----
>>> From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-owner@vger.kernel.org] On Behalf Of Stanimir Varbanov
>>> Sent: Tuesday, January 10, 2017 10:14 PM
>>> To: Rajendra Nayak <rnayak@codeaurora.org>; sboyd@codeaurora.org; mturquette@baylibre.com
>>> Cc: linux-clk@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; sricharan@codeaurora.org
>>> Subject: Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable
>>>
>>> Hi Rajendra,
>>>
>>> On 01/10/2017 07:54 AM, Rajendra Nayak wrote:
>>>> Once a gdsc is brought in and out of HW control, there is a
>>>> power down and up cycle which can take upto 1us. Polling on
>>>> the gdsc status immediately after the hw control enable/disable
>>>> can mislead software/firmware to belive the gdsc is already either on
>>>> or off, while its yet to complete the power cycle.
>>>> To avoid this add a 1us delay post a enable/disable of HW control
>>>> mode.
>>>>
>>>> Also after the HW control mode is disabled, poll on the status to
>>>> check gdsc status reflects its 'on' before force disabling it
>>>> in software.
>>>>
>>>> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>>> ---
>>>>
>>>> Stan,
>>>> If there was a specific issue you saw with venus because of the missing
>>>> delay and poll, can you check if this fixes any of that.
>>>
>>> Something more about the issue.
>>>
>>> I had re-designed venus driver on three platform drivers venus-core,
>>> venus-dec and venus-enc in order to be able to control those three
>>> power-domains (VENUS_GDSC, VENUS_CORE0_GDSC and VENUS_CORE1_GDSC).
>>>
>>> After that I abstracted MMAGIC hw on a separate mmagic driver. This
>>> driver just controls mmagic clocks and GDSC in its runtime_suspend and
>>> runtime_resume methods.
>>>
>>> The DT nodes looks like:
>>>
>>> mmagic_video {
>>> 	compatible = "qcom,msm8996-mmagic";
>>> 	clocks = <&rpmcc MSM8996_RPM_SMD_MMAXI_CLK>,
>>> 		 <&mmcc MMSS_MMAGIC_AHB_CLK>,
>>> 		 <&mmcc MMSS_MMAGIC_CFG_AHB_CLK>,
>>> 		 <&mmcc MMAGIC_VIDEO_NOC_CFG_AHB_CLK>,
>>> 		 <&mmcc MMSS_MMAGIC_MAXI_CLK>,
>>> 		 <&mmcc MMAGIC_VIDEO_AXI_CLK>,
>>> 		 <&mmcc SMMU_VIDEO_AHB_CLK>,
>>> 		 <&mmcc SMMU_VIDEO_AXI_CLK>;
>>> 	power-domains = <&mmcc MMAGIC_VIDEO_GDSC>;
>>>
>>> 	video-codec {
>>> 		compatible = "qcom,msm8996-venus";
>>> 		clocks = <&mmcc VIDEO_CORE_CLK>,
>>> 			 <&mmcc VIDEO_AHB_CLK>,
>>> 			 <&mmcc VIDEO_AXI_CLK>,
>>> 			 <&mmcc VIDEO_MAXI_CLK>;
>>> 		power-domains = <&mmcc VENUS_GDSC>;
>>> 		...
>>>
>>> 		video-decoder {
>>> 			compatible = "venus-decoder";
>>> 			clocks = "subcore0";
>>> 			clock-names = <&mmcc VIDEO_SUBCORE0_CLK>;
>>> 			power-domains = <&mmcc VENUS_CORE0_GDSC>;
>>> 		};
>>>
>>> 		video-encoder {
>>> 			compatible = "venus-encoder";
>>> 			clocks = "subcore1";
>>> 			clock-names = <&mmcc VIDEO_SUBCORE1_CLK>;
>>> 			power-domains = <&mmcc VENUS_CORE1_GDSC>;
>>> 		};
>>> 	};
>>> };
>>>
>>> Note that mmagic_video is a parent dt node for smmu_video DT node so
>>> that clocks and mmagic_video gdsc will be enabled once smmu driver is
>>> instantiated by venus-core diriver.
>>>
>>
>> mmagic_video is a parent DT for smmu_video ? , so there are no clocks
>> populated for the smmu node as such ?
>
>Yes, I completely disabled runtime pm in the arm-smmu driver.
>

So completely disabling runtime pm in smmu has a downside because,
when the smmu is accessed standalone like, say in case of a fault,
would then fail. Anyways i think this is experimental probably.

>>
>>> Now when video-dec driver calls pm_runtime_get_sync() the sequence of
>>> enabling is:
>>>
>>> MMAGIC_VIDEO_GDSC -> MMAGIC clocks and SMMU clocks -> VENUS_GDSC ->
>>> VIDEO clocks -> VENUS_CORE0_GDSC -> VIDEO subcore0 clock
>>>
>>> When video-dec platform driver calls pm_runtime_put_sync() we should
>>> disabling of GDSC and clocks in the reversed oder.
>>>
>>> The issue comes when I have ran video decoder, the decoder hw finish
>>> stream decoding and we want to suspend venus core. The issue is that
>>> when I start disabling SMMU_VIDEO_AXI_CLK and/or MMAGIC_VIDEO_AXI_CLK
>>> the system reboots.
>>>
>>> I have added a delay (200ms) before disabling mmagic clocks and then
>>> everything is fine again.
>>>
>>> Any idea?
>>>
>>
>> Can you share me a branch, i can have a quick check with a t32
>> if there is any crash logged in the TZ buffer when the system reboots.
>
>I can share a branch but you will need my initramfs too.
>
>I don't think it is tz related, most probably it is MMAGIC sequence
>issue or something in GDSC/MMCC.
>

I was not suspecting a TZ issue, but some crash because of which the board reboots and
debug msgs getting logged in the TZ log buffer.  Not sure if you already proceeded
further on this.

Regards,
 Sricharan

Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Stephen Boyd]

On 01/10, Stanimir Varbanov wrote:
> 
> > +		udelay(1);
> > +
> > +		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
> > +		ret = gdsc_poll_status(sc, reg, 0);
> 
> This should be gdsc_poll_status(sc, reg, true) because after disabling
> hw_control we expect that the GDSC is in power_on state.
> 
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> >  	if (sc->pwrsts & PWRSTS_OFF)
> > 

I expect this patch is going for v2?

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

Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Rajendra Nayak]

On 01/21/2017 04:50 AM, Stephen Boyd wrote:
> On 01/10, Stanimir Varbanov wrote:
>>
>>> +		udelay(1);
>>> +
>>> +		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
>>> +		ret = gdsc_poll_status(sc, reg, 0);
>>
>> This should be gdsc_poll_status(sc, reg, true) because after disabling
>> hw_control we expect that the GDSC is in power_on state.
>>
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>>  
>>>  	if (sc->pwrsts & PWRSTS_OFF)
>>>
> 
> I expect this patch is going for v2?

Yes, sorry, coming up shortly.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] clk: qcom: gdsc: Fix handling of hw control enable/disable

[Rajendra Nayak]

[]..

>> ---
>>
>> Stan,
>> If there was a specific issue you saw with venus because of the missing
>> delay and poll, can you check if this fixes any of that.
>>
>>  drivers/clk/qcom/gdsc.c | 58 ++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 45 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 288186c..6fbd6df 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -63,11 +63,26 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
>>  	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
>>  }
>>  
>> +static int gdsc_poll_status(struct gdsc *sc, unsigned int reg, bool val)
> 
> Could you rename 'val' to 'en'.

yes, I seem to have messed up with the value to write into the register and
the one to poll. I will fix this up and post a v2.

> 
>> +{
>> +	ktime_t start;
>> +
>> +	start = ktime_get();
>> +	do {
>> +		if (gdsc_is_enabled(sc, reg) == val)
>> +			return 0;
>> +	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
>> +
>> +	if (gdsc_is_enabled(sc, reg) == val)
>> +		return 0;
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>>  static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  {
>>  	int ret;
>>  	u32 val = en ? 0 : SW_COLLAPSE_MASK;
>> -	ktime_t start;
>>  	unsigned int status_reg = sc->gdscr;
>>  
>>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
>> @@ -100,16 +115,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  		udelay(1);
>>  	}
>>  
>> -	start = ktime_get();
>> -	do {
>> -		if (gdsc_is_enabled(sc, status_reg) == en)
>> -			return 0;
>> -	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
>> -
>> -	if (gdsc_is_enabled(sc, status_reg) == en)
>> -		return 0;
>> -
>> -	return -ETIMEDOUT;
>> +	return gdsc_poll_status(sc, status_reg, en);
>>  }
>>  
>>  static inline int gdsc_deassert_reset(struct gdsc *sc)
>> @@ -188,8 +194,19 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>  	udelay(1);
>>  
>>  	/* Turn on HW trigger mode if supported */
>> -	if (sc->flags & HW_CTRL)
>> -		return gdsc_hwctrl(sc, true);
>> +	if (sc->flags & HW_CTRL) {
>> +		ret = gdsc_hwctrl(sc, true);
>> +		/*
>> +		 * Wait for the GDSC to go through a power down and
>> +		 * up cycle.  In case a firmware ends up polling status
>> +		 * bits for the gdsc, it might read an 'on' status before
>> +		 * the GDSC can finish the power cycle.
>> +		 * We wait 1us before returning to ensure the firmware
>> +		 * can't immediately poll the status bits.
>> +		 */
>> +		mb();
> 
> Missing comment for this memory barrier, although I think it is
> pointless because regmap-mmio using readl and writel.

will get rid of the barrier here and below

> 
> 
>> +		udelay(1);
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -204,9 +221,24 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  
>>  	/* Turn off HW trigger mode if supported */
>>  	if (sc->flags & HW_CTRL) {
>> +		unsigned int reg;
>> +
[]..

> 
>> +		udelay(1);
>> +
>> +		reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
>> +		ret = gdsc_poll_status(sc, reg, 0);
> 
> This should be gdsc_poll_status(sc, reg, true) because after disabling
> hw_control we expect that the GDSC is in power_on state.

correct, will fix. thanks for the review.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation