Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Hi Thomas, On 23.05.2014 16:41, Thomas Abraham wrote: [snip] > Thanks for your detailed review. I have made all the changes that you > have suggested. Unfortunately it seems like you have missed quite a lot of my comments, especially those regarding patch 4/8, which adds DT binding. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Hi Tomasz,
On Fri, May 16, 2014 at 10:47 PM, Tomasz Figa wrote:
> Hi Thomas,
>
> On 14.05.2014 03:11, Thomas Abraham wrote:
>> From: Thomas Abraham
>>
>> 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.
>>
>> Cc: Tomasz Figa
>> Signed-off-by: Thomas Abraham
>> ---
>> drivers/clk/samsung/Makefile |2 +-
>> drivers/clk/samsung/clk-cpu.c | 458
>> +
>> drivers/clk/samsung/clk.h |5 +
>> 3 files changed, 464 insertions(+), 1 deletions(-)
>> create mode 100644 drivers/clk/samsung/clk-cpu.c
>>
>> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
>> index 8eb4799..e2b453f 100644
>> --- a/drivers/clk/samsung/Makefile
>> +++ b/drivers/clk/samsung/Makefile
>> @@ -2,7 +2,7 @@
>> # Samsung Clock specific Makefile
>> #
>>
>> -obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
>> +obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o clk-cpu.o
>> obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
>> obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
>> obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
>> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
>> new file mode 100644
>> index 000..6a40862
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-cpu.c
>> @@ -0,0 +1,458 @@
>> +/*
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Thomas Abraham
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This file contains the utility functions to register the cpu clocks
>> + * for samsung platforms.
>
> s/cpu/CPU/
> s/samsung/Samsung/
>
>> +*/
>> +
>> +#include
>> +#include "clk.h"
>> +
>> +#define SRC_CPU 0x0
>> +#define STAT_CPU 0x200
>> +#define DIV_CPU0 0x300
>> +#define DIV_CPU1 0x304
>> +#define DIV_STAT_CPU00x400
>> +#define DIV_STAT_CPU10x404
>> +
>> +#define MAX_DIV 8
>> +
>> +#define EXYNOS4210_ARM_DIV1(div) ((div & 0x7) + 1)
>> +#define EXYNOS4210_ARM_DIV2(div) (((div >> 28) & 0x7) + 1)
>> +
>> +#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0) \
>> + ((d5 << 24) | (d4 << 20) | (d3 << 16) | (d2 << 12) |\
>> + (d1 << 8) | (d0 << 4))
>> +#define EXYNOS4210_DIV_CPU1(d2, d1, d0)
>> \
>> + ((d2 << 8) | (d1 << 4) | (d0 << 0))
>
> Macro arguments should be put into parentheses to make sure that whole
> argument is subject to further arithmetic operations.
>
>> +
>> +#define EXYNOS4210_DIV1_HPM_MASK ((0x7 << 0) | (0x7 << 4))
>> +#define EXYNOS4210_MUX_HPM_MASK (1 << 20)
>> +
>> +/**
>> + * struct exynos4210_armclk_data: config data to setup exynos4210 cpu
>> clocks.
>> + * @prate: frequency of the parent clock.
>> + * @div0:value to be programmed in the div_cpu0 register.
>> + * @div1:value to be programmed in the div_cpu1 register.
>> + *
>> + * This structure holds the divider configuration data for divider clocks
>> + * belonging to the CMU_CPU clock domain. The parent frequency at which
>> these
>> + * divider values are vaild is specified in @prate.
>
> s/vaild/valid/
>
>> + */
>> +struct exynos4210_armclk_data {
>> + unsigned long prate;
>> + unsigned intdiv0;
>> + unsigned intdiv1;
>> +};
>> +
>> +/**
>> + * struct exynos_cpuclk: information about clock supplied to a CPU core.
>> + * @hw: handle between ccf and cpu clock.
>
> s/ccf/CCF/
> s/cpu/CPU/
>
>> + * @alt_parent: alternate parent clock to use when switching the speed
>> + * of the primary parent clock.
>> + * @ctrl_base: base address of the clock controller.
>> + * @offset: offset from the ctrl_base address where the cpu clock div/mux
>
> s/cpu/CPU/
>
>> + * registers can be accessed.
>> + * @clk_nb: clock notifier registered for changes in clock speed of the
>> + * primary parent clock.
>> + * @lock:register access lock.
>> + * @data:optional data which the acutal instantiation of this clock
>> + * can use.
>
> s/acutal/actual/
>
>> + */
>> +struct exynos_cpuclk {
>> + struct clk_hw hw;
>> + struct clk *alt_parent;
>> + void __iomem*ctrl_base;
>> + unsigned long offset;
>> + struct notifier_block clk_nb;
>> + spinlock_t *lock;
>> + void*data;
>> +};
>> +
>> +#define to_exynos_cpuclk_hw(hw) container_of(hw, str
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Hi Thomas,
On 14.05.2014 03:11, Thomas Abraham wrote:
> From: Thomas Abraham
>
> 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.
>
> Cc: Tomasz Figa
> Signed-off-by: Thomas Abraham
> ---
> drivers/clk/samsung/Makefile |2 +-
> drivers/clk/samsung/clk-cpu.c | 458
> +
> drivers/clk/samsung/clk.h |5 +
> 3 files changed, 464 insertions(+), 1 deletions(-)
> create mode 100644 drivers/clk/samsung/clk-cpu.c
>
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 8eb4799..e2b453f 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -2,7 +2,7 @@
> # Samsung Clock specific Makefile
> #
>
> -obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
> +obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o clk-cpu.o
> obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
> obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
> obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> new file mode 100644
> index 000..6a40862
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-cpu.c
> @@ -0,0 +1,458 @@
> +/*
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Author: Thomas Abraham
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This file contains the utility functions to register the cpu clocks
> + * for samsung platforms.
s/cpu/CPU/
s/samsung/Samsung/
> +*/
> +
> +#include
> +#include "clk.h"
> +
> +#define SRC_CPU 0x0
> +#define STAT_CPU 0x200
> +#define DIV_CPU0 0x300
> +#define DIV_CPU1 0x304
> +#define DIV_STAT_CPU00x400
> +#define DIV_STAT_CPU10x404
> +
> +#define MAX_DIV 8
> +
> +#define EXYNOS4210_ARM_DIV1(div) ((div & 0x7) + 1)
> +#define EXYNOS4210_ARM_DIV2(div) (((div >> 28) & 0x7) + 1)
> +
> +#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0) \
> + ((d5 << 24) | (d4 << 20) | (d3 << 16) | (d2 << 12) |\
> + (d1 << 8) | (d0 << 4))
> +#define EXYNOS4210_DIV_CPU1(d2, d1, d0)
> \
> + ((d2 << 8) | (d1 << 4) | (d0 << 0))
Macro arguments should be put into parentheses to make sure that whole
argument is subject to further arithmetic operations.
> +
> +#define EXYNOS4210_DIV1_HPM_MASK ((0x7 << 0) | (0x7 << 4))
> +#define EXYNOS4210_MUX_HPM_MASK (1 << 20)
> +
> +/**
> + * struct exynos4210_armclk_data: config data to setup exynos4210 cpu clocks.
> + * @prate: frequency of the parent clock.
> + * @div0:value to be programmed in the div_cpu0 register.
> + * @div1:value to be programmed in the div_cpu1 register.
> + *
> + * This structure holds the divider configuration data for divider clocks
> + * belonging to the CMU_CPU clock domain. The parent frequency at which these
> + * divider values are vaild is specified in @prate.
s/vaild/valid/
> + */
> +struct exynos4210_armclk_data {
> + unsigned long prate;
> + unsigned intdiv0;
> + unsigned intdiv1;
> +};
> +
> +/**
> + * struct exynos_cpuclk: information about clock supplied to a CPU core.
> + * @hw: handle between ccf and cpu clock.
s/ccf/CCF/
s/cpu/CPU/
> + * @alt_parent: alternate parent clock to use when switching the speed
> + * of the primary parent clock.
> + * @ctrl_base: base address of the clock controller.
> + * @offset: offset from the ctrl_base address where the cpu clock div/mux
s/cpu/CPU/
> + * registers can be accessed.
> + * @clk_nb: clock notifier registered for changes in clock speed of the
> + * primary parent clock.
> + * @lock:register access lock.
> + * @data:optional data which the acutal instantiation of this clock
> + * can use.
s/acutal/actual/
> + */
> +struct exynos_cpuclk {
> + struct clk_hw hw;
> + struct clk *alt_parent;
> + void __iomem*ctrl_base;
> + unsigned long offset;
> + struct notifier_block clk_nb;
> + spinlock_t *lock;
> + void*data;
> +};
> +
> +#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)
> +#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk,
> clk_nb)
> +
> +/**
> + * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
> + * @parser: pointer to a f
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
On Fri, May 16, 2014 at 1:56 AM, Doug Anderson wrote:
> Heiko,
>
> On Thu, May 15, 2014 at 1:12 PM, Heiko Stübner wrote:
>> Hi Doug,
>>
>> Am Donnerstag, 15. Mai 2014, 12:36:45 schrieb Doug Anderson:
>>> On Thu, May 15, 2014 at 12:17 PM, Heiko Stübner wrote:
>>> > Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
>>> >> Thomas,
>>> >>
>>> >> On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham
>> wrote:
>>> >> > From: Thomas Abraham
>>> >> > +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
>>> >> > *ndata, + struct exynos_cpuclk *armclk, void
>>> >> > __iomem *base) +{
>>> >> > + struct exynos4210_armclk_data *armclk_data = armclk->data;
>>> >> > + unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
>>> >> > + unsigned long alt_div, div0, div1, tdiv0, mux_reg;
>>> >> > + unsigned long cur_armclk_rate, timeout;
>>> >> > + unsigned long flags;
>>> >> > +
>>> >> > + /* find out the divider values to use for clock data */
>>> >> > + while (armclk_data->prate != ndata->new_rate) {
>>> >> > + if (armclk_data->prate == 0)
>>> >> > + return -EINVAL;
>>> >> > + armclk_data++;
>>> >> > + }
>>> >> > +
>>> >> > + div0 = armclk_data->div0;
>>> >> > + div1 = armclk_data->div1;
>>> >> > + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
>>> >> > + div1 = readl(base + DIV_CPU1) &
>>> >> > EXYNOS4210_DIV1_HPM_MASK;
>>> >> > + div1 |= ((armclk_data->div1) &
>>> >> > ~EXYNOS4210_DIV1_HPM_MASK);
>>> >> > + }
>>> >> > +
>>> >> > + /*
>>> >> > +* if the new and old parent clock speed is less than the clock
>>> >> > speed +* of the alternate parent, then it should be ensured
>>> >> > that
>>> >> > at no point +* the armclk speed is more than the old_prate
>>> >> > until
>>> >> > the dividers are +* set.
>>> >> > +*/
>>> >> > + tdiv0 = readl(base + DIV_CPU0);
>>> >> > + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0)
>>> >> > /
>>> >> > + EXYNOS4210_ARM_DIV2(tdiv0);
>>> >> > + if (alt_prate > cur_armclk_rate) {
>>> >> > + alt_div = _calc_div(alt_prate, cur_armclk_rate);
>>> >> > + _exynos4210_set_armclk_div(base, alt_div);
>>> >> > + div0 |= alt_div;
>>> >>
>>> >> Don't you need to up the voltage here, too? ...I haven't reviewed
>>> >> this whole patch (so perhaps it's elsewhere in the patch or in the
>>> >> series), but I stumbled upon this while trying to solve a different
>>> >> problem and figured I'd check...
>>> >
>>> > setting the voltage should be done by the cpufreq driver like cpufreq-cpu0
>>> > - whose usage this series intents to allow.
>>> >
>>> > As I've hijacked Thomas' concept for my current rockchip clock work, I've
>>> > already seen this working nicely :-) .
>>>
>>> I guess I should have been more clear. I was talking more
>>> specifically about upping the voltage as part of the mux switch in the
>>> case that alt_prate > cur_armclk_rate.
>>
>> from earlier discussions I remember Thomas and me talked about setting a
>> divider to make sure that alt_prate <= cur_armclk_rate, so the voltage can
>> stay at its current level. I haven't looked deeply into this revision, but
>> the
>> last one did exactly this.
>
> Ah-ha, that's a reasonable solution to this problem. I was familiar
> with the old code and know that it used to set the CPUD ratio here, so
> I assumed that was what Thomas's code did. ...but you're right, his
> code is setting the divider here. I didn't double-check all of his
> code / calculations, but he's certainly tweaking the right bits.
>
> Nice!
>
>
>>> ...if you're switching from 200MHz to 300MHz and the alt_prate is
>>> 800MHz, you need to account for that fact. The code here accounts for
>>> the fact in setting the "armclk_div", but (I don't think) it accounts
>>> for the fact that 800MHz will need a higher voltage.
>>>
>>> As per a separate discussion, a clean solution might be to move the
>>> mux switching to the core of CPU_FREQ. That would have the side
>>> effect of also making it very easy to send notifications.
>>
>> I'll just wait until you all decide what the best solution is :-), but
>> personally I like the concept of keeping the clock logic inside the clock
>> driver, especially as this is not limited to setting the mux but also
>> adapting
>> tightly bound child clocks and this all may not fit into a generic
>> implementation of a cpufreq driver.
+1. I also prefer to go with this approach.
>
> Yup, I didn't think of the solution you guys came up with. Your
> solution handles things nicely.
>
> Thanks!
>
> -Doug
Doug,
Heiko had suggested to use the divider to keep the armclk within
limits during the frequency transition. This helped to avoid the
corresponding voltage scaling step which greatly simplified t
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Heiko,
On Thu, May 15, 2014 at 1:12 PM, Heiko Stübner wrote:
> Hi Doug,
>
> Am Donnerstag, 15. Mai 2014, 12:36:45 schrieb Doug Anderson:
>> On Thu, May 15, 2014 at 12:17 PM, Heiko Stübner wrote:
>> > Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
>> >> Thomas,
>> >>
>> >> On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham
> wrote:
>> >> > From: Thomas Abraham
>> >> > +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
>> >> > *ndata, + struct exynos_cpuclk *armclk, void
>> >> > __iomem *base) +{
>> >> > + struct exynos4210_armclk_data *armclk_data = armclk->data;
>> >> > + unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
>> >> > + unsigned long alt_div, div0, div1, tdiv0, mux_reg;
>> >> > + unsigned long cur_armclk_rate, timeout;
>> >> > + unsigned long flags;
>> >> > +
>> >> > + /* find out the divider values to use for clock data */
>> >> > + while (armclk_data->prate != ndata->new_rate) {
>> >> > + if (armclk_data->prate == 0)
>> >> > + return -EINVAL;
>> >> > + armclk_data++;
>> >> > + }
>> >> > +
>> >> > + div0 = armclk_data->div0;
>> >> > + div1 = armclk_data->div1;
>> >> > + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
>> >> > + div1 = readl(base + DIV_CPU1) &
>> >> > EXYNOS4210_DIV1_HPM_MASK;
>> >> > + div1 |= ((armclk_data->div1) &
>> >> > ~EXYNOS4210_DIV1_HPM_MASK);
>> >> > + }
>> >> > +
>> >> > + /*
>> >> > +* if the new and old parent clock speed is less than the clock
>> >> > speed +* of the alternate parent, then it should be ensured
>> >> > that
>> >> > at no point +* the armclk speed is more than the old_prate
>> >> > until
>> >> > the dividers are +* set.
>> >> > +*/
>> >> > + tdiv0 = readl(base + DIV_CPU0);
>> >> > + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0)
>> >> > /
>> >> > + EXYNOS4210_ARM_DIV2(tdiv0);
>> >> > + if (alt_prate > cur_armclk_rate) {
>> >> > + alt_div = _calc_div(alt_prate, cur_armclk_rate);
>> >> > + _exynos4210_set_armclk_div(base, alt_div);
>> >> > + div0 |= alt_div;
>> >>
>> >> Don't you need to up the voltage here, too? ...I haven't reviewed
>> >> this whole patch (so perhaps it's elsewhere in the patch or in the
>> >> series), but I stumbled upon this while trying to solve a different
>> >> problem and figured I'd check...
>> >
>> > setting the voltage should be done by the cpufreq driver like cpufreq-cpu0
>> > - whose usage this series intents to allow.
>> >
>> > As I've hijacked Thomas' concept for my current rockchip clock work, I've
>> > already seen this working nicely :-) .
>>
>> I guess I should have been more clear. I was talking more
>> specifically about upping the voltage as part of the mux switch in the
>> case that alt_prate > cur_armclk_rate.
>
> from earlier discussions I remember Thomas and me talked about setting a
> divider to make sure that alt_prate <= cur_armclk_rate, so the voltage can
> stay at its current level. I haven't looked deeply into this revision, but the
> last one did exactly this.
Ah-ha, that's a reasonable solution to this problem. I was familiar
with the old code and know that it used to set the CPUD ratio here, so
I assumed that was what Thomas's code did. ...but you're right, his
code is setting the divider here. I didn't double-check all of his
code / calculations, but he's certainly tweaking the right bits.
Nice!
>> ...if you're switching from 200MHz to 300MHz and the alt_prate is
>> 800MHz, you need to account for that fact. The code here accounts for
>> the fact in setting the "armclk_div", but (I don't think) it accounts
>> for the fact that 800MHz will need a higher voltage.
>>
>> As per a separate discussion, a clean solution might be to move the
>> mux switching to the core of CPU_FREQ. That would have the side
>> effect of also making it very easy to send notifications.
>
> I'll just wait until you all decide what the best solution is :-), but
> personally I like the concept of keeping the clock logic inside the clock
> driver, especially as this is not limited to setting the mux but also adapting
> tightly bound child clocks and this all may not fit into a generic
> implementation of a cpufreq driver.
Yup, I didn't think of the solution you guys came up with. Your
solution handles things nicely.
Thanks!
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Hi Doug,
Am Donnerstag, 15. Mai 2014, 12:36:45 schrieb Doug Anderson:
> On Thu, May 15, 2014 at 12:17 PM, Heiko Stübner wrote:
> > Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
> >> Thomas,
> >>
> >> On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham
wrote:
> >> > From: Thomas Abraham
> >> > +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
> >> > *ndata, + struct exynos_cpuclk *armclk, void
> >> > __iomem *base) +{
> >> > + struct exynos4210_armclk_data *armclk_data = armclk->data;
> >> > + unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
> >> > + unsigned long alt_div, div0, div1, tdiv0, mux_reg;
> >> > + unsigned long cur_armclk_rate, timeout;
> >> > + unsigned long flags;
> >> > +
> >> > + /* find out the divider values to use for clock data */
> >> > + while (armclk_data->prate != ndata->new_rate) {
> >> > + if (armclk_data->prate == 0)
> >> > + return -EINVAL;
> >> > + armclk_data++;
> >> > + }
> >> > +
> >> > + div0 = armclk_data->div0;
> >> > + div1 = armclk_data->div1;
> >> > + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
> >> > + div1 = readl(base + DIV_CPU1) &
> >> > EXYNOS4210_DIV1_HPM_MASK;
> >> > + div1 |= ((armclk_data->div1) &
> >> > ~EXYNOS4210_DIV1_HPM_MASK);
> >> > + }
> >> > +
> >> > + /*
> >> > +* if the new and old parent clock speed is less than the clock
> >> > speed +* of the alternate parent, then it should be ensured
> >> > that
> >> > at no point +* the armclk speed is more than the old_prate
> >> > until
> >> > the dividers are +* set.
> >> > +*/
> >> > + tdiv0 = readl(base + DIV_CPU0);
> >> > + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0)
> >> > /
> >> > + EXYNOS4210_ARM_DIV2(tdiv0);
> >> > + if (alt_prate > cur_armclk_rate) {
> >> > + alt_div = _calc_div(alt_prate, cur_armclk_rate);
> >> > + _exynos4210_set_armclk_div(base, alt_div);
> >> > + div0 |= alt_div;
> >>
> >> Don't you need to up the voltage here, too? ...I haven't reviewed
> >> this whole patch (so perhaps it's elsewhere in the patch or in the
> >> series), but I stumbled upon this while trying to solve a different
> >> problem and figured I'd check...
> >
> > setting the voltage should be done by the cpufreq driver like cpufreq-cpu0
> > - whose usage this series intents to allow.
> >
> > As I've hijacked Thomas' concept for my current rockchip clock work, I've
> > already seen this working nicely :-) .
>
> I guess I should have been more clear. I was talking more
> specifically about upping the voltage as part of the mux switch in the
> case that alt_prate > cur_armclk_rate.
from earlier discussions I remember Thomas and me talked about setting a
divider to make sure that alt_prate <= cur_armclk_rate, so the voltage can
stay at its current level. I haven't looked deeply into this revision, but the
last one did exactly this.
> ...if you're switching from 200MHz to 300MHz and the alt_prate is
> 800MHz, you need to account for that fact. The code here accounts for
> the fact in setting the "armclk_div", but (I don't think) it accounts
> for the fact that 800MHz will need a higher voltage.
>
> As per a separate discussion, a clean solution might be to move the
> mux switching to the core of CPU_FREQ. That would have the side
> effect of also making it very easy to send notifications.
I'll just wait until you all decide what the best solution is :-), but
personally I like the concept of keeping the clock logic inside the clock
driver, especially as this is not limited to setting the mux but also adapting
tightly bound child clocks and this all may not fit into a generic
implementation of a cpufreq driver.
And this is also working really nice on my rockchip platform.
Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Heiko,
On Thu, May 15, 2014 at 12:17 PM, Heiko Stübner wrote:
> Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
>> Thomas,
>>
>> On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham wrote:
>> > From: Thomas Abraham
>> > +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
>> > *ndata, + struct exynos_cpuclk *armclk, void
>> > __iomem *base) +{
>> > + struct exynos4210_armclk_data *armclk_data = armclk->data;
>> > + unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
>> > + unsigned long alt_div, div0, div1, tdiv0, mux_reg;
>> > + unsigned long cur_armclk_rate, timeout;
>> > + unsigned long flags;
>> > +
>> > + /* find out the divider values to use for clock data */
>> > + while (armclk_data->prate != ndata->new_rate) {
>> > + if (armclk_data->prate == 0)
>> > + return -EINVAL;
>> > + armclk_data++;
>> > + }
>> > +
>> > + div0 = armclk_data->div0;
>> > + div1 = armclk_data->div1;
>> > + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
>> > + div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
>> > + div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
>> > + }
>> > +
>> > + /*
>> > +* if the new and old parent clock speed is less than the clock
>> > speed +* of the alternate parent, then it should be ensured that
>> > at no point +* the armclk speed is more than the old_prate until
>> > the dividers are +* set.
>> > +*/
>> > + tdiv0 = readl(base + DIV_CPU0);
>> > + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
>> > + EXYNOS4210_ARM_DIV2(tdiv0);
>> > + if (alt_prate > cur_armclk_rate) {
>> > + alt_div = _calc_div(alt_prate, cur_armclk_rate);
>> > + _exynos4210_set_armclk_div(base, alt_div);
>> > + div0 |= alt_div;
>>
>> Don't you need to up the voltage here, too? ...I haven't reviewed
>> this whole patch (so perhaps it's elsewhere in the patch or in the
>> series), but I stumbled upon this while trying to solve a different
>> problem and figured I'd check...
>
> setting the voltage should be done by the cpufreq driver like cpufreq-cpu0 -
> whose usage this series intents to allow.
>
> As I've hijacked Thomas' concept for my current rockchip clock work, I've
> already seen this working nicely :-) .
I guess I should have been more clear. I was talking more
specifically about upping the voltage as part of the mux switch in the
case that alt_prate > cur_armclk_rate.
...if you're switching from 200MHz to 300MHz and the alt_prate is
800MHz, you need to account for that fact. The code here accounts for
the fact in setting the "armclk_div", but (I don't think) it accounts
for the fact that 800MHz will need a higher voltage.
As per a separate discussion, a clean solution might be to move the
mux switching to the core of CPU_FREQ. That would have the side
effect of also making it very easy to send notifications.
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
> Thomas,
>
> On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham wrote:
> > From: Thomas Abraham
> > +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
> > *ndata, + struct exynos_cpuclk *armclk, void
> > __iomem *base) +{
> > + struct exynos4210_armclk_data *armclk_data = armclk->data;
> > + unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
> > + unsigned long alt_div, div0, div1, tdiv0, mux_reg;
> > + unsigned long cur_armclk_rate, timeout;
> > + unsigned long flags;
> > +
> > + /* find out the divider values to use for clock data */
> > + while (armclk_data->prate != ndata->new_rate) {
> > + if (armclk_data->prate == 0)
> > + return -EINVAL;
> > + armclk_data++;
> > + }
> > +
> > + div0 = armclk_data->div0;
> > + div1 = armclk_data->div1;
> > + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
> > + div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
> > + div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
> > + }
> > +
> > + /*
> > +* if the new and old parent clock speed is less than the clock
> > speed +* of the alternate parent, then it should be ensured that
> > at no point +* the armclk speed is more than the old_prate until
> > the dividers are +* set.
> > +*/
> > + tdiv0 = readl(base + DIV_CPU0);
> > + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
> > + EXYNOS4210_ARM_DIV2(tdiv0);
> > + if (alt_prate > cur_armclk_rate) {
> > + alt_div = _calc_div(alt_prate, cur_armclk_rate);
> > + _exynos4210_set_armclk_div(base, alt_div);
> > + div0 |= alt_div;
>
> Don't you need to up the voltage here, too? ...I haven't reviewed
> this whole patch (so perhaps it's elsewhere in the patch or in the
> series), but I stumbled upon this while trying to solve a different
> problem and figured I'd check...
setting the voltage should be done by the cpufreq driver like cpufreq-cpu0 -
whose usage this series intents to allow.
As I've hijacked Thomas' concept for my current rockchip clock work, I've
already seen this working nicely :-) .
Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham wrote:
> From: Thomas Abraham
> +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data *ndata,
> + struct exynos_cpuclk *armclk, void __iomem *base)
> +{
> + struct exynos4210_armclk_data *armclk_data = armclk->data;
> + unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
> + unsigned long alt_div, div0, div1, tdiv0, mux_reg;
> + unsigned long cur_armclk_rate, timeout;
> + unsigned long flags;
> +
> + /* find out the divider values to use for clock data */
> + while (armclk_data->prate != ndata->new_rate) {
> + if (armclk_data->prate == 0)
> + return -EINVAL;
> + armclk_data++;
> + }
> +
> + div0 = armclk_data->div0;
> + div1 = armclk_data->div1;
> + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
> + div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
> + div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
> + }
> +
> + /*
> +* if the new and old parent clock speed is less than the clock speed
> +* of the alternate parent, then it should be ensured that at no point
> +* the armclk speed is more than the old_prate until the dividers are
> +* set.
> +*/
> + tdiv0 = readl(base + DIV_CPU0);
> + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
> + EXYNOS4210_ARM_DIV2(tdiv0);
> + if (alt_prate > cur_armclk_rate) {
> + alt_div = _calc_div(alt_prate, cur_armclk_rate);
> + _exynos4210_set_armclk_div(base, alt_div);
> + div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks
From: Thomas Abraham
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.
Cc: Tomasz Figa
Signed-off-by: Thomas Abraham
---
drivers/clk/samsung/Makefile |2 +-
drivers/clk/samsung/clk-cpu.c | 458 +
drivers/clk/samsung/clk.h |5 +
3 files changed, 464 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index 8eb4799..e2b453f 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -2,7 +2,7 @@
# Samsung Clock specific Makefile
#
-obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
+obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o clk-cpu.o
obj-$(CONFIG_ARCH_EXYNOS4) += clk-exynos4.o
obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
new file mode 100644
index 000..6a40862
--- /dev/null
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,458 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Thomas Abraham
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This file contains the utility functions to register the cpu clocks
+ * for samsung platforms.
+*/
+
+#include
+#include "clk.h"
+
+#define SRC_CPU0x0
+#define STAT_CPU 0x200
+#define DIV_CPU0 0x300
+#define DIV_CPU1 0x304
+#define DIV_STAT_CPU0 0x400
+#define DIV_STAT_CPU1 0x404
+
+#define MAX_DIV8
+
+#define EXYNOS4210_ARM_DIV1(div) ((div & 0x7) + 1)
+#define EXYNOS4210_ARM_DIV2(div) (((div >> 28) & 0x7) + 1)
+
+#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0)\
+ ((d5 << 24) | (d4 << 20) | (d3 << 16) | (d2 << 12) |\
+(d1 << 8) | (d0 << 4))
+#define EXYNOS4210_DIV_CPU1(d2, d1, d0)
\
+ ((d2 << 8) | (d1 << 4) | (d0 << 0))
+
+#define EXYNOS4210_DIV1_HPM_MASK ((0x7 << 0) | (0x7 << 4))
+#define EXYNOS4210_MUX_HPM_MASK(1 << 20)
+
+/**
+ * struct exynos4210_armclk_data: config data to setup exynos4210 cpu clocks.
+ * @prate: frequency of the parent clock.
+ * @div0: value to be programmed in the div_cpu0 register.
+ * @div1: value to be programmed in the div_cpu1 register.
+ *
+ * This structure holds the divider configuration data for divider clocks
+ * belonging to the CMU_CPU clock domain. The parent frequency at which these
+ * divider values are vaild is specified in @prate.
+ */
+struct exynos4210_armclk_data {
+ unsigned long prate;
+ unsigned intdiv0;
+ unsigned intdiv1;
+};
+
+/**
+ * struct exynos_cpuclk: information about clock supplied to a CPU core.
+ * @hw:handle between ccf and cpu clock.
+ * @alt_parent:alternate parent clock to use when switching the speed
+ * of the primary parent clock.
+ * @ctrl_base: base address of the clock controller.
+ * @offset:offset from the ctrl_base address where the cpu clock div/mux
+ * registers can be accessed.
+ * @clk_nb:clock notifier registered for changes in clock speed of the
+ * primary parent clock.
+ * @lock: register access lock.
+ * @data: optional data which the acutal instantiation of this clock
+ * can use.
+ */
+struct exynos_cpuclk {
+ struct clk_hw hw;
+ struct clk *alt_parent;
+ void __iomem*ctrl_base;
+ unsigned long offset;
+ struct notifier_block clk_nb;
+ spinlock_t *lock;
+ void*data;
+};
+
+#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)
+#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk, clk_nb)
+
+/**
+ * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
+ * @parser:pointer to a function that can parse SoC specific data.
+ * @ops: clock operations to be used for this clock.
+ * @offset:optional offset from base of clock controller register base, to
+ * be used when accessing clock controller registers related to the
+ * cpu clock.
+ * @clk_cb:the clock notifier callback to be called for changes in the
+ * clock rate of the primary parent clock.
+ *
+ * This structure provides SoC specific da
