[1/6] clk: Tegra: Add CPU0 clock driver

This patch adds CPU0's clk driver for Tegra. It will be used by the generic
cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/clk/tegra/Makefile      |   1 +
 drivers/clk/tegra/clk-cpu.c     | 164 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/tegra/clk-tegra30.c |   4 +
 include/linux/clk/tegra.h       |   1 +
 4 files changed, 170 insertions(+)
 create mode 100644 drivers/clk/tegra/clk-cpu.c

Quoting Viresh Kumar (2013-08-07 07:46:43)
> This patch adds CPU0's clk driver for Tegra. It will be used by the generic
> cpufreq-cpu0 driver to get/set cpu clk.
> 
> Most of the platform specific bits are picked from tegra-cpufreq.c.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Hi Viresh,

It is nice to see more CPUfreq consolidation.

I'm currently hacking on a patch to introduce clk_coordinate_rates().
That function may be a better fit for this sort of thing compared to
overloading the .set_rate callback. I'll try to get the patches on the
list ASAP and will Cc you.

Regards,
Mike

> ---
>  drivers/clk/tegra/Makefile      |   1 +
>  drivers/clk/tegra/clk-cpu.c     | 164 ++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/tegra/clk-tegra30.c |   4 +
>  include/linux/clk/tegra.h       |   1 +
>  4 files changed, 170 insertions(+)
>  create mode 100644 drivers/clk/tegra/clk-cpu.c
> 
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index f49fac2..0e818c0 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -10,3 +10,4 @@ obj-y                                 += clk-super.o
>  obj-$(CONFIG_ARCH_TEGRA_2x_SOC)         += clk-tegra20.o
>  obj-$(CONFIG_ARCH_TEGRA_3x_SOC)         += clk-tegra30.o
>  obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += clk-tegra114.o
> +obj-$(CONFIG_GENERIC_CPUFREQ_CPU0)     += clk-cpu.o
> diff --git a/drivers/clk/tegra/clk-cpu.c b/drivers/clk/tegra/clk-cpu.c
> new file mode 100644
> index 0000000..01716d6
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-cpu.c
> @@ -0,0 +1,164 @@
> +/*
> + * Copyright (C) 2013 Linaro
> + *
> + * Author: Viresh Kumar <viresh.kumar@linaro.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +/*
> + * Responsible for setting cpu0 clk as requested by cpufreq-cpu0 driver
> + *
> + * All platform specific bits are taken from tegra-cpufreq driver.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +#define to_clk_cpu0(_hw) container_of(_hw, struct clk_cpu0, hw)
> +
> +struct clk_cpu0 {
> +       struct clk_hw hw;
> +       spinlock_t *lock;
> +};
> +
> +static struct clk *cpu_clk;
> +static struct clk *pll_x_clk;
> +static struct clk *pll_p_clk;
> +static struct clk *emc_clk;
> +
> +static unsigned long cpu0_recalc_rate(struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       return clk_get_rate(cpu_clk);
> +}
> +
> +static long cpu0_round_rate(struct clk_hw *hw, unsigned long drate,
> +               unsigned long *parent_rate)
> +{
> +       return clk_round_rate(cpu_clk, drate);
> +}
> +
> +static int cpu0_set_rate(struct clk_hw *hw, unsigned long rate,
> +               unsigned long parent_rate)
> +{
> +       int ret;
> +
> +       /*
> +        * Vote on memory bus frequency based on cpu frequency
> +        * This sets the minimum frequency, display or avp may request higher
> +        */
> +       if (rate >= 816000000)
> +               clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
> +       else if (rate >= 456000000)
> +               clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
> +       else
> +               clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */
> +
> +       /*
> +        * Take an extra reference to the main pll so it doesn't turn
> +        * off when we move the cpu off of it
> +        */
> +       clk_prepare_enable(pll_x_clk);
> +
> +       ret = clk_set_parent(cpu_clk, pll_p_clk);
> +       if (ret) {
> +               pr_err("%s: Failed to switch cpu to clock pll_p\n", __func__);
> +               goto out;
> +       }
> +
> +       if (rate == clk_get_rate(pll_p_clk))
> +               goto out;
> +
> +       ret = clk_set_rate(pll_x_clk, rate);
> +       if (ret) {
> +               pr_err("Failed to change pll_x to %lu\n", rate);
> +               goto out;
> +       }
> +
> +       ret = clk_set_parent(cpu_clk, pll_x_clk);
> +       if (ret) {
> +               pr_err("Failed to switch cpu to clock pll_x\n");
> +               goto out;
> +       }
> +
> +out:
> +       clk_disable_unprepare(pll_x_clk);
> +       return ret;
> +}
> +
> +static struct clk_ops clk_cpu0_ops = {
> +       .recalc_rate = cpu0_recalc_rate,
> +       .round_rate = cpu0_round_rate,
> +       .set_rate = cpu0_set_rate,
> +};
> +
> +struct clk *tegra_clk_register_cpu0(void)
> +{
> +       struct clk_init_data init;
> +       struct clk_cpu0 *cpu0;
> +       struct clk *clk;
> +
> +       cpu0 = kzalloc(sizeof(*cpu0), GFP_KERNEL);
> +       if (!cpu0) {
> +               pr_err("%s: could not allocate cpu0 clk\n", __func__);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       cpu_clk = clk_get_sys(NULL, "cpu");
> +       if (IS_ERR(cpu_clk)) {
> +               clk = cpu_clk;
> +               goto free_mem;
> +       }
> +
> +       pll_x_clk = clk_get_sys(NULL, "pll_x");
> +       if (IS_ERR(pll_x_clk)) {
> +               clk = pll_x_clk;
> +               goto put_cpu_clk;
> +       }
> +
> +       pll_p_clk = clk_get_sys(NULL, "pll_p_cclk");
> +       if (IS_ERR(pll_p_clk)) {
> +               clk = pll_p_clk;
> +               goto put_pll_x_clk;
> +       }
> +
> +       emc_clk = clk_get_sys("cpu", "emc");
> +       if (IS_ERR(emc_clk)) {
> +               clk = emc_clk;
> +               goto put_pll_p_clk;
> +       }
> +
> +       cpu0->hw.init = &init;
> +
> +       init.name = "cpu0";
> +       init.ops = &clk_cpu0_ops;
> +       init.flags = CLK_IS_ROOT | CLK_GET_RATE_NOCACHE;
> +       init.num_parents = 0;
> +
> +       clk = clk_register(NULL, &cpu0->hw);
> +       if (!IS_ERR(clk))
> +               return clk;
> +
> +       clk_prepare_enable(emc_clk);
> +       clk_prepare_enable(cpu_clk);
> +
> +       clk_put(emc_clk);
> +put_pll_p_clk:
> +       clk_put(pll_p_clk);
> +put_pll_x_clk:
> +       clk_put(pll_x_clk);
> +put_cpu_clk:
> +       clk_put(cpu_clk);
> +free_mem:
> +       kfree(cpu0);
> +
> +       pr_err("%s: clk register failed\n", __func__);
> +
> +       return NULL;
> +}
> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> index e2c6ca0..1cabeea 100644
> --- a/drivers/clk/tegra/clk-tegra30.c
> +++ b/drivers/clk/tegra/clk-tegra30.c
> @@ -1396,6 +1396,10 @@ static void __init tegra30_super_clk_init(void)
>                                         CLK_SET_RATE_PARENT, 1, 2);
>         clk_register_clkdev(clk, "twd", NULL);
>         clks[twd] = clk;
> +
> +       /* cpu0 clk for cpufreq driver */
> +       clk = tegra_clk_register_cpu0();
> +       clk_register_clkdev(clk, NULL, "cpu0");
>  }
>  
>  static const char *mux_pllacp_clkm[] = { "pll_a_out0", "unused", "pll_p",
> diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
> index 23a0cee..d416138 100644
> --- a/include/linux/clk/tegra.h
> +++ b/include/linux/clk/tegra.h
> @@ -128,5 +128,6 @@ static inline void tegra_periph_reset_deassert(struct clk *c) {}
>  static inline void tegra_periph_reset_assert(struct clk *c) {}
>  #endif
>  void tegra_clocks_apply_init_table(void);
> +struct clk *tegra_clk_register_cpu0(void);
>  
>  #endif /* __LINUX_CLK_TEGRA_H_ */
> -- 
> 1.7.12.rc2.18.g61b472e

On 08/07/2013 08:46 AM, Viresh Kumar wrote:
> This patch adds CPU0's clk driver for Tegra. It will be used by the generic
> cpufreq-cpu0 driver to get/set cpu clk.
> 
> Most of the platform specific bits are picked from tegra-cpufreq.c.

Hmmm. I'm not sure if it makes sense to represent this as a clock
object; isn't this more of a virtual construct that manages the rate of
the clock, rather than an actual clock? The actual clock already exists
as "cpu".

On 7 August 2013 23:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>> This patch adds CPU0's clk driver for Tegra. It will be used by the generic
>> cpufreq-cpu0 driver to get/set cpu clk.
>>
>> Most of the platform specific bits are picked from tegra-cpufreq.c.
>
> Hmmm. I'm not sure if it makes sense to represent this as a clock
> object; isn't this more of a virtual construct that manages the rate of
> the clock, rather than an actual clock? The actual clock already exists
> as "cpu".

I see it as this: There is a clock in system for cpu, call it "cpu". Now we
must be able to provide get/set routines for it. A set should set the
frequency to whatever is asked for and should really worry about how
that is being set. This part is internal to "cpu" clk.

This is what cpufreq-cpu0 driver should expect and does. Current "cpu"
clock implemented doesn't provide this facility ? And so this wrapper
made sense to me.

On 08/07/2013 11:45 AM, Viresh Kumar wrote:
> On 7 August 2013 23:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>> This patch adds CPU0's clk driver for Tegra. It will be used by the generic
>>> cpufreq-cpu0 driver to get/set cpu clk.
>>>
>>> Most of the platform specific bits are picked from tegra-cpufreq.c.
>>
>> Hmmm. I'm not sure if it makes sense to represent this as a clock
>> object; isn't this more of a virtual construct that manages the rate of
>> the clock, rather than an actual clock? The actual clock already exists
>> as "cpu".
> 
> I see it as this: There is a clock in system for cpu, call it "cpu". Now we
> must be able to provide get/set routines for it. A set should set the
> frequency to whatever is asked for and should really worry about how
> that is being set. This part is internal to "cpu" clk.

Sure.

> This is what cpufreq-cpu0 driver should expect and does. Current "cpu"
> clock implemented doesn't provide this facility ? And so this wrapper
> made sense to me.

But the additional management logic on top of the raw clock is exactly
what the cpufreq driver is for. This patch series is basically moving
the cpufreq driver code inside the clock code instead.

On 7 August 2013 23:18, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/07/2013 11:45 AM, Viresh Kumar wrote:
>> On 7 August 2013 23:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>>> This patch adds CPU0's clk driver for Tegra. It will be used by the generic
>>>> cpufreq-cpu0 driver to get/set cpu clk.
>>>>
>>>> Most of the platform specific bits are picked from tegra-cpufreq.c.
>>>
>>> Hmmm. I'm not sure if it makes sense to represent this as a clock
>>> object; isn't this more of a virtual construct that manages the rate of
>>> the clock, rather than an actual clock? The actual clock already exists
>>> as "cpu".
>>
>> I see it as this: There is a clock in system for cpu, call it "cpu". Now we
>> must be able to provide get/set routines for it. A set should set the
>> frequency to whatever is asked for and should really worry about how
>> that is being set. This part is internal to "cpu" clk.
>
> Sure.
>
>> This is what cpufreq-cpu0 driver should expect and does. Current "cpu"
>> clock implemented doesn't provide this facility ? And so this wrapper
>> made sense to me.
>
> But the additional management logic on top of the raw clock is exactly
> what the cpufreq driver is for. This patch series is basically moving
> the cpufreq driver code inside the clock code instead.

Above "sure" didn't go very well with what you wrote here :)

The additional management that we are required to do isn't cpufreq
driver specific but cpu or platform specific. cpufreq shouldn't care
about how CPU's clock is set to a particular frequency, its headache
of CPU's clk driver instead. cpu is yet another device and so
clk_set_rate() must be enough to set its frequency....

There might be other frameworks that need to set frequency of this
device later on and surely we don't want to replicate such piece of
code to every user..

Does it make sense to you?

On 08/07/2013 11:54 AM, Viresh Kumar wrote:
> On 7 August 2013 23:18, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/07/2013 11:45 AM, Viresh Kumar wrote:
>>> On 7 August 2013 23:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>>>> This patch adds CPU0's clk driver for Tegra. It will be used by the generic
>>>>> cpufreq-cpu0 driver to get/set cpu clk.
>>>>>
>>>>> Most of the platform specific bits are picked from tegra-cpufreq.c.
>>>>
>>>> Hmmm. I'm not sure if it makes sense to represent this as a clock
>>>> object; isn't this more of a virtual construct that manages the rate of
>>>> the clock, rather than an actual clock? The actual clock already exists
>>>> as "cpu".
>>>
>>> I see it as this: There is a clock in system for cpu, call it "cpu". Now we
>>> must be able to provide get/set routines for it. A set should set the
>>> frequency to whatever is asked for and should really worry about how
>>> that is being set. This part is internal to "cpu" clk.
>>
>> Sure.
>>
>>> This is what cpufreq-cpu0 driver should expect and does. Current "cpu"
>>> clock implemented doesn't provide this facility ? And so this wrapper
>>> made sense to me.
>>
>> But the additional management logic on top of the raw clock is exactly
>> what the cpufreq driver is for. This patch series is basically moving
>> the cpufreq driver code inside the clock code instead.
> 
> Above "sure" didn't go very well with what you wrote here :)
> 
> The additional management that we are required to do isn't cpufreq
> driver specific but cpu or platform specific.

Right, and that's *exactly* what having a cpufreq driver is for; to
implement the details of CPU clock management.

On 8 August 2013 00:20, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Right, and that's *exactly* what having a cpufreq driver is for; to
> implement the details of CPU clock management.

cpufreq drivers used to keep such information since a long time,
probably because there wasn't another place to keep them and
provide generic API's (like generic clock framework).. And so this
replication started to get in place which we are trying to get rid of
now.

All cpufreq drivers share a lot of common code which can go
away and so cpufreq-cpu0 was introduced..

With this patchset this replication goes away for tegra atleast at
the cost of a platform specific clk-cpu driver.. I think that's a good
deal, isn't it?

And that's the only way you can use these generic drivers that we
have...

On 08/07/2013 08:42 PM, Viresh Kumar wrote:
> On 8 August 2013 00:20, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> Right, and that's *exactly* what having a cpufreq driver is for; to
>> implement the details of CPU clock management.
> 
> cpufreq drivers used to keep such information since a long time,
> probably because there wasn't another place to keep them and
> provide generic API's (like generic clock framework).. And so this
> replication started to get in place which we are trying to get rid of
> now.
> 
> All cpufreq drivers share a lot of common code which can go
> away and so cpufreq-cpu0 was introduced..
> 
> With this patchset this replication goes away for tegra atleast at
> the cost of a platform specific clk-cpu driver.. I think that's a good
> deal, isn't it?

I think this patch series is simply moving the custom per-SoC code
somewhere else (clock driver) so that the cpufreq drivers can be
simpler. However, the clock drivers are more complex, and now represent
concepts that aren't really clocks.

So, no I'm not sure it's good.

> And that's the only way you can use these generic drivers that we
> have...

I don't think so. I think it's reasonable to have a per-SoC cpufreq
driver whose primary content is the parameterization data and/or custom
hooks for a unified core cpufreq driver. The duplicate parts of each
cpufreq driver can be moved into the core cpufreq driver, but the
non-duplicate parts remain. That's how many other subsystems work (MMC,
USB, ASoC spring to mind).

On 9 August 2013 00:20, Stephen Warren <swarren@wwwdotorg.org> wrote:
> I don't think so. I think it's reasonable to have a per-SoC cpufreq
> driver whose primary content is the parameterization data and/or custom
> hooks for a unified core cpufreq driver. The duplicate parts of each
> cpufreq driver can be moved into the core cpufreq driver, but the
> non-duplicate parts remain. That's how many other subsystems work (MMC,
> USB, ASoC spring to mind).

Guys in the --to list:

Please shout before its too late...

I can understand why Stephen is asking not to implement a virtual clock
driver for cpu as there is no clock corresponding to that.. We are just playing
with existing clocks there..

But I thought these clock APIs can be considered as hooks that we were
looking for and so shouldn't be a problem..

But yes, different people see things differently..

So, if I take Stephen's suggestions then I need to implement hooks into
cpufreq-cpu0 driver for taking freq-table/ setting clk rates, etc...

Let me know if anybody has a issue with that before we actually implement
that..

--
viresh