| Message ID | 20210504052844.21096-2-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add MSM8939 APCS/A53PLL clock support | expand |
Quoting Shawn Guo (2021-05-03 22:28:40) > The clock source for MSM8916 cpu cores is like below. > > |\ > a53pll --------| \ a53mux +------+ > | |------------| cpus | > gpll0_vote --------| / +------+ > |/ > > So clock a53mux rather than a53pll is actually the clock source of cpu > cores. It makes more sense to flag a53mux rather than a53pll as > critical, since a53pll could be irrelevant if a53mux switches its parent > clock to be gpll0_vote. Can you add some more detail here? I think the idea is to mark the mux as critical so that either a53pll or gpll0_vote is kept enabled, but only if they're used by the CPU. That isn't very clear from the commit text. Otherwise it seems OK. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/clk/qcom/a53-pll.c | 1 - > drivers/clk/qcom/apcs-msm8916.c | 2 +- > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c > index 45cfc57bff92..8614b0b0e82c 100644 > --- a/drivers/clk/qcom/a53-pll.c > +++ b/drivers/clk/qcom/a53-pll.c > @@ -70,7 +70,6 @@ static int qcom_a53pll_probe(struct platform_device *pdev) > init.parent_names = (const char *[]){ "xo" }; > init.num_parents = 1; > init.ops = &clk_pll_sr2_ops; > - init.flags = CLK_IS_CRITICAL; > pll->clkr.hw.init = &init; > > ret = devm_clk_register_regmap(dev, &pll->clkr); > diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c > index cf69a97d0439..d7ac6d6b15b6 100644 > --- a/drivers/clk/qcom/apcs-msm8916.c > +++ b/drivers/clk/qcom/apcs-msm8916.c > @@ -65,7 +65,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev) > init.parent_data = pdata; > init.num_parents = ARRAY_SIZE(pdata); > init.ops = &clk_regmap_mux_div_ops; > - init.flags = CLK_SET_RATE_PARENT; > + init.flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT; > > a53cc->clkr.hw.init = &init; > a53cc->clkr.regmap = regmap; > -- > 2.17.1 >
On Sun, Jun 27, 2021 at 05:27:41PM -0700, Stephen Boyd wrote: > Quoting Shawn Guo (2021-05-03 22:28:40) > > The clock source for MSM8916 cpu cores is like below. > > > > |\ > > a53pll --------| \ a53mux +------+ > > | |------------| cpus | > > gpll0_vote --------| / +------+ > > |/ > > > > So clock a53mux rather than a53pll is actually the clock source of cpu > > cores. It makes more sense to flag a53mux rather than a53pll as > > critical, since a53pll could be irrelevant if a53mux switches its parent > > clock to be gpll0_vote. > > Can you add some more detail here? I think the idea is to mark the mux > as critical so that either a53pll or gpll0_vote is kept enabled, but > only if they're used by the CPU. That isn't very clear from the commit > text. Otherwise it seems OK. Sure. I will rewrite the commit log. Shawn
diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c index 45cfc57bff92..8614b0b0e82c 100644 --- a/drivers/clk/qcom/a53-pll.c +++ b/drivers/clk/qcom/a53-pll.c @@ -70,7 +70,6 @@ static int qcom_a53pll_probe(struct platform_device *pdev) init.parent_names = (const char *[]){ "xo" }; init.num_parents = 1; init.ops = &clk_pll_sr2_ops; - init.flags = CLK_IS_CRITICAL; pll->clkr.hw.init = &init; ret = devm_clk_register_regmap(dev, &pll->clkr); diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c index cf69a97d0439..d7ac6d6b15b6 100644 --- a/drivers/clk/qcom/apcs-msm8916.c +++ b/drivers/clk/qcom/apcs-msm8916.c @@ -65,7 +65,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev) init.parent_data = pdata; init.num_parents = ARRAY_SIZE(pdata); init.ops = &clk_regmap_mux_div_ops; - init.flags = CLK_SET_RATE_PARENT; + init.flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT; a53cc->clkr.hw.init = &init; a53cc->clkr.regmap = regmap;
The clock source for MSM8916 cpu cores is like below. |\ a53pll --------| \ a53mux +------+ | |------------| cpus | gpll0_vote --------| / +------+ |/ So clock a53mux rather than a53pll is actually the clock source of cpu cores. It makes more sense to flag a53mux rather than a53pll as critical, since a53pll could be irrelevant if a53mux switches its parent clock to be gpll0_vote. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/clk/qcom/a53-pll.c | 1 - drivers/clk/qcom/apcs-msm8916.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)