Port-arm archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Clock framework implementation for TI H4 OMAP2420 development board

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

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.


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

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.

Home | Main Index | Thread Index | Old Index