* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Tomi Valkeinen @ 2012-06-27 4:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20120627004709.GA22766@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]
On Tue, 2012-06-26 at 17:47 -0700, Mike Turquette wrote:
> On 20120625-16:14, Tomi Valkeinen wrote:
> > A question about clk_prepare/unprepare, not directly related: let's say
> > I have a driver for some HW block. The driver doesn't use clk functions,
> > but uses runtime PM. The driver also sets pm_runtime_irq_safe().
> >
> > Now, the driver can call pm_runtime_get_sync() in an atomic context, and
> > this would lead to the underlying framework (hwmod, omap_device, I don't
> > know who =) enabling the func clock for that HW. But this would happen
> > in atomic context, so the underlying framework can't use clk_prepare.
> >
> > How does the underlying framework handle that case? (sorry if that's a
> > stupid question =).
> >
>
> I think it's a good question!
>
> If we're going to call clk_prepare_enable from within a runtime pm
> callback then I think we'll need to check if _irq_safe() is set and
> conditionally call only clk_enable in such a case.
>
> I'm not a runtime pm expert, but if the driver owns the responsibility
> of calling pm_runtime_irq_safe then the driver has the proper context
> to know that it should call clk_prepare BEFORE calling
> pm_runtime_get_sync.
That's not quite what I meant. If it's the driver that does clk_enable,
be it in runtime PM callback or not, it's driver's responsibility.
But some clocks are not handled by the driver, but the hwmod/omap_device
framework. Mainly I think this is for the functional and interface
clocks. The driver has no visibility to those, they are implicitly
enabled via pm_runtime_get.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Jassi Brar @ 2012-06-27 4:54 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340736243.24530.98.camel@deskari>
On 27 June 2012 00:14, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-06-26 at 22:31 +0530, Jassi Brar wrote:
>>
>> > But if pm_runtime_get_sync() returns an error, it means the HW has not
>> > been resumed successfully, and is not operational,
>> >
>> Not always. The HW could be in RPM_ACTIVE state while PM on it could
>> be disabled, if the returned error is -EACCESS. And
>> pm_runtime_enabled() only catches a potential -EACCESS.
>
> True. But the HW could also be in disabled state. And that would lead to
> a crash when accessing the registers.
>
> It is not a fatal error if pm_runtime_get returns -EACCES, but we sure
> shouldn't ignore it (or avoid it with pm_runtime_enabled()), but handle
> it. In some rare cases it could be ok to get -EACCES, but that's a
> special case, not standard.
>
You are mixing up generic concepts with what we have in omapdss.
Believe me, I do understand it's bad to proceed without caring for
returned _errors_.
The way omapdss is organized -EACCESS is _not_ an error, it just
denotes PM is disabled on the device and that DISPC is in RPM_ACTIVE
is backed by the fact that HDMI always hold a reference between
resume-suspend and DISPC goes to suspend last and resume first.
>> BTW, I just tested your patch and it worked for me as well. But as
>> suspected, it doesn't help the stack spew of CONFIG_PM_RUNTIME:=n
>>
>> So I understand, I only need to resend the other three patches ?
>
> Yes, please.
>
OK, will do today later.
Regards.
^ permalink raw reply
* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Rajendra Nayak @ 2012-06-27 5:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340770798.1972.4.camel@lappyti>
On Wednesday 27 June 2012 09:49 AM, Tomi Valkeinen wrote:
> On Tue, 2012-06-26 at 17:47 -0700, Mike Turquette wrote:
>> On 20120625-16:14, Tomi Valkeinen wrote:
>>> A question about clk_prepare/unprepare, not directly related: let's say
>>> I have a driver for some HW block. The driver doesn't use clk functions,
>>> but uses runtime PM. The driver also sets pm_runtime_irq_safe().
>>>
>>> Now, the driver can call pm_runtime_get_sync() in an atomic context, and
>>> this would lead to the underlying framework (hwmod, omap_device, I don't
>>> know who =) enabling the func clock for that HW. But this would happen
>>> in atomic context, so the underlying framework can't use clk_prepare.
>>>
>>> How does the underlying framework handle that case? (sorry if that's a
>>> stupid question =).
>>>
>>
>> I think it's a good question!
>>
>> If we're going to call clk_prepare_enable from within a runtime pm
>> callback then I think we'll need to check if _irq_safe() is set and
>> conditionally call only clk_enable in such a case.
>>
>> I'm not a runtime pm expert, but if the driver owns the responsibility
>> of calling pm_runtime_irq_safe then the driver has the proper context
>> to know that it should call clk_prepare BEFORE calling
>> pm_runtime_get_sync.
>
> That's not quite what I meant. If it's the driver that does clk_enable,
> be it in runtime PM callback or not, it's driver's responsibility.
>
> But some clocks are not handled by the driver, but the hwmod/omap_device
> framework. Mainly I think this is for the functional and interface
> clocks. The driver has no visibility to those, they are implicitly
> enabled via pm_runtime_get.
yes, thats the tricky part on how would hwmod/omap_device know if the
driver would use runtime pm within atomic context or non-atomic context.
The driver does inform the runtime pm framework about this by calling a
pm_runtime_irq_safe(), which is then used to set the .irq_safe flag
telling it to leave the interrupts disabled during callbacks.
This information however does not flow down to hwmod/omap_device in any
way. So the way its currently handled is to do an early prepare of all
hwmod controlled clocks.
I have copied Kevin in case he has any better ideas on how this should
be handled.
>
> Tomi
>
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-27 5:58 UTC (permalink / raw)
To: Jassi Brar; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <CAJe_ZheovnRsJ6opMPkMxwjLSv-OKBPCHu5rsAX_KNz1mb64ZQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]
On Wed, 2012-06-27 at 10:12 +0530, Jassi Brar wrote:
> > True. But the HW could also be in disabled state. And that would lead to
> > a crash when accessing the registers.
> >
> > It is not a fatal error if pm_runtime_get returns -EACCES, but we sure
> > shouldn't ignore it (or avoid it with pm_runtime_enabled()), but handle
> > it. In some rare cases it could be ok to get -EACCES, but that's a
> > special case, not standard.
> >
> You are mixing up generic concepts with what we have in omapdss.
> Believe me, I do understand it's bad to proceed without caring for
> returned _errors_.
> The way omapdss is organized -EACCESS is _not_ an error, it just
> denotes PM is disabled on the device and that DISPC is in RPM_ACTIVE
> is backed by the fact that HDMI always hold a reference between
> resume-suspend and DISPC goes to suspend last and resume first.
I'm not arguing that your solution would not work with the omapdss code
we have now, and presuming the underlying frameworks work fine. But I
want omapdss to have code that works also in the future, when other
parts of omapdss change.
It doesn't matter how omapdss is organized, -EACCES _is_ an error. It
tells us that something unexpected happened, and we should react to it
somehow.
When we call, for example, dispc_runtime_get(), we normally expect that
runtime PM is enabled, and it 1) "gets" it, increasing the use count,
and 2) makes sure the HW is enabled so it can be used. Your patch breaks
both of these if runtime PM is disabled.
Sure, in the current omapdss neither is a breaking problem, because 1)
the matching dispc_runtime_put() is called also with runtime PM
disabled, and thus we don't decrease the use count, and 2) the HW
happens to be already enabled. But that's just by "luck", and tomorrow
omapdss could be different.
If, for some reason, we need to call dispc_runtime_get/put during
suspend, the caller should either 1) realize that we're suspending,
runtime PM is disabled, but the HW is anyway enabled, and thus skip
calling both get and put, or 2) call both get and put, but handle the
-EACCES, understanding what it means and knowing the HW is anyway
enabled.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Jassi Brar @ 2012-06-27 7:53 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340776683.1972.41.camel@lappyti>
On 27 June 2012 11:28, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> It doesn't matter how omapdss is organized, -EACCES _is_ an error. It
> tells us that something unexpected happened, and we should react to it
> somehow.
>
$ git show 5025ce070
Exactly how omapdss is organised is the reason -EBUSY isn't an error there :)
Otherwise, omapdss should panic that somehow 'imbalance' has been
introduced in rpm.
>
> Sure, in the current omapdss neither is a breaking problem, because 1)
> the matching dispc_runtime_put() is called also with runtime PM
> disabled, and thus we don't decrease the use count, and 2) the HW
> happens to be already enabled. But that's just by "luck", and tomorrow
> omapdss could be different.
>
It's no 'luck', but it's because today omapdss takes proper care of PM
enable/disable and get/put.
Rather, if tomorrow that stops working, it would hint that we managed
to screw up the balance.
Because if omapdss suspended and disabled PM on DISPC, and still HDMI
attempted to access dss regs, that clearly means HDMI hasn't been duly
made aware of the DISPC status.
Just as preemption and suspend/resume don't introduce any race in
locking, RPM won't introduce new imbalance in get/put of omapdss.
I am afraid, we won't reach any eureka moment on this, so I would
leave us to our conditions. This patch and discussion made me look
deep into rpm, I thank you for that and for your patience.
Cheers!
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-27 8:13 UTC (permalink / raw)
To: Jassi Brar; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <CAJe_Zhdz5mAZbyer8fL4YfvcQ8xMF31cPG2zr3AbBNWv8nctnw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3862 bytes --]
On Wed, 2012-06-27 at 13:11 +0530, Jassi Brar wrote:
> On 27 June 2012 11:28, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> > It doesn't matter how omapdss is organized, -EACCES _is_ an error. It
> > tells us that something unexpected happened, and we should react to it
> > somehow.
> >
> $ git show 5025ce070
> Exactly how omapdss is organised is the reason -EBUSY isn't an error there :)
> Otherwise, omapdss should panic that somehow 'imbalance' has been
> introduced in rpm.
There's no imbalance there, as each get() is still matched by a put(),
and the use count is correct. Your patch may cause either get or put to
be skipped.
In 5025ce070 the function in question is dss_runtime_put(), and -EBUSY
_is_ an error there. Normally if you call pm_runtime_put_sync() and the
use count drops to zero, the device should be suspended. In this case,
however, it won't be suspended as a child device is still enabled. Thus,
the framework informs about this with -EBUSY.
It's ok to ignore -EBUSY, because we're not really interested about if
the device is actually suspended or not.
However, dispc_runtime_get() is a different matter, because there we
_are_ interested about the state of the HW. If we skip
dispc_runtime_get() because runtime PM is disabled, we don't know
whether the HW is enabled or not.
And even if your patch was modified to check the HW status after
pm_runtime_enabled(), and return 0 is HW is enabled and an error if HW
is disabled, it'd be wrong, because you skip the pm_runtime_get() call.
This means that the use count is not increased, and there's no guarantee
that the HW would be functional after dispc_runtime_get() returns.
> > Sure, in the current omapdss neither is a breaking problem, because 1)
> > the matching dispc_runtime_put() is called also with runtime PM
> > disabled, and thus we don't decrease the use count, and 2) the HW
> > happens to be already enabled. But that's just by "luck", and tomorrow
> > omapdss could be different.
> >
> It's no 'luck', but it's because today omapdss takes proper care of PM
> enable/disable and get/put.
> Rather, if tomorrow that stops working, it would hint that we managed
> to screw up the balance.
> Because if omapdss suspended and disabled PM on DISPC, and still HDMI
> attempted to access dss regs, that clearly means HDMI hasn't been duly
> made aware of the DISPC status.
There are two different things here. First one is how
dispc_runtime_get/put & co. should work. The second is how they are
used.
As I see, you are arguing that it's ok to have dispc_runtime_get/put
broken, as long as they are used in a way that causes no problems.
> Just as preemption and suspend/resume don't introduce any race in
> locking, RPM won't introduce new imbalance in get/put of omapdss.
>
> I am afraid, we won't reach any eureka moment on this, so I would
> leave us to our conditions. This patch and discussion made me look
> deep into rpm, I thank you for that and for your patience.
Yes, same here. I think this discussion and related code digging has
really improved my understanding of runtime PM =). Perhaps I'll get it
correct this time with this new information.
There's still the system suspend, which I think is quite broken. The
patch I gave fixes it for the time being, but I see it as a temporary
solution.
I don't like it at all that omapdss disables and enables the panels in
omapdss's suspend/resume hooks. But I'm not sure how this should work...
Should panel drivers each have their own suspend/resume hooks, and
handle it themselves? Or should the call to suspend/resume come from
upper layers, like omapfb or omapdrm.
I made a prototype patch a few weeks ago to move the suspend to omapfb,
and it feels better than the current one, but I'm still not sure...
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v2] OMAPDSS: add clk_prepare and clk_unprepare
From: Rajendra Nayak @ 2012-06-27 8:55 UTC (permalink / raw)
To: linux-omap
Cc: Rajendra Nayak, Tomi Valkeinen, linux-fbdev, Paul Walmsley,
Mike Turquette
In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
and clk_unprepare() for the omapdss clocks.
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: <linux-fbdev@vger.kernel.org>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Mike Turquette <mturquette@linaro.org>
---
drivers/video/omap2/dss/dsi.c | 4 ++--
drivers/video/omap2/dss/hdmi.c | 4 ++--
drivers/video/omap2/dss/venc.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index ca8382d..423ef6a 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1085,9 +1085,9 @@ static inline void dsi_enable_pll_clock(struct platform_device *dsidev,
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
if (enable)
- clk_enable(dsi->sys_clk);
+ clk_prepare_enable(dsi->sys_clk);
else
- clk_disable(dsi->sys_clk);
+ clk_disable_unprepare(dsi->sys_clk);
if (enable && dsi->pll_locked) {
if (wait_for_bit_change(dsidev, DSI_PLL_STATUS, 1, 1) != 1)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8195c71..b9424b3 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -785,7 +785,7 @@ static int __exit omapdss_hdmihw_remove(struct platform_device *pdev)
static int hdmi_runtime_suspend(struct device *dev)
{
- clk_disable(hdmi.sys_clk);
+ clk_disable_unprepare(hdmi.sys_clk);
dispc_runtime_put();
@@ -800,7 +800,7 @@ static int hdmi_runtime_resume(struct device *dev)
if (r < 0)
return r;
- clk_enable(hdmi.sys_clk);
+ clk_prepare_enable(hdmi.sys_clk);
return 0;
}
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2b89739..fd37440 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -930,7 +930,7 @@ static int __exit omap_venchw_remove(struct platform_device *pdev)
static int venc_runtime_suspend(struct device *dev)
{
if (venc.tv_dac_clk)
- clk_disable(venc.tv_dac_clk);
+ clk_disable_unprepare(venc.tv_dac_clk);
dispc_runtime_put();
@@ -946,7 +946,7 @@ static int venc_runtime_resume(struct device *dev)
return r;
if (venc.tv_dac_clk)
- clk_enable(venc.tv_dac_clk);
+ clk_prepare_enable(venc.tv_dac_clk);
return 0;
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v2] OMAPDSS: add clk_prepare and clk_unprepare
From: Tomi Valkeinen @ 2012-06-27 10:42 UTC (permalink / raw)
To: Rajendra Nayak; +Cc: linux-omap, linux-fbdev, Paul Walmsley, Mike Turquette
In-Reply-To: <1340787086-29619-1-git-send-email-rnayak@ti.com>
[-- Attachment #1: Type: text/plain, Size: 380 bytes --]
On Wed, 2012-06-27 at 14:21 +0530, Rajendra Nayak wrote:
> In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
> and clk_unprepare() for the omapdss clocks.
Looks fine, and works fine with a quick test.
I'll update the subject and desc to match the use of clk_prepare_enable
and clk_disable_unprepare, and apply this to omapdss tree.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 05/17] OMAPDSS: Add some new fields to omap_video_timings
From: Tomi Valkeinen @ 2012-06-27 11:48 UTC (permalink / raw)
To: Archit Taneja; +Cc: rob, linux-fbdev, linux-omap
In-Reply-To: <1340703414-1915-7-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]
On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
> Some panel timing related fields are contained in omap_panel_config in the form
> of flags. The fields are:
>
> - Hsync logic level
> - Vsync logic level
> - Data driven on rising/falling edge of pixel clock
> - Output enable/Data enable logic level
> - HSYNC/VSYNC driven on rising/falling edge of pixel clock
>
> Out of these parameters, Hsync and Vsync logic levels are a part of the timings
> in the Xorg modeline configuration. So it makes sense to move the to
> omap_video_timings. The rest aren't a part of modeline, but it still makes
> sense to move these since they are related to panel timings.
>
> These fields stored in omap_panel_config in dssdev are configured for LCD
> panels, and the corresponding LCD managers in the DISPC_POL_FREQo registers.
>
> Add the above fields in omap_video_timings. Represent their state via new enums.
> The parameter pclk_edge is configured via omap_dss_signal_level, however it
> actually configures whether data is driven on the rising or falling edge. This
> is a bit unclean, but it prevents us from creating another enum.
Hmm, why can't omap_dss_signal_edge be used for pclk_edge? I think it'd
fit fine, except OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES would be an illegal
value for it.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 17/17] OMAPDSS: DSI: Remove redundant fields in omap_dss_dsi_videomode_data
From: Tomi Valkeinen @ 2012-06-27 12:05 UTC (permalink / raw)
To: Archit Taneja; +Cc: rob, linux-fbdev, linux-omap
In-Reply-To: <1340703414-1915-19-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]
On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
> The struct omap_dss_dsi_videomode_data holds polaritiy/logic level information
> of the DISPC video port signals DE, HSYNC and VSYNC. This information already
> exists in the omap_video_timings struct.
>
> Use the fields in omap_video_timings to program VP_DE_POL, VP_HSYNC_POL and
> VP_VSYNC_POL in DSI_CTRL. Remove the redundant fields in
> omap_dss_dsi_videomode_data.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/dsi.c | 11 ++++++++---
> include/video/omapdss.h | 3 ---
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 061bf53..3844430 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -3628,13 +3628,18 @@ static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
> static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
> - int de_pol = dssdev->panel.dsi_vm_data.vp_de_pol;
> - int hsync_pol = dssdev->panel.dsi_vm_data.vp_hsync_pol;
> - int vsync_pol = dssdev->panel.dsi_vm_data.vp_vsync_pol;
> + int de_pol, hsync_pol, vsync_pol;
> + int de_level = dssdev->panel.timings.de_level;
> + int hsync_level = dssdev->panel.timings.hsync_level;
> + int vsync_level = dssdev->panel.timings.vsync_level;
> bool vsync_end = dssdev->panel.dsi_vm_data.vp_vsync_end;
> bool hsync_end = dssdev->panel.dsi_vm_data.vp_hsync_end;
> u32 r;
>
> + de_pol = de_level == OMAPDSS_SIG_ACTIVE_HIGH ? 1 : 0;
> + hsync_pol = hsync_level == OMAPDSS_SIG_ACTIVE_HIGH ? 1 : 0;
> + vsync_pol = vsync_level == OMAPDSS_SIG_ACTIVE_HIGH ? 1 : 0;
> +
> r = dsi_read_reg(dsidev, DSI_CTRL);
> r = FLD_MOD(r, de_pol, 9, 9); /* VP_DE_POL */
> r = FLD_MOD(r, hsync_pol, 10, 10); /* VP_HSYNC_POL */
This patch makes the code cleaner, but I find this DSI sync code a bit
strange.
The sync polarities between DISPC and DSI do not matter elsewhere, they
do not affect the DSI output, so why do we have them in the panel data?
Why doesn't dsi.c just use some hardcoded values for these.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 17/17] OMAPDSS: DSI: Remove redundant fields in omap_dss_dsi_videomode_data
From: Archit Taneja @ 2012-06-27 12:30 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: rob, linux-fbdev, linux-omap
In-Reply-To: <1340798746.2649.51.camel@deskari>
On Wednesday 27 June 2012 05:35 PM, Tomi Valkeinen wrote:
> On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
>> The struct omap_dss_dsi_videomode_data holds polaritiy/logic level information
>> of the DISPC video port signals DE, HSYNC and VSYNC. This information already
>> exists in the omap_video_timings struct.
>>
>> Use the fields in omap_video_timings to program VP_DE_POL, VP_HSYNC_POL and
>> VP_VSYNC_POL in DSI_CTRL. Remove the redundant fields in
>> omap_dss_dsi_videomode_data.
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>> drivers/video/omap2/dss/dsi.c | 11 ++++++++---
>> include/video/omapdss.h | 3 ---
>> 2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
>> index 061bf53..3844430 100644
>> --- a/drivers/video/omap2/dss/dsi.c
>> +++ b/drivers/video/omap2/dss/dsi.c
>> @@ -3628,13 +3628,18 @@ static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
>> static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
>> {
>> struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
>> - int de_pol = dssdev->panel.dsi_vm_data.vp_de_pol;
>> - int hsync_pol = dssdev->panel.dsi_vm_data.vp_hsync_pol;
>> - int vsync_pol = dssdev->panel.dsi_vm_data.vp_vsync_pol;
>> + int de_pol, hsync_pol, vsync_pol;
>> + int de_level = dssdev->panel.timings.de_level;
>> + int hsync_level = dssdev->panel.timings.hsync_level;
>> + int vsync_level = dssdev->panel.timings.vsync_level;
>> bool vsync_end = dssdev->panel.dsi_vm_data.vp_vsync_end;
>> bool hsync_end = dssdev->panel.dsi_vm_data.vp_hsync_end;
>> u32 r;
>>
>> + de_pol = de_level = OMAPDSS_SIG_ACTIVE_HIGH ? 1 : 0;
>> + hsync_pol = hsync_level = OMAPDSS_SIG_ACTIVE_HIGH ? 1 : 0;
>> + vsync_pol = vsync_level = OMAPDSS_SIG_ACTIVE_HIGH ? 1 : 0;
>> +
>> r = dsi_read_reg(dsidev, DSI_CTRL);
>> r = FLD_MOD(r, de_pol, 9, 9); /* VP_DE_POL */
>> r = FLD_MOD(r, hsync_pol, 10, 10); /* VP_HSYNC_POL */
>
> This patch makes the code cleaner, but I find this DSI sync code a bit
> strange.
>
> The sync polarities between DISPC and DSI do not matter elsewhere, they
> do not affect the DSI output, so why do we have them in the panel data?
> Why doesn't dsi.c just use some hardcoded values for these.
Ok, are you saying that unlike DPI, where a panel may request for some
different polarities. There is no such need for DSI panels, and we can
set the polarities of DISPC and DSI always to active high(or any other
combination)?
Well, we are doing that indirectly in a way, a DSI panel driver would
populate a omap_video_timings struct, and would leave hsync_level,
vsync_level and de_level empty(i.e, the default values). This would be
passed to the DSI driver, and the timings would be applied to DISPC. The
function above would just pick up the same default values and program to
the DSI registers.
What we could do is ignore these fields in the omap_video_timings when
passed from the panel driver to DSI driver, and always use a fixed value
for them, and this way we can use the same fixed values for DSI too. Do
you think that is better?
Archit
^ permalink raw reply
* Re: [PATCH 17/17] OMAPDSS: DSI: Remove redundant fields in omap_dss_dsi_videomode_data
From: Tomi Valkeinen @ 2012-06-27 12:31 UTC (permalink / raw)
To: Archit Taneja; +Cc: rob, linux-fbdev, linux-omap
In-Reply-To: <4FEAFA18.3070908@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]
On Wed, 2012-06-27 at 17:48 +0530, Archit Taneja wrote:
> On Wednesday 27 June 2012 05:35 PM, Tomi Valkeinen wrote:
> > The sync polarities between DISPC and DSI do not matter elsewhere, they
> > do not affect the DSI output, so why do we have them in the panel data?
> > Why doesn't dsi.c just use some hardcoded values for these.
>
> Ok, are you saying that unlike DPI, where a panel may request for some
> different polarities. There is no such need for DSI panels, and we can
> set the polarities of DISPC and DSI always to active high(or any other
> combination)?
Yes. There are no sync polarities in DSI bus, there are only sync
packets. So afaik, the polarities used here matter only for DISPC and
DSI communication. And there the only thing that matters is that both
DISPC and DSI have the same configuration for the polarities, so that
the communication works.
> Well, we are doing that indirectly in a way, a DSI panel driver would
> populate a omap_video_timings struct, and would leave hsync_level,
> vsync_level and de_level empty(i.e, the default values). This would be
> passed to the DSI driver, and the timings would be applied to DISPC. The
> function above would just pick up the same default values and program to
> the DSI registers.
>
> What we could do is ignore these fields in the omap_video_timings when
> passed from the panel driver to DSI driver, and always use a fixed value
> for them, and this way we can use the same fixed values for DSI too. Do
> you think that is better?
I think that is clearer. Optimally we wouldn't even have a video timings
struct for DSI panels, the kind that contains sync polarities and such,
but a separate timings struct that contains stuff relevant for DSI. But
for now I think we should just ignore the "extra" values in video
timings.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 05/17] OMAPDSS: Add some new fields to omap_video_timings
From: Archit Taneja @ 2012-06-27 12:38 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, rob, linux-fbdev, linux-omap
In-Reply-To: <1340797733.2649.48.camel@deskari>
On Wednesday 27 June 2012 05:18 PM, Tomi Valkeinen wrote:
> On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
>> Some panel timing related fields are contained in omap_panel_config in the form
>> of flags. The fields are:
>>
>> - Hsync logic level
>> - Vsync logic level
>> - Data driven on rising/falling edge of pixel clock
>> - Output enable/Data enable logic level
>> - HSYNC/VSYNC driven on rising/falling edge of pixel clock
>>
>> Out of these parameters, Hsync and Vsync logic levels are a part of the timings
>> in the Xorg modeline configuration. So it makes sense to move the to
>> omap_video_timings. The rest aren't a part of modeline, but it still makes
>> sense to move these since they are related to panel timings.
>>
>> These fields stored in omap_panel_config in dssdev are configured for LCD
>> panels, and the corresponding LCD managers in the DISPC_POL_FREQo registers.
>>
>> Add the above fields in omap_video_timings. Represent their state via new enums.
>> The parameter pclk_edge is configured via omap_dss_signal_level, however it
>> actually configures whether data is driven on the rising or falling edge. This
>> is a bit unclean, but it prevents us from creating another enum.
>
> Hmm, why can't omap_dss_signal_edge be used for pclk_edge? I think it'd
> fit fine, except OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES would be an illegal
> value for it.
I think my paragraph is a bit misleading. The issue is more about the
default value. For hsync_vsync_edge(which programs ONOFF and RF), the
default value is OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES, and is the case for
most panels. But for pclk_edge, the value for most panels is
OMAPDSS_DRIVE_SIG_ACTIVE_HIGH.
So if I use the same enum for both, I'll need to populate either
pclk_edge or hsync_vsync_edge for almost every panel. With my approach,
I only need to populate these fields for panels having the non-default
requirements. The negative thing is that it's a bit misleading to
represent pclk_edge with the omap_dss_signal_level enum.
Archit
^ permalink raw reply
* Re: [PATCH 05/17] OMAPDSS: Add some new fields to omap_video_timings
From: Tomi Valkeinen @ 2012-06-27 12:42 UTC (permalink / raw)
To: Archit Taneja; +Cc: Archit Taneja, rob, linux-fbdev, linux-omap
In-Reply-To: <4FEAFC02.2070006@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]
On Wed, 2012-06-27 at 17:56 +0530, Archit Taneja wrote:
> On Wednesday 27 June 2012 05:18 PM, Tomi Valkeinen wrote:
> > On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
> >> Some panel timing related fields are contained in omap_panel_config in the form
> >> of flags. The fields are:
> >>
> >> - Hsync logic level
> >> - Vsync logic level
> >> - Data driven on rising/falling edge of pixel clock
> >> - Output enable/Data enable logic level
> >> - HSYNC/VSYNC driven on rising/falling edge of pixel clock
> >>
> >> Out of these parameters, Hsync and Vsync logic levels are a part of the timings
> >> in the Xorg modeline configuration. So it makes sense to move the to
> >> omap_video_timings. The rest aren't a part of modeline, but it still makes
> >> sense to move these since they are related to panel timings.
> >>
> >> These fields stored in omap_panel_config in dssdev are configured for LCD
> >> panels, and the corresponding LCD managers in the DISPC_POL_FREQo registers.
> >>
> >> Add the above fields in omap_video_timings. Represent their state via new enums.
> >> The parameter pclk_edge is configured via omap_dss_signal_level, however it
> >> actually configures whether data is driven on the rising or falling edge. This
> >> is a bit unclean, but it prevents us from creating another enum.
> >
> > Hmm, why can't omap_dss_signal_edge be used for pclk_edge? I think it'd
> > fit fine, except OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES would be an illegal
> > value for it.
>
> I think my paragraph is a bit misleading. The issue is more about the
> default value. For hsync_vsync_edge(which programs ONOFF and RF), the
> default value is OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES, and is the case for
> most panels. But for pclk_edge, the value for most panels is
> OMAPDSS_DRIVE_SIG_ACTIVE_HIGH.
>
> So if I use the same enum for both, I'll need to populate either
> pclk_edge or hsync_vsync_edge for almost every panel. With my approach,
> I only need to populate these fields for panels having the non-default
> requirements. The negative thing is that it's a bit misleading to
> represent pclk_edge with the omap_dss_signal_level enum.
Ah, I see.
I really think using the ACTIVE_LOW/HIGH for pclk_edge is quite ugly =).
Well, one option is to add new entry for the enums, "UNDEFINED" or
perhaps "DEFAULT", which would have value 0. Then omapdss would know
that it should use the default value, whatever that is.
Another option is to have all panels define the pclk_edge, so that
there's no default value needed.
Also, related thing, I see you're writing enum values directly to the
registers. If you do that, it'd be good to explicitly set the number
value of the enums. Otherwise it's quite easy to add a new enum value
later between the old ones, breaking everything (perhaps not a problem
here, though).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH, RFC] displaymodes in devicetree
From: Sascha Hauer @ 2012-06-27 12:43 UTC (permalink / raw)
To: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
Hi All,
I'd like to have a possibility to describe fixed display modes in the
devicetree. This topic has been discussed before here:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2010-February/080683.html
The result at that time was that EDID data should be considered to use
as this is a format that already exists. I want to come back to this
topic since:
- EDID data is a binary format and as such quite inconvenient to handle.
There exist several tools to parse EDID data, but I'm not aware of any
(open source) tool which can generate EDID data.
- EDID blobs are hard to modify and hard to review in patches.
- EDID is designed to describe multiple modes, but fixed displays
usually only support a single fixed mode.
There are several ways of describing the mode, for this patch I chose to
use the format (and naming) used by the Linux Framebuffer Layer as this
is the only description which does not allow for inconsistent modes. I
added the most common flags like [v|h]sync_active_high. Others can be
added, but these flags are pretty much agreed upon and won't be
superseeded with other flags. The mode from the devicetree can be
converted to the most common modes used in Linux, struct fb_videomode
and struct drm_display_mode.
Comments welcome
Sascha
8<--------------------------------------------
of: Add videomode helper
This patch adds a helper function for parsing videomodes from the devicetree.
The videomode is returned either as a struct drm_display_mode or a
struct fb_videomode.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
.../devicetree/bindings/video/displaymode | 40 ++++++++
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 108 ++++++++++++++++++++
include/linux/of_videomode.h | 19 ++++
5 files changed, 173 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/displaymode
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 0000000..303cfc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+=========
+
+Required properties:
+ - xres, yres: Display resolution
+ - left_margin, right_margin, hsync_len: Horizontal Display timing parameters
+ in pixels
+ upper_margin, lower_margin, vsync_len: Vertical display timing parameters in
+ lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width_mm, height_mm: Display dimensions in mm
+ - hsync_active_high (bool): Hsync pulse is active high
+ - vsync_active_high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree representation
+corresponds to the one used by the Linux Framebuffer framework described here in
+Documentation/fb/framebuffer.txt. This representation has been chosen because it's
+the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
+framework the devicetree has the clock in Hz instead of ps.
+
+Example:
+
+ display@0 {
+ /* 1920x1080p24 */
+ clock = <52000000>;
+ xres = <1920>;
+ yres = <1080>;
+ left_margin = <25>;
+ right_margin = <25>;
+ hsync_len = <25>;
+ lower_margin = <2>;
+ upper_margin = <2>;
+ vsync_len = <2>;
+ hsync_active_high;
+ };
+
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a3acaa3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_VIDEOMODE
+ def_bool y
+ help
+ helper to parse videomodes from the devicetree
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..80e6db3 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 0000000..859aefb
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,108 @@
+/*
+ * OF helpers for parsing display modes
+ *
+ * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/export.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+ struct fb_videomode *fbmode)
+{
+ int ret = 0;
+ u32 left_margin, xres, right_margin, hsync_len;
+ u32 upper_margin, yres, lower_margin, vsync_len;
+ u32 width_mm = 0, height_mm = 0;
+ u32 clock;
+ bool hah = false, vah = false, interlaced = false, doublescan = false;
+
+ if (!np)
+ return -EINVAL;
+
+ ret |= of_property_read_u32(np, "left_margin", &left_margin);
+ ret |= of_property_read_u32(np, "xres", &xres);
+ ret |= of_property_read_u32(np, "right_margin", &right_margin);
+ ret |= of_property_read_u32(np, "hsync_len", &hsync_len);
+ ret |= of_property_read_u32(np, "upper_margin", &upper_margin);
+ ret |= of_property_read_u32(np, "yres", &yres);
+ ret |= of_property_read_u32(np, "lower_margin", &lower_margin);
+ ret |= of_property_read_u32(np, "vsync_len", &vsync_len);
+ ret |= of_property_read_u32(np, "clock", &clock);
+ if (ret)
+ return -EINVAL;
+
+ of_property_read_u32(np, "width_mm", &width_mm);
+ of_property_read_u32(np, "height_mm", &height_mm);
+
+ hah = of_property_read_bool(np, "hsync_active_high");
+ vah = of_property_read_bool(np, "vsync_active_high");
+ interlaced = of_property_read_bool(np, "interlaced");
+ doublescan = of_property_read_bool(np, "doublescan");
+
+ if (dmode) {
+ memset(dmode, 0, sizeof(*dmode));
+
+ dmode->hdisplay = xres;
+ dmode->hsync_start = xres + right_margin;
+ dmode->hsync_end = xres + right_margin + hsync_len;
+ dmode->htotal = xres + right_margin + hsync_len + left_margin;
+
+ dmode->vdisplay = yres;
+ dmode->vsync_start = yres + lower_margin;
+ dmode->vsync_end = yres + lower_margin + vsync_len;
+ dmode->vtotal = yres + lower_margin + vsync_len + upper_margin;
+
+ dmode->width_mm = width_mm;
+ dmode->height_mm = height_mm;
+
+ dmode->clock = clock / 1000;
+
+ if (hah)
+ dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+ else
+ dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+ if (vah)
+ dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+ else
+ dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+ if (interlaced)
+ dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+ if (doublescan)
+ dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+
+ drm_mode_set_name(dmode);
+ }
+
+ if (fbmode) {
+ memset(fbmode, 0, sizeof(*fbmode));
+
+ fbmode->xres = xres;
+ fbmode->left_margin = left_margin;
+ fbmode->right_margin = right_margin;
+ fbmode->hsync_len = hsync_len;
+
+ fbmode->yres = yres;
+ fbmode->upper_margin = upper_margin;
+ fbmode->lower_margin = lower_margin;
+ fbmode->vsync_len = vsync_len;
+
+ fbmode->pixclock = KHZ2PICOS(clock / 1000);
+
+ if (hah)
+ fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+ if (vah)
+ fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+ if (interlaced)
+ fbmode->vmode |= FB_VMODE_INTERLACED;
+ if (doublescan)
+ fbmode->vmode |= FB_VMODE_DOUBLE;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_video_mode);
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..a988429
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * OF helpers for videomodes.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+struct device_node;
+struct fb_videomode;
+struct drm_display_mode;
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+ struct fb_videomode *fbmode);
+
+#endif /* __LINUX_OF_VIDEOMODE_H */
--
1.7.10
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply related
* Re: [PATCH 05/17] OMAPDSS: Add some new fields to omap_video_timings
From: Archit Taneja @ 2012-06-27 12:59 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, rob, linux-fbdev, linux-omap
In-Reply-To: <1340800933.2649.72.camel@deskari>
On Wednesday 27 June 2012 06:12 PM, Tomi Valkeinen wrote:
> On Wed, 2012-06-27 at 17:56 +0530, Archit Taneja wrote:
>> On Wednesday 27 June 2012 05:18 PM, Tomi Valkeinen wrote:
>>> On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
>>>> Some panel timing related fields are contained in omap_panel_config in the form
>>>> of flags. The fields are:
>>>>
>>>> - Hsync logic level
>>>> - Vsync logic level
>>>> - Data driven on rising/falling edge of pixel clock
>>>> - Output enable/Data enable logic level
>>>> - HSYNC/VSYNC driven on rising/falling edge of pixel clock
>>>>
>>>> Out of these parameters, Hsync and Vsync logic levels are a part of the timings
>>>> in the Xorg modeline configuration. So it makes sense to move the to
>>>> omap_video_timings. The rest aren't a part of modeline, but it still makes
>>>> sense to move these since they are related to panel timings.
>>>>
>>>> These fields stored in omap_panel_config in dssdev are configured for LCD
>>>> panels, and the corresponding LCD managers in the DISPC_POL_FREQo registers.
>>>>
>>>> Add the above fields in omap_video_timings. Represent their state via new enums.
>>>> The parameter pclk_edge is configured via omap_dss_signal_level, however it
>>>> actually configures whether data is driven on the rising or falling edge. This
>>>> is a bit unclean, but it prevents us from creating another enum.
>>>
>>> Hmm, why can't omap_dss_signal_edge be used for pclk_edge? I think it'd
>>> fit fine, except OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES would be an illegal
>>> value for it.
>>
>> I think my paragraph is a bit misleading. The issue is more about the
>> default value. For hsync_vsync_edge(which programs ONOFF and RF), the
>> default value is OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES, and is the case for
>> most panels. But for pclk_edge, the value for most panels is
>> OMAPDSS_DRIVE_SIG_ACTIVE_HIGH.
>>
>> So if I use the same enum for both, I'll need to populate either
>> pclk_edge or hsync_vsync_edge for almost every panel. With my approach,
>> I only need to populate these fields for panels having the non-default
>> requirements. The negative thing is that it's a bit misleading to
>> represent pclk_edge with the omap_dss_signal_level enum.
>
> Ah, I see.
>
> I really think using the ACTIVE_LOW/HIGH for pclk_edge is quite ugly =).
>
> Well, one option is to add new entry for the enums, "UNDEFINED" or
> perhaps "DEFAULT", which would have value 0. Then omapdss would know
> that it should use the default value, whatever that is.
>
> Another option is to have all panels define the pclk_edge, so that
> there's no default value needed.
I think I'll go with this.
>
> Also, related thing, I see you're writing enum values directly to the
> registers. If you do that, it'd be good to explicitly set the number
> value of the enums. Otherwise it's quite easy to add a new enum value
> later between the old ones, breaking everything (perhaps not a problem
> here, though).
Yes, that's true. I'll set the numbers anyway, for clarity.
Archit
^ permalink raw reply
* Re: [PATCH 05/17] OMAPDSS: Add some new fields to omap_video_timings
From: Tomi Valkeinen @ 2012-06-27 13:02 UTC (permalink / raw)
To: Archit Taneja; +Cc: Archit Taneja, rob, linux-fbdev, linux-omap
In-Reply-To: <4FEB00CB.4070502@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]
On Wed, 2012-06-27 at 18:17 +0530, Archit Taneja wrote:
> On Wednesday 27 June 2012 06:12 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-06-27 at 17:56 +0530, Archit Taneja wrote:
> >> On Wednesday 27 June 2012 05:18 PM, Tomi Valkeinen wrote:
> >>> On Tue, 2012-06-26 at 15:06 +0530, Archit Taneja wrote:
> >>>> Some panel timing related fields are contained in omap_panel_config in the form
> >>>> of flags. The fields are:
> >>>>
> >>>> - Hsync logic level
> >>>> - Vsync logic level
> >>>> - Data driven on rising/falling edge of pixel clock
> >>>> - Output enable/Data enable logic level
> >>>> - HSYNC/VSYNC driven on rising/falling edge of pixel clock
> >>>>
> >>>> Out of these parameters, Hsync and Vsync logic levels are a part of the timings
> >>>> in the Xorg modeline configuration. So it makes sense to move the to
> >>>> omap_video_timings. The rest aren't a part of modeline, but it still makes
> >>>> sense to move these since they are related to panel timings.
> >>>>
> >>>> These fields stored in omap_panel_config in dssdev are configured for LCD
> >>>> panels, and the corresponding LCD managers in the DISPC_POL_FREQo registers.
> >>>>
> >>>> Add the above fields in omap_video_timings. Represent their state via new enums.
> >>>> The parameter pclk_edge is configured via omap_dss_signal_level, however it
> >>>> actually configures whether data is driven on the rising or falling edge. This
> >>>> is a bit unclean, but it prevents us from creating another enum.
> >>>
> >>> Hmm, why can't omap_dss_signal_edge be used for pclk_edge? I think it'd
> >>> fit fine, except OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES would be an illegal
> >>> value for it.
> >>
> >> I think my paragraph is a bit misleading. The issue is more about the
> >> default value. For hsync_vsync_edge(which programs ONOFF and RF), the
> >> default value is OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES, and is the case for
> >> most panels. But for pclk_edge, the value for most panels is
> >> OMAPDSS_DRIVE_SIG_ACTIVE_HIGH.
> >>
> >> So if I use the same enum for both, I'll need to populate either
> >> pclk_edge or hsync_vsync_edge for almost every panel. With my approach,
> >> I only need to populate these fields for panels having the non-default
> >> requirements. The negative thing is that it's a bit misleading to
> >> represent pclk_edge with the omap_dss_signal_level enum.
> >
> > Ah, I see.
> >
> > I really think using the ACTIVE_LOW/HIGH for pclk_edge is quite ugly =).
> >
> > Well, one option is to add new entry for the enums, "UNDEFINED" or
> > perhaps "DEFAULT", which would have value 0. Then omapdss would know
> > that it should use the default value, whatever that is.
> >
> > Another option is to have all panels define the pclk_edge, so that
> > there's no default value needed.
>
> I think I'll go with this.
Ok. But if you do this, then perhaps you should fill all the other
properties also. It looks a bit strange to require only one particular
to be always entered, while the others have default values.
But I guess that's not a bad thing at all. If the panel doesn't define
what kind of signal it wants, and trusts that the defaults are always
the same, it may break later or perhaps with other platforms.
So I think the panel drivers should define all the signal properties,
and not leave them to default values. That's the safest option, although
a bit verbose.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] simple device tree support for mxsfb
From: Shawn Guo @ 2012-06-27 13:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340701379-8619-1-git-send-email-shawn.guo@linaro.org>
Hi Florian,
I have a mxs platform patch depends on these two patches to covert
a few board files over to device tree. I would like to have the patches
go via arm-soc tree if they look good. Please let me know if you have
problem with the patches go through arm-soc tree. Thanks.
Regards,
Shawn
On Tue, Jun 26, 2012 at 05:02:57PM +0800, Shawn Guo wrote:
> It adds a simple device tree support for mxsfb driver.
>
> Shawn Guo (2):
> video: mxsfb: move mxsfb.h into include/linux
> video: mxsfb: add simple device tree probe
>
> Documentation/devicetree/bindings/fb/mxsfb.txt | 21 +++++++
> arch/arm/mach-mxs/devices-mx23.h | 2 +-
> arch/arm/mach-mxs/devices-mx28.h | 2 +-
> arch/arm/mach-mxs/devices/platform-mxsfb.c | 2 +-
> drivers/video/mxsfb.c | 62 +++++++++++++++-----
> .../include/mach => include/linux}/mxsfb.h | 6 +-
> 6 files changed, 75 insertions(+), 20 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/fb/mxsfb.txt
> rename {arch/arm/mach-mxs/include/mach => include/linux}/mxsfb.h (95%)
>
> --
> 1.7.5.4
>
>
^ permalink raw reply
* [PATCH 1/3] OMAPDSS: HDMI: Discard phy_tx_enabled member
From: jaswinder.singh @ 2012-06-27 14:16 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, patches,
Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
It is simpler to read the current status from a register as compared
to maintaining a state variable to hold the information.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/ti_hdmi.h | 1 -
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 11 ++++-------
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index e734cb4..d174ca1 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,7 +177,6 @@ struct hdmi_ip_data {
/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
int hpd_gpio;
- bool phy_tx_enabled;
};
int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 4dae1b2..3fa3d98 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
/* PHY_PWR_CMD */
static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
{
+ /* Return if already the state */
+ if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) = val)
+ return 0;
+
/* Command for power control of HDMI PHY */
REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
@@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
hpd = gpio_get_value(ip_data->hpd_gpio);
- if (hpd = ip_data->phy_tx_enabled) {
- spin_unlock_irqrestore(&phy_tx_lock, flags);
- return 0;
- }
-
if (hpd)
r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
else
@@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
goto err;
}
- ip_data->phy_tx_enabled = hpd;
err:
spin_unlock_irqrestore(&phy_tx_lock, flags);
return r;
@@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
- ip_data->phy_tx_enabled = false;
}
static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/3] OMAPDSS: HDMI: Replace spinlock with mutex in hdmi_check_hpd_state
From: jaswinder.singh @ 2012-06-27 14:16 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, patches,
Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
State change of HDMI PHY could potentially take many millisecs, we can do
better by protecting things in hdmi_set_phy_pwr() with a mutex rather than
a spin_lock_irqsave.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/hdmi.c | 1 +
drivers/video/omap2/dss/ti_hdmi.h | 1 +
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 6 ++----
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8195c71..0738090 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -758,6 +758,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
hdmi.ip_data.phy_offset = HDMI_PHY;
+ mutex_init(&hdmi.ip_data.lock);
hdmi_panel_init();
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index d174ca1..cc292b8 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,6 +177,7 @@ struct hdmi_ip_data {
/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
int hpd_gpio;
+ struct mutex lock;
};
int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 3fa3d98..04acca9 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -238,10 +238,8 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
unsigned long flags;
bool hpd;
int r;
- /* this should be in ti_hdmi_4xxx_ip private data */
- static DEFINE_SPINLOCK(phy_tx_lock);
- spin_lock_irqsave(&phy_tx_lock, flags);
+ mutex_lock(&ip_data->lock);
hpd = gpio_get_value(ip_data->hpd_gpio);
@@ -257,7 +255,7 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
}
err:
- spin_unlock_irqrestore(&phy_tx_lock, flags);
+ mutex_unlock(&ip_data->lock);
return r;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: jaswinder.singh @ 2012-06-27 14:17 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, patches,
Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
We can easily keep track of latest EDID from the display and hence avoid
expensive EDID re-reads over I2C.
This could also help some cheapo displays that provide EDID reliably only
immediately after asserting HPD and not later.
Even with good displays, there is something in OMAPDSS that apparantly
messes up DDC occasionally while EDID is being read, giving the
"operation stopped when reading edid" error.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/hdmi.c | 1 +
drivers/video/omap2/dss/ti_hdmi.h | 2 ++
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 15 +++++++++++++--
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 0738090..9853621 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -758,6 +758,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
hdmi.ip_data.phy_offset = HDMI_PHY;
+ hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
mutex_init(&hdmi.ip_data.lock);
hdmi_panel_init();
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index cc292b8..4735860 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -178,6 +178,8 @@ struct hdmi_ip_data {
/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
int hpd_gpio;
struct mutex lock;
+ u8 edid_cached[256];
+ unsigned edid_len;
};
int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 04acca9..b5c3dc4 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
hpd = gpio_get_value(ip_data->hpd_gpio);
- if (hpd)
+ if (hpd) {
r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
- else
+ } else {
+ /* Invalidate EDID Cache */
+ ip_data->edid_len = 0;
r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
+ }
if (r) {
DSSERR("Failed to %s PHY TX power\n",
@@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
{
int r, l;
+ if (ip_data->edid_len) {
+ memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
+ return ip_data->edid_len;
+ }
+
if (len < 128)
return -EINVAL;
@@ -474,6 +482,9 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
l += 128;
}
+ ip_data->edid_len = l;
+ memcpy(ip_data->edid_cached, edid, l);
+
return l;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 1/2] OMAPDSS: Use PM notifiers for system suspend
From: Tomi Valkeinen @ 2012-06-27 14:34 UTC (permalink / raw)
To: linux-omap, linux-fbdev, Jassi Brar; +Cc: Tomi Valkeinen
The current way how omapdss handles system suspend and resume is that
omapdss device (a platform device, which is not part of the device
hierarchy of the DSS HW devices, like DISPC and DSI, or panels.) uses
the suspend and resume callbacks from platform_driver to handle system
suspend. It does this by disabling all enabled panels on suspend, and
resuming the previously disabled panels on resume.
This presents a few problems.
One is that as omapdss device is not related to the panel devices or the
DSS HW devices, there's no ordering in the suspend process. This means
that suspend could be first ran for DSS HW devices and panels, and only
then for omapdss device. Currently this is not a problem, as DSS HW
devices and panels do not handle suspend.
Another, more pressing problem, is that when suspending or resuming, the
runtime PM functions return -EACCES as runtime PM is disabled during
system suspend. This causes the driver to print warnings, and operations
to fail as they think that they failed to bring up the HW.
This patch changes the omapdss suspend handling to use PM notifiers,
which are called before suspend and after resume. This way we have a
normally functioning system when we are suspending and resuming the
panels.
This patch, I believe, creates a problem that somebody could enable or
disable a panel between PM_SUSPEND_PREPARE and the system suspend, and
similarly the other way around in resume. I choose to ignore the problem
for now, as it sounds rather unlikely, and if it happens, it's not
fatal.
In the long run the system suspend handling of omapdss and panels should
be thought out properly. The current approach feels rather hacky.
Perhaps the panel drivers should handle system suspend, or the users of
omapdss (omapfb, omapdrm) should handle system suspend.
Note that after this patch we could probably revert
0eaf9f52e94f756147dbfe1faf1f77a02378dbf9 (OMAPDSS: use sync versions of
pm_runtime_put). But as I said, this patch may be temporary, so let's
leave the sync version still in place.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reported-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/core.c | 45 ++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 16 deletions(-)
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index d9b0bf3..e4fadf9 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -32,6 +32,7 @@
#include <linux/io.h>
#include <linux/device.h>
#include <linux/regulator/consumer.h>
+#include <linux/suspend.h>
#include <video/omapdss.h>
@@ -207,6 +208,30 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
#endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
/* PLATFORM DEVICE */
+static int omap_dss_pm_notif(struct notifier_block *b, unsigned long v, void *d)
+{
+ DSSDBG("pm notif %lu\n", v);
+
+ switch (v)
+ {
+ case PM_SUSPEND_PREPARE:
+ DSSDBG("suspending displays\n");
+ return dss_suspend_all_devices();
+
+ case PM_POST_SUSPEND:
+ DSSDBG("resuming displays\n");
+ return dss_resume_all_devices();
+
+ default:
+ return 0;
+ }
+}
+
+static struct notifier_block omap_dss_pm_notif_block +{
+ .notifier_call = omap_dss_pm_notif,
+};
+
static int __init omap_dss_probe(struct platform_device *pdev)
{
struct omap_dss_board_info *pdata = pdev->dev.platform_data;
@@ -230,6 +255,8 @@ static int __init omap_dss_probe(struct platform_device *pdev)
else if (pdata->default_device)
core.default_display_name = pdata->default_device->name;
+ register_pm_notifier(&omap_dss_pm_notif_block);
+
return 0;
err_debugfs:
@@ -239,6 +266,8 @@ err_debugfs:
static int omap_dss_remove(struct platform_device *pdev)
{
+ unregister_pm_notifier(&omap_dss_pm_notif_block);
+
dss_uninitialize_debugfs();
dss_uninit_overlays(pdev);
@@ -253,25 +282,9 @@ static void omap_dss_shutdown(struct platform_device *pdev)
dss_disable_all_devices();
}
-static int omap_dss_suspend(struct platform_device *pdev, pm_message_t state)
-{
- DSSDBG("suspend %d\n", state.event);
-
- return dss_suspend_all_devices();
-}
-
-static int omap_dss_resume(struct platform_device *pdev)
-{
- DSSDBG("resume\n");
-
- return dss_resume_all_devices();
-}
-
static struct platform_driver omap_dss_driver = {
.remove = omap_dss_remove,
.shutdown = omap_dss_shutdown,
- .suspend = omap_dss_suspend,
- .resume = omap_dss_resume,
.driver = {
.name = "omapdss",
.owner = THIS_MODULE,
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/2] OMAPDSS: fix warnings if CONFIG_PM_RUNTIME=n
From: Tomi Valkeinen @ 2012-06-27 14:34 UTC (permalink / raw)
To: linux-omap, linux-fbdev, Jassi Brar; +Cc: Tomi Valkeinen, Grazvydas Ignotas
In-Reply-To: <1340807697-24847-1-git-send-email-tomi.valkeinen@ti.com>
If runtime PM is not enabled in the kernel config, pm_runtime_get_sync()
will always return 1 and pm_runtime_put_sync() will always return
-ENOSYS. pm_runtime_get_sync() returning 1 presents no problem to the
driver, but -ENOSYS from pm_runtime_put_sync() causes the driver to
print a warning.
One option would be to ignore errors returned by pm_runtime_put_sync()
totally, as they only say that the call was unable to put the hardware
into suspend mode.
However, I chose to ignore the returned -ENOSYS explicitly, and print a
warning for other errors, as I think we should get notified if the HW
failed to go to suspend properly.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Cc: Grazvydas Ignotas <notasas@gmail.com>
---
drivers/video/omap2/dss/dispc.c | 2 +-
drivers/video/omap2/dss/dsi.c | 2 +-
drivers/video/omap2/dss/dss.c | 2 +-
drivers/video/omap2/dss/hdmi.c | 2 +-
drivers/video/omap2/dss/rfbi.c | 2 +-
drivers/video/omap2/dss/venc.c | 2 +-
6 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 4749ac3..397d4ee 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -384,7 +384,7 @@ void dispc_runtime_put(void)
DSSDBG("dispc_runtime_put\n");
r = pm_runtime_put_sync(&dispc.pdev->dev);
- WARN_ON(r < 0);
+ WARN_ON(r < 0 && r != -ENOSYS);
}
static inline bool dispc_mgr_is_lcd(enum omap_channel channel)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 2a0055e..084906c 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1075,7 +1075,7 @@ void dsi_runtime_put(struct platform_device *dsidev)
DSSDBG("dsi_runtime_put\n");
r = pm_runtime_put_sync(&dsi->pdev->dev);
- WARN_ON(r < 0);
+ WARN_ON(r < 0 && r != -ENOSYS);
}
/* source clock for DSI PLL. this could also be PCLKFREE */
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 7706323..d2b5719 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -731,7 +731,7 @@ static void dss_runtime_put(void)
DSSDBG("dss_runtime_put\n");
r = pm_runtime_put_sync(&dss.pdev->dev);
- WARN_ON(r < 0 && r != -EBUSY);
+ WARN_ON(r < 0 && r != -ENOSYS && r != -EBUSY);
}
/* DEBUGFS */
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index de6920b..ae1688d 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -138,7 +138,7 @@ static void hdmi_runtime_put(void)
DSSDBG("hdmi_runtime_put\n");
r = pm_runtime_put_sync(&hdmi.pdev->dev);
- WARN_ON(r < 0);
+ WARN_ON(r < 0 && r != -ENOSYS);
}
static int __init hdmi_init_display(struct omap_dss_device *dssdev)
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 3d8c206..7985fa1 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -141,7 +141,7 @@ static void rfbi_runtime_put(void)
DSSDBG("rfbi_runtime_put\n");
r = pm_runtime_put_sync(&rfbi.pdev->dev);
- WARN_ON(r < 0);
+ WARN_ON(r < 0 && r != -ENOSYS);
}
void rfbi_bus_lock(void)
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index fd37440..416d478 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -402,7 +402,7 @@ static void venc_runtime_put(void)
DSSDBG("venc_runtime_put\n");
r = pm_runtime_put_sync(&venc.pdev->dev);
- WARN_ON(r < 0);
+ WARN_ON(r < 0 && r != -ENOSYS);
}
static const struct venc_config *venc_timings_to_config(
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Jassi Brar @ 2012-06-27 14:56 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340784821.2649.17.camel@deskari>
On 27 June 2012 13:43, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> I don't like it at all that omapdss disables and enables the panels in
> omapdss's suspend/resume hooks. But I'm not sure how this should work...
> Should panel drivers each have their own suspend/resume hooks, and
> handle it themselves? Or should the call to suspend/resume come from
> upper layers, like omapfb or omapdrm.
>
> I made a prototype patch a few weeks ago to move the suspend to omapfb,
> and it feels better than the current one, but I'm still not sure...
>
IIUC, I have similar opinion.
Each panel having its own suspend/resume sounds like inter-dependency trouble.
I too would prefer suspend/resume propagating from omap-fb/drm, which
imho fits better with the notion of a linux device(omapdss is only
backend). Though I don't have strong feelings about how core then take
various panels up/down optimally.
BR.
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: Use PM notifiers for system suspend
From: Jassi Brar @ 2012-06-27 15:32 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1340807697-24847-1-git-send-email-tomi.valkeinen@ti.com>
On 27 June 2012 20:04, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> The current way how omapdss handles system suspend and resume is that
> omapdss device (a platform device, which is not part of the device
> hierarchy of the DSS HW devices, like DISPC and DSI, or panels.) uses
> the suspend and resume callbacks from platform_driver to handle system
> suspend. It does this by disabling all enabled panels on suspend, and
> resuming the previously disabled panels on resume.
>
> This presents a few problems.
>
> One is that as omapdss device is not related to the panel devices or the
> DSS HW devices, there's no ordering in the suspend process. This means
> that suspend could be first ran for DSS HW devices and panels, and only
> then for omapdss device. Currently this is not a problem, as DSS HW
> devices and panels do not handle suspend.
>
> Another, more pressing problem, is that when suspending or resuming, the
> runtime PM functions return -EACCES as runtime PM is disabled during
> system suspend. This causes the driver to print warnings, and operations
> to fail as they think that they failed to bring up the HW.
>
> This patch changes the omapdss suspend handling to use PM notifiers,
> which are called before suspend and after resume. This way we have a
> normally functioning system when we are suspending and resuming the
> panels.
>
> This patch, I believe, creates a problem that somebody could enable or
> disable a panel between PM_SUSPEND_PREPARE and the system suspend, and
> similarly the other way around in resume. I choose to ignore the problem
> for now, as it sounds rather unlikely, and if it happens, it's not
> fatal.
>
> In the long run the system suspend handling of omapdss and panels should
> be thought out properly. The current approach feels rather hacky.
> Perhaps the panel drivers should handle system suspend, or the users of
> omapdss (omapfb, omapdrm) should handle system suspend.
>
> Note that after this patch we could probably revert
> 0eaf9f52e94f756147dbfe1faf1f77a02378dbf9 (OMAPDSS: use sync versions of
> pm_runtime_put).
>
I would think only DISPC should need the sync version.
> But as I said, this patch may be temporary, so let's
> leave the sync version still in place.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reported-by: Jassi Brar <jaswinder.singh@linaro.org>
>
Please feel free to add
Tested-by: Jassi Brar <jaswinder.singh@linaro.org>
^ 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