[PATCH 0/3] [RFC] introduce clk_associate

[PATCH 0/3] [RFC] introduce clk_associate

[Felipe Balbi]

The following patches introduce a new mechanism
to associate the device with its clock during
platform_device registration and move musb and
watchdog drivers to use the new mechanism as
an example.

Felipe Balbi (3):
  clk: introduce clk_associate
  clk: use clk_associate for musb driver
  clk: use clk_associate on watchdog driver

 Documentation/arm/OMAP/clk_function_name.txt |   13 ++++
 arch/arm/mach-omap2/usb-musb.c               |   34 +++-------
 arch/arm/plat-omap/clock.c                   |   38 +++++++++++-
 arch/arm/plat-omap/devices.c                 |   18 ++++--
 arch/arm/plat-omap/include/mach/clock.h      |    5 ++
 drivers/usb/musb/musb_core.c                 |   23 ++-----
 drivers/watchdog/omap_wdt.c                  |   87 ++++++++------------------
 7 files changed, 109 insertions(+), 109 deletions(-)
 create mode 100644 Documentation/arm/OMAP/clk_function_name.txt

[PATCH 1/3] [RFC] clk: introduce clk_associate

[Felipe Balbi]

Introduce a new mechanism to omap's clk implementation to
associate the device with its clock during platform_device
registration. Also gives the clock a function name (like
mmc_fck, uart_fck, ehci_fck, etc) so drivers won't have to
care about clk names any longer.

With this mechanism we can also be sure drivers won't be able
to clk_get the wrong clk (only true when we move all drivers
to this new mechanism and drop the old ones).

Clk's function names will be defined as they come necessary
but let's try to keep it as simple as possible.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 arch/arm/plat-omap/clock.c              |   38 +++++++++++++++++++++++++++++-
 arch/arm/plat-omap/include/mach/clock.h |    5 ++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index 197974d..1c1f6c7 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -43,7 +43,7 @@ static struct clk_functions *arch_clock;
  */
 struct clk * clk_get(struct device *dev, const char *id)
 {
-	struct clk *p, *clk = ERR_PTR(-ENOENT);
+	struct clk *p, *clk;
 	int idno;
 
 	if (dev == NULL || dev->bus != &platform_bus_type)
@@ -53,6 +53,15 @@ struct clk * clk_get(struct device *dev, const char *id)
 
 	mutex_lock(&clocks_mutex);
 
+	list_for_each_entry(clk, &clocks, node) {
+		if (clk->function && (dev == clk->dev) &&
+				strcmp(id, clk->function) == 0)
+			goto found;
+
+		if (strcmp(id, clk->name) == 0)
+			goto found;
+	}
+
 	list_for_each_entry(p, &clocks, node) {
 		if (p->id == idno &&
 		    strcmp(id, p->name) == 0 && try_module_get(p->owner)) {
@@ -64,10 +73,14 @@ struct clk * clk_get(struct device *dev, const char *id)
 	list_for_each_entry(p, &clocks, node) {
 		if (strcmp(id, p->name) == 0 && try_module_get(p->owner)) {
 			clk = p;
-			break;
+			goto found;
 		}
 	}
 
+	mutex_unlock(&clocks_mutex);
+
+	return ERR_PTR(-ENOENT);
+
 found:
 	mutex_unlock(&clocks_mutex);
 
@@ -75,6 +88,27 @@ found:
 }
 EXPORT_SYMBOL(clk_get);
 
+/**
+ * clk_associate - associates a user to a clock so device drivers don't
+ * have to care about clock names
+ *
+ * @id: clock id as defined in arch/arm/mach-omapX/clkxxxx.h
+ * @dev: device pointer for the clock user
+ * @f: a function for the clock (uart_[if]ck, musb_ick, ehci_[if]ck, etc)
+ */
+void __init clk_associate(const char *id, struct device *dev, const char *f)
+{
+	struct clk *clk = clk_get(NULL, id);
+
+	if (!dev || !clk || !IS_ERR(clk_get(dev, f)))
+		return;
+
+	clk->function = f;
+	clk->dev = dev;
+
+	clk_put(clk);
+};
+
 int clk_enable(struct clk *clk)
 {
 	unsigned long flags;
diff --git a/arch/arm/plat-omap/include/mach/clock.h b/arch/arm/plat-omap/include/mach/clock.h
index 9088925..dcfb205 100644
--- a/arch/arm/plat-omap/include/mach/clock.h
+++ b/arch/arm/plat-omap/include/mach/clock.h
@@ -13,6 +13,8 @@
 #ifndef __ARCH_ARM_OMAP_CLOCK_H
 #define __ARCH_ARM_OMAP_CLOCK_H
 
+#include <linux/device.h>
+
 struct module;
 struct clk;
 struct clockdomain;
@@ -61,8 +63,10 @@ struct dpll_data {
 
 struct clk {
 	struct list_head	node;
+	struct device		*dev;
 	struct module		*owner;
 	const char		*name;
+	const char		*function;
 	int			id;
 	struct clk		*parent;
 	unsigned long		rate;
@@ -116,6 +120,7 @@ struct clk_functions {
 
 extern unsigned int mpurate;
 
+extern void clk_associate(const char *id, struct device *dev, const char *func);
 extern int clk_init(struct clk_functions *custom_clocks);
 extern int clk_register(struct clk *clk);
 extern void clk_unregister(struct clk *clk);
-- 
1.6.0.2.307.gc427

[PATCH 2/3] [RFC] clk: use clk_associate for musb driver

[Felipe Balbi]

Let musb use clk_associate to avoid introducing non-standard
clk functions and passing a clk name via pdata.

Also introduce txt file to track clk function names.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 Documentation/arm/OMAP/clk_function_name.txt |    9 +++++++
 arch/arm/mach-omap2/usb-musb.c               |   34 +++++++-------------------
 drivers/usb/musb/musb_core.c                 |   23 +++++------------
 3 files changed, 25 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/arm/OMAP/clk_function_name.txt

diff --git a/Documentation/arm/OMAP/clk_function_name.txt b/Documentation/arm/OMAP/clk_function_name.txt
new file mode 100644
index 0000000..85cc980
--- /dev/null
+++ b/Documentation/arm/OMAP/clk_function_name.txt
@@ -0,0 +1,9 @@
+			OMAP Clock Function Names
+
+When moving a new driver to use clk_associate, please add here the clk
+function name given to the clks in omap1, omap2 and omap3.
+
+
+Function name		|     OMAP1	|     OMAP2	|      OMAP3
+-------------------------------------------------------------------------------
+musb_ick		|	-	|   usbhs_ick	|   hsotgusb_ick
diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
index 61c5709..2df4f84 100644
--- a/arch/arm/mach-omap2/usb-musb.c
+++ b/arch/arm/mach-omap2/usb-musb.c
@@ -31,6 +31,7 @@
 #include <mach/pm.h>
 #include <mach/mux.h>
 #include <mach/usb.h>
+#include <mach/clock.h>
 
 #ifdef CONFIG_USB_MUSB_SOC
 static struct resource musb_resources[] = {
@@ -53,27 +54,6 @@ static struct resource musb_resources[] = {
 	},
 };
 
-static int clk_on;
-
-static int musb_set_clock(struct clk *clk, int state)
-{
-	if (state) {
-		if (clk_on > 0)
-			return -ENODEV;
-
-		clk_enable(clk);
-		clk_on = 1;
-	} else {
-		if (clk_on == 0)
-			return -ENODEV;
-
-		clk_disable(clk);
-		clk_on = 0;
-	}
-
-	return 0;
-}
-
 static struct musb_hdrc_eps_bits musb_eps[] = {
 	{	"ep1_tx", 10,	},
 	{	"ep1_rx", 10,	},
@@ -127,10 +107,6 @@ static struct musb_hdrc_platform_data musb_plat = {
 #elif defined(CONFIG_USB_GADGET_MUSB_HDRC)
 	.mode		= MUSB_PERIPHERAL,
 #endif
-	.clock		= cpu_is_omap34xx()
-			? "hsotgusb_ick"
-			: "usbhs_ick",
-	.set_clock	= musb_set_clock,
 	.config		= &musb_config,
 
 	/* REVISIT charge pump on TWL4030 can supply up to
@@ -159,6 +135,14 @@ static struct platform_device musb_device = {
 void __init usb_musb_init(void)
 {
 #ifdef CONFIG_USB_MUSB_SOC
+	/* associate musb clocks with musb device */
+
+	if (cpu_is_omap34xx())
+		clk_associate("hsotgusb_ick", &musb_device.dev, "musb_ick");
+
+	if (cpu_is_omap24xx())
+		clk_associate("usbhs_ick", &musb_device.dev, "musb_ick");
+
 	if (platform_device_register(&musb_device) < 0) {
 		printk(KERN_ERR "Unable to register HS-USB (MUSB) device\n");
 		return;
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index c939f81..152cda7 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1922,7 +1922,6 @@ bad_config:
 	spin_lock_init(&musb->lock);
 	musb->board_mode = plat->mode;
 	musb->board_set_power = plat->set_power;
-	musb->set_clock = plat->set_clock;
 	musb->min_power = plat->min_power;
 
 	/* Clock usage is chip-specific ... functional clock (DaVinci,
@@ -1930,13 +1929,11 @@ bad_config:
 	 * code does is make sure a clock handle is available; platform
 	 * code manages it during start/stop and suspend/resume.
 	 */
-	if (plat->clock) {
-		musb->clock = clk_get(dev, plat->clock);
-		if (IS_ERR(musb->clock)) {
-			status = PTR_ERR(musb->clock);
-			musb->clock = NULL;
-			goto fail;
-		}
+	musb->clock = clk_get(dev, "musb_ick");
+	if (IS_ERR(musb->clock)) {
+		status = PTR_ERR(musb->clock);
+		musb->clock = NULL;
+		goto fail;
 	}
 
 	/* assume vbus is off */
@@ -2159,10 +2156,7 @@ static int musb_suspend(struct platform_device *pdev, pm_message_t message)
 		 */
 	}
 
-	if (musb->set_clock)
-		musb->set_clock(musb->clock, 0);
-	else
-		clk_disable(musb->clock);
+	clk_disable(musb->clock);
 	spin_unlock_irqrestore(&musb->lock, flags);
 	return 0;
 }
@@ -2177,10 +2171,7 @@ static int musb_resume(struct platform_device *pdev)
 
 	spin_lock_irqsave(&musb->lock, flags);
 
-	if (musb->set_clock)
-		musb->set_clock(musb->clock, 1);
-	else
-		clk_enable(musb->clock);
+	clk_enable(musb->clock);
 
 	/* for static cmos like DaVinci, register values were preserved
 	 * unless for some reason the whole soc powered down and we're
-- 
1.6.0.2.307.gc427

[PATCH 3/3] [RFC] clk: use clk_associate on watchdog driver

[Felipe Balbi]

Get rid of clk mess on omap_watchdog driver by
using clk_associate.

Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 Documentation/arm/OMAP/clk_function_name.txt |    4 +
 arch/arm/plat-omap/devices.c                 |   18 ++++--
 drivers/watchdog/omap_wdt.c                  |   87 ++++++++------------------
 3 files changed, 43 insertions(+), 66 deletions(-)

diff --git a/Documentation/arm/OMAP/clk_function_name.txt b/Documentation/arm/OMAP/clk_function_name.txt
index 85cc980..19a3ed6 100644
--- a/Documentation/arm/OMAP/clk_function_name.txt
+++ b/Documentation/arm/OMAP/clk_function_name.txt
@@ -7,3 +7,7 @@ function name given to the clks in omap1, omap2 and omap3.
 Function name		|     OMAP1	|     OMAP2	|      OMAP3
 -------------------------------------------------------------------------------
 musb_ick		|	-	|   usbhs_ick	|   hsotgusb_ick
+-------------------------------------------------------------------------------
+wdt_fck			| armwdt_ck	|   mpu_wdt_fck	|   wdt2_fck
+-------------------------------------------------------------------------------
+wdt_ick			|	-	|   mpu_wdt_fck	|   wdt2_ick
diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index 1ad179d..6ce9368 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -298,16 +298,24 @@ static struct platform_device omap_wdt_device = {
 
 static void omap_init_wdt(void)
 {
-	if (cpu_is_omap16xx())
+	if (cpu_is_omap16xx()) {
+		clk_associate("armwdt_ck", &omap_wdt_device.dev, "wdt_fck");
 		wdt_resources[0].start = 0xfffeb000;
-	else if (cpu_is_omap2420())
+	} else if (cpu_is_omap2420()) {
+		clk_associate("mpu_wdt_fck", &omap_wdt_device.dev, "wdt_fck");
+		clk_associate("mpu_wdt_ick", &omap_wdt_device.dev, "wdt_ick");
 		wdt_resources[0].start = 0x48022000; /* WDT2 */
-	else if (cpu_is_omap2430())
+	} else if (cpu_is_omap2430()) {
+		clk_associate("mpu_wdt_fck", &omap_wdt_device.dev, "wdt_fck");
+		clk_associate("mpu_wdt_ick", &omap_wdt_device.dev, "wdt_ick");
 		wdt_resources[0].start = 0x49016000; /* WDT2 */
-	else if (cpu_is_omap343x())
+	} else if (cpu_is_omap343x()) {
+		clk_associate("wdt2_fck", &omap_wdt_device.dev, "wdt_fck");
+		clk_associate("wdt2_ick", &omap_wdt_device.dev, "wdt_ick");
 		wdt_resources[0].start = 0x48314000; /* WDT2 */
-	else
+	} else {
 		return;
+	}
 
 	wdt_resources[0].end = wdt_resources[0].start + 0x4f;
 
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 8c02fb0..61f6e6a 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -60,9 +60,8 @@ struct omap_wdt_dev {
 	void __iomem    *base;          /* physical */
 	struct device   *dev;
 	int             omap_wdt_users;
-	struct clk      *armwdt_ck;
-	struct clk      *mpu_wdt_ick;
-	struct clk      *mpu_wdt_fck;
+	struct clk      *ick;
+	struct clk      *fck;
 	struct resource *mem;
 	struct miscdevice omap_wdt_miscdev;
 };
@@ -146,13 +145,10 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
 
-	if (cpu_is_omap16xx())
-		clk_enable(wdev->armwdt_ck);	/* Enable the clock */
-
-	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
-		clk_enable(wdev->mpu_wdt_ick);    /* Enable the interface clock */
-		clk_enable(wdev->mpu_wdt_fck);    /* Enable the functional clock */
-	}
+	if (wdev->fck)
+		clk_enable(wdev->fck);    /* Enable the functional clock */
+	if (wdev->ick)
+		clk_enable(wdev->ick);    /* Enable the interface clock */
 
 	/* initialize prescaler */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
@@ -303,44 +299,20 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 	wdev->omap_wdt_users = 0;
 	wdev->mem = mem;
 
-	if (cpu_is_omap16xx()) {
-		wdev->armwdt_ck = clk_get(&pdev->dev, "armwdt_ck");
-		if (IS_ERR(wdev->armwdt_ck)) {
-			ret = PTR_ERR(wdev->armwdt_ck);
-			wdev->armwdt_ck = NULL;
-			goto err_clk;
-		}
+	wdev->fck = clk_get(&pdev->dev, "wdt_fck");
+	if (IS_ERR(wdev->fck)) {
+		ret = PTR_ERR(wdev->fck);
+		wdev->fck = NULL;
+		goto err_clk;
 	}
 
-	if (cpu_is_omap24xx()) {
-		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "mpu_wdt_ick");
-		if (IS_ERR(wdev->mpu_wdt_ick)) {
-			ret = PTR_ERR(wdev->mpu_wdt_ick);
-			wdev->mpu_wdt_ick = NULL;
-			goto err_clk;
-		}
-		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "mpu_wdt_fck");
-		if (IS_ERR(wdev->mpu_wdt_fck)) {
-			ret = PTR_ERR(wdev->mpu_wdt_fck);
-			wdev->mpu_wdt_fck = NULL;
-			goto err_clk;
-		}
+	wdev->ick = clk_get(&pdev->dev, "wdt_ick");
+	if (IS_ERR(wdev->ick)) {
+		ret = PTR_ERR(wdev->ick);
+		wdev->ick = NULL;
+		goto err_clk;
 	}
 
-	if (cpu_is_omap34xx()) {
-		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "wdt2_ick");
-		if (IS_ERR(wdev->mpu_wdt_ick)) {
-			ret = PTR_ERR(wdev->mpu_wdt_ick);
-			wdev->mpu_wdt_ick = NULL;
-			goto err_clk;
-		}
-		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "wdt2_fck");
-		if (IS_ERR(wdev->mpu_wdt_fck)) {
-			ret = PTR_ERR(wdev->mpu_wdt_fck);
-			wdev->mpu_wdt_fck = NULL;
-			goto err_clk;
-		}
-	}
 	wdev->base = ioremap(res->start, res->end - res->start + 1);
 	if (!wdev->base) {
 		ret = -ENOMEM;
@@ -380,12 +352,10 @@ err_ioremap:
 	wdev->base = NULL;
 
 err_clk:
-	if (wdev->armwdt_ck)
-		clk_put(wdev->armwdt_ck);
-	if (wdev->mpu_wdt_ick)
-		clk_put(wdev->mpu_wdt_ick);
-	if (wdev->mpu_wdt_fck)
-		clk_put(wdev->mpu_wdt_fck);
+	if (wdev->ick)
+		clk_put(wdev->ick);
+	if (wdev->fck)
+		clk_put(wdev->fck);
 	kfree(wdev);
 
 err_kzalloc:
@@ -417,19 +387,14 @@ static int omap_wdt_remove(struct platform_device *pdev)
 	release_mem_region(res->start, res->end - res->start + 1);
 	platform_set_drvdata(pdev, NULL);
 
-	if (wdev->armwdt_ck) {
-		clk_put(wdev->armwdt_ck);
-		wdev->armwdt_ck = NULL;
-	}
-
-	if (wdev->mpu_wdt_ick) {
-		clk_put(wdev->mpu_wdt_ick);
-		wdev->mpu_wdt_ick = NULL;
+	if (wdev->ick) {
+		clk_put(wdev->ick);
+		wdev->ick = NULL;
 	}
 
-	if (wdev->mpu_wdt_fck) {
-		clk_put(wdev->mpu_wdt_fck);
-		wdev->mpu_wdt_fck = NULL;
+	if (wdev->fck) {
+		clk_put(wdev->fck);
+		wdev->fck = NULL;
 	}
 	iounmap(wdev->base);
 
-- 
1.6.0.2.307.gc427

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[David Brownell]

On Wednesday 01 October 2008, Felipe Balbi wrote:
> +/**
> + * clk_associate - associates a user to a clock so device drivers don't
> + * have to care about clock names
> + *
> + * @id: clock id as defined in arch/arm/mach-omapX/clkxxxx.h
> + * @dev: device pointer for the clock user
> + * @f: a function for the clock (uart_[if]ck, musb_ick, ehci_[if]ck, etc)
> + */
> +void __init clk_associate(const char *id, struct device *dev, const char *f)

Heh.  I remember coming up with that same abstraction
for mach-at91/clock.c a few years back.  It seems to
have worked fairly well in that far simpler environment,
and I can't imagine why it wouldn't work here too.

The name might be confusing though, since it's not part
of the standard clk_*() interface ... and the name might
be needed there, eventually.

So mirroring "at91_clock_associate()" ... maybe this
should be "omap_clock_associate()" not "clk_associate()".

- Dave

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Felipe Balbi]

On Wed, Oct 01, 2008 at 08:51:46AM -0700, David Brownell wrote:
> On Wednesday 01 October 2008, Felipe Balbi wrote:
> > +/**
> > + * clk_associate - associates a user to a clock so device drivers don't
> > + * have to care about clock names
> > + *
> > + * @id: clock id as defined in arch/arm/mach-omapX/clkxxxx.h
> > + * @dev: device pointer for the clock user
> > + * @f: a function for the clock (uart_[if]ck, musb_ick, ehci_[if]ck, etc)
> > + */
> > +void __init clk_associate(const char *id, struct device *dev, const char *f)
> 
> Heh.  I remember coming up with that same abstraction
> for mach-at91/clock.c a few years back.  It seems to
> have worked fairly well in that far simpler environment,
> and I can't imagine why it wouldn't work here too.

aha, so it was you. I was reading clk implementations from other arm
architectures and found that at91_clk_associate() which seemed really
nice. So I decided to move to linux-omap since it'll solve some clk
issues we have here :-)

> The name might be confusing though, since it's not part
> of the standard clk_*() interface ... and the name might
> be needed there, eventually.

I suppose clk_associate() could become part of the clk api, yes. It's
quite useful when you have different version of the SoC with different
clk names (omap1/2/3, for instance).

> So mirroring "at91_clock_associate()" ... maybe this
> should be "omap_clock_associate()" not "clk_associate()".

Well, I'm ok with that but I'd rather see clk_associate() moving to
clk api so anyone who needs that, could use it.

-- 
balbi

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[David Brownell]

On Wednesday 01 October 2008, Felipe Balbi wrote:
> > So mirroring "at91_clock_associate()" ... maybe this
> > should be "omap_clock_associate()" not "clk_associate()".
> 
> Well, I'm ok with that but I'd rather see clk_associate() moving to
> clk api so anyone who needs that, could use it.

Seems like that's an "implementor's interface" rather
than a "user's interface".

So something like a "clock library" (duck!) might be
a good place for such a call... ;)

- dave

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Hiroshi DOYU]

From: "ext David Brownell" <david-b@pacbell.net>
Subject: Re: [PATCH 1/3] [RFC] clk: introduce clk_associate
Date: Wed, 1 Oct 2008 09:15:45 -0700

> On Wednesday 01 October 2008, Felipe Balbi wrote:
> > > So mirroring "at91_clock_associate()" ... maybe this
> > > should be "omap_clock_associate()" not "clk_associate()".
> > 
> > Well, I'm ok with that but I'd rather see clk_associate() moving to
> > clk api so anyone who needs that, could use it.
> 
> Seems like that's an "implementor's interface" rather
> than a "user's interface".
> 
> So something like a "clock library" (duck!) might be
> a good place for such a call... ;)

Or, this feature itself can be covered by 'virtual clock(vclk)'?

    http://marc.info/?l=linux-omap&m=122066992729949&w=2

which means that,
in this case, if 'vclk' just has a single child, not multiple, it can
be used just as 'aliasing' of clock names, without touching the
contents of 'struct clk', since 'vclk' is a inhritance of 'struct clk'.

I think that the point here is how to __hide__ the real detail clock
information(names, numbers, functionalites and so on) from client
device drivers. Some driver may need to control multiple clocks at
once. Some may need a clock which has different names between omap1,
omap2/3 or target boards. Or some may need to control multiple clock
groups from the functional perspective. So I think that a *flexible*
infrastructure would be better to afford such requiments, keeping
'struct clk' as simple as possible.

       Hiroshi DOYU

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[David Brownell]

On Wednesday 01 October 2008, Hiroshi DOYU wrote:
> Or, this feature itself can be covered by 'virtual clock(vclk)'?
> 
>     http://marc.info/?l=linux-omap&m=122066992729949&w=2
> 
> which means that,
> in this case, if 'vclk' just has a single child, not multiple, it can
> be used just as 'aliasing' of clock names, without touching the
> contents of 'struct clk', since 'vclk' is a inhritance of 'struct clk'.

If clk_get(dev, alias) keys on "dev", then that could seem to
be an alternate implementation strategy.  Does it?

I've not been tracking the evolution of the OMAP clock tree
code, but the treatment of "dev" seems quite indirect.  It
doesn't seem possible, for example, to stick a programmable
clock onto a non-platform device ... even when that's the
relevant input to, for example, some external CODEC.


> I think that the point here is how to __hide__ the real detail clock
> information(names, numbers, functionalites and so on) from client
> device drivers.

That's one way to look at it.  Hiding is not the requirement
though; I have no problem if they can see that information.
(Not that the clock interfaces support querying by any scheme
more intelligent than looking up all possible names!)

The requirement is instead to provide a portable "logical" view
of the clock tree ... it doesn't matter if a "physical" view is
available too, or even that both models exist.


> Some driver may need to control multiple clocks at 
> once. Some may need a clock which has different names between omap1,
> omap2/3 or target boards. Or some may need to control multiple clock
> groups from the functional perspective. So I think that a *flexible*
> infrastructure would be better to afford such requiments, keeping
> 'struct clk' as simple as possible.

That vclk stuff looked a bit less obvious than I like.
Maybe I just haven't seen the need for those particular
flavors of flexibility.

- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Felipe Balbi]

On Thu, Oct 02, 2008 at 01:50:02PM -0700, David Brownell wrote:
> On Wednesday 01 October 2008, Hiroshi DOYU wrote:
> > Or, this feature itself can be covered by 'virtual clock(vclk)'?
> > 
> >     http://marc.info/?l=linux-omap&m=122066992729949&w=2
> > 
> > which means that,
> > in this case, if 'vclk' just has a single child, not multiple, it can
> > be used just as 'aliasing' of clock names, without touching the
> > contents of 'struct clk', since 'vclk' is a inhritance of 'struct clk'.
> 
> If clk_get(dev, alias) keys on "dev", then that could seem to
> be an alternate implementation strategy.  Does it?

in that case, clk_associate (or clk_add_alias like in pxa) would be
needed to associate the device with the struct clk, right ?

> I've not been tracking the evolution of the OMAP clock tree
> code, but the treatment of "dev" seems quite indirect.  It
> doesn't seem possible, for example, to stick a programmable
> clock onto a non-platform device ... even when that's the
> relevant input to, for example, some external CODEC.

in this case I think clk_associate would fail. Same for anything
else that "hides" the device pointer from board-files.

> That's one way to look at it.  Hiding is not the requirement
> though; I have no problem if they can see that information.
> (Not that the clock interfaces support querying by any scheme
> more intelligent than looking up all possible names!)

the problem with names for omap is that it's useful to name the clock
with the same name we see in TRM to ease the search (it's +3k pages
manual, anything to make searching easier is valid :-) but then again,
clk names changes among omap versions. So an alias (or function) name
would help us keeping the correct clk name in the header files and yet
have a standard/simpler name for drivers to use, like "mmc interface
clock" (mmc_ick) and "mmc functional clock" (mmc_fck). No matter if it's
omap1/2/3/... we could keep the function name the same while clk name
refers to TRM.

> The requirement is instead to provide a portable "logical" view
> of the clock tree ... it doesn't matter if a "physical" view is
> available too, or even that both models exist.

that's true. We need a logical and rather stable (I'm referring to clk
names here) view of the clk tree so drivers don't have to care anymore
about different clk names. Having to add cpu conditional code in the
driver just because of a different clk name doesn't sound doable anymore
neither passing an extra char * to the driver via pdata.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Hiroshi DOYU]

Hi David,

From: "ext David Brownell" <david-b@pacbell.net>
Subject: Re: [PATCH 1/3] [RFC] clk: introduce clk_associate
Date: Thu, 2 Oct 2008 13:50:02 -0700

> On Wednesday 01 October 2008, Hiroshi DOYU wrote:
> > Or, this feature itself can be covered by 'virtual clock(vclk)'?
> > 
> >     http://marc.info/?l=linux-omap&m=122066992729949&w=2
> > 
> > which means that,
> > in this case, if 'vclk' just has a single child, not multiple, it can
> > be used just as 'aliasing' of clock names, without touching the
> > contents of 'struct clk', since 'vclk' is a inhritance of 'struct clk'.
> 
<snip>
>
> > Some driver may need to control multiple clocks at 
> > once. Some may need a clock which has different names between omap1,
> > omap2/3 or target boards. Or some may need to control multiple clock
> > groups from the functional perspective. So I think that a *flexible*
> > infrastructure would be better to afford such requiments, keeping
> > 'struct clk' as simple as possible.
> 
> That vclk stuff looked a bit less obvious than I like.
> Maybe I just haven't seen the need for those particular
> flavors of flexibility.

I've looked around for some examples;). For this abstruction (or
logical clock view), one of the case which clk_associate doesn't deal
with is to handle multiple clocks together. There are some cases,
where multiple clocks are handled(enable/disable) at once as below:

 drivers/usb/gadget/omap_udc.c:	omap_udc_enable_clock()
 drivers/video/omap/rfbi.c:	rfbi_enable_clocks()
 arch/arm/mach-omap2/mcbsp.c:	omap_mcbsp_clk_enable()*1
 arch/arm/mach-omap2/serial.c:	omap_serial_enable_clocks()
 sound/arm/omap/eac.c:		eac_enable_clocks()

With vclk, all the above home-brewed functions, *_enable_clocks(), can
be replaced by a normal clk_enable(), with grouping the logical set of
clocks in advance.

For some of the above drivers, omap's "functional clock" and
"interface clock" doesn't make sense. For such device drivers, those
clocks may look just a single necessary clock and there's no "one to
one" correspondence from the omap clock functionality definitions
("ick"/"fck") perspective. I think that this is one of the examples,
where the flexibily is required. Since required functionaliies for
clocks depends on each device drivers, I think that it would give a
wider solution to let device drivers to define their logical
clocks(functionality) flexibly(not 1-to-1), rather than statically
pre-defined standardized functional names, which is the 1-to-1
correspondence of ick and fck in the TRM with aliasing.

But I agree on that 'vclk' is too flexible and I think that's why Paul
hasn't taken it yet;)

*1: http://marc.info/?l=linux-omap&m=122066992729951&w=2

     Hiroshi DOYU

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Tony Lindgren]

* Hiroshi DOYU <Hiroshi.DOYU@nokia.com> [081003 09:24]:
> Hi David,
> 
> From: "ext David Brownell" <david-b@pacbell.net>
> Subject: Re: [PATCH 1/3] [RFC] clk: introduce clk_associate
> Date: Thu, 2 Oct 2008 13:50:02 -0700
> 
> > On Wednesday 01 October 2008, Hiroshi DOYU wrote:
> > > Or, this feature itself can be covered by 'virtual clock(vclk)'?
> > > 
> > >     http://marc.info/?l=linux-omap&m=122066992729949&w=2
> > > 
> > > which means that,
> > > in this case, if 'vclk' just has a single child, not multiple, it can
> > > be used just as 'aliasing' of clock names, without touching the
> > > contents of 'struct clk', since 'vclk' is a inhritance of 'struct clk'.
> > 
> <snip>
> >
> > > Some driver may need to control multiple clocks at 
> > > once. Some may need a clock which has different names between omap1,
> > > omap2/3 or target boards. Or some may need to control multiple clock
> > > groups from the functional perspective. So I think that a *flexible*
> > > infrastructure would be better to afford such requiments, keeping
> > > 'struct clk' as simple as possible.
> > 
> > That vclk stuff looked a bit less obvious than I like.
> > Maybe I just haven't seen the need for those particular
> > flavors of flexibility.
> 
> I've looked around for some examples;). For this abstruction (or
> logical clock view), one of the case which clk_associate doesn't deal
> with is to handle multiple clocks together. There are some cases,
> where multiple clocks are handled(enable/disable) at once as below:
> 
>  drivers/usb/gadget/omap_udc.c:	omap_udc_enable_clock()
>  drivers/video/omap/rfbi.c:	rfbi_enable_clocks()
>  arch/arm/mach-omap2/mcbsp.c:	omap_mcbsp_clk_enable()*1
>  arch/arm/mach-omap2/serial.c:	omap_serial_enable_clocks()
>  sound/arm/omap/eac.c:		eac_enable_clocks()
> 
> With vclk, all the above home-brewed functions, *_enable_clocks(), can
> be replaced by a normal clk_enable(), with grouping the logical set of
> clocks in advance.

Adding something like the enable_clocks() we've already gotten comments
on, and it's considered abuse of the clock framework. So the drivers
should just use clk_enable/disable() and that's it.

Since some drivers may need to set fck and ick separately from PM
point of view, I think it's OK for the driver to handle multiple
clocks in the driver.

But maybe we can find a way to treat multiple clocks as one clock still
with clk_associate?

> For some of the above drivers, omap's "functional clock" and
> "interface clock" doesn't make sense. For such device drivers, those
> clocks may look just a single necessary clock and there's no "one to
> one" correspondence from the omap clock functionality definitions
> ("ick"/"fck") perspective. I think that this is one of the examples,
> where the flexibily is required. Since required functionaliies for
> clocks depends on each device drivers, I think that it would give a
> wider solution to let device drivers to define their logical
> clocks(functionality) flexibly(not 1-to-1), rather than statically
> pre-defined standardized functional names, which is the 1-to-1
> correspondence of ick and fck in the TRM with aliasing.

Maybe a combination of clk_associate() and adding a vclk in some cases
is the way to go?

Adding a vclk has the issue Paul pointed out on how to figure out the
parent. So the vclk would always have to implement custom set_rate()
and parent.

The main advantage I see with clk_associate() is that it solves the
trying to match struct device to struct clk with the name and instance.
Getting the instance right is not obvious as some clocks start numbering
at 0 and some at 1... If the driver does any logic on the instance,
things break easily in mysterious ways. Like the MMC did for hsmmc with
my recent MMC init patches.

> But I agree on that 'vclk' is too flexible and I think that's why Paul
> hasn't taken it yet;)

Or it should be used carefully and only when really needed.

Tony


> *1: http://marc.info/?l=linux-omap&m=122066992729951&w=2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[David Brownell]

On Thursday 02 October 2008, Hiroshi DOYU wrote:
> For some of the above drivers, omap's "functional clock" and
> "interface clock" doesn't make sense. 

Actually, I thought OMAP was pretty consistent about having
both on all modules where it made sense.


> For such device drivers, those 
> clocks may look just a single necessary clock and there's no "one to
> one" correspondence from the omap clock functionality definitions
> ("ick"/"fck") perspective.

They're all OMAP-specific drivers.  They can know that they
need to ask for both clocks.  If perchance only one of them
were actually needed, that would be exceptional ... and the
driver should be able to assume the device was properly
set up, and continue without it.


> I think that this is one of the examples, 
> where the flexibily is required. Since required functionaliies for
> clocks depends on each device drivers, I think that it would give a
> wider solution to let device drivers to define their logical
> clocks(functionality) flexibly(not 1-to-1), rather than statically
> pre-defined standardized functional names, which is the 1-to-1
> correspondence of ick and fck in the TRM with aliasing.

The ICK/FCK example is not IMO persuasive; this is OMAP, so
the assumption can be that all drivers have both.

Having the clocks set up by clk_associate() would suffice
for most devices and drivers.  Are there ones where that's
not enough ... where a device needs more than those "logical"
clock identifiers?

- Dave

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Felipe Balbi]

On Mon, Oct 06, 2008 at 11:42:08AM -0700, David Brownell wrote:
> They're all OMAP-specific drivers.  They can know that they
> need to ask for both clocks.  If perchance only one of them
> were actually needed, that would be exceptional ... and the
> driver should be able to assume the device was properly
> set up, and continue without it.

exactly, look at the changes to watchdog driver for an example :-)
if interface clock fails, we continue anyways since we could be using it
in an omap1-based board.

-- 
balbi

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Paul Walmsley]

Hi,

between omap_clk_associate() and vclk, my preference is for the 
omap_clk_associate() approach.

The core problem is that the vclk patches create clocks with multiple 
parents in a way that is hidden from the clock framework.  This causes 
both semantic and practical problems.

Semantically, the patches cause the meaning of set_rate() and set_parent() 
to be confused, and any driver that wants to call set_parent() or 
set_rate() on those clocks will need to have their own custom functions 
for those operations.  I'd like to keep the amount of that custom code 
minimized.

In practical terms, the vclk code will cause spinlock recursion bugs like 
the ones that currently exist with the McBSP driver:

   http://marc.info/?l=linux-omap&m=122310205615940&w=2

since vclks will call clk_enable()/disable()/set_rate()/etc. inside the 
clockfw_lock spinlock.  Using the vclk patches also means that the number 
of custom clocks will explode, as each driver is likely to define at 
least one custom clock.

[ eventually, we'll need to deal with the multiple parent issue, if for no 
other reason than to fix usecounting for clocks like dss_ick.  But it will 
need to be handled by the clock framework code itself.  And this problem 
is pretty low on the priority list...  ]

So between the two, I'd like to see omap_clk_associate() integrated.  I 
have some comments to post on Felipe's code; once those are 
addressed, hopefully we can merge it.


- Paul

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Hiroshi DOYU]

Hi Paul,

I understood both points you explained below, while I still think that
standardizing clock names may be a little bit rigid.

Thank you for your review and comments.

  Hiroshi DOYU

From: "ext Paul Walmsley" <paul@pwsan.com>
Subject: Re: [PATCH 1/3] [RFC] clk: introduce clk_associate
Date: Tue, 14 Oct 2008 10:33:42 -0600 (MDT)

> 
> Hi,
> 
> between omap_clk_associate() and vclk, my preference is for the 
> omap_clk_associate() approach.
> 
> The core problem is that the vclk patches create clocks with multiple 
> parents in a way that is hidden from the clock framework.  This causes 
> both semantic and practical problems.
> 
> Semantically, the patches cause the meaning of set_rate() and set_parent() 
> to be confused, and any driver that wants to call set_parent() or 
> set_rate() on those clocks will need to have their own custom functions 
> for those operations.  I'd like to keep the amount of that custom code 
> minimized.
> 
> In practical terms, the vclk code will cause spinlock recursion bugs like 
> the ones that currently exist with the McBSP driver:
> 
>    http://marc.info/?l=linux-omap&m=122310205615940&w=2
> 
> since vclks will call clk_enable()/disable()/set_rate()/etc. inside the 
> clockfw_lock spinlock.  Using the vclk patches also means that the number 
> of custom clocks will explode, as each driver is likely to define at 
> least one custom clock.
> 
> [ eventually, we'll need to deal with the multiple parent issue, if for no 
> other reason than to fix usecounting for clocks like dss_ick.  But it will 
> need to be handled by the clock framework code itself.  And this problem 
> is pretty low on the priority list...  ]
> 
> So between the two, I'd like to see omap_clk_associate() integrated.  I 
> have some comments to post on Felipe's code; once those are 
> addressed, hopefully we can merge it.
> 
> 
> - Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Paul Walmsley]

Hello Hiroshi,

On Tue, 14 Oct 2008, Hiroshi DOYU wrote:

> I understood both points you explained below, while I still think that
> standardizing clock names may be a little bit rigid.

Perhaps you can help me understand - are you referring to the use of the 
TRM clocks, rather than creating a custom clock for each device driver?  
Or are you referring to the clock names used in the omap_clk_associate() 
calls?

> Thank you for your review and comments.

You're welcome,

- Paul

Re: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Hiroshi DOYU]

Hi Paul,

From: "ext Paul Walmsley" <paul@pwsan.com>
Subject: Re: [PATCH 1/3] [RFC] clk: introduce clk_associate
Date: Wed, 15 Oct 2008 03:13:49 -0600 (MDT)

> Hello Hiroshi,
> 
> On Tue, 14 Oct 2008, Hiroshi DOYU wrote:
> 
> > I understood both points you explained below, while I still think that
> > standardizing clock names may be a little bit rigid.
> 
> Perhaps you can help me understand - are you referring to the use of the 
> TRM clocks, rather than creating a custom clock for each device driver?  
> Or are you referring to the clock names used in the omap_clk_associate() 
> calls?

This has been already solved since you suggested just to use more
shorter logical name, "ick" and "fck". It would be much cleaner;)

	Hiroshi DOYU

RE: [PATCH 1/3] [RFC] clk: introduce clk_associate

[Woodruff, Richard]

Hi,

> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Paul Walmsley

> between omap_clk_associate() and vclk, my preference is for the
> omap_clk_associate() approach.
>
> The core problem is that the vclk patches create clocks with multiple
> parents in a way that is hidden from the clock framework.  This causes
> both semantic and practical problems.
>
> Semantically, the patches cause the meaning of set_rate() and set_parent()
> to be confused, and any driver that wants to call set_parent() or
> set_rate() on those clocks will need to have their own custom functions
> for those operations.  I'd like to keep the amount of that custom code
> minimized.

I agree and these mirror my comments when it was first proposed.

Having a virtual node does however have some benefits which may be worth exploiting for future.

For these types of nodes it might be they just return an error if someone tries to use them.  Or have some way to get its attributes.

When you do allow a set rate on a virtual clock it will have to be custom.  There may be a number of valid internal combinations.  Providing a "clock-cluster" OPP table is probably enough.

Regards,
Richard W.