Re: [PATCH 2/6] MPC5121 clock driver
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 000..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 = 6600;
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
[PATCH 2/6] MPC5121 clock driver
Implements the api defined in include/clk.h
Current only getting frequencies is supported
not setting.
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
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 000..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;
+
+#define CLK_HAS_RATE 0x1 /* has rate in MHz */
+#define CLK_HAS_CTRL 0x2 /* has control reg and bit */
+
+struct clk {
+ struct list_head node;
+ char name[32];
+ int flags;
+ struct device *dev;
+ unsigned long rate;
+ struct module *owner;
+ void (*calc) (struct clk *);
+ struct clk *parent;
+ int reg, bit; /* CLK_HAS_CTRL */
+ int div_shift; /* only used by generic_div_clk_calc */
+};
+
+static LIST_HEAD(clocks);
+static DEFINE_MUTEX(clocks_mutex);
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+ struct clk *p, *clk = ERR_PTR(-ENOENT);
+ int dev_match = 0;
+ int id_match = 0;
+
+ if (dev == NULL id == NULL)
+ return NULL;
+
+ mutex_lock(clocks_mutex);
+ list_for_each_entry(p, clocks, node) {
+ if (dev dev == p-dev)
+ dev_match++;
+ if (strcmp(id, p-name) == 0)
+ id_match++;
+ if ((dev_match || id_match) try_module_get(p-owner)) {
+ clk = p;
+ break;
+ }
+ }
+ mutex_unlock(clocks_mutex);
+
+ return clk;
+}
+EXPORT_SYMBOL(clk_get);
+
+#undef CLK_DEBUG
+#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);
+}
+#defineDEBUG_CLK_DUMP() dump_clocks()
+#else
+#defineDEBUG_CLK_DUMP()
+#endif
+
+
+void clk_put(struct clk *clk)
+{
+ module_put(clk-owner);
+}
+EXPORT_SYMBOL(clk_put);
+
+#define NRPSC 12
+
+struct mpc512x_clockctl {
+ u32 spmr; /* System PLL Mode Reg */
+ u32 sccr[2];/* System Clk Ctrl Reg 1 2 */
+ u32 scfr1; /* System Clk Freq Reg 1 */
+ u32 scfr2; /* System Clk Freq Reg 2 */
+ u32 reserved;
+ u32 bcr;/* Bread Crumb Reg */
+ u32 pccr[NRPSC];/* PSC Clk Ctrl Reg 0-11 */
+ u32 spccr; /* SPDIF Clk Ctrl Reg */
+ u32 cccr; /* CFM Clk Ctrl Reg */
+ u32 dccr; /* DIU Clk Cnfg Reg */
+};
+
+struct mpc512x_clockctl __iomem *clockctl;
+
+int clk_enable(struct clk *clk)
+{
+ unsigned int mask;
+
+ if (clk-flags CLK_HAS_CTRL) {
+ mask = in_be32(clockctl-sccr[clk-reg]);
+ mask |= 1 clk-bit;
+ out_be32(clockctl-sccr[clk-reg], mask);
+ }
+ return 0;
+}
+EXPORT_SYMBOL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+ unsigned int mask;
+
+ if (clk-flags CLK_HAS_CTRL) {
+ mask =
Re: [PATCH 2/6] MPC5121 clock driver
Hi John,
On Fri, 20 Jun 2008 10:58:35 -0600 John Rigby [EMAIL PROTECTED] wrote:
+struct module;
Since you include linux/module.h, you don't need this.
+static unsigned long devtree_getfreq(char *nodetype, char *clockname)
+{
+ struct device_node *node;
+ const unsigned int *fp;
+ unsigned int val = 0;
+
+ node = of_find_node_by_type(NULL, soc);
+ if (node) {
+ fp = of_get_property(node, clockname, NULL);
+ if (fp)
+ val = of_read_ulong(fp, 1);
+ }
+ return val;
You leak a reference to the node here. Also every call has nodetype set to
soc and you don't use nodetype anyway.
--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/
pgpW3etpiVRpY.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
