DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org F28F4614FD
Date: Wed, 2 Nov 2016 15:17:41 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
To: Rajendra Nayak <rnayak@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
Message-ID: <20161102221741.GS16026@codeaurora.org>
References: <1475138152-859-1-git-send-email-rnayak@codeaurora.org>
 <1475138152-859-12-git-send-email-rnayak@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1475138152-859-12-git-send-email-rnayak@codeaurora.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 14065
Lines: 508

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.

> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

DT binding document?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project