lkml.org 
[lkml]   [2018]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH] clk: qcom: rpmcc: Add support to XO buffered clocks
From
Date
Thanks for the review comments,

On 15/03/18 17:52, Stephen Boyd wrote:
> Quoting srinivas.kandagatla@linaro.org (2018-03-15 07:37:24)
>> diff --git a/drivers/clk/qcom/clk-rpm.c b/drivers/clk/qcom/clk-rpm.c
>> index c60f61b10c7f..261f5505e714 100644
>> --- a/drivers/clk/qcom/clk-rpm.c
>> +++ b/drivers/clk/qcom/clk-rpm.c
>> @@ -56,6 +57,19 @@
>> }, \
>> }
>>
>> +#define DEFINE_CLK_RPM_XO_BUFFER(_platform, _name, _active, offset) \
>> + static struct clk_rpm _platform##_##_name = { \
>> + .rpm_clk_id = QCOM_RPM_CXO_BUFFERS, \
>> + .xo_offset = (offset), \
>> + .rate = 19200000, \
>
> Shouldn't these rates also come from parent, i.e. pxo_board? So drop
> this line?
Yep, will remove this.
>
>> + .hw.init = &(struct clk_init_data){ \
>> + .ops = &clk_rpm_fixed_ops, \
>> + .name = #_name, \
>> + .parent_names = (const char *[]){ "pxo_board" }, \
>> + .num_parents = 1, \
>> + }, \
>> + }
>> +
>> #define DEFINE_CLK_RPM_FIXED(_platform, _name, _active, r_id, r) \
>> static struct clk_rpm _platform##_##_name = { \
>> .rpm_clk_id = (r_id), \
>> @@ -128,6 +142,8 @@
>>
>> struct clk_rpm {
>> const int rpm_clk_id;
>> + const int xo_offset;
>> + u32 xo_buffer_value;
>> const bool active_only;
>> unsigned long rate;
>> bool enabled;
>> @@ -308,6 +330,11 @@ static void clk_rpm_fixed_unprepare(struct clk_hw *hw)
>> u32 value = 0;
>> int ret;
>>
>> + if (r->rpm_clk_id == QCOM_RPM_CXO_BUFFERS) {
>> + r->xo_buffer_value &= ~(QCOM_RPM_XO_MODE_ON << r->xo_offset);
>> + value = r->xo_buffer_value;
>> + }
>> +
>
> I seem to recall that the xo buffers are within the same "word" or
> something like that for each of the buffers. So we would need to make
> sure we don't overwrite the bits in there between clks that are
Code as it is will not work.
> enabling/disabling stuff. Maybe make some sort of shared variable they
> all point to and then put a mutex around it? May also be worth making
> more clk_ops at that point to not conflate with the simpler on/off code.
Yep, I will give something like that a try in next version.


-srini

>

\
 
 \ /
  Last update: 2018-03-15 19:05    [W:0.052 / U:0.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site