[PATCH 5/6] fbdev: sh_mobile_meram: Add clock enable/disble hooks via runtime PM

[PATCH 5/6] fbdev: sh_mobile_meram: Add clock enable/disble hooks via runtime PM

[Damian Hobson-Garcia]

Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp>
---
 drivers/video/sh_mobile_meram.c |   27 +++++++++++++++++++++++++++
 include/video/sh_mobile_meram.h |    6 ++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/video/sh_mobile_meram.c b/drivers/video/sh_mobile_meram.c
index 9170c82..5e4ce98 100644
--- a/drivers/video/sh_mobile_meram.c
+++ b/drivers/video/sh_mobile_meram.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/pm_runtime.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
@@ -460,11 +461,33 @@ static int sh_mobile_meram_update(struct sh_mobile_meram_info *pdata,
 	return 0;
 }
 
+int sh_mobile_meram_clk_on(struct sh_mobile_meram_info *pdata)
+{
+	if (!pdata || !pdata->pdev)
+		return -EINVAL;
+
+	dev_dbg(&pdata->pdev->dev, "Enabling sh_mobile_meram clock.");
+	pm_runtime_get(&pdata->pdev->dev);
+	return 0;
+}
+
+int sh_mobile_meram_clk_off(struct sh_mobile_meram_info *pdata)
+{
+	if (!pdata || !pdata->pdev)
+		return -EINVAL;
+
+	dev_dbg(&pdata->pdev->dev, "Disabling sh_mobile_meram clock.");
+	pm_runtime_put(&pdata->pdev->dev);
+	return 0;
+}
+
 static struct sh_mobile_meram_ops sh_mobile_meram_ops = {
 	.module			= THIS_MODULE,
 	.meram_register		= sh_mobile_meram_register,
 	.meram_unregister	= sh_mobile_meram_unregister,
 	.meram_update		= sh_mobile_meram_update,
+	.meram_clk_off		= sh_mobile_meram_clk_off,
+	.meram_clk_on		= sh_mobile_meram_clk_on,
 };
 
 /*
@@ -515,6 +538,8 @@ static int __devinit sh_mobile_meram_probe(struct platform_device *pdev)
 	if (pdata->addr_mode = SH_MOBILE_MERAM_MODE1)
 		meram_write_reg(priv->base, MEVCR1, 1 << 29);
 
+	pm_runtime_enable(&pdev->dev);
+
 	dev_info(&pdev->dev, "sh_mobile_meram initialized.");
 
 	return 0;
@@ -530,6 +555,8 @@ static int sh_mobile_meram_remove(struct platform_device *pdev)
 {
 	struct sh_mobile_meram_priv *priv = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&pdev->dev);
+
 	if (priv->base)
 		iounmap(priv->base);
 
diff --git a/include/video/sh_mobile_meram.h b/include/video/sh_mobile_meram.h
index af602d6..3605874 100644
--- a/include/video/sh_mobile_meram.h
+++ b/include/video/sh_mobile_meram.h
@@ -63,6 +63,12 @@ struct sh_mobile_meram_ops {
 			    unsigned long base_addr_c,
 			    unsigned long *icb_addr_y,
 			    unsigned long *icb_addr_c);
+
+	/* enable meram clock */
+	int (*meram_clk_on)(struct sh_mobile_meram_info *meram_dev);
+
+	/* disable meram clock */
+	int (*meram_clk_off)(struct sh_mobile_meram_info *meram_dev);
 };
 
 #endif /* __VIDEO_SH_MOBILE_MERAM_H__  */
-- 
1.7.1

Re: [PATCH 5/6] fbdev: sh_mobile_meram: Add clock enable/disble hooks

[Magnus Damm]

On Wed, May 18, 2011 at 8:10 PM, Damian Hobson-Garcia
<dhobsong@igel.co.jp> wrote:
> Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp>
> ---
>  drivers/video/sh_mobile_meram.c |   27 +++++++++++++++++++++++++++
>  include/video/sh_mobile_meram.h |    6 ++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_meram.c b/drivers/video/sh_mobile_meram.c
> index 9170c82..5e4ce98 100644
> --- a/drivers/video/sh_mobile_meram.c
> +++ b/drivers/video/sh_mobile_meram.c
> @@ -12,6 +12,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> @@ -460,11 +461,33 @@ static int sh_mobile_meram_update(struct sh_mobile_meram_info *pdata,
>        return 0;
>  }
>
> +int sh_mobile_meram_clk_on(struct sh_mobile_meram_info *pdata)
> +{
> +       if (!pdata || !pdata->pdev)
> +               return -EINVAL;
> +
> +       dev_dbg(&pdata->pdev->dev, "Enabling sh_mobile_meram clock.");
> +       pm_runtime_get(&pdata->pdev->dev);

This should be pm_runtime_get_sync() to force block until the clock is enabled.

> diff --git a/include/video/sh_mobile_meram.h b/include/video/sh_mobile_meram.h
> index af602d6..3605874 100644
> --- a/include/video/sh_mobile_meram.h
> +++ b/include/video/sh_mobile_meram.h
> @@ -63,6 +63,12 @@ struct sh_mobile_meram_ops {
>                            unsigned long base_addr_c,
>                            unsigned long *icb_addr_y,
>                            unsigned long *icb_addr_c);
> +
> +       /* enable meram clock */
> +       int (*meram_clk_on)(struct sh_mobile_meram_info *meram_dev);
> +
> +       /* disable meram clock */
> +       int (*meram_clk_off)(struct sh_mobile_meram_info *meram_dev);

Hm, we need more than just clock control. Runtime PM is used for both
clock and power domain control. This means that after pm_runtime_put()
the power to the MERAM may be turned off. So you probably want to add
some context save/restore code to make sure the MERAM settings are
re-initialized after power-up. At this point there are no patches
upstream for this, but I think we should give it a try after
2.6.40-rc1 or rc2 is out.

Thanks,

/ magnus

Re: [PATCH 5/6] fbdev: sh_mobile_meram: Add clock enable/disble hooks

[Damian Hobson-Garcia]

Hi Magnus,
>> +int sh_mobile_meram_clk_on(struct sh_mobile_meram_info *pdata)
>> +{
>> +       if (!pdata || !pdata->pdev)
>> +               return -EINVAL;
>> +
>> +       dev_dbg(&pdata->pdev->dev, "Enabling sh_mobile_meram clock.");
>> +       pm_runtime_get(&pdata->pdev->dev);
> 
> This should be pm_runtime_get_sync() to force block until the clock is enabled.
Not a problem. I'll update that.
> 
>> diff --git a/include/video/sh_mobile_meram.h b/include/video/sh_mobile_meram.h
>> index af602d6..3605874 100644
>> --- a/include/video/sh_mobile_meram.h
>> +++ b/include/video/sh_mobile_meram.h
>> @@ -63,6 +63,12 @@ struct sh_mobile_meram_ops {
>>                            unsigned long base_addr_c,
>>                            unsigned long *icb_addr_y,
>>                            unsigned long *icb_addr_c);
>> +
>> +       /* enable meram clock */
>> +       int (*meram_clk_on)(struct sh_mobile_meram_info *meram_dev);
>> +
>> +       /* disable meram clock */
>> +       int (*meram_clk_off)(struct sh_mobile_meram_info *meram_dev);
> 
> Hm, we need more than just clock control. Runtime PM is used for both
> clock and power domain control. This means that after pm_runtime_put()
> the power to the MERAM may be turned off. So you probably want to add
> some context save/restore code to make sure the MERAM settings are
> re-initialized after power-up. At this point there are no patches
> upstream for this, but I think we should give it a try after
> 2.6.40-rc1 or rc2 is out.
>
WRT the context save/restore, I'm thinking that unlike the clock
enable/disable, which is tied into the LCDC driver clocks, the context
save/restore should be implemented via the ->runtime_suspend(),
->runtime_restore() callbacks, tied into the power domain (which is this
case is actually the same as LCDC A4LC). If that makes sense I'll submit
a patch to that effect along with the above.

Thanks,
Damian

Re: [PATCH 5/6] fbdev: sh_mobile_meram: Add clock enable/disble hooks

[Magnus Damm]

On Tue, May 24, 2011 at 12:04 PM, Damian Hobson-Garcia
<dhobsong@igel.co.jp> wrote:
> Hi Magnus,
>>> +int sh_mobile_meram_clk_on(struct sh_mobile_meram_info *pdata)
>>> +{
>>> +       if (!pdata || !pdata->pdev)
>>> +               return -EINVAL;
>>> +
>>> +       dev_dbg(&pdata->pdev->dev, "Enabling sh_mobile_meram clock.");
>>> +       pm_runtime_get(&pdata->pdev->dev);
>>
>> This should be pm_runtime_get_sync() to force block until the clock is enabled.
> Not a problem. I'll update that.
>>
>>> diff --git a/include/video/sh_mobile_meram.h b/include/video/sh_mobile_meram.h
>>> index af602d6..3605874 100644
>>> --- a/include/video/sh_mobile_meram.h
>>> +++ b/include/video/sh_mobile_meram.h
>>> @@ -63,6 +63,12 @@ struct sh_mobile_meram_ops {
>>>                            unsigned long base_addr_c,
>>>                            unsigned long *icb_addr_y,
>>>                            unsigned long *icb_addr_c);
>>> +
>>> +       /* enable meram clock */
>>> +       int (*meram_clk_on)(struct sh_mobile_meram_info *meram_dev);
>>> +
>>> +       /* disable meram clock */
>>> +       int (*meram_clk_off)(struct sh_mobile_meram_info *meram_dev);
>>
>> Hm, we need more than just clock control. Runtime PM is used for both
>> clock and power domain control. This means that after pm_runtime_put()
>> the power to the MERAM may be turned off. So you probably want to add
>> some context save/restore code to make sure the MERAM settings are
>> re-initialized after power-up. At this point there are no patches
>> upstream for this, but I think we should give it a try after
>> 2.6.40-rc1 or rc2 is out.
>>
> WRT the context save/restore, I'm thinking that unlike the clock
> enable/disable, which is tied into the LCDC driver clocks, the context
> save/restore should be implemented via the ->runtime_suspend(),
> ->runtime_restore() callbacks, tied into the power domain (which is this
> case is actually the same as LCDC A4LC). If that makes sense I'll submit
> a patch to that effect along with the above.

So the context save/restore needs to happen from the MERAM driver
callbacks. But with Runtime PM they may only be executed in the "idle"
state (after put()) and if I've understood these patches correctly you
use pm_runtime_get_sync() in the meram_clk_on/off() callbacks. My
point is that the callbacks are named "meram_clk_on/off" but you
actually control more than the clock state.

Let's revisit all this after Linuxcon Japan and 2.6.40-rcN is out.

Thanks,

/ magnus