-----Original Message-----
From: Stephen Boyd <sboyd@kernel.org>
Sent: Monday, March 19, 2018 18:57
To: Ilia Lin <redacted>; 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 08/10] clk: qcom: Add ACD path to CPU clock driver
for msm8996

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

quoted

diff --git a/drivers/clk/qcom/clk-cpu-8996.c

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

--- a/drivers/clk/qcom/clk-cpu-8996.c
+++ b/drivers/clk/qcom/clk-cpu-8996.c

@@ -17,6 +17,7 @@
 #include <linux/regmap.h>
 #include <linux/clk-provider.h>
 #include "clk-alpha-pll.h"
+#include <soc/qcom/kryo-l2-accessors.h>

Put this above local includes please.

Will be changed in the next spin.

quoted

 #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
+#define MUX_OFFSET     0x40
+#define ALT_PLL_OFFSET 0x100
+#define SSSCTL_OFFSET 0x160
+/*
+APCy_QLL_SSSCTL value:
+SACDRCLEN=1
+SSWEN=1
+SSTRTEN=1
+SSTPAPMSWEN=1
+*/

Bad comment style and I have no idea what it means.

Will be changed in the next spin.

quoted

+#define SSSCTL_VAL 0xF
+
+enum {
+       APC_BASE,
+       EFUSE_BASE,

Is this used? efuse should go through nvmem APIs.

Will be removed in the next spin.

quoted

+       NUM_BASES
+};
+
+static void __iomem *vbases[NUM_BASES];

Please just pass this to the function that uses it and drop EFUSE_BASE.

Will be changed in the next spin.

quoted

 static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = {
        [PLL_OFF_L_VAL] = 0x04,

@@ -399,10 +424,64 @@ struct clk_hw_clks {
        return ret;
 }

+#define CPU_AFINITY_MASK 0xFFF
+#define PWRCL_CPU_REG_MASK 0x3
+#define PERFCL_CPU_REG_MASK 0x103
+
+/* ACD static settings (HMSS HPG 7.2.2) */ #define L2ACDCR_REG
+0x580ULL #define L2ACDTD_REG 0x581ULL #define L2ACDDVMRC_REG

0x584ULL

quoted

+#define L2ACDSSCR_REG 0x589ULL #define ACDTD_VAL 0x00006A11

#define

quoted

+ACDCR_VAL 0x002C5FFD #define ACDSSCR_VAL 0x00000601 #define
+ACDDVMRC_VAL 0x000E0F0F

Please don't have #defines for random register settings. It just obfuscates
what's going on at the place where the define is used.

So should I just use the values directly?

quoted

+
+static DEFINE_SPINLOCK(acd_lock);
+
+static void qcom_cpu_clk_msm8996_acd_init(void)
+{
+       u64 hwid;
+       unsigned long flags;
+
+       spin_lock_irqsave(&acd_lock, flags);
+
+       hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
+
+       /* Program ACD Tunable-Length Delay (TLD) */
+       set_l2_indirect_reg(L2ACDTD_REG, ACDTD_VAL);
+       /* Initial ACD for *this* cluster */
+       set_l2_indirect_reg(L2ACDDVMRC_REG, ACDDVMRC_VAL);
+       /* Program ACD soft start control bits. */
+       set_l2_indirect_reg(L2ACDSSCR_REG, ACDSSCR_VAL);

Please remove all useless comments, the code is obviously touching
registers.

Will be changed in the next spin.

quoted

+
+       if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
+               /* Enable Soft Stop/Start */

Sigh.

quoted

+               if (vbases[APC_BASE])

When is this false?

It is checked in the probe. You are right, I'll remove this check and pass the base as argument.

quoted

+                       writel_relaxed(SSSCTL_VAL, vbases[APC_BASE] +
+                                       PWRCL_REG_OFFSET + SSSCTL_OFFSET);
+               /* Ensure SSSCTL config goes through before enabling ACD. */
+               mb();

Use writel instead.

OK

quoted

+               /* Program ACD control bits */
+               set_l2_indirect_reg(L2ACDCR_REG, ACDCR_VAL);
+       }
+       if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
+ //else {

What is that '// else {' stuff?

Missed leftover. Will be changed in the next spin.

quoted

+               /* Program ACD control bits */
+               set_l2_indirect_reg(L2ACDCR_REG, ACDCR_VAL);
+               /* Enable Soft Stop/Start */
+               if (vbases[APC_BASE])
+                       writel_relaxed(SSSCTL_VAL, vbases[APC_BASE] +
+                                       PERFCL_REG_OFFSET + SSSCTL_OFFSET);
+               /* Ensure SSSCTL config goes through before enabling ACD. */
+               mb();

Again, use writel.

OK

quoted

+       }
+
+       spin_unlock_irqrestore(&acd_lock, flags); }
 static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device
*pdev)  {
        int ret;
-       void __iomem *base;
        struct resource *res;
        struct regmap *regmap_cpu;
        struct clk_hw_clks *hws;

@@ -415,17 +494,17 @@ static int

qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)

quoted

        if (!data)
                return -ENOMEM;

-       hws = devm_kzalloc(dev, sizeof(*hws) + 2 * sizeof(struct clk_hw *),
+       hws = devm_kzalloc(dev, sizeof(*hws) + 4 * 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);
+       vbases[APC_BASE] = devm_ioremap_resource(dev, res);
+       if (IS_ERR(vbases[APC_BASE]))
+               return PTR_ERR(vbases[APC_BASE]);

-       regmap_cpu = devm_regmap_init_mmio(dev, base,
+       regmap_cpu = devm_regmap_init_mmio(dev, vbases[APC_BASE],
                                           &cpu_msm8996_regmap_config);
        if (IS_ERR(regmap_cpu))
                return PTR_ERR(regmap_cpu);

Cool, none of this diff is needed.

Since the effuse code will not be implemented in the clock driver, you are right. Will be changed in the next spin.

quoted

@@ -433,6 +512,7 @@ static int

qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)

quoted

        ret = qcom_cpu_clk_msm8996_register_clks(dev, hws, regmap_cpu);
        if (ret)
                return ret;
+       qcom_cpu_clk_msm8996_acd_init();

Pass base here.

Sure.

quoted

        data->hws[0] = &pwrcl_pmux.clkr.hw;
        data->hws[1] = &perfcl_pmux.clkr.hw;