[RFC 0/8]: omap4: clk: move clock data under drivers/clk

[RFC 0/8]: omap4: clk: move clock data under drivers/clk

[Tero Kristo]

Hi,

This is an RFC version of the clock data move under drivers/clk.
Tested under 3.8 and boots fine, but don't try this out unless
you are experimental sort (I quickly tried with 3.9-rc3 and it failed to
boot with that.)

The approach taken here has minimal impact on the clock data
and should make it easy to transfer clock data for omap2 / omap3 and
omap5 also. omap4 was only done as a proof of concept.

Any comments appreciated.

-Tero

[RFC 1/8] RFC: CLK: OMAP: Add basic infrastructure for OMAP clocks

[Mike Turquette]

Quoting Tero Kristo (2013-03-21 10:35:40)
> This patch adds basic infrastructure support for registering clocks
> under common clock framework. This patch is done in preparation for
> moving clock data from arch/arm/mach-omap2/ folder under /drivers/clk/omap.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Mike Turquette <mturquette@linaro.org>

Thanks for taking a crack at this Tero!  This is a great step towards
getting rid of clk-private.h.

Regarding the long-term roadmap of the omap clock code:

In general all of the changes to the omap clock code for using the
common struct clk have been very incremental.  We still have the old
legacy struct clk definition, the name of that struct was changed to
clk_hw_omap, but it is still a fairly large, monolithic structure.  Are
there plans to break the clock types up into smaller, more focused clock
types?  E.g. get rid of struct dpll_data and have a dedicated dpll clock
type.

The question above is not reason to block this patch since it is a
fairly large refactoring effort, but it is something to consider.

Some comments below.

<snip>

> +struct omap_clk {
> +       u16                             cpu;
> +       const char                      *dev_id;
> +       const char                      *con_id;
> +       struct omap_init_clk            *clk;
> +};
> +
> +#define CLK(dev, con, ck, cp)          \
> +       {                               \
> +               .cpu = cp,              \
> +               .dev_id = dev,          \
> +               .con_id = con,          \
> +               .clk = ck,              \
> +       }       
> +

IS omap_clk and CLK really necessary today?  I thought that the move to
separate clocks out by tables got rid of this macro/structure?

> +#define OMAP_CLK_FIXED_RATE(_name, _flags, _rate, _ignore)     \
> +       static struct omap_init_clk _name __initdata = {        \
> +               .name = #_name,                                 \
> +               .clk_flags = _flags,                            \
> +               .rate = _rate,                                  \
> +               .clk_register = omap_clk_register_fixed_rate,   \
> +       };
> +

These macros are unreadable.  They were originally born out of the nasty
clk-private.h stuff which included forward declarations of struct clk
and all sorts of ugliness.  I know it saves you LoC to use them now but
I really prefer to use designated initializers for the sake of
readability.  Do you plan to convert the OMAP clock code back to how it
was, pre-macros?

Regards,
Mike

[RFC 0/8]: omap4: clk: move clock data under drivers/clk

[Rajendra Nayak]

Tero,

On Thursday 21 March 2013 11:05 PM, Tero Kristo wrote:
> Hi,
> 
> This is an RFC version of the clock data move under drivers/clk.
> Tested under 3.8 and boots fine, but don't try this out unless
> you are experimental sort (I quickly tried with 3.9-rc3 and it failed to
> boot with that.)
> 
> The approach taken here has minimal impact on the clock data
> and should make it easy to transfer clock data for omap2 / omap3 and
> omap5 also. omap4 was only done as a proof of concept.
> 
> Any comments appreciated.

I strangely only see the cover letter delivered to me and no
patches. Do you have a branch with the patches which you can
share?

regards,
Rajendra
 
> 
> -Tero
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC 1/8] RFC: CLK: OMAP: Add basic infrastructure for OMAP clocks

[Tero Kristo]

On Thu, 2013-03-21 at 11:50 -0700, Mike Turquette wrote:
> Quoting Tero Kristo (2013-03-21 10:35:40)
> > This patch adds basic infrastructure support for registering clocks
> > under common clock framework. This patch is done in preparation for
> > moving clock data from arch/arm/mach-omap2/ folder under /drivers/clk/omap.
> > 
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > Cc: Mike Turquette <mturquette@linaro.org>
> 
> Thanks for taking a crack at this Tero!  This is a great step towards
> getting rid of clk-private.h.

Hi Mike,

Thanks for the quick comments.

> 
> Regarding the long-term roadmap of the omap clock code:
> 
> In general all of the changes to the omap clock code for using the
> common struct clk have been very incremental.  We still have the old
> legacy struct clk definition, the name of that struct was changed to
> clk_hw_omap, but it is still a fairly large, monolithic structure.  Are
> there plans to break the clock types up into smaller, more focused clock
> types?  E.g. get rid of struct dpll_data and have a dedicated dpll clock
> type.

I think this sounds like a fair approach to me, currently the struct is
huge and we register almost everything under it. This RFC set only tries
to tackle with the most immediate problem, removing the dependency to
clk-private.h and moving the clock data out from mach-omap2.

> 
> The question above is not reason to block this patch since it is a
> fairly large refactoring effort, but it is something to consider.
> 
> Some comments below.
> 
> <snip>
> 
> > +struct omap_clk {
> > +       u16                             cpu;
> > +       const char                      *dev_id;
> > +       const char                      *con_id;
> > +       struct omap_init_clk            *clk;
> > +};
> > +
> > +#define CLK(dev, con, ck, cp)          \
> > +       {                               \
> > +               .cpu = cp,              \
> > +               .dev_id = dev,          \
> > +               .con_id = con,          \
> > +               .clk = ck,              \
> > +       }       
> > +
> 
> IS omap_clk and CLK really necessary today?  I thought that the move to
> separate clocks out by tables got rid of this macro/structure?

Well, it looks like we have a few clocks which actually use same clock
nodes, like dpll_mpu_ck is declared twice with different dev / con ids
here. It is possible to get rid of this in most cases and incorporate
the data from omap_clk to the omap_init_clk. Maybe for the special cases
we can just add a static clk registration in the code.

> 
> > +#define OMAP_CLK_FIXED_RATE(_name, _flags, _rate, _ignore)     \
> > +       static struct omap_init_clk _name __initdata = {        \
> > +               .name = #_name,                                 \
> > +               .clk_flags = _flags,                            \
> > +               .rate = _rate,                                  \
> > +               .clk_register = omap_clk_register_fixed_rate,   \
> > +       };
> > +
> 
> These macros are unreadable.  They were originally born out of the nasty
> clk-private.h stuff which included forward declarations of struct clk
> and all sorts of ugliness.  I know it saves you LoC to use them now but
> I really prefer to use designated initializers for the sake of
> readability.  Do you plan to convert the OMAP clock code back to how it
> was, pre-macros?

Yea, I think we should do this eventually once we also split the
different clock types to their own init structs. Then we can get rid of
these macros at the same point. However, I decided not to do this at
this step, as this simple data conversion allows us to convert also
omap2 / omap3 clocks rather easily, for which we don't have auto
generation available.

-Tero

[RFC 0/8]: omap4: clk: move clock data under drivers/clk

[Tero Kristo]

On Fri, 2013-03-22 at 10:58 +0530, Rajendra Nayak wrote:
> Tero,
> 
> On Thursday 21 March 2013 11:05 PM, Tero Kristo wrote:
> > Hi,
> > 
> > This is an RFC version of the clock data move under drivers/clk.
> > Tested under 3.8 and boots fine, but don't try this out unless
> > you are experimental sort (I quickly tried with 3.9-rc3 and it failed to
> > boot with that.)
> > 
> > The approach taken here has minimal impact on the clock data
> > and should make it easy to transfer clock data for omap2 / omap3 and
> > omap5 also. omap4 was only done as a proof of concept.
> > 
> > Any comments appreciated.
> 
> I strangely only see the cover letter delivered to me and no
> patches. Do you have a branch with the patches which you can
> share?

Keerthy had a similar question. It looks like TI people have had
problems with mailing list subscriptions as of late, and I am actually
also getting moderator bounces for my emails from linux-arm-kernel list
atm.

Anyway, I just pushed a branch here:

git://gitorious.org/~kristo/omap-pm/omap-pm-work.git
branch: linux-3.8-omap4-clk-move

-Tero

[RFC 1/8] RFC: CLK: OMAP: Add basic infrastructure for OMAP clocks

[Mike Turquette]

Quoting Tero Kristo (2013-03-22 01:39:08)
> On Thu, 2013-03-21 at 11:50 -0700, Mike Turquette wrote:
> > Quoting Tero Kristo (2013-03-21 10:35:40)
> > > This patch adds basic infrastructure support for registering clocks
> > > under common clock framework. This patch is done in preparation for
> > > moving clock data from arch/arm/mach-omap2/ folder under /drivers/clk/omap.
> > > 
> > > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > > Cc: Mike Turquette <mturquette@linaro.org>
> > 
> > Thanks for taking a crack at this Tero!  This is a great step towards
> > getting rid of clk-private.h.
> 
> Hi Mike,
> 
> Thanks for the quick comments.
> 
> > 
> > Regarding the long-term roadmap of the omap clock code:
> > 
> > In general all of the changes to the omap clock code for using the
> > common struct clk have been very incremental.  We still have the old
> > legacy struct clk definition, the name of that struct was changed to
> > clk_hw_omap, but it is still a fairly large, monolithic structure.  Are
> > there plans to break the clock types up into smaller, more focused clock
> > types?  E.g. get rid of struct dpll_data and have a dedicated dpll clock
> > type.
> 
> I think this sounds like a fair approach to me, currently the struct is
> huge and we register almost everything under it. This RFC set only tries
> to tackle with the most immediate problem, removing the dependency to
> clk-private.h and moving the clock data out from mach-omap2.
> 
> > 
> > The question above is not reason to block this patch since it is a
> > fairly large refactoring effort, but it is something to consider.
> > 
> > Some comments below.
> > 
> > <snip>
> > 
> > > +struct omap_clk {
> > > +       u16                             cpu;
> > > +       const char                      *dev_id;
> > > +       const char                      *con_id;
> > > +       struct omap_init_clk            *clk;
> > > +};
> > > +
> > > +#define CLK(dev, con, ck, cp)          \
> > > +       {                               \
> > > +               .cpu = cp,              \
> > > +               .dev_id = dev,          \
> > > +               .con_id = con,          \
> > > +               .clk = ck,              \
> > > +       }       
> > > +
> > 
> > IS omap_clk and CLK really necessary today?  I thought that the move to
> > separate clocks out by tables got rid of this macro/structure?
> 
> Well, it looks like we have a few clocks which actually use same clock
> nodes, like dpll_mpu_ck is declared twice with different dev / con ids
> here. It is possible to get rid of this in most cases and incorporate
> the data from omap_clk to the omap_init_clk. Maybe for the special cases
> we can just add a static clk registration in the code.
> 
> > 
> > > +#define OMAP_CLK_FIXED_RATE(_name, _flags, _rate, _ignore)     \
> > > +       static struct omap_init_clk _name __initdata = {        \
> > > +               .name = #_name,                                 \
> > > +               .clk_flags = _flags,                            \
> > > +               .rate = _rate,                                  \
> > > +               .clk_register = omap_clk_register_fixed_rate,   \
> > > +       };
> > > +
> > 
> > These macros are unreadable.  They were originally born out of the nasty
> > clk-private.h stuff which included forward declarations of struct clk
> > and all sorts of ugliness.  I know it saves you LoC to use them now but
> > I really prefer to use designated initializers for the sake of
> > readability.  Do you plan to convert the OMAP clock code back to how it
> > was, pre-macros?
> 
> Yea, I think we should do this eventually once we also split the
> different clock types to their own init structs. Then we can get rid of
> these macros at the same point. However, I decided not to do this at
> this step, as this simple data conversion allows us to convert also
> omap2 / omap3 clocks rather easily, for which we don't have auto
> generation available.
> 

Cool.  As long as there is a plan to clean this stuff up in the future
then I'm happy with this incremental approach.

Hopefully I'll have time to test this out on my omap hardware next week.

Regards,
Mike

> -Tero

[RFC 1/8] RFC: CLK: OMAP: Add basic infrastructure for OMAP clocks

[Tero Kristo]

On Fri, 2013-03-22 at 09:47 -0700, Mike Turquette wrote:
> Quoting Tero Kristo (2013-03-22 01:39:08)
> > On Thu, 2013-03-21 at 11:50 -0700, Mike Turquette wrote:
> > > Quoting Tero Kristo (2013-03-21 10:35:40)
> > > > This patch adds basic infrastructure support for registering clocks
> > > > under common clock framework. This patch is done in preparation for
> > > > moving clock data from arch/arm/mach-omap2/ folder under /drivers/clk/omap.
> > > > 
> > > > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > > > Cc: Mike Turquette <mturquette@linaro.org>
> > > 
> > > Thanks for taking a crack at this Tero!  This is a great step towards
> > > getting rid of clk-private.h.
> > 
> > Hi Mike,
> > 
> > Thanks for the quick comments.
> > 
> > > 
> > > Regarding the long-term roadmap of the omap clock code:
> > > 
> > > In general all of the changes to the omap clock code for using the
> > > common struct clk have been very incremental.  We still have the old
> > > legacy struct clk definition, the name of that struct was changed to
> > > clk_hw_omap, but it is still a fairly large, monolithic structure.  Are
> > > there plans to break the clock types up into smaller, more focused clock
> > > types?  E.g. get rid of struct dpll_data and have a dedicated dpll clock
> > > type.
> > 
> > I think this sounds like a fair approach to me, currently the struct is
> > huge and we register almost everything under it. This RFC set only tries
> > to tackle with the most immediate problem, removing the dependency to
> > clk-private.h and moving the clock data out from mach-omap2.
> > 
> > > 
> > > The question above is not reason to block this patch since it is a
> > > fairly large refactoring effort, but it is something to consider.
> > > 
> > > Some comments below.
> > > 
> > > <snip>
> > > 
> > > > +struct omap_clk {
> > > > +       u16                             cpu;
> > > > +       const char                      *dev_id;
> > > > +       const char                      *con_id;
> > > > +       struct omap_init_clk            *clk;
> > > > +};
> > > > +
> > > > +#define CLK(dev, con, ck, cp)          \
> > > > +       {                               \
> > > > +               .cpu = cp,              \
> > > > +               .dev_id = dev,          \
> > > > +               .con_id = con,          \
> > > > +               .clk = ck,              \
> > > > +       }       
> > > > +
> > > 
> > > IS omap_clk and CLK really necessary today?  I thought that the move to
> > > separate clocks out by tables got rid of this macro/structure?
> > 
> > Well, it looks like we have a few clocks which actually use same clock
> > nodes, like dpll_mpu_ck is declared twice with different dev / con ids
> > here. It is possible to get rid of this in most cases and incorporate
> > the data from omap_clk to the omap_init_clk. Maybe for the special cases
> > we can just add a static clk registration in the code.
> > 
> > > 
> > > > +#define OMAP_CLK_FIXED_RATE(_name, _flags, _rate, _ignore)     \
> > > > +       static struct omap_init_clk _name __initdata = {        \
> > > > +               .name = #_name,                                 \
> > > > +               .clk_flags = _flags,                            \
> > > > +               .rate = _rate,                                  \
> > > > +               .clk_register = omap_clk_register_fixed_rate,   \
> > > > +       };
> > > > +
> > > 
> > > These macros are unreadable.  They were originally born out of the nasty
> > > clk-private.h stuff which included forward declarations of struct clk
> > > and all sorts of ugliness.  I know it saves you LoC to use them now but
> > > I really prefer to use designated initializers for the sake of
> > > readability.  Do you plan to convert the OMAP clock code back to how it
> > > was, pre-macros?
> > 
> > Yea, I think we should do this eventually once we also split the
> > different clock types to their own init structs. Then we can get rid of
> > these macros at the same point. However, I decided not to do this at
> > this step, as this simple data conversion allows us to convert also
> > omap2 / omap3 clocks rather easily, for which we don't have auto
> > generation available.
> > 
> 
> Cool.  As long as there is a plan to clean this stuff up in the future
> then I'm happy with this incremental approach.
> 
> Hopefully I'll have time to test this out on my omap hardware next week.

Just a quick note, you need the clock init order patch from Rajendra
with this set, a working patch set is available in my branch here:

git://gitorious.org/~kristo/omap-pm/omap-pm-work.git
branch: linux-3.8-omap4-clk-move

... thats on top of 3.8.

-Tero

[RFC 1/8] RFC: CLK: OMAP: Add basic infrastructure for OMAP clocks

[Mike Turquette]

Quoting Tero Kristo (2013-03-22 11:39:45)
> On Fri, 2013-03-22 at 09:47 -0700, Mike Turquette wrote:
> > Quoting Tero Kristo (2013-03-22 01:39:08)
> > > On Thu, 2013-03-21 at 11:50 -0700, Mike Turquette wrote:
> > > > Quoting Tero Kristo (2013-03-21 10:35:40)
> > > > > This patch adds basic infrastructure support for registering clocks
> > > > > under common clock framework. This patch is done in preparation for
> > > > > moving clock data from arch/arm/mach-omap2/ folder under /drivers/clk/omap.
> > > > > 
> > > > > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > > > > Cc: Mike Turquette <mturquette@linaro.org>
> > > > 
> > > > Thanks for taking a crack at this Tero!  This is a great step towards
> > > > getting rid of clk-private.h.
> > > 
> > > Hi Mike,
> > > 
> > > Thanks for the quick comments.
> > > 
> > > > 
> > > > Regarding the long-term roadmap of the omap clock code:
> > > > 
> > > > In general all of the changes to the omap clock code for using the
> > > > common struct clk have been very incremental.  We still have the old
> > > > legacy struct clk definition, the name of that struct was changed to
> > > > clk_hw_omap, but it is still a fairly large, monolithic structure.  Are
> > > > there plans to break the clock types up into smaller, more focused clock
> > > > types?  E.g. get rid of struct dpll_data and have a dedicated dpll clock
> > > > type.
> > > 
> > > I think this sounds like a fair approach to me, currently the struct is
> > > huge and we register almost everything under it. This RFC set only tries
> > > to tackle with the most immediate problem, removing the dependency to
> > > clk-private.h and moving the clock data out from mach-omap2.
> > > 
> > > > 
> > > > The question above is not reason to block this patch since it is a
> > > > fairly large refactoring effort, but it is something to consider.
> > > > 
> > > > Some comments below.
> > > > 
> > > > <snip>
> > > > 
> > > > > +struct omap_clk {
> > > > > +       u16                             cpu;
> > > > > +       const char                      *dev_id;
> > > > > +       const char                      *con_id;
> > > > > +       struct omap_init_clk            *clk;
> > > > > +};
> > > > > +
> > > > > +#define CLK(dev, con, ck, cp)          \
> > > > > +       {                               \
> > > > > +               .cpu = cp,              \
> > > > > +               .dev_id = dev,          \
> > > > > +               .con_id = con,          \
> > > > > +               .clk = ck,              \
> > > > > +       }       
> > > > > +
> > > > 
> > > > IS omap_clk and CLK really necessary today?  I thought that the move to
> > > > separate clocks out by tables got rid of this macro/structure?
> > > 
> > > Well, it looks like we have a few clocks which actually use same clock
> > > nodes, like dpll_mpu_ck is declared twice with different dev / con ids
> > > here. It is possible to get rid of this in most cases and incorporate
> > > the data from omap_clk to the omap_init_clk. Maybe for the special cases
> > > we can just add a static clk registration in the code.
> > > 
> > > > 
> > > > > +#define OMAP_CLK_FIXED_RATE(_name, _flags, _rate, _ignore)     \
> > > > > +       static struct omap_init_clk _name __initdata = {        \
> > > > > +               .name = #_name,                                 \
> > > > > +               .clk_flags = _flags,                            \
> > > > > +               .rate = _rate,                                  \
> > > > > +               .clk_register = omap_clk_register_fixed_rate,   \
> > > > > +       };
> > > > > +
> > > > 
> > > > These macros are unreadable.  They were originally born out of the nasty
> > > > clk-private.h stuff which included forward declarations of struct clk
> > > > and all sorts of ugliness.  I know it saves you LoC to use them now but
> > > > I really prefer to use designated initializers for the sake of
> > > > readability.  Do you plan to convert the OMAP clock code back to how it
> > > > was, pre-macros?
> > > 
> > > Yea, I think we should do this eventually once we also split the
> > > different clock types to their own init structs. Then we can get rid of
> > > these macros at the same point. However, I decided not to do this at
> > > this step, as this simple data conversion allows us to convert also
> > > omap2 / omap3 clocks rather easily, for which we don't have auto
> > > generation available.
> > > 
> > 
> > Cool.  As long as there is a plan to clean this stuff up in the future
> > then I'm happy with this incremental approach.
> > 
> > Hopefully I'll have time to test this out on my omap hardware next week.
> 
> Just a quick note, you need the clock init order patch from Rajendra
> with this set, a working patch set is available in my branch here:
> 

Yes, I know.  I plan to take that patch and this series and remove
clk-private.h and see what happens.

Regards,
Mike

> git://gitorious.org/~kristo/omap-pm/omap-pm-work.git
> branch: linux-3.8-omap4-clk-move
> 
> ... thats on top of 3.8.
> 
> -Tero