diff mbox

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

Message ID 201303120044.29499.heiko@sntech.de (mailing list archive)
State New, archived
Headers show

Commit Message

Heiko Stübner March 11, 2013, 11:44 p.m. UTC
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(-)

Comments

Thomas Abraham March 12, 2013, 9:54 a.m. UTC | #1
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
>
--
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
Heiko Stübner March 12, 2013, 10:50 a.m. UTC | #2
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

--
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
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
--
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
Thomas Abraham March 12, 2013, 11:26 a.m. UTC | #4
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
>
--
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
Thomas Abraham March 12, 2013, 11:46 a.m. UTC | #5
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
--
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
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
--
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
Thomas Abraham March 12, 2013, 2:24 p.m. UTC | #7
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
--
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
Alim Akhtar March 13, 2013, 3 a.m. UTC | #8
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
Sachin Kamat March 13, 2013, 3:35 a.m. UTC | #9
>
> 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
Kyungmin Park March 13, 2013, 5:13 a.m. UTC | #10
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
--
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
diff mbox

Patch

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);