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.
>
> #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.
> +#define SSSCTL_VAL 0xF
> +
> +enum {
> + APC_BASE,
> + EFUSE_BASE,
Is this used? efuse should go through nvmem APIs.
> + NUM_BASES
> +};
> +
> +static void __iomem *vbases[NUM_BASES];
Please just pass this to the function that uses it and drop EFUSE_BASE.
>
> 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.
> +
> +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.
> +
> + if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
> + /* Enable Soft Stop/Start */
Sigh.
> + if (vbases[APC_BASE])
When is this false?
> + writel_relaxed(SSSCTL_VAL, vbases[APC_BASE] +
> + PWRCL_REG_OFFSET + SSSCTL_OFFSET);
> + /* Ensure SSSCTL config goes through before enabling ACD. */
> + mb();
Use writel instead.
> + /* 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?
> + /* 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.
> + }
> +
> + 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.
> @@ -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.
>
> data->hws[0] = &pwrcl_pmux.clkr.hw;
> data->hws[1] = &perfcl_pmux.clkr.hw;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
