On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Jeffrey Hugo (2019-10-01 18:16:40)
> > diff --git a/drivers/clk/qcom/gpucc-msm8998.c b/drivers/clk/qcom/gpucc-msm8998.c
> > new file mode 100644
> > index 000000000000..f0ccb4963885
> > --- /dev/null
> > +++ b/drivers/clk/qcom/gpucc-msm8998.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019, Jeffrey Hugo
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/err.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/clk.h>
>
> Drop this include please.
Will do.
>
> > +
> > +
> > +static struct clk_rcg2 rbcpr_clk_src = {
> > + .cmd_rcgr = 0x1030,
> > + .hid_width = 5,
> > + .parent_map = gpu_xo_gpll0_map,
> > + .freq_tbl = ftbl_rbcpr_clk_src,
> > + .clkr.hw.init = &(struct clk_init_data){
> > + .name = "rbcpr_clk_src",
> > + .parent_data = gpu_xo_gpll0,
> > + .num_parents = 2,
> > + .ops = &clk_rcg2_ops,
> > + },
> > +};
> > +
> > +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.
>
> > +};
> > +
> > +static struct clk_rcg2 gfx3d_clk_src = {
> > + .cmd_rcgr = 0x1070,
> > + .hid_width = 5,
> > + .parent_map = gpu_xo_gpupll0_map,
> > + .freq_tbl = ftbl_gfx3d_clk_src,
> > + .clkr.hw.init = &(struct clk_init_data){
> > + .name = "gfx3d_clk_src",
> > + .parent_data = gpu_xo_gpupll0,
> > + .num_parents = 2,
> > + .ops = &clk_rcg2_ops,
> > + .flags = CLK_OPS_PARENT_ENABLE,
>
> Needs CLK_SET_RATE_PARENT presumably?
Ah yeah. Thanks for catching.
>
> > + },
> > +};
> > +
> > +static const struct freq_tbl ftbl_rbbmtimer_clk_src[] = {
> > + F(19200000, P_XO, 1, 0, 0),
> > + { }
> > +};
> > +
> [...]
> > +
> > +static const struct qcom_cc_desc gpucc_msm8998_desc = {
> > + .config = &gpucc_msm8998_regmap_config,
> > + .clks = gpucc_msm8998_clocks,
> > + .num_clks = ARRAY_SIZE(gpucc_msm8998_clocks),
> > + .resets = gpucc_msm8998_resets,
> > + .num_resets = ARRAY_SIZE(gpucc_msm8998_resets),
> > + .gdscs = gpucc_msm8998_gdscs,
> > + .num_gdscs = ARRAY_SIZE(gpucc_msm8998_gdscs),
> > +};
> > +
> > +static const struct of_device_id gpucc_msm8998_match_table[] = {
> > + { .compatible = "qcom,gpucc-msm8998" },
>
> The compatible is different. In the merged binding it is
> qcom,msm8998-gpucc. Either fix this or fix the binding please.
This is wrong. Will fix.
