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

[Stephen Boyd]

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.

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

> +};
> +
> +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?

> +       },
> +};
> +
> +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.

> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, gpucc_msm8998_match_table);
> +