[PATCH 0/4] clk: samsung: small fixes and enhancements

[PATCH 0/4] clk: samsung: small fixes and enhancements

[Heiko Stübner]

Small fixes and enhancements that came up when implementing the
common clock support for s3c2443, s3c2416 and s3c2450.

The 3rd and 4th patch enable the adding of separate aliases
to previously defined clocks, as discussed with Sylwester Nawrocki.

This is often needed as some clocks need more than one alias. The hsmmc
hclks on s3c2416 for example are both "hsmmc" and "mmc_busclk.0" .

These changes are required for the common clock conversion of s3c24xx SoCs.

Heiko Stuebner (4):
  clk: samsung: register clk_div_tables for divider clocks
  clk: samsung: remove np check in clock init
  clk: samsung: always allocate the clk_table
  clk: samsung: add infrastructure to add separate aliases

 drivers/clk/samsung/clk.c |   51 +++++++++++++++++++++++++++++++++++++++-----
 drivers/clk/samsung/clk.h |   34 ++++++++++++++++++++++++++---
 2 files changed, 75 insertions(+), 10 deletions(-)

-- 
1.7.2.3

[PATCH 1/4] clk: samsung: register clk_div_tables for divider clocks

[Heiko Stübner]

On some Samsung platforms divider clocks only use specific divider combinations
like the armdiv on s3c2443 and s3c2416. For these usecases the generic divider
clock already provides the option of providing a lookup table mapping register
values to divider values.

Therefore add a new field to samsung_div_clock and if filled with a table,
use clk_register_divider_table instead of clk_register_divider to register
a divider clock

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/samsung/clk.c |   14 +++++++++++---
 drivers/clk/samsung/clk.h |   13 +++++++++----
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 91d12f3..d36cdd5 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -183,9 +183,17 @@ void __init samsung_clk_register_div(struct samsung_div_clock *list,
 	unsigned int idx, ret;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_divider(NULL, list->name, list->parent_name,
-			list->flags, reg_base + list->offset, list->shift,
-			list->width, list->div_flags, &lock);
+		if (list->table)
+			clk = clk_register_divider_table(NULL, list->name,
+					list->parent_name, list->flags,
+					reg_base + list->offset, list->shift,
+					list->width, list->div_flags,
+					list->table, &lock);
+		else
+			clk = clk_register_divider(NULL, list->name,
+					list->parent_name, list->flags,
+					reg_base + list->offset, list->shift,
+					list->width, list->div_flags, &lock);
 		if (IS_ERR(clk)) {
 			pr_err("%s: failed to register clock %s\n", __func__,
 				list->name);
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index 961192f..26a752b 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -150,9 +150,10 @@ struct samsung_div_clock {
 	u8			width;
 	u8			div_flags;
 	const char		*alias;
+	struct clk_div_table	*table;
 };
 
-#define __DIV(_id, dname, cname, pname, o, s, w, f, df, a)	\
+#define __DIV(_id, dname, cname, pname, o, s, w, f, df, a, t)	\
 	{							\
 		.id		= _id,				\
 		.dev_name	= dname,			\
@@ -164,16 +165,20 @@ struct samsung_div_clock {
 		.width		= w,				\
 		.div_flags	= df,				\
 		.alias		= a,				\
+		.table		= t,				\
 	}
 
 #define DIV(_id, cname, pname, o, s, w)				\
-	__DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL)
+	__DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL, NULL)
 
 #define DIV_A(_id, cname, pname, o, s, w, a)			\
-	__DIV(_id, NULL, cname, pname, o, s, w, 0, 0, a)
+	__DIV(_id, NULL, cname, pname, o, s, w, 0, 0, a, NULL)
 
 #define DIV_F(_id, cname, pname, o, s, w, f, df)		\
-	__DIV(_id, NULL, cname, pname, o, s, w, f, df, NULL)
+	__DIV(_id, NULL, cname, pname, o, s, w, f, df, NULL, NULL)
+
+#define DIV_T(_id, cname, pname, o, s, w, t)			\
+	__DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL, t)
 
 /**
  * struct samsung_gate_clock: information about gate clock
-- 
1.7.2.3

Re: [PATCH 1/4] clk: samsung: register clk_div_tables for divider clocks

[Thomas Abraham]

On 12 March 2013 05:13, Heiko Stübner <heiko@sntech.de> wrote:
> On some Samsung platforms divider clocks only use specific divider combinations
> like the armdiv on s3c2443 and s3c2416. For these usecases the generic divider
> clock already provides the option of providing a lookup table mapping register
> values to divider values.
>
> Therefore add a new field to samsung_div_clock and if filled with a table,
> use clk_register_divider_table instead of clk_register_divider to register
> a divider clock
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/samsung/clk.c |   14 +++++++++++---
>  drivers/clk/samsung/clk.h |   13 +++++++++----
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 91d12f3..d36cdd5 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -183,9 +183,17 @@ void __init samsung_clk_register_div(struct samsung_div_clock *list,
>         unsigned int idx, ret;
>
>         for (idx = 0; idx < nr_clk; idx++, list++) {
> -               clk = clk_register_divider(NULL, list->name, list->parent_name,
> -                       list->flags, reg_base + list->offset, list->shift,
> -                       list->width, list->div_flags, &lock);
> +               if (list->table)
> +                       clk = clk_register_divider_table(NULL, list->name,
> +                                       list->parent_name, list->flags,
> +                                       reg_base + list->offset, list->shift,
> +                                       list->width, list->div_flags,
> +                                       list->table, &lock);
> +               else
> +                       clk = clk_register_divider(NULL, list->name,
> +                                       list->parent_name, list->flags,
> +                                       reg_base + list->offset, list->shift,
> +                                       list->width, list->div_flags, &lock);
>                 if (IS_ERR(clk)) {
>                         pr_err("%s: failed to register clock %s\n", __func__,
>                                 list->name);
> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> index 961192f..26a752b 100644
> --- a/drivers/clk/samsung/clk.h
> +++ b/drivers/clk/samsung/clk.h
> @@ -150,9 +150,10 @@ struct samsung_div_clock {
>         u8                      width;
>         u8                      div_flags;
>         const char              *alias;
> +       struct clk_div_table    *table;
>  };
>
> -#define __DIV(_id, dname, cname, pname, o, s, w, f, df, a)     \
> +#define __DIV(_id, dname, cname, pname, o, s, w, f, df, a, t)  \
>         {                                                       \
>                 .id             = _id,                          \
>                 .dev_name       = dname,                        \
> @@ -164,16 +165,20 @@ struct samsung_div_clock {
>                 .width          = w,                            \
>                 .div_flags      = df,                           \
>                 .alias          = a,                            \
> +               .table          = t,                            \
>         }
>
>  #define DIV(_id, cname, pname, o, s, w)                                \
> -       __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL)
> +       __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL, NULL)
>
>  #define DIV_A(_id, cname, pname, o, s, w, a)                   \
> -       __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, a)
> +       __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, a, NULL)
>
>  #define DIV_F(_id, cname, pname, o, s, w, f, df)               \
> -       __DIV(_id, NULL, cname, pname, o, s, w, f, df, NULL)
> +       __DIV(_id, NULL, cname, pname, o, s, w, f, df, NULL, NULL)
> +
> +#define DIV_T(_id, cname, pname, o, s, w, t)                   \
> +       __DIV(_id, NULL, cname, pname, o, s, w, 0, 0, NULL, t)
>
>  /**
>   * struct samsung_gate_clock: information about gate clock
> --
> 1.7.2.3
>

Reviewed-by: Thomas Abraham <thomas.abraham@linaro.org>

[PATCH 2/4] clk: samsung: remove np check in clock init

[Heiko Stübner]

This let to the suspend init never being reached on non-DT platforms.

Signed-off-by: Heiko Stueber <heiko@sntech.de>
---
 drivers/clk/samsung/clk.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index d36cdd5..1a5de69 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -57,8 +57,6 @@ void __init samsung_clk_init(struct device_node *np, void __iomem *base,
 		unsigned long nr_rdump)
 {
 	reg_base = base;
-	if (!np)
-		return;
 
 #ifdef CONFIG_OF
 	clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
-- 
1.7.2.3

Re: [PATCH 2/4] clk: samsung: remove np check in clock init

[Thomas Abraham]

On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
> This let to the suspend init never being reached on non-DT platforms.
>
> Signed-off-by: Heiko Stueber <heiko@sntech.de>
> ---
>  drivers/clk/samsung/clk.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index d36cdd5..1a5de69 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -57,8 +57,6 @@ void __init samsung_clk_init(struct device_node *np, void __iomem *base,
>                 unsigned long nr_rdump)
>  {
>         reg_base = base;
> -       if (!np)
> -               return;

Hi Heiko,

Sorry, I did not understand the need for this. Could you please add
few more details on this change.

Thanks,
Thomas.

>
>  #ifdef CONFIG_OF
>         clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
> --
> 1.7.2.3
>

Re: [PATCH 2/4] clk: samsung: remove np check in clock init

[Heiko Stübner]

Am Dienstag, 12. März 2013, 09:53:00 schrieb Thomas Abraham:
> On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
> > This let to the suspend init never being reached on non-DT platforms.
> > 
> > Signed-off-by: Heiko Stueber <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/samsung/clk.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> > index d36cdd5..1a5de69 100644
> > --- a/drivers/clk/samsung/clk.c
> > +++ b/drivers/clk/samsung/clk.c
> > @@ -57,8 +57,6 @@ void __init samsung_clk_init(struct device_node *np,
> > void __iomem *base,
> > 
> >                 unsigned long nr_rdump)
> >  
> >  {
> >  
> >         reg_base = base;
> > 
> > -       if (!np)
> > -               return;
> 
> Hi Heiko,
> 
> Sorry, I did not understand the need for this. Could you please add
> few more details on this change.

Hi Thomas,

On non-dt platforms the init would stop here, therefore never reaching the 
code that inits the syscore ops below to save the register values on suspend 
and restores them on resume.

I might be overlooking something, but I think we want to save/restore the 
register values on both dt and non-dt platforms.


Heiko


> >  #ifdef CONFIG_OF
> >  
> >         clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
> > 
> > --
> > 1.7.2.3

Re: [PATCH 2/4] clk: samsung: remove np check in clock init

[Heiko Stübner]

Am Dienstag, 12. März 2013, 10:02:55 schrieb Heiko Stübner:
> Am Dienstag, 12. März 2013, 09:53:00 schrieb Thomas Abraham:
> > On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
> > > This let to the suspend init never being reached on non-DT platforms.
> > > 
> > > Signed-off-by: Heiko Stueber <heiko@sntech.de>
> > > ---
> > > 
> > >  drivers/clk/samsung/clk.c |    2 --
> > >  1 files changed, 0 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> > > index d36cdd5..1a5de69 100644
> > > --- a/drivers/clk/samsung/clk.c
> > > +++ b/drivers/clk/samsung/clk.c
> > > @@ -57,8 +57,6 @@ void __init samsung_clk_init(struct device_node *np,
> > > void __iomem *base,
> > > 
> > >                 unsigned long nr_rdump)
> > >  
> > >  {
> > >  
> > >         reg_base = base;
> > > 
> > > -       if (!np)
> > > -               return;
> > 
> > Hi Heiko,
> > 
> > Sorry, I did not understand the need for this. Could you please add
> > few more details on this change.
> 
> Hi Thomas,
> 
> On non-dt platforms the init would stop here, therefore never reaching the
> code that inits the syscore ops below to save the register values on
> suspend and restores them on resume.
> 
> I might be overlooking something, but I think we want to save/restore the
> register values on both dt and non-dt platforms.

ok, I did overlook something :-)

The register saving seems to be done separately on non-dt platforms (mach-
exynos/pm.c for example). So I probably need to redo patch 2 and 3.


Heiko


> > >  #ifdef CONFIG_OF
> > >  
> > >         clk_table = kzalloc(sizeof(struct clk *) * nr_clks,
> > >         GFP_KERNEL);
> > > 
> > > --
> > > 1.7.2.3

Re: [PATCH 2/4] clk: samsung: remove np check in clock init

[Thomas Abraham]

On 12 March 2013 14:47, Heiko Stübner <heiko@sntech.de> wrote:
> Am Dienstag, 12. März 2013, 10:02:55 schrieb Heiko Stübner:
>> Am Dienstag, 12. März 2013, 09:53:00 schrieb Thomas Abraham:
>> > On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
>> > > This let to the suspend init never being reached on non-DT platforms.
>> > >
>> > > Signed-off-by: Heiko Stueber <heiko@sntech.de>
>> > > ---
>> > >
>> > >  drivers/clk/samsung/clk.c |    2 --
>> > >  1 files changed, 0 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> > > index d36cdd5..1a5de69 100644
>> > > --- a/drivers/clk/samsung/clk.c
>> > > +++ b/drivers/clk/samsung/clk.c
>> > > @@ -57,8 +57,6 @@ void __init samsung_clk_init(struct device_node *np,
>> > > void __iomem *base,
>> > >
>> > >                 unsigned long nr_rdump)
>> > >
>> > >  {
>> > >
>> > >         reg_base = base;
>> > >
>> > > -       if (!np)
>> > > -               return;
>> >
>> > Hi Heiko,
>> >
>> > Sorry, I did not understand the need for this. Could you please add
>> > few more details on this change.
>>
>> Hi Thomas,
>>
>> On non-dt platforms the init would stop here, therefore never reaching the
>> code that inits the syscore ops below to save the register values on
>> suspend and restores them on resume.
>>
>> I might be overlooking something, but I think we want to save/restore the
>> register values on both dt and non-dt platforms.
>
> ok, I did overlook something :-)
>
> The register saving seems to be done separately on non-dt platforms (mach-
> exynos/pm.c for example). So I probably need to redo patch 2 and 3.

The register saving is supposed to be done in the clock driver itself
for both dt and non-dt platforms. So you are right in pointing out
this issue.

But in this patch, removing the check on 'np' is not right. Because,
on builds were CONFIG_OF is enabled but kernel image executing on
non-dt platforms, the allocation of clk_table should be avoided.

So instead of removing the check on 'np', the code inside #ifdef
CONFIG_PM_SLEEP should be placed after the assignment to reg_base. The
check on 'np' should then follow.

Thanks,
Thomas.

>
>
> Heiko
>
>
>> > >  #ifdef CONFIG_OF
>> > >
>> > >         clk_table = kzalloc(sizeof(struct clk *) * nr_clks,
>> > >         GFP_KERNEL);
>> > >
>> > > --
>> > > 1.7.2.3
>

Re: [PATCH 2/4] clk: samsung: remove np check in clock init

[Heiko Stübner]

Am Dienstag, 12. März 2013, 10:36:54 schrieb Thomas Abraham:
> On 12 March 2013 14:47, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Dienstag, 12. März 2013, 10:02:55 schrieb Heiko Stübner:
> >> Am Dienstag, 12. März 2013, 09:53:00 schrieb Thomas Abraham:
> >> > On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
> >> > > This let to the suspend init never being reached on non-DT
> >> > > platforms.
> >> > > 
> >> > > Signed-off-by: Heiko Stueber <heiko@sntech.de>
> >> > > ---
> >> > > 
> >> > >  drivers/clk/samsung/clk.c |    2 --
> >> > >  1 files changed, 0 insertions(+), 2 deletions(-)
> >> > > 
> >> > > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> >> > > index d36cdd5..1a5de69 100644
> >> > > --- a/drivers/clk/samsung/clk.c
> >> > > +++ b/drivers/clk/samsung/clk.c
> >> > > @@ -57,8 +57,6 @@ void __init samsung_clk_init(struct device_node
> >> > > *np, void __iomem *base,
> >> > > 
> >> > >                 unsigned long nr_rdump)
> >> > >  
> >> > >  {
> >> > >  
> >> > >         reg_base = base;
> >> > > 
> >> > > -       if (!np)
> >> > > -               return;
> >> > 
> >> > Hi Heiko,
> >> > 
> >> > Sorry, I did not understand the need for this. Could you please add
> >> > few more details on this change.
> >> 
> >> Hi Thomas,
> >> 
> >> On non-dt platforms the init would stop here, therefore never reaching
> >> the code that inits the syscore ops below to save the register values
> >> on suspend and restores them on resume.
> >> 
> >> I might be overlooking something, but I think we want to save/restore
> >> the register values on both dt and non-dt platforms.
> > 
> > ok, I did overlook something :-)
> > 
> > The register saving seems to be done separately on non-dt platforms
> > (mach- exynos/pm.c for example). So I probably need to redo patch 2 and
> > 3.
> 
> The register saving is supposed to be done in the clock driver itself
> for both dt and non-dt platforms. So you are right in pointing out
> this issue.
>
> But in this patch, removing the check on 'np' is not right. Because,
> on builds were CONFIG_OF is enabled but kernel image executing on
> non-dt platforms, the allocation of clk_table should be avoided.
> 
> So instead of removing the check on 'np', the code inside #ifdef
> CONFIG_PM_SLEEP should be placed after the assignment to reg_base. The
> check on 'np' should then follow.

The following two patches do exactly that ... allocating the clk_table all the 
time, to allow us to add separate aliases to clocks later.

So I'll wait until we talked about them :-) .


Thanks for looking thru the patches
Heiko

[PATCH 3/4] clk: samsung: always allocate the clk_table

[Heiko Stübner]

This is needed to allow looking up previous created clocks when
adding separate aliases to them.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/samsung/clk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 1a5de69..7c943f8 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -58,11 +58,11 @@ void __init samsung_clk_init(struct device_node *np, void __iomem *base,
 {
 	reg_base = base;
 
-#ifdef CONFIG_OF
 	clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
 	if (!clk_table)
 		panic("could not allocate clock lookup table\n");
 
+#ifdef CONFIG_OF
 	clk_data.clks = clk_table;
 	clk_data.clk_num = nr_clks;
 	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
-- 
1.7.2.3

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Thomas Abraham]

On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
> This is needed to allow looking up previous created clocks when
> adding separate aliases to them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/samsung/clk.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 1a5de69..7c943f8 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -58,11 +58,11 @@ void __init samsung_clk_init(struct device_node *np, void __iomem *base,
>  {
>         reg_base = base;
>
> -#ifdef CONFIG_OF
>         clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
>         if (!clk_table)
>                 panic("could not allocate clock lookup table\n");

This change is fine but one point that should be considered is that on
non-dt platforms, the memory allocation of clk_table cannot really be
justified just because few clocks require two or more aliases.

Instead, the optional alias passed for divider/mux register functions
can actually be a list of alias names, the list being terminated by a
zero-length string. The clock register helper functions can then loop
through that list and register all the aliases.

Thanks,
Thomas.

>
> +#ifdef CONFIG_OF
>         clk_data.clks = clk_table;
>         clk_data.clk_num = nr_clks;
>         of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> --
> 1.7.2.3
>

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Heiko Stübner]

Am Dienstag, 12. März 2013, 10:54:47 schrieb Thomas Abraham:
> On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
> > This is needed to allow looking up previous created clocks when
> > adding separate aliases to them.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/samsung/clk.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> > index 1a5de69..7c943f8 100644
> > --- a/drivers/clk/samsung/clk.c
> > +++ b/drivers/clk/samsung/clk.c
> > @@ -58,11 +58,11 @@ void __init samsung_clk_init(struct device_node *np,
> > void __iomem *base,
> > 
> >  {
> >  
> >         reg_base = base;
> > 
> > -#ifdef CONFIG_OF
> > 
> >         clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
> >         if (!clk_table)
> >         
> >                 panic("could not allocate clock lookup table\n");
> 
> This change is fine but one point that should be considered is that on
> non-dt platforms, the memory allocation of clk_table cannot really be
> justified just because few clocks require two or more aliases.

hmm, at least on s3c24xx there are quite a lot of the clocks requiring more 
than one alias. Also the clk_table is allocated on all dt platforms so these 
platforms have to handle the (small) memory consumption in any case and non-dt 
platforms are supposed to die out in the not to distant future.

I.e. I'm working on s3c24xx dt support, Tomasz is working on s3c64xx dt 
support, exynos5 already only has dt support and exynos4 will probably lose 
non-dt support at some point. And if someone converts the other s5p SoCs to 
the common clock framework they should already have the means to go to dt 
directly by then.

So the non-dt common-clock path is merely a "short" intermediate step to 
support smooth transitions to dt making the memory argument in my opinion a 
moot point :-)


Handling the aliases directly also requires adding quit a lot more 
{MUX/DIV/...}_*A aliases for all the necessary combinations.

And in general I also find the readability of separate aliases a lot better - 
as shown in the s3c2443 clk driver in the second series. And the removal once 
we only support dt will be a lot cleaner too.


Heiko

> Instead, the optional alias passed for divider/mux register functions
> can actually be a list of alias names, the list being terminated by a
> zero-length string. The clock register helper functions can then loop
> through that list and register all the aliases.


> > +#ifdef CONFIG_OF
> > 
> >         clk_data.clks = clk_table;
> >         clk_data.clk_num = nr_clks;
> >         of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > 
> > --
> > 1.7.2.3

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Thomas Abraham]

On 12 March 2013 16:20, Heiko Stübner <heiko@sntech.de> wrote:
> Am Dienstag, 12. März 2013, 10:54:47 schrieb Thomas Abraham:
>> On 12 March 2013 05:14, Heiko Stübner <heiko@sntech.de> wrote:
>> > This is needed to allow looking up previous created clocks when
>> > adding separate aliases to them.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> >
>> >  drivers/clk/samsung/clk.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> > index 1a5de69..7c943f8 100644
>> > --- a/drivers/clk/samsung/clk.c
>> > +++ b/drivers/clk/samsung/clk.c
>> > @@ -58,11 +58,11 @@ void __init samsung_clk_init(struct device_node *np,
>> > void __iomem *base,
>> >
>> >  {
>> >
>> >         reg_base = base;
>> >
>> > -#ifdef CONFIG_OF
>> >
>> >         clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
>> >         if (!clk_table)
>> >
>> >                 panic("could not allocate clock lookup table\n");
>>
>> This change is fine but one point that should be considered is that on
>> non-dt platforms, the memory allocation of clk_table cannot really be
>> justified just because few clocks require two or more aliases.
>
> hmm, at least on s3c24xx there are quite a lot of the clocks requiring more
> than one alias. Also the clk_table is allocated on all dt platforms so these
> platforms have to handle the (small) memory consumption in any case and non-dt
> platforms are supposed to die out in the not to distant future.
>
> I.e. I'm working on s3c24xx dt support, Tomasz is working on s3c64xx dt
> support, exynos5 already only has dt support and exynos4 will probably lose
> non-dt support at some point. And if someone converts the other s5p SoCs to
> the common clock framework they should already have the means to go to dt
> directly by then.
>
> So the non-dt common-clock path is merely a "short" intermediate step to
> support smooth transitions to dt making the memory argument in my opinion a
> moot point :-)
>
>
> Handling the aliases directly also requires adding quit a lot more
> {MUX/DIV/...}_*A aliases for all the necessary combinations.
>
> And in general I also find the readability of separate aliases a lot better -
> as shown in the s3c2443 clk driver in the second series. And the removal once
> we only support dt will be a lot cleaner too.

True, I agree on your comments.

For this patch,
Reviewed-by: Thomas Abraham <thomas.abraham@linaro.org>

>
>
> Heiko
>
>> Instead, the optional alias passed for divider/mux register functions
>> can actually be a list of alias names, the list being terminated by a
>> zero-length string. The clock register helper functions can then loop
>> through that list and register all the aliases.
>
>
>> > +#ifdef CONFIG_OF
>> >
>> >         clk_data.clks = clk_table;
>> >         clk_data.clk_num = nr_clks;
>> >         of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>> >
>> > --
>> > 1.7.2.3
>

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Sylwester Nawrocki]

Hi,

On 03/12/2013 10:54 AM, Thomas Abraham wrote:
> This change is fine but one point that should be considered is that on
> non-dt platforms, the memory allocation of clk_table cannot really be
> justified just because few clocks require two or more aliases.
>
> Instead, the optional alias passed for divider/mux register functions
> can actually be a list of alias names, the list being terminated by a
> zero-length string. The clock register helper functions can then loop
> through that list and register all the aliases.

Heiko's approach look much more clean to me and even for Exynos platform
which has currently about 350 clocks and is going to be dt-only starting
from 3.10 we are talking about 1.5kB of memory. Other (s3c64/24xx)
platforms have much less clocks (70...150).

IMHO all clocks aliases should be registered as it is done in Heiko's
patch series. And the clocks table could just be freed on non-dt
platforms if it is a real issue and anyone is concerned about it.

Regards,
Sylwester

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Thomas Abraham]

On 12 March 2013 16:53, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> Hi,
>
> On 03/12/2013 10:54 AM, Thomas Abraham wrote:
>> This change is fine but one point that should be considered is that on
>> non-dt platforms, the memory allocation of clk_table cannot really be
>> justified just because few clocks require two or more aliases.
>>
>> Instead, the optional alias passed for divider/mux register functions
>> can actually be a list of alias names, the list being terminated by a
>> zero-length string. The clock register helper functions can then loop
>> through that list and register all the aliases.
>
> Heiko's approach look much more clean to me and even for Exynos platform
> which has currently about 350 clocks and is going to be dt-only starting
> from 3.10 we are talking about 1.5kB of memory. Other (s3c64/24xx)
> platforms have much less clocks (70...150).
>
> IMHO all clocks aliases should be registered as it is done in Heiko's
> patch series. And the clocks table could just be freed on non-dt
> platforms if it is a real issue and anyone is concerned about it.

Ok, thanks.

And, you mentioned Exynos4 will be dt-only from 3.10. Does that mean
we just drop support for universal and nuri non-dt board support? Or,
will there be a equivalent dt support added for these boards?

Thanks,
Thomas.

>
> Regards,
> Sylwester

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Sylwester Nawrocki]

On 03/12/2013 12:46 PM, Thomas Abraham wrote:
> And, you mentioned Exynos4 will be dt-only from 3.10. Does that mean
> we just drop support for universal and nuri non-dt board support? Or,
> will there be a equivalent dt support added for these boards?

I think Tomasz has some initial dts files for Universal_c210 ready, and
Nuri could probably just be dropped. But we need Kyungmin's opinion on
that.

I'm not sure about other boards, they look pretty basic though.
So it shouldn't be difficult to replace them with corresponding dts
files. Currently there are:

arch/arm/mach-exynos/mach-smdk4x12.c
arch/arm/mach-exynos/mach-origen.c
arch/arm/mach-exynos/mach-nuri.c
arch/arm/mach-exynos/mach-universal_c210.c
arch/arm/mach-exynos/mach-armlex4210.c
arch/arm/mach-exynos/mach-smdkv310.c

And there are dts files for:

arch/arm/boot/dts/exynos4210-smdkv310.dts
arch/arm/boot/dts/exynos4210-origen.dts
arch/arm/boot/dts/exynos5250-smdk5250.dts
arch/arm/boot/dts/exynos5250-snow.dts
arch/arm/boot/dts/exynos4210-trats.dts
arch/arm/boot/dts/exynos5440-ssdk5440.dts
arch/arm/boot/dts/exynos4412-smdk4412.dts

So except Universal_c210 and Nuri the only ones not supporting booting
from DT are these two simple boards, which seem to be maintained by
Samsung:

arch/arm/mach-exynos/mach-smdk4x12.c
arch/arm/mach-exynos/mach-armlex4210.c

It would be nice to make Exynos DT only in this kernel release.
I guess there was enough time to get all boards converted to DT
already.

--

Regards,
Sylwester

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Thomas Abraham]

On 12 March 2013 19:18, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> On 03/12/2013 12:46 PM, Thomas Abraham wrote:
>> And, you mentioned Exynos4 will be dt-only from 3.10. Does that mean
>> we just drop support for universal and nuri non-dt board support? Or,
>> will there be a equivalent dt support added for these boards?
>
> I think Tomasz has some initial dts files for Universal_c210 ready, and
> Nuri could probably just be dropped. But we need Kyungmin's opinion on
> that.
>
> I'm not sure about other boards, they look pretty basic though.
> So it shouldn't be difficult to replace them with corresponding dts
> files. Currently there are:
>
> arch/arm/mach-exynos/mach-smdk4x12.c
> arch/arm/mach-exynos/mach-origen.c
> arch/arm/mach-exynos/mach-nuri.c
> arch/arm/mach-exynos/mach-universal_c210.c
> arch/arm/mach-exynos/mach-armlex4210.c
> arch/arm/mach-exynos/mach-smdkv310.c
>
> And there are dts files for:
>
> arch/arm/boot/dts/exynos4210-smdkv310.dts
> arch/arm/boot/dts/exynos4210-origen.dts
> arch/arm/boot/dts/exynos5250-smdk5250.dts
> arch/arm/boot/dts/exynos5250-snow.dts
> arch/arm/boot/dts/exynos4210-trats.dts
> arch/arm/boot/dts/exynos5440-ssdk5440.dts
> arch/arm/boot/dts/exynos4412-smdk4412.dts
>
> So except Universal_c210 and Nuri the only ones not supporting booting
> from DT are these two simple boards, which seem to be maintained by
> Samsung:
>
> arch/arm/mach-exynos/mach-smdk4x12.c
> arch/arm/mach-exynos/mach-armlex4210.c

There has not been much board support development for these boards for
couple of kernel releases now.

>
> It would be nice to make Exynos DT only in this kernel release.
> I guess there was enough time to get all boards converted to DT
> already.

Thanks for sharing your thoughts on this.

Regards,
Thomas.

>
> --
>
> Regards,
> Sylwester

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Alim Akhtar]

Hi All

On Tue, Mar 12, 2013 at 7:54 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> On 12 March 2013 19:18, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>> On 03/12/2013 12:46 PM, Thomas Abraham wrote:
>>> And, you mentioned Exynos4 will be dt-only from 3.10. Does that mean
>>> we just drop support for universal and nuri non-dt board support? Or,
>>> will there be a equivalent dt support added for these boards?
>>
>> I think Tomasz has some initial dts files for Universal_c210 ready, and
>> Nuri could probably just be dropped. But we need Kyungmin's opinion on
>> that.
>>
>> I'm not sure about other boards, they look pretty basic though.
>> So it shouldn't be difficult to replace them with corresponding dts
>> files. Currently there are:
>>
>> arch/arm/mach-exynos/mach-smdk4x12.c
>> arch/arm/mach-exynos/mach-origen.c
>> arch/arm/mach-exynos/mach-nuri.c
>> arch/arm/mach-exynos/mach-universal_c210.c
>> arch/arm/mach-exynos/mach-armlex4210.c
>> arch/arm/mach-exynos/mach-smdkv310.c
>>
>> And there are dts files for:
>>
>> arch/arm/boot/dts/exynos4210-smdkv310.dts
>> arch/arm/boot/dts/exynos4210-origen.dts
>> arch/arm/boot/dts/exynos5250-smdk5250.dts
>> arch/arm/boot/dts/exynos5250-snow.dts
>> arch/arm/boot/dts/exynos4210-trats.dts
>> arch/arm/boot/dts/exynos5440-ssdk5440.dts
>> arch/arm/boot/dts/exynos4412-smdk4412.dts
>>
>> So except Universal_c210 and Nuri the only ones not supporting booting
>> from DT are these two simple boards, which seem to be maintained by
>> Samsung:
>>
>> arch/arm/mach-exynos/mach-smdk4x12.c
>> arch/arm/mach-exynos/mach-armlex4210.c
>
> There has not been much board support development for these boards for
> couple of kernel releases now.
>

Right, Armlex4210 can be removed as there is no much development
happening. And I do not think any one else using it now.

I do not know about smdk4x12.c though,

>>
>> It would be nice to make Exynos DT only in this kernel release.
>> I guess there was enough time to get all boards converted to DT
>> already.
>
> Thanks for sharing your thoughts on this.
>
> Regards,
> Thomas.
>
>>
>> --
>>
>> Regards,
>> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards,
Alim

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Sachin Kamat]

>
> So except Universal_c210 and Nuri the only ones not supporting booting
> from DT are these two simple boards, which seem to be maintained by
> Samsung:
>
> arch/arm/mach-exynos/mach-smdk4x12.c

SMDK4412 is already supported through DT (exynos4412-smdk4412.dts) as
you already pointed out. I am not sure if anyone is using SMDK4212.
Even if it is required, one can easily add a dts file for that board
as DT support for 4x12 already exists.

arch/arm/boot/dts/exynos4x12.dtsi
arch/arm/boot/dts/exynos4x12-pinctrl.dtsi

-- 
With warm regards,
Sachin

Re: [PATCH 3/4] clk: samsung: always allocate the clk_table

[Kyungmin Park]

On Tue, Mar 12, 2013 at 10:48 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 03/12/2013 12:46 PM, Thomas Abraham wrote:
>> And, you mentioned Exynos4 will be dt-only from 3.10. Does that mean
>> we just drop support for universal and nuri non-dt board support? Or,
>> will there be a equivalent dt support added for these boards?
>
> I think Tomasz has some initial dts files for Universal_c210 ready, and
> Nuri could probably just be dropped. But we need Kyungmin's opinion on
> that.
Agree.
Tomasz will send unviersal DT files soon.

Thank you,
Kyungmin Park
>
> I'm not sure about other boards, they look pretty basic though.
> So it shouldn't be difficult to replace them with corresponding dts
> files. Currently there are:
>
> arch/arm/mach-exynos/mach-smdk4x12.c
> arch/arm/mach-exynos/mach-origen.c
> arch/arm/mach-exynos/mach-nuri.c
> arch/arm/mach-exynos/mach-universal_c210.c
> arch/arm/mach-exynos/mach-armlex4210.c
> arch/arm/mach-exynos/mach-smdkv310.c
>
> And there are dts files for:
>
> arch/arm/boot/dts/exynos4210-smdkv310.dts
> arch/arm/boot/dts/exynos4210-origen.dts
> arch/arm/boot/dts/exynos5250-smdk5250.dts
> arch/arm/boot/dts/exynos5250-snow.dts
> arch/arm/boot/dts/exynos4210-trats.dts
> arch/arm/boot/dts/exynos5440-ssdk5440.dts
> arch/arm/boot/dts/exynos4412-smdk4412.dts
>
> So except Universal_c210 and Nuri the only ones not supporting booting
> from DT are these two simple boards, which seem to be maintained by
> Samsung:
>
> arch/arm/mach-exynos/mach-smdk4x12.c
> arch/arm/mach-exynos/mach-armlex4210.c
>
> It would be nice to make Exynos DT only in this kernel release.
> I guess there was enough time to get all boards converted to DT
> already.
>
> --
>
> Regards,
> Sylwester
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] clk: samsung: add infrastructure to add separate aliases

[Heiko Stübner]

The current code adds aliases, if necessary, directly when adding the clock,
limiting the number of possible aliases to one.

Some platforms need more than one alias, like the hsmmc pclocks on s3c2416
which need a "hsmmc" and "mmc_busclk.0" alias for the s3c-sdhci driver.

Therefore add the possibility to separately add clock aliases for previously
created clocks.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
change since the discussion: removed the list->alias check

 drivers/clk/samsung/clk.c |   33 +++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk.h |   21 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 7c943f8..20ec566 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -94,6 +94,39 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned int id)
 		clk_table[id] = clk;
 }
 
+/* register a list of aliases */
+void __init samsung_clk_register_alias(struct samsung_clock_alias *list,
+					unsigned int nr_clk)
+{
+	struct clk *clk;
+	unsigned int idx, ret;
+
+	if (!clk_table) {
+		pr_err("%s: clock table missing\n", __func__);
+		return;
+	}
+
+	for (idx = 0; idx < nr_clk; idx++, list++) {
+		if (!list->id) {
+			pr_err("%s: clock id missing for index %d\n", __func__,
+				idx);
+			continue;
+		}
+
+		clk = clk_table[list->id];
+		if (!clk) {
+			pr_err("%s: failed to find clock %d\n", __func__,
+				list->id);
+			continue;
+		}
+
+		ret = clk_register_clkdev(clk, list->alias, list->dev_name);
+		if (ret)
+			pr_err("%s: failed to register lookup %s\n",
+					__func__, list->alias);
+	}
+}
+
 /* register a list of fixed clocks */
 void __init samsung_clk_register_fixed_rate(
 		struct samsung_fixed_rate_clock *list, unsigned int nr_clk)
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index 26a752b..6bacd6f 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -23,6 +23,25 @@
 #include <mach/map.h>
 
 /**
+ * struct samsung_clock_alias: information about mux clock
+ * @id: platform specific id of the clock.
+ * @dev_name: name of the device to which this clock belongs.
+ * @alias: optional clock alias name to be assigned to this clock.
+ */
+struct samsung_clock_alias {
+	unsigned int		id;
+	const char		*dev_name;
+	const char		*alias;
+};
+
+#define ALIAS(_id, dname, a)	\
+	{							\
+		.id		= _id,				\
+		.dev_name	= dname,			\
+		.alias		= a,				\
+	}
+
+/**
  * struct samsung_fixed_rate_clock: information about fixed-rate clock
  * @id: platform specific id of the clock.
  * @name: name of this fixed-rate clock.
@@ -251,6 +270,8 @@ extern void __init samsung_clk_of_register_fixed_ext(
 
 extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
 
+extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
+		unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_rate(
 		struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_factor(
-- 
1.7.2.3

Re: [PATCH 4/4] clk: samsung: add infrastructure to add separate aliases

[Thomas Abraham]

On 12 March 2013 05:15, Heiko Stübner <heiko@sntech.de> wrote:
> The current code adds aliases, if necessary, directly when adding the clock,
> limiting the number of possible aliases to one.
>
> Some platforms need more than one alias, like the hsmmc pclocks on s3c2416
> which need a "hsmmc" and "mmc_busclk.0" alias for the s3c-sdhci driver.
>
> Therefore add the possibility to separately add clock aliases for previously
> created clocks.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> change since the discussion: removed the list->alias check
>
>  drivers/clk/samsung/clk.c |   33 +++++++++++++++++++++++++++++++++
>  drivers/clk/samsung/clk.h |   21 +++++++++++++++++++++
>  2 files changed, 54 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 7c943f8..20ec566 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -94,6 +94,39 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned int id)
>                 clk_table[id] = clk;
>  }
>
> +/* register a list of aliases */
> +void __init samsung_clk_register_alias(struct samsung_clock_alias *list,
> +                                       unsigned int nr_clk)
> +{
> +       struct clk *clk;
> +       unsigned int idx, ret;
> +
> +       if (!clk_table) {
> +               pr_err("%s: clock table missing\n", __func__);
> +               return;
> +       }
> +
> +       for (idx = 0; idx < nr_clk; idx++, list++) {
> +               if (!list->id) {
> +                       pr_err("%s: clock id missing for index %d\n", __func__,
> +                               idx);
> +                       continue;
> +               }
> +
> +               clk = clk_table[list->id];
> +               if (!clk) {
> +                       pr_err("%s: failed to find clock %d\n", __func__,
> +                               list->id);
> +                       continue;
> +               }
> +
> +               ret = clk_register_clkdev(clk, list->alias, list->dev_name);
> +               if (ret)
> +                       pr_err("%s: failed to register lookup %s\n",
> +                                       __func__, list->alias);
> +       }
> +}
> +
>  /* register a list of fixed clocks */
>  void __init samsung_clk_register_fixed_rate(
>                 struct samsung_fixed_rate_clock *list, unsigned int nr_clk)
> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> index 26a752b..6bacd6f 100644
> --- a/drivers/clk/samsung/clk.h
> +++ b/drivers/clk/samsung/clk.h
> @@ -23,6 +23,25 @@
>  #include <mach/map.h>
>
>  /**
> + * struct samsung_clock_alias: information about mux clock
> + * @id: platform specific id of the clock.
> + * @dev_name: name of the device to which this clock belongs.
> + * @alias: optional clock alias name to be assigned to this clock.
> + */
> +struct samsung_clock_alias {
> +       unsigned int            id;
> +       const char              *dev_name;
> +       const char              *alias;
> +};
> +
> +#define ALIAS(_id, dname, a)   \
> +       {                                                       \
> +               .id             = _id,                          \
> +               .dev_name       = dname,                        \
> +               .alias          = a,                            \
> +       }
> +
> +/**
>   * struct samsung_fixed_rate_clock: information about fixed-rate clock
>   * @id: platform specific id of the clock.
>   * @name: name of this fixed-rate clock.
> @@ -251,6 +270,8 @@ extern void __init samsung_clk_of_register_fixed_ext(
>
>  extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
>
> +extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
> +               unsigned int nr_clk);
>  extern void __init samsung_clk_register_fixed_rate(
>                 struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk);
>  extern void __init samsung_clk_register_fixed_factor(
> --
> 1.7.2.3
>

This change looks fine but as discussed on 3/4 patch of this series,
allocating clk_table for non-dt platforms would not be very helpful.

Thanks,
Thomas.

Re: [PATCH 4/4] clk: samsung: add infrastructure to add separate aliases

[Heiko Stübner]

Am Dienstag, 12. März 2013, 10:57:30 schrieb Thomas Abraham:
> On 12 March 2013 05:15, Heiko Stübner <heiko@sntech.de> wrote:
> > The current code adds aliases, if necessary, directly when adding the
> > clock, limiting the number of possible aliases to one.
> > 
> > Some platforms need more than one alias, like the hsmmc pclocks on
> > s3c2416 which need a "hsmmc" and "mmc_busclk.0" alias for the s3c-sdhci
> > driver.
> > 
> > Therefore add the possibility to separately add clock aliases for
> > previously created clocks.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > change since the discussion: removed the list->alias check
> > 
> >  drivers/clk/samsung/clk.c |   33 +++++++++++++++++++++++++++++++++
> >  drivers/clk/samsung/clk.h |   21 +++++++++++++++++++++
> >  2 files changed, 54 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> > index 7c943f8..20ec566 100644
> > --- a/drivers/clk/samsung/clk.c
> > +++ b/drivers/clk/samsung/clk.c
> > @@ -94,6 +94,39 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned
> > int id)
> > 
> >                 clk_table[id] = clk;
> >  
> >  }
> > 
> > +/* register a list of aliases */
> > +void __init samsung_clk_register_alias(struct samsung_clock_alias *list,
> > +                                       unsigned int nr_clk)
> > +{
> > +       struct clk *clk;
> > +       unsigned int idx, ret;
> > +
> > +       if (!clk_table) {
> > +               pr_err("%s: clock table missing\n", __func__);
> > +               return;
> > +       }
> > +
> > +       for (idx = 0; idx < nr_clk; idx++, list++) {
> > +               if (!list->id) {
> > +                       pr_err("%s: clock id missing for index %d\n",
> > __func__, +                               idx);
> > +                       continue;
> > +               }
> > +
> > +               clk = clk_table[list->id];
> > +               if (!clk) {
> > +                       pr_err("%s: failed to find clock %d\n", __func__,
> > +                               list->id);
> > +                       continue;
> > +               }
> > +
> > +               ret = clk_register_clkdev(clk, list->alias,
> > list->dev_name); +               if (ret)
> > +                       pr_err("%s: failed to register lookup %s\n",
> > +                                       __func__, list->alias);
> > +       }
> > +}
> > +
> > 
> >  /* register a list of fixed clocks */
> >  void __init samsung_clk_register_fixed_rate(
> >  
> >                 struct samsung_fixed_rate_clock *list, unsigned int
> >                 nr_clk)
> > 
> > diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> > index 26a752b..6bacd6f 100644
> > --- a/drivers/clk/samsung/clk.h
> > +++ b/drivers/clk/samsung/clk.h
> > @@ -23,6 +23,25 @@
> > 
> >  #include <mach/map.h>
> >  
> >  /**
> > 
> > + * struct samsung_clock_alias: information about mux clock
> > + * @id: platform specific id of the clock.
> > + * @dev_name: name of the device to which this clock belongs.
> > + * @alias: optional clock alias name to be assigned to this clock.
> > + */
> > +struct samsung_clock_alias {
> > +       unsigned int            id;
> > +       const char              *dev_name;
> > +       const char              *alias;
> > +};
> > +
> > +#define ALIAS(_id, dname, a)   \
> > +       {                                                       \
> > +               .id             = _id,                          \
> > +               .dev_name       = dname,                        \
> > +               .alias          = a,                            \
> > +       }
> > +
> > +/**
> > 
> >   * struct samsung_fixed_rate_clock: information about fixed-rate clock
> >   * @id: platform specific id of the clock.
> >   * @name: name of this fixed-rate clock.
> > 
> > @@ -251,6 +270,8 @@ extern void __init samsung_clk_of_register_fixed_ext(
> > 
> >  extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
> > 
> > +extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
> > +               unsigned int nr_clk);
> > 
> >  extern void __init samsung_clk_register_fixed_rate(
> >  
> >                 struct samsung_fixed_rate_clock *clk_list, unsigned int
> >                 nr_clk);
> >  
> >  extern void __init samsung_clk_register_fixed_factor(
> > 
> > --
> > 1.7.2.3
> 
> This change looks fine but as discussed on 3/4 patch of this series,
> allocating clk_table for non-dt platforms would not be very helpful.

But how would we look up the previously registered clk otherwise?
Using clk_table for this lookup seemed like the best way to easily access 
these clocks.


Thanks
Heiko

Re: [PATCH 4/4] clk: samsung: add infrastructure to add separate aliases

[Thomas Abraham]

On 12 March 2013 15:34, Heiko Stübner <heiko@sntech.de> wrote:
> Am Dienstag, 12. März 2013, 10:57:30 schrieb Thomas Abraham:
>> On 12 March 2013 05:15, Heiko Stübner <heiko@sntech.de> wrote:
>> > The current code adds aliases, if necessary, directly when adding the
>> > clock, limiting the number of possible aliases to one.
>> >
>> > Some platforms need more than one alias, like the hsmmc pclocks on
>> > s3c2416 which need a "hsmmc" and "mmc_busclk.0" alias for the s3c-sdhci
>> > driver.
>> >
>> > Therefore add the possibility to separately add clock aliases for
>> > previously created clocks.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> > ---
>> > change since the discussion: removed the list->alias check
>> >
>> >  drivers/clk/samsung/clk.c |   33 +++++++++++++++++++++++++++++++++
>> >  drivers/clk/samsung/clk.h |   21 +++++++++++++++++++++
>> >  2 files changed, 54 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> > index 7c943f8..20ec566 100644
>> > --- a/drivers/clk/samsung/clk.c
>> > +++ b/drivers/clk/samsung/clk.c
>> > @@ -94,6 +94,39 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned
>> > int id)
>> >
>> >                 clk_table[id] = clk;
>> >
>> >  }
>> >
>> > +/* register a list of aliases */
>> > +void __init samsung_clk_register_alias(struct samsung_clock_alias *list,
>> > +                                       unsigned int nr_clk)
>> > +{
>> > +       struct clk *clk;
>> > +       unsigned int idx, ret;
>> > +
>> > +       if (!clk_table) {
>> > +               pr_err("%s: clock table missing\n", __func__);
>> > +               return;
>> > +       }
>> > +
>> > +       for (idx = 0; idx < nr_clk; idx++, list++) {
>> > +               if (!list->id) {
>> > +                       pr_err("%s: clock id missing for index %d\n",
>> > __func__, +                               idx);
>> > +                       continue;
>> > +               }
>> > +
>> > +               clk = clk_table[list->id];
>> > +               if (!clk) {
>> > +                       pr_err("%s: failed to find clock %d\n", __func__,
>> > +                               list->id);
>> > +                       continue;
>> > +               }
>> > +
>> > +               ret = clk_register_clkdev(clk, list->alias,
>> > list->dev_name); +               if (ret)
>> > +                       pr_err("%s: failed to register lookup %s\n",
>> > +                                       __func__, list->alias);
>> > +       }
>> > +}
>> > +
>> >
>> >  /* register a list of fixed clocks */
>> >  void __init samsung_clk_register_fixed_rate(
>> >
>> >                 struct samsung_fixed_rate_clock *list, unsigned int
>> >                 nr_clk)
>> >
>> > diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
>> > index 26a752b..6bacd6f 100644
>> > --- a/drivers/clk/samsung/clk.h
>> > +++ b/drivers/clk/samsung/clk.h
>> > @@ -23,6 +23,25 @@
>> >
>> >  #include <mach/map.h>
>> >
>> >  /**
>> >
>> > + * struct samsung_clock_alias: information about mux clock
>> > + * @id: platform specific id of the clock.
>> > + * @dev_name: name of the device to which this clock belongs.
>> > + * @alias: optional clock alias name to be assigned to this clock.
>> > + */
>> > +struct samsung_clock_alias {
>> > +       unsigned int            id;
>> > +       const char              *dev_name;
>> > +       const char              *alias;
>> > +};
>> > +
>> > +#define ALIAS(_id, dname, a)   \
>> > +       {                                                       \
>> > +               .id             = _id,                          \
>> > +               .dev_name       = dname,                        \
>> > +               .alias          = a,                            \
>> > +       }
>> > +
>> > +/**
>> >
>> >   * struct samsung_fixed_rate_clock: information about fixed-rate clock
>> >   * @id: platform specific id of the clock.
>> >   * @name: name of this fixed-rate clock.
>> >
>> > @@ -251,6 +270,8 @@ extern void __init samsung_clk_of_register_fixed_ext(
>> >
>> >  extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
>> >
>> > +extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
>> > +               unsigned int nr_clk);
>> >
>> >  extern void __init samsung_clk_register_fixed_rate(
>> >
>> >                 struct samsung_fixed_rate_clock *clk_list, unsigned int
>> >                 nr_clk);
>> >
>> >  extern void __init samsung_clk_register_fixed_factor(
>> >
>> > --
>> > 1.7.2.3
>>
>> This change looks fine but as discussed on 3/4 patch of this series,
>> allocating clk_table for non-dt platforms would not be very helpful.
>
> But how would we look up the previously registered clk otherwise?
> Using clk_table for this lookup seemed like the best way to easily access
> these clocks.

I did assume that the aliases for a clock are set when that clock is
registered. Are there any instances where the aliases have to be set
after a clock has been registered (using the samsung clock driver
helper functions)?

Thanks,
Thomas.

>
>
> Thanks
> Heiko

[PATCH v2 0/4] clk: samsung: pm fixes and multiple aliases

[Heiko Stübner]

Second version of the fixes to address comments gathered from the first
version, like not entering the dt code on non-dt platforms even if
dt-support is present in the kernel.

Patch 1 and 3 got "Reviewed-by" tags but I only included the one for
patch 1, because patch 3 changed due to the changes in patch 2.

Heiko Stuebner (4):
  clk: samsung: register clk_div_tables for divider clocks
  clk: samsung: fix pm init on non-dt platforms
  clk: samsung: always allocate the clk_table
  clk: samsung: add infrastructure to add separate aliases

 drivers/clk/samsung/clk.c |   72 +++++++++++++++++++++++++++++++++++---------
 drivers/clk/samsung/clk.h |   34 ++++++++++++++++++--
 2 files changed, 87 insertions(+), 19 deletions(-)

-- 
1.7.2.3

[PATCH 4/4] clk: samsung: add infrastructure to add separate aliases

[Heiko Stübner]

The current code adds aliases, if necessary, directly when adding the clock,
limiting the number of possible aliases to one.

Some platforms need more than one alias, like the hsmmc pclocks on s3c2416
which need a "hsmmc" and "mmc_busclk.0" alias for the s3c-sdhci driver.

Therefore add the possibility to separately add clock aliases for previously
created clocks.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/samsung/clk.c |   33 +++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk.h |   21 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 1ed5716..82f27f6 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -97,6 +97,39 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned int id)
 		clk_table[id] = clk;
 }
 
+/* register a list of aliases */
+void __init samsung_clk_register_alias(struct samsung_clock_alias *list,
+					unsigned int nr_clk)
+{
+	struct clk *clk;
+	unsigned int idx, ret;
+
+	if (!clk_table) {
+		pr_err("%s: clock table missing\n", __func__);
+		return;
+	}
+
+	for (idx = 0; idx < nr_clk; idx++, list++) {
+		if (!list->id) {
+			pr_err("%s: clock id missing for index %d\n", __func__,
+				idx);
+			continue;
+		}
+
+		clk = clk_table[list->id];
+		if (!clk) {
+			pr_err("%s: failed to find clock %d\n", __func__,
+				list->id);
+			continue;
+		}
+
+		ret = clk_register_clkdev(clk, list->alias, list->dev_name);
+		if (ret)
+			pr_err("%s: failed to register lookup %s\n",
+					__func__, list->alias);
+	}
+}
+
 /* register a list of fixed clocks */
 void __init samsung_clk_register_fixed_rate(
 		struct samsung_fixed_rate_clock *list, unsigned int nr_clk)
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index 26a752b..6bacd6f 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -23,6 +23,25 @@
 #include <mach/map.h>
 
 /**
+ * struct samsung_clock_alias: information about mux clock
+ * @id: platform specific id of the clock.
+ * @dev_name: name of the device to which this clock belongs.
+ * @alias: optional clock alias name to be assigned to this clock.
+ */
+struct samsung_clock_alias {
+	unsigned int		id;
+	const char		*dev_name;
+	const char		*alias;
+};
+
+#define ALIAS(_id, dname, a)	\
+	{							\
+		.id		= _id,				\
+		.dev_name	= dname,			\
+		.alias		= a,				\
+	}
+
+/**
  * struct samsung_fixed_rate_clock: information about fixed-rate clock
  * @id: platform specific id of the clock.
  * @name: name of this fixed-rate clock.
@@ -251,6 +270,8 @@ extern void __init samsung_clk_of_register_fixed_ext(
 
 extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
 
+extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
+		unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_rate(
 		struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_factor(
-- 
1.7.2.3