RE: [PATCH v3 08/10] clk: qcom: Add ACD path to CPU clock driver for msm8996

[<ilialin@codeaurora.org>]

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Monday, March 19, 2018 18:57
> To: Ilia Lin <ilialin@codeaurora.org>; linux-arm-kernel@lists.infradead.org;
> linux-arm-msm@vger.kernel.org; linux-clk@vger.kernel.org;
> sboyd@codeaurora.org
> Cc: mark.rutland@arm.com; devicetree@vger.kernel.org;
> rnayak@codeaurora.org; robh@kernel.org; will.deacon@arm.com;
> amit.kucheria@linaro.org; tfinkel@codeaurora.org; ilialin@codeaurora.org;
> nicolas.dechesne@linaro.org; celster@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)
> > 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.

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

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

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

> 
> >
> >  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
> > +#define L2ACDSSCR_REG 0x589ULL #define ACDTD_VAL 0x00006A11
> #define
> > +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?

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

> 
> > +
> > +       if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
> > +               /* Enable Soft Stop/Start */
> 
> Sigh.
> 
> > +               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.

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

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

> 
> > +               /* 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

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

> 
> > @@ -433,6 +512,7 @@ static int
> qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
> >         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.

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