clk: qcom: CPU clock driver for msm8996

arch/arm64/Kconfig.platforms +4 -0

+       help
+       This enables support for the MSM8996 chipset. If you do not
+       wish to build a kernel that runs on this chipset, say 'N' here.
+
 config ARCH_ROCKCHIP
        bool "Rockchip Platforms"
        select ARCH_HAS_RESET_CONTROLLER

drivers/perf/qcom_l2_pmu.c +0 -0

index 4fdc848..8cece9e 100644

drivers/perf/qcom_l2_pmu.c +0 -0

  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and

drivers/soc/qcom/Makefile +1 -0

index dcebf28..4e91e2a 100644
@@ -12,3 +12,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)       += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)        += smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_ARCH_MSM8996) +=  kryo-l2-accessors.o

drivers/soc/qcom/kryo-l2-accessors.c new file +2 -0

new file mode 100644
index 0000000..6743848
@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.

include/soc/qcom/kryo-l2-accessors.h new file +5 -0

new file mode 100644
index 0000000..e9a5eab
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 19:45
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 01/10] soc: qcom: Separate kryo l2 accessors from PMU driver

Quoting Ilia Lin (2018-02-14 05:59:43)

Quoted 8 lines

(quoted diff) +0 -0

b/arch/arm64/Kconfig.platforms index fbedbd8..78a103b 100644

arch/arm64/Kconfig.platforms +0 -0

@@ -145,6 +145,13 @@ config ARCH_REALTEK
          This enables support for the ARMv8 based Realtek chipsets,
          like the RTD1295.
 

arch/arm64/Kconfig.platforms config ARCH_REALTEK · line 150

Quoted 3 lines

arch/arm64/Kconfig.platforms +3 -0

+config ARCH_MSM8996
+    bool "Enable Support for Qualcomm Technologies, Inc. MSM8996"
+       depends on ARCH_QCOM

Is this really the proposed approach? Doesn't the PMU code use these accessors? I would think the accessors are compiled if ARCH_QCOM=y so that the PMU stuff keeps working. Drop this Kconfig addition?

Quoted 10 lines

arch/arm64/Kconfig.platforms +4 -0

+       help
+       This enables support for the MSM8996 chipset. If you do not
+       wish to build a kernel that runs on this chipset, say 'N' here.
+
 config ARCH_ROCKCHIP
        bool "Rockchip Platforms"
        select ARCH_HAS_RESET_CONTROLLER diff --git

a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c index 4fdc848..8cece9e 100644

drivers/perf/qcom_l2_pmu.c line 1

Quoted 5 lines

drivers/perf/qcom_l2_pmu.c +1 -1

@@ -1,4 +1,4 @@
-/* Copyright (c) 2015-2017 The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2018 The Linux Foundation. All rights reserved.

Sorry, this makes little sense. First off, code is only being deleted and more than half the file isn't being modified so why update copyright dates?

drivers/soc/qcom/Makefile obj-$(CONFIG_QCOM_SMEM_STATE) +=

Quoted 9 lines

drivers/perf/qcom_l2_pmu.c +0 -0

  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and

drivers/soc/qcom/Makefile +0 -0

index dcebf28..4e91e2a 100644
@@ -12,3 +12,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) +=

smem_state.o

Quoted 18 lines

drivers/soc/qcom/Makefile +1 -0

 obj-$(CONFIG_QCOM_SMP2P)       += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)        += smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_ARCH_MSM8996) +=  kryo-l2-accessors.o

drivers/soc/qcom/kryo-l2-accessors.c new file +7 -0

new file mode 100644
index 0000000..6743848
@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.

drivers/soc/qcom/kryo-l2-accessors.c line 12

Quoted 6 lines

drivers/soc/qcom/kryo-l2-accessors.c new file +6 -0

+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */

SPDX tags?

Quoted 18 lines

include/soc/qcom/kryo-l2-accessors.h new file +10 -0

new file mode 100644
index 0000000..e9a5eab
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

include/soc/qcom/kryo-l2-accessors.h line 15

Quoted 6 lines

include/soc/qcom/kryo-l2-accessors.h new file +6 -0

+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ASM_ARCH_MSM_MSM_KRYO_L2_ACCESSORS_H
+#define __ASM_ARCH_MSM_MSM_KRYO_L2_ACCESSORS_H

Maybe change this tO __SOC_ARCH_QCOM_KRYO_L2_ACCESSORS_H

include/soc/qcom/kryo-l2-accessors.h

Quoted 5 lines

include/soc/qcom/kryo-l2-accessors.h new file +4 -0

+
+#ifdef CONFIG_ARCH_QCOM
+void set_l2_indirect_reg(u64 reg_addr, u64 val);
+u64 get_l2_indirect_reg(u64 reg_addr); #endif

This ifdef isn't doing much. Drop it? Or provide the inline nop alternatives.

From: Rajendra Nayak <[email protected]>

Allow clk_alpha_pll_configure to be called from loadable kernel modules.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters (Power and Perf) on msm8996 are clocked via 2 PLLs, a primary and alternate. There are also 2 Mux'es, a primary and secondary all connected together as shown below

                             +-------+
              XO             |       |
          +------------------>0      |
                             |       |
                   PLL/2     | SMUX  +----+
                     +------->1      |    |
                     |       |       |    |
                     |       +-------+    |    +-------+
                     |                    +---->0      |
                     |                         |       |
+---------------+    |             +----------->1      | CPU clk
|Primary PLL    +----+ PLL_EARLY   |           |       +------>

|               +------+-----------+    +------>2 PMUX |
+---------------+      |                |      |       |
                       |   +------+     |   +-->3      |
                       +--^+  ACD +-----+   |  +-------+
+---------------+          +------+         |
|Alt PLL        |                           |
|               +---------------------------+
+---------------+         PLL_EARLY

The primary PLL is what drives the CPU clk, except for times when we are reprogramming the PLL itself (for rate changes) when we temporarily switch to an alternate PLL. A subsequent patch adds support to switch between primary and alternate PLL during rate changes.

The primary PLL operates on a single VCO range, between 600Mhz and 3Ghz. However the CPUs do support OPPs with frequencies between 300Mhz and 600Mhz. In order to support running the CPUs at those frequencies we end up having to lock the PLL at twice the rate and drive the CPU clk via the PLL/2 output and SMUX.

So for frequencies above 600Mhz we follow the following path
Primary PLL --> PLL_EARLY --> PMUX(1) --> CPU clk
and for frequencies between 300Mhz and 600Mhz we follow

Primary PLL --> PLL/2 --> SMUX(1) --> PMUX(0) --> CPU clk
Support for this is added in a subsequent patch as well.

ACD stands for Adaptive Clock Distribution and is used to detect voltage droops. We do not add support for ACD as yet. This can be added at a later point as needed.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/Kconfig        |   8 +
 drivers/clk/qcom/Makefile       |   1 +
 drivers/clk/qcom/clk-cpu-8996.c | 409 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 418 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-cpu-8996.c

drivers/clk/qcom/Kconfig +0 -0

index fbf4532..3274877 100644
@@ -226,3 +226,11 @@ config SPMI_PMIC_CLKDIV
          Technologies, Inc. SPMI PMIC. It configures the frequency of

drivers/clk/qcom/Makefile +0 -0

 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o

drivers/clk/qcom/clk-cpu-8996.c new file +2 -0

new file mode 100644
index 0000000..42489f1
@@ -0,0 +1,409 @@
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.

drivers/clk/qcom/clk-cpu-8996.c new file +94 -0

+       [PLL_OFF_TEST_CTL] = 0x20,
+       [PLL_OFF_TEST_CTL_U] = 0x24,
+       [PLL_OFF_STATUS] = 0x28,
+};
+
+static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
+       [PLL_OFF_L_VAL] = 0x04,
+       [PLL_OFF_ALPHA_VAL] = 0x08,
+       [PLL_OFF_ALPHA_VAL_U] = 0x0c,
+       [PLL_OFF_USER_CTL] = 0x10,
+       [PLL_OFF_USER_CTL_U] = 0x14,
+       [PLL_OFF_CONFIG_CTL] = 0x18,
+       [PLL_OFF_TEST_CTL] = 0x20,
+       [PLL_OFF_TEST_CTL_U] = 0x24,
+       [PLL_OFF_STATUS] = 0x28,
+};
+
+/* PLLs */
+
+static const struct alpha_pll_config hfpll_config = {
+       .l = 60,
+       .config_ctl_val = 0x200d4828,
+       .config_ctl_hi_val = 0x006,
+       .pre_div_mask = BIT(12),
+       .post_div_mask = 0x3 << 8,
+       .main_output_mask = BIT(0),
+       .early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll perfcl_pll = {
+       .offset = 0x80000,
+       .regs = prim_pll_regs,
+       .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "perfcl_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_huayra_ops,
+       },
+};
+
+static struct clk_alpha_pll pwrcl_pll = {
+       .offset = 0x0,
+       .regs = prim_pll_regs,
+       .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "pwrcl_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_huayra_ops,
+       },
+};
+
+static const struct pll_vco alt_pll_vco_modes[] = {
+       VCO(3,  250000000,  500000000),
+       VCO(2,  500000000,  750000000),
+       VCO(1,  750000000, 1000000000),
+       VCO(0, 1000000000, 2150400000),
+};
+
+static const struct alpha_pll_config altpll_config = {
+       .l = 16,
+       .vco_val = 0x3 << 20,
+       .vco_mask = 0x3 << 20,
+       .config_ctl_val = 0x4001051b,
+       .post_div_mask = 0x3 << 8,
+       .post_div_val = 0x1,
+       .main_output_mask = BIT(0),
+       .early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll perfcl_alt_pll = {
+       .offset = 0x80100,
+       .regs = alt_pll_regs,
+       .vco_table = alt_pll_vco_modes,
+       .num_vco = ARRAY_SIZE(alt_pll_vco_modes),
+       .flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "perfcl_alt_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_hwfsm_ops,
+       },
+};
+
+static struct clk_alpha_pll pwrcl_alt_pll = {
+       .offset = 0x100,
+       .regs = alt_pll_regs,
+       .vco_table = alt_pll_vco_modes,
+       .num_vco = ARRAY_SIZE(alt_pll_vco_modes),
+       .flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "pwrcl_alt_pll",
+               .parent_names = (const char *[]){ "xo" },

drivers/clk/qcom/clk-cpu-8996.c new file +13 -0

+       struct clk_hw   *pll;
+       struct clk_regmap clkr;
+};
+
+static inline
+struct clk_cpu_8996_mux *to_clk_cpu_8996_mux_hw(struct clk_hw *hw)
+{
+       return container_of(to_clk_regmap(hw), struct clk_cpu_8996_mux, clkr);
+}
+
+static u8 clk_cpu_8996_mux_get_parent(struct clk_hw *hw)
+{
+       unsigned int val;

drivers/clk/qcom/clk-cpu-8996.c new file +9 -0

+       struct clk_regmap *clkr = to_clk_regmap(hw);
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       unsigned int mask = GENMASK(cpuclk->width + cpuclk->shift - 1,
+                                   cpuclk->shift);
+
+       val = index;
+       val <<= cpuclk->shift;
+
+       return regmap_update_bits(clkr->regmap, cpuclk->reg, mask, val);

drivers/clk/qcom/clk-cpu-8996.c new file +9 -0

+
+       return 0;
+}
+
+const struct clk_ops clk_cpu_8996_mux_ops = {
+       .set_parent = clk_cpu_8996_mux_set_parent,
+       .get_parent = clk_cpu_8996_mux_get_parent,
+       .determine_rate = clk_cpu_8996_mux_determine_rate,
+};

drivers/clk/qcom/clk-cpu-8996.c new file +26 -0

+
+static struct clk_cpu_8996_mux pwrcl_pmux = {
+       .reg = 0x40,
+       .shift = 0,
+       .width = 2,
+       .pll = &pwrcl_pll.clkr.hw,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "pwrcl_pmux",
+               .parent_names = (const char *[]){
+                       "pwrcl_smux",
+                       "pwrcl_pll",
+                       "pwrcl_pll_acd",
+                       "pwrcl_alt_pll",
+               },
+               .num_parents = 4,
+               .ops = &clk_cpu_8996_mux_ops,
+               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
+       },
+};
+
+static struct clk_cpu_8996_mux perfcl_pmux = {
+       .reg = 0x80040,
+       .shift = 0,
+       .width = 2,
+       .pll = &perfcl_pll.clkr.hw,
+       .clkr.hw.init = &(struct clk_init_data) {

+ { .compatible = "qcom-msm8996-apcc" },

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+       {}
+};
+
+struct clk_regmap *clks[] = {

drivers/clk/qcom/clk-cpu-8996.c new file +8 -0

+
+static int
+qcom_cpu_clk_msm8996_register_clks(struct device *dev, struct clk_hw_clks *hws,
+                                  struct regmap *regmap)
+{
+       int i, ret;
+
+       hws->hws[0] = clk_hw_register_fixed_factor(dev, "perfcl_pll_main",

drivers/clk/qcom/clk-cpu-8996.c new file +11 -0

+
+       for (i = 0; i < ARRAY_SIZE(clks); i++) {
+               ret = devm_clk_register_regmap(dev, clks[i]);
+               if (ret)
+                       return ret;
+       }
+
+       clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
+       clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
+       clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
+       clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);

drivers/clk/qcom/clk-cpu-8996.c new file +39 -0

+       struct clk_hw_clks *hws;
+       struct clk_hw_onecell_data *data;
+       struct device *dev = &pdev->dev;
+       struct device_node *node = dev->of_node;
+
+       data = devm_kzalloc(dev, sizeof(*data) + 2 * sizeof(struct clk_hw *),
+                           GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       hws = devm_kzalloc(dev, sizeof(*hws) + 2 * sizeof(struct clk_hw *),
+                          GFP_KERNEL);
+       if (!hws)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(base))
+               return PTR_ERR(base);
+
+       regmap_cpu = devm_regmap_init_mmio(dev, base,
+                                          &cpu_msm8996_regmap_config);
+       if (IS_ERR(regmap_cpu))
+               return PTR_ERR(regmap_cpu);
+
+       ret = qcom_cpu_clk_msm8996_register_clks(dev, hws, regmap_cpu);
+       if (ret)
+               return ret;
+
+       data->hws[0] = &pwrcl_pmux.clkr.hw;
+       data->hws[1] = &perfcl_pmux.clkr.hw;
+
+       data->num = 2;
+
+       platform_set_drvdata(pdev, hws);
+
+       return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, data);
+}
+

drivers/clk/qcom/clk-cpu-8996.c new file +7 -0

+
+       return 0;
+}
+
+static struct platform_driver qcom_cpu_clk_msm8996_driver = {
+       .probe = qcom_cpu_clk_msm8996_driver_probe,
+       .remove = qcom_cpu_clk_msm8996_driver_remove,

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 19:37
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 03/10] clk: qcom: Add CPU clock driver for msm8996

Quoting Ilia Lin (2018-02-14 05:59:45)

Quoted 19 lines

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters (Power and Perf) on msm8996 are clocked via 2 PLLs, a primary and alternate. There are also
2 Mux'es, a primary and secondary all connected together as shown below

                             +-------+
              XO             |       |
          +------------------>0      |
                             |       |
                   PLL/2     | SMUX  +----+
                     +------->1      |    |
                     |       |       |    |
                     |       +-------+    |    +-------+
                     |                    +---->0      |
                     |                         |       |
+---------------+    |             +----------->1      | CPU clk
|Primary PLL    +----+ PLL_EARLY   |           |       +------>

Quoted 8 lines

|               +------+-----------+    +------>2 PMUX |
+---------------+      |                |      |       |
                       |   +------+     |   +-->3      |
                       +--^+  ACD +-----+   |  +-------+
+---------------+          +------+         |
|Alt PLL        |                           |
|               +---------------------------+
+---------------+         PLL_EARLY

Thanks for the diagram. Please also put it at the top of the driver file.

Quoted 10 lines

The primary PLL is what drives the CPU clk, except for times when we are reprogramming the PLL itself (for rate changes) when we temporarily switch to an alternate PLL. A subsequent patch adds support to switch between primary and alternate PLL during rate changes.

The primary PLL operates on a single VCO range, between 600Mhz and 3Ghz. However the CPUs do support OPPs with frequencies between

300Mhz

Quoted 4 lines

and 600Mhz. In order to support running the CPUs at those frequencies we end up having to lock the PLL at twice the rate and drive the CPU clk via the PLL/2 output and SMUX.

Quoted 3 lines

So for frequencies above 600Mhz we follow the following path Primary PLL --> PLL_EARLY --> PMUX(1) --> CPU clk and for frequencies between 300Mhz and 600Mhz we follow

Capitalize 'h' in units please.

Quoted 28 lines

Primary PLL --> PLL/2 --> SMUX(1) --> PMUX(0) --> CPU clk Support for
this is added in a subsequent patch as well.

ACD stands for Adaptive Clock Distribution and is used to detect voltage droops. We do not add support for ACD as yet. This can be added at a later point as needed.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/Kconfig        |   8 +
 drivers/clk/qcom/Makefile       |   1 +
 drivers/clk/qcom/clk-cpu-8996.c | 409
++++++++++++++++++++++++++++++++++++++++
 3 files changed, 418 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-cpu-8996.c

drivers/clk/qcom/Kconfig index renamed from drivers/clk/qcom/Kconfig +0 -0

fbf4532..3274877 100644

drivers/clk/qcom/Kconfig +2 -0

@@ -226,3 +226,11 @@ config SPMI_PMIC_CLKDIV
          Technologies, Inc. SPMI PMIC. It configures the frequency of
          clkdiv outputs of the PMIC. These clocks are typically wired
          through alternate functions on GPIO pins.
+
+config MSM_APCC_8996

drivers/clk/qcom/Kconfig config SPMI_PMIC_CLKDIV · line 236

Quoted 6 lines

drivers/clk/qcom/Kconfig +6 -0

+       tristate "MSM8996 CPU Clock Controller"
+       depends on COMMON_CLK_QCOM
+       help
+         Support for the CPU clock controller on msm8996 devices.
+         Say Y if you want to support CPU clock scaling using CPUfreq
+         drivers for dyanmic power management.

Sort?

drivers/clk/qcom/Makefile obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o · line 36

Quoted 10 lines

drivers/clk/qcom/Makefile +1 -0

index 230332c..57b38ba 100644
@@ -33,6 +33,7 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.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
+obj-$(CONFIG_MSM_APCC_8996) += clk-cpu-8996.o

This doesn't look sorted.

Quoted 16 lines

drivers/clk/qcom/Makefile +0 -0

 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o diff --git

a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c new file mode 100644 index 0000000..42489f1

drivers/clk/qcom/clk-cpu-8996.c new file +7 -0

@@ -0,0 +1,409 @@
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.

drivers/clk/qcom/clk-cpu-8996.c line 12

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */

Can we get the SPDX tags here please?

drivers/clk/qcom/clk-cpu-8996.c line 14

Quoted 3 lines

drivers/clk/qcom/clk-cpu-8996.c new file +2 -0

+
+#include <linux/clk.h>

clk-provider.h?

drivers/clk/qcom/clk-cpu-8996.c line 19

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c new file +5 -0

+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "clk-alpha-pll.h"

#include "clk-regmap.h"

drivers/clk/qcom/clk-cpu-8996.c line 25

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+
+#define VCO(a, b, c) { \
+       .val = a,\
+       .min_freq = b,\
+       .max_freq = c,\
+}

Can this define go into whatever PLL header file defines the struct?

drivers/clk/qcom/clk-cpu-8996.c line 37

Quoted 13 lines

drivers/clk/qcom/clk-cpu-8996.c new file +12 -0

+
+#define DIV_2_INDEX            0
+#define PLL_INDEX              1
+#define ACD_INDEX              2
+#define ALT_INDEX              3
+
+static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
+       [PLL_OFF_L_VAL] = 0x04,
+       [PLL_OFF_ALPHA_VAL] = 0x08,
+       [PLL_OFF_USER_CTL] = 0x10,
+       [PLL_OFF_CONFIG_CTL] = 0x18,
+       [PLL_OFF_CONFIG_CTL_U] = 0x1C,

Please use lowercase hex throughout. Consistency is key!

Quoted 98 lines

drivers/clk/qcom/clk-cpu-8996.c new file +97 -0

+       [PLL_OFF_TEST_CTL] = 0x20,
+       [PLL_OFF_TEST_CTL_U] = 0x24,
+       [PLL_OFF_STATUS] = 0x28,
+};
+
+static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
+       [PLL_OFF_L_VAL] = 0x04,
+       [PLL_OFF_ALPHA_VAL] = 0x08,
+       [PLL_OFF_ALPHA_VAL_U] = 0x0c,
+       [PLL_OFF_USER_CTL] = 0x10,
+       [PLL_OFF_USER_CTL_U] = 0x14,
+       [PLL_OFF_CONFIG_CTL] = 0x18,
+       [PLL_OFF_TEST_CTL] = 0x20,
+       [PLL_OFF_TEST_CTL_U] = 0x24,
+       [PLL_OFF_STATUS] = 0x28,
+};
+
+/* PLLs */
+
+static const struct alpha_pll_config hfpll_config = {
+       .l = 60,
+       .config_ctl_val = 0x200d4828,
+       .config_ctl_hi_val = 0x006,
+       .pre_div_mask = BIT(12),
+       .post_div_mask = 0x3 << 8,
+       .main_output_mask = BIT(0),
+       .early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll perfcl_pll = {
+       .offset = 0x80000,
+       .regs = prim_pll_regs,
+       .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "perfcl_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_huayra_ops,
+       },
+};
+
+static struct clk_alpha_pll pwrcl_pll = {
+       .offset = 0x0,
+       .regs = prim_pll_regs,
+       .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "pwrcl_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_huayra_ops,
+       },
+};
+
+static const struct pll_vco alt_pll_vco_modes[] = {
+       VCO(3,  250000000,  500000000),
+       VCO(2,  500000000,  750000000),
+       VCO(1,  750000000, 1000000000),
+       VCO(0, 1000000000, 2150400000), };
+
+static const struct alpha_pll_config altpll_config = {
+       .l = 16,
+       .vco_val = 0x3 << 20,
+       .vco_mask = 0x3 << 20,
+       .config_ctl_val = 0x4001051b,
+       .post_div_mask = 0x3 << 8,
+       .post_div_val = 0x1,
+       .main_output_mask = BIT(0),
+       .early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll perfcl_alt_pll = {
+       .offset = 0x80100,
+       .regs = alt_pll_regs,
+       .vco_table = alt_pll_vco_modes,
+       .num_vco = ARRAY_SIZE(alt_pll_vco_modes),
+       .flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "perfcl_alt_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_hwfsm_ops,
+       },
+};
+
+static struct clk_alpha_pll pwrcl_alt_pll = {
+       .offset = 0x100,
+       .regs = alt_pll_regs,
+       .vco_table = alt_pll_vco_modes,
+       .num_vco = ARRAY_SIZE(alt_pll_vco_modes),
+       .flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "pwrcl_alt_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_hwfsm_ops,
+       },
+};

drivers/clk/qcom/clk-cpu-8996.c line 141

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+
+/* Mux'es */
+
+struct clk_cpu_8996_mux {
+       u32     reg;
+       u32     shift;

u8 shift?

drivers/clk/qcom/clk-cpu-8996.c line 142

Quoted 2 lines

drivers/clk/qcom/clk-cpu-8996.c new file +1 -0

+       u32     width;

u8 width?

drivers/clk/qcom/clk-cpu-8996.c

Quoted 7 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+       struct clk_hw   *pll;
+       struct clk_regmap clkr;
+};
+
+static inline
+struct clk_cpu_8996_mux *to_clk_cpu_8996_mux_hw(struct clk_hw

*hw) {

Quoted 9 lines

drivers/clk/qcom/clk-cpu-8996.c new file +9 -0

+       return container_of(to_clk_regmap(hw), struct
+clk_cpu_8996_mux, clkr); }
+
+static u8 clk_cpu_8996_mux_get_parent(struct clk_hw *hw) {
+       unsigned int val;
+       struct clk_regmap *clkr = to_clk_regmap(hw);
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       unsigned int mask = GENMASK(cpuclk->width - 1, 0);
+

drivers/clk/qcom/clk-cpu-8996.c line 165

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+       regmap_read(clkr->regmap, cpuclk->reg, &val);
+
+       val >>= cpuclk->shift;
+       val &= mask;
+
+       return val;

return val & mask;

drivers/clk/qcom/clk-cpu-8996.c line 155

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+}
+
+static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index) {
+       unsigned int val;

u32 val?

Quoted 14 lines

drivers/clk/qcom/clk-cpu-8996.c new file +13 -0

+       struct clk_regmap *clkr = to_clk_regmap(hw);
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       unsigned int mask = GENMASK(cpuclk->width + cpuclk->shift - 1,
+                                   cpuclk->shift);
+
+       val = index;
+       val <<= cpuclk->shift;
+
+       return regmap_update_bits(clkr->regmap, cpuclk->reg, mask,
+val); }
+
+static int
+clk_cpu_8996_mux_determine_rate(struct clk_hw *hw, struct

drivers/clk/qcom/clk-cpu-8996.c line 189

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+clk_rate_request *req) {
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       struct clk_hw *parent = cpuclk->pll;
+
+       if (!cpuclk->pll)
+               return -EINVAL;

Does this happen?

drivers/clk/qcom/clk-cpu-8996.c line 192

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

+
+       req->best_parent_rate = clk_hw_round_rate(parent, req->rate);
+       req->best_parent_hw = parent;

Is the parent index always the same? Perhaps just call clk_hw_get_parent_by_index() then instead of adding a pointer to the clk_cpu_8996_mux.

Quoted 8 lines

drivers/clk/qcom/clk-cpu-8996.c new file +8 -0

+
+       return 0;
+}
+
+const struct clk_ops clk_cpu_8996_mux_ops = {
+       .set_parent = clk_cpu_8996_mux_set_parent,
+       .get_parent = clk_cpu_8996_mux_get_parent,
+       .determine_rate = clk_cpu_8996_mux_determine_rate, };

[...]

Quoted 31 lines

drivers/clk/qcom/clk-cpu-8996.c new file +31 -0

+
+static struct clk_cpu_8996_mux pwrcl_pmux = {
+       .reg = 0x40,
+       .shift = 0,
+       .width = 2,
+       .pll = &pwrcl_pll.clkr.hw,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "pwrcl_pmux",
+               .parent_names = (const char *[]){
+                       "pwrcl_smux",
+                       "pwrcl_pll",
+                       "pwrcl_pll_acd",
+                       "pwrcl_alt_pll",
+               },
+               .num_parents = 4,
+               .ops = &clk_cpu_8996_mux_ops,
+               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
+       },
+};
+
+static struct clk_cpu_8996_mux perfcl_pmux = {
+       .reg = 0x80040,
+       .shift = 0,
+       .width = 2,
+       .pll = &perfcl_pll.clkr.hw,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "perfcl_pmux",
+               .parent_names = (const char *[]){
+                       "perfcl_smux",
+                       "perfcl_pll",
+                       "perfcl_pll_acd",

drivers/clk/qcom/clk-cpu-8996.c line 269

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c new file +5 -0

+                       "perfcl_alt_pll",
+               },
+               .num_parents = 4,
+               .ops = &clk_cpu_8996_mux_ops,
+               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,

Not sure CLK_IS_CRITICAL is the right mode. Perhaps CLK_IGNORE_UNUSED for now? We don't want to force XO to stay on forever here.

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

+       },
+};
+

drivers/clk/qcom/clk-cpu-8996.c line 282

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+static const struct regmap_config cpu_msm8996_regmap_config = {
+       .reg_bits               = 32,
+       .reg_stride             = 4,
+       .val_bits               = 32,
+       .max_register           = 0x80210,
+       .fast_io                = true,
+       .val_format_endian      = REGMAP_ENDIAN_LITTLE,
+};
+
+static const struct of_device_id match_table[] = {

Move this next to driver structure and give it a more specific name.

Quoted 2 lines

+ { .compatible = "qcom-msm8996-apcc" },

Bad compatible? Should be qcom,msm8996-apcc?

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+       {}
+};
+
+struct clk_regmap *clks[] = {

drivers/clk/qcom/clk-cpu-8996.c line 297

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+       /* PLLs */
+       &perfcl_pll.clkr,
+       &pwrcl_pll.clkr,
+       &perfcl_alt_pll.clkr,
+       &pwrcl_alt_pll.clkr,
+       /* MUXs */
+       &perfcl_smux.clkr,
+       &pwrcl_smux.clkr,
+       &perfcl_pmux.clkr,
+       &pwrcl_pmux.clkr,

Please drop useless comments inside this array.

drivers/clk/qcom/clk-cpu-8996.c line 303

Quoted 7 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+};
+
+struct clk_hw_clks {
+       unsigned int num;
+       struct clk_hw *hws[];
+};

Please just NULL terminate the list of clk_hw pointers, or keep the size around instead.

drivers/clk/qcom/clk-cpu-8996.c

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

+
+static int
+qcom_cpu_clk_msm8996_register_clks(struct device *dev, struct

clk_hw_clks *hws,

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+                                  struct regmap *regmap) {
+       int i, ret;
+
+       hws->hws[0] = clk_hw_register_fixed_factor(dev, "perfcl_pll_main",
+                                                  "perfcl_pll",
+                                                  CLK_SET_RATE_PARENT, 1, 2);
+       perfcl_smux.pll = hws->hws[0];
+
+       hws->hws[1] = clk_hw_register_fixed_factor(dev, "pwrcl_pll_main",
+                                                  "pwrcl_pll",

drivers/clk/qcom/clk-cpu-8996.c line 321

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+                                                  CLK_SET_RATE_PARENT, 1, 2);
+       pwrcl_smux.pll = hws->hws[1];
+
+       hws->num = 2;

Maybe just open code the fixed factor clk registration? Then the devm_clk_hw_register() function can be used on those static structs to make removal simpler.

Quoted 17 lines

drivers/clk/qcom/clk-cpu-8996.c new file +16 -0

+
+       for (i = 0; i < ARRAY_SIZE(clks); i++) {
+               ret = devm_clk_register_regmap(dev, clks[i]);
+               if (ret)
+                       return ret;
+       }
+
+       clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
+       clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
+       clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
+       clk_alpha_pll_configure(&pwrcl_alt_pll, regmap,
+ &altpll_config);
+
+       return ret;
+}
+

drivers/clk/qcom/clk-cpu-8996.c line 342

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device
+*pdev) {
+       int ret;
+       void __iomem *base;
+       struct resource *res;
+       struct regmap *regmap_cpu;

Just call it regmap please.

Quoted 31 lines

drivers/clk/qcom/clk-cpu-8996.c new file +30 -0

+       struct clk_hw_clks *hws;
+       struct clk_hw_onecell_data *data;
+       struct device *dev = &pdev->dev;
+       struct device_node *node = dev->of_node;
+
+       data = devm_kzalloc(dev, sizeof(*data) + 2 * sizeof(struct clk_hw *),
+                           GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       hws = devm_kzalloc(dev, sizeof(*hws) + 2 * sizeof(struct clk_hw *),
+                          GFP_KERNEL);
+       if (!hws)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(base))
+               return PTR_ERR(base);
+
+       regmap_cpu = devm_regmap_init_mmio(dev, base,
+                                          &cpu_msm8996_regmap_config);
+       if (IS_ERR(regmap_cpu))
+               return PTR_ERR(regmap_cpu);
+
+       ret = qcom_cpu_clk_msm8996_register_clks(dev, hws, regmap_cpu);
+       if (ret)
+               return ret;
+
+       data->hws[0] = &pwrcl_pmux.clkr.hw;

drivers/clk/qcom/clk-cpu-8996.c

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+       data->hws[1] = &perfcl_pmux.clkr.hw;
+
+       data->num = 2;
+
+       platform_set_drvdata(pdev, hws);
+
+       return of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
+data); }
+
+static int qcom_cpu_clk_msm8996_driver_remove(struct

platform_device

drivers/clk/qcom/clk-cpu-8996.c line 391

Quoted 9 lines

drivers/clk/qcom/clk-cpu-8996.c new file +9 -0

+*pdev) {
+       int i;
+       struct device *dev = &pdev->dev;
+       struct clk_hw_clks *hws = platform_get_drvdata(pdev);
+
+       for (i = 0; i < hws->num; i++)
+               clk_hw_unregister_fixed_rate(hws->hws[i]);
+
+       of_clk_del_provider(dev->of_node);

Use devm_of_clk_add_hw_provider() instead.

Quoted 12 lines

drivers/clk/qcom/clk-cpu-8996.c new file +11 -0

+
+       return 0;
+}
+
+static struct platform_driver qcom_cpu_clk_msm8996_driver = {
+       .probe = qcom_cpu_clk_msm8996_driver_probe,
+       .remove = qcom_cpu_clk_msm8996_driver_remove,
+       .driver = {
+               .name = "qcom-msm8996-apcc",
+               .of_match_table = match_table,
+       },

drivers/clk/qcom/clk-cpu-8996.c line 408

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+};
+
+module_platform_driver(qcom_cpu_clk_msm8996_driver);
+
+MODULE_ALIAS("platform:msm8996-apcc");
+MODULE_DESCRIPTION("QCOM MSM8996 CPU clock Driver");

Not sure why Driver is capitalized and clock is not.

Quoted 15 lines

From: Stephen Boyd <[email protected]>

Quoted 13 lines

drivers/clk/qcom/clk-cpu-8996.c new file +13 -0

+       struct clk_regmap *clkr = to_clk_regmap(hw);
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       unsigned int mask = GENMASK(cpuclk->width + cpuclk->shift - 1,
+                                   cpuclk->shift);
+
+       val = index;
+       val <<= cpuclk->shift;
+
+       return regmap_update_bits(clkr->regmap, cpuclk->reg, mask,
+val); }
+
+static int
+clk_cpu_8996_mux_determine_rate(struct clk_hw *hw, struct

Quoted 8 lines

drivers/clk/qcom/clk-cpu-8996.c line 189

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+clk_rate_request *req) {
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       struct clk_hw *parent = cpuclk->pll;
+
+       if (!cpuclk->pll)
+               return -EINVAL;

Does this happen?

On successful probe it shouldn't. May I omit pointer checks in this case? Future maintenance ma y change the flow.

Quoted 11 lines

drivers/clk/qcom/clk-cpu-8996.c line 192

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

+
+       req->best_parent_rate = clk_hw_round_rate(parent, req->rate);
+       req->best_parent_hw = parent;

Is the parent index always the same? Perhaps just call clk_hw_get_parent_by_index() then instead of adding a pointer to the clk_cpu_8996_mux.

Quoted 15 lines

drivers/clk/qcom/clk-cpu-8996.c line 282

Quoted 13 lines

drivers/clk/qcom/clk-cpu-8996.c new file +13 -0

+       },
+};
+
+static const struct regmap_config cpu_msm8996_regmap_config = {
+       .reg_bits               = 32,
+       .reg_stride             = 4,
+       .val_bits               = 32,
+       .max_register           = 0x80210,
+       .fast_io                = true,
+       .val_format_endian      = REGMAP_ENDIAN_LITTLE,
+};
+
+static const struct of_device_id match_table[] = {

Move this next to driver structure and give it a more specific name.

Will be changed in the next spin.

Quoted 4 lines

Quoted 2 lines

+ { .compatible = "qcom-msm8996-apcc" },

Bad compatible? Should be qcom,msm8996-apcc?

The naming is per Rob Herring's requirement.

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 19:37
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 03/10] clk: qcom: Add CPU clock driver for msm8996

Quoting Ilia Lin (2018-02-14 05:59:45)

Quoted 19 lines

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters (Power and Perf) on msm8996 are clocked via 2 PLLs, a primary and alternate. There are also
2 Mux'es, a primary and secondary all connected together as shown below

                             +-------+
              XO             |       |
          +------------------>0      |
                             |       |
                   PLL/2     | SMUX  +----+
                     +------->1      |    |
                     |       |       |    |
                     |       +-------+    |    +-------+
                     |                    +---->0      |
                     |                         |       |
+---------------+    |             +----------->1      | CPU clk
|Primary PLL    +----+ PLL_EARLY   |           |       +------>

Quoted 8 lines

|               +------+-----------+    +------>2 PMUX |
+---------------+      |                |      |       |
                       |   +------+     |   +-->3      |
                       +--^+  ACD +-----+   |  +-------+
+---------------+          +------+         |
|Alt PLL        |                           |
|               +---------------------------+
+---------------+         PLL_EARLY

Thanks for the diagram. Please also put it at the top of the driver file.

Quoted 9 lines

The primary PLL is what drives the CPU clk, except for times when we are reprogramming the PLL itself (for rate changes) when we temporarily switch to an alternate PLL. A subsequent patch adds support to switch between primary and alternate PLL during rate changes.

The primary PLL operates on a single VCO range, between 600Mhz and 3Ghz. However the CPUs do support OPPs with frequencies between

300Mhz

Quoted 7 lines

and 600Mhz. In order to support running the CPUs at those frequencies we end up having to lock the PLL at twice the rate and drive the CPU clk via the PLL/2 output and SMUX.

So for frequencies above 600Mhz we follow the following path Primary PLL --> PLL_EARLY --> PMUX(1) --> CPU clk and for frequencies between 300Mhz and 600Mhz we follow

Capitalize 'h' in units please.

Quoted 23 lines

Primary PLL --> PLL/2 --> SMUX(1) --> PMUX(0) --> CPU clk Support for
this is added in a subsequent patch as well.

ACD stands for Adaptive Clock Distribution and is used to detect voltage droops. We do not add support for ACD as yet. This can be added at a later point as needed.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/Kconfig        |   8 +
 drivers/clk/qcom/Makefile       |   1 +
 drivers/clk/qcom/clk-cpu-8996.c | 409
++++++++++++++++++++++++++++++++++++++++
 3 files changed, 418 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-cpu-8996.c

drivers/clk/qcom/Kconfig index renamed from drivers/clk/qcom/Kconfig +0 -0

fbf4532..3274877 100644

drivers/clk/qcom/Kconfig +0 -0

@@ -226,3 +226,11 @@ config SPMI_PMIC_CLKDIV
          Technologies, Inc. SPMI PMIC. It configures the frequency of

drivers/clk/qcom/Kconfig config SPMI_PMIC_CLKDIV · line 236

Quoted 10 lines

drivers/clk/qcom/Kconfig +8 -0

          clkdiv outputs of the PMIC. These clocks are typically wired
          through alternate functions on GPIO pins.
+
+config MSM_APCC_8996
+       tristate "MSM8996 CPU Clock Controller"
+       depends on COMMON_CLK_QCOM
+       help
+         Support for the CPU clock controller on msm8996 devices.
+         Say Y if you want to support CPU clock scaling using CPUfreq
+         drivers for dyanmic power management.

Sort?

drivers/clk/qcom/Makefile obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o · line 36

Quoted 9 lines

drivers/clk/qcom/Makefile +1 -0

index 230332c..57b38ba 100644
@@ -33,6 +33,7 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.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
+obj-$(CONFIG_MSM_APCC_8996) += clk-cpu-8996.o

This doesn't look sorted.

Quoted 11 lines

drivers/clk/qcom/Makefile +0 -0

 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o diff --git

a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c new file mode 100644 index 0000000..42489f1

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

@@ -0,0 +1,409 @@
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *

drivers/clk/qcom/clk-cpu-8996.c line 13

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */

Can we get the SPDX tags here please?

drivers/clk/qcom/clk-cpu-8996.c line 14

Quoted 2 lines

drivers/clk/qcom/clk-cpu-8996.c new file +2 -0

+
+#include <linux/clk.h>

clk-provider.h?

drivers/clk/qcom/clk-cpu-8996.c line 19

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c new file +5 -0

+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "clk-alpha-pll.h"

#include "clk-regmap.h"

drivers/clk/qcom/clk-cpu-8996.c line 25

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+
+#define VCO(a, b, c) { \
+       .val = a,\
+       .min_freq = b,\
+       .max_freq = c,\
+}

Can this define go into whatever PLL header file defines the struct?

drivers/clk/qcom/clk-cpu-8996.c line 37

Quoted 12 lines

drivers/clk/qcom/clk-cpu-8996.c new file +12 -0

+
+#define DIV_2_INDEX            0
+#define PLL_INDEX              1
+#define ACD_INDEX              2
+#define ALT_INDEX              3
+
+static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
+       [PLL_OFF_L_VAL] = 0x04,
+       [PLL_OFF_ALPHA_VAL] = 0x08,
+       [PLL_OFF_USER_CTL] = 0x10,
+       [PLL_OFF_CONFIG_CTL] = 0x18,
+       [PLL_OFF_CONFIG_CTL_U] = 0x1C,

Please use lowercase hex throughout. Consistency is key!

Quoted 93 lines

drivers/clk/qcom/clk-cpu-8996.c new file +93 -0

+       [PLL_OFF_TEST_CTL] = 0x20,
+       [PLL_OFF_TEST_CTL_U] = 0x24,
+       [PLL_OFF_STATUS] = 0x28,
+};
+
+static const u8 alt_pll_regs[PLL_OFF_MAX_REGS] = {
+       [PLL_OFF_L_VAL] = 0x04,
+       [PLL_OFF_ALPHA_VAL] = 0x08,
+       [PLL_OFF_ALPHA_VAL_U] = 0x0c,
+       [PLL_OFF_USER_CTL] = 0x10,
+       [PLL_OFF_USER_CTL_U] = 0x14,
+       [PLL_OFF_CONFIG_CTL] = 0x18,
+       [PLL_OFF_TEST_CTL] = 0x20,
+       [PLL_OFF_TEST_CTL_U] = 0x24,
+       [PLL_OFF_STATUS] = 0x28,
+};
+
+/* PLLs */
+
+static const struct alpha_pll_config hfpll_config = {
+       .l = 60,
+       .config_ctl_val = 0x200d4828,
+       .config_ctl_hi_val = 0x006,
+       .pre_div_mask = BIT(12),
+       .post_div_mask = 0x3 << 8,
+       .main_output_mask = BIT(0),
+       .early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll perfcl_pll = {
+       .offset = 0x80000,
+       .regs = prim_pll_regs,
+       .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "perfcl_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_huayra_ops,
+       },
+};
+
+static struct clk_alpha_pll pwrcl_pll = {
+       .offset = 0x0,
+       .regs = prim_pll_regs,
+       .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data){
+               .name = "pwrcl_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_huayra_ops,
+       },
+};
+
+static const struct pll_vco alt_pll_vco_modes[] = {
+       VCO(3,  250000000,  500000000),
+       VCO(2,  500000000,  750000000),
+       VCO(1,  750000000, 1000000000),
+       VCO(0, 1000000000, 2150400000), };
+
+static const struct alpha_pll_config altpll_config = {
+       .l = 16,
+       .vco_val = 0x3 << 20,
+       .vco_mask = 0x3 << 20,
+       .config_ctl_val = 0x4001051b,
+       .post_div_mask = 0x3 << 8,
+       .post_div_val = 0x1,
+       .main_output_mask = BIT(0),
+       .early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll perfcl_alt_pll = {
+       .offset = 0x80100,
+       .regs = alt_pll_regs,
+       .vco_table = alt_pll_vco_modes,
+       .num_vco = ARRAY_SIZE(alt_pll_vco_modes),
+       .flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "perfcl_alt_pll",
+               .parent_names = (const char *[]){ "xo" },
+               .num_parents = 1,
+               .ops = &clk_alpha_pll_hwfsm_ops,
+       },
+};
+
+static struct clk_alpha_pll pwrcl_alt_pll = {
+       .offset = 0x100,
+       .regs = alt_pll_regs,
+       .vco_table = alt_pll_vco_modes,
+       .num_vco = ARRAY_SIZE(alt_pll_vco_modes),
+       .flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "pwrcl_alt_pll",
+               .parent_names = (const char *[]){ "xo" },

drivers/clk/qcom/clk-cpu-8996.c line 141

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+               .num_parents = 1,
+               .ops = &clk_alpha_pll_hwfsm_ops,
+       },
+};
+
+/* Mux'es */
+
+struct clk_cpu_8996_mux {
+       u32     reg;
+       u32     shift;

u8 shift?

drivers/clk/qcom/clk-cpu-8996.c line 142

Quoted 1 line

drivers/clk/qcom/clk-cpu-8996.c new file +1 -0

+       u32     width;

u8 width?

drivers/clk/qcom/clk-cpu-8996.c

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+       struct clk_hw   *pll;
+       struct clk_regmap clkr;
+};
+
+static inline
+struct clk_cpu_8996_mux *to_clk_cpu_8996_mux_hw(struct clk_hw

*hw) {

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c new file +5 -0

+       return container_of(to_clk_regmap(hw), struct
+clk_cpu_8996_mux, clkr); }
+
+static u8 clk_cpu_8996_mux_get_parent(struct clk_hw *hw) {
+       unsigned int val;

drivers/clk/qcom/clk-cpu-8996.c line 165

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+       struct clk_regmap *clkr = to_clk_regmap(hw);
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       unsigned int mask = GENMASK(cpuclk->width - 1, 0);
+
+       regmap_read(clkr->regmap, cpuclk->reg, &val);
+
+       val >>= cpuclk->shift;
+       val &= mask;
+
+       return val;

return val & mask;

drivers/clk/qcom/clk-cpu-8996.c line 155

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+}
+
+static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index) {
+       unsigned int val;

u32 val?

Quoted 9 lines

drivers/clk/qcom/clk-cpu-8996.c new file +9 -0

+       struct clk_regmap *clkr = to_clk_regmap(hw);
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       unsigned int mask = GENMASK(cpuclk->width + cpuclk->shift - 1,
+                                   cpuclk->shift);
+
+       val = index;
+       val <<= cpuclk->shift;
+
+       return regmap_update_bits(clkr->regmap, cpuclk->reg, mask,

drivers/clk/qcom/clk-cpu-8996.c line 189

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+val); }
+
+static int
+clk_cpu_8996_mux_determine_rate(struct clk_hw *hw, struct
+clk_rate_request *req) {
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_hw(hw);
+       struct clk_hw *parent = cpuclk->pll;
+
+       if (!cpuclk->pll)
+               return -EINVAL;

Does this happen?

drivers/clk/qcom/clk-cpu-8996.c line 192

Quoted 3 lines

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

+
+       req->best_parent_rate = clk_hw_round_rate(parent, req->rate);
+       req->best_parent_hw = parent;

Is the parent index always the same? Perhaps just call clk_hw_get_parent_by_index() then instead of adding a pointer to the clk_cpu_8996_mux.

Quoted 8 lines

drivers/clk/qcom/clk-cpu-8996.c new file +8 -0

+
+       return 0;
+}
+
+const struct clk_ops clk_cpu_8996_mux_ops = {
+       .set_parent = clk_cpu_8996_mux_set_parent,
+       .get_parent = clk_cpu_8996_mux_get_parent,
+       .determine_rate = clk_cpu_8996_mux_determine_rate, };

[...]

Quoted 26 lines

drivers/clk/qcom/clk-cpu-8996.c new file +26 -0

+
+static struct clk_cpu_8996_mux pwrcl_pmux = {
+       .reg = 0x40,
+       .shift = 0,
+       .width = 2,
+       .pll = &pwrcl_pll.clkr.hw,
+       .clkr.hw.init = &(struct clk_init_data) {
+               .name = "pwrcl_pmux",
+               .parent_names = (const char *[]){
+                       "pwrcl_smux",
+                       "pwrcl_pll",
+                       "pwrcl_pll_acd",
+                       "pwrcl_alt_pll",
+               },
+               .num_parents = 4,
+               .ops = &clk_cpu_8996_mux_ops,
+               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
+       },
+};
+
+static struct clk_cpu_8996_mux perfcl_pmux = {
+       .reg = 0x80040,
+       .shift = 0,
+       .width = 2,
+       .pll = &perfcl_pll.clkr.hw,
+       .clkr.hw.init = &(struct clk_init_data) {

drivers/clk/qcom/clk-cpu-8996.c line 269

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+               .name = "perfcl_pmux",
+               .parent_names = (const char *[]){
+                       "perfcl_smux",
+                       "perfcl_pll",
+                       "perfcl_pll_acd",
+                       "perfcl_alt_pll",
+               },
+               .num_parents = 4,
+               .ops = &clk_cpu_8996_mux_ops,
+               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,

Not sure CLK_IS_CRITICAL is the right mode. Perhaps CLK_IGNORE_UNUSED for now? We don't want to force XO to stay on forever here.

drivers/clk/qcom/clk-cpu-8996.c line 282

Quoted 13 lines

drivers/clk/qcom/clk-cpu-8996.c new file +13 -0

+       },
+};
+
+static const struct regmap_config cpu_msm8996_regmap_config = {
+       .reg_bits               = 32,
+       .reg_stride             = 4,
+       .val_bits               = 32,
+       .max_register           = 0x80210,
+       .fast_io                = true,
+       .val_format_endian      = REGMAP_ENDIAN_LITTLE,
+};
+
+static const struct of_device_id match_table[] = {

Move this next to driver structure and give it a more specific name.

Quoted 1 line

+ { .compatible = "qcom-msm8996-apcc" },

Bad compatible? Should be qcom,msm8996-apcc?

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+       {}
+};
+
+struct clk_regmap *clks[] = {

drivers/clk/qcom/clk-cpu-8996.c line 297

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+       /* PLLs */
+       &perfcl_pll.clkr,
+       &pwrcl_pll.clkr,
+       &perfcl_alt_pll.clkr,
+       &pwrcl_alt_pll.clkr,
+       /* MUXs */
+       &perfcl_smux.clkr,
+       &pwrcl_smux.clkr,
+       &perfcl_pmux.clkr,
+       &pwrcl_pmux.clkr,

Please drop useless comments inside this array.

drivers/clk/qcom/clk-cpu-8996.c line 303

Quoted 6 lines

drivers/clk/qcom/clk-cpu-8996.c new file +6 -0

+};
+
+struct clk_hw_clks {
+       unsigned int num;
+       struct clk_hw *hws[];
+};

Please just NULL terminate the list of clk_hw pointers, or keep the size around instead.

drivers/clk/qcom/clk-cpu-8996.c

Quoted 3 lines

drivers/clk/qcom/clk-cpu-8996.c new file +3 -0

+
+static int
+qcom_cpu_clk_msm8996_register_clks(struct device *dev, struct

clk_hw_clks *hws,

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+                                  struct regmap *regmap) {
+       int i, ret;
+
+       hws->hws[0] = clk_hw_register_fixed_factor(dev, "perfcl_pll_main",
+                                                  "perfcl_pll",
+                                                  CLK_SET_RATE_PARENT, 1, 2);
+       perfcl_smux.pll = hws->hws[0];
+
+       hws->hws[1] = clk_hw_register_fixed_factor(dev, "pwrcl_pll_main",
+                                                  "pwrcl_pll",

drivers/clk/qcom/clk-cpu-8996.c line 321

Quoted 4 lines

drivers/clk/qcom/clk-cpu-8996.c new file +4 -0

+                                                  CLK_SET_RATE_PARENT, 1, 2);
+       pwrcl_smux.pll = hws->hws[1];
+
+       hws->num = 2;

Maybe just open code the fixed factor clk registration? Then the devm_clk_hw_register() function can be used on those static structs to make removal simpler.

Quoted 13 lines

drivers/clk/qcom/clk-cpu-8996.c new file +12 -0

+
+       for (i = 0; i < ARRAY_SIZE(clks); i++) {
+               ret = devm_clk_register_regmap(dev, clks[i]);
+               if (ret)
+                       return ret;
+       }
+
+       clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
+       clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
+       clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
+       clk_alpha_pll_configure(&pwrcl_alt_pll, regmap,
+ &altpll_config);

drivers/clk/qcom/clk-cpu-8996.c line 342

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+
+       return ret;
+}
+
+static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device
+*pdev) {
+       int ret;
+       void __iomem *base;
+       struct resource *res;
+       struct regmap *regmap_cpu;

Just call it regmap please.

Quoted 30 lines

drivers/clk/qcom/clk-cpu-8996.c new file +30 -0

+       struct clk_hw_clks *hws;
+       struct clk_hw_onecell_data *data;
+       struct device *dev = &pdev->dev;
+       struct device_node *node = dev->of_node;
+
+       data = devm_kzalloc(dev, sizeof(*data) + 2 * sizeof(struct clk_hw *),
+                           GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       hws = devm_kzalloc(dev, sizeof(*hws) + 2 * sizeof(struct clk_hw *),
+                          GFP_KERNEL);
+       if (!hws)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(base))
+               return PTR_ERR(base);
+
+       regmap_cpu = devm_regmap_init_mmio(dev, base,
+                                          &cpu_msm8996_regmap_config);
+       if (IS_ERR(regmap_cpu))
+               return PTR_ERR(regmap_cpu);
+
+       ret = qcom_cpu_clk_msm8996_register_clks(dev, hws, regmap_cpu);
+       if (ret)
+               return ret;
+
+       data->hws[0] = &pwrcl_pmux.clkr.hw;

drivers/clk/qcom/clk-cpu-8996.c

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+       data->hws[1] = &perfcl_pmux.clkr.hw;
+
+       data->num = 2;
+
+       platform_set_drvdata(pdev, hws);
+
+       return of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
+data); }
+
+static int qcom_cpu_clk_msm8996_driver_remove(struct

platform_device

drivers/clk/qcom/clk-cpu-8996.c line 391

Quoted 9 lines

drivers/clk/qcom/clk-cpu-8996.c new file +9 -0

+*pdev) {
+       int i;
+       struct device *dev = &pdev->dev;
+       struct clk_hw_clks *hws = platform_get_drvdata(pdev);
+
+       for (i = 0; i < hws->num; i++)
+               clk_hw_unregister_fixed_rate(hws->hws[i]);
+
+       of_clk_del_provider(dev->of_node);

Use devm_of_clk_add_hw_provider() instead.

Quoted 7 lines

drivers/clk/qcom/clk-cpu-8996.c new file +7 -0

+
+       return 0;
+}
+
+static struct platform_driver qcom_cpu_clk_msm8996_driver = {
+       .probe = qcom_cpu_clk_msm8996_driver_probe,
+       .remove = qcom_cpu_clk_msm8996_driver_remove,

drivers/clk/qcom/clk-cpu-8996.c line 408

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c new file +10 -0

+       .driver = {
+               .name = "qcom-msm8996-apcc",
+               .of_match_table = match_table,
+       },
+};
+
+module_platform_driver(qcom_cpu_clk_msm8996_driver);
+
+MODULE_ALIAS("platform:msm8996-apcc");
+MODULE_DESCRIPTION("QCOM MSM8996 CPU clock Driver");

Not sure why Driver is capitalized and clock is not.

I will have to change the clk_hw_unregister_fixed_rate() as well then, right? This will be an API change.

Signed-off-by Ilia Lin <[email protected]>

---
 Documentation/devicetree/bindings/clock/qcom,kryocc.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,kryocc.txt

Signed-off-by Ilia Lin <[email protected]>

---
 Documentation/devicetree/bindings/clock/qcom,kryocc.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,kryocc.txt

Documentation/devicetree/bindings/clock/qcom,kryocc.txt new file +0 -0

new file mode 100644
index 0000000..e7cf15a

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 19:47
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 04/10] clk: qcom: Add DT bindings for CPU clock driver for msm8996

Quoting Ilia Lin (2018-02-14 05:59:46)

Quoted 3 lines

Signed-off-by Ilia Lin <[email protected]>

---
 Documentation/devicetree/bindings/clock/qcom,kryocc.txt | 17

+++++++++++++++++

Quoted 2 lines

 1 file changed, 17 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/clock/qcom,kryocc.txt

Review

Quoted 2 lines

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/clock/qcom,kryocc.txt

Documentation/devicetree/bindings/clock/qcom,kryocc.txt line 7

Quoted 12 lines

new file mode 100644
index 0000000..e7cf15a

Documentation/devicetree/bindings/clock/qcom,kryocc.txt new file +7 -0

@@ -0,0 +1,17 @@
+Qualcomm CPUSS clock controller for Kryo CPUs
+----------------------------------------------------
+
+Required properties :
+- compatible : shall contain only one of the following:
+
+                       "qcom-msm8996-apcc"

Do you mean qcom,msm8996-apcc?

Rob?

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters on msm8996 and powered via a primary

PLL and a secondary PLL. The primary PLL is what drivers the

CPU clk, except for times when we are reprogramming the PLL itself, when we temporarily switch to an alternate PLL. Use clock rate change notifiers to support this.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/clk-cpu-8996.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

drivers/clk/qcom/clk-cpu-8996.c +27 -0

index 42489f1..75bd014 100644
@@ -140,10 +140,14 @@ struct clk_cpu_8996_mux {
        u32     reg;
        u32     shift;
        u32     width;
+       struct notifier_block nb;
        struct clk_hw   *pll;
        struct clk_regmap clkr;
 };
 
+#define to_clk_cpu_8996_mux_nb(_nb) \
+       container_of(_nb, struct clk_cpu_8996_mux, nb)
+
 static inline
 struct clk_cpu_8996_mux *to_clk_cpu_8996_mux_hw(struct clk_hw *hw)
 {

@@ -194,6 +198,27 @@ static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)
        return 0;
 }
 
+int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
+                       void *data)
+{
+       int ret;
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_nb(nb);
+
+       switch (event) {
+       case PRE_RATE_CHANGE:
+               ret = clk_cpu_8996_mux_set_parent(&cpuclk->clkr.hw, ALT_INDEX);
+               break;
+       case POST_RATE_CHANGE:
+               ret = clk_cpu_8996_mux_set_parent(&cpuclk->clkr.hw, PLL_INDEX);
+               break;
+       default:
+               ret = 0;
+               break;
+       }
+
+       return notifier_from_errno(ret);
+};
+
 const struct clk_ops clk_cpu_8996_mux_ops = {
        .set_parent = clk_cpu_8996_mux_set_parent,
        .get_parent = clk_cpu_8996_mux_get_parent,

@@ -237,6 +262,7 @@ static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)
        .shift = 0,
        .width = 2,
        .pll = &pwrcl_pll.clkr.hw,
+       .nb.notifier_call = cpu_clk_notifier_cb,
        .clkr.hw.init = &(struct clk_init_data) {
                .name = "pwrcl_pmux",
                .parent_names = (const char *[]){

@@ -256,6 +282,7 @@ static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)
        .shift = 0,
        .width = 2,
        .pll = &perfcl_pll.clkr.hw,
+       .nb.notifier_call = cpu_clk_notifier_cb,
        .clkr.hw.init = &(struct clk_init_data) {
                .name = "perfcl_pmux",
                .parent_names = (const char *[]){

@@ -331,6 +358,14 @@ struct clk_hw_clks {
        clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 19:48
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 05/10] clk: qcom: cpu-8996: Add support to switch to alternate PLL

Quoting Ilia Lin (2018-02-14 05:59:47)

Quoted 3 lines

From: Rajendra Nayak <[email protected]>

Each of the CPU clusters on msm8996 and powered via a primary

s/and/are/

Quoted 1 line

PLL and a secondary PLL. The primary PLL is what drivers the

s/drivers/drives/

I make the same typo all the time!

Quoted 19 lines

CPU clk, except for times when we are reprogramming the PLL itself, when we temporarily switch to an alternate PLL.
Use clock rate change notifiers to support this.

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/clk-cpu-8996.c | 35
+++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

(quoted diff) +0 -0

b/drivers/clk/qcom/clk-cpu-8996.c index 42489f1..75bd014 100644

drivers/clk/qcom/clk-cpu-8996.c +0 -0

@@ -140,10 +140,14 @@ struct clk_cpu_8996_mux {
        u32     reg;
        u32     shift;
        u32     width;

drivers/clk/qcom/clk-cpu-8996.c struct clk_cpu_8996_mux { · line 152

Quoted 10 lines

drivers/clk/qcom/clk-cpu-8996.c +4 -0

+       struct notifier_block nb;
        struct clk_hw   *pll;
        struct clk_regmap clkr;
 };
 
+#define to_clk_cpu_8996_mux_nb(_nb) \
+       container_of(_nb, struct clk_cpu_8996_mux, nb)
+
 static inline
 struct clk_cpu_8996_mux *to_clk_cpu_8996_mux_hw(struct clk_hw *hw)

{

drivers/clk/qcom/clk-cpu-8996.c static int clk_cpu_8996_mux_set_parent(struct

Quoted 1 line

drivers/clk/qcom/clk-cpu-8996.c +0 -0

@@ -194,6 +198,27 @@ static int clk_cpu_8996_mux_set_parent(struct

clk_hw *hw, u8 index)

drivers/clk/qcom/clk-cpu-8996.c static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)

Quoted 12 lines

drivers/clk/qcom/clk-cpu-8996.c +9 -0

        return 0;
 }
 
+int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
+                       void *data)
+{
+       int ret;
+       struct clk_cpu_8996_mux *cpuclk = to_clk_cpu_8996_mux_nb(nb);
+
+       switch (event) {
+       case PRE_RATE_CHANGE:
+               ret = clk_cpu_8996_mux_set_parent(&cpuclk->clkr.hw,

ALT_INDEX);

drivers/clk/qcom/clk-cpu-8996.c static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)

Quoted 3 lines

drivers/clk/qcom/clk-cpu-8996.c +3 -0

+               break;
+       case POST_RATE_CHANGE:
+               ret = clk_cpu_8996_mux_set_parent(&cpuclk->clkr.hw,

PLL_INDEX);

Quoted 32 lines

drivers/clk/qcom/clk-cpu-8996.c +8 -0

+               break;
+       default:
+               ret = 0;
+               break;
+       }
+
+       return notifier_from_errno(ret); };
+
 const struct clk_ops clk_cpu_8996_mux_ops = {
        .set_parent = clk_cpu_8996_mux_set_parent,
        .get_parent = clk_cpu_8996_mux_get_parent, @@ -237,6 +262,7 @@

static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)

        .shift = 0,
        .width = 2,
        .pll = &pwrcl_pll.clkr.hw,
+       .nb.notifier_call = cpu_clk_notifier_cb,
        .clkr.hw.init = &(struct clk_init_data) {
                .name = "pwrcl_pmux",
                .parent_names = (const char *[]){ @@ -256,6 +282,7 @@
static int clk_cpu_8996_mux_set_parent(struct clk_hw *hw, u8 index)
        .shift = 0,
        .width = 2,
        .pll = &perfcl_pll.clkr.hw,
+       .nb.notifier_call = cpu_clk_notifier_cb,
        .clkr.hw.init = &(struct clk_init_data) {
                .name = "perfcl_pmux",
                .parent_names = (const char *[]){ @@ -331,6 +358,14 @@
struct clk_hw_clks {
        clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
        clk_alpha_pll_configure(&pwrcl_alt_pll, regmap,

&altpll_config);

drivers/clk/qcom/clk-cpu-8996.c struct clk_hw_clks {

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c +5 -0

+       ret = clk_notifier_register(pwrcl_pmux.clkr.hw.clk, &pwrcl_pmux.nb);
+       if (ret)
+               return ret;
+
+       ret = clk_notifier_register(perfcl_pmux.clkr.hw.clk,

&perfcl_pmux.nb);

drivers/clk/qcom/clk-cpu-8996.c struct clk_hw_clks { · line 364

Quoted 3 lines

drivers/clk/qcom/clk-cpu-8996.c +3 -0

+       if (ret)
+               return ret;
+

Please resend this with the other patches that need rework.

From: Rajendra Nayak <[email protected]>

The CPU clock controllers primary PLL operates on a single VCO range,

between 600Mhz and 3Ghz. However the CPUs do support OPPs with frequencies between 300Mhz and 600Mhz. In order to support running the

CPUs at those frequencies we end up having to lock the PLL at twice the rate and drive the CPU clk via the PLL/2 output and SMUX.

So for frequencies above 600Mhz we follow the following path
Primary PLL --> PLL_EARLY --> PMUX(1) --> CPU clk
and for frequencies between 300Mhz and 600Mhz we follow
Primary PLL --> PLL/2 --> SMUX(1) --> PMUX(0) --> CPU clk

Signed-off-by Rajendra Nayak <[email protected]>

Signed-off-by Ilia Lin <[email protected]>

The PLLs must be prepared enabled during the probe to be accessible by the OPPs. Otherwise an OPP may switch to non-enabled clock.

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/clk-cpu-8996.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/clk/qcom/clk-cpu-8996.c +0 -0

index 854f327..b0a3b73 100644

drivers/clk/qcom/clk-cpu-8996.c +1 -0

+
        ret = clk_notifier_register(pwrcl_pmux.clkr.hw.clk, &pwrcl_pmux.nb);
        if (ret)
                return ret;

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 18:50
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 07/10] clk: qcom: clk-cpu-8996: Prepare PLLs on probe

Quoting Ilia Lin (2018-02-14 05:59:49)

Quoted 2 lines

The PLLs must be prepared enabled during the probe to be accessible by the OPPs. Otherwise an OPP may switch to non-enabled clock.

Sounds like an OPP problem.

Quoted 7 lines

Signed-off-by Ilia Lin <[email protected]>

---
 drivers/clk/qcom/clk-cpu-8996.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/clk/qcom/clk-cpu-8996.c line 18

Quoted 10 lines

(quoted diff) +0 -0

b/drivers/clk/qcom/clk-cpu-8996.c index 854f327..b0a3b73 100644

drivers/clk/qcom/clk-cpu-8996.c +1 -1

@@ -15,7 +15,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
-
+#include <linux/clk-provider.h>

Please leave a newline between linux/* and local includes.

Quoted 9 lines

drivers/clk/qcom/clk-cpu-8996.c +0 -0

 #include "clk-alpha-pll.h"
 
 #define VCO(a, b, c) { \

@@ -376,6 +376,18 @@ struct clk_hw_clks {
        clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
        clk_alpha_pll_configure(&pwrcl_alt_pll, regmap,
 &altpll_config);
 

drivers/clk/qcom/clk-cpu-8996.c struct clk_hw_clks { · line 383

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c +5 -0

+       /* Enable all PLLs and alt PLLs */
+       clk_prepare_enable(pwrcl_alt_pll.clkr.hw.clk);
+       clk_prepare_enable(perfcl_alt_pll.clkr.hw.clk);
+       clk_prepare_enable(pwrcl_pll.clkr.hw.clk);
+       clk_prepare_enable(perfcl_pll.clkr.hw.clk);

And this can't be done by the cpufreq-dt driver?

drivers/clk/qcom/clk-cpu-8996.c struct clk_hw_clks { · line 389

Quoted 7 lines

drivers/clk/qcom/clk-cpu-8996.c +6 -0

+
+       /* Set initial boot frequencies for power/perf PLLs */
+       clk_set_rate(pwrcl_alt_pll.clkr.hw.clk, 652800000);
+       clk_set_rate(perfcl_alt_pll.clkr.hw.clk, 652800000);
+       clk_set_rate(pwrcl_pll.clkr.hw.clk, 652800000);
+       clk_set_rate(perfcl_pll.clkr.hw.clk, 652800000);

We have assigned rates in DT for this.

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c +1 -0

+
        ret = clk_notifier_register(pwrcl_pmux.clkr.hw.clk, &pwrcl_pmux.nb);
        if (ret)

                return ret;

Quoted 14 lines

-----Original Message-----
From: Stephen Boyd <[email protected]>
Sent: Monday, March 19, 2018 18:50
To: Ilia Lin <[email protected]>; linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; linux-clk at vger.kernel.org;
sboyd at codeaurora.org
Cc: mark.rutland at arm.com; devicetree at vger.kernel.org;
rnayak at codeaurora.org; robh at kernel.org; will.deacon at arm.com; amit.kucheria at linaro.org; tfinkel at codeaurora.org; ilialin at codeaurora.org; nicolas.dechesne at linaro.org; celster at codeaurora.org
Subject: Re: [PATCH v3 07/10] clk: qcom: clk-cpu-8996: Prepare PLLs on probe

Quoted 5 lines

Quoting Ilia Lin (2018-02-14 05:59:49)

Quoted 2 lines

The PLLs must be prepared enabled during the probe to be accessible by the OPPs. Otherwise an OPP may switch to non-enabled clock.

Sounds like an OPP problem.

And again, it could be solved by a platform specific cpufreq driver. Worth it?

Quoted 9 lines

Quoted 9 lines

#include "clk-alpha-pll.h"

#define VCO(a, b, c) { \

drivers/clk/qcom/clk-cpu-8996.c +0 -0

@@ -376,6 +376,18 @@ struct clk_hw_clks {
        clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
        clk_alpha_pll_configure(&pwrcl_alt_pll, regmap,
 &altpll_config);
 

Quoted 7 lines

drivers/clk/qcom/clk-cpu-8996.c struct clk_hw_clks { · line 383

Quoted 5 lines

drivers/clk/qcom/clk-cpu-8996.c +5 -0

+       /* Enable all PLLs and alt PLLs */
+       clk_prepare_enable(pwrcl_alt_pll.clkr.hw.clk);
+       clk_prepare_enable(perfcl_alt_pll.clkr.hw.clk);
+       clk_prepare_enable(pwrcl_pll.clkr.hw.clk);
+       clk_prepare_enable(perfcl_pll.clkr.hw.clk);

And this can't be done by the cpufreq-dt driver?

Are you suggesting changing the cpufreq-dt as well?

Quoted 9 lines

Quoted 8 lines

drivers/clk/qcom/clk-cpu-8996.c +6 -0

+
+       /* Set initial boot frequencies for power/perf PLLs */
+       clk_set_rate(pwrcl_alt_pll.clkr.hw.clk, 652800000);
+       clk_set_rate(perfcl_alt_pll.clkr.hw.clk, 652800000);
+       clk_set_rate(pwrcl_pll.clkr.hw.clk, 652800000);
+       clk_set_rate(perfcl_pll.clkr.hw.clk, 652800000);

Quoted 1 line

We have assigned rates in DT for this.

I assumed that the clock driver can live without the OPP table and any cpufreq driver. Or do you mean adding this as parameters for the kryocc DT node?

drivers/clk/qcom/clk-cpu-8996.c +2 -0

 #define VCO(a, b, c) { \
        .val = a,\

@@ -29,6 +30,27 @@
 #define ACD_INDEX              2
 #define ALT_INDEX              3
 #define DIV_2_THRESHOLD                600000000
+#define PWRCL_REG_OFFSET 0x0
+#define PERFCL_REG_OFFSET 0x80000