Quoting Taniya Das (2018-06-13 02:18:59)

Hello Stephen,

Thanks for review.

On 6/12/2018 1:25 PM, Stephen Boyd wrote:

quoted

Quoting Taniya Das (2018-06-04 00:56:25)

quoted

diff --git a/drivers/clk/qcom/dispcc-sdm845.c b/drivers/clk/qcom/dispcc-sdm845.c
new file mode 100644
index 0000000..317ab33
--- /dev/null
+++ b/drivers/clk/qcom/dispcc-sdm845.c

quoted

quoted

+#include "common.h"
+#include "gdsc.h"
+#include "reset.h"
+
+#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }

We really need to move this into the header file.

$ git grep '#define F(' -- drivers/clk/qcom/ | wc -l
13

Sure, will move this in a header and submit the patch.

Thanks!

quoted

quoted

+
+static struct clk_alpha_pll disp_cc_pll0 = {
+       .offset = 0x0,
+       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+       .clkr = {
+               .hw.init = &(struct clk_init_data){
+                       .name = "disp_cc_pll0",
+                       .parent_names = (const char *[]){ "bi_tcxo" },
+                       .num_parents = 1,
+                       .ops = &clk_alpha_pll_fabia_ops,
+               },
+       },
+};

[...]

quoted

+
+static struct clk_rcg2 disp_cc_mdss_pclk0_clk_src = {
+       .cmd_rcgr = 0x2058,
+       .mnd_width = 8,
+       .hid_width = 5,
+       .parent_map = disp_cc_parent_map_4,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "disp_cc_mdss_pclk0_clk_src",
+               .parent_names = disp_cc_parent_names_4,
+               .num_parents = 4,
+               .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,

Why is the nocache flag needed? Applies to all clks in this file.

This flag is required for all RCGs whose PLLs are controlled outside the 
clock controller. The display code would require the recalculated rate 
always.

That doesn't really answer the question. It seems to describe some sort
of problem with the PLL being controlled outside the clk controller/clk
framework. Please add more details and comments into the code here.

quoted

quoted

+               .ops = &clk_pixel_ops,
+       },
+};
+
+static struct clk_rcg2 disp_cc_mdss_pclk1_clk_src = {
+       .cmd_rcgr = 0x2070,
+       .mnd_width = 8,
+       .hid_width = 5,
+       .parent_map = disp_cc_parent_map_4,
+       .clkr.hw.init = &(struct clk_init_data){

[...]

quoted

+
+static const struct regmap_config disp_cc_sdm845_regmap_config = {
+       .reg_bits       = 32,
+       .reg_stride     = 4,
+       .val_bits       = 32,
+       .max_register   = 0x10000,

This seems arbitrarily large. List the actual end?

The actual end is larger than this :( , I have listed the range till the 
register offset are used here.

Ok.

quoted

quoted

+       .fast_io        = true,
+};
+
+static const struct qcom_cc_desc disp_cc_sdm845_desc = {
+       .config = &disp_cc_sdm845_regmap_config,
+       .clks = disp_cc_sdm845_clocks,
+       .num_clks = ARRAY_SIZE(disp_cc_sdm845_clocks),
+       .resets = disp_cc_sdm845_resets,
+       .num_resets = ARRAY_SIZE(disp_cc_sdm845_resets),
+       .gdscs = disp_cc_sdm845_gdscs,
+       .num_gdscs = ARRAY_SIZE(disp_cc_sdm845_gdscs),
+};
+
+static const struct of_device_id disp_cc_sdm845_match_table[] = {
+       { .compatible = "qcom,sdm845-dispcc" },
+       { }
+};
+MODULE_DEVICE_TABLE(of, disp_cc_sdm845_match_table);
+
+static int disp_cc_sdm845_probe(struct platform_device *pdev)
+{
+       struct regmap *regmap;
+
+       regmap = qcom_cc_map(pdev, &disp_cc_sdm845_desc);
+       if (IS_ERR(regmap))
+               return PTR_ERR(regmap);
+
+       clk_fabia_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
+
+       /* Enable clock gating for DSI and MDP clocks */

Hardware clock gating? What does this mean.

These are used for deciding whether to enable HW based Clock gating or 
not. Setting these bit will enable HW based Clock gating.

Please state it's hardware clock gating then in the comment.

quoted

quoted

+       regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0);
+
+       return qcom_cc_really_probe(pdev, &disp_cc_sdm845_desc, regmap);
+}