Re: [RFC PATCH] clk: qcom: rpmcc: Add support to XO buffered clocks

[Srinivas Kandagatla]

Thanks for the review comments,

On 15/03/18 17:52, Stephen Boyd wrote:
> Quoting srinivas.kandagatla@xxxxxxxxxx (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

>