DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 1EB8A6145A
Message-ID: <581AF63D.9090509@codeaurora.org>
Date: Thu, 03 Nov 2016 14:03:01 +0530
From: Rajendra Nayak <rnayak@codeaurora.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0
MIME-Version: 1.0
To: Stephen Boyd <sboyd@codeaurora.org>
CC: mturquette@baylibre.com, linux-clk@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        tdas@codeaurora.org
Subject: Re: [RFC v3 11/11] clk: qcom: Add basic CPU clock driver for msm8996
References: <1475138152-859-1-git-send-email-rnayak@codeaurora.org> <1475138152-859-12-git-send-email-rnayak@codeaurora.org> <20161102221741.GS16026@codeaurora.org>
In-Reply-To: <20161102221741.GS16026@codeaurora.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 15282
Lines: 565


On 11/03/2016 03:47 AM, Stephen Boyd wrote:
> On 09/29, Rajendra Nayak wrote:
>> This is a skeletal CPU clock driver, which adds support for the
>> CPU SS primary as well as secondary/alternate PLLs, and the
>> primary/secondary muxes.
>>
>> This still has support missing for
>> 1. CBF PLL and mux
>> 2. ACD
>> 3. APM
> 
> Maybe you can put a clk tree diagram here so we understand the
> hierarchy.

okay, will do.

> 
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
> 
> DT binding document?

will add.

>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 2a25f4e..407668d 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -10,6 +10,7 @@ clk-qcom-y += clk-branch.o
>>  clk-qcom-y += clk-regmap-divider.o
>>  clk-qcom-y += clk-regmap-mux.o
>>  clk-qcom-y += reset.o
>> +clk-qcom-y += clk-cpu-8996.o
> 
> Please add a config option.

will do,

> 
>>  clk-qcom-$(CONFIG_QCOM_GDSC) += gdsc.o
>>  
>>  obj-$(CONFIG_APQ_GCC_8084) += gcc-apq8084.o
>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>> new file mode 100644
>> index 0000000..e690544
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>> @@ -0,0 +1,408 @@
>> +/*
>> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/cpu.h>
> 
> Is this used?

not used, will remove

> 
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#include "clk-alpha-pll.h"
>> +#include "clk-pll.h"
>> +#include "clk-regmap.h"
>> +#include "clk-regmap-mux.h"
>> +
>> +#define VCO(a, b, c) { \
>> +	.val = a,\
>> +	.min_freq = b,\
>> +	.max_freq = c,\
>> +}
>> +
>> +static const struct alpha_pll_config hfpll_config = {
>> +	.l = 60,
>> +	.config_ctl_val = 0x200D4828,
>> +	.config_ctl_hi_val = 0x006,
>> +	.pre_div_mask = BIT(12),
>> +	.post_div_mask = 0x3 << 8,
>> +	.main_output_mask = BIT(0),
>> +	.early_output_mask = BIT(3),
>> +};
>> +
>> +static struct clk_alpha_pll perfcl_pll = {
>> +	.offset = 0x80000,
>> +	.min_rate = 600000000,
>> +	.max_rate = 3000000000,
>> +	.flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_16BIT_ALPHA
>> +			| SUPPORTS_FSM_MODE,
>> +	.clkr.hw.init = &(struct clk_init_data){
>> +		.name = "perfcl_pll",
>> +		.parent_names = (const char *[]){ "xo_board" },
>> +		.num_parents = 1,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>> +	},
>> +};
>> +
>> +static struct clk_alpha_pll pwrcl_pll = {
>> +	.offset = 0x0,
>> +	.min_rate = 600000000,
>> +	.max_rate = 3000000000,
>> +	.flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_16BIT_ALPHA
>> +			| SUPPORTS_FSM_MODE,
>> +	.clkr.hw.init = &(struct clk_init_data){
>> +		.name = "pwrcl_pll",
>> +		.parent_names = (const char *[]){ "xo_board" },
>> +		.num_parents = 1,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>> +	},
>> +};
>> +
>> +static const struct pll_vco alt_pll_vco_modes[] = {
>> +	VCO(3,  250000000,  500000000),
>> +	VCO(2,  500000000,  750000000),
>> +	VCO(1,  750000000, 1000000000),
>> +	VCO(0, 1000000000, 2150400000),
>> +};
>> +
>> +static const struct alpha_pll_config altpll_config = {
>> +	.l = 16,
>> +	.vco_val = 0x3 << 20,
>> +	.vco_mask = 0x3 << 20,
>> +	.config_ctl_val = 0x4001051B,
> 
> Lower case hex please.

will fix

> 
>> +	.post_div_mask = 0x3 << 8,
>> +	.post_div_val = 0x1,
>> +	.main_output_mask = BIT(0),
>> +	.early_output_mask = BIT(3),
>> +};
>> +
>> +static struct clk_alpha_pll perfcl_alt_pll = {
>> +	.offset = 0x80100,
>> +	.vco_table = alt_pll_vco_modes,
>> +	.num_vco = ARRAY_SIZE(alt_pll_vco_modes),
>> +	.flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "perfcl_alt_pll",
>> +		.parent_names = (const char *[]){ "xo_board" },
>> +		.num_parents = 1,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>> +	},
>> +};
>> +
>> +static struct clk_alpha_pll pwrcl_alt_pll = {
>> +	.offset = 0x100,
>> +	.vco_table = alt_pll_vco_modes,
>> +	.num_vco = ARRAY_SIZE(alt_pll_vco_modes),
>> +	.flags = SUPPORTS_OFFLINE_REQ | SUPPORTS_FSM_MODE,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "pwrcl_alt_pll",
>> +		.parent_names = (const char *[]){ "xo_board" },
>> +		.num_parents = 1,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>> +	},
>> +};
>> +
>> +static struct clk_regmap_mux pwrcl_pmux = {
>> +	.reg = 0x40,
>> +	.shift = 0,
>> +	.width = 2,
>> +	.table = (u32 []){0, 1, 3},
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "pwrcl_pmux",
>> +		.parent_names = (const char *[]){
>> +			"pwrcl_smux",
>> +			"pwrcl_pll",
>> +			"pwrcl_alt_pll",
>> +		},
>> +		.num_parents = 3,
>> +		.ops = &clk_regmap_mux_closest_ops,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static struct clk_regmap_mux pwrcl_smux = {
>> +	.reg = 0x40,
>> +	.shift = 2,
>> +	.width = 2,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "pwrcl_smux",
>> +		.parent_names = (const char *[]){
>> +			"xo_board",
>> +			"pwrcl_pll_main",
>> +			"sys_apcscbf_clk",
>> +			"sys_apcsaux_clk",
>> +		},
>> +		.num_parents = 4,
>> +		.ops = &clk_regmap_mux_closest_ops,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static struct clk_regmap_mux perfcl_pmux = {
>> +	.reg = 0x80040,
>> +	.shift = 0,
>> +	.width = 2,
>> +	.table = (u32 []){0, 1, 3},
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "perfcl_pmux",
>> +		.parent_names = (const char *[]){
>> +			"perfcl_smux",
>> +			"perfcl_pll",
>> +			"perfcl_alt_pll",
>> +		},
>> +		.num_parents = 3,
>> +		.ops = &clk_regmap_mux_closest_ops,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static struct clk_regmap_mux perfcl_smux = {
>> +	.reg = 0x80040,
>> +	.shift = 2,
>> +	.width = 2,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "perfcl_smux",
>> +		.parent_names = (const char *[]){
>> +			"xo_board",
> 
> Just use xo please.

will do

> 
>> +			"perfcl_pll_main",
>> +			"sys_apcscbf_clk",
>> +			"sys_apcsaux_clk",
>> +		},
>> +		.num_parents = 4,
>> +		.ops = &clk_regmap_mux_closest_ops,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +struct clk_cpu_8996 {
>> +	struct clk_hw *alt_clk;
>> +	struct clk_hw *pll;
>> +	struct clk_regmap clkr;
>> +};
>> +
>> +static inline struct clk_cpu_8996 *to_clk_cpu_8996(struct clk_hw *hw)
>> +{
>> +	return container_of(to_clk_regmap(hw), struct clk_cpu_8996, clkr);
>> +}
>> +
>> +static int clk_cpu_8996_set_rate(struct clk_hw *hw, unsigned long rate,
>> +				 unsigned long prate)
>> +{
>> +	int ret;
>> +	struct clk_cpu_8996 *cpuclk = to_clk_cpu_8996(hw);
>> +	struct clk *alt_clk, *pll, *parent;
>> +
>> +	alt_clk = clk_hw_get_clk(cpuclk->alt_clk);
>> +	pll = clk_hw_get_clk(cpuclk->pll);
>> +	parent = clk_hw_get_clk(clk_hw_get_parent(hw));
>> +
>> +	/* Switch parent to alt clk */
>> +	if (cpuclk->alt_clk) {
> 
> This would be false sometimes?

maybe not, it would be better to error out without an alt_clk

> 
>> +		ret = clk_set_parent(parent, alt_clk);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/* Set the PLL to new rate */
>> +	ret = clk_set_rate(pll, rate);
>> +	if (ret)
>> +		goto error;
>> +
>> +	/* Switch back to primary pll */
>> +	if (cpuclk->alt_clk) {
>> +		ret = clk_set_parent(parent, pll);
>> +		if (ret)
>> +			goto error;
>> +	}
>> +	return 0;
>> +
>> +error:
>> +	if (cpuclk->alt_clk)
>> +		clk_set_parent(parent, pll);
>> +
>> +	return ret;
>> +}
>> +
>> +static unsigned long clk_cpu_8996_recalc_rate(struct clk_hw *hw,
>> +					      unsigned long prate)
>> +{
>> +	return clk_hw_get_rate(clk_hw_get_parent(hw));
>> +}
> 
> If we just pass through parent rate I'm confused what the point
> of recalc is here.
> 
>> +
>> +static long clk_cpu_8996_round_rate(struct clk_hw *hw, unsigned long rate,
>> +				    unsigned long *prate)
>> +{
>> +	return clk_hw_round_rate(clk_hw_get_parent(hw), rate);
> 
> Same here. The core does this already?
> 
>> +}
>> +
>> +static struct clk_ops clk_cpu_8996_ops = {
> 
> const?
> 
>> +	.set_rate = clk_cpu_8996_set_rate,
>> +	.recalc_rate = clk_cpu_8996_recalc_rate,
>> +	.round_rate = clk_cpu_8996_round_rate,
> 
> This all feels fake... Please fold it onto the mux clk, because
> the mux is the true output to the CPU and not this software clk
> thing here.

Okay I will try to fold this into the mux clk

> 
>> +};
>> +
>> +static struct clk_cpu_8996 pwrcl_clk = {
>> +	.alt_clk = &pwrcl_alt_pll.clkr.hw,
>> +	.pll = &pwrcl_pll.clkr.hw,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "pwrcl_clk",
>> +		.parent_names = (const char *[]){ "pwrcl_pmux" },
>> +		.num_parents = 1,
>> +		.ops = &clk_cpu_8996_ops,
>> +	},
>> +};
>> +
>> +static struct clk_cpu_8996 perfcl_clk = {
>> +	.alt_clk = &perfcl_alt_pll.clkr.hw,
>> +	.pll = &perfcl_pll.clkr.hw,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "perfcl_clk",
>> +		.parent_names = (const char *[]){ "perfcl_pmux" },
>> +		.num_parents = 1,
>> +		.ops = &clk_cpu_8996_ops,
>> +	},
>> +};
>> +
>> +static const struct regmap_config cpu_msm8996_regmap_config = {
>> +	.reg_bits		= 32,
>> +	.reg_stride		= 4,
>> +	.val_bits		= 32,
>> +	.max_register		= 0x80210,
>> +	.fast_io		= true,
>> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
>> +};
>> +
>> +static const struct of_device_id match_table[] = {
>> +	{ .compatible = "qcom,cpu-clk-msm8996" },
>> +	{}
>> +};
>> +
>> +#define cluster_clk_register(dev, clk, clkr) { \
>> +	clk = devm_clk_register_regmap(dev, clkr); \
>> +	if (IS_ERR(clk)) \
>> +		return PTR_ERR(clk); }
> 
> Yuck. Why not have an array that we register in a loop instead?
> 
>> +
>> +#define cpu_clk_register_fixed(dev, clk, name, pname, flags, m, n) { \
>> +	clk = clk_register_fixed_factor(dev, name, pname, flags, m, n); \
>> +	if (IS_ERR(clk)) \
>> +		return PTR_ERR(clk); }
> 
> These could also be specified statically and registered in a
> loop. Also, please use clk_hw based registration APIs.

sure, will define an array and loop over

> 
>> +
>> +#define cpu_set_rate(dev, clk, rate) { \
>> +	if (clk_set_rate(clk, rate)) \
>> +		dev_err(dev, "Failed to set " #clk " to " #rate "\n"); }
> 
> Not used?

yes, will remove

> 
>> +
>> +#define cpu_prepare_enable(dev, clk) { \
>> +	if (clk_prepare_enable(clk)) \
>> +		dev_err(dev, "Failed to enable " #clk "\n"); }
>> +
>> +#define cpu_set_parent(dev, clk, parent) { \
>> +	if (clk_set_parent(clk, parent)) \
>> +		dev_err(dev, "Failed to set parent for " #clk "\n"); }
>> +
>> +struct clk *pwr_clk, *perf_clk;
> 
> static? Why do these need to be global though?

because I used them in probe to do

>> +	data->clks[0] = pwr_clk;
>> +	data->clks[1] = perf_clk;

> 
>> +
>> +static int register_cpu_clocks(struct device *dev, struct regmap *regmap)
>> +{
>> +	struct clk *perf_alt_pll, *pwr_alt_pll, *perf_pll, *pwr_pll;
>> +	struct clk *perf_pmux, *perf_smux, *pwr_pmux, *pwr_smux;
>> +	struct clk *perf_pll_main, *pwr_pll_main;
>> +
>> +	/* register PLLs */
>> +	cluster_clk_register(dev, perf_pll, &perfcl_pll.clkr);
>> +	cluster_clk_register(dev, pwr_pll, &pwrcl_pll.clkr);
>> +	cluster_clk_register(dev, perf_alt_pll, &perfcl_alt_pll.clkr);
>> +	cluster_clk_register(dev, pwr_alt_pll, &pwrcl_alt_pll.clkr);
>> +
>> +	/* register MUXs */
>> +	cluster_clk_register(dev, perf_pmux, &perfcl_pmux.clkr);
>> +	cluster_clk_register(dev, perf_smux, &perfcl_smux.clkr);
>> +	cluster_clk_register(dev, pwr_pmux, &pwrcl_pmux.clkr);
>> +	cluster_clk_register(dev, pwr_smux, &pwrcl_smux.clkr);
>> +
>> +	/* register Fixed clks */
>> +	cpu_clk_register_fixed(dev, perf_pll_main, "perfcl_pll_main",
>> +			       "perfcl_pll", CLK_SET_RATE_PARENT, 1, 2);
>> +	cpu_clk_register_fixed(dev, pwr_pll_main, "pwrcl_pll_main",
>> +			       "pwrcl_pll", CLK_SET_RATE_PARENT, 1, 2);
>> +
>> +	/* Register CPU clks */
> 
> Capitalized register this time?

:) will fix

> 
>> +	cluster_clk_register(dev, perf_clk, &perfcl_clk.clkr);
>> +	cluster_clk_register(dev, pwr_clk, &pwrcl_clk.clkr);
>> +
>> +	/* Initialise the PLLs */
>> +	clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>> +	clk_alpha_pll_configure(&pwrcl_pll, regmap, &hfpll_config);
>> +	clk_alpha_pll_configure(&perfcl_alt_pll, regmap, &altpll_config);
>> +	clk_alpha_pll_configure(&pwrcl_alt_pll, regmap, &altpll_config);
>> +
>> +	/* Enable all PLLs and alt PLLs */
>> +	cpu_prepare_enable(dev, perf_pll);
>> +	cpu_prepare_enable(dev, pwr_pll);
>> +	cpu_prepare_enable(dev, perf_alt_pll);
>> +	cpu_prepare_enable(dev, pwr_alt_pll);
> 
> Is this so the clks don't turn off at late init? Or because clk
> framework doesn't know these things are already enabled? The
> comment doesn't help in understanding here.

I don;t quite remember why I did this, will take a relook

> 
>> +
>> +	/* Init MUXes with default parents */
>> +	cpu_set_parent(dev, perf_pmux, perf_pll);
>> +	cpu_set_parent(dev, pwr_pmux, pwr_pll);
>> +	cpu_set_parent(dev, perf_smux, perf_pll_main);
>> +	cpu_set_parent(dev, pwr_smux, pwr_pll_main);
> 
> Can we use assigned-parents in DT instead?

sure, will do

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	void __iomem *base;
>> +	struct resource *res;
>> +	struct clk_onecell_data *data;
>> +	struct device *dev = &pdev->dev;
>> +	struct regmap *regmap_cpu;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->clks = devm_kcalloc(dev, 3, sizeof(struct clk *), GFP_KERNEL);
> 
> sizeof(*data->clks) or I guess hw pointers now. Also 3 != 2?

I had the cbf_clk which I removed because there where more things
to be taken care of, and did not update the 3 to 2 :/

> 
>> +	if (!data->clks)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	regmap_cpu = devm_regmap_init_mmio(dev, base,
>> +					   &cpu_msm8996_regmap_config);
>> +	if (IS_ERR(regmap_cpu))
>> +		return PTR_ERR(regmap_cpu);
>> +
>> +	ret = register_cpu_clocks(dev, regmap_cpu);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->clks[0] = pwr_clk;
>> +	data->clks[1] = perf_clk;
>> +	data->clk_num = 2;
>> +
>> +	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get, data);
>> +}
>> +
>> +static struct platform_driver qcom_cpu_clk_msm8996_driver = {
>> +	.probe = qcom_cpu_clk_msm8996_driver_probe,
>> +	.driver = {
>> +		.name = "qcom-cpu-clk-msm8996",
>> +		.of_match_table = match_table,
>> +	},
>> +};
>> +
>> +builtin_platform_driver(qcom_cpu_clk_msm8996_driver);
> 
> It can't be a module?

will fix

thanks for the review
Rajendra
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation