Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680917 > unrolled thread

[PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches

Back to article view | Back to linux.kernel

Contents

  [PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches Georgi Djakov <georgi.djakov@linaro.org> - 2017-07-04 14:40 +0200
    Re: [PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches Archit Taneja <architt@codeaurora.org> - 2017-07-06 12:00 +0200
      Re: [PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches Hans Verkuil <hverkuil@xs4all.nl> - 2017-07-06 12:20 +0200
    Re: [PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:20 +0200
    Re: [PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:20 +0200
      [PATCH v2] clk: qcom: clk-smd-rpm: Fix the reported rate of branches Georgi Djakov <georgi.djakov@linaro.org> - 2017-07-13 15:40 +0200
        Re: [PATCH v2] clk: qcom: clk-smd-rpm: Fix the reported rate of  branches Stephen Boyd <sboyd@codeaurora.org> - 2017-07-14 00:00 +0200
          Re: [PATCH v2] clk: qcom: clk-smd-rpm: Fix the reported rate of  branches Georgi Djakov <georgi.djakov@linaro.org> - 2017-07-14 15:10 +0200
          [PATCH v3] clk: qcom: clk-smd-rpm: Fix the reported rate of branches Georgi Djakov <georgi.djakov@linaro.org> - 2017-07-17 17:40 +0200
            Re: [PATCH v3] clk: qcom: clk-smd-rpm: Fix the reported rate of  branches Stephen Boyd <sboyd@codeaurora.org> - 2017-07-18 03:30 +0200

#1680917 — [PATCH] clk: qcom: clk-smd-rpm: Fix the initial rate of branches

As there is no way to actually query the hardware for the current clock
rate, now racalc_rate() just returns the last rate that was previously
set. But if the rate was not set yet, we return the bogus rate of 1KHz.

Knowing what the rate of XO is and that some clocks are just branches of
it, we can do better and return that rate instead of a bogus one.

Reported-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index d990fe44aef3..7350a43b0573 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -116,12 +116,12 @@
 
 #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
 		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
-		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
+		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
 		QCOM_RPM_KEY_SOFTWARE_ENABLE)
 
 #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
 		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
-		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
+		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
 		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
 
 #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)

[toc] | [next] | [standalone]

#1682282

On 07/04/2017 06:04 PM, Georgi Djakov wrote:
> As there is no way to actually query the hardware for the current clock
> rate, now racalc_rate() just returns the last rate that was previously
> set. But if the rate was not set yet, we return the bogus rate of 1KHz.
> 
> Knowing what the rate of XO is and that some clocks are just branches of
> it, we can do better and return that rate instead of a bogus one.

+Hans.

Thanks for the fix.

Reviewed-by: Archit Taneja <architt@codeaurora.org>
> 
> Reported-by: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>   drivers/clk/qcom/clk-smd-rpm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index d990fe44aef3..7350a43b0573 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -116,12 +116,12 @@
>   
>   #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
>   		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
> -		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> +		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
>   		QCOM_RPM_KEY_SOFTWARE_ENABLE)
>   
>   #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
>   		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
> -		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> +		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
>   		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
>   
>   #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[toc] | [prev] | [next] | [standalone]

#1682312

On 07/06/17 11:53, Archit Taneja wrote:
> 
> 
> On 07/04/2017 06:04 PM, Georgi Djakov wrote:
>> As there is no way to actually query the hardware for the current clock
>> rate, now racalc_rate() just returns the last rate that was previously
>> set. But if the rate was not set yet, we return the bogus rate of 1KHz.
>>
>> Knowing what the rate of XO is and that some clocks are just branches of
>> it, we can do better and return that rate instead of a bogus one.
> 
> +Hans.
> 
> Thanks for the fix.
> 
> Reviewed-by: Archit Taneja <architt@codeaurora.org>

Tested-by: Hans Verkuil <hans.verkuil@cisco.com>

(Tested with the upcoming HDMI CEC driver for the adv7511/33)

Regards,

	Hans

>>
>> Reported-by: Archit Taneja <architt@codeaurora.org>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>   drivers/clk/qcom/clk-smd-rpm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index d990fe44aef3..7350a43b0573 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -116,12 +116,12 @@
>>   
>>   #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
>>   		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
>> -		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
>> +		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
>>   		QCOM_RPM_KEY_SOFTWARE_ENABLE)
>>   
>>   #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
>>   		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
>> -		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
>> +		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
>>   		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
>>   
>>   #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

[toc] | [prev] | [next] | [standalone]

#1686113

On 07/04, Georgi Djakov wrote:
> As there is no way to actually query the hardware for the current clock
> rate, now racalc_rate() just returns the last rate that was previously
> set. But if the rate was not set yet, we return the bogus rate of 1KHz.
> 
> Knowing what the rate of XO is and that some clocks are just branches of
> it, we can do better and return that rate instead of a bogus one.
> 
> Reported-by: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---

Applied to clk-next, any Fixes: tag to attach here?

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

[toc] | [prev] | [next] | [standalone]

#1686115

On 07/04, Georgi Djakov wrote:
> As there is no way to actually query the hardware for the current clock
> rate, now racalc_rate() just returns the last rate that was previously
> set. But if the rate was not set yet, we return the bogus rate of 1KHz.
> 
> Knowing what the rate of XO is and that some clocks are just branches of
> it, we can do better and return that rate instead of a bogus one.
> 
> Reported-by: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index d990fe44aef3..7350a43b0573 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -116,12 +116,12 @@
>  
>  #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
>  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
> -		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> +		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
>  		QCOM_RPM_KEY_SOFTWARE_ENABLE)
>  
>  #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
>  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
> -		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> +		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 19200000,		      \
>  		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
>  
>  #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)

Also, it would be better if these were children of the xo_board
clk. That way we don't have to specify a rate at all, just take
the rate from the parent (xo_board) and then things look correct
regardless of the board configuration.

So I'll drop this patch from clk-next for now.

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

[toc] | [prev] | [next] | [standalone]

#1686535 — [PATCH v2] clk: qcom: clk-smd-rpm: Fix the reported rate of branches

As there is no way to actually query the hardware for the current clock
rate, now racalc_rate() just returns the last rate that was previously
set. But if the rate was not set yet, we return the bogus rate of 1000Hz.

The branch clocks actually have the same rate as their parent (xo_board),
so just return this rate.

Reported-by: Archit Taneja <architt@codeaurora.org>
Fixes: 00f64b58874e ("clk: qcom: Add support for SMD-RPM Clocks")
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index d990fe44aef3..b45782657ca9 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -364,6 +364,10 @@ static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
 {
 	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
 
+	/* Return the parent rate for branches */
+	if (r->branch)
+		return parent_rate;
+
 	/*
 	 * RPM handles rate rounding and we don't have a way to
 	 * know what the rate will be, so just return whatever

[toc] | [prev] | [next] | [standalone]

#1686952 — Re: [PATCH v2] clk: qcom: clk-smd-rpm: Fix the reported rate of branches

On 07/13, Georgi Djakov wrote:
> As there is no way to actually query the hardware for the current clock
> rate, now racalc_rate() just returns the last rate that was previously
> set. But if the rate was not set yet, we return the bogus rate of 1000Hz.
> 
> The branch clocks actually have the same rate as their parent (xo_board),
> so just return this rate.
> 
> Reported-by: Archit Taneja <architt@codeaurora.org>
> Fixes: 00f64b58874e ("clk: qcom: Add support for SMD-RPM Clocks")
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index d990fe44aef3..b45782657ca9 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -364,6 +364,10 @@ static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
>  {
>  	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>  
> +	/* Return the parent rate for branches */
> +	if (r->branch)
> +		return parent_rate;
> +

What's parent_rate here though? 0? I don't see where we parent
the branch clks to anything.

And we should really just remove the recalc_rate() op for
branches entirely so that we don't have to call down into the
driver to find out something we could have known in the core.

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

[toc] | [prev] | [next] | [standalone]

#1687408 — Re: [PATCH v2] clk: qcom: clk-smd-rpm: Fix the reported rate of branches

On 07/14/2017 12:56 AM, Stephen Boyd wrote:
> On 07/13, Georgi Djakov wrote:
>> As there is no way to actually query the hardware for the current clock
>> rate, now racalc_rate() just returns the last rate that was previously
>> set. But if the rate was not set yet, we return the bogus rate of 1000Hz.
>>
>> The branch clocks actually have the same rate as their parent (xo_board),
>> so just return this rate.
>>
>> Reported-by: Archit Taneja <architt@codeaurora.org>
>> Fixes: 00f64b58874e ("clk: qcom: Add support for SMD-RPM Clocks")
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>  drivers/clk/qcom/clk-smd-rpm.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index d990fe44aef3..b45782657ca9 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -364,6 +364,10 @@ static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
>>  {
>>  	struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>>  
>> +	/* Return the parent rate for branches */
>> +	if (r->branch)
>> +		return parent_rate;
>> +
> 
> What's parent_rate here though? 0? I don't see where we parent
> the branch clks to anything.

Its 19.2MHz as xo_board is a fixed-clock in DT. We use the
__DEFINE_CLK_SMD_RPM_BRANCH macro to define all the branch clks
and this macro sets their parent name to "xo_board". Sorry for
not explaining this earlier.

> And we should really just remove the recalc_rate() op for
> branches entirely so that we don't have to call down into the
> driver to find out something we could have known in the core.

Yes this would be better. I agree. Will remove recalc/round_rate
ops for the branch clks.

Thanks,
Georgi

[toc] | [prev] | [next] | [standalone]

#1689181 — [PATCH v3] clk: qcom: clk-smd-rpm: Fix the reported rate of branches

As there is no way to actually query the hardware for the current clock
rate, now racalc_rate() just returns the last rate that was previously
set. But if the rate was not set yet, we return the bogus rate of 1000Hz.

The branch clocks have the same rate as their parent, so in this case we
just need to remove recalc_rate ops and then the core framework will handle
this automagically. The round_rate() is unused, so remove it as well.

Reported-by: Archit Taneja <architt@codeaurora.org>
Fixes: 00f64b58874e ("clk: qcom: Add support for SMD-RPM Clocks")
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index d990fe44aef3..cc03d5508627 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -412,8 +412,6 @@ static const struct clk_ops clk_smd_rpm_ops = {
 static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
-	.round_rate	= clk_smd_rpm_round_rate,
-	.recalc_rate	= clk_smd_rpm_recalc_rate,
 };
 
 /* msm8916 */

[toc] | [prev] | [next] | [standalone]

#1689596 — Re: [PATCH v3] clk: qcom: clk-smd-rpm: Fix the reported rate of branches

On 07/17, Georgi Djakov wrote:
> As there is no way to actually query the hardware for the current clock
> rate, now racalc_rate() just returns the last rate that was previously
> set. But if the rate was not set yet, we return the bogus rate of 1000Hz.
> 
> The branch clocks have the same rate as their parent, so in this case we
> just need to remove recalc_rate ops and then the core framework will handle
> this automagically. The round_rate() is unused, so remove it as well.
> 
> Reported-by: Archit Taneja <architt@codeaurora.org>
> Fixes: 00f64b58874e ("clk: qcom: Add support for SMD-RPM Clocks")
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---

Applied to clk-next

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web