[PATCH v4 0/2] MSM8998 GPUCC Support

[PATCH v4 0/2] MSM8998 GPUCC Support

[Jeffrey Hugo]

The Adreno GPU on MSM8998 has its own clock controller, which is a
dependency for bringing up the GPU.  This series gets the gpucc all in
place as another step on the road to getting the GPU enabled.

v4:
-rebase onto mmcc series
-remove clk_get from the clock provider

v3:
-drop accepted DT patch
-correct "avoid" typo
-expand comment on why XO is required

v2:
-drop dead code

Jeffrey Hugo (2):
  clk: qcom: Add MSM8998 GPU Clock Controller (GPUCC) driver
  arm64: dts: qcom: msm8998: Add gpucc node

 arch/arm64/boot/dts/qcom/msm8998.dtsi |  14 ++
 drivers/clk/qcom/Kconfig              |   9 +
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/gpucc-msm8998.c      | 346 ++++++++++++++++++++++++++
 4 files changed, 370 insertions(+)
 create mode 100644 drivers/clk/qcom/gpucc-msm8998.c

-- 
2.17.1

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

[Jeffrey Hugo]

The GPUCC manages the clocks for the Adreno GPU found on MSM8998.

Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
 drivers/clk/qcom/Kconfig         |   9 +
 drivers/clk/qcom/Makefile        |   1 +
 drivers/clk/qcom/gpucc-msm8998.c | 346 +++++++++++++++++++++++++++++++
 3 files changed, 356 insertions(+)
 create mode 100644 drivers/clk/qcom/gpucc-msm8998.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 96efee18fa6c..31a70168327c 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -230,6 +230,15 @@ config MSM_MMCC_8998
 	  Say Y if you want to support multimedia devices such as display,
 	  graphics, video encode/decode, camera, etc.
 
+config MSM_GPUCC_8998
+	tristate "MSM8998 Graphics Clock Controller"
+	select MSM_GCC_8998
+	select QCOM_GDSC
+	help
+	  Support for the graphics clock controller on MSM8998 devices.
+	  Say Y if you want to support graphics controller devices and
+	  functionality such as 3D graphics.
+
 config QCS_GCC_404
 	tristate "QCS404 Global Clock Controller"
 	help
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 0ac3c1459313..616b68f91db6 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_MSM_GCC_8994) += gcc-msm8994.o
 obj-$(CONFIG_MSM_GCC_8996) += gcc-msm8996.o
 obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
 obj-$(CONFIG_MSM_GCC_8998) += gcc-msm8998.o
+obj-$(CONFIG_MSM_GPUCC_8998) += gpucc-msm8998.o
 obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
 obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
 obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
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>
+
+#include <dt-bindings/clock/qcom,gpucc-msm8998.h>
+
+#include "common.h"
+#include "clk-regmap.h"
+#include "clk-regmap-divider.h"
+#include "clk-alpha-pll.h"
+#include "clk-rcg.h"
+#include "clk-branch.h"
+#include "reset.h"
+#include "gdsc.h"
+
+enum {
+	P_XO,
+	P_GPLL0,
+	P_GPUPLL0_OUT_EVEN,
+};
+
+/* Instead of going directly to the block, XO is routed through this branch */
+static struct clk_branch gpucc_cxo_clk = {
+	.halt_reg = 0x1020,
+	.clkr = {
+		.enable_reg = 0x1020,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpucc_cxo_clk",
+			.parent_data = &(const struct clk_parent_data){
+				.fw_name = "xo",
+				.name = "xo"
+			},
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+			.flags = CLK_IS_CRITICAL,
+		},
+	},
+};
+
+static const struct clk_div_table post_div_table_fabia_even[] = {
+	{ 0x0, 1 },
+	{ 0x1, 2 },
+	{ 0x3, 4 },
+	{ 0x7, 8 },
+	{ }
+};
+
+static struct clk_alpha_pll gpupll0 = {
+	.offset = 0x0,
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "gpupll0",
+		.parent_hws = (const struct clk_hw *[]){ &gpucc_cxo_clk.clkr.hw },
+		.num_parents = 1,
+		.ops = &clk_alpha_pll_fixed_fabia_ops,
+	},
+};
+
+static struct clk_alpha_pll_postdiv gpupll0_out_even = {
+	.offset = 0x0,
+	.post_div_shift = 8,
+	.post_div_table = post_div_table_fabia_even,
+	.num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
+	.width = 4,
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "gpupll0_out_even",
+		.parent_hws = (const struct clk_hw *[]){ &gpupll0.clkr.hw },
+		.num_parents = 1,
+		.ops = &clk_alpha_pll_postdiv_fabia_ops,
+	},
+};
+
+static const struct parent_map gpu_xo_gpll0_map[] = {
+	{ P_XO, 0 },
+	{ P_GPLL0, 5 },
+};
+
+static const struct clk_parent_data gpu_xo_gpll0[] = {
+	{ .hw = &gpucc_cxo_clk.clkr.hw },
+	{ .fw_name = "gpll0", .name = "gpll0" },
+};
+
+static const struct parent_map gpu_xo_gpupll0_map[] = {
+	{ P_XO, 0 },
+	{ P_GPUPLL0_OUT_EVEN, 1 },
+};
+
+static const struct clk_parent_data gpu_xo_gpupll0[] = {
+	{ .hw = &gpucc_cxo_clk.clkr.hw },
+	{ .hw = &gpupll0_out_even.clkr.hw },
+};
+
+static const struct freq_tbl ftbl_rbcpr_clk_src[] = {
+	F(19200000, P_XO, 1, 0, 0),
+	F(50000000, P_GPLL0, 12, 0, 0),
+	{ }
+};
+
+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),
+	{ }
+};
+
+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,
+	},
+};
+
+static const struct freq_tbl ftbl_rbbmtimer_clk_src[] = {
+	F(19200000, P_XO, 1, 0, 0),
+	{ }
+};
+
+static struct clk_rcg2 rbbmtimer_clk_src = {
+	.cmd_rcgr = 0x10b0,
+	.hid_width = 5,
+	.parent_map = gpu_xo_gpll0_map,
+	.freq_tbl = ftbl_rbbmtimer_clk_src,
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "rbbmtimer_clk_src",
+		.parent_data = gpu_xo_gpll0,
+		.num_parents = 2,
+		.ops = &clk_rcg2_ops,
+	},
+};
+
+static const struct freq_tbl ftbl_gfx3d_isense_clk_src[] = {
+	F(19200000, P_XO, 1, 0, 0),
+	F(40000000, P_GPLL0, 15, 0, 0),
+	F(200000000, P_GPLL0, 3, 0, 0),
+	F(300000000, P_GPLL0, 2, 0, 0),
+	{ }
+};
+
+static struct clk_rcg2 gfx3d_isense_clk_src = {
+	.cmd_rcgr = 0x1100,
+	.hid_width = 5,
+	.parent_map = gpu_xo_gpll0_map,
+	.freq_tbl = ftbl_gfx3d_isense_clk_src,
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "gfx3d_isense_clk_src",
+		.parent_data = gpu_xo_gpll0,
+		.num_parents = 2,
+		.ops = &clk_rcg2_ops,
+	},
+};
+
+static struct clk_branch rbcpr_clk = {
+	.halt_reg = 0x1054,
+	.clkr = {
+		.enable_reg = 0x1054,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "rbcpr_clk",
+			.parent_hws = (const struct clk_hw *[]){ &rbcpr_clk_src.clkr.hw },
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+			.flags = CLK_SET_RATE_PARENT,
+		},
+	},
+};
+
+static struct clk_branch gfx3d_clk = {
+	.halt_reg = 0x1098,
+	.clkr = {
+		.enable_reg = 0x1098,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gfx3d_clk",
+			.parent_hws = (const struct clk_hw *[]){ &gfx3d_clk_src.clkr.hw },
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+			.flags = CLK_SET_RATE_PARENT,
+		},
+	},
+};
+
+static struct clk_branch rbbmtimer_clk = {
+	.halt_reg = 0x10d0,
+	.clkr = {
+		.enable_reg = 0x10d0,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "rbbmtimer_clk",
+			.parent_hws = (const struct clk_hw *[]){ &rbbmtimer_clk_src.clkr.hw },
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+			.flags = CLK_SET_RATE_PARENT,
+		},
+	},
+};
+
+static struct clk_branch gfx3d_isense_clk = {
+	.halt_reg = 0x1124,
+	.clkr = {
+		.enable_reg = 0x1124,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gfx3d_isense_clk",
+			.parent_hws = (const struct clk_hw *[]){ &gfx3d_isense_clk_src.clkr.hw },
+			.num_parents = 1,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct gdsc gpu_cx_gdsc = {
+	.gdscr = 0x1004,
+	.pd = {
+		.name = "gpu_cx",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc gpu_gx_gdsc = {
+	.gdscr = 0x1094,
+	.clamp_io_ctrl = 0x130,
+	.pd = {
+		.name = "gpu_gx",
+	},
+	.parent = &gpu_cx_gdsc.pd,
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = CLAMP_IO | AON_RESET,
+};
+
+static struct clk_regmap *gpucc_msm8998_clocks[] = {
+	[GPUPLL0] = &gpupll0.clkr,
+	[GPUPLL0_OUT_EVEN] = &gpupll0_out_even.clkr,
+	[RBCPR_CLK_SRC] = &rbcpr_clk_src.clkr,
+	[GFX3D_CLK_SRC] = &gfx3d_clk_src.clkr,
+	[RBBMTIMER_CLK_SRC] = &rbbmtimer_clk_src.clkr,
+	[GFX3D_ISENSE_CLK_SRC] = &gfx3d_isense_clk_src.clkr,
+	[RBCPR_CLK] = &rbcpr_clk.clkr,
+	[GFX3D_CLK] = &gfx3d_clk.clkr,
+	[RBBMTIMER_CLK] = &rbbmtimer_clk.clkr,
+	[GFX3D_ISENSE_CLK] = &gfx3d_isense_clk.clkr,
+	[GPUCC_CXO_CLK] = &gpucc_cxo_clk.clkr,
+};
+
+static struct gdsc *gpucc_msm8998_gdscs[] = {
+	[GPU_CX_GDSC] = &gpu_cx_gdsc,
+	[GPU_GX_GDSC] = &gpu_gx_gdsc,
+};
+
+static const struct qcom_reset_map gpucc_msm8998_resets[] = {
+	[GPU_CX_BCR] = { 0x1000 },
+	[RBCPR_BCR] = { 0x1050 },
+	[GPU_GX_BCR] = { 0x1090 },
+	[GPU_ISENSE_BCR] = { 0x1120 },
+};
+
+static const struct regmap_config gpucc_msm8998_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= 0x9000,
+	.fast_io	= true,
+};
+
+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" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpucc_msm8998_match_table);
+
+static int gpucc_msm8998_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+
+	regmap = qcom_cc_map(pdev, &gpucc_msm8998_desc);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/* force periph logic on to avoid perf counter corruption */
+	regmap_write_bits(regmap, gfx3d_clk.clkr.enable_reg, BIT(13), BIT(13));
+	/* tweak droop detector (GPUCC_GPU_DD_WRAP_CTRL) to reduce leakage */
+	regmap_write_bits(regmap, gfx3d_clk.clkr.enable_reg, BIT(0), BIT(0));
+
+	return qcom_cc_really_probe(pdev, &gpucc_msm8998_desc, regmap);
+}
+
+static struct platform_driver gpucc_msm8998_driver = {
+	.probe		= gpucc_msm8998_probe,
+	.driver		= {
+		.name	= "gpucc-msm8998",
+		.of_match_table = gpucc_msm8998_match_table,
+	},
+};
+module_platform_driver(gpucc_msm8998_driver);
+
+MODULE_DESCRIPTION("QCOM GPUCC MSM8998 Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v4 2/2] arm64: dts: qcom: msm8998: Add gpucc node

[Jeffrey Hugo]

Add MSM8998 GPU Clock Controller DT node.

Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 5a9efd749fa6..896f37c936ee 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -3,6 +3,7 @@
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,gcc-msm8998.h>
+#include <dt-bindings/clock/qcom,gpucc-msm8998.h>
 #include <dt-bindings/clock/qcom,mmcc-msm8998.h>
 #include <dt-bindings/clock/qcom,rpmcc.h>
 #include <dt-bindings/power/qcom-rpmpd.h>
@@ -951,6 +952,19 @@
 			#interrupt-cells = <0x2>;
 		};
 
+		gpucc: clock-controller@5065000 {
+			compatible = "qcom,gpucc-msm8998";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+			reg = <0x05065000 0x9000>;
+
+			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
+				 <&gcc GPLL0_OUT_MAIN>;
+			clock-names = "xo",
+				      "gpll0";
+		};
+
 		spmi_bus: spmi@800f000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg =	<0x0800f000 0x1000>,
-- 
2.17.1

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);
> +

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

[Jeffrey Hugo]

On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@kernel.org> 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.

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

[Taniya Das]

Hi Jeffrey,

On 10/2/2019 6:46 AM, Jeffrey Hugo wrote:
> The GPUCC manages the clocks for the Adreno GPU found on MSM8998.
> 
> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> ---
>   drivers/clk/qcom/Kconfig         |   9 +
>   drivers/clk/qcom/Makefile        |   1 +
>   drivers/clk/qcom/gpucc-msm8998.c | 346 +++++++++++++++++++++++++++++++
>   3 files changed, 356 insertions(+)
>   create mode 100644 drivers/clk/qcom/gpucc-msm8998.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 96efee18fa6c..31a70168327c 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -230,6 +230,15 @@ config MSM_MMCC_8998
>   	  Say Y if you want to support multimedia devices such as display,
>   	  graphics, video encode/decode, camera, etc.
>   
> +config MSM_GPUCC_8998
> +	tristate "MSM8998 Graphics Clock Controller"
> +	select MSM_GCC_8998
> +	select QCOM_GDSC
> +	help
> +	  Support for the graphics clock controller on MSM8998 devices.
> +	  Say Y if you want to support graphics controller devices and
> +	  functionality such as 3D graphics.
> +
>   config QCS_GCC_404
>   	tristate "QCS404 Global Clock Controller"
>   	help
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 0ac3c1459313..616b68f91db6 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_MSM_GCC_8994) += gcc-msm8994.o
>   obj-$(CONFIG_MSM_GCC_8996) += gcc-msm8996.o
>   obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
>   obj-$(CONFIG_MSM_GCC_8998) += gcc-msm8998.o
> +obj-$(CONFIG_MSM_GPUCC_8998) += gpucc-msm8998.o
>   obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
>   obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
>   obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
> 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
> + */
> +

Copyright (c) 2019, The Linux Foundation. All rights reserved.

> +#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>
> +
> +#include <dt-bindings/clock/qcom,gpucc-msm8998.h>
> +
> +#include "common.h"
> +#include "clk-regmap.h"
> +#include "clk-regmap-divider.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-rcg.h"
> +#include "clk-branch.h"
> +#include "reset.h"
> +#include "gdsc.h"
> +
> +enum {
> +	P_XO,
> +	P_GPLL0,
> +	P_GPUPLL0_OUT_EVEN,
> +};
> +
> +/* Instead of going directly to the block, XO is routed through this branch */
> +static struct clk_branch gpucc_cxo_clk = {
> +	.halt_reg = 0x1020,
> +	.clkr = {
> +		.enable_reg = 0x1020,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpucc_cxo_clk",
> +			.parent_data = &(const struct clk_parent_data){
> +				.fw_name = "xo",
> +				.name = "xo"
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +			.flags = CLK_IS_CRITICAL,
> +		},
> +	},
> +};
> +
> +static const struct clk_div_table post_div_table_fabia_even[] = {
> +	{ 0x0, 1 },
> +	{ 0x1, 2 },
> +	{ 0x3, 4 },
> +	{ 0x7, 8 },
> +	{ }
> +};
> +
> +static struct clk_alpha_pll gpupll0 = {
> +	.offset = 0x0,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gpupll0",
> +		.parent_hws = (const struct clk_hw *[]){ &gpucc_cxo_clk.clkr.hw },
> +		.num_parents = 1,
> +		.ops = &clk_alpha_pll_fixed_fabia_ops,
> +	},
> +};
> +
> +static struct clk_alpha_pll_postdiv gpupll0_out_even = {
> +	.offset = 0x0,
> +	.post_div_shift = 8,
> +	.post_div_table = post_div_table_fabia_even,
> +	.num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
> +	.width = 4,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gpupll0_out_even",
> +		.parent_hws = (const struct clk_hw *[]){ &gpupll0.clkr.hw },
> +		.num_parents = 1,
> +		.ops = &clk_alpha_pll_postdiv_fabia_ops,
> +	},
> +};
> +
> +static const struct parent_map gpu_xo_gpll0_map[] = {
> +	{ P_XO, 0 },
> +	{ P_GPLL0, 5 },
> +};
> +
> +static const struct clk_parent_data gpu_xo_gpll0[] = {
> +	{ .hw = &gpucc_cxo_clk.clkr.hw },
> +	{ .fw_name = "gpll0", .name = "gpll0" },
> +};
> +
> +static const struct parent_map gpu_xo_gpupll0_map[] = {
> +	{ P_XO, 0 },
> +	{ P_GPUPLL0_OUT_EVEN, 1 },
> +};
> +
> +static const struct clk_parent_data gpu_xo_gpupll0[] = {
> +	{ .hw = &gpucc_cxo_clk.clkr.hw },
> +	{ .hw = &gpupll0_out_even.clkr.hw },
> +};
> +
> +static const struct freq_tbl ftbl_rbcpr_clk_src[] = {
> +	F(19200000, P_XO, 1, 0, 0),
> +	F(50000000, P_GPLL0, 12, 0, 0),
> +	{ }
> +};
> +
> +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),
> +	{ }
> +};
> +
> +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,
> +	},
> +};
> +
> +static const struct freq_tbl ftbl_rbbmtimer_clk_src[] = {
> +	F(19200000, P_XO, 1, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 rbbmtimer_clk_src = {
> +	.cmd_rcgr = 0x10b0,
> +	.hid_width = 5,
> +	.parent_map = gpu_xo_gpll0_map,
> +	.freq_tbl = ftbl_rbbmtimer_clk_src,
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "rbbmtimer_clk_src",
> +		.parent_data = gpu_xo_gpll0,
> +		.num_parents = 2,
> +		.ops = &clk_rcg2_ops,
> +	},
> +};
> +
> +static const struct freq_tbl ftbl_gfx3d_isense_clk_src[] = {
> +	F(19200000, P_XO, 1, 0, 0),
> +	F(40000000, P_GPLL0, 15, 0, 0),
> +	F(200000000, P_GPLL0, 3, 0, 0),
> +	F(300000000, P_GPLL0, 2, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 gfx3d_isense_clk_src = {
> +	.cmd_rcgr = 0x1100,
> +	.hid_width = 5,
> +	.parent_map = gpu_xo_gpll0_map,
> +	.freq_tbl = ftbl_gfx3d_isense_clk_src,
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gfx3d_isense_clk_src",
> +		.parent_data = gpu_xo_gpll0,
> +		.num_parents = 2,
> +		.ops = &clk_rcg2_ops,
> +	},
> +};
> +
> +static struct clk_branch rbcpr_clk = {
> +	.halt_reg = 0x1054,
> +	.clkr = {
> +		.enable_reg = 0x1054,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "rbcpr_clk",
> +			.parent_hws = (const struct clk_hw *[]){ &rbcpr_clk_src.clkr.hw },
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +			.flags = CLK_SET_RATE_PARENT,
> +		},
> +	},
> +};
> +

If unused by consumer it would good to drop this clock.

> +static struct clk_branch gfx3d_clk = {
> +	.halt_reg = 0x1098,
> +	.clkr = {
> +		.enable_reg = 0x1098,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gfx3d_clk",
> +			.parent_hws = (const struct clk_hw *[]){ &gfx3d_clk_src.clkr.hw },
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +			.flags = CLK_SET_RATE_PARENT,
> +		},
> +	},
> +};
> +
> +static struct clk_branch rbbmtimer_clk = {
> +	.halt_reg = 0x10d0,
> +	.clkr = {
> +		.enable_reg = 0x10d0,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "rbbmtimer_clk",
> +			.parent_hws = (const struct clk_hw *[]){ &rbbmtimer_clk_src.clkr.hw },
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +			.flags = CLK_SET_RATE_PARENT,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gfx3d_isense_clk = {
> +	.halt_reg = 0x1124,
> +	.clkr = {
> +		.enable_reg = 0x1124,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gfx3d_isense_clk",
> +			.parent_hws = (const struct clk_hw *[]){ &gfx3d_isense_clk_src.clkr.hw },
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct gdsc gpu_cx_gdsc = {
> +	.gdscr = 0x1004,
> +	.pd = {
> +		.name = "gpu_cx",
> +	},
> +	.pwrsts = PWRSTS_OFF_ON,
> +};
> +
> +static struct gdsc gpu_gx_gdsc = {
> +	.gdscr = 0x1094,
> +	.clamp_io_ctrl = 0x130,
> +	.pd = {
> +		.name = "gpu_gx",
> +	},
> +	.parent = &gpu_cx_gdsc.pd,
> +	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = CLAMP_IO | AON_RESET,
> +};
> +
> +static struct clk_regmap *gpucc_msm8998_clocks[] = {
> +	[GPUPLL0] = &gpupll0.clkr,
> +	[GPUPLL0_OUT_EVEN] = &gpupll0_out_even.clkr,
> +	[RBCPR_CLK_SRC] = &rbcpr_clk_src.clkr,
> +	[GFX3D_CLK_SRC] = &gfx3d_clk_src.clkr,
> +	[RBBMTIMER_CLK_SRC] = &rbbmtimer_clk_src.clkr,
> +	[GFX3D_ISENSE_CLK_SRC] = &gfx3d_isense_clk_src.clkr,
> +	[RBCPR_CLK] = &rbcpr_clk.clkr,
> +	[GFX3D_CLK] = &gfx3d_clk.clkr,
> +	[RBBMTIMER_CLK] = &rbbmtimer_clk.clkr,
> +	[GFX3D_ISENSE_CLK] = &gfx3d_isense_clk.clkr,
> +	[GPUCC_CXO_CLK] = &gpucc_cxo_clk.clkr,
> +};
> +
> +static struct gdsc *gpucc_msm8998_gdscs[] = {
> +	[GPU_CX_GDSC] = &gpu_cx_gdsc,
> +	[GPU_GX_GDSC] = &gpu_gx_gdsc,
> +};
> +
> +static const struct qcom_reset_map gpucc_msm8998_resets[] = {
> +	[GPU_CX_BCR] = { 0x1000 },
> +	[RBCPR_BCR] = { 0x1050 },
> +	[GPU_GX_BCR] = { 0x1090 },
> +	[GPU_ISENSE_BCR] = { 0x1120 },
> +};
> +
> +static const struct regmap_config gpucc_msm8998_regmap_config = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x9000,
> +	.fast_io	= true,
> +};
> +
> +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" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, gpucc_msm8998_match_table);
> +
> +static int gpucc_msm8998_probe(struct platform_device *pdev)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = qcom_cc_map(pdev, &gpucc_msm8998_desc);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	/* force periph logic on to avoid perf counter corruption */
> +	regmap_write_bits(regmap, gfx3d_clk.clkr.enable_reg, BIT(13), BIT(13));
> +	/* tweak droop detector (GPUCC_GPU_DD_WRAP_CTRL) to reduce leakage */
> +	regmap_write_bits(regmap, gfx3d_clk.clkr.enable_reg, BIT(0), BIT(0));
> +
> +	return qcom_cc_really_probe(pdev, &gpucc_msm8998_desc, regmap);
> +}
> +
> +static struct platform_driver gpucc_msm8998_driver = {
> +	.probe		= gpucc_msm8998_probe,
> +	.driver		= {
> +		.name	= "gpucc-msm8998",
> +		.of_match_table = gpucc_msm8998_match_table,
> +	},
> +};
> +module_platform_driver(gpucc_msm8998_driver);
> +
> +MODULE_DESCRIPTION("QCOM GPUCC MSM8998 Driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

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

[Jeffrey Hugo]

On Thu, Oct 17, 2019 at 10:11 PM Taniya Das <tdas@codeaurora.org> wrote:
>
> Hi Jeffrey,
>
> On 10/2/2019 6:46 AM, Jeffrey Hugo wrote:
> > The GPUCC manages the clocks for the Adreno GPU found on MSM8998.
> >
> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> > ---
> >   drivers/clk/qcom/Kconfig         |   9 +
> >   drivers/clk/qcom/Makefile        |   1 +
> >   drivers/clk/qcom/gpucc-msm8998.c | 346 +++++++++++++++++++++++++++++++
> >   3 files changed, 356 insertions(+)
> >   create mode 100644 drivers/clk/qcom/gpucc-msm8998.c
> >
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index 96efee18fa6c..31a70168327c 100644
> > --- a/drivers/clk/qcom/Kconfig
> > +++ b/drivers/clk/qcom/Kconfig
> > @@ -230,6 +230,15 @@ config MSM_MMCC_8998
> >         Say Y if you want to support multimedia devices such as display,
> >         graphics, video encode/decode, camera, etc.
> >
> > +config MSM_GPUCC_8998
> > +     tristate "MSM8998 Graphics Clock Controller"
> > +     select MSM_GCC_8998
> > +     select QCOM_GDSC
> > +     help
> > +       Support for the graphics clock controller on MSM8998 devices.
> > +       Say Y if you want to support graphics controller devices and
> > +       functionality such as 3D graphics.
> > +
> >   config QCS_GCC_404
> >       tristate "QCS404 Global Clock Controller"
> >       help
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index 0ac3c1459313..616b68f91db6 100644
> > --- a/drivers/clk/qcom/Makefile
> > +++ b/drivers/clk/qcom/Makefile
> > @@ -33,6 +33,7 @@ obj-$(CONFIG_MSM_GCC_8994) += gcc-msm8994.o
> >   obj-$(CONFIG_MSM_GCC_8996) += gcc-msm8996.o
> >   obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
> >   obj-$(CONFIG_MSM_GCC_8998) += gcc-msm8998.o
> > +obj-$(CONFIG_MSM_GPUCC_8998) += gpucc-msm8998.o
> >   obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
> >   obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
> >   obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
> > 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
> > + */
> > +
>
> Copyright (c) 2019, The Linux Foundation. All rights reserved.

No, what I have is correct.  I'll send you the resources regarding
this issue since its not really relevant to the community.

>
> > +#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>
> > +
> > +#include <dt-bindings/clock/qcom,gpucc-msm8998.h>
> > +
> > +#include "common.h"
> > +#include "clk-regmap.h"
> > +#include "clk-regmap-divider.h"
> > +#include "clk-alpha-pll.h"
> > +#include "clk-rcg.h"
> > +#include "clk-branch.h"
> > +#include "reset.h"
> > +#include "gdsc.h"
> > +
> > +enum {
> > +     P_XO,
> > +     P_GPLL0,
> > +     P_GPUPLL0_OUT_EVEN,
> > +};
> > +
> > +/* Instead of going directly to the block, XO is routed through this branch */
> > +static struct clk_branch gpucc_cxo_clk = {
> > +     .halt_reg = 0x1020,
> > +     .clkr = {
> > +             .enable_reg = 0x1020,
> > +             .enable_mask = BIT(0),
> > +             .hw.init = &(struct clk_init_data){
> > +                     .name = "gpucc_cxo_clk",
> > +                     .parent_data = &(const struct clk_parent_data){
> > +                             .fw_name = "xo",
> > +                             .name = "xo"
> > +                     },
> > +                     .num_parents = 1,
> > +                     .ops = &clk_branch2_ops,
> > +                     .flags = CLK_IS_CRITICAL,
> > +             },
> > +     },
> > +};
> > +
> > +static const struct clk_div_table post_div_table_fabia_even[] = {
> > +     { 0x0, 1 },
> > +     { 0x1, 2 },
> > +     { 0x3, 4 },
> > +     { 0x7, 8 },
> > +     { }
> > +};
> > +
> > +static struct clk_alpha_pll gpupll0 = {
> > +     .offset = 0x0,
> > +     .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> > +     .clkr.hw.init = &(struct clk_init_data){
> > +             .name = "gpupll0",
> > +             .parent_hws = (const struct clk_hw *[]){ &gpucc_cxo_clk.clkr.hw },
> > +             .num_parents = 1,
> > +             .ops = &clk_alpha_pll_fixed_fabia_ops,
> > +     },
> > +};
> > +
> > +static struct clk_alpha_pll_postdiv gpupll0_out_even = {
> > +     .offset = 0x0,
> > +     .post_div_shift = 8,
> > +     .post_div_table = post_div_table_fabia_even,
> > +     .num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
> > +     .width = 4,
> > +     .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> > +     .clkr.hw.init = &(struct clk_init_data){
> > +             .name = "gpupll0_out_even",
> > +             .parent_hws = (const struct clk_hw *[]){ &gpupll0.clkr.hw },
> > +             .num_parents = 1,
> > +             .ops = &clk_alpha_pll_postdiv_fabia_ops,
> > +     },
> > +};
> > +
> > +static const struct parent_map gpu_xo_gpll0_map[] = {
> > +     { P_XO, 0 },
> > +     { P_GPLL0, 5 },
> > +};
> > +
> > +static const struct clk_parent_data gpu_xo_gpll0[] = {
> > +     { .hw = &gpucc_cxo_clk.clkr.hw },
> > +     { .fw_name = "gpll0", .name = "gpll0" },
> > +};
> > +
> > +static const struct parent_map gpu_xo_gpupll0_map[] = {
> > +     { P_XO, 0 },
> > +     { P_GPUPLL0_OUT_EVEN, 1 },
> > +};
> > +
> > +static const struct clk_parent_data gpu_xo_gpupll0[] = {
> > +     { .hw = &gpucc_cxo_clk.clkr.hw },
> > +     { .hw = &gpupll0_out_even.clkr.hw },
> > +};
> > +
> > +static const struct freq_tbl ftbl_rbcpr_clk_src[] = {
> > +     F(19200000, P_XO, 1, 0, 0),
> > +     F(50000000, P_GPLL0, 12, 0, 0),
> > +     { }
> > +};
> > +
> > +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),
> > +     { }
> > +};
> > +
> > +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,
> > +     },
> > +};
> > +
> > +static const struct freq_tbl ftbl_rbbmtimer_clk_src[] = {
> > +     F(19200000, P_XO, 1, 0, 0),
> > +     { }
> > +};
> > +
> > +static struct clk_rcg2 rbbmtimer_clk_src = {
> > +     .cmd_rcgr = 0x10b0,
> > +     .hid_width = 5,
> > +     .parent_map = gpu_xo_gpll0_map,
> > +     .freq_tbl = ftbl_rbbmtimer_clk_src,
> > +     .clkr.hw.init = &(struct clk_init_data){
> > +             .name = "rbbmtimer_clk_src",
> > +             .parent_data = gpu_xo_gpll0,
> > +             .num_parents = 2,
> > +             .ops = &clk_rcg2_ops,
> > +     },
> > +};
> > +
> > +static const struct freq_tbl ftbl_gfx3d_isense_clk_src[] = {
> > +     F(19200000, P_XO, 1, 0, 0),
> > +     F(40000000, P_GPLL0, 15, 0, 0),
> > +     F(200000000, P_GPLL0, 3, 0, 0),
> > +     F(300000000, P_GPLL0, 2, 0, 0),
> > +     { }
> > +};
> > +
> > +static struct clk_rcg2 gfx3d_isense_clk_src = {
> > +     .cmd_rcgr = 0x1100,
> > +     .hid_width = 5,
> > +     .parent_map = gpu_xo_gpll0_map,
> > +     .freq_tbl = ftbl_gfx3d_isense_clk_src,
> > +     .clkr.hw.init = &(struct clk_init_data){
> > +             .name = "gfx3d_isense_clk_src",
> > +             .parent_data = gpu_xo_gpll0,
> > +             .num_parents = 2,
> > +             .ops = &clk_rcg2_ops,
> > +     },
> > +};
> > +
> > +static struct clk_branch rbcpr_clk = {
> > +     .halt_reg = 0x1054,
> > +     .clkr = {
> > +             .enable_reg = 0x1054,
> > +             .enable_mask = BIT(0),
> > +             .hw.init = &(struct clk_init_data){
> > +                     .name = "rbcpr_clk",
> > +                     .parent_hws = (const struct clk_hw *[]){ &rbcpr_clk_src.clkr.hw },
> > +                     .num_parents = 1,
> > +                     .ops = &clk_branch2_ops,
> > +                     .flags = CLK_SET_RATE_PARENT,
> > +             },
> > +     },
> > +};
> > +
>
> If unused by consumer it would good to drop this clock.

This gets used, hence why I have it listed.

>
> > +static struct clk_branch gfx3d_clk = {
> > +     .halt_reg = 0x1098,
> > +     .clkr = {
> > +             .enable_reg = 0x1098,
> > +             .enable_mask = BIT(0),
> > +             .hw.init = &(struct clk_init_data){
> > +                     .name = "gfx3d_clk",
> > +                     .parent_hws = (const struct clk_hw *[]){ &gfx3d_clk_src.clkr.hw },
> > +                     .num_parents = 1,
> > +                     .ops = &clk_branch2_ops,
> > +                     .flags = CLK_SET_RATE_PARENT,
> > +             },
> > +     },
> > +};
> > +
> > +static struct clk_branch rbbmtimer_clk = {
> > +     .halt_reg = 0x10d0,
> > +     .clkr = {
> > +             .enable_reg = 0x10d0,
> > +             .enable_mask = BIT(0),
> > +             .hw.init = &(struct clk_init_data){
> > +                     .name = "rbbmtimer_clk",
> > +                     .parent_hws = (const struct clk_hw *[]){ &rbbmtimer_clk_src.clkr.hw },
> > +                     .num_parents = 1,
> > +                     .ops = &clk_branch2_ops,
> > +                     .flags = CLK_SET_RATE_PARENT,
> > +             },
> > +     },
> > +};
> > +
> > +static struct clk_branch gfx3d_isense_clk = {
> > +     .halt_reg = 0x1124,
> > +     .clkr = {
> > +             .enable_reg = 0x1124,
> > +             .enable_mask = BIT(0),
> > +             .hw.init = &(struct clk_init_data){
> > +                     .name = "gfx3d_isense_clk",
> > +                     .parent_hws = (const struct clk_hw *[]){ &gfx3d_isense_clk_src.clkr.hw },
> > +                     .num_parents = 1,
> > +                     .ops = &clk_branch2_ops,
> > +             },
> > +     },
> > +};
> > +
> > +static struct gdsc gpu_cx_gdsc = {
> > +     .gdscr = 0x1004,
> > +     .pd = {
> > +             .name = "gpu_cx",
> > +     },
> > +     .pwrsts = PWRSTS_OFF_ON,
> > +};
> > +
> > +static struct gdsc gpu_gx_gdsc = {
> > +     .gdscr = 0x1094,
> > +     .clamp_io_ctrl = 0x130,
> > +     .pd = {
> > +             .name = "gpu_gx",
> > +     },
> > +     .parent = &gpu_cx_gdsc.pd,
> > +     .pwrsts = PWRSTS_OFF_ON,
> > +     .flags = CLAMP_IO | AON_RESET,
> > +};
> > +
> > +static struct clk_regmap *gpucc_msm8998_clocks[] = {
> > +     [GPUPLL0] = &gpupll0.clkr,
> > +     [GPUPLL0_OUT_EVEN] = &gpupll0_out_even.clkr,
> > +     [RBCPR_CLK_SRC] = &rbcpr_clk_src.clkr,
> > +     [GFX3D_CLK_SRC] = &gfx3d_clk_src.clkr,
> > +     [RBBMTIMER_CLK_SRC] = &rbbmtimer_clk_src.clkr,
> > +     [GFX3D_ISENSE_CLK_SRC] = &gfx3d_isense_clk_src.clkr,
> > +     [RBCPR_CLK] = &rbcpr_clk.clkr,
> > +     [GFX3D_CLK] = &gfx3d_clk.clkr,
> > +     [RBBMTIMER_CLK] = &rbbmtimer_clk.clkr,
> > +     [GFX3D_ISENSE_CLK] = &gfx3d_isense_clk.clkr,
> > +     [GPUCC_CXO_CLK] = &gpucc_cxo_clk.clkr,
> > +};
> > +
> > +static struct gdsc *gpucc_msm8998_gdscs[] = {
> > +     [GPU_CX_GDSC] = &gpu_cx_gdsc,
> > +     [GPU_GX_GDSC] = &gpu_gx_gdsc,
> > +};
> > +
> > +static const struct qcom_reset_map gpucc_msm8998_resets[] = {
> > +     [GPU_CX_BCR] = { 0x1000 },
> > +     [RBCPR_BCR] = { 0x1050 },
> > +     [GPU_GX_BCR] = { 0x1090 },
> > +     [GPU_ISENSE_BCR] = { 0x1120 },
> > +};
> > +
> > +static const struct regmap_config gpucc_msm8998_regmap_config = {
> > +     .reg_bits       = 32,
> > +     .reg_stride     = 4,
> > +     .val_bits       = 32,
> > +     .max_register   = 0x9000,
> > +     .fast_io        = true,
> > +};
> > +
> > +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" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, gpucc_msm8998_match_table);
> > +
> > +static int gpucc_msm8998_probe(struct platform_device *pdev)
> > +{
> > +     struct regmap *regmap;
> > +
> > +     regmap = qcom_cc_map(pdev, &gpucc_msm8998_desc);
> > +     if (IS_ERR(regmap))
> > +             return PTR_ERR(regmap);
> > +
> > +     /* force periph logic on to avoid perf counter corruption */
> > +     regmap_write_bits(regmap, gfx3d_clk.clkr.enable_reg, BIT(13), BIT(13));
> > +     /* tweak droop detector (GPUCC_GPU_DD_WRAP_CTRL) to reduce leakage */
> > +     regmap_write_bits(regmap, gfx3d_clk.clkr.enable_reg, BIT(0), BIT(0));
> > +
> > +     return qcom_cc_really_probe(pdev, &gpucc_msm8998_desc, regmap);
> > +}
> > +
> > +static struct platform_driver gpucc_msm8998_driver = {
> > +     .probe          = gpucc_msm8998_probe,
> > +     .driver         = {
> > +             .name   = "gpucc-msm8998",
> > +             .of_match_table = gpucc_msm8998_match_table,
> > +     },
> > +};
> > +module_platform_driver(gpucc_msm8998_driver);
> > +
> > +MODULE_DESCRIPTION("QCOM GPUCC MSM8998 Driver");
> > +MODULE_LICENSE("GPL v2");
> >
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
>
> --

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@gmail.com> wrote:
>
> On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@kernel.org> 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

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

[Stephen Boyd]

Quoting Jeffrey Hugo (2019-10-18 14:11:09)
> On Thu, Oct 17, 2019 at 5:16 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
> >
> > On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@kernel.org> 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).

Yes. You have to have some sort of frequency table to tell us what the
source and predivider to use.

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

Ouch!

> 
> Please advise how you would like to proceed.

Please have a frequency table like

 { .src = SOME_PLL, .div = 4 }


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

Why isn't rcg_ops doing it? The idea is to copy whatever is happening
with this snippet in the _freq_tbl_determine_rate() in rcg.c to rcg2.c

	clk_flags = clk_hw_get_flags(hw);
	p = clk_hw_get_parent_by_index(hw, index);
	if (clk_flags & CLK_SET_RATE_PARENT) {
		rate = rate * f->pre_div;

We have checked for CLK_SET_RATE_PARENT from the beginning. Maybe it was
always broken! If the frequency table pointer can return us the pre div
and source then we can do math to ask the parent PLL for something.

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

Please make rcg2 clk ops "do the right thing" when the flag
CLK_SET_RATE_PARENT is set and the frequency table is just a
source/divider sort of thing. That way we don't have to have different
clk ops or even put anything in the frequency table besides the source
PLL we want to use and the predivider we want to configure.

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

[Jeffrey Hugo]

On Sun, Oct 27, 2019 at 3:36 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Jeffrey Hugo (2019-10-18 14:11:09)
> > On Thu, Oct 17, 2019 at 5:16 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 3:50 PM Stephen Boyd <sboyd@kernel.org> 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).
>
> Yes. You have to have some sort of frequency table to tell us what the
> source and predivider to use.
>
> > 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.
>
> Ouch!
>
> >
> > Please advise how you would like to proceed.
>
> Please have a frequency table like
>
>  { .src = SOME_PLL, .div = 4 }
>
>
> >
> > I can still extend rcg2_ops to do what you want, but it won't be based
> > on what rcg_ops is doing.
>
> Why isn't rcg_ops doing it? The idea is to copy whatever is happening
> with this snippet in the _freq_tbl_determine_rate() in rcg.c to rcg2.c

That more or less exists in both cases, although rcg2_ops may need a
bit of additional work.  The problem is that in both cases, before
this snippet, we've already called qcom_find_freq(), and in both cases
(rcg_ops and rcg2_ops), f will be an invalid entry and we won't have
the proper pre_div value.

>
>         clk_flags = clk_hw_get_flags(hw);
>         p = clk_hw_get_parent_by_index(hw, index);
>         if (clk_flags & CLK_SET_RATE_PARENT) {
>                 rate = rate * f->pre_div;
>
> We have checked for CLK_SET_RATE_PARENT from the beginning. Maybe it was
> always broken! If the frequency table pointer can return us the pre div
> and source then we can do math to ask the parent PLL for something.
>
> >
> > 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.
>
> Please make rcg2 clk ops "do the right thing" when the flag
> CLK_SET_RATE_PARENT is set and the frequency table is just a
> source/divider sort of thing. That way we don't have to have different
> clk ops or even put anything in the frequency table besides the source
> PLL we want to use and the predivider we want to configure.
>

Got it.  Will do.