* [PATCH 25/27] OMAP4: CLKDEV: Remove omapdss clock aliases
From: Tomi Valkeinen @ 2011-06-03 10:00 UTC (permalink / raw)
To: linux-omap, linux-fbdev; +Cc: b-cousson, paul, khilman, Tomi Valkeinen
In-Reply-To: <1307095237-14805-1-git-send-email-tomi.valkeinen@ti.com>
omapdss driver now gets the clocks via hwmod opt clocks, so clock
aliases for omapdss_dss are no longer needed.
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/mach-omap2/clock44xx_data.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c
index 8c96567..4d278e7 100644
--- a/arch/arm/mach-omap2/clock44xx_data.c
+++ b/arch/arm/mach-omap2/clock44xx_data.c
@@ -3114,11 +3114,11 @@ static struct omap_clk omap44xx_clks[] = {
CLK(NULL, "dmic_sync_mux_ck", &dmic_sync_mux_ck, CK_443X),
CLK(NULL, "dmic_fck", &dmic_fck, CK_443X),
CLK(NULL, "dsp_fck", &dsp_fck, CK_443X),
- CLK("omapdss_dss", "sys_clk", &dss_sys_clk, CK_443X),
- CLK("omapdss_dss", "tv_clk", &dss_tv_clk, CK_443X),
- CLK("omapdss_dss", "video_clk", &dss_48mhz_clk, CK_443X),
- CLK("omapdss_dss", "fck", &dss_dss_clk, CK_443X),
- CLK("omapdss_dss", "ick", &dss_fck, CK_443X),
+ CLK(NULL, "sys_clk", &dss_sys_clk, CK_443X),
+ CLK(NULL, "tv_clk", &dss_tv_clk, CK_443X),
+ CLK(NULL, "video_clk", &dss_48mhz_clk, CK_443X),
+ CLK(NULL, "fck", &dss_dss_clk, CK_443X),
+ CLK(NULL, "ick", &dss_fck, CK_443X),
CLK(NULL, "efuse_ctrl_cust_fck", &efuse_ctrl_cust_fck, CK_443X),
CLK(NULL, "emif1_fck", &emif1_fck, CK_443X),
CLK(NULL, "emif2_fck", &emif2_fck, CK_443X),
--
1.7.4.1
^ permalink raw reply related
* [PATCH 26/27] OMAP: DSS2: DISPC: Fix context save/restore
From: Tomi Valkeinen @ 2011-06-03 10:00 UTC (permalink / raw)
To: linux-omap, linux-fbdev; +Cc: b-cousson, paul, khilman, Tomi Valkeinen
In-Reply-To: <1307095237-14805-1-git-send-email-tomi.valkeinen@ti.com>
The current method of saving and restoring the context could cause a
restore before saving, effectively "restoring" zero values to registers.
Add ctx_valid field to indicate if the saved context is valid and can be
restored.
Also restructure the code to save the ctx_loss_count in save_context(),
which makes more sense than the previous method of storing new
ctx_loss_count in dispc_need_ctx_restore.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dispc.c | 97 +++++++++++++++-----------------------
1 files changed, 38 insertions(+), 59 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index b13a656..73f1105 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -110,6 +110,7 @@ static struct {
u32 error_irqs;
struct work_struct error_work;
+ bool ctx_valid;
u32 ctx[DISPC_SZ_REGS / sizeof(u32)];
#ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
@@ -142,6 +143,23 @@ static inline u32 dispc_read_reg(const u16 idx)
return __raw_readl(dispc.base + idx);
}
+static int dispc_get_ctx_loss_count(void)
+{
+ struct device *dev = &dispc.pdev->dev;
+ struct omap_display_platform_data *pdata = dev->platform_data;
+ struct omap_dss_board_info *board_data = pdata->board_data;
+ int cnt;
+
+ if (!board_data->get_context_loss_count)
+ return -ENOENT;
+
+ cnt = board_data->get_context_loss_count(dev);
+
+ WARN_ONCE(cnt < 0, "get_context_loss_count failed: %d\n", cnt);
+
+ return cnt;
+}
+
#define SR(reg) \
dispc.ctx[DISPC_##reg / sizeof(u32)] = dispc_read_reg(DISPC_##reg)
#define RR(reg) \
@@ -318,14 +336,30 @@ static void dispc_save_context(void)
if (dss_has_feature(FEAT_CORE_CLK_DIV))
SR(DIVISOR);
+
+ dispc.ctx_loss_cnt = dispc_get_ctx_loss_count();
+ dispc.ctx_valid = true;
+
+ DSSDBG("context saved, ctx_loss_count %d\n", dispc.ctx_loss_cnt);
}
static void dispc_restore_context(void)
{
- int i;
+ int i, ctx;
DSSDBG("dispc_restore_context\n");
+ if (!dispc.ctx_valid)
+ return;
+
+ ctx = dispc_get_ctx_loss_count();
+
+ if (ctx >= 0 && ctx = dispc.ctx_loss_cnt)
+ return;
+
+ DSSDBG("ctx_loss_count: saved %d, current %d\n",
+ dispc.ctx_loss_cnt, ctx);
+
/*RR(IRQENABLE);*/
/*RR(CONTROL);*/
RR(CONFIG);
@@ -504,65 +538,13 @@ static void dispc_restore_context(void)
* the context is fully restored
*/
RR(IRQENABLE);
+
+ DSSDBG("context restored\n");
}
#undef SR
#undef RR
-static void dispc_init_ctx_loss_count(void)
-{
- struct device *dev = &dispc.pdev->dev;
- struct omap_display_platform_data *pdata = dev->platform_data;
- struct omap_dss_board_info *board_data = pdata->board_data;
- int cnt = 0;
-
- /*
- * get_context_loss_count returns negative on error. We'll ignore the
- * error and store the error to ctx_loss_cnt, which will cause
- * dispc_need_ctx_restore() call to return true.
- */
-
- if (board_data->get_context_loss_count)
- cnt = board_data->get_context_loss_count(dev);
-
- WARN_ON(cnt < 0);
-
- dispc.ctx_loss_cnt = cnt;
-
- DSSDBG("initial ctx_loss_cnt %u\n", cnt);
-}
-
-static bool dispc_need_ctx_restore(void)
-{
- struct device *dev = &dispc.pdev->dev;
- struct omap_display_platform_data *pdata = dev->platform_data;
- struct omap_dss_board_info *board_data = pdata->board_data;
- int cnt;
-
- /*
- * If get_context_loss_count is not available, assume that we need
- * context restore always.
- */
- if (!board_data->get_context_loss_count)
- return true;
-
- cnt = board_data->get_context_loss_count(dev);
- if (cnt < 0) {
- dev_err(dev, "getting context loss count failed, will force "
- "context restore\n");
- dispc.ctx_loss_cnt = cnt;
- return true;
- }
-
- if (cnt = dispc.ctx_loss_cnt)
- return false;
-
- DSSDBG("ctx_loss_cnt %d -> %d\n", dispc.ctx_loss_cnt, cnt);
- dispc.ctx_loss_cnt = cnt;
-
- return true;
-}
-
int dispc_runtime_get(void)
{
int r;
@@ -584,8 +566,7 @@ int dispc_runtime_get(void)
if (r < 0)
goto err_runtime_get;
- if (dispc_need_ctx_restore())
- dispc_restore_context();
+ dispc_restore_context();
}
mutex_unlock(&dispc.runtime_lock);
@@ -3682,8 +3663,6 @@ static int omap_dispchw_probe(struct platform_device *pdev)
goto err_irq;
}
- dispc_init_ctx_loss_count();
-
mutex_init(&dispc.runtime_lock);
pm_runtime_enable(&pdev->dev);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 27/27] OMAP: DSS2: DSS: Fix context save/restore
From: Tomi Valkeinen @ 2011-06-03 10:00 UTC (permalink / raw)
To: linux-omap, linux-fbdev; +Cc: b-cousson, paul, khilman, Tomi Valkeinen
In-Reply-To: <1307095237-14805-1-git-send-email-tomi.valkeinen@ti.com>
The current method of saving and restoring the context could cause a
restore before saving, effectively "restoring" zero values to registers.
Add ctx_valid field to indicate if the saved context is valid and can be
restored.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dss.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 91572b6..964eb08 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -77,6 +77,7 @@ static struct {
enum omap_dss_clk_source dispc_clk_source;
enum omap_dss_clk_source lcd_clk_source[MAX_DSS_LCD_MANAGERS];
+ bool ctx_valid;
u32 ctx[DSS_SZ_REGS / sizeof(u32)];
} dss;
@@ -112,12 +113,19 @@ static void dss_save_context(void)
SR(SDI_CONTROL);
SR(PLL_CONTROL);
}
+
+ dss.ctx_valid = true;
+
+ DSSDBG("context saved\n");
}
static void dss_restore_context(void)
{
DSSDBG("dss_restore_context\n");
+ if (!dss.ctx_valid)
+ return;
+
RR(CONTROL);
if (dss_feat_get_supported_displays(OMAP_DSS_CHANNEL_LCD) &
@@ -125,6 +133,8 @@ static void dss_restore_context(void)
RR(SDI_CONTROL);
RR(PLL_CONTROL);
}
+
+ DSSDBG("context restored\n");
}
#undef SR
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 01/27] OMAP: change get_context_loss_count ret value to int
From: Kevin Hilman @ 2011-06-03 16:32 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, b-cousson, paul
In-Reply-To: <1307095237-14805-2-git-send-email-tomi.valkeinen@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> get_context_loss_count functions return context loss count as u32, and
> zero means an error. However, zero is also returned when context has
> never been lost and could also be returned when the context loss count
> has wrapped and goes to zero.
>
> Change the functions to return an int, with negative value meaning an
> error.
>
> OMAP HSMMC code uses omap_pm_get_dev_context_loss_count(), but as the
> hsmmc code handles the returned value as an int, with negative value
> meaning an error, this patch actually fixes hsmmc code also.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Acked-by: Kevin Hilman <khilman@ti.com>
This one should be separated out into a fix for v3.0-rc, and queued by
Paul (also Cc'd to linux-arm-kernel.)
Kevin
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Kevin Hilman @ 2011-06-03 16:45 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, b-cousson, paul
In-Reply-To: <1307095237-14805-20-git-send-email-tomi.valkeinen@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> Use PM runtime and HWMOD support to handle enabling and disabling of DSS
> modules.
>
> Each DSS module will have get and put functions which can be used to
> enable and disable that module. The functions use pm_runtime and hwmod
> opt-clocks to enable the hardware.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
[...]
> +int dispc_runtime_get(void)
> +{
> + int r;
> +
> + mutex_lock(&dispc.runtime_lock);
It's not clear to me what the lock is trying to protect. I guess it's
the counter? I don't think it should be needed...
> + if (dispc.runtime_count++ = 0) {
You shouldn't need your own use-counting here. The runtime PM core is
already doing usage counting.
Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
core when the usage count goes to/from zero.
IOW, this function should simply be a pm_runtime_get_sync(), and the
rest of the stuff in this function should be done in the
->runtime_resume() callback, which is only executed when the usage_count
transitions from zero.
> + DSSDBG("dispc_runtime_get\n");
> +
> + r = dss_runtime_get();
> + if (r)
> + goto err_dss_get;
> +
> + /* XXX dispc fclk can also come from DSI PLL */
> + clk_enable(dispc.dss_clk);
> +
> + r = pm_runtime_get_sync(&dispc.pdev->dev);
> + WARN_ON(r);
> + if (r < 0)
> + goto err_runtime_get;
> +
> + if (dispc_need_ctx_restore())
> + dispc_restore_context();
> + }
> +
> + mutex_unlock(&dispc.runtime_lock);
> +
> + return 0;
> +
> +err_runtime_get:
> + clk_disable(dispc.dss_clk);
> + dss_runtime_put();
> +err_dss_get:
> + mutex_unlock(&dispc.runtime_lock);
> +
> + return r;
> }
>
> +void dispc_runtime_put(void)
> +{
> + mutex_lock(&dispc.runtime_lock);
> +
> + if (--dispc.runtime_count = 0) {
> + int r;
Same problem here.
No usage counting is required (and probably no locking either.) This
function should simply be a pm_runtime_put(), and the rest of the stuff
should be in the driver's ->runtime_suspend() callback.
> + DSSDBG("dispc_runtime_put\n");
> +
> + dispc_save_context();
> +
> + r = pm_runtime_put_sync(&dispc.pdev->dev);
Does this need to be the _sync() version? It looks like it could be use
the "normal" (async) version.: pm_runtime_put().
> + WARN_ON(r);
> +
> + clk_disable(dispc.dss_clk);
> +
> + dss_runtime_put();
> + }
> +
> + mutex_unlock(&dispc.runtime_lock);
> +}
> +
> +
> bool dispc_go_busy(enum omap_channel channel)
> {
> int bit;
> @@ -530,7 +645,7 @@ void dispc_go(enum omap_channel channel)
> int bit;
> bool enable_bit, go_bit;
>
> - enable_clocks(1);
> + dispc_runtime_get();
Based on the above suggestions, you probably don't need dedicated
functions for this. Just call pm_runtime_get_sync() here...
> if (channel = OMAP_DSS_CHANNEL_LCD ||
> channel = OMAP_DSS_CHANNEL_LCD2)
> @@ -571,7 +686,7 @@ void dispc_go(enum omap_channel channel)
> else
> REG_FLD_MOD(DISPC_CONTROL, 1, bit, bit);
> end:
> - enable_clocks(0);
> + dispc_runtime_put();
...and the pm_runtime_put() here.
Kevin
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-03 17:43 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, linux-fbdev, b-cousson, paul
In-Reply-To: <871uzahnib.fsf@ti.com>
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>
> > Use PM runtime and HWMOD support to handle enabling and disabling of DSS
> > modules.
> >
> > Each DSS module will have get and put functions which can be used to
> > enable and disable that module. The functions use pm_runtime and hwmod
> > opt-clocks to enable the hardware.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
> [...]
>
> > +int dispc_runtime_get(void)
> > +{
> > + int r;
> > +
> > + mutex_lock(&dispc.runtime_lock);
>
> It's not clear to me what the lock is trying to protect. I guess it's
> the counter? I don't think it should be needed...
Yes, the counter. I don't think
if (dispc.runtime_count++ = 0)
is thread safe.
> > + if (dispc.runtime_count++ = 0) {
>
> You shouldn't need your own use-counting here. The runtime PM core is
> already doing usage counting.
>
> Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
> core when the usage count goes to/from zero.
Yes, I wish I could do that =).
I tried to explain this in the 00-patch, I guess I should've explained
it in this patch also. Perhaps also in a comment.
From the introduction:
---
Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
problem is that on OMAP4 we have to enable an optional clock before calling
pm_runtime_get(), and similarly we need to keep the optional clock enabled
until after pm_runtime_put() has been called.
This causes some extra complications. For example, we can't use runtime_resume
callback to enable the opt clocks, as they are required before calling
pm_runtime_get().
---
So, the opt clock handling required by the driver pretty much messes the
whole idea of pm_runtime callbacks here. We can't do pretty much
anything in the suspend and resume callbacks.
We can't do save_context() in the suspend callback, because suspend
could be called later, after pm_runtime_put_sync() and
dispc_runtime_put() has returned. And and that point we've turned off
the opt clock and can't touch the registers. If we'd move the opt-clock
clk_disable to suspend, but clk_enable in dispc_runtime_get(), we'd get
mismatched clk_disable/enable, so we can't do that. etc.
I didn't come up with any other solutions to this. In the end, the
dispc_runtime_get/put are quite clean functions (well, says me =), and
their usage is more or less equivalent to pm_runtime_get/put. So I don't
see it as a horrible solution.
If and when the hwmod/pm_runtime/something handles this opt-clock issue,
it shouldn't be too big of a job to use pm_runtime properly in the
omapdss. Of course, if you or somebody else has a solution for this now,
I'm more than happy to use it =).
And this opt-clock problem pretty much covers all your comments below,
except one which I've answered also.
> IOW, this function should simply be a pm_runtime_get_sync(), and the
> rest of the stuff in this function should be done in the
> ->runtime_resume() callback, which is only executed when the usage_count
> transitions from zero.
>
> > + DSSDBG("dispc_runtime_get\n");
> > +
> > + r = dss_runtime_get();
> > + if (r)
> > + goto err_dss_get;
> > +
> > + /* XXX dispc fclk can also come from DSI PLL */
> > + clk_enable(dispc.dss_clk);
> > +
> > + r = pm_runtime_get_sync(&dispc.pdev->dev);
> > + WARN_ON(r);
> > + if (r < 0)
> > + goto err_runtime_get;
> > +
> > + if (dispc_need_ctx_restore())
> > + dispc_restore_context();
> > + }
> > +
> > + mutex_unlock(&dispc.runtime_lock);
> > +
> > + return 0;
> > +
> > +err_runtime_get:
> > + clk_disable(dispc.dss_clk);
> > + dss_runtime_put();
> > +err_dss_get:
> > + mutex_unlock(&dispc.runtime_lock);
> > +
> > + return r;
> > }
> >
> > +void dispc_runtime_put(void)
> > +{
> > + mutex_lock(&dispc.runtime_lock);
> > +
> > + if (--dispc.runtime_count = 0) {
> > + int r;
>
> Same problem here.
>
> No usage counting is required (and probably no locking either.) This
> function should simply be a pm_runtime_put(), and the rest of the stuff
> should be in the driver's ->runtime_suspend() callback.
>
> > + DSSDBG("dispc_runtime_put\n");
> > +
> > + dispc_save_context();
> > +
> > + r = pm_runtime_put_sync(&dispc.pdev->dev);
>
> Does this need to be the _sync() version? It looks like it could be use
> the "normal" (async) version.: pm_runtime_put().
It's sync because we turn off the opt clock below, and the HW shouldn't
be touched after that, which I guess pm_runtime_put (or the subsequent
async suspend) could do.
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Kevin Hilman @ 2011-06-03 22:53 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, b-cousson, paul
In-Reply-To: <1307122985.2016.72.camel@deskari>
Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> Hi Kevin,
>
> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
>> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>>
>> > Use PM runtime and HWMOD support to handle enabling and disabling of DSS
>> > modules.
>> >
>> > Each DSS module will have get and put functions which can be used to
>> > enable and disable that module. The functions use pm_runtime and hwmod
>> > opt-clocks to enable the hardware.
>> >
>> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> [...]
>>
>> > +int dispc_runtime_get(void)
>> > +{
>> > + int r;
>> > +
>> > + mutex_lock(&dispc.runtime_lock);
>>
>> It's not clear to me what the lock is trying to protect. I guess it's
>> the counter? I don't think it should be needed...
>
> Yes, the counter. I don't think
>
> if (dispc.runtime_count++ = 0)
>
> is thread safe.
OK, if it's just the counter, you can drop the mutex and use an atomic
variable and use atomic_inc(), atomic_dec() etc. Then it will be clear
from reading what exactly is protected.
>> > + if (dispc.runtime_count++ = 0) {
>>
>> You shouldn't need your own use-counting here. The runtime PM core is
>> already doing usage counting.
>>
>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
>> core when the usage count goes to/from zero.
>
> Yes, I wish I could do that =).
>
> I tried to explain this in the 00-patch, I guess I should've explained
> it in this patch also. Perhaps also in a comment.
Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant
about DSS, so I was focused in on the runtime PM implementation only.
Sorry about that.
> From the introduction:
>
> ---
>
> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
> problem is that on OMAP4 we have to enable an optional clock before calling
> pm_runtime_get(), and similarly we need to keep the optional clock enabled
> until after pm_runtime_put() has been called.
Just to clarify, what exactly does the opt clock have to be enabled for?
From what I can gather, this particular opt clock has to be enabled for
the hwmod itself to be enabled (or disabled), correct?
This has been a known issue for reset[1], but I wasn't aware that it was
needed for a normal (re)enable of the hwmod.
> This causes some extra complications. For example, we can't use runtime_resume
> callback to enable the opt clocks, as they are required before calling
> pm_runtime_get().
>
Yuck.
> ---
>
> So, the opt clock handling required by the driver pretty much messes the
> whole idea of pm_runtime callbacks here. We can't do pretty much
> anything in the suspend and resume callbacks.
>
> We can't do save_context() in the suspend callback, because suspend
> could be called later, after pm_runtime_put_sync() and
> dispc_runtime_put() has returned. And and that point we've turned off
> the opt clock and can't touch the registers. If we'd move the opt-clock
> clk_disable to suspend, but clk_enable in dispc_runtime_get(), we'd get
> mismatched clk_disable/enable, so we can't do that. etc.
Double yuck.
It's clear now that you at least wanted to do the "right" thing and
thought about the variousways to do it, but weren't able to for various
reasons. Thanks!
> I didn't come up with any other solutions to this. In the end, the
> dispc_runtime_get/put are quite clean functions (well, says me =), and
> their usage is more or less equivalent to pm_runtime_get/put. So I don't
> see it as a horrible solution.
I agree, it's not horrible. Just induces very mild nausea. ;)
> If and when the hwmod/pm_runtime/something handles this opt-clock issue,
> it shouldn't be too big of a job to use pm_runtime properly in the
> omapdss.
Agreed. I certainly won't hold up this patch unless we come up with an
alternate solution very quickly (which I will try below, and wait for
Paul/Benoit to correct me.)
> Of course, if you or somebody else has a solution for this now, I'm
> more than happy to use it =).
I don't have a solid solution, but so far, this sounds like an IP
requirement that should be managed at the hwmod level.
One approach would be to have an option to selectively manage optional
clocks in the hwmod enable/idle path. We're doing it for reset[1], but
not for anything else. And based on the changelog there[1], it doesn't
even sound like we fully understand the exact requirements around reset.
From the contortions you've had to go through though, it sounds like the
same optional clocks that are needed for reset are needed for a "normal"
module enable/disable.
I tried a simple hack to do that below[2]. Can you see if that works
for you and if you can remove the opt clk mgmt from your driver(s)? I
only boot tested it on 3430/n900 which only has this opt-clk flag set
for the GPIO IP blocks.
Another idea off the top of my head is to extend runtime PM to have a
couple additional callbacks. Something like ->runtime_resume_prepare()
which would be called before the HW is enabled (and thus before
->runtime_resume()), and similarily ->runtime_suspend_complete() which
would be called after ->runtime_suspend() and after the HW has been
disabled.
I don't really like this approach because so far this is the only driver
that has needed something like this. And in the case of this IP the
enable/disable of the optional clocks seems like a HW requirement for
the correct enable/disable of the IP, so it seems like something that
should be managed by the hwmod.
Now I'll wait for Benoit/Paul to enlighten us.
> And this opt-clock problem pretty much covers all your comments below,
> except one which I've answered also.
>
>> IOW, this function should simply be a pm_runtime_get_sync(), and the
>> rest of the stuff in this function should be done in the
>> ->runtime_resume() callback, which is only executed when the usage_count
>> transitions from zero.
>>
>> > + DSSDBG("dispc_runtime_get\n");
>> > +
>> > + r = dss_runtime_get();
>> > + if (r)
>> > + goto err_dss_get;
>> > +
>> > + /* XXX dispc fclk can also come from DSI PLL */
>> > + clk_enable(dispc.dss_clk);
>> > +
>> > + r = pm_runtime_get_sync(&dispc.pdev->dev);
>> > + WARN_ON(r);
>> > + if (r < 0)
>> > + goto err_runtime_get;
>> > +
>> > + if (dispc_need_ctx_restore())
>> > + dispc_restore_context();
>> > + }
>> > +
>> > + mutex_unlock(&dispc.runtime_lock);
>> > +
>> > + return 0;
>> > +
>> > +err_runtime_get:
>> > + clk_disable(dispc.dss_clk);
>> > + dss_runtime_put();
>> > +err_dss_get:
>> > + mutex_unlock(&dispc.runtime_lock);
>> > +
>> > + return r;
>> > }
>> >
>> > +void dispc_runtime_put(void)
>> > +{
>> > + mutex_lock(&dispc.runtime_lock);
>> > +
>> > + if (--dispc.runtime_count = 0) {
>> > + int r;
>>
>> Same problem here.
>>
>> No usage counting is required (and probably no locking either.) This
>> function should simply be a pm_runtime_put(), and the rest of the stuff
>> should be in the driver's ->runtime_suspend() callback.
>>
>> > + DSSDBG("dispc_runtime_put\n");
>> > +
>> > + dispc_save_context();
>> > +
>> > + r = pm_runtime_put_sync(&dispc.pdev->dev);
>>
>> Does this need to be the _sync() version? It looks like it could be use
>> the "normal" (async) version.: pm_runtime_put().
>
> It's sync because we turn off the opt clock below, and the HW shouldn't
> be touched after that, which I guess pm_runtime_put (or the subsequent
> async suspend) could do.
OK, I see now.
Kevin
[1]
commit 96835af970e5d6aeedf868e53590a947be5e4a7a
Author: Benoit Cousson <b-cousson@ti.com>
Date: Tue Sep 21 18:57:58 2010 +0200
OMAP: hwmod: Fix softreset for modules with optional clocks
Some modules (like GPIO, DSS...) require optionals clock to be enabled
in order to complete the sofreset properly.
Add a HWMOD_CONTROL_OPT_CLKS_IN_RESET flag to force all optional clocks
to be enabled before reset. Disabled them once the reset is done.
TODO:
For the moment it is very hard to understand from the HW spec, which
optional clock is needed and which one is not. So the current approach
will enable all the optional clocks.
Paul proposed a much finer approach that will allow to tag only the needed
clock in the optional clock table. This might be doable as soon as we have
a clear understanding of these dependencies.
Reported-by: Partha Basak <p-basak2@ti.com>
Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
[2]
From d2806a4148fbed869eff8703b1137cd35d16ef53 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Fri, 3 Jun 2011 15:33:25 -0700
Subject: [PATCH] OMAP2+: omap_hwmod: selectively manage optional clocks in enable/disable
Only-a-Test-Hack-and-Certainly-Not-Signed-off-by: Kevin Hilman <khilman@ti.com>
---
arch/arm/mach-omap2/omap_hwmod.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 293fa6c..7bcf802 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -586,6 +586,9 @@ static int _init_opt_clks(struct omap_hwmod *oh)
return ret;
}
+static void _enable_optional_clocks(struct omap_hwmod *oh);
+static void _disable_optional_clocks(struct omap_hwmod *oh);
+
/**
* _enable_clocks - enable hwmod main clock and interface clocks
* @oh: struct omap_hwmod *
@@ -599,6 +602,9 @@ static int _enable_clocks(struct omap_hwmod *oh)
pr_debug("omap_hwmod: %s: enabling clocks\n", oh->name);
+ if (oh->flags & HWMOD_CONTROL_OPT_CLKS_IN_RESET)
+ _enable_optional_clocks(oh);
+
if (oh->_clk)
clk_enable(oh->_clk);
@@ -642,6 +648,9 @@ static int _disable_clocks(struct omap_hwmod *oh)
}
}
+ if (oh->flags & HWMOD_CONTROL_OPT_CLKS_IN_RESET)
+ _disable_optional_clocks(oh);
+
/* The opt clocks are controlled by the device driver. */
return 0;
--
1.7.4
^ permalink raw reply related
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-04 8:01 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, linux-fbdev, b-cousson, paul
In-Reply-To: <87hb86a5mm.fsf@ti.com>
On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>
> > Hi Kevin,
> >
> > On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
> >> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> >>
> >> > Use PM runtime and HWMOD support to handle enabling and disabling of DSS
> >> > modules.
> >> >
> >> > Each DSS module will have get and put functions which can be used to
> >> > enable and disable that module. The functions use pm_runtime and hwmod
> >> > opt-clocks to enable the hardware.
> >> >
> >> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>
> >> [...]
> >>
> >> > +int dispc_runtime_get(void)
> >> > +{
> >> > + int r;
> >> > +
> >> > + mutex_lock(&dispc.runtime_lock);
> >>
> >> It's not clear to me what the lock is trying to protect. I guess it's
> >> the counter? I don't think it should be needed...
> >
> > Yes, the counter. I don't think
> >
> > if (dispc.runtime_count++ = 0)
> >
> > is thread safe.
>
> OK, if it's just the counter, you can drop the mutex and use an atomic
> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear
> from reading what exactly is protected.
Hmm, sorry, my mistake. It's actually for the whole function: we can't
do "put" before the whole "get" has finished. Otherwise we could end up,
for example, disabling a clock before enabling it.
> >> > + if (dispc.runtime_count++ = 0) {
> >>
> >> You shouldn't need your own use-counting here. The runtime PM core is
> >> already doing usage counting.
> >>
> >> Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
> >> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
> >> core when the usage count goes to/from zero.
> >
> > Yes, I wish I could do that =).
> >
> > I tried to explain this in the 00-patch, I guess I should've explained
> > it in this patch also. Perhaps also in a comment.
>
> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant
> about DSS, so I was focused in on the runtime PM implementation only.
> Sorry about that.
>
> > From the introduction:
> >
> > ---
> >
> > Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
> > problem is that on OMAP4 we have to enable an optional clock before calling
> > pm_runtime_get(), and similarly we need to keep the optional clock enabled
> > until after pm_runtime_put() has been called.
>
> Just to clarify, what exactly does the opt clock have to be enabled for?
I'm not sure if this is a valid definition, but in my mind the opt clock
has two uses: 1) a functional clock, to make the HW tick and registers
accessible, and 2) act as a source clock for the outgoing pixel clock.
Of these, the first one is the one that matters in this scope. So, it's
a mandatory clock, but it's optional in the sense that there are other
clocks that can be used in place of that clock.
All OMAPs support the DSS fclk from PRCM (this is the default). If I
remember right, OMAP2 also supports using sys clock as the DSS fclk.
OMAP3 and 4 support using DSI PLL (whose source clock is sys clock) as
the fclk.
> From what I can gather, this particular opt clock has to be enabled for
> the hwmod itself to be enabled (or disabled), correct?
Yes, the registers are unaccessible and the HW doesn't come out of reset
without the fclk.
> This has been a known issue for reset[1], but I wasn't aware that it was
> needed for a normal (re)enable of the hwmod.
>
> > This causes some extra complications. For example, we can't use runtime_resume
> > callback to enable the opt clocks, as they are required before calling
> > pm_runtime_get().
> >
>
> Yuck.
>
> > ---
> >
> > So, the opt clock handling required by the driver pretty much messes the
> > whole idea of pm_runtime callbacks here. We can't do pretty much
> > anything in the suspend and resume callbacks.
> >
> > We can't do save_context() in the suspend callback, because suspend
> > could be called later, after pm_runtime_put_sync() and
> > dispc_runtime_put() has returned. And and that point we've turned off
> > the opt clock and can't touch the registers. If we'd move the opt-clock
> > clk_disable to suspend, but clk_enable in dispc_runtime_get(), we'd get
> > mismatched clk_disable/enable, so we can't do that. etc.
>
> Double yuck.
>
> It's clear now that you at least wanted to do the "right" thing and
> thought about the variousways to do it, but weren't able to for various
> reasons. Thanks!
>
> > I didn't come up with any other solutions to this. In the end, the
> > dispc_runtime_get/put are quite clean functions (well, says me =), and
> > their usage is more or less equivalent to pm_runtime_get/put. So I don't
> > see it as a horrible solution.
>
> I agree, it's not horrible. Just induces very mild nausea. ;)
>
> > If and when the hwmod/pm_runtime/something handles this opt-clock issue,
> > it shouldn't be too big of a job to use pm_runtime properly in the
> > omapdss.
>
> Agreed. I certainly won't hold up this patch unless we come up with an
> alternate solution very quickly (which I will try below, and wait for
> Paul/Benoit to correct me.)
>
> > Of course, if you or somebody else has a solution for this now, I'm
> > more than happy to use it =).
>
> I don't have a solid solution, but so far, this sounds like an IP
> requirement that should be managed at the hwmod level.
I agree.
How I imagine things should work is that the hwmod code defaults to the
standard PRCM clock, but allows us to change the functional clock at a
later time, allowing the original clock to be disabled.
But I don't know how this would work in practice. The DSI PLL clock has
to be programmed via DSI HW, and the bits that control the clock muxes
are inside DSS. So the HWMOD code can't probably handle this by itself,
but needs the DSS to do certain things at certain times.
> One approach would be to have an option to selectively manage optional
> clocks in the hwmod enable/idle path. We're doing it for reset[1], but
> not for anything else. And based on the changelog there[1], it doesn't
> even sound like we fully understand the exact requirements around reset.
Yes, the OMAP4 DSS reset is a bit unclear for me.
> From the contortions you've had to go through though, it sounds like the
> same optional clocks that are needed for reset are needed for a "normal"
> module enable/disable.
>
> I tried a simple hack to do that below[2]. Can you see if that works
> for you and if you can remove the opt clk mgmt from your driver(s)? I
> only boot tested it on 3430/n900 which only has this opt-clk flag set
> for the GPIO IP blocks.
I didn't test it yet, but I would imagine it works. But it doesn't help
us towards properly using the other clocks as fclk.
However, it's not really any worse than the current DSS code. We don't
currently turn off the DSS fclk from the PRCM, even if we would use the
clock from the DSI PLL.
So perhaps an approach where hwmod would enable the fclk from the PRCM
automatically would be good. It's no worse than the other option, and
it'd give us the ability to use pm_runtime in the proper way.
In this (hopefully temporary) solution the clock wouldn't really be
optional clock, but mandatory.
> Another idea off the top of my head is to extend runtime PM to have a
> couple additional callbacks. Something like ->runtime_resume_prepare()
> which would be called before the HW is enabled (and thus before
> ->runtime_resume()), and similarily ->runtime_suspend_complete() which
> would be called after ->runtime_suspend() and after the HW has been
> disabled.
>
> I don't really like this approach because so far this is the only driver
> that has needed something like this. And in the case of this IP the
> enable/disable of the optional clocks seems like a HW requirement for
> the correct enable/disable of the IP, so it seems like something that
> should be managed by the hwmod.
Yes, I agree, that doesn't sound like a very good approach.
Tomi
^ permalink raw reply
* (no subject)
From: @ 2011-06-04 9:31 UTC (permalink / raw)
To: linux-fbdev
--
04-06-20011
Your Mail-ID has been awarded 750,000.00 GBP From The Coca-Cola Online
Bonanza 2011. For claims send
Name:
Address:
Phone No:
Age: Sex:
Occupation:
Country:
Contact: Mr. Jim Gardner
Claims department : jim-gardner.c@live.com
TEL: +44 755 284 8328
^ permalink raw reply
* Re: [Patch 1/2] Fix use-after-free by vga16fb on rmmod
From: Paul Mundt @ 2011-06-06 3:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20110524215917.4b01df45@neptune.home>
Hi Bruno,
On Thu, Jun 02, 2011 at 08:18:57PM +0200, Bruno Pr??mont wrote:
> On Tue, 24 May 2011 Bruno Pr??mont <bonbons@linux-vserver.org> wrote:
> > Since fb_info is now refcounted and thus may get freed at any time it
> > gets unregistered module unloading will try to unregister framebuffer
> > as stored in platform data on probe though this pointer may
> > be stale.
> >
> > Cleanup platform data on framebuffer release.
> >
> > CC: stable@kernel.org
> > Signed-off-by: Bruno Pr??mont <bonbons@linux-vserver.org>
> > ---
> > This should also go into 2.6.39 stable as it didn't make it into 2.6.39
> > with the rest of fb_info refcounting work.
> >
> > This comes from
> > [2.6.39-rc2, framebuffer] use after free oops
> > ...
> > [PATCH 0/2] fbcon sanity
> > thread
>
> Any chance of applying these two patches?
>
> I've had no feedback from you on them and they don't show up in your tree.
>
Patchwork has been a bit spotty lately with some patches showing up and
others not, so I've invariably missed a few. I've applied the first one
now, and will address the second one separately.
^ permalink raw reply
* Re: [Patch 2/2 resend] Prevent vga16fb from accessing hw after it was unregistered
From: Paul Mundt @ 2011-06-06 3:07 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20110524223221.741ffbc0@neptune.home>
On Tue, May 24, 2011 at 10:32:21PM +0200, Bruno Pr??mont wrote:
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..bd9f93b 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1661,6 +1661,11 @@ static int do_unregister_framebuffer(struct fb_info *fb_info)
> device_destroy(fb_class, MKDEV(FB_MAJOR, i));
> event.info = fb_info;
> fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
> + if (fb_info->fbops->fb_unregistered) {
> + mutex_lock(&fb_info->lock);
> + fb_info->fbops->fb_unregistered(fb_info);
> + mutex_unlock(&fb_info->lock);
> + }
>
> /* this may free fb info */
> put_fb_info(fb_info);
I'm not sure I really see the point, given that you can already do all of
the same work by tying in to the notifier chain. See for example the
sh_mobile_hdmi driver and its unreg notifier.
^ permalink raw reply
* [GIT PULL] fbdev fixes for 3.0-rc2
From: Paul Mundt @ 2011-06-06 3:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fbdev, linux-kernel
Please pull from:
master.kernel.org:/pub/scm/linux/kernel/git/lethal/fbdev-3.x.git fbdev-fixes-for-linus
Which contains:
Andy Lutomirski (3):
efifb: Enable write-combining
efifb: Fix mismatched request/release_mem_region
efifb: Disallow manual bind and unbind
Bruno Prémont (1):
video: Fix use-after-free by vga16fb on rmmod
Joe Perches (1):
video: Convert vmalloc/memset to vzalloc
Julia Lawall (2):
drivers/video/imxfb.c: add missing clk_put
drivers/video/pxa168fb.c: add missing clk_put
Paul Mundt (1):
fbdev: sh_mobile_lcdcfb: Fix up fallout from MERAM changes.
Steven Miao (1):
fbdev: bf537-lq035: add missing blacklight properties type
Tormod Volden (1):
savagefb: Use panel CVT mode as default
drivers/video/arcfb.c | 5 +--
drivers/video/bf537-lq035.c | 1 +
drivers/video/broadsheetfb.c | 4 +--
drivers/video/efifb.c | 34 ++++++++++++++++++++-----------
drivers/video/hecubafb.c | 5 +--
drivers/video/imxfb.c | 4 +-
drivers/video/metronomefb.c | 4 +--
drivers/video/modedb.c | 1 +
drivers/video/pxa168fb.c | 17 +++++++++------
drivers/video/savage/savagefb_driver.c | 16 +++++++++++++++
drivers/video/sh_mobile_lcdcfb.c | 4 +-
drivers/video/vga16fb.c | 2 +
drivers/video/xen-fbfront.c | 3 +-
13 files changed, 63 insertions(+), 37 deletions(-)
^ permalink raw reply
* Re: [PATCH 03/27] OMAP: DSS2: Reset LANEx_ULPS_SIG2 bits after use
From: Archit Taneja @ 2011-06-06 5:53 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: linux-omap@vger.kernel.org, linux-fbdev@vger.kernel.org,
Cousson, Benoit, paul@pwsan.com, Hilman, Kevin
In-Reply-To: <1307095237-14805-4-git-send-email-tomi.valkeinen@ti.com>
Hi,
On Friday 03 June 2011 03:30 PM, Valkeinen, Tomi wrote:
> LANEx_ULPS_SIG2 bits are left on after entering ULPS. This doesn't cause
> any problems currently, as DSI HW is reset when it is enabled. However,
> if the reset is not done, operation fails if the bits are still set.
>
> So reset the bits after entering ULPS to ensure operation even without
> HW reset.
>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/dsi.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 69c2d4f..4496d09 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -3395,6 +3395,10 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
> dsi_unregister_isr_cio(dsidev, dsi_completion_handler,&completion,
> DSI_CIO_IRQ_ULPSACTIVENOT_ALL0);
>
> + /* Reset LANEx_ULPS_SIG2 */
> + REG_FLD_MOD(dsidev, DSI_COMPLEXIO_CFG2, (0<< 0) | (0<< 1) | (0<< 2),
> + 7, 5);
> +
We may need to reset more lanes based on the number of lanes the panel
is using. We could calculate a mask here instead.
Archit
> dsi_cio_power(dsidev, DSI_COMPLEXIO_POWER_ULPS);
>
> dsi_if_enable(dsidev, false);
^ permalink raw reply
* Re: [PATCH 03/27] OMAP: DSS2: Reset LANEx_ULPS_SIG2 bits after use
From: Tomi Valkeinen @ 2011-06-06 7:21 UTC (permalink / raw)
To: Archit Taneja
Cc: linux-omap@vger.kernel.org, linux-fbdev@vger.kernel.org,
Cousson, Benoit, paul@pwsan.com, Hilman, Kevin
In-Reply-To: <4DEC6889.6020607@ti.com>
On Mon, 2011-06-06 at 11:11 +0530, Archit Taneja wrote:
> Hi,
>
> On Friday 03 June 2011 03:30 PM, Valkeinen, Tomi wrote:
> > LANEx_ULPS_SIG2 bits are left on after entering ULPS. This doesn't cause
> > any problems currently, as DSI HW is reset when it is enabled. However,
> > if the reset is not done, operation fails if the bits are still set.
> >
> > So reset the bits after entering ULPS to ensure operation even without
> > HW reset.
> >
> > Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> > ---
> > drivers/video/omap2/dss/dsi.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> > index 69c2d4f..4496d09 100644
> > --- a/drivers/video/omap2/dss/dsi.c
> > +++ b/drivers/video/omap2/dss/dsi.c
> > @@ -3395,6 +3395,10 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
> > dsi_unregister_isr_cio(dsidev, dsi_completion_handler,&completion,
> > DSI_CIO_IRQ_ULPSACTIVENOT_ALL0);
> >
> > + /* Reset LANEx_ULPS_SIG2 */
> > + REG_FLD_MOD(dsidev, DSI_COMPLEXIO_CFG2, (0<< 0) | (0<< 1) | (0<< 2),
> > + 7, 5);
> > +
>
> We may need to reset more lanes based on the number of lanes the panel
> is using. We could calculate a mask here instead.
Yes, I noticed that but I decided just to fix the bug here. The same
bits are set a few lines earlier.
We should go through all the lane configs in dsi.c, and come up with a
way to easily get the necessary masks.
Tomi
^ permalink raw reply
* Re: [PATCH 01/27] OMAP: change get_context_loss_count ret value to
From: Tomi Valkeinen @ 2011-06-06 7:28 UTC (permalink / raw)
To: Kevin Hilman, paul; +Cc: linux-omap, linux-fbdev, b-cousson
In-Reply-To: <871uzaj2oy.fsf@ti.com>
On Fri, 2011-06-03 at 09:32 -0700, Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>
> > get_context_loss_count functions return context loss count as u32, and
> > zero means an error. However, zero is also returned when context has
> > never been lost and could also be returned when the context loss count
> > has wrapped and goes to zero.
> >
> > Change the functions to return an int, with negative value meaning an
> > error.
> >
> > OMAP HSMMC code uses omap_pm_get_dev_context_loss_count(), but as the
> > hsmmc code handles the returned value as an int, with negative value
> > meaning an error, this patch actually fixes hsmmc code also.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Acked-by: Kevin Hilman <khilman@ti.com>
>
> This one should be separated out into a fix for v3.0-rc, and queued by
> Paul (also Cc'd to linux-arm-kernel.)
Yup, I added it here for completeness to get a working patch series. The
same patch has also been sent separately.
Tomi
^ permalink raw reply
* [PATCH] atmel_lcdfb: fix usage of wrong registers in suspend/resume
From: Hubert Feurstein @ 2011-06-06 8:50 UTC (permalink / raw)
To: linux-fbdev
I assume the intention was to set the contrast value to 0 and not
the contrast control register (in atmel_lcdfb_suspend). And in
atmel_lcdfb_resume the contrast value should be restored.
Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
drivers/video/atmel_lcdfb.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 4484c72..2ed7ec1 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -1086,7 +1086,7 @@ static int atmel_lcdfb_suspend(struct platform_device *pdev, pm_message_t mesg)
lcdc_writel(sinfo, ATMEL_LCDC_IDR, ~0UL);
sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
- lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);
+ lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, 0);
if (sinfo->atmel_lcdfb_power_control)
sinfo->atmel_lcdfb_power_control(0);
@@ -1105,7 +1105,7 @@ static int atmel_lcdfb_resume(struct platform_device *pdev)
atmel_lcdfb_start(sinfo);
if (sinfo->atmel_lcdfb_power_control)
sinfo->atmel_lcdfb_power_control(1);
- lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);
+ lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, sinfo->saved_lcdcon);
/* Enable FIFO & DMA errors */
lcdc_writel(sinfo, ATMEL_LCDC_IER, ATMEL_LCDC_UFLWI
--
1.7.1
^ permalink raw reply related
* Re: [GIT PULL] fbdev fixes for 3.0-rc2
From: Linus Torvalds @ 2011-06-06 9:04 UTC (permalink / raw)
To: Paul Mundt; +Cc: linux-fbdev, linux-kernel
In-Reply-To: <20110606033900.GF26101@linux-sh.org>
On Mon, Jun 6, 2011 at 12:39 PM, Paul Mundt <lethal@linux-sh.org> wrote:
>
> Bruno Prémont (1):
> video: Fix use-after-free by vga16fb on rmmod
Do you even *look* at what you ask me to pull from you?
Spend a bit of effort in not sending obviously bogus and corrupted
authorship patches. That one has Bruno's name correctly in the
sign-off, but you've done something horrible to it in the authorship.
Linus
^ permalink raw reply
* Re: [GIT PULL] fbdev fixes for 3.0-rc2
From: Paul Mundt @ 2011-06-06 9:15 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fbdev, linux-kernel
In-Reply-To: <BANLkTik1nLG6f27JkR0kBvcJEoqUrq602A@mail.gmail.com>
On Mon, Jun 06, 2011 at 06:04:58PM +0900, Linus Torvalds wrote:
> On Mon, Jun 6, 2011 at 12:39 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> >
> > Bruno Pr??????????????????mont (1):
> > ?? ?? ??video: Fix use-after-free by vga16fb on rmmod
>
> Do you even *look* at what you ask me to pull from you?
>
> Spend a bit of effort in not sending obviously bogus and corrupted
> authorship patches. That one has Bruno's name correctly in the
> sign-off, but you've done something horrible to it in the authorship.
>
That's how I got it from patchwork, so it looks like it was mangled
already on the way in. I assumed it was just my console that was screwing
it up, but it looks to be in a similar state in the list archives, too.
In any event, I've fixed it up and pushed out a cleaned up version, sorry
for the noise.
^ permalink raw reply
* Re: [GIT PULL] fbdev fixes for 3.0-rc2
From: Bruno Prémont @ 2011-06-06 9:39 UTC (permalink / raw)
To: Paul Mundt; +Cc: Linus Torvalds, linux-fbdev, linux-kernel
In-Reply-To: <20110606091556.GG26101@linux-sh.org>
On Mon, 6 Jun 2011 18:15:57 Paul Mundt <lethal@linux-sh.org> wrote:
> On Mon, Jun 06, 2011 at 06:04:58PM +0900, Linus Torvalds wrote:
> > On Mon, Jun 6, 2011 at 12:39 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > >
> > > Bruno Pr??????????????????mont (1):
> > > ?? ?? ??video: Fix use-after-free by vga16fb on rmmod
> >
> > Do you even *look* at what you ask me to pull from you?
> >
> > Spend a bit of effort in not sending obviously bogus and corrupted
> > authorship patches. That one has Bruno's name correctly in the
> > sign-off, but you've done something horrible to it in the authorship.
> >
> That's how I got it from patchwork, so it looks like it was mangled
> already on the way in. I assumed it was just my console that was screwing
> it up, but it looks to be in a similar state in the list archives, too.
Hm, patchwork's mail headers look correct but its post-decoding display
is mangled (hard to guess what multiple transcodings it did!). (your
mail-client also dislikes non-ascii characters)
From archives, marc.info seems not to handle charsets at all - it dumps
the bytes and flags them as iso-8859-1 (don't know what other archives
you were looking at, marc.info being the only one listed at vger)
The only not so usual thing I see in my From header is that just the
lastname is being encoded and base64 is being used (while
quoted-printable is more common).
Bruno
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-06 12:56 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307174504.1777.24.camel@lappyti>
Hi Tomi,
On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote:
> On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote:
>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes:
>>
>>> Hi Kevin,
>>>
>>> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
>>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes:
>>>>
>>>>> Use PM runtime and HWMOD support to handle enabling and disabling of DSS
>>>>> modules.
>>>>>
>>>>> Each DSS module will have get and put functions which can be used to
>>>>> enable and disable that module. The functions use pm_runtime and hwmod
>>>>> opt-clocks to enable the hardware.
>>>>>
>>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
>>>>
>>>> [...]
>>>>
>>>>> +int dispc_runtime_get(void)
>>>>> +{
>>>>> + int r;
>>>>> +
>>>>> + mutex_lock(&dispc.runtime_lock);
>>>>
>>>> It's not clear to me what the lock is trying to protect. I guess it's
>>>> the counter? I don't think it should be needed...
>>>
>>> Yes, the counter. I don't think
>>>
>>> if (dispc.runtime_count++ = 0)
>>>
>>> is thread safe.
>>
>> OK, if it's just the counter, you can drop the mutex and use an atomic
>> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear
>> from reading what exactly is protected.
>
> Hmm, sorry, my mistake. It's actually for the whole function: we can't
> do "put" before the whole "get" has finished. Otherwise we could end up,
> for example, disabling a clock before enabling it.
>
>>>>> + if (dispc.runtime_count++ = 0) {
>>>>
>>>> You shouldn't need your own use-counting here. The runtime PM core is
>>>> already doing usage counting.
>>>>
>>>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
>>>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
>>>> core when the usage count goes to/from zero.
>>>
>>> Yes, I wish I could do that =).
>>>
>>> I tried to explain this in the 00-patch, I guess I should've explained
>>> it in this patch also. Perhaps also in a comment.
>>
>> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant
>> about DSS, so I was focused in on the runtime PM implementation only.
>> Sorry about that.
>>
>>> From the introduction:
>>>
>>> ---
>>>
>>> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
>>> problem is that on OMAP4 we have to enable an optional clock before calling
>>> pm_runtime_get(), and similarly we need to keep the optional clock enabled
>>> until after pm_runtime_put() has been called.
>>
>> Just to clarify, what exactly does the opt clock have to be enabled for?
>
> I'm not sure if this is a valid definition, but in my mind the opt clock
> has two uses: 1) a functional clock, to make the HW tick and registers
> accessible, and 2) act as a source clock for the outgoing pixel clock.
That terminology in the PRCM just means that an opt clock will not be
handled automatically by the PRCM and will require SW control.
This is not the case for mandatory clock. Upon module enable the PRCM
will ensure that all mandatory clocks (functional and interface) are
enabled automagically. If the clock is marked as optional it means that
the SW will have to enable it explicitly before enabling the module.
The modulemode was not there previously on OMAP2 & 3, but it is more or
less equivalent to icken=1 + fcken=1.
This idea was to hide the explicit clock management especially for the
iclk that were already supposed to always be in autoidle.
Since the current hwmod + clock fmwks are still based on the previous
clock centric approach we used to have on OMAP2 & 3, we cannot match
properly the modulemode to any clock and thus cannot handle properly the
DSS fclk as the main clock instead of the optional clock.
A temporary option will be to consider the modulemode as the interface
clock and thus remove it from the main_clk and replace it by the real
DSS fclk.
It should work be will unfortunately not be compliant with PRCM
recommendation to enable the modulemode once every clocks are enabled.
The long term solution is to update the hwmod fmwk to handle the
modulemode directly and not through the clock fmwk. It will allow the
main_clk to be connnected to the dss_fclk.
You will not have that nasty opt_clock issue anymore.
Regards,
Benoit
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-06 13:01 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DECCE90.6070201@ti.com>
On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
> Hi Tomi,
>
> On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote:
> > On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote:
> >> Tomi Valkeinen<tomi.valkeinen@ti.com> writes:
> >>
> >>> Hi Kevin,
> >>>
> >>> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
> >>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes:
> >>>>
> >>>>> Use PM runtime and HWMOD support to handle enabling and disabling of DSS
> >>>>> modules.
> >>>>>
> >>>>> Each DSS module will have get and put functions which can be used to
> >>>>> enable and disable that module. The functions use pm_runtime and hwmod
> >>>>> opt-clocks to enable the hardware.
> >>>>>
> >>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> >>>>
> >>>> [...]
> >>>>
> >>>>> +int dispc_runtime_get(void)
> >>>>> +{
> >>>>> + int r;
> >>>>> +
> >>>>> + mutex_lock(&dispc.runtime_lock);
> >>>>
> >>>> It's not clear to me what the lock is trying to protect. I guess it's
> >>>> the counter? I don't think it should be needed...
> >>>
> >>> Yes, the counter. I don't think
> >>>
> >>> if (dispc.runtime_count++ = 0)
> >>>
> >>> is thread safe.
> >>
> >> OK, if it's just the counter, you can drop the mutex and use an atomic
> >> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear
> >> from reading what exactly is protected.
> >
> > Hmm, sorry, my mistake. It's actually for the whole function: we can't
> > do "put" before the whole "get" has finished. Otherwise we could end up,
> > for example, disabling a clock before enabling it.
> >
> >>>>> + if (dispc.runtime_count++ = 0) {
> >>>>
> >>>> You shouldn't need your own use-counting here. The runtime PM core is
> >>>> already doing usage counting.
> >>>>
> >>>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
> >>>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
> >>>> core when the usage count goes to/from zero.
> >>>
> >>> Yes, I wish I could do that =).
> >>>
> >>> I tried to explain this in the 00-patch, I guess I should've explained
> >>> it in this patch also. Perhaps also in a comment.
> >>
> >> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant
> >> about DSS, so I was focused in on the runtime PM implementation only.
> >> Sorry about that.
> >>
> >>> From the introduction:
> >>>
> >>> ---
> >>>
> >>> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
> >>> problem is that on OMAP4 we have to enable an optional clock before calling
> >>> pm_runtime_get(), and similarly we need to keep the optional clock enabled
> >>> until after pm_runtime_put() has been called.
> >>
> >> Just to clarify, what exactly does the opt clock have to be enabled for?
> >
> > I'm not sure if this is a valid definition, but in my mind the opt clock
> > has two uses: 1) a functional clock, to make the HW tick and registers
> > accessible, and 2) act as a source clock for the outgoing pixel clock.
>
> That terminology in the PRCM just means that an opt clock will not be
> handled automatically by the PRCM and will require SW control.
> This is not the case for mandatory clock. Upon module enable the PRCM
> will ensure that all mandatory clocks (functional and interface) are
> enabled automagically. If the clock is marked as optional it means that
> the SW will have to enable it explicitly before enabling the module.
>
> The modulemode was not there previously on OMAP2 & 3, but it is more or
> less equivalent to icken=1 + fcken=1.
> This idea was to hide the explicit clock management especially for the
> iclk that were already supposed to always be in autoidle.
>
> Since the current hwmod + clock fmwks are still based on the previous
> clock centric approach we used to have on OMAP2 & 3, we cannot match
> properly the modulemode to any clock and thus cannot handle properly the
> DSS fclk as the main clock instead of the optional clock.
>
> A temporary option will be to consider the modulemode as the interface
> clock and thus remove it from the main_clk and replace it by the real
> DSS fclk.
>
> It should work be will unfortunately not be compliant with PRCM
> recommendation to enable the modulemode once every clocks are enabled.
>
> The long term solution is to update the hwmod fmwk to handle the
> modulemode directly and not through the clock fmwk. It will allow the
> main_clk to be connnected to the dss_fclk.
>
> You will not have that nasty opt_clock issue anymore.
In this long term solution, if the dss_fclk is the main_clk, how does
the framework handle the situation when we want to switch from the
standard DSS fclk to the one from DSI PLL?
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-06 13:15 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307365290.1910.39.camel@deskari>
On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
>> Hi Tomi,
>>
>> On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote:
>>> On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote:
>>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes:
>>>>
>>>>> Hi Kevin,
>>>>>
>>>>> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
>>>>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes:
>>>>>>
>>>>>>> Use PM runtime and HWMOD support to handle enabling and disabling of DSS
>>>>>>> modules.
>>>>>>>
>>>>>>> Each DSS module will have get and put functions which can be used to
>>>>>>> enable and disable that module. The functions use pm_runtime and hwmod
>>>>>>> opt-clocks to enable the hardware.
>>>>>>>
>>>>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +int dispc_runtime_get(void)
>>>>>>> +{
>>>>>>> + int r;
>>>>>>> +
>>>>>>> + mutex_lock(&dispc.runtime_lock);
>>>>>>
>>>>>> It's not clear to me what the lock is trying to protect. I guess it's
>>>>>> the counter? I don't think it should be needed...
>>>>>
>>>>> Yes, the counter. I don't think
>>>>>
>>>>> if (dispc.runtime_count++ = 0)
>>>>>
>>>>> is thread safe.
>>>>
>>>> OK, if it's just the counter, you can drop the mutex and use an atomic
>>>> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear
>>>> from reading what exactly is protected.
>>>
>>> Hmm, sorry, my mistake. It's actually for the whole function: we can't
>>> do "put" before the whole "get" has finished. Otherwise we could end up,
>>> for example, disabling a clock before enabling it.
>>>
>>>>>>> + if (dispc.runtime_count++ = 0) {
>>>>>>
>>>>>> You shouldn't need your own use-counting here. The runtime PM core is
>>>>>> already doing usage counting.
>>>>>>
>>>>>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
>>>>>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
>>>>>> core when the usage count goes to/from zero.
>>>>>
>>>>> Yes, I wish I could do that =).
>>>>>
>>>>> I tried to explain this in the 00-patch, I guess I should've explained
>>>>> it in this patch also. Perhaps also in a comment.
>>>>
>>>> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant
>>>> about DSS, so I was focused in on the runtime PM implementation only.
>>>> Sorry about that.
>>>>
>>>>> From the introduction:
>>>>>
>>>>> ---
>>>>>
>>>>> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
>>>>> problem is that on OMAP4 we have to enable an optional clock before calling
>>>>> pm_runtime_get(), and similarly we need to keep the optional clock enabled
>>>>> until after pm_runtime_put() has been called.
>>>>
>>>> Just to clarify, what exactly does the opt clock have to be enabled for?
>>>
>>> I'm not sure if this is a valid definition, but in my mind the opt clock
>>> has two uses: 1) a functional clock, to make the HW tick and registers
>>> accessible, and 2) act as a source clock for the outgoing pixel clock.
>>
>> That terminology in the PRCM just means that an opt clock will not be
>> handled automatically by the PRCM and will require SW control.
>> This is not the case for mandatory clock. Upon module enable the PRCM
>> will ensure that all mandatory clocks (functional and interface) are
>> enabled automagically. If the clock is marked as optional it means that
>> the SW will have to enable it explicitly before enabling the module.
>>
>> The modulemode was not there previously on OMAP2& 3, but it is more or
>> less equivalent to icken=1 + fcken=1.
>> This idea was to hide the explicit clock management especially for the
>> iclk that were already supposed to always be in autoidle.
>>
>> Since the current hwmod + clock fmwks are still based on the previous
>> clock centric approach we used to have on OMAP2& 3, we cannot match
>> properly the modulemode to any clock and thus cannot handle properly the
>> DSS fclk as the main clock instead of the optional clock.
>>
>> A temporary option will be to consider the modulemode as the interface
>> clock and thus remove it from the main_clk and replace it by the real
>> DSS fclk.
>>
>> It should work be will unfortunately not be compliant with PRCM
>> recommendation to enable the modulemode once every clocks are enabled.
>>
>> The long term solution is to update the hwmod fmwk to handle the
>> modulemode directly and not through the clock fmwk. It will allow the
>> main_clk to be connnected to the dss_fclk.
>>
>> You will not have that nasty opt_clock issue anymore.
>
> In this long term solution, if the dss_fclk is the main_clk, how does
> the framework handle the situation when we want to switch from the
> standard DSS fclk to the one from DSI PLL?
That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
is to ensure that the module is accessible by the driver whatever the
PRCM clock used.
Enabling the DSI PLL will require the PRCM clock to be enabled first.
Using the DSI PLL as the fclk is doable, but is it really useful or needed?
Assuming you need that mode, you will always have to explicitly switch
from DSI to PRCM clock before trying to disable the DSS.
This is something you will have to do inside the DSS driver. It should
be transparent to the hwmod fmwk.
Regards,
Benoit
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-06 13:21 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DECD2D7.6030207@ti.com>
On Mon, 2011-06-06 at 15:15 +0200, Cousson, Benoit wrote:
> On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
> > On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
> > In this long term solution, if the dss_fclk is the main_clk, how does
> > the framework handle the situation when we want to switch from the
> > standard DSS fclk to the one from DSI PLL?
>
> That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
> is to ensure that the module is accessible by the driver whatever the
> PRCM clock used.
> Enabling the DSI PLL will require the PRCM clock to be enabled first.
>
> Using the DSI PLL as the fclk is doable, but is it really useful or needed?
Yes, it's useful and needed. It gives us much finer control to the clock
frequencies, and so allows us to go to higher frequencies and also more
exactly to the required pixel clock.
> Assuming you need that mode, you will always have to explicitly switch
> from DSI to PRCM clock before trying to disable the DSS.
> This is something you will have to do inside the DSS driver. It should
> be transparent to the hwmod fmwk.
This sounds ok.
I think the main question is how do we disable the standard DSS fclk
from PRCM when using DSI PLL? As far as I know, disabling that clock
will allow some areas of OMAP to be shut down even while DSS is working.
So from power management point of view it sounds a needed feature.
If the clock is main_clk for the HWMOD, it sounds to me it's always
enabled if the HWMOD is enabled?
Tomi
^ permalink raw reply
* Re: [PATCH] atmel_lcdfb: fix usage of wrong registers in suspend/resume
From: Hubert Feurstein @ 2011-06-06 13:24 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1307350251-5767-1-git-send-email-h.feurstein@gmail.com>
Or it must be this way:
---
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 4484c72..c2ceae4 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -1085,7 +1085,7 @@ static int atmel_lcdfb_suspend(struct
platform_device *pdev, pm_message_t mesg)
*/
lcdc_writel(sinfo, ATMEL_LCDC_IDR, ~0UL);
- sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
+ sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_CTR);
lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);
if (sinfo->atmel_lcdfb_power_control)
sinfo->atmel_lcdfb_power_control(0);
--
So which solution was originally intended?
Best regards
Hubert
2011/6/6 Hubert Feurstein <h.feurstein@gmail.com>:
> I assume the intention was to set the contrast value to 0 and not
> the contrast control register (in atmel_lcdfb_suspend). And in
> atmel_lcdfb_resume the contrast value should be restored.
>
> Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
> drivers/video/atmel_lcdfb.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
> index 4484c72..2ed7ec1 100644
> --- a/drivers/video/atmel_lcdfb.c
> +++ b/drivers/video/atmel_lcdfb.c
> @@ -1086,7 +1086,7 @@ static int atmel_lcdfb_suspend(struct platform_device *pdev, pm_message_t mesg)
> lcdc_writel(sinfo, ATMEL_LCDC_IDR, ~0UL);
>
> sinfo->saved_lcdcon = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
> - lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, 0);
> + lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, 0);
> if (sinfo->atmel_lcdfb_power_control)
> sinfo->atmel_lcdfb_power_control(0);
>
> @@ -1105,7 +1105,7 @@ static int atmel_lcdfb_resume(struct platform_device *pdev)
> atmel_lcdfb_start(sinfo);
> if (sinfo->atmel_lcdfb_power_control)
> sinfo->atmel_lcdfb_power_control(1);
> - lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, sinfo->saved_lcdcon);
> + lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, sinfo->saved_lcdcon);
>
> /* Enable FIFO & DMA errors */
> lcdc_writel(sinfo, ATMEL_LCDC_IER, ATMEL_LCDC_UFLWI
> --
> 1.7.1
>
>
^ permalink raw reply related
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-06 13:46 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307366474.1910.44.camel@deskari>
On 6/6/2011 3:21 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 15:15 +0200, Cousson, Benoit wrote:
>> On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
>>> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
>
>>> In this long term solution, if the dss_fclk is the main_clk, how does
>>> the framework handle the situation when we want to switch from the
>>> standard DSS fclk to the one from DSI PLL?
>>
>> That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
>> is to ensure that the module is accessible by the driver whatever the
>> PRCM clock used.
>> Enabling the DSI PLL will require the PRCM clock to be enabled first.
>>
>> Using the DSI PLL as the fclk is doable, but is it really useful or needed?
>
> Yes, it's useful and needed. It gives us much finer control to the clock
> frequencies, and so allows us to go to higher frequencies and also more
> exactly to the required pixel clock.
>
>> Assuming you need that mode, you will always have to explicitly switch
>> from DSI to PRCM clock before trying to disable the DSS.
>> This is something you will have to do inside the DSS driver. It should
>> be transparent to the hwmod fmwk.
>
> This sounds ok.
>
> I think the main question is how do we disable the standard DSS fclk
> from PRCM when using DSI PLL? As far as I know, disabling that clock
> will allow some areas of OMAP to be shut down even while DSS is working.
> So from power management point of view it sounds a needed feature.
Yes, at least in theory, but considering that any use case that will
require the DSI PLL will use a LCD panel + backlight, or an OLED panel
that will consume 50 times more than the 186 MHz clock, I do not think
it is really needed.
Moreover, that clock is generated by the PER DPLL that will be always
enabled in most usecase because it does generate the UART, I2C and most
basic peripherals clocks. If we cannot gate the PER DPLL, there is no
saving to expect from gating the DSS fclk only.
Bottom-line is that there is no practical power saving to expect from
that mode.
> If the clock is main_clk for the HWMOD, it sounds to me it's always
> enabled if the HWMOD is enabled?
Yes, but that sounds to me a good trade off to avoid unnecessary
complexity in your driver or in the hwmod fmwk.
Regards,
Benoit
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox