Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Thomas,

> The CPU clock provider supplies the clock to the CPU clock domain. The
> composition and organization of the CPU clock provider could vary
> among Exynos SoCs. A CPU clock provider can be composed of clock mux,
> dividers and gates. This patch defines a new clock type for CPU clock
> provider and adds infrastructure to register the CPU clock providers
> for Samsung platforms.
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/clk/samsung/clk.c |   71
> +++++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/samsung/clk.h |   37 +++++++++++++++++++++++- 2 files
> changed, 107 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index f503f32..9ac9056 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -316,3 +316,74 @@ unsigned long _get_rate(const char *clk_name)
>  
>  	return clk_get_rate(clk);
>  }
> +
> +/*
> + * On most platform, the core clock rate is equal to the clock rate
> of
> + * parent pll. This is a simple helper function to support
> recalc_rate
> + * callback for such platforms.
> + */
> +unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
> +				unsigned long parent_rate)
> +{
> +	/*
> +	 * Assuming that the output of the parent pll is the output
> clock
> +	 * frequency of the core clock.
> +	 */
> +	return parent_rate;
> +}
> +
> +/* This is a helper function to perform clock rounding for core
> clocks. */ +long samsung_core_clk_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate)
> +{
> +	struct samsung_core_clock *core_clk;
> +	const struct samsung_core_clock_freq_table *freq_tbl;
> +	int i;
> +
> +	core_clk = container_of(hw, struct samsung_core_clock, hw);
> +	freq_tbl = core_clk->freq_table;
> +	drate /= 1000;
> +
> +	for (i = 0; i < freq_tbl->freq_count; i++) {
> +		if (drate >= freq_tbl->freq[i])
> +			return freq_tbl->freq[i] * 1000;
> +	}
> +	return freq_tbl->freq[i - 1] * 1000;
> +}
> +
> +/* helper function to register a core clock */
> +void __init samsung_coreclk_register(const char *name, const char
> **parents,
> +			unsigned int num_parents, const char *pllclk,
> +			const struct clk_ops *clk_ops, unsigned int
> lookup_id,
> +			const struct samsung_core_clock_freq_table
> *freq_tbl) +{
> +	struct samsung_core_clock *coreclk;
> +	struct clk_init_data init;
> +	struct clk *clk;
> +
> +	coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
> +	if (!coreclk) {
> +		pr_err("%s: could not allocate memory for coreclk
> %s\n",
> +					__func__, name);
> +		return;
> +	}
> +
> +	init.name = name;
> +	init.flags = CLK_GET_RATE_NOCACHE;
> +	init.parent_names = parents;
> +	init.num_parents = num_parents;
> +	init.ops = clk_ops;
> +
> +	coreclk->hw.init = &init;
> +	coreclk->ctrl_base = reg_base;
> +	coreclk->fout_pll = __clk_lookup(pllclk);
> +	coreclk->freq_table = freq_tbl;
> +
> +	clk = clk_register(NULL, &coreclk->hw);
> +	if (IS_ERR(clk)) {
> +		pr_err("%s: could not register coreclk %s\n",
> __func__,	name);
> +		kfree(coreclk);
> +		return;
> +	}
> +	samsung_clk_add_lookup(clk, lookup_id);
> +}

I think, that those definitions for samsung_core_clock shall be moved
to a separate file (maybe clk-exynos-cpu.c)?

Moreover, maybe you can choose a shorter name?

> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> index 31b4174..0e43023 100644
> --- a/drivers/clk/samsung/clk.h
> +++ b/drivers/clk/samsung/clk.h
> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
>  	__PLL(_typ, _id, NULL, _name, _pname,
> CLK_GET_RATE_NOCACHE,	\ _lock, _con, _rtable, _alias)
>  
> +/**
> + * struct samsung_core_clock_freq_table: table of frequency
> supported by
> + * a core clock and associated data if any.
> + * @freq: points to a table of supported frequencies (in KHz)
> + * @freq_count: number of entries in the frequency table
> + * @data: core clock specific data, if any
> + */
> +struct samsung_core_clock_freq_table {
> +	const unsigned long	*freq;       /* in KHz */
> +	unsigned long		freq_count;
> +	const void		*data;
> +};

This is another instance of a structure, which holds the frequency for
the system (like struct cpufreq_frequency_table). 

Unfortunately, since the clk subsystem starts very early, the
cpufreq_frequency_table is not yet parsed from "operating-points"
attribute.

Maybe we can defer the clock registration?

> +
> +/**
> + * struct samsung_core_clock: information about clock supplied to a
> CPU core.
> + * @hw: handle between ccf and core clock.
> + * @ctrl_base: base address of the clock controller.
> + * @fout_pll: clock handle representing the clock output of the
> associated PLL.
> + */
> +struct samsung_core_clock {
> +	struct clk_hw		hw;
> +	void __iomem		*ctrl_base;
> +	struct clk		*fout_pll;
> +	const struct samsung_core_clock_freq_table *freq_table;
> +};
> +
> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
> *hw,
> +				unsigned long parent_rate);
> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate);

IMHO, those methods shall be defined as static in a separate file.

> +
>  extern void __init samsung_clk_init(struct device_node *np, void
> __iomem *base, unsigned long nr_clks, unsigned long *rdump,
>  		unsigned long nr_rdump, unsigned long *soc_rdump,
> @@ -337,7 +368,11 @@ extern void __init samsung_clk_register_gate(
>  		struct samsung_gate_clock *clk_list, unsigned int
> nr_clk); extern void __init samsung_clk_register_pll(struct
> samsung_pll_clock *pll_list, unsigned int nr_clk, void __iomem *base);
> -
> +extern void __init samsung_coreclk_register(const char *coreclk,
> +		const char **parents, unsigned int num_parents,
> +		const char *pllclk, const struct clk_ops *clk_ops,
> +		unsigned int lookup_id,
> +		const struct samsung_core_clock_freq_table
> *freq_tbl); extern unsigned long _get_rate(const char *clk_name);
>  
>  #endif /* __SAMSUNG_CLK_H */



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group