Mostly looks good, a few comments below.
On Fri, Jun 20, 2008 at 10:58:35AM -0600, John Rigby wrote:
> Implements the api defined in include/clk.h
>
> Current only getting frequencies is supported
> not setting.
Need a more detailed commit message. This doesn't tell me much.
>
> Signed-off-by: John Rigby <[EMAIL PROTECTED]>
> ---
> arch/powerpc/platforms/512x/Makefile | 1 +
> arch/powerpc/platforms/512x/clock.c | 729
> ++++++++++++++++++++++++++++++++++
> 2 files changed, 730 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/platforms/512x/clock.c
>
> diff --git a/arch/powerpc/platforms/512x/Makefile
> b/arch/powerpc/platforms/512x/Makefile
> index 232c89f..ef6c925 100644
> --- a/arch/powerpc/platforms/512x/Makefile
> +++ b/arch/powerpc/platforms/512x/Makefile
> @@ -1,4 +1,5 @@
> #
> # Makefile for the Freescale PowerPC 512x linux kernel.
> #
> +obj-y := clock.o
should be +=
> obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o
> diff --git a/arch/powerpc/platforms/512x/clock.c
> b/arch/powerpc/platforms/512x/clock.c
> new file mode 100644
> index 0000000..39db722
> --- /dev/null
> +++ b/arch/powerpc/platforms/512x/clock.c
> @@ -0,0 +1,729 @@
> +/*
> + * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: John Rigby <[EMAIL PROTECTED]>
> + *
> + * Implements the clk api defined in include/linux/clk.h
> + *
> + * Original based on linux/arch/arm/mach-integrator/clock.c
> + *
> + * Copyright (C) 2004 ARM Limited.
> + * Written by Deep Blue Solutions Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/string.h>
> +#include <linux/clk.h>
> +#include <linux/mutex.h>
> +#include <linux/io.h>
> +
> +#include <linux/of_platform.h>
> +#include <asm/mpc512x.h>
> +
> +static int clocks_initialized;
> +
> +struct module;
This is already defined in linux/module.h?
> +
> +#undef CLK_DEBUG
I think this line should be at the top of the file to be easier to find
when toggling it.
> +#ifdef CLK_DEBUG
> +void dump_clocks(void)
> +{
> + struct clk *p;
> +
> + mutex_lock(&clocks_mutex);
> + printk(KERN_INFO "CLOCKS:\n");
> + list_for_each_entry(p, &clocks, node) {
> + printk(KERN_INFO " %s %ld", p->name, p->rate);
> + if (p->parent)
> + printk(KERN_INFO " %s %ld", p->parent->name,
> + p->parent->rate);
> + if (p->flags & CLK_HAS_CTRL)
> + printk(KERN_INFO " reg/bit %d/%d", p->reg, p->bit);
> + printk("\n");
> + }
> + mutex_unlock(&clocks_mutex);
> +}
> +#define DEBUG_CLK_DUMP() dump_clocks()
> +#else
> +#define DEBUG_CLK_DUMP()
> +#endif
> +
> +static long ips_to_ref(unsigned long rate)
> +{
> + int ips_div = (clockctl->scfr1 >> 23) & 0x7;
> +
> + rate *= ips_div; /* csb_clk = ips_clk * ips_div */
> + rate *= 2; /* sys_clk = csb_clk * 2 */
> + return sys_to_ref(rate);
> +}
> +
> +static unsigned long devtree_getfreq(char *nodetype, char *clockname)
Why is nodetype even passed in here? It isn't used, and besides, you
shouldn't test against device_type anyway. Test against compatible
instead.
> +{
> + struct device_node *node;
> + const unsigned int *fp;
> + unsigned int val = 0;
> +
> + node = of_find_node_by_type(NULL, "soc");
Once again; don't look for device_type == "soc". Use compatible.
> + if (node) {
> + fp = of_get_property(node, clockname, NULL);
> + if (fp)
> + val = of_read_ulong(fp, 1);
> + }
> + return val;
> +}
> +
> +static void ref_clk_calc(struct clk *clk)
> +{
> + unsigned long rate;
> +
> + rate = devtree_getfreq("soc", "ref-frequency");
> + if (rate == 0) {
> + /*
> + * no reference clock in device tree
> + * get ips clock freq and go backwards from there
> + */
> + rate = devtree_getfreq("soc", "bus-frequency");
> + if (rate == 0) {
> + printk(KERN_WARNING
> + "No bus-frequency in dev tree using 66MHz\n");
> + clk->rate = 66000000;
Is it even worth trying to use a default here? I think it should fail
loudly instead to reduce the risk of people shipping boards with badly
formed device trees. I don't think there is any backwards compatibility
need for doing this.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
