[PATCH 0/3] clk: qcom: Add support for hw controlled gdscs/clocks

[PATCH 0/3] clk: qcom: Add support for hw controlled gdscs/clocks

[Sricharan R]

This series adds support for gdscs(powerdomains) that can be configured
in hw controlled mode. So they are turned 'ON' based on needs dynamically,
helping to save power. Also updated the venus video ip's gdsc/clock
data to put them in hw control.

Rajendra Nayak (1):
  clk: qcom: gdsc: Add support for gdscs with HW control

Sricharan R (2):
  clk: qcom: Put venus core0/1 gdscs to hw control mode
  clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

 drivers/clk/qcom/gdsc.c         | 15 +++++++++++++++
 drivers/clk/qcom/gdsc.h         |  1 +
 drivers/clk/qcom/mmcc-msm8996.c |  4 ++++
 3 files changed, 20 insertions(+)

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

[PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan R]

From: Rajendra Nayak <rnayak@codeaurora.org>

Some GDSCs might support a HW control mode, where in the power
domain (gdsc) is brought in and out of low power state (while
unsued) without any SW assistance, saving power.
Such GDSCs can be configured in a HW control mode when powered on
until they are explicitly requested to be powered off by software.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 15 +++++++++++++++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index f12d7b2..a5e1c8c 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
 	return !!(val & PWR_ON_MASK);
 }
 
+static int gdsc_hwctrl(struct gdsc *sc, bool en)
+{
+	u32 val = en ? HW_CONTROL_MASK : 0;
+
+	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, bool en)
 {
 	int ret;
@@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	 */
 	udelay(1);
 
+	/* Turn on HW trigger mode if supported */
+	if (sc->flags & HW_CTRL)
+		gdsc_hwctrl(sc, true);
+
 	return 0;
 }
 
@@ -174,6 +185,10 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_assert_reset(sc);
 
+	/* Turn off HW trigger mode if supported */
+	if (sc->flags & HW_CTRL)
+		gdsc_hwctrl(sc, false);
+
 	if (sc->pwrsts & PWRSTS_OFF)
 		gdsc_clear_mem_on(sc);
 
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 3bf497c..b1f30f8 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -50,6 +50,7 @@ struct gdsc {
 #define PWRSTS_RET_ON		(PWRSTS_RET | PWRSTS_ON)
 	const u8			flags;
 #define VOTABLE		BIT(0)
+#define HW_CTRL		BIT(1)
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/3] clk: qcom: Put venus core0/1 gdscs to hw control mode

[Sricharan R]

The venus video ip's internal core blocks are under the
control of the firmware and their powerdomains needs to be
'ON' only when used by the firmware. So putting it into
hw controlled mode lets this to happen, otherwise the firmware
hangs checking for this.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/clk/qcom/mmcc-msm8996.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index ca97e11..41aabe3 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2945,6 +2945,7 @@ enum {
 		.name = "venus_core0",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = HW_CTRL,
 };
 
 static struct gdsc venus_core1_gdsc = {
@@ -2955,6 +2956,7 @@ enum {
 		.name = "venus_core1",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = HW_CTRL,
 };
 
 static struct gdsc camss_gdsc = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan R]

With the venus subcore0/1 gdscs(powerdomains) in
hw controlled mode, the clock controller does not handle
the status bit for the clocks in that domain. So avoid
checking for the status bit of those clocks by setting the
BRANCH_HALT_DELAY flag. This avoids the WARN_ONs which
otherwise occurs when enabling/disabling those clocks.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/clk/qcom/mmcc-msm8996.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index 41aabe3..8f3f480 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -1760,6 +1760,7 @@ enum {
 };
 
 static struct clk_branch video_subcore0_clk = {
+	.halt_check = BRANCH_HALT_DELAY,
 	.halt_reg = 0x1048,
 	.clkr = {
 		.enable_reg = 0x1048,
@@ -1775,6 +1776,7 @@ enum {
 };
 
 static struct clk_branch video_subcore1_clk = {
+	.halt_check = BRANCH_HALT_DELAY,
 	.halt_reg = 0x104c,
 	.clkr = {
 		.enable_reg = 0x104c,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Stanimir Varbanov]

Hi Sricharan,

On 10/24/2016 01:18 PM, Sricharan R wrote:
> From: Rajendra Nayak <rnayak@codeaurora.org>
> 
> Some GDSCs might support a HW control mode, where in the power
> domain (gdsc) is brought in and out of low power state (while
> unsued) without any SW assistance, saving power.
> Such GDSCs can be configured in a HW control mode when powered on
> until they are explicitly requested to be powered off by software.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 15 +++++++++++++++
>  drivers/clk/qcom/gdsc.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index f12d7b2..a5e1c8c 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
>  	return !!(val & PWR_ON_MASK);
>  }
>  
> +static int gdsc_hwctrl(struct gdsc *sc, bool en)
> +{
> +	u32 val = en ? HW_CONTROL_MASK : 0;
> +
> +	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
> +}
> +
>  static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>  {
>  	int ret;
> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> +	/* Turn on HW trigger mode if supported */
> +	if (sc->flags & HW_CTRL)
> +		gdsc_hwctrl(sc, true);

Could you check gdsc_hwctrl() for an error.

<cut>

-- 
regards,
Stan

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stan,

>Hi Sricharan,
>
>On 10/24/2016 01:18 PM, Sricharan R wrote:
>> From: Rajendra Nayak <rnayak@codeaurora.org>
>>
>> Some GDSCs might support a HW control mode, where in the power
>> domain (gdsc) is brought in and out of low power state (while
>> unsued) without any SW assistance, saving power.
>> Such GDSCs can be configured in a HW control mode when powered on
>> until they are explicitly requested to be powered off by software.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>>  drivers/clk/qcom/gdsc.c | 15 +++++++++++++++
>>  drivers/clk/qcom/gdsc.h |  1 +
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index f12d7b2..a5e1c8c 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -55,6 +55,13 @@ static int gdsc_is_enabled(struct gdsc *sc, unsigned int reg)
>>  	return !!(val & PWR_ON_MASK);
>>  }
>>
>> +static int gdsc_hwctrl(struct gdsc *sc, bool en)
>> +{
>> +	u32 val = en ? HW_CONTROL_MASK : 0;
>> +
>> +	return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
>> +}
>> +
>>  static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  {
>>  	int ret;
>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>  	 */
>>  	udelay(1);
>>
>> +	/* Turn on HW trigger mode if supported */
>> +	if (sc->flags & HW_CTRL)
>> +		gdsc_hwctrl(sc, true);
>
   Sure, will add the check.

Regards,
 Sricharan

Re: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Stephen Boyd]

On 10/24, Sricharan R wrote:
> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> +	/* Turn on HW trigger mode if supported */
> +	if (sc->flags & HW_CTRL)
> +		gdsc_hwctrl(sc, true);
> +

It sounds like this will cause glitches if the hardware isn't
asserting their hw control bit by default? This has me concerned
that we can't just throw the hw control enable part into here,
because that bit doesn't live in the clock controller, instead it
lives in the hw block that is powered by the power domain?

Or does the power on reset value of that hw control signal
asserted? If that's true then we should be ok to force it into hw
control mode by default.

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

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stephen,

>On 10/24, Sricharan R wrote:
>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>  	 */
>>  	udelay(1);
>>
>> +	/* Turn on HW trigger mode if supported */
>> +	if (sc->flags & HW_CTRL)
>> +		gdsc_hwctrl(sc, true);
>> +
>
>It sounds like this will cause glitches if the hardware isn't
>asserting their hw control bit by default? This has me concerned
>that we can't just throw the hw control enable part into here,
>because that bit doesn't live in the clock controller, instead it
>lives in the hw block that is powered by the power domain?
>
>Or does the power on reset value of that hw control signal
>asserted? If that's true then we should be ok to force it into hw
>control mode by default.
>

The hw control bit is set by default. Instead its turned 'off'
with the reset value. So it has to not 
be turned 'on' at some point
to put the gdsc in hw control if required. This bit is part of the
gdscr register. So i did not quite understand the reason for the
glitch here ?

Regards,
 Sricharan

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi,

>-----Original Message-----
>From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-owner@vger.kernel.org] On Behalf Of Sricharan
>Sent: Wednesday, November 02, 2016 12:21 PM
>To: 'Stephen Boyd' <sboyd@codeaurora.org>
>Cc: mturquette@baylibre.com; linux-clk@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org;
>rnayak@codeaurora.org; stanimir.varbanov@linaro.org
>Subject: RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control
>
>Hi Stephen,
>
>>On 10/24, Sricharan R wrote:
>>> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>>  	 */
>>>  	udelay(1);
>>>
>>> +	/* Turn on HW trigger mode if supported */
>>> +	if (sc->flags & HW_CTRL)
>>> +		gdsc_hwctrl(sc, true);
>>> +
>>
>>It sounds like this will cause glitches if the hardware isn't
>>asserting their hw control bit by default? This has me concerned
>>that we can't just throw the hw control enable part into here,
>>because that bit doesn't live in the clock controller, instead it
>>lives in the hw block that is powered by the power domain?
>>
>>Or does the power on reset value of that hw control signal
>>asserted? If that's true then we should be ok to force it into hw
>>control mode by default.
>>
>
>The hw control bit is set by default. Instead its turned 'off'
>with the reset value. So it has to not
>be turned 'on' at some point
>to put the gdsc in hw control if required. This bit is part of the
>gdscr register. So i did not quite understand the reason for the
>glitch here ?
>

typo above, i meant it has to be turned 'on' at some point
if required.

Regards,
 Sricharan

Re: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

['Stephen Boyd']

On 11/02, Sricharan wrote:
> Hi Stephen,
> 
> >On 10/24, Sricharan R wrote:
> >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> >>  	 */
> >>  	udelay(1);
> >>
> >> +	/* Turn on HW trigger mode if supported */
> >> +	if (sc->flags & HW_CTRL)
> >> +		gdsc_hwctrl(sc, true);
> >> +
> >
> >It sounds like this will cause glitches if the hardware isn't
> >asserting their hw control bit by default? This has me concerned
> >that we can't just throw the hw control enable part into here,
> >because that bit doesn't live in the clock controller, instead it
> >lives in the hw block that is powered by the power domain?
> >
> >Or does the power on reset value of that hw control signal
> >asserted? If that's true then we should be ok to force it into hw
> >control mode by default.
> >
> 
> The hw control bit is set by default. Instead its turned 'off'
> with the reset value. So it has to not 
> be turned 'on' at some point
> to put the gdsc in hw control if required. This bit is part of the
> gdscr register. So i did not quite understand the reason for the
> glitch here ?

I mean the reset value of the hw control signal inside the device
that is inside the GDSC power domain. For example, the hw control
bit inside the video core.

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

RE: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stephen,

>> >On 10/24, Sricharan R wrote:
>> >> @@ -164,6 +171,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>> >>  	 */
>> >>  	udelay(1);
>> >>
>> >> +	/* Turn on HW trigger mode if supported */
>> >> +	if (sc->flags & HW_CTRL)
>> >> +		gdsc_hwctrl(sc, true);
>> >> +
>> >
>> >It sounds like this will cause glitches if the hardware isn't
>> >asserting their hw control bit by default? This has me concerned
>> >that we can't just throw the hw control enable part into here,
>> >because that bit doesn't live in the clock controller, instead it
>> >lives in the hw block that is powered by the power domain?
>> >
>> >Or does the power on reset value of that hw control signal
>> >asserted? If that's true then we should be ok to force it into hw
>> >control mode by default.
>> >
>>
>> The hw control bit is set by default. Instead its turned 'off'
>> with the reset value. So it has to not
>> be turned 'on' at some point
>> to put the gdsc in hw control if required. This bit is part of the
>> gdscr register. So i did not quite understand the reason for the
>> glitch here ?
>
>I mean the reset value of the hw control signal inside the device
>that is inside the GDSC power domain. For example, the hw control
>bit inside the video core.
>
 Ok, so the video ip core, has a hw control signal/bit.
 I checked this by dumping this out that,  the moment the
gdsc is put to hw control, the video ip's hw control bit also
gets asserted/set. so this means that video ip's bit get
aligned with the gdsc setting.  so this should avoid the
glitches, right ?

Regards,
 Sricharan

Re: [PATCH 1/3] clk: qcom: gdsc: Add support for gdscs with HW control

['Stephen Boyd']

On 11/03, Sricharan wrote:
>  Ok, so the video ip core, has a hw control signal/bit.
>  I checked this by dumping this out that,  the moment the
> gdsc is put to hw control, the video ip's hw control bit also
> gets asserted/set. so this means that video ip's bit get
> aligned with the gdsc setting.  so this should avoid the
> glitches, right ?
> 

Yes that matches my understanding. Thanks for confirming.

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

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Stephen Boyd]

On 10/24, Sricharan R wrote:
> With the venus subcore0/1 gdscs(powerdomains) in
> hw controlled mode, the clock controller does not handle
> the status bit for the clocks in that domain. So avoid
> checking for the status bit of those clocks by setting the
> BRANCH_HALT_DELAY flag. This avoids the WARN_ONs which
> otherwise occurs when enabling/disabling those clocks.
> 
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>

A better design would be to check if the associated GDSC is in hw
control mode and then skip the checks because the clocks are no
longer under the control of the registers. I presume we only
enable the clocks here to turn on parent clocks which don't turn
on/off automatically, i.e. the PLL.

Given that hw control is a static decision I guess that is an
over-engineered solution though? The problem is that this seems
brittle because we have to keep two things in sync, the branches
and the gdsc. So I guess this is ok, but it deserves a comment
like "GDSC is in HW control" so we know what's going on. Also the
commit text could be more explicit that clocks within the gdsc
power domain don't work when the gdsc is off, and with hw control
of a gdsc we can't tell when the gdsc may be off or on.

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

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi,
>
>A better design would be to check if the associated GDSC is in hw
>control mode and then skip the checks because the clocks are no
>longer under the control of the registers. I presume we only
>enable the clocks here to turn on parent clocks which don't turn
>on/off automatically, i.e. the PLL.
>
I was thinking clocks in the powerdomain still needs to be turned
on explicitly, these are branch clocks, irrespective of the PLLs.
Putting the gdsc in hw_ctrl, only makes the polling on their status
invalid. Anyways would be good to be aligned on this.

>Given that hw control is a static decision I guess that is an
>over-engineered solution though? The problem is that this seems
>brittle because we have to keep two things in sync, the branches
>and the gdsc. So I guess this is ok, but it deserves a comment
>like "GDSC is in HW control" so we know what's going on. Also the
>commit text could be more explicit that clocks within the gdsc
>power domain don't work when the gdsc is off, and with hw control
>of a gdsc we can't tell when the gdsc may be off or on.
>

ok, i will reword the commit log better as above.

So i understand its ok to continue with this way of checking ?
since we are always having a static association which never changes,
than introducing additional fields in the clk_branch which can
get the status of the gdsc.

Regards,
 Sricharan

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

['Stephen Boyd']

On 11/04, Sricharan wrote:
> Hi,
> >
> >A better design would be to check if the associated GDSC is in hw
> >control mode and then skip the checks because the clocks are no
> >longer under the control of the registers. I presume we only
> >enable the clocks here to turn on parent clocks which don't turn
> >on/off automatically, i.e. the PLL.
> >
> I was thinking clocks in the powerdomain still needs to be turned
> on explicitly, these are branch clocks, irrespective of the PLLs.
> Putting the gdsc in hw_ctrl, only makes the polling on their status
> invalid. Anyways would be good to be aligned on this.

Sure. We also need to make sure the branches are on themselves.
When the gdsc is disabled the clocks are killed though. This is
why we can't enable clocks until the gdsc is enabled.

> 
> >Given that hw control is a static decision I guess that is an
> >over-engineered solution though? The problem is that this seems
> >brittle because we have to keep two things in sync, the branches
> >and the gdsc. So I guess this is ok, but it deserves a comment
> >like "GDSC is in HW control" so we know what's going on. Also the
> >commit text could be more explicit that clocks within the gdsc
> >power domain don't work when the gdsc is off, and with hw control
> >of a gdsc we can't tell when the gdsc may be off or on.
> >
> 
> ok, i will reword the commit log better as above.
> 
> So i understand its ok to continue with this way of checking ?
> since we are always having a static association which never changes,
> than introducing additional fields in the clk_branch which can
> get the status of the gdsc.
> 

Well I'm also curious which case is failing. Does turning on the
clocks work after the gdsc is enabled? Does turning off the
clocks fail because we don't know when the gdsc has turned off? I
would hope that the firmware keeps the gdsc on when it's done
processing things, goes idle, and hands back control to software.
Right now I'm failing to see how the halt bits fail to toggle
assuming that firmware isn't misbehaving and the kernel driver is
power controlling in a coordinated manner with the firmware.

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

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Rajendra Nayak]

On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote:
> On 11/04, Sricharan wrote:
>> Hi,
>>>
>>> A better design would be to check if the associated GDSC is in hw
>>> control mode and then skip the checks because the clocks are no
>>> longer under the control of the registers. I presume we only
>>> enable the clocks here to turn on parent clocks which don't turn
>>> on/off automatically, i.e. the PLL.
>>>
>> I was thinking clocks in the powerdomain still needs to be turned
>> on explicitly, these are branch clocks, irrespective of the PLLs.
>> Putting the gdsc in hw_ctrl, only makes the polling on their status
>> invalid. Anyways would be good to be aligned on this.
> 
> Sure. We also need to make sure the branches are on themselves.
> When the gdsc is disabled the clocks are killed though. This is
> why we can't enable clocks until the gdsc is enabled.
> 
>>
>>> Given that hw control is a static decision I guess that is an
>>> over-engineered solution though? The problem is that this seems
>>> brittle because we have to keep two things in sync, the branches
>>> and the gdsc. So I guess this is ok, but it deserves a comment
>>> like "GDSC is in HW control" so we know what's going on. Also the
>>> commit text could be more explicit that clocks within the gdsc
>>> power domain don't work when the gdsc is off, and with hw control
>>> of a gdsc we can't tell when the gdsc may be off or on.
>>>
>>
>> ok, i will reword the commit log better as above.
>>
>> So i understand its ok to continue with this way of checking ?
>> since we are always having a static association which never changes,
>> than introducing additional fields in the clk_branch which can
>> get the status of the gdsc.
>>
> 
> Well I'm also curious which case is failing. Does turning on the
> clocks work after the gdsc is enabled? Does turning off the
> clocks fail because we don't know when the gdsc has turned off? I
> would hope that the firmware keeps the gdsc on when it's done
> processing things, goes idle, and hands back control to software.
> Right now I'm failing to see how the halt bits fail to toggle
> assuming that firmware isn't misbehaving and the kernel driver is
> power controlling in a coordinated manner with the firmware.

What fails is turning ON the clocks after the gdsc is put under
hardware control (by fails I mean the halt checks fail to indicate
the clock is running, but register accesses etc thereafter suggest
the clocks are actually running)
The halt checks seem to work only while the gdsc is put in SW enabled
state.

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

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

['Stephen Boyd']

On 11/07, Rajendra Nayak wrote:
> 
> 
> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote:
> > Well I'm also curious which case is failing. Does turning on the
> > clocks work after the gdsc is enabled? Does turning off the
> > clocks fail because we don't know when the gdsc has turned off? I
> > would hope that the firmware keeps the gdsc on when it's done
> > processing things, goes idle, and hands back control to software.
> > Right now I'm failing to see how the halt bits fail to toggle
> > assuming that firmware isn't misbehaving and the kernel driver is
> > power controlling in a coordinated manner with the firmware.
> 
> What fails is turning ON the clocks after the gdsc is put under
> hardware control (by fails I mean the halt checks fail to indicate
> the clock is running, but register accesses etc thereafter suggest
> the clocks are actually running)
> The halt checks seem to work only while the gdsc is put in SW enabled
> state.
> 

Um... that is bad. I don't see how that is possible. It sounds
like the clocks are not turning on when we're asking for them to
turn on. The register accesses are always working because these
subcore clks aren't required for register accesses. Most likely
the GDSC for the subdomains is off (even after we thought we
enabled it).

Let's take a step back. The video hardware has three GDSCs. One
for the main logic, and two for each subdomain. We're adding hw
control for the two subdomains here. From what I can tell there
isn't any hw control for the main domain. I see that there are
two registers in the video hardware to control those subdomain hw
enable signals that go to the GDSC. The reset value is OFF (not
ON like was stated earlier), so it seems that if we switch the
subdomain GDSCs on in these patches it will turn on for a short
time, and then turn off when we switch into hw mode (by virtue of
the way we enable the GDSC). Presumably we can assert these hw
signal bits regardless of the subdomain power states, because
otherwise we're in a chicken-egg situation for the firmware
controlling this stuff.

The proper sequence sounds like it should be:
	
	1. Enable GDSC for main domain
	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
	3. Write the two registers to assert hw signal for subdomains
	4. Enable GDSCs for two subdomains
	5. Enable clocks for subdomains (video_subcore{0,1}_clk)

I can only guess that we're not doing this. Probably the sequence
right now is:

	1. Enable GDSC for main domain and both sub-domains
	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
	3. Enable clocks for subdomains (video_subcore{0,1}_clk)
	<clk stuff OFF because hw signal is still deasserted>

Sound right? If so, please fix the sequence in the video driver.

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

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Stephen,

>>
>> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote:
>> > Well I'm also curious which case is failing. Does turning on the
>> > clocks work after the gdsc is enabled? Does turning off the
>> > clocks fail because we don't know when the gdsc has turned off? I
>> > would hope that the firmware keeps the gdsc on when it's done
>> > processing things, goes idle, and hands back control to software.
>> > Right now I'm failing to see how the halt bits fail to toggle
>> > assuming that firmware isn't misbehaving and the kernel driver is
>> > power controlling in a coordinated manner with the firmware.
>>
>> What fails is turning ON the clocks after the gdsc is put under
>> hardware control (by fails I mean the halt checks fail to indicate
>> the clock is running, but register accesses etc thereafter suggest
>> the clocks are actually running)
>> The halt checks seem to work only while the gdsc is put in SW enabled
>> state.
>>
>
>Um... that is bad. I don't see how that is possible. It sounds
>like the clocks are not turning on when we're asking for them to
>turn on. The register accesses are always working because these
>subcore clks aren't required for register accesses. Most likely
>the GDSC for the subdomains is off (even after we thought we
>enabled it).
>
>Let's take a step back. The video hardware has three GDSCs. One
>for the main logic, and two for each subdomain. We're adding hw
>control for the two subdomains here. From what I can tell there
>isn't any hw control for the main domain. I see that there are
>two registers in the video hardware to control those subdomain hw
>enable signals that go to the GDSC. The reset value is OFF (not
>ON like was stated earlier), so it seems that if we switch the
>subdomain GDSCs on in these patches it will turn on for a short
>time, and then turn off when we switch into hw mode (by virtue of
>the way we enable the GDSC). Presumably we can assert these hw
>signal bits regardless of the subdomain power states, because
>otherwise we're in a chicken-egg situation for the firmware
>controlling this stuff.
>
>The proper sequence sounds like it should be:
>
>	1. Enable GDSC for main domain
>	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>	3. Write the two registers to assert hw signal for subdomains
>	4. Enable GDSCs for two subdomains
>	5. Enable clocks for subdomains (video_subcore{0,1}_clk)
>
>I can only guess that we're not doing this. Probably the sequence
>right now is:
>
>	1. Enable GDSC for main domain and both sub-domains
>	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>	3. Enable clocks for subdomains (video_subcore{0,1}_clk)
>	<clk stuff OFF because hw signal is still deasserted>
>
>Sound right? If so, please fix the sequence in the video driver.
>

So the above is the sequence which is actually carried out on the
firmware side. The same can be done in host as well.
The clocks stuck issue indeed is not there with this. But with the
above sequence we need to add a step to do inverse of STEP3
above (ie write the registers to de-assert hw_signal), to keep
the subdomains in off, till firmware uses it. So the above sequence
helps to avoid masking the halt check, although the host really
does not wants to use these clocks, except setting it up for the
firmware.

Regards,
 Sricharan

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Rajendra Nayak]

[]..

>>
>> The proper sequence sounds like it should be:
>>
>> 	1. Enable GDSC for main domain
>> 	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>> 	3. Write the two registers to assert hw signal for subdomains
>> 	4. Enable GDSCs for two subdomains
>> 	5. Enable clocks for subdomains (video_subcore{0,1}_clk)
>>
[]..

> 
> So the above is the sequence which is actually carried out on the
> firmware side. The same can be done in host as well.

By the 'above sequence is done on firmware side', I hope you don;t mean *all* 5 steps.
I guess you mean only step 3 is done by firmware?

> The clocks stuck issue indeed is not there with this. But with the
> above sequence we need to add a step to do inverse of STEP3
> above (ie write the registers to de-assert hw_signal), to keep
> the subdomains in off, till firmware uses it. So the above sequence
> helps to avoid masking the halt check, although the host really
> does not wants to use these clocks, except setting it up for the
> firmware.

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

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Rajendra,
>
>>>
>>> The proper sequence sounds like it should be:
>>>
>>> 	1. Enable GDSC for main domain
>>> 	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>>> 	3. Write the two registers to assert hw signal for subdomains
>>> 	4. Enable GDSCs for two subdomains
>>> 	5. Enable clocks for subdomains (video_subcore{0,1}_clk)
>>>
>[]..
>
>>
>> So the above is the sequence which is actually carried out on the
>> firmware side. The same can be done in host as well.
>
>By the 'above sequence is done on firmware side', I hope you don;t mean *all* 5 steps.
>I guess you mean only step 3 is done by firmware?
>

Yes, only step 3.

Regards,
 Sricharan

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

['Stephen Boyd']

On 11/09, Sricharan wrote:
> 
> So the above is the sequence which is actually carried out on the
> firmware side. The same can be done in host as well.
> The clocks stuck issue indeed is not there with this.

Great! We've finally connected on what the actual problem is.

> But with the above sequence we need to add a step to do inverse
> of STEP3 above (ie write the registers to de-assert hw_signal),
> to keep the subdomains in off, till firmware uses it. So the
> above sequence helps to avoid masking the halt check, although
> the host really does not wants to use these clocks, except
> setting it up for the firmware.
> 

Right, but knowing that the clocks failed to turn on in the first
place is much safer than silently ignoring the failure.
Otherwise, we could hand over control to the firmware, and the
firmware would fail to operate the hardware, and we're stuck with
debugging the firmware now. That sounds quite painful to figure
out.

If we properly toggle the video hw bits in coordination with
firmware and turn on/off the clocks with the GDSC ON, then
debugging is made simpler. The point is, we don't want to lose
robustness by silencing halt checks. The semantics of
clk_enable() means the clock is running, and that won't be true
here unless we ensure the GDSC is enabled.

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

RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Stephen,

>>
>> So the above is the sequence which is actually carried out on the
>> firmware side. The same can be done in host as well.
>> The clocks stuck issue indeed is not there with this.
>
>Great! We've finally connected on what the actual problem is.
>
>> But with the above sequence we need to add a step to do inverse
>> of STEP3 above (ie write the registers to de-assert hw_signal),
>> to keep the subdomains in off, till firmware uses it. So the
>> above sequence helps to avoid masking the halt check, although
>> the host really does not wants to use these clocks, except
>> setting it up for the firmware.
>>
>
>Right, but knowing that the clocks failed to turn on in the first
>place is much safer than silently ignoring the failure.
>Otherwise, we could hand over control to the firmware, and the
>firmware would fail to operate the hardware, and we're stuck with
>debugging the firmware now. That sounds quite painful to figure
>out.
>

Right, i already debugged this sort of a scenario which was quite
paintful sometime back :)

>If we properly toggle the video hw bits in coordination with
>firmware and turn on/off the clocks with the GDSC ON, then
>debugging is made simpler. The point is, we don't want to lose
>robustness by silencing halt checks. The semantics of
>clk_enable() means the clock is running, and that won't be true
>here unless we ensure the GDSC is enabled.
>

ok, which means with this approach, this patch can be dropped and
the other bits needs to be added to the video driver. I will follow that
up with Stanimir in his video driver patches.

Regards,
 Sricharan

Re: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Stanimir Varbanov]

Hi Stephen,

On 11/09/2016 12:33 AM, 'Stephen Boyd' wrote:
> On 11/07, Rajendra Nayak wrote:
>>
>>
>> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote:
>>> Well I'm also curious which case is failing. Does turning on the
>>> clocks work after the gdsc is enabled? Does turning off the
>>> clocks fail because we don't know when the gdsc has turned off? I
>>> would hope that the firmware keeps the gdsc on when it's done
>>> processing things, goes idle, and hands back control to software.
>>> Right now I'm failing to see how the halt bits fail to toggle
>>> assuming that firmware isn't misbehaving and the kernel driver is
>>> power controlling in a coordinated manner with the firmware.
>>
>> What fails is turning ON the clocks after the gdsc is put under
>> hardware control (by fails I mean the halt checks fail to indicate
>> the clock is running, but register accesses etc thereafter suggest
>> the clocks are actually running)
>> The halt checks seem to work only while the gdsc is put in SW enabled
>> state.
>>
> 
> Um... that is bad. I don't see how that is possible. It sounds
> like the clocks are not turning on when we're asking for them to
> turn on. The register accesses are always working because these
> subcore clks aren't required for register accesses. Most likely
> the GDSC for the subdomains is off (even after we thought we
> enabled it).
> 
> Let's take a step back. The video hardware has three GDSCs. One
> for the main logic, and two for each subdomain. We're adding hw
> control for the two subdomains here. From what I can tell there
> isn't any hw control for the main domain. I see that there are
> two registers in the video hardware to control those subdomain hw
> enable signals that go to the GDSC. The reset value is OFF (not
> ON like was stated earlier), so it seems that if we switch the
> subdomain GDSCs on in these patches it will turn on for a short
> time, and then turn off when we switch into hw mode (by virtue of
> the way we enable the GDSC). Presumably we can assert these hw
> signal bits regardless of the subdomain power states, because
> otherwise we're in a chicken-egg situation for the firmware
> controlling this stuff.
> 
> The proper sequence sounds like it should be:
> 	
> 	1. Enable GDSC for main domain
> 	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
> 	3. Write the two registers to assert hw signal for subdomains
> 	4. Enable GDSCs for two subdomains
> 	5. Enable clocks for subdomains (video_subcore{0,1}_clk)

Do you think this sequence 'enable GDSC and the clocks behind it' is
valid for MMAGIC GDSCs too?

1. Enable GDSC for MMAGIC_VIDEO
2. Enable clocks (not sure which they are) -
		msm8996_mmssnoc_axi_rpm_clk,
		mmss_mmagic_ahb_clk
		mmss_mmagic_cfg_ahb_clk,
		mmss_mmagic_maxi_clk,
		mmagic_video_axi_clk,
		smmu_video_ahb_clk,
		smmu_video_axi_clk
3. Continue with the sequence above.


-- 
regards,
Stan