| Message ID | 1518616792-29028-9-git-send-email-ilialin@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/03/18 16:57, Stephen Boyd wrote: [...] >> + >> + 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. Note that writel() only gives an implicit wmb() *before* the store to ensure ordering against any previous writes. If this code really needs to ensure that the given write has definitely completed before any other accesses happen, then it still needs an explicit barrier *after* the write*(), unless perhaps the next access is always guaranteed to be a non-relaxed write (thus implicitly providing a suitable DSB). Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Robin Murphy (2018-03-19 11:16:15) > On 19/03/18 16:57, Stephen Boyd wrote: > [...] > >> + > > > >> + writel_relaxed(SSSCTL_VAL, vbases[APC_BASE] + > >> + PWRCL_REG_OFFSET + SSSCTL_OFFSET); > >> + /* Ensure SSSCTL config goes through before enabling ACD. */ > >> + mb(); > > > > Use writel instead. > > Note that writel() only gives an implicit wmb() *before* the store to > ensure ordering against any previous writes. If this code really needs > to ensure that the given write has definitely completed before any other > accesses happen, then it still needs an explicit barrier *after* the > write*(), unless perhaps the next access is always guaranteed to be a > non-relaxed write (thus implicitly providing a suitable DSB). > Ah right. So this should be a wmb() too? I suspect it's to order with the write to the l2 indirect registers, but reading that register before the MMIO write is not a problem. The comment above the l2 accessors could be slightly more specific here and it would help immensely. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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; -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting ilialin@codeaurora.org (2018-03-20 07:04:05) > > From: Stephen Boyd <sboyd@kernel.org> > > Quoting Ilia Lin (2018-02-14 05:59:50) > > > > > > > > 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? Yes. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/03/18 21:21, Stephen Boyd wrote: > Quoting Robin Murphy (2018-03-19 11:16:15) >> On 19/03/18 16:57, Stephen Boyd wrote: >> [...] >>>> + >>> >>>> + writel_relaxed(SSSCTL_VAL, vbases[APC_BASE] + >>>> + PWRCL_REG_OFFSET + SSSCTL_OFFSET); >>>> + /* Ensure SSSCTL config goes through before enabling ACD. */ >>>> + mb(); >>> >>> Use writel instead. >> >> Note that writel() only gives an implicit wmb() *before* the store to >> ensure ordering against any previous writes. If this code really needs >> to ensure that the given write has definitely completed before any other >> accesses happen, then it still needs an explicit barrier *after* the >> write*(), unless perhaps the next access is always guaranteed to be a >> non-relaxed write (thus implicitly providing a suitable DSB). >> > > Ah right. So this should be a wmb() too? I suspect it's to order with > the write to the l2 indirect registers, but reading that register before > the MMIO write is not a problem. The comment above the l2 accessors > could be slightly more specific here and it would help immensely. It depends a bit on what exactly the hardware requires. If it's merely that write(s) to register A (and perhaps others) arrive before any write to register B, then the sensible thing to do would be to just make the write to B non-relaxed, i.e.: writel_relaxed(x, base + A); ... writel(y, base + B); That still only guarantees that the previous write(s) have been pushed all the way to the endpoint before the write to B is issued, though - for many devices that's sufficient, but in some cases the only way to be totally sure that the write has both been received *and* taken effect is by reading back some suitable indicator before proceeding. If we only care about making sure writes are pushed out, rather than synchronising more complicated memory accesses between multiple agents, then a full mb() is probably overkill (as ordering against outstanding loads too only adds more overhead in the interconnect), and wmb() (ideally implicit in a subsequent writel() as above) should be enough. Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> #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 +*/ +#define SSSCTL_VAL 0xF + +enum { + APC_BASE, + EFUSE_BASE, + NUM_BASES +}; + +static void __iomem *vbases[NUM_BASES]; static const u8 prim_pll_regs[PLL_OFF_MAX_REGS] = { [PLL_OFF_L_VAL] = 0x04, @@ -67,7 +89,7 @@ }; static struct clk_alpha_pll perfcl_pll = { - .offset = 0x80000, + .offset = PERFCL_REG_OFFSET, .regs = prim_pll_regs, .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE, .clkr.hw.init = &(struct clk_init_data){ @@ -79,7 +101,7 @@ }; static struct clk_alpha_pll pwrcl_pll = { - .offset = 0x0, + .offset = PWRCL_REG_OFFSET, .regs = prim_pll_regs, .flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE, .clkr.hw.init = &(struct clk_init_data){ @@ -109,7 +131,7 @@ }; static struct clk_alpha_pll perfcl_alt_pll = { - .offset = 0x80100, + .offset = PERFCL_REG_OFFSET + ALT_PLL_OFFSET, .regs = alt_pll_regs, .vco_table = alt_pll_vco_modes, .num_vco = ARRAY_SIZE(alt_pll_vco_modes), @@ -123,7 +145,7 @@ }; static struct clk_alpha_pll pwrcl_alt_pll = { - .offset = 0x100, + .offset = PWRCL_REG_OFFSET + ALT_PLL_OFFSET, .regs = alt_pll_regs, .vco_table = alt_pll_vco_modes, .num_vco = ARRAY_SIZE(alt_pll_vco_modes), @@ -136,6 +158,8 @@ }, }; +static void qcom_cpu_clk_msm8996_acd_init(void); + /* Mux'es */ struct clk_cpu_8996_mux { @@ -218,6 +242,7 @@ int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event, switch (event) { case PRE_RATE_CHANGE: ret = clk_cpu_8996_mux_set_parent(&cpuclk->clkr.hw, ALT_INDEX); + qcom_cpu_clk_msm8996_acd_init(); break; case POST_RATE_CHANGE: if (cnd->new_rate < DIV_2_THRESHOLD) @@ -225,7 +250,7 @@ int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event, DIV_2_INDEX); else ret = clk_cpu_8996_mux_set_parent(&cpuclk->clkr.hw, - PLL_INDEX); + ACD_INDEX); break; default: ret = 0; @@ -242,7 +267,7 @@ int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event, }; static struct clk_cpu_8996_mux pwrcl_smux = { - .reg = 0x40, + .reg = PWRCL_REG_OFFSET + MUX_OFFSET, .shift = 2, .width = 2, .clkr.hw.init = &(struct clk_init_data) { @@ -258,7 +283,7 @@ int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event, }; static struct clk_cpu_8996_mux perfcl_smux = { - .reg = 0x80040, + .reg = PERFCL_REG_OFFSET + MUX_OFFSET, .shift = 2, .width = 2, .clkr.hw.init = &(struct clk_init_data) { @@ -274,7 +299,7 @@ int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event, }; static struct clk_cpu_8996_mux pwrcl_pmux = { - .reg = 0x40, + .reg = PWRCL_REG_OFFSET + MUX_OFFSET, .shift = 0, .width = 2, .pll = &pwrcl_pll.clkr.hw, @@ -295,7 +320,7 @@ int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event, }; static struct clk_cpu_8996_mux perfcl_pmux = { - .reg = 0x80040, + .reg = PERFCL_REG_OFFSET + MUX_OFFSET, .shift = 0, .width = 2, .pll = &perfcl_pll.clkr.hw, @@ -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 + +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); + + if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) { + /* Enable Soft Stop/Start */ + if (vbases[APC_BASE]) + writel_relaxed(SSSCTL_VAL, vbases[APC_BASE] + + PWRCL_REG_OFFSET + SSSCTL_OFFSET); + /* Ensure SSSCTL config goes through before enabling ACD. */ + mb(); + /* Program ACD control bits */ + set_l2_indirect_reg(L2ACDCR_REG, ACDCR_VAL); + } + if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) { //else { + /* 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(); + } + + 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); @@ -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(); data->hws[0] = &pwrcl_pmux.clkr.hw; data->hws[1] = &perfcl_pmux.clkr.hw;
The PMUX for each duplex allows for selection of ACD clock source. The DVM (Dynamic Variation Monitor) will flag an error when a voltage droop event is detected. This flagged error enables ACD to provide a div-by-2 clock, sourced from the primary PLL. The duplex will be provided the divided clock until a pre-programmed delay has expired. This change configures ACD during the probe and switches the PMUXes to the ACD clock source. Signed-off-by: Ilia Lin <ilialin@codeaurora.org> --- drivers/clk/qcom/clk-cpu-8996.c | 110 ++++++++++++++++++++++++++++++++++------ 1 file changed, 95 insertions(+), 15 deletions(-)