Re: Clock framework implementation for TI H4 OMAP2420 development board

[Matt Thomas <matt%3am-software.com@localhost>]

On Sep 10, 2008, at 2:12 AM, Juha Keski-Saari wrote:

> Hello everyone
>
> We'd like to publish a clock framework implementation which we have
> been using in development for more than half a year, so you can
> consider it tested R&D grade code even though coverage hasn't been a
> full 100%.
>
> We began the implementation to satisfy a need for centralized
> peripheral clock state control on the TI H4 OMAP2420 board we were
> using. We found a reference implementation from Linux Maemo code for
> the same hardware platform, yet what exists today is barely
> recognisable as having similar roots. Most of the remaining
> similarities spur from the technical reference manual instructions of
> the HW and are thus unavoidable. Later on we extended the
> implementation to also support clock operating point transitions,  
> which
> we used to fulfill basic Frequency Scaling functions on the hardware.
> DPLL and SDRC reprogramming was not in our scope but could be easily
> implemented inside this framework. We believe this can be helpful to
> anyone involved with arm-based hardware, to accelerate development of
> features that require manipulation or information on system clocks for
> operation.
>
> We wish to publish this code under the copyright of the NetBSD
> foundation, so it can be useful in any way is seen fit. We welcome all
> nature of discussion regarding this component. Below you can find a
> link to the patch formatted on top of netbsd-current dated Sep 9th.
>
> http://koowaldah.org/people/ash/netbsd/clock_framework.patch
>
> Regards,
> Juha Keski-Saari

It's a good start.  But it's way too omap2 specific at this point.

First some general comments:

no space after ( or before )
char* name -> char *name
please start struct members with a common prefix.
struct clk -> clk_*
the flags for clk_flags should start with CLK_
magic values are bad.  (prcm_reg |= 0x03 << 3;)


The information in <sys/clk.h> is not MI.



const is your friend, use it.

the omap2 specific stuff in struct clk should be moved to omap2_clk.
and the first member of omap2_clk is struct clk.  That gives you a
nice MI/MD split.

The implicit ordered in clock_index[] is prone to breakage.  It
shouldn't exist but be dynamically created as clocks are made known
to the system.

instead of parent being a pointer to another clk, it should be
a const char * containing the name of the parent.

The hierarchy of the clocks should be hidden behind an API:

int clk_add(struct clk *);
int clk_remove(struct clk *);
struct clk *clk_parent(struct clk *);
struct clk *clk_child(struct clk *);
struct clk *clk_next(struct clk *);

How this hierarchy is maintained behind the API is up to the
implementation.

The clk members

+       .enable         = &omap2_enable_clock,
+       .get_status     = &omap2_get_status,
+       .force_onoff    = &omap2_force_onoff,
+       .disable        = &omap2_disable_clock,
+       .set_rate       = &omap2_set_rate,
+       .get_rate       = &omap2_get_rate,

but not

+       .recalc         = &omap2_sys_clk_recalc,

should move to a clk_ops structure for which clk has a
const struct clk_ops *clk_ops member.  The iclk_parent
isn't needed since it could just be an ancestor of the
clock and flagged as such.