video: amba_clcd: Clock balance fixes and power optimization changes

video: amba_clcd: Clock balance fixes and power optimization changes

[wellsk40 at gmail.com]

These patches fix clock balance issues (clk_disable called when
clk_enabled was not previously called) and optimizes clocking for
register access.

[PATCH 1/2] video: amba_clcd: Fix driver clock enable/disable balance issues
[PATCH 2/2] video: amba_clcd: Seperate controller and register clock enables

Patch 1 is basically RK's suggested fix repackaged here.

The register clocking fix was written to avoid calling the AMBA
clock enable and disable functions as nested calls (ie, 2 calls
to disable). The number of calls can be reduced by a few calls if
nesting can be used. This depends on the arch's clock drivers and
whether they support nesting. The method used here is safe for
clock drivers that don't like nested calls.

These patches can be pulled from:
	git://lpclinux.com/linux-2.6-lpc pl11x_clk_fixes

[PATCH 1/2] video: amba_clcd: Fix driver clock enable/disable balance issues

[wellsk40 at gmail.com]

From: Kevin Wells <wellsk40@gmail.com>

The amba clcd clock enable and disable calls can become unbalanced.
This usually occurs on the first call to clcdfb_set_par() during
LCD init, but can also occur when the LCD enters and exits blank
state. This fix prevents clk_disable() from being called until
clk_enable() has been called first.

Signed-off-by: Kevin Wells <wellsk40@gmail.com>
---
 drivers/video/amba-clcd.c |   10 ++++++++--
 include/linux/amba/clcd.h |    1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
index afe21e6..1c2c683 100644
--- a/drivers/video/amba-clcd.c
+++ b/drivers/video/amba-clcd.c
@@ -80,7 +80,10 @@ static void clcdfb_disable(struct clcd_fb *fb)
 	/*
 	 * Disable CLCD clock source.
 	 */
-	clk_disable(fb->clk);
+	if (fb->clk_enabled) {
+		fb->clk_enabled = false;
+		clk_disable(fb->clk);
+	}
 }
 
 static void clcdfb_enable(struct clcd_fb *fb, u32 cntl)
@@ -88,7 +91,10 @@ static void clcdfb_enable(struct clcd_fb *fb, u32 cntl)
 	/*
 	 * Enable the CLCD clock source.
 	 */
-	clk_enable(fb->clk);
+	if (!fb->clk_enabled) {
+		fb->clk_enabled = true;
+		clk_enable(fb->clk);
+	}
 
 	/*
 	 * Bring up by first enabling..
diff --git a/include/linux/amba/clcd.h b/include/linux/amba/clcd.h
index ca16c38..7bbf26a 100644
--- a/include/linux/amba/clcd.h
+++ b/include/linux/amba/clcd.h
@@ -142,6 +142,7 @@ struct clcd_fb {
 	struct fb_info		fb;
 	struct amba_device	*dev;
 	struct clk		*clk;
+	bool			clk_enabled;
 	struct clcd_panel	*panel;
 	struct clcd_board	*board;
 	void			*board_data;
-- 
1.7.1.1

[PATCH 1/2] video: amba_clcd: Fix driver clock enable/disable balance issues

[Russell King - ARM Linux]

On Wed, Aug 11, 2010 at 04:03:03PM -0700, wellsk40 at gmail.com wrote:
> From: Kevin Wells <wellsk40@gmail.com>
> 
> The amba clcd clock enable and disable calls can become unbalanced.
> This usually occurs on the first call to clcdfb_set_par() during
> LCD init, but can also occur when the LCD enters and exits blank
> state. This fix prevents clk_disable() from being called until
> clk_enable() has been called first.
> 
> Signed-off-by: Kevin Wells <wellsk40@gmail.com>

Not quite the right way to do things, because this makes yourself the
author of the patch which is obviously wrong.

I'll commit it myself now, as I'm about to send stuff to Linus.  I'm
going to assume you've tested it, but as I don't know positively I
can't add an attributation for it.

[PATCH 1/2] video: amba_clcd: Fix driver clock enable/disable balance issues

[Kevin Wells]

> 
> On Wed, Aug 11, 2010 at 04:03:03PM -0700, wellsk40 at gmail.com wrote:
> > From: Kevin Wells <wellsk40@gmail.com>
> >
> > The amba clcd clock enable and disable calls can become unbalanced.
> > This usually occurs on the first call to clcdfb_set_par() during
> > LCD init, but can also occur when the LCD enters and exits blank
> > state. This fix prevents clk_disable() from being called until
> > clk_enable() has been called first.
> >
> > Signed-off-by: Kevin Wells <wellsk40@gmail.com>
> 
> Not quite the right way to do things, because this makes yourself the
> author of the patch which is obviously wrong.
> 
> I'll commit it myself now, as I'm about to send stuff to Linus.  I'm
> going to assume you've tested it, but as I don't know positively I
> can't add an attributation for it.
> 

Thanks for checking that in.
Sorry - I had sent another mail earlier a few weeks ago asking if you
wanted to check in that change (patch 1), but I think it got lost in
the shuffle.

Both patches are tested. You can add a "Tested by" from me if it's not
too late.

[PATCH 2/2] video: amba_clcd: Seperate controller and register clock enables

[wellsk40 at gmail.com]

From: Kevin Wells <wellsk40@gmail.com>

To optimize power for architectures that disable the clock to the
clcd primecell, clock enable logic for the controller (maintaining
the LCD) and accessing the registers (AMBA bus clock) has been
seperated.

When accessing registers, the register clock is enabled prior to
access and then disabled when accesses are complete. The clcd probe
function will disable the clock enabled by the AMBA bus driver prior
to exiting probe. The clock is re-enabled prior to exiting remove.
amba_pclk_enable() and amba_pclk_disable() functions macros are used
for register clock enable/disable. All calls to the macros are
balanced and un-nested.

Signed-off-by: Kevin Wells <wellsk40@gmail.com>
---
 drivers/video/amba-clcd.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
index 1c2c683..a67b33a 100644
--- a/drivers/video/amba-clcd.c
+++ b/drivers/video/amba-clcd.c
@@ -65,6 +65,8 @@ static void clcdfb_disable(struct clcd_fb *fb)
 	if (fb->board->disable)
 		fb->board->disable(fb);
 
+	amba_pclk_enable(fb->dev);
+
 	val = readl(fb->regs + fb->off_cntl);
 	if (val & CNTL_LCDPWR) {
 		val &= ~CNTL_LCDPWR;
@@ -77,6 +79,8 @@ static void clcdfb_disable(struct clcd_fb *fb)
 		writel(val, fb->regs + fb->off_cntl);
 	}
 
+	amba_pclk_disable(fb->dev);
+
 	/*
 	 * Disable CLCD clock source.
 	 */
@@ -96,6 +100,8 @@ static void clcdfb_enable(struct clcd_fb *fb, u32 cntl)
 		clk_enable(fb->clk);
 	}
 
+	amba_pclk_enable(fb->dev);
+
 	/*
 	 * Bring up by first enabling..
 	 */
@@ -110,6 +116,8 @@ static void clcdfb_enable(struct clcd_fb *fb, u32 cntl)
 	cntl |= CNTL_LCDPWR;
 	writel(cntl, fb->regs + fb->off_cntl);
 
+	amba_pclk_disable(fb->dev);
+
 	/*
 	 * finally, enable the interface.
 	 */
@@ -217,6 +225,7 @@ static int clcdfb_set_par(struct fb_info *info)
 	fb->board->decode(fb, &regs);
 
 	clcdfb_disable(fb);
+	amba_pclk_enable(fb->dev);
 
 	writel(regs.tim0, fb->regs + CLCD_TIM0);
 	writel(regs.tim1, fb->regs + CLCD_TIM1);
@@ -229,9 +238,11 @@ static int clcdfb_set_par(struct fb_info *info)
 
 	fb->clcd_cntl = regs.cntl;
 
+	amba_pclk_disable(fb->dev);
 	clcdfb_enable(fb, regs.cntl);
 
 #ifdef DEBUG
+	amba_pclk_enable(fb->dev);
 	printk(KERN_INFO
 	       "CLCD: Registers set to\n"
 	       "  %08x %08x %08x %08x\n"
@@ -240,6 +251,7 @@ static int clcdfb_set_par(struct fb_info *info)
 		readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
 		readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
 		readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
+	amba_pclk_disable(fb->dev);
 #endif
 
 	return 0;
@@ -290,8 +302,10 @@ clcdfb_setcolreg(unsigned int regno, unsigned int red, unsigned int green,
 			mask = 0xffff0000;
 		}
 
+		amba_pclk_enable(fb->dev);
 		val = readl(fb->regs + hw_reg) & mask;
 		writel(val | newval, fb->regs + hw_reg);
+		amba_pclk_disable(fb->dev);
 	}
 
 	return regno > 255;
@@ -492,6 +506,9 @@ static int clcdfb_probe(struct amba_device *dev, struct amba_id *id)
 
 	ret = clcdfb_register(fb); 
 	if (ret == 0) {
+		/* Disable AMBA PCLK */
+		amba_pclk_disable(dev);
+
 		amba_set_drvdata(dev, fb);
 		goto out;
 	}
@@ -524,6 +541,9 @@ static int clcdfb_remove(struct amba_device *dev)
 
 	amba_release_regions(dev);
 
+	/* Restore AMBA PCLK */
+	amba_pclk_enable(dev);
+
 	return 0;
 }
 
-- 
1.7.1.1

video: amba_clcd: Clock balance fixes and power optimization changes

[Kevin Wells]

On Wed, Aug 11, 2010 at 4:03 PM,  <wellsk40@gmail.com> wrote:
> These patches fix clock balance issues (clk_disable called when
> clk_enabled was not previously called) and optimizes clocking for
> register access.
>
> [PATCH 1/2] video: amba_clcd: Fix driver clock enable/disable balance issues
> [PATCH 2/2] video: amba_clcd: Seperate controller and register clock enables
>
> Patch 1 is basically RK's suggested fix repackaged here.
>
> The register clocking fix was written to avoid calling the AMBA
> clock enable and disable functions as nested calls (ie, 2 calls
> to disable). The number of calls can be reduced by a few calls if
> nesting can be used. This depends on the arch's clock drivers and
> whether they support nesting. The method used here is safe for
> clock drivers that don't like nested calls.
>

I haven't heard any feedback on these patch suggestions. Patch 1 is needed.
Patch 2 is highly desirable to keep power usage to a minimum when the
lcd is off.

> These patches can be pulled from:
> ? ? ? ?git://lpclinux.com/linux-2.6-lpc pl11x_clk_fixes
>
>