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

Lukasz Majewski
2014-01-10T12:04:17Z

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