| Message ID | 1305717011-20742-6-git-send-email-dhobsong@igel.co.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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__ */
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(-)