[PATCH] ARM: S5PV210: allow clk to use clksrc as parents
Kukjin Kim
kgene.kim at samsung.com
Mon Jul 12 01:12:25 EDT 2010
MyungJoo Ham wrote:
>
> Hello,
>
> On Mon, Jul 12, 2010 at 11:27 AM, Kukjin Kim <kgene.kim at samsung.com> wrote:
> > MyungJoo Ham wrote:
> >>
> >> Hello,
> >>
> >> On Fri, Jul 9, 2010 at 7:29 PM, Kukjin Kim <kgene.kim at samsung.com> wrote:
> >> > MyungJoo Ham wrote:
> >> >>
> >> > Hi,
> >> >
> >> > Cc'ed Ben Dooks.
> >> >
> >> >> This patch allows clk in init_clocks[] and init_clocks_disable[] to use
> >> >> clksrc_clk.clk in clksrcs[].
> >> >>
> >> >> For example, clk "lcd" may need to set "sclk-fimd" as a parent if
> >> >> the div and src settting of sclk-fimd changes and a user of "lcd" wants
> >> >> to get the exact information about the rate. If "sclk-fimd" is set as a
> >> >> parent of "lcd", then the clk framework can provide the wanted
> >> >> information; otherwise, the change in "sclk-fimd" does not update "lcd"
> >> >> properly.
> >> >>
> >> > I don't think so.
> >> >
> >> > This can be problem because the parent clock of LCD(FIMD) is not always
> >> > SCLK_FIMD. And LCD block is on DSYS clock domain, so the parent clock
> of
> >> > 'lcd' should be dsys clock, not sclk_fimd.
> >> > Basically used dsys bus clock as LCD IP clock and can be used sclk_fimd or
> >> > not. So should separate them and need to separate control.
> >>
> >> Well, yes SCLK_FIMD is not ALWAYS the parent of LCD(FIMD), but it is one of
> the
> >> possible parents of LCD(FIMD); thus, SCLK_FIMD should be able to become
> the
> >> parent. I'm not saying that SCLK_FIMD should be the parent, but SCLK_FIMD
> >> should
> >> be ABLE to be the parent. That's why I made it just "possible', not making it an
> >> actual parent.
> >>
> > More correctly, the clk 'lcd' means clock that should be used basically at FIMD
> block.
> > So it means used clock on system bus, should be SCLK_ACLK.
> >
> > Actually, if disabled clk 'lcd', we can't access FIMD block such as its register.
> >
> > As I say again, clk 'lcd' is on dsys clock domain, because of these meaning, so
> the parent clock of 'lcd' should be dsys clock.
>
> Ok, then, clk_get_rate(lcd) returns 166MHz, always, which in turn,
> makes it more difficult
> for lcd clock users to get the correct current rate when sclk_fimd's
> DIV changes. When we need to show that lcd is in dsys domain, isn't it
> enough to set dsys is an ancestor (grandparent) of lcd?
>
> I thought it'd contain more semantics if lcd's parent is fimd and
> fimd's parent is dsys; i.e., dsys is still an ancestor of lcd and
> clk_get_rate(lcd) may return the clock value of lcd while DIV/SRC
> changes. When lcd does not use sclk_fimd anymore, we can
> set_parent(lcd, something) anytime (to do this more easily, we may
> need to define lcd within clksrc_clk).
>
>
Please refer to below diagram.
------------------------------------
dsys bus
----------------+-------------------
|
|1.clk 'lcd'
|
+---+-----------+
| | FIMD block|
| +-+ |
| | |
| | |\ |
| +-|m| |
2.SCLK_FIMD | |u|----+ |
------------+---|x| | |
| |/ | |
| | |
+----------+----+
|
inside of SoC |
-----------------------+--------------------------
outside of SoC |
| 3.clk?
|
+--------------+
| LCD module |
+--------------+
In clock.c the clk 'lcd' means #1 in above diagram.
So you mean clk 'lcd' is #3', maybe confused :-(
> Besides, we may need to allow clock-clksrc.c's s3c_register_clksrc
> function to use the default .parent value to set the SRC register
> accordingly. For example (a patch under construction, not uploaded yet
> and requires patches submitted before):
>
>
> Author: MyungJoo Ham <myungjoo.ham at samsung.com>
> Date: Thu Jul 8 18:34:01 2010 +0900
>
> ARM: SAMSUNG SoC: clksrc register uses parent value to set default src.
>
> If .parent field is filled when a struct clksrc_clk is registered,
> s3c_register_clksrc sets the source clock register accordingly to the
> parent clock.
>
> In the previous versions, the parent value was "ignored" because the
> value of default register value overwrites the parent field.
>
> diff --git a/arch/arm/plat-samsung/clock-clksrc.c
> b/arch/arm/plat-samsung/clock-clksrc.c
> index ae8b850..236fb59 100644
> --- a/arch/arm/plat-samsung/clock-clksrc.c
> +++ b/arch/arm/plat-samsung/clock-clksrc.c
> @@ -177,7 +177,7 @@ static struct clk_ops clksrc_ops_nosrc = {
>
> void __init s3c_register_clksrc(struct clksrc_clk *clksrc, int size)
> {
> - int ret;
> + int ret, err = 0;
>
> for (; size > 0; size--, clksrc++) {
> if (!clksrc->reg_div.reg && !clksrc->reg_src.reg)
> @@ -195,12 +195,25 @@ void __init s3c_register_clksrc(struct
> clksrc_clk *clksrc, int size)
> clksrc->clk.ops = &clksrc_ops;
> }
>
> + if (clksrc->clk.parent && clksrc->sources &&
> + clksrc->reg_src.reg) {
> + err = s3c_setparent_clksrc(&clksrc->clk,
> + clksrc->clk.parent);
> + if (err < 0)
> + printk(KERN_ERR "[%s:%d] setparent error
> at reg"
> + "ister_clksrc %s p
> = %s...\n",
> + __FILE__, __LINE__,
> + clksrc->clk.name,
> + clksrc->clk.parent->name);
> + }
> +
> /* setup the clocksource, but do not announce it
> * as it may be re-set by the setup routines
> * called after the rest of the clocks have been
> * registered
> */
> - s3c_set_clksrc(clksrc, false);
> + if (err < 0)
> + s3c_set_clksrc(clksrc, false);
>
> ret = s3c24xx_register_clock(&clksrc->clk);
>
>
>
>
> >
> >> However, in order to make it look better, we may need to make LCD(FIMD)
> easier
> >> to switch between SCLK_ACLK and SCLK_FIMD; e.g., by making it another
> >> clksrc_clk
> >> with source clocks of SCLK_ACLK and SCLK_FIMD controlling with
> CLK_GATE_IP.
> >>
> >> >
> >> > As I still saying, the CLK_GATE_IP1[0] for LCD block can gate clock of
> >> > SCLK_ACLK and SCLK_FIMD which can be source of LCD block, and
> >> > CLK_SRC_MASK0[5] can gate only SCLK_FIMD, output clock of MUX FIMD.
> >> >
> >> >> To do so, we have relocated clksrcs[] and put enum indexes to clksrcs in
> >> >> order to allow clk in init_clocks[] and init_clocks_disable[] may refer
> >> >> one of clksrcs[] as its parent. (enum clksrc_refer)
> >> >>
> >> >> For example, "lcd" may set "sclk-fimd" as a parent if the following line
> >> >> replaces the line 878:
> >> >>
> >> >> .parent = &clk_hclk_dsys.clk,
> >> >>
> >> >> to
> >> >>
> >> >> .parent = &clksrcs[_clksrc_sclk_fimd],
> >> >>
> >> >
> >> >
> >> >> Signed-off-by: MyungJoo Ham <myungjoo.ham at samsung.com>
> >> >> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> >> >> ---
> >> >> arch/arm/mach-s5pv210/clock.c | 986
> >> > ++++++++++++++++++++++-------------------
> >> >> 1 files changed, 520 insertions(+), 466 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> >> >> index aa2b1c5..e33c6ed 100644
> >> >> --- a/arch/arm/mach-s5pv210/clock.c
> >> >> +++ b/arch/arm/mach-s5pv210/clock.c
> >> >> @@ -264,6 +264,526 @@ static struct clksrc_clk clk_sclk_vpll = {
> >> >> .reg_src = { .reg = S5P_CLK_SRC0, .shift = 12, .size = 1 },
> >> >> };
> >> >>
> >> >
> >> > And...hmm...maybe made this patch with wrong way...
> >> >
> >> >> +static struct clk *clkset_uart_list[] = {
> >> >> + [6] = &clk_mout_mpll.clk,
> >> >> + [7] = &clk_mout_epll.clk,
> >> >> +};
> >> >> +
> >> >
> >> > Following is from this patch...
> >> >
> >> > -static struct clk *clkset_uart_list[] = {
> >> > - [6] = &clk_mout_mpll.clk,
> >> > - [7] = &clk_mout_epll.clk,
> >> > -};
> >> > -
> >> >
> >> > Same? :-(
> >> > Why did you remove and add same code?
> >>
> >> It is done so in order to make clksrc_clk to be able to be parents of
> >> clk. (same with the
> >> later parts)
> >>
> >> To do so, I moved clksrc_clk to the front of clk definitions, which in
> >> turn moved
> >> clock source list to the front of clksrc_clk.
> >>
> >> Or, in order to reduce the changed lines number, would it be better to declare
> >> the list of struct clksrc_clk before and keep the locations of the lists?
> >>
> >>
> >> >
> >> > (snip)
> >> >
> >> > And...see the below.
> >> >
> >> > Added like below...
> >> >
> >> >> +static struct clksrc_clk clksrcs[] = {
> >> >> + [_clksrc_sclk_dmc] = {
> >> >> + .clk = {
> >> >> + .name = "sclk_dmc",
> >> >> + .id = -1,
> >> >> + },
> >> >> + .sources = &clkset_group1,
> >> >> + .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> >> + .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> >> + },
> >> >> + [_clksrc_sclk_onenand] = {
> >> >> + .clk = {
> >> >> + .name = "sclk_onenand",
> >> >> + .id = -1,
> >> >> + },
> >> >> + .sources = &clkset_sclk_onenand,
> >> >> + .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> >> >> + .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> >> >> + },
> >> >
> >> > (snip)
> >> >
> >> > And removed like below...
> >> >
> >> >> -static struct clksrc_clk clksrcs[] = {
> >> >> - {
> >> >> - .clk = {
> >> >> - .name = "sclk_dmc",
> >> >> - .id = -1,
> >> >> - },
> >> >> - .sources = &clkset_group1,
> >> >> - .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> >> - .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> >> - }, {
> >> >> - .clk = {
> >> >> - .name = "sclk_onenand",
> >> >> - .id = -1,
> >> >> - },
> >> >> - .sources = &clkset_sclk_onenand,
> >> >> - .reg_src = { .reg = S5P_CLK_SRC0, .shift = 28, .size = 1 },
> >> >> - .reg_div = { .reg = S5P_CLK_DIV6, .shift = 12, .size = 3 },
> >> >> - }, {
> >> >
> >> > But only changed small lines not every.
> >> > Following is from my local machine.
> >> >
> >> > @@ -665,7 +665,7 @@ static struct clksrc_sources clkset_group2 = {
> >> > };
> >> >
> >> > static struct clksrc_clk clksrcs[] = {
> >> > - {
> >> > + [_clksrc_sclk_dmc] = {
> >> > .clk = {
> >> > .name = "sclk_dmc",
> >> > .id = -1,
> >> > @@ -673,7 +673,8 @@ static struct clksrc_clk clksrcs[] = {
> >> > .sources = &clkset_group1,
> >> > .reg_src = { .reg = S5P_CLK_SRC6, .shift = 24, .size = 2 },
> >> > .reg_div = { .reg = S5P_CLK_DIV6, .shift = 28, .size = 4 },
> >> > - }, {
> >> > + },
> >> > + [_clksrc_sclk_onenand] = {
> >> > .clk = {
> >> > .name = "sclk_onenand",
> >> > .id = -1,
> >> > (snip)
> >> >
> >> > Please make sure your patch has no problem before submitting.
> >> >
> >> > Thanks.
> >> >
> >> > Best regards,
> >> > Kgene.
> >> > --
> >> > Kukjin Kim <kgene.kim at samsung.com>, Senior Engineer,
> >> > SW Solution Development Team, Samsung Electronics Co., Ltd.
> >> >
> >> >
> >
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim at samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
More information about the linux-arm-kernel
mailing list