diff mbox series

[RFT,v2,02/14] clk: qcom: smd-rpm: Add .is_enabled hook

Message ID 20230303-topic-rpmcc_sleep-v2-2-ae80a325fe94@linaro.org
State New
Headers
Series SMD RPMCC sleep preparations |

Commit Message

Konrad Dybcio March 8, 2023, 9:35 p.m. UTC 
  From: Shawn Guo <shawn.guo@linaro.org>

The RPM clock enabling state can be found with 'enabled' in struct
clk_smd_rpm.  Add .is_enabled hook so that clk_summary in debugfs can
show a correct enabling state for RPM clocks.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
[Konrad: rebase]
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Dmitry Baryshkov March 9, 2023, 12:47 a.m. UTC | #1 
On 08/03/2023 23:35, Konrad Dybcio wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The RPM clock enabling state can be found with 'enabled' in struct
> clk_smd_rpm.  Add .is_enabled hook so that clk_summary in debugfs can
> show a correct enabling state for RPM clocks.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> [Konrad: rebase]
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Bjorn Andersson March 22, 2023, 3:02 a.m. UTC | #2 
On Wed, Mar 08, 2023 at 10:35:18PM +0100, Konrad Dybcio wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The RPM clock enabling state can be found with 'enabled' in struct
> clk_smd_rpm.  Add .is_enabled hook so that clk_summary in debugfs can
> show a correct enabling state for RPM clocks.
> 

I don't think .is_enabled should be implemented for clocks where the
actual state can't be queried.

E.g. should a clock which is is_enabled = false be unprepared during
disable_unused? It's already disabled...

Regards,
Bjorn

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> [Konrad: rebase]
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 198886c1b6c8..ecacfbc4a16c 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -424,18 +424,27 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
>  	return 0;
>  }
>  
> +static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> +
> +	return r->enabled;
> +}
> +
>  static const struct clk_ops clk_smd_rpm_ops = {
>  	.prepare	= clk_smd_rpm_prepare,
>  	.unprepare	= clk_smd_rpm_unprepare,
>  	.set_rate	= clk_smd_rpm_set_rate,
>  	.round_rate	= clk_smd_rpm_round_rate,
>  	.recalc_rate	= clk_smd_rpm_recalc_rate,
> +	.is_enabled	= clk_smd_rpm_is_enabled,
>  };
>  
>  static const struct clk_ops clk_smd_rpm_branch_ops = {
>  	.prepare	= clk_smd_rpm_prepare,
>  	.unprepare	= clk_smd_rpm_unprepare,
>  	.recalc_rate	= clk_smd_rpm_recalc_rate,
> +	.is_enabled	= clk_smd_rpm_is_enabled,
>  };
>  
>  DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
> 
> -- 
> 2.39.2
>
Konrad Dybcio April 6, 2023, 2:43 p.m. UTC | #3 
On 22.03.2023 04:02, Bjorn Andersson wrote:
> On Wed, Mar 08, 2023 at 10:35:18PM +0100, Konrad Dybcio wrote:
>> From: Shawn Guo <shawn.guo@linaro.org>
>>
>> The RPM clock enabling state can be found with 'enabled' in struct
>> clk_smd_rpm.  Add .is_enabled hook so that clk_summary in debugfs can
>> show a correct enabling state for RPM clocks.
>>
> 
> I don't think .is_enabled should be implemented for clocks where the
> actual state can't be queried.
> 
> E.g. should a clock which is is_enabled = false be unprepared during
> disable_unused? It's already disabled...
That's true, it sounds silly.

However, I feel like it's the least painful option, as trying to disable
a clock that's already actually disabled (read, in hw+RPM, not Linux)
will not do any harm.

Not adding this (and by extension not making use of any sort of unused
clk cleanup) will prevent the system from hitting low power modes and
SMD RPM is strictly speaking, too dumb to figure out that these clocks
aren't really consumed.

Konrad
> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> [Konrad: rebase]
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index 198886c1b6c8..ecacfbc4a16c 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -424,18 +424,27 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
>>  	return 0;
>>  }
>>  
>> +static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
>> +{
>> +	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>> +
>> +	return r->enabled;
>> +}
>> +
>>  static const struct clk_ops clk_smd_rpm_ops = {
>>  	.prepare	= clk_smd_rpm_prepare,
>>  	.unprepare	= clk_smd_rpm_unprepare,
>>  	.set_rate	= clk_smd_rpm_set_rate,
>>  	.round_rate	= clk_smd_rpm_round_rate,
>>  	.recalc_rate	= clk_smd_rpm_recalc_rate,
>> +	.is_enabled	= clk_smd_rpm_is_enabled,
>>  };
>>  
>>  static const struct clk_ops clk_smd_rpm_branch_ops = {
>>  	.prepare	= clk_smd_rpm_prepare,
>>  	.unprepare	= clk_smd_rpm_unprepare,
>>  	.recalc_rate	= clk_smd_rpm_recalc_rate,
>> +	.is_enabled	= clk_smd_rpm_is_enabled,
>>  };
>>  
>>  DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
>>
>> -- 
>> 2.39.2
>>
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 198886c1b6c8..ecacfbc4a16c 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -424,18 +424,27 @@  static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
 	return 0;
 }
 
+static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
+{
+	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+	return r->enabled;
+}
+
 static const struct clk_ops clk_smd_rpm_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
 	.set_rate	= clk_smd_rpm_set_rate,
 	.round_rate	= clk_smd_rpm_round_rate,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
+	.is_enabled	= clk_smd_rpm_is_enabled,
 };
 
 static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
 	.recalc_rate	= clk_smd_rpm_recalc_rate,
+	.is_enabled	= clk_smd_rpm_is_enabled,
 };
 
 DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);