diff mbox series

[RFT,v2,09/14] clk: qcom: smd-rpm: Add support for keepalive votes

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

Commit Message

Konrad Dybcio March 8, 2023, 9:35 p.m. UTC 
  Some bus clock should always have a minimum (19.2 MHz) vote cast on
them, otherwise the platform will fall apart, hang and reboot.

Add support for specifying which clocks should be kept alive and
always keep a vote on XO_A to make sure the clock tree doesn't
collapse. This removes the need to keep a maximum vote that was
previously guaranteed by clk_smd_rpm_handoff.

This commit is a combination of existing (not-exactly-upstream) work
by Taniya Das, Shawn Guo and myself.

Co-developed-by: Shawn Guo <shawn.guo@linaro.org>
Co-developed-by: Taniya Das <quic_tdas@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Dmitry Baryshkov March 9, 2023, 12:54 a.m. UTC | #1 
On 08/03/2023 23:35, Konrad Dybcio wrote:
> Some bus clock should always have a minimum (19.2 MHz) vote cast on
> them, otherwise the platform will fall apart, hang and reboot.
> 
> Add support for specifying which clocks should be kept alive and
> always keep a vote on XO_A to make sure the clock tree doesn't
> collapse. This removes the need to keep a maximum vote that was
> previously guaranteed by clk_smd_rpm_handoff.
> 
> This commit is a combination of existing (not-exactly-upstream) work
> by Taniya Das, Shawn Guo and myself.
> 
> Co-developed-by: Shawn Guo <shawn.guo@linaro.org>
> Co-developed-by: Taniya Das <quic_tdas@quicinc.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++--
>   1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index eb7781e5c8c1..d89918f9ae60 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -45,15 +45,17 @@
>   		},							      \
>   	};								      \
>   	__DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type,	      \
> -				       r_id, key, ao_flags)
> +				       r_id, key, ao_flags, false)
>   
>   #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active,		      \
> -				       type, r_id, key, ao_flags)	      \
> +				       type, r_id, key, ao_flags,	      \
> +				       _keep_alive)			      \
>   	static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = {	      \
>   		.rpm_res_type = (type),					      \
>   		.rpm_clk_id = (r_id),					      \
>   		.active_only = true,					      \
>   		.rpm_key = (key),					      \
> +		.keep_alive = (_keep_alive),				      \
>   		.peer = &clk_smd_rpm_##_prefix##_name,			      \
>   		.rate = INT_MAX,					      \
>   		.hw.init = &(struct clk_init_data){			      \
> @@ -170,6 +172,7 @@ struct clk_smd_rpm {
>   	const bool active_only;
>   	bool enabled;
>   	bool branch;
> +	bool keep_alive;
>   	struct clk_smd_rpm *peer;
>   	struct clk_hw hw;
>   	unsigned long rate;
> @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
>   		.value = cpu_to_le32(r->branch ? 1 : INT_MAX),
>   	};
>   
> +	/* Set up keepalive clocks with a minimum bus rate */
> +	if (r->keep_alive)
> +		req.value = cpu_to_le32(19200); /* 19.2 MHz */


Should this be set to cpu_to_le32(max(19200, ...)) ?

> +
>   	ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
>   				 r->rpm_res_type, r->rpm_clk_id, &req,
>   				 sizeof(req));
>   	if (ret)
>   		return ret;
> +
>   	ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE,
>   				 r->rpm_res_type, r->rpm_clk_id, &req,
>   				 sizeof(req));
> @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
>   	return r->enabled;
>   }
>   
> +static int clk_smd_rpm_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> +
> +	/*
> +	 * RPM resolves the rates internally. All we have to do on the kernel
> +	 * side is ensure that we don't accidentally put down the keepalive
> +	 * clocks, which could happen if they received a vote below 19.2 MHz.
> +	 */
> +	if (r->keep_alive)
> +		req->rate = max(req->rate, 19200000UL);
> +
> +	return 0;
> +}
> +
>   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,
> +	.determine_rate = clk_smd_rpm_determine_rate,
>   	.is_enabled	= clk_smd_rpm_is_enabled,
>   	.is_prepared	= clk_smd_rpm_is_enabled,
>   };
>
Konrad Dybcio March 9, 2023, 1:22 a.m. UTC | #2 
On 9.03.2023 01:54, Dmitry Baryshkov wrote:
> On 08/03/2023 23:35, Konrad Dybcio wrote:
>> Some bus clock should always have a minimum (19.2 MHz) vote cast on
>> them, otherwise the platform will fall apart, hang and reboot.
>>
>> Add support for specifying which clocks should be kept alive and
>> always keep a vote on XO_A to make sure the clock tree doesn't
>> collapse. This removes the need to keep a maximum vote that was
>> previously guaranteed by clk_smd_rpm_handoff.
>>
>> This commit is a combination of existing (not-exactly-upstream) work
>> by Taniya Das, Shawn Guo and myself.
>>
>> Co-developed-by: Shawn Guo <shawn.guo@linaro.org>
>> Co-developed-by: Taniya Das <quic_tdas@quicinc.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index eb7781e5c8c1..d89918f9ae60 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -45,15 +45,17 @@
>>           },                                  \
>>       };                                      \
>>       __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type,          \
>> -                       r_id, key, ao_flags)
>> +                       r_id, key, ao_flags, false)
>>     #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active,              \
>> -                       type, r_id, key, ao_flags)          \
>> +                       type, r_id, key, ao_flags,          \
>> +                       _keep_alive)                  \
>>       static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = {          \
>>           .rpm_res_type = (type),                          \
>>           .rpm_clk_id = (r_id),                          \
>>           .active_only = true,                          \
>>           .rpm_key = (key),                          \
>> +        .keep_alive = (_keep_alive),                      \
>>           .peer = &clk_smd_rpm_##_prefix##_name,                  \
>>           .rate = INT_MAX,                          \
>>           .hw.init = &(struct clk_init_data){                  \
>> @@ -170,6 +172,7 @@ struct clk_smd_rpm {
>>       const bool active_only;
>>       bool enabled;
>>       bool branch;
>> +    bool keep_alive;
>>       struct clk_smd_rpm *peer;
>>       struct clk_hw hw;
>>       unsigned long rate;
>> @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
>>           .value = cpu_to_le32(r->branch ? 1 : INT_MAX),
>>       };
>>   +    /* Set up keepalive clocks with a minimum bus rate */
>> +    if (r->keep_alive)
>> +        req.value = cpu_to_le32(19200); /* 19.2 MHz */
> 
> 
> Should this be set to cpu_to_le32(max(19200, ...)) ?
I was debating this. Downstream explicitly sets 19.2 Mhz here and
the only regression I can think of is that we'd throttle a bus that
was left on by the bootloader and is (ab)used by us..

But then, it's only an active vote, and we're voting INT_MAX on the
non-active-only one, so that's a non-issue.

So I think 19.2 here is okay as the bare minimum, whatever stupidity
the eventual interconnect driver may entail..

Konrad
> 
>> +
>>       ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
>>                    r->rpm_res_type, r->rpm_clk_id, &req,
>>                    sizeof(req));
>>       if (ret)
>>           return ret;
>> +
>>       ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE,
>>                    r->rpm_res_type, r->rpm_clk_id, &req,
>>                    sizeof(req));
>> @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
>>       return r->enabled;
>>   }
>>   +static int clk_smd_rpm_determine_rate(struct clk_hw *hw,
>> +                      struct clk_rate_request *req)
>> +{
>> +    struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>> +
>> +    /*
>> +     * RPM resolves the rates internally. All we have to do on the kernel
>> +     * side is ensure that we don't accidentally put down the keepalive
>> +     * clocks, which could happen if they received a vote below 19.2 MHz.
>> +     */
>> +    if (r->keep_alive)
>> +        req->rate = max(req->rate, 19200000UL);
>> +
>> +    return 0;
>> +}
>> +
>>   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,
>> +    .determine_rate = clk_smd_rpm_determine_rate,
>>       .is_enabled    = clk_smd_rpm_is_enabled,
>>       .is_prepared    = clk_smd_rpm_is_enabled,
>>   };
>>
>
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index eb7781e5c8c1..d89918f9ae60 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -45,15 +45,17 @@ 
 		},							      \
 	};								      \
 	__DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type,	      \
-				       r_id, key, ao_flags)
+				       r_id, key, ao_flags, false)
 
 #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active,		      \
-				       type, r_id, key, ao_flags)	      \
+				       type, r_id, key, ao_flags,	      \
+				       _keep_alive)			      \
 	static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = {	      \
 		.rpm_res_type = (type),					      \
 		.rpm_clk_id = (r_id),					      \
 		.active_only = true,					      \
 		.rpm_key = (key),					      \
+		.keep_alive = (_keep_alive),				      \
 		.peer = &clk_smd_rpm_##_prefix##_name,			      \
 		.rate = INT_MAX,					      \
 		.hw.init = &(struct clk_init_data){			      \
@@ -170,6 +172,7 @@  struct clk_smd_rpm {
 	const bool active_only;
 	bool enabled;
 	bool branch;
+	bool keep_alive;
 	struct clk_smd_rpm *peer;
 	struct clk_hw hw;
 	unsigned long rate;
@@ -198,11 +201,16 @@  static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
 		.value = cpu_to_le32(r->branch ? 1 : INT_MAX),
 	};
 
+	/* Set up keepalive clocks with a minimum bus rate */
+	if (r->keep_alive)
+		req.value = cpu_to_le32(19200); /* 19.2 MHz */
+
 	ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
 				 r->rpm_res_type, r->rpm_clk_id, &req,
 				 sizeof(req));
 	if (ret)
 		return ret;
+
 	ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE,
 				 r->rpm_res_type, r->rpm_clk_id, &req,
 				 sizeof(req));
@@ -438,12 +446,29 @@  static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
 	return r->enabled;
 }
 
+static int clk_smd_rpm_determine_rate(struct clk_hw *hw,
+				      struct clk_rate_request *req)
+{
+	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+	/*
+	 * RPM resolves the rates internally. All we have to do on the kernel
+	 * side is ensure that we don't accidentally put down the keepalive
+	 * clocks, which could happen if they received a vote below 19.2 MHz.
+	 */
+	if (r->keep_alive)
+		req->rate = max(req->rate, 19200000UL);
+
+	return 0;
+}
+
 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,
+	.determine_rate = clk_smd_rpm_determine_rate,
 	.is_enabled	= clk_smd_rpm_is_enabled,
 	.is_prepared	= clk_smd_rpm_is_enabled,
 };