Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
On Mon, Oct 03, 2011 at 11:34:34AM -0600, Paul Walmsley wrote: + devicetree-discuss, lkml On Mon, 3 Oct 2011, Cousson, Benoit wrote: But at that time, device tree was not there... Now, the whole dev_attr stuff will be replaced because device tree is able to provide the driver any kind of custom information that can be retrieved directly from the driver without having to use a pdata in between. So I'm not sure it worth spending too much time on that feature stuff. As an example here is the ongoing GPIO DT migration: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg56505.html 3.2 will have the basic DT support using hwmod as a backend, but the idea is that for 3.3, we start removing some information from hwmod to rely on device tree only. One comment here though -- and I will make this comment on the original series too -- is that we should avoid adding direct DT dependencies into the driver. Specifically, these of_get_property() and of_property*() calls in the driver aren't right. We need some way of doing this that is completely independent from the device data format. Some way that does not care whether the input data is coming from DT, platform_data, ACPI, or whatever the new flavor of the year will be next year. Or we need to declare that these of_*() calls are not DT-specific, and define them as hooks that the device data format code can handle as it pleases. Generally, I agree. For example, I've been thinking of either modifying or creating bus_type-agnostic variants of the platform_get_*() hooks so that the driver can get the data it needs without knowing what the data source is. (Actually, it already works that way for platform_devices and DT, but that is only because the DT code populates the resource table when the device is created). This works best for well understood things like GPIOs, IRQs, memory ranges, and the like. It doesn't really work very well for data specific to the device. g. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Paul,
On Sun, 2011-10-02 at 22:45 -0600, Paul Walmsley wrote:
Hi
some comments:
On Mon, 12 Sep 2011, Archit Taneja wrote:
Resetting DISPC when a DISPC output is enabled causes the DSS to go into an
inconsistent state. Thus if the bootloader has enabled a display, the hwmod
code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay
manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The
issue
is resolved with this patch.
snip
+struct omap_dss_dispc_dev_attr {
+ u8 manager_count;
+ boolhas_framedonetv_irq;
+};
+
+#endif
diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
index 09d9395..8e32cb3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
@@ -945,6 +945,7 @@ static struct omap_hwmod omap2420_dss_dispc_hwmod = {
.slaves_cnt = ARRAY_SIZE(omap2420_dss_dispc_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2420),
.flags = HWMOD_NO_IDLEST,
+ .dev_attr = omap2_3_dss_dispc_dev_attr
};
I didn't know you can add arbitrary data like that to hwmods. What kind
of data is it meant for? Can the data be used by the driver, or is it
meant just for arch stuff?
I'm wondering this as we have a complex mechanism in the dss driver to
find out about the differences of DSS hardware
(drivers/video/omap2/dss/dss_features.[ch]). The dss_features system is
currently part of the driver, but should be moved under arch/arm/*omap*
at some point, and this hwmod dev_attr sounds like it could possibly be
a right place to handle these.
I looked at how the dev_attrs are used, and all of them seemed to be
very small, a few fields at max. The DSS features set is, on the other
hand, quite big amount of data, and meant for the driver.
Tomi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Tomi,
On 10/3/2011 10:21 AM, Valkeinen, Tomi wrote:
Hi Paul,
On Sun, 2011-10-02 at 22:45 -0600, Paul Walmsley wrote:
[...]
+struct omap_dss_dispc_dev_attr {
+ u8 manager_count;
+ boolhas_framedonetv_irq;
+};
+
+#endif
diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
index 09d9395..8e32cb3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
@@ -945,6 +945,7 @@ static struct omap_hwmod omap2420_dss_dispc_hwmod = {
.slaves_cnt = ARRAY_SIZE(omap2420_dss_dispc_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2420),
.flags = HWMOD_NO_IDLEST,
+ .dev_attr =omap2_3_dss_dispc_dev_attr
};
I didn't know you can add arbitrary data like that to hwmods. What kind
of data is it meant for? Can the data be used by the driver, or is it
meant just for arch stuff?
It was added in order to add HW related information for an IP.
So most of the time, this is use for IP version, since this information
is not necessarily accessible from the IP itself. Some time it can be
the number of entries in the mailbox IP that will change depending of
the version too.
I'm wondering this as we have a complex mechanism in the dss driver to
find out about the differences of DSS hardware
(drivers/video/omap2/dss/dss_features.[ch]). The dss_features system is
currently part of the driver, but should be moved under arch/arm/*omap*
at some point, and this hwmod dev_attr sounds like it could possibly be
a right place to handle these.
Please note that I made that kind of comment to Archit when he started
submitted this dss_feature series. That feature management mechanism
could have been useful for any other IPs / driver at that time.
I looked at how the dev_attrs are used, and all of them seemed to be
very small, a few fields at max. The DSS features set is, on the other
hand, quite big amount of data, and meant for the driver.
That's why, most of the time, only the version is in the dev_attr, and
the various information that will depend of that version are stored in
the driver.
But at that time, device tree was not there...
Now, the whole dev_attr stuff will be replaced because device tree is
able to provide the driver any kind of custom information that can be
retrieved directly from the driver without having to use a pdata in
between. So I'm not sure it worth spending too much time on that feature
stuff.
As an example here is the ongoing GPIO DT migration:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg56505.html
3.2 will have the basic DT support using hwmod as a backend, but the
idea is that for 3.3, we start removing some information from hwmod to
rely on device tree only.
Regards,
Benoit
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Tomi,
On Mon, 3 Oct 2011, Tomi Valkeinen wrote:
On Sun, 2011-10-02 at 22:45 -0600, Paul Walmsley wrote:
On Mon, 12 Sep 2011, Archit Taneja wrote:
Resetting DISPC when a DISPC output is enabled causes the DSS to go into
an
inconsistent state. Thus if the bootloader has enabled a display, the
hwmod code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay
manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when
the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The
issue
is resolved with this patch.
snip
+struct omap_dss_dispc_dev_attr {
+ u8 manager_count;
+ boolhas_framedonetv_irq;
+};
+
+#endif
diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
index 09d9395..8e32cb3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
@@ -945,6 +945,7 @@ static struct omap_hwmod omap2420_dss_dispc_hwmod = {
.slaves_cnt = ARRAY_SIZE(omap2420_dss_dispc_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2420),
.flags = HWMOD_NO_IDLEST,
+ .dev_attr = omap2_3_dss_dispc_dev_attr
};
I didn't know you can add arbitrary data like that to hwmods. What kind
of data is it meant for?
It's intended for data that's specific to the integration of that IP block
on a given SoC.
In an ideal world, Linux could just read some registers from the IP block
to determine these. Many of our IP blocks have a REVISION register. But
many types of integration-specific data are not identified in that
register. And often when IP blocks are revised, the hardware people seem
to forget to update the REVISION register :-(. So we need some way to
supply this information in software.
In terms of concrete uses, one use would be to identify IP block
features that may be enabled on certain instances of the IP. For example,
on OMAPs, some DMTIMER blocks have 1ms tick adjustment support; others do
not. As far as I know, there's no way for the driver to determine this
from the IP block.
The dev_attr data is intended to be fairly high-level functional data.
Can the data be used by the driver, or is it meant just for arch stuff?
It can be used by both. But if it's intended for use by the driver, then
the dev_attr data needs to be copied into struct platform_data by the
arch-specific code. This is because the drivers themselves should have no
direct dependencies on hwmod data or code, in case the IP block is used on
a non-OMAP SoC.
For example, if you look at arch/arm/mach-omap2/hsmmc.c around line 471,
you can see the arch-specific code copying dev_attr data into
platform_data.
I'm wondering this as we have a complex mechanism in the dss driver to
find out about the differences of DSS hardware
(drivers/video/omap2/dss/dss_features.[ch]). The dss_features system is
currently part of the driver, but should be moved under arch/arm/*omap*
at some point, and this hwmod dev_attr sounds like it could possibly be
a right place to handle these.
I looked at how the dev_attrs are used, and all of them seemed to be
very small, a few fields at max. The DSS features set is, on the other
hand, quite big amount of data, and meant for the driver.
Just from a brief look, it looks like much of that data would be good
candidates to pass via the dev_attr mechanism. The reg_fields would be one
exception: it would be better (in terms of dev_attr) to simply pass data
like .reg_field_layout = 1 or .reg_field_layout = 2, and then select
between those tables in the driver code.
Benoît is right, though, that you might want to take the upcoming DT
migration into account in your plans.
- Paul
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
+ devicetree-discuss, lkml On Mon, 3 Oct 2011, Cousson, Benoit wrote: But at that time, device tree was not there... Now, the whole dev_attr stuff will be replaced because device tree is able to provide the driver any kind of custom information that can be retrieved directly from the driver without having to use a pdata in between. So I'm not sure it worth spending too much time on that feature stuff. As an example here is the ongoing GPIO DT migration: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg56505.html 3.2 will have the basic DT support using hwmod as a backend, but the idea is that for 3.3, we start removing some information from hwmod to rely on device tree only. One comment here though -- and I will make this comment on the original series too -- is that we should avoid adding direct DT dependencies into the driver. Specifically, these of_get_property() and of_property*() calls in the driver aren't right. We need some way of doing this that is completely independent from the device data format. Some way that does not care whether the input data is coming from DT, platform_data, ACPI, or whatever the new flavor of the year will be next year. Or we need to declare that these of_*() calls are not DT-specific, and define them as hooks that the device data format code can handle as it pleases. Otherwise we'll need shim layers for each new data format in the driver code and that will be a huge and unnecessary mess. - Paul -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Archit,
thanks for the quick and informative response -
On Mon, 3 Oct 2011, Archit Taneja wrote:
On Monday 03 October 2011 10:15 AM, Paul Walmsley wrote:
On Mon, 12 Sep 2011, Archit Taneja wrote:
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 93db7c1..eab81f4 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -30,6 +30,30 @@
#include control.h
+#define DISPC_BASE_OMAP2 0x48050400
+#define DISPC_BASE_OMAP4 0x48041000
These should definitely not be needed -- they can be obtained from the
hwmod data and there is clearly something wrong if any IP block addresses
exist outside of those data files.
The reason we had to do this was because the function omap_dss_reset()
is tied to the dss hwmod and not dispc hwmod. Hence, we don't have
DISPC related info through the hwmod struct available through
omap_dss_reset().
You're right. My replacement patch is broken in that regard.
Could you suggest how we could go about this? Is there a way to access dispc
hwmod's data in dss hwmod's custom reset function?
There is. The dss hwmod's custom reset function can call
omap_hwmod_lookup() for the dss_dispc hwmod. It's not the best long term
solution, but should work until we can determine the best way to handle
these types of subsystem resets with hwmod and/or DT. Revised patch
below.
I agree with all the other comments and things you have done in the patch you
made. Thanks for the thorough review and the patch, i'll keep these comments
in mind for future.
Thanks for looking over the revised patch and correcting my mistake,
- Paul
From: Archit Taneja arc...@ti.com
Date: Mon, 12 Sep 2011 12:38:26 +0530
Subject: [PATCH] ARM: OMAP2PLUS: DSS: Ensure DSS works correctly if display
is enabled in bootloader
Resetting DISPC when a DISPC output is enabled causes the DSS to go into an
inconsistent state. Thus if the bootloader has enabled a display, the hwmod code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The issue
is resolved with this patch.
Acked-by: Tomi Valkeinen tomi.valkei...@ti.com
Tested-by: R, Sricharan r.sricha...@ti.com
Signed-off-by: Archit Taneja arc...@ti.com
[p...@pwsan.com: restructured code, removed omap_{read,write}l(), removed
cpu_is_omap*() calls and converted to dev_attr]
Signed-off-by: Paul Walmsley p...@pwsan.com
---
arch/arm/mach-omap2/display.c| 125 ++
arch/arm/mach-omap2/display.h| 29 ++
arch/arm/mach-omap2/omap_hwmod_2420_data.c |1 +
arch/arm/mach-omap2/omap_hwmod_2430_data.c |1 +
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |1 +
arch/arm/mach-omap2/omap_hwmod_44xx_data.c |6 ++
arch/arm/mach-omap2/omap_hwmod_common_data.c |4 +
arch/arm/mach-omap2/omap_hwmod_common_data.h |4 +
8 files changed, 171 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-omap2/display.h
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index cdb675a..3bf8dbe 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -28,6 +28,33 @@
#include plat/omap-pm.h
#include plat/common.h
+#include display.h
+
+#define DISPC_CONTROL 0x0040
+#define DISPC_CONTROL2 0x0238
+#define DISPC_IRQSTATUS0x0018
+
+#define DSS_SYSCONFIG 0x10
+#define DSS_SYSSTATUS 0x14
+#define DSS_CONTROL0x40
+#define DSS_SDI_CONTROL0x44
+#define DSS_PLL_CONTROL0x48
+
+#define LCD_EN_MASK(0x1 0)
+#define DIGIT_EN_MASK (0x1 1)
+
+#define FRAMEDONE_IRQ_SHIFT0
+#define EVSYNC_EVEN_IRQ_SHIFT 2
+#define EVSYNC_ODD_IRQ_SHIFT 3
+#define FRAMEDONE2_IRQ_SHIFT 22
+#define FRAMEDONETV_IRQ_SHIFT 24
+
+/*
+ * FRAMEDONE_IRQ_TIMEOUT: how long (in milliseconds) to wait during DISPC
+ * reset before deciding that something has gone wrong
+ */
+#define FRAMEDONE_IRQ_TIMEOUT 100
+
static struct platform_device omap_display_device = {
.name = omapdss,
.id= -1,
@@ -128,6 +155,90 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
return r;
}
+static void dispc_disable_outputs(void)
+{
+ u32 v, irq_mask = 0;
+ bool lcd_en, digit_en, lcd2_en = false;
+ int i;
+ struct omap_dss_dispc_dev_attr *da;
+ struct omap_hwmod *oh;
+
+ oh =
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi
some comments:
On Mon, 12 Sep 2011, Archit Taneja wrote:
Resetting DISPC when a DISPC output is enabled causes the DSS to go into an
inconsistent state. Thus if the bootloader has enabled a display, the hwmod
code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The
issue
is resolved with this patch.
Acked-by: Tomi Valkeinen tomi.valkei...@ti.com
Tested-by: R, Sricharan r.sricha...@ti.com
Signed-off-by: Archit Taneja arc...@ti.com
---
v2:
- Added more info in the commit message, fixed some typos.
The patch depends on a HWMOD patch series which has been posted by Tomi, it
can
be tested by applying over the following branch:
https://gitorious.org/linux-omap-dss2/linux/commits/master
arch/arm/mach-omap2/display.c | 110
+
1 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 93db7c1..eab81f4 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -30,6 +30,30 @@
#include control.h
+#define DISPC_BASE_OMAP2 0x48050400
+#define DISPC_BASE_OMAP4 0x48041000
These should definitely not be needed -- they can be obtained from the
hwmod data and there is clearly something wrong if any IP block addresses
exist outside of those data files.
+
+#define DISPC_REG(base, offset) (base + offset)
+
+#define DISPC_CONTROL0x0040
+#define DISPC_CONTROL2 0x0238
+#define DISPC_IRQSTATUS 0x0018
+
+#define DSS_SYSCONFIG0x10
+#define DSS_SYSSTATUS0x14
+#define DSS_CONTROL 0x40
+#define DSS_SDI_CONTROL 0x44
+#define DSS_PLL_CONTROL 0x48
+
+#define LCD_EN_MASK (0x1 0)
+#define DIGIT_EN_MASK(0x1 1)
+
+#define FRAMEDONE_IRQ_SHIFT 0
+#define EVSYNC_EVEN_IRQ_SHIFT2
+#define EVSYNC_ODD_IRQ_SHIFT 3
+#define FRAMEDONE2_IRQ_SHIFT 22
+#define FRAMEDONETV_IRQ_SHIFT24
+
static struct platform_device omap_display_device = {
.name = omapdss,
.id= -1,
@@ -182,6 +206,78 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
return r;
}
+static void dispc_disable_outputs(void)
+{
+ u32 val, irq_mask, base;
+ bool lcd_en, digit_en, lcd2_en = false;
+ int i, num_mgrs;
+
+ if (cpu_is_omap44xx()) {
+ base = DISPC_BASE_OMAP4;
+ num_mgrs = 3;
+ } else {
+ base = DISPC_BASE_OMAP2;
+ num_mgrs = 2;
+ }
base should not be needed here. The num_mgrs should come from the hwmod
data. We're trying to get rid of cpu_is_omap*() functions wherever
possible.
+
+ /* store value of LCDENABLE and DIGITENABLE bits */
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL));
omap_{read,write}l() are deprecated and should no longer be used. This
code can use omap_hwmod_{read,write}() instead. You can pass the struct
omap_hwmod * into this function from the caller.
+ lcd_en = val LCD_EN_MASK;
+ digit_en = val DIGIT_EN_MASK;
+
+ /* store value of LCDENABLE for LCD2 */
+ if (num_mgrs 2) {
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL2));
+ lcd2_en = val LCD_EN_MASK;
+ }
+
+ /*
+ * If any manager was enabled, we need to disable it before DSS clocks
+ * are disabled or DISPC module is reset
+ */
+ if (lcd_en || digit_en || lcd2_en) {
Rather than this massive if block, please test the inverse condition and
bail out early. This avoids unnecessary indentation levels that make code
harder to read.
+ irq_mask = (lcd_en ? 1 : 0) FRAMEDONE_IRQ_SHIFT;
+
+ if (cpu_is_omap44xx())
+ irq_mask |= (digit_en ? 1 : 0) FRAMEDONETV_IRQ_SHIFT;
+ else
+ irq_mask |= (digit_en ? 1 : 0) EVSYNC_EVEN_IRQ_SHIFT
|
+ (digit_en ? 1 : 0) EVSYNC_ODD_IRQ_SHIFT;
Rather than a cpu_is_omap*() test, the presence of a working FRAMEDONETV
interrupt bit should be passed through the hwmod data.
+
+ irq_mask |= (lcd2_en ? 1 : 0) FRAMEDONE2_IRQ_SHIFT;
+
+ /*
+ * clear any previous FRAMEDONE, FRAMEDONETV, EVSYNC_EVEN/ODD
+ * or FRAMEDONE2 interrupts
+ */
+ omap_writel(irq_mask, DISPC_REG(base,
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Paul,
On Monday 03 October 2011 10:15 AM, Paul Walmsley wrote:
Hi
some comments:
On Mon, 12 Sep 2011, Archit Taneja wrote:
Resetting DISPC when a DISPC output is enabled causes the DSS to go into an
inconsistent state. Thus if the bootloader has enabled a display, the hwmod code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The issue
is resolved with this patch.
Acked-by: Tomi Valkeinentomi.valkei...@ti.com
Tested-by: R, Sricharanr.sricha...@ti.com
Signed-off-by: Archit Tanejaarc...@ti.com
---
v2:
- Added more info in the commit message, fixed some typos.
The patch depends on a HWMOD patch series which has been posted by Tomi, it can
be tested by applying over the following branch:
https://gitorious.org/linux-omap-dss2/linux/commits/master
arch/arm/mach-omap2/display.c | 110 +
1 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 93db7c1..eab81f4 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -30,6 +30,30 @@
#include control.h
+#define DISPC_BASE_OMAP2 0x48050400
+#define DISPC_BASE_OMAP4 0x48041000
These should definitely not be needed -- they can be obtained from the
hwmod data and there is clearly something wrong if any IP block addresses
exist outside of those data files.
The reason we had to do this was because the function omap_dss_reset()
is tied to the dss hwmod and not dispc hwmod. Hence, we don't have DISPC
related info through the hwmod struct available through omap_dss_reset().
It would have been easy(and more sensible) if we had tied the code in
dispc_disable_outputs() to a custom reset function for the dispc hwmod
directly, but that unfortunately breaks some functionality. The current
omap_dss_reset() function does this:
- enable DSS opt clocks to complete power on reset.
- see if the power on reset is completed by reading DSS_SYSNCONG
- disable DSS opt clocks
If we don't do the things done in dispc_disable_outputs() before
disabling DSS opt clocks, we would be in trouble.
Hence, there is a need to access DISPC registers in the dss hwmod
itself, this forced us to create the base macros and the use of
omap_readl/writel functions.
I considered changing the order in which the hwmods are registered, i.e
dispc first and dss later, but that didn't seem right
Could you suggest how we could go about this? Is there a way to access
dispc hwmod's data in dss hwmod's custom reset function?
I agree with all the other comments and things you have done in the
patch you made. Thanks for the thorough review and the patch, i'll keep
these comments in mind for future.
Regards,
Archit
+
+#define DISPC_REG(base, offset) (base + offset)
+
+#define DISPC_CONTROL0x0040
+#define DISPC_CONTROL2 0x0238
+#define DISPC_IRQSTATUS 0x0018
+
+#define DSS_SYSCONFIG0x10
+#define DSS_SYSSTATUS0x14
+#define DSS_CONTROL 0x40
+#define DSS_SDI_CONTROL 0x44
+#define DSS_PLL_CONTROL 0x48
+
+#define LCD_EN_MASK (0x1 0)
+#define DIGIT_EN_MASK(0x1 1)
+
+#define FRAMEDONE_IRQ_SHIFT 0
+#define EVSYNC_EVEN_IRQ_SHIFT2
+#define EVSYNC_ODD_IRQ_SHIFT 3
+#define FRAMEDONE2_IRQ_SHIFT 22
+#define FRAMEDONETV_IRQ_SHIFT24
+
static struct platform_device omap_display_device = {
.name = omapdss,
.id= -1,
@@ -182,6 +206,78 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
return r;
}
+static void dispc_disable_outputs(void)
+{
+ u32 val, irq_mask, base;
+ bool lcd_en, digit_en, lcd2_en = false;
+ int i, num_mgrs;
+
+ if (cpu_is_omap44xx()) {
+ base = DISPC_BASE_OMAP4;
+ num_mgrs = 3;
+ } else {
+ base = DISPC_BASE_OMAP2;
+ num_mgrs = 2;
+ }
base should not be needed here. The num_mgrs should come from the hwmod
data. We're trying to get rid of cpu_is_omap*() functions wherever
possible.
+
+ /* store value of LCDENABLE and DIGITENABLE bits */
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL));
omap_{read,write}l() are deprecated and should no longer be used. This
code can use omap_hwmod_{read,write}() instead. You can pass the struct
omap_hwmod * into this function from the caller.
+ lcd_en = val LCD_EN_MASK;
+
Re: [PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Paul,
On Monday 12 September 2011 12:38 PM, Taneja, Archit wrote:
Resetting DISPC when a DISPC output is enabled causes the DSS to go into an
inconsistent state. Thus if the bootloader has enabled a display, the hwmod code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The issue
is resolved with this patch.
Is it possible to get this in for the next merge window? It applies over
your branch hwmod_dss_fixes_3.2.
Thanks,
Archit
Acked-by: Tomi Valkeinentomi.valkei...@ti.com
Tested-by: R, Sricharanr.sricha...@ti.com
Signed-off-by: Archit Tanejaarc...@ti.com
---
v2:
- Added more info in the commit message, fixed some typos.
The patch depends on a HWMOD patch series which has been posted by Tomi, it can
be tested by applying over the following branch:
https://gitorious.org/linux-omap-dss2/linux/commits/master
arch/arm/mach-omap2/display.c | 110 +
1 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 93db7c1..eab81f4 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -30,6 +30,30 @@
#include control.h
+#define DISPC_BASE_OMAP2 0x48050400
+#define DISPC_BASE_OMAP4 0x48041000
+
+#define DISPC_REG(base, offset)(base + offset)
+
+#define DISPC_CONTROL 0x0040
+#define DISPC_CONTROL2 0x0238
+#define DISPC_IRQSTATUS0x0018
+
+#define DSS_SYSCONFIG 0x10
+#define DSS_SYSSTATUS 0x14
+#define DSS_CONTROL0x40
+#define DSS_SDI_CONTROL0x44
+#define DSS_PLL_CONTROL0x48
+
+#define LCD_EN_MASK(0x1 0)
+#define DIGIT_EN_MASK (0x1 1)
+
+#define FRAMEDONE_IRQ_SHIFT0
+#define EVSYNC_EVEN_IRQ_SHIFT 2
+#define EVSYNC_ODD_IRQ_SHIFT 3
+#define FRAMEDONE2_IRQ_SHIFT 22
+#define FRAMEDONETV_IRQ_SHIFT 24
+
static struct platform_device omap_display_device = {
.name = omapdss,
.id= -1,
@@ -182,6 +206,78 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
return r;
}
+static void dispc_disable_outputs(void)
+{
+ u32 val, irq_mask, base;
+ bool lcd_en, digit_en, lcd2_en = false;
+ int i, num_mgrs;
+
+ if (cpu_is_omap44xx()) {
+ base = DISPC_BASE_OMAP4;
+ num_mgrs = 3;
+ } else {
+ base = DISPC_BASE_OMAP2;
+ num_mgrs = 2;
+ }
+
+ /* store value of LCDENABLE and DIGITENABLE bits */
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL));
+ lcd_en = val LCD_EN_MASK;
+ digit_en = val DIGIT_EN_MASK;
+
+ /* store value of LCDENABLE for LCD2 */
+ if (num_mgrs 2) {
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL2));
+ lcd2_en = val LCD_EN_MASK;
+ }
+
+ /*
+* If any manager was enabled, we need to disable it before DSS clocks
+* are disabled or DISPC module is reset
+*/
+ if (lcd_en || digit_en || lcd2_en) {
+ irq_mask = (lcd_en ? 1 : 0) FRAMEDONE_IRQ_SHIFT;
+
+ if (cpu_is_omap44xx())
+ irq_mask |= (digit_en ? 1 : 0) FRAMEDONETV_IRQ_SHIFT;
+ else
+ irq_mask |= (digit_en ? 1 : 0) EVSYNC_EVEN_IRQ_SHIFT
|
+ (digit_en ? 1 : 0) EVSYNC_ODD_IRQ_SHIFT;
+
+ irq_mask |= (lcd2_en ? 1 : 0) FRAMEDONE2_IRQ_SHIFT;
+
+ /*
+* clear any previous FRAMEDONE, FRAMEDONETV, EVSYNC_EVEN/ODD
+* or FRAMEDONE2 interrupts
+*/
+ omap_writel(irq_mask, DISPC_REG(base, DISPC_IRQSTATUS));
+
+ /* disable LCD and TV managers */
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL));
+ val= ~(LCD_EN_MASK | DIGIT_EN_MASK);
+ omap_writel(val, DISPC_REG(base, DISPC_CONTROL));
+
+ /* disable LCD2 manager */
+ if (num_mgrs 2) {
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL2));
+ val= ~LCD_EN_MASK;
+ omap_writel(val, DISPC_REG(base, DISPC_CONTROL2));
+ }
+
+ i = 0;
+ while ((omap_readl(DISPC_REG(base, DISPC_IRQSTATUS))
irq_mask) !=
+ irq_mask) {
+ i++;
+ if (i 100) {
+
[PATCH v2] OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Resetting DISPC when a DISPC output is enabled causes the DSS to go into an
inconsistent state. Thus if the bootloader has enabled a display, the hwmod code
cannot reset the DISPC module just like that, but the outputs need to be
disabled first.
Add function dispc_disable_outputs() which disables all active overlay manager
and ensure all frame transfers are completed.
Modify omap_dss_reset() to call this function and clear DSS_CONTROL,
DSS_SDI_CONTROL and DSS_PLL_CONTROL so that DSS is in a clean state when the
DSS2 driver starts.
This resolves the hang issue(caused by a L3 error during boot) seen on the
beagle board C3, which has a factory bootloader that enables display. The issue
is resolved with this patch.
Acked-by: Tomi Valkeinen tomi.valkei...@ti.com
Tested-by: R, Sricharan r.sricha...@ti.com
Signed-off-by: Archit Taneja arc...@ti.com
---
v2:
- Added more info in the commit message, fixed some typos.
The patch depends on a HWMOD patch series which has been posted by Tomi, it can
be tested by applying over the following branch:
https://gitorious.org/linux-omap-dss2/linux/commits/master
arch/arm/mach-omap2/display.c | 110 +
1 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 93db7c1..eab81f4 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -30,6 +30,30 @@
#include control.h
+#define DISPC_BASE_OMAP2 0x48050400
+#define DISPC_BASE_OMAP4 0x48041000
+
+#define DISPC_REG(base, offset)(base + offset)
+
+#define DISPC_CONTROL 0x0040
+#define DISPC_CONTROL2 0x0238
+#define DISPC_IRQSTATUS0x0018
+
+#define DSS_SYSCONFIG 0x10
+#define DSS_SYSSTATUS 0x14
+#define DSS_CONTROL0x40
+#define DSS_SDI_CONTROL0x44
+#define DSS_PLL_CONTROL0x48
+
+#define LCD_EN_MASK(0x1 0)
+#define DIGIT_EN_MASK (0x1 1)
+
+#define FRAMEDONE_IRQ_SHIFT0
+#define EVSYNC_EVEN_IRQ_SHIFT 2
+#define EVSYNC_ODD_IRQ_SHIFT 3
+#define FRAMEDONE2_IRQ_SHIFT 22
+#define FRAMEDONETV_IRQ_SHIFT 24
+
static struct platform_device omap_display_device = {
.name = omapdss,
.id= -1,
@@ -182,6 +206,78 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
return r;
}
+static void dispc_disable_outputs(void)
+{
+ u32 val, irq_mask, base;
+ bool lcd_en, digit_en, lcd2_en = false;
+ int i, num_mgrs;
+
+ if (cpu_is_omap44xx()) {
+ base = DISPC_BASE_OMAP4;
+ num_mgrs = 3;
+ } else {
+ base = DISPC_BASE_OMAP2;
+ num_mgrs = 2;
+ }
+
+ /* store value of LCDENABLE and DIGITENABLE bits */
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL));
+ lcd_en = val LCD_EN_MASK;
+ digit_en = val DIGIT_EN_MASK;
+
+ /* store value of LCDENABLE for LCD2 */
+ if (num_mgrs 2) {
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL2));
+ lcd2_en = val LCD_EN_MASK;
+ }
+
+ /*
+* If any manager was enabled, we need to disable it before DSS clocks
+* are disabled or DISPC module is reset
+*/
+ if (lcd_en || digit_en || lcd2_en) {
+ irq_mask = (lcd_en ? 1 : 0) FRAMEDONE_IRQ_SHIFT;
+
+ if (cpu_is_omap44xx())
+ irq_mask |= (digit_en ? 1 : 0) FRAMEDONETV_IRQ_SHIFT;
+ else
+ irq_mask |= (digit_en ? 1 : 0) EVSYNC_EVEN_IRQ_SHIFT
|
+ (digit_en ? 1 : 0) EVSYNC_ODD_IRQ_SHIFT;
+
+ irq_mask |= (lcd2_en ? 1 : 0) FRAMEDONE2_IRQ_SHIFT;
+
+ /*
+* clear any previous FRAMEDONE, FRAMEDONETV, EVSYNC_EVEN/ODD
+* or FRAMEDONE2 interrupts
+*/
+ omap_writel(irq_mask, DISPC_REG(base, DISPC_IRQSTATUS));
+
+ /* disable LCD and TV managers */
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL));
+ val = ~(LCD_EN_MASK | DIGIT_EN_MASK);
+ omap_writel(val, DISPC_REG(base, DISPC_CONTROL));
+
+ /* disable LCD2 manager */
+ if (num_mgrs 2) {
+ val = omap_readl(DISPC_REG(base, DISPC_CONTROL2));
+ val = ~LCD_EN_MASK;
+ omap_writel(val, DISPC_REG(base, DISPC_CONTROL2));
+ }
+
+ i = 0;
+ while ((omap_readl(DISPC_REG(base, DISPC_IRQSTATUS))
irq_mask) !=
+ irq_mask) {
+ i++;
+ if (i 100) {
+ printk(KERN_ERR didn't get FRAMEDONE1/2 or TV
+interrupt\n);
+ break;
+ }
