[PATCH V2 0/2] clk: qcom: gdsc: Add support for gdscs with HW control

[PATCH V2 0/2] clk: qcom: gdsc: Add support for gdscs with HW control

[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.

V2:
 Dropped patch#3 [1] as it was concluded that the patch was effectively
 masking the fact the clocks were not getting turned on when the gdsc
 is put in hwctrl. With some change in sequence from venus core, masking
 is not needed and so patch needs to handled in venus driver.

 Fixed a comment in patch#1 to check the return value for gdsc_hwctrl

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1270922.html

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

Sricharan R (1):
  clk: qcom: Put venus core0/1 gdscs to hw control mode

 drivers/clk/qcom/gdsc.c         | 19 +++++++++++++++++++
 drivers/clk/qcom/gdsc.h         |  1 +
 drivers/clk/qcom/mmcc-msm8996.c |  2 ++
 3 files changed, 22 insertions(+)

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

[PATCH V2 1/2] 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>
---
[V2] Fixed to take care of the return value of gdsc_hwctrl

 drivers/clk/qcom/gdsc.c | 19 +++++++++++++++++++
 drivers/clk/qcom/gdsc.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index f12d7b2..57c7c1b 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,16 +171,28 @@ 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);
+
 	return 0;
 }
 
 static int gdsc_disable(struct generic_pm_domain *domain)
 {
 	struct gdsc *sc = domain_to_gdsc(domain);
+	int ret;
 
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_assert_reset(sc);
 
+	/* Turn off HW trigger mode if supported */
+	if (sc->flags & HW_CTRL) {
+		ret = gdsc_hwctrl(sc, false);
+		if (ret < 0)
+			return ret;
+	}
+
 	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 V2 2/2] 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

Re: [PATCH V2 0/2] clk: qcom: gdsc: Add support for gdscs with HW control

[Stanimir Varbanov]

Hi,

On 11/18/2016 02:28 PM, Sricharan R wrote:
> 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.
> 
> V2:
>  Dropped patch#3 [1] as it was concluded that the patch was effectively
>  masking the fact the clocks were not getting turned on when the gdsc
>  is put in hwctrl. With some change in sequence from venus core, masking
>  is not needed and so patch needs to handled in venus driver.

Which sequence should be changed in venus driver?

> 
>  Fixed a comment in patch#1 to check the return value for gdsc_hwctrl
> 
> [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1270922.html
> 
> Rajendra Nayak (1):
>   clk: qcom: gdsc: Add support for gdscs with HW control
> 
> Sricharan R (1):
>   clk: qcom: Put venus core0/1 gdscs to hw control mode
> 
>  drivers/clk/qcom/gdsc.c         | 19 +++++++++++++++++++
>  drivers/clk/qcom/gdsc.h         |  1 +
>  drivers/clk/qcom/mmcc-msm8996.c |  2 ++
>  3 files changed, 22 insertions(+)
> 

RE: [PATCH V2 0/2] clk: qcom: gdsc: Add support for gdscs with HW control

[Sricharan]

Hi Stan,

>Hi,
>
>On 11/18/2016 02:28 PM, Sricharan R wrote:
>> 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.
>>
>> V2:
>>  Dropped patch#3 [1] as it was concluded that the patch was effectively
>>  masking the fact the clocks were not getting turned on when the gdsc
>>  is put in hwctrl. With some change in sequence from venus core, masking
>>  is not needed and so patch needs to handled in venus driver.
>
>Which sequence should be changed in venus driver?
>
Ya wanted to discuss this with you on the venus thread, but let me put it here.

So while enabling the hw control bit for the venus subcores 0/1 gdscs and turning
on the subcore 0/1 clks, we saw that unless the
VENUS_WRAPPER_VENUS0_MMCC_VDEC_VCODEC_POWER_CONTROL
register is programmed to '0'(reset value is 1), the subcores domain/
clocks do not turn on. So this means that the,

1) venus driver should turn on all clocks except the subcore clocks.
2) Program VENUS_WRAPPER_VENUS0_MMCC_VDEC_VCODEC_POWER_CONTROL to
    '0' to turn on sub domains.
3) Turn on subcore clocks (cbc) and verify their running status using clk_enable
4) Program VENUS_WRAPPER_VENUS0_MMCC_VDEC_VCODEC_POWER_CONTROL to
     '1' again to turn off subdomain/clocks and let the firmware turn it on when required.

Note that in my previous patch set, i was skipping the check to verify the subcore clocks
'running status' previously, assuming that it can't be done while the gdsc is in hwctrl, but
that was not right.

Regards,
 Sricharan

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

[Stephen Boyd]

On 11/18, 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>
> ---

Applied to clk-next

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

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

[Stanimir Varbanov]

Hi Sricharan,

On 11/18/2016 02:28 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>
> ---
> [V2] Fixed to take care of the return value of gdsc_hwctrl
> 
>  drivers/clk/qcom/gdsc.c | 19 +++++++++++++++++++
>  drivers/clk/qcom/gdsc.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index f12d7b2..57c7c1b 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,16 +171,28 @@ 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);
> +
>  	return 0;
>  }
>  
>  static int gdsc_disable(struct generic_pm_domain *domain)
>  {
>  	struct gdsc *sc = domain_to_gdsc(domain);
> +	int ret;
>  
>  	if (sc->pwrsts == PWRSTS_ON)
>  		return gdsc_assert_reset(sc);
>  
> +	/* Turn off HW trigger mode if supported */
> +	if (sc->flags & HW_CTRL) {
> +		ret = gdsc_hwctrl(sc, false);

Looking in the downstream implementation the disabling of the hw control
bit shouldn't be enough.

After disabling hw control bit we must have a 1us delay and polling for
enabled PWR_ON bit with timeout of 100us, only then we should continue
with disabling the GDSC in software controlled mode.

-- 
regards,
Stan

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

[Rajendra Nayak]

[]..

>>  static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  {
>>  	int ret;
>> @@ -164,16 +171,28 @@ 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);
>> +
>>  	return 0;
>>  }
>>  
>>  static int gdsc_disable(struct generic_pm_domain *domain)
>>  {
>>  	struct gdsc *sc = domain_to_gdsc(domain);
>> +	int ret;
>>  
>>  	if (sc->pwrsts == PWRSTS_ON)
>>  		return gdsc_assert_reset(sc);
>>  
>> +	/* Turn off HW trigger mode if supported */
>> +	if (sc->flags & HW_CTRL) {
>> +		ret = gdsc_hwctrl(sc, false);
> 
> Looking in the downstream implementation the disabling of the hw control
> bit shouldn't be enough.
> 
> After disabling hw control bit we must have a 1us delay and polling for
> enabled PWR_ON bit with timeout of 100us, only then we should continue
> with disabling the GDSC in software controlled mode.

Stan, thats right. I will send a patch to fix this up right-away.

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