Re: [PATCH v4 1/2] clk: qcom: Add MSM8998 GPU Clock Controller (GPUCC) driver

[Jeffrey Hugo]

On Thu, Oct 17, 2019 at 5:16 PM Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx> wrote:
>
> On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> >
> > Quoting Jeffrey Hugo (2019-10-01 18:16:40)
> > > +static const struct freq_tbl ftbl_gfx3d_clk_src[] = {
> > > +       F(180000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(257000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(342000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(414000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(515000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(596000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(670000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       F(710000000, P_GPUPLL0_OUT_EVEN, 2, 0, 0),
> > > +       { }
> >
> > I guess this one doesn't do PLL ping pong? Instead we just reprogram the
> > PLL all the time? Can we have rcg2 clk ops that set the rate on the
> > parent to be exactly twice as much as the incoming frequency? I thought
> > we already had this support in the code. Indeed, it is part of
> > _freq_tbl_determine_rate() in clk-rcg.c, but not yet implemented in the
> > same function name in clk-rcg2.c! Can you implement it? That way we
> > don't need this long frequency table, just this weird one where it looks
> > like:
> >
> >         { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }
> >         { }
> >
> > And then some more logic in the rcg2 ops to allow this possibility for a
> > frequency table when CLK_SET_RATE_PARENT is set.
>
> Does not do PLL ping pong.  I'll look at extending the rcg2 ops like
> you describe.

Am I missing something?  From what I can tell, what you describe is
not implemented.

The only in-tree example of a freq_tbl with only a src and a pre_div
defined for rcg ops is for the tv_src clk in mmcc-msm8960 [1]
However, that uses a variant of rcg ops, clk_rcg_bypass_ops, not clk_rcg_ops.

clk_rcg_bypass_ops has its own determine_rate implementation which
does not utilize _freq_tbl_determine_rate(), and can only do a 1:1
input rate to output ratio (we want a 1:2).

_freq_tbl_determine_rate() in either rcg_ops or rcg2_ops won't work,
because they both use qcom_find_freq() which doesn't work when your
table doesn't specify any frequencies (f->freq is 0).
qcom_find_freq() won't iterate through the table, therefore f in
qcom_find_freq() won't be pointing at the end of the table (the null
entry), so when qcom_find_freq decrements f in the return, it actually
goes off the beginning of the array in an array out of bounds
violation.

Please advise how you would like to proceed.

I can still extend rcg2_ops to do what you want, but it won't be based
on what rcg_ops is doing.

I can spin a rcg2_ops variant to do what you want, with a custom
determine_rate, but it doesn't seem like I'll really be saving any
lines of code.  Whatever I eliminate by minimizing the gfx3d
freq_table I will surely be putting into clk-rcg2.c

Or, I can just drop this idea and keep the freq_tbl as it is.  Seems
like just a one off scenario.

[1] - https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/clk/qcom/mmcc-msm8960.c#L1416