Hi, Here is an updated version of the clocklib patchset. Changes since last repost: * fix an clks_register error path, as noted by Pavel Machek -- With best wishes Dmitry --
Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
arch/arm/common/sa1111.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index eb06d0b..282a4d9 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
if (!sachip)
return -ENOMEM;
- sachip->clk = clk_get(me, "GPIO27_CLK");
+ sachip->clk = clk_get(me, "3_6MHz_CLK");
if (!sachip->clk) {
ret = PTR_ERR(sachip->clk);
goto err_free;
--
1.5.4.4
--
With best wishes
Dmitry
--On Thu, 3 Apr 2008 17:24:11 +0400 Again, there's just not enough information for us (well: me) to be able to evaluate this patch. For example, if the current name is "incorrect" then why shouldn't we fix it in 2.6.25? 2.6.24? etc. --
The name GPIO27_CLK came from sa1100 arm sub-arch. There the 3.6 MHz
clock was provided via GPIO 27. The PXA clocks code have copied the
name for the clock (as it's used by sa1111 companion chip that can be
used with both sa1100 and pxa). However on pxa the 3.6MHz clock is
provided by different PIN. So the name GPIO27_CLK is misleading and
incorrect for PXA.
In my patchset I've changed the name of the clock to correct
GPIO11_CLK and have made both sa1100 and pxa provide common clock name
("3_6MHz_CLK").
This isn't a matter of userspace interfaces or communication. It's a
matter of logic and self-correctness.
If you wish I can put this information in the patch header.
--
With best wishes
Dmitry
--So... what is the correct name. Bear in mind what I said in the previous reply this evening - which says that it should be the name used by the SA1111. Look in the data sheet - the pin itself to which the 3.6MHz clock is supplised will have a name. That's the name which should be used. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
Hi, I use the same pin/clock for the tc6393xb driver. And I'm pretty sure the datasheets won't agree on the name of the pin. Which name should I use? -- With best wishes Dmitry --
You missed the fundamental issue about the clock API - the _name_ is not the clock name defined by the host. It's the _device_ clock name. So, you shouldn't be using the SA1111 clock name for the tc6393xb driver. You should be using its own name. The platform specific bit of the clock API is then supposed to return you the struct clk corresponding with that input, by using the platform knowledge that it's connected to GPIO27 or whatever. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
Hi, BTW: Who should know that there should be a clock for a device? E.g. on pxa, 3.6MHz clock is registered in clock.c, various CKEN-based clocks are registered from pxa*.c. Which file should contain host_name -> device_name allocation board config? And BTW2: The sa1111 spec names the clock simply as "CLK". Should we use this name? -- With best wishes Dmitry --
Hi, I think I understood your idea. I'll repost updated patchset later. -- With best wishes Dmitry --
I don't see any reason for this change. Except maybe someone wanted a nicer name to be exposed to userland. I don't see the point of exposing what's supposed to be a kernel _internal_ API to userland and then having an issue with those names being exported there. To put it another way: I don't ever want to have to think about userland issues when dealing with device clocking interfaces. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
drivers/net/irda/pxaficp_ir.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
index 8c09344..36d2ec0 100644
--- a/drivers/net/irda/pxaficp_ir.c
+++ b/drivers/net/irda/pxaficp_ir.c
@@ -814,7 +814,7 @@ static int pxa_irda_probe(struct platform_device *pdev)
si->dev = &pdev->dev;
si->pdata = pdev->dev.platform_data;
- si->sir_clk = clk_get(&pdev->dev, "UARTCLK");
+ si->sir_clk = clk_get(&pdev->dev, "SIRCLK");
si->fir_clk = clk_get(&pdev->dev, "FICPCLK");
if (IS_ERR(si->sir_clk) || IS_ERR(si->fir_clk)) {
err = PTR_ERR(IS_ERR(si->sir_clk) ? si->sir_clk : si->fir_clk);
--
1.5.4.4
--
With best wishes
Dmitry
--On Thu, 3 Apr 2008 17:24:02 +0400 When fixing a bug, please describe what the bug is, and how the patch fixes it. A patch needs to be very very obvious to be able to get away with no changelog, and this one is not very very obvious. --
It's not a bug fix - it's a stupidity of the clock lib that the API doesn't really require. I've not made up my mind what to do with this stuff, but if it requires drivers to work around its short comings, then I'm not in favour of it. (We have the above code working as is.) -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
Hi, Yes, it works currently. But there are a few problems: we declare STUART's UARTCLK with dev=NULL (all other UARTCLKs are declared with proper devices). Therefore, I consider it as a hack and would like to remove it. As the clocklib allows to register multiple children clocks with different names, I'd propose to separate UARTCLK and SIRCLK functions of STUART clock. The UARTCLK will be used (as usual) by the pxa uart driver. And IrDA will use SIRCLK. -- With best wishes Dmitry --
I don't consider it a hack at all - it's a work around for the fact that the PXA FIR driver shares the UART, but the FIR driver doesn't bind to the UART itself. The _real_ issue is with IrDA itself, and is larger than just the clock library. Any serial port which supports IrDA, even on x86, has to be shared between the serial driver and the IrDA driver - there's no way for them to quietly co-exist and "just work" as requested. So, let's not work around the short comings of Serial/IrDA interactions by adding additional complexity to random other layers which _shouldn't_ even be seeing the issue. In addition, the point of the clock framework is that you ask for the device plus clock NAME on _that_ device. Inventing random other names for the same physical clock on the same physical device is just nonsense - even more so than the existing workaround. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
Hi, Would you then accept the patch that still contains UARTCLK bound to See my proposition above. I highly dislike the UARTCLK w/o device declared. Once it has already lead me to (small) problems due to messed other UARTCLKs declarations on pxa25x. -- With best wishes Dmitry --
As I've suggested, it isn't. On x86, for instance, you have to use setserial to deconfigure the serial port from the serial driver (by setserial /dev/ttyS1 uart none) and then load the IrDA driver. Unfortunately, there's currently no way to properly hand off a real serial device to the IrDA layer when the UART is connected to an IrDA interface. TBH, I'm not sure what the status of the kernel-side IrDA drivers are - maybe this is an issue which Samuel Ortiz could tackle if he has time. That would solve this issue on both 8250-based ports as well as SA11x0 and PXA platforms. Basically what I'm thinking is a serial_core function which could be called to say "I'm an IrDA driver, and I think I should be using the serial port located <here>, please give me control of it" and it'd pass over the various parameters including the struct device for it. If we get the underlying issue fixed, that issue magically goes away. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
I think "in need of much love" would be a polite way to put it and some of them are probably going to break soon as they pretend to be tty It's called a line discipline, we've had them for many years. We may need a way for ldiscs and drivers to co-operate a bit more but these days we support proper buffering and arbitary baud rates (except on a few platforms whose maintainers are not paying attention ;)). So I don't actually see why the FIR layer can't become a good citizen. Alan --
I feel that's a "it would be nice if" solution - and something worth aiming for, but the amount of work required to get there is not going to be insignificant. Maybe Dmitry can bite his nails on the NULL device clklib issue for a while as this transition is happening ? (Though we need to find someone to make it happen... we need volunteers!) -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
The work required to fix up the existing FIR hacks is not insignifcant either. Also right now the tty layer is getting a major rework so now is actually the time to sort out anything extra that is needed. If you want 4MBit please just use an ldisc and do struct ktermios tmp; mutex_lock(&tty->termios_mutex); tmp = *tty->termios; tty_encode_baud_rate(tty, 4000000, 4000000); tty->driver->set_termios(tty, &tmp); mutex_unlock(&tty->termios_mutex); Alan --
There's more to FIR than just a baud rate change. On PXA for instance, SIR is implemented using the standard UART device in "SIR" mode, but FIR is a completely separate hardware block with its own IRQs and clocks - you need to switch the pin muxing from the UART to the FIR device. So it's not just a matter of setting the baud rate to 4Mbps. However, you can't just say "have two separate drivers and only use one or the other" - all IrDA link negotiation is done at SIR at 9600 baud and only when negotiation is complete will the selected rate become effective - be that SIR or FIR based. So yes, using a ldisc for SIR (with a hook into the driver to tell the driver to setup the port for IR) sounds ideal, but we're still going to need to deal with the FIR device and switch IrDA between that and the SIR UART ldisc. Note - there are other reasons for finally sorting this out as well - there are systems with IR which want to support both IrDA up to FIR and other uart-based IR applications. IIRC this came up on the iPAQs. I forget exactly which applications but I believe lirc might fall into the "want a serial port not the FIR network device interface" class. Having SIR always go via ldiscs should sort that out nicely as well. (I'm thinking maybe we want some control to set a port into IR transmission/reception mode independent of selecting the IrDA ldisc - but I'd suggest further research first - I may be just handwaving about an already solved problem.) -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
Hi, Russell, Russell, I'd propose to agree to bind PXA's IrDA to STUARTCLK and leave UARTCLK function to real pxa uart driver. If you would oppose, I won't have any choises other that to agree with it. -- With best wishes Dmitry --
Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
arch/arm/Kconfig | 1 +
arch/arm/mach-pxa/clock.c | 152 ++++++++++++++++----------------------------
arch/arm/mach-pxa/clock.h | 64 ++++++++----------
arch/arm/mach-pxa/pxa25x.c | 74 +++++++++++++--------
arch/arm/mach-pxa/pxa27x.c | 67 +++++++++++--------
arch/arm/mach-pxa/pxa3xx.c | 132 +++++++++++++++++++++-----------------
6 files changed, 244 insertions(+), 246 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 6d78a27..ce2ffe0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -402,6 +402,7 @@ config ARCH_PXA
select GENERIC_TIME
select GENERIC_CLOCKEVENTS
select TICK_ONESHOT
+ select HAVE_CLOCK_LIB
help
Support for Intel/Marvell's PXA2xx/PXA3xx processor line.
diff --git a/arch/arm/mach-pxa/clock.c b/arch/arm/mach-pxa/clock.c
index df5ae27..ba45ec0 100644
--- a/arch/arm/mach-pxa/clock.c
+++ b/arch/arm/mach-pxa/clock.c
@@ -8,6 +8,7 @@
#include <linux/err.h>
#include <linux/string.h>
#include <linux/clk.h>
+#include <linux/clklib.h>
#include <linux/spinlock.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
@@ -19,135 +20,92 @@
#include "generic.h"
#include "clock.h"
-static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
-static DEFINE_SPINLOCK(clocks_lock);
-
-static struct clk *clk_lookup(struct device *dev, const char *id)
+static int clk_gpio11_set_mode(struct clk *clk, bool enable)
{
- struct clk *p;
-
- list_for_each_entry(p, &clocks, node)
- if (strcmp(id, p->name) == 0 && p->dev == dev)
- return p;
+ if (enable)
+ pxa_gpio_mode(GPIO11_3_6MHz_MD);
- return NULL;
+ return 0;
}
-struct clk *clk_get(struct device *dev, const char *id)
+static unsigned long clk_gpio11_get_rate(struct clk *clk)
{
- struct clk *p, *clk = ERR_PTR(-ENOENT);
+ return 3686400;
+}
- mutex_lock(&clocks_mutex);
- p = clk_lookup(dev, i...Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
arch/arm/Kconfig | 1 +
arch/arm/mach-sa1100/clock.c | 131 ++++++++++-------------------------------
2 files changed, 33 insertions(+), 99 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4039a13..6d78a27 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -426,6 +426,7 @@ config ARCH_SA1100
select GENERIC_GPIO
select GENERIC_TIME
select HAVE_IDE
+ select HAVE_CLOCK_LIB
help
Support for StrongARM 11x0 based boards.
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index fc97fe5..8f9b777 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -8,127 +8,60 @@
#include <linux/err.h>
#include <linux/string.h>
#include <linux/clk.h>
+#include <linux/clklib.h>
#include <linux/spinlock.h>
#include <linux/mutex.h>
#include <asm/hardware.h>
-/*
- * Very simple clock implementation - we only have one clock to
- * deal with at the moment, so we only match using the "name".
- */
-struct clk {
- struct list_head node;
- unsigned long rate;
- const char *name;
- unsigned int enabled;
- void (*enable)(void);
- void (*disable)(void);
-};
-
-static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
-static DEFINE_SPINLOCK(clocks_lock);
-
-struct clk *clk_get(struct device *dev, const char *id)
-{
- struct clk *p, *clk = ERR_PTR(-ENOENT);
-
- mutex_lock(&clocks_mutex);
- list_for_each_entry(p, &clocks, node) {
- if (strcmp(id, p->name) == 0) {
- clk = p;
- break;
- }
- }
- mutex_unlock(&clocks_mutex);
-
- return clk;
-}
-EXPORT_SYMBOL(clk_get);
-
-void clk_put(struct clk *clk)
-{
-}
-EXPORT_SYMBOL(clk_put);
-
-int clk_enable(struct clk *clk)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&clocks_lock, flags);
- if (clk->enabled++ == 0)
- clk->enable();
- spin_unlock_irqrestore(&clocks_lock, ...Provide /sys/kernel/debug/clock to ease debugging.
Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
include/linux/clklib.h | 2 +
kernel/clklib.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+), 0 deletions(-)
diff --git a/include/linux/clklib.h b/include/linux/clklib.h
index 3d0ffaf..77514cc 100644
--- a/include/linux/clklib.h
+++ b/include/linux/clklib.h
@@ -19,6 +19,7 @@ struct seq_file;
* @get_rate: obtain the current clock rate of a specified clock
* @set_rate: set the clock rate for a specified clock
* @round_rate: adjust a reate to the exact rate a clock can provide
+ * @format: output any additional information for a clock
*
* This structure specifies clock operations that are used to configure
* specific clock.
@@ -30,6 +31,7 @@ struct clk_ops {
unsigned long (*get_rate) (struct clk *clk);
int (*set_rate) (struct clk *, unsigned long);
long (*round_rate) (struct clk *, unsigned long);
+ int (*format) (struct clk *, struct seq_file *);
};
/**
diff --git a/kernel/clklib.c b/kernel/clklib.c
index 012f845..8d872e1 100644
--- a/kernel/clklib.c
+++ b/kernel/clklib.c
@@ -324,3 +324,77 @@ out:
return rc;
}
EXPORT_SYMBOL(clk_alloc_function);
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#define NAME_FIELD_LEN 20
+
+static void dump_clocks(struct seq_file *s, struct clk *parent, int nest)
+{
+ struct clk *clk;
+ int i;
+
+ list_for_each_entry(clk, &clocks, node) {
+ if (clk->parent == parent) {
+ for (i = 0; i < nest; i++) {
+ seq_putc(s, ' ');
+ seq_putc(s, ' ');
+ }
+ seq_puts(s, clk->name);
+
+ i = 2 * nest + strlen(clk->name);
+ if (i >= NAME_FIELD_LEN)
+ i = NAME_FIELD_LEN - 1;
+ for (; i < NAME_FIELD_LEN; i++) {
+ seq_putc(s, ' ');
+ }
+ seq_printf(s, "%c use=%d rate=%10lu Hz",
+ clk->ops && clk->ops->set_parent ? '*' : ' ',
...On Thu, 3 Apr 2008 17:23:37 +0400 Plese fully document the proposed kernel->userspace interface in the It's conventional to leave no space between the ) and the ( here. Using a hm, this would break the one-value-per-file rule, except that applies to sysfs, but this interface is implemented via debug fs, but it is proposed that it appear under /sys. Tricky. --
You can do this easily by providing a file for Documentation/ABI/ Huh? How can a seq_file entry show up in sysfs? Or do you mean /sys/kernel/debug/ ? That's where debugfs is supposed to be mounted so that's fine, it's really a debugfs file, not a sysfs one. thanks, greg k-h --
Provide a generic framework that platform may choose
to support clocks api. In particular this provides
platform-independant struct clk definition, a full
implementation of clocks api and a set of functions
for registering and unregistering clocks in a safe way.
Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
include/linux/clklib.h | 145 +++++++++++++++++++++
init/Kconfig | 7 +
kernel/Makefile | 1 +
kernel/clklib.c | 326 ++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 479 insertions(+), 0 deletions(-)
create mode 100644 include/linux/clklib.h
create mode 100644 kernel/clklib.c
diff --git a/include/linux/clklib.h b/include/linux/clklib.h
new file mode 100644
index 0000000..3d0ffaf
--- /dev/null
+++ b/include/linux/clklib.h
@@ -0,0 +1,145 @@
+/*
+ * Copyright (C) 2008 Dmitry Baryshkov
+ *
+ * This file is released under the GPL v2.
+ */
+
+#ifndef CLKLIB_H
+#define CLKLIB_H
+
+#include <linux/list.h>
+
+struct seq_file;
+
+/**
+ * struct clk_ops - generic clock management operations
+ * @can_get: checks whether passed device can get this clock
+ * @set_parent: reconfigures the clock to use specified parent
+ * @set_mode: enable or disable specified clock
+ * @get_rate: obtain the current clock rate of a specified clock
+ * @set_rate: set the clock rate for a specified clock
+ * @round_rate: adjust a reate to the exact rate a clock can provide
+ *
+ * This structure specifies clock operations that are used to configure
+ * specific clock.
+ */
+struct clk_ops {
+ int (*can_get)(struct clk *clk, struct device *dev);
+ int (*set_parent)(struct clk *clk, struct clk *parent);
+ int (*set_mode) (struct clk *clk, bool enable);
+ unsigned long (*get_rate) (struct clk *clk);
+ int (*set_rate) (struct clk *, unsigned long);
+ long (*round_rate) (struct clk *, unsigned long);
+};
+
+/**
+ * struct clk - generic struct clk implementation used in the clocklib
+ * @node: used to place all c...On Thu, 3 Apr 2008 17:23:29 +0400 This should be `static int'. I'm surprised the compiler doesn't get There are a few coding-style glitches in here. Please run scripts/checkpatch.pl and consider its reportage. --
| Martin Michlmayr | Network slowdown due to CFS |
| Linus Torvalds | Linux 2.6.27-rc5 |
| Ingo Molnar | [git pull] x86 arch updates for v2.6.25 |
| Greg KH | [GIT PATCH] driver core patches against 2.6.24 |
git: | |
| Alexander Gladysh | [Q] Encrypted GIT? |
| Andreas Ericsson | Re: About git and the use of SHA-1 |
| Gerrit Pape | [PATCH/rfc] git-svn.perl: workaround assertions in svn library 1.5.0 |
| Matthieu Moy | git push to a non-bare repository |
| Christian Weisgerber | Re: libiconv problem |
| Richard Stallman | Real men don't attack straw men |
| GVG GVG | ssh_exchange_identification: Connection closed by remote host |
| Daniel Ouellet | identifying sparse files and get ride of them trick available? |
| Jarek Poplawski | Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl_lock(). |
| Jeff Garzik | Re: [bug?] tg3: Failed to load firmware "tigon/tg3_tso.bin" |
| Ben Hutchings | Re: [GIT]: Networking |
| Joerg Roedel | [PATCH 06/10] x86: add check code for map/unmap_sg code |
