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

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

[Georgi Djakov]

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)

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

[Archit Taneja]

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

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

[Hans Verkuil]

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

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

[Stephen Boyd]

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

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

[Stephen Boyd]

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

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

[Georgi Djakov]

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

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

[Stephen Boyd]

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

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

[Georgi Djakov]

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

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

[Georgi Djakov]

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 */

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

[Stephen Boyd]

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