OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Subsystems (4)
PATCH v2 1/1 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.
---
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(-)
arch/arm/mach-omap2/display.c +110 -0
@@ -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_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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 ; + } +mdelay (1 ); + } + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {
@@ -198,6 +294,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs (); + +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c);
Signature · 1 line
1.7.1
Hi Paul,
Quoted 8 lines · Archit
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.
Quoted 7 lines
Modify
omap_dss_reset()to call this function and clearDSS_CONTROL,DSS_SDI_CONTROLandDSS_PLL_CONTROLso 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
Quoted 152 lines
Acked-by Tomi Valkeinen<[email protected]>Tested-by R, Sricharan<[email protected]>Signed-off-by Archit Taneja<[email protected]>---
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(-)arch/arm/mach-omap2/display.c +110
-0@@ -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_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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 ; + } +mdelay (1 ); + } + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {@@ -198,6 +294,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs (); + +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c);
Paul Walmsley
maintainerPATCH v2 1/1 OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
In reply to Archit TanejaHi
some comments:
Quoted 37 lines · Archit Taneja
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 clearDSS_CONTROL,DSS_SDI_CONTROLandDSS_PLL_CONTROLso 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 <[email protected]>Tested-by R, Sricharan <[email protected]>Signed-off-by Archit Taneja <[email protected]>---
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(-)arch/arm/mach-omap2/display.c +0
-0
arch/arm/mach-omap2/display.c line 34
Quoted 5 lines
arch/arm/mach-omap2/display.c +2
-0@@ -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.
Quoted 32 lines
arch/arm/mach-omap2/display.c +25
-0+ +#define DISPC_REG (base, offset) (base + offset) + +#define DISPC_CONTROL 0x0040 +#define DISPC_CONTROL2 0x0238 +#define DISPC_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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;
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 221
Quoted 10 lines
arch/arm/mach-omap2/display.c +10
-0+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.
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 224
Quoted 3 lines
arch/arm/mach-omap2/display.c +3
-0+ +/* 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.
Quoted 4 lines
arch/arm/mach-omap2/display.c +4
-0+ lcd_en = val &LCD_EN_MASK ; + digit_en = val &DIGIT_EN_MASK ; + +/* store value of LCDENABLE for LCD2 */
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 238
Quoted 10 lines
arch/arm/mach-omap2/display.c +10
-0+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.
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 245
Quoted 7 lines
arch/arm/mach-omap2/display.c +7
-0+ 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.
Quoted 16 lines
arch/arm/mach-omap2/display.c +16
-0+ + 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 ) {
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 271
Quoted 10 lines
arch/arm/mach-omap2/display.c +10
-0+ 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 ) {
Please hoist this constant up to the top of this file, and use a macro with a comment.
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 273
Quoted 2 lines
arch/arm/mach-omap2/display.c +2
-0+printk (KERN_ERR "didn't get FRAMEDONE1/2 or TV" +" interrupt\n" );
pr_err() is shorter and better here.
Quoted 11 lines
arch/arm/mach-omap2/display.c +7
-0+break ; + } +mdelay (1 ); + } + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {
arch/arm/mach-omap2/display.c int omap_dss_reset(struct omap_hwmod *oh) · line 297
Quoted 4 lines
arch/arm/mach-omap2/display.c +1
-0@@ -198,6 +294,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs ();
Pass the struct omap_hwmod *oh in here.
Quoted 18 lines
arch/arm/mach-omap2/display.c +13
-0+ +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c);Signature · 1 line
1.7.1
In the interest of expediency, I've made the above changes to the patch -- updated patch below. The following Compile-tested only, so could you review it please and make sure I haven't broken anything? For future patches, please keep the comments above in mind.
thanks,
- Paul
From: Archit Taneja <[email protected]>
Date: Mon, 12 Sep 2011 12:38:26 +0530
Subject: [PATCH 1/2] 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.
[paul at pwsan.com: restructured code, removed omap_{read,write}l(), removed
cpu_is_omap*() calls and converted to dev_attr]
--- arch/arm/mach-omap2/display.c | 118 ++++++++++++++++++++++++++ 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, 164 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-omap2/display.h
arch/arm/mach-omap2/display.c +118 -0
@@ -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_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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,83 @@ int __init omap_display_init(struct omap_dss_board_info *board_data)return r; } +static void dispc_disable_outputs (struct omap_hwmod *oh) +{ +u32 v, irq_mask =0 ; +bool lcd_en, digit_en, lcd2_en = false; +int i; +struct omap_dss_dispc_dev_attr *da; + +if (!oh->dev_attr) { +pr_err ("display: could not disable outputs during reset due to missing dev_attr\n" ); +return ; + } + + da = (struct omap_dss_dispc_dev_attr *)oh->dev_attr; + +/* store value of LCDENABLE and DIGITENABLE bits */ + v =omap_hwmod_read (oh,DISPC_CONTROL ); + lcd_en = v &LCD_EN_MASK ; + digit_en = v &DIGIT_EN_MASK ; + +/* store value of LCDENABLE for LCD2 */ +if (da->manager_count >2 ) { + v =omap_hwmod_read (oh,DISPC_CONTROL2 ); + lcd2_en = v &LCD_EN_MASK ; + } + +if (!(lcd_en | digit_en | lcd2_en)) +return ;/* no managers currently enabled */ + +/* +* If any manager was enabled, we need to disable it before +* DSS clocks are disabled or DISPC module is reset +*/ +if (lcd_en) + irq_mask |=1 <<FRAMEDONE_IRQ_SHIFT ; + +if (digit_en) { +if (da->has_framedonetv_irq) { + irq_mask |=1 <<FRAMEDONETV_IRQ_SHIFT ; + }else { + irq_mask |=1 <<EVSYNC_EVEN_IRQ_SHIFT | +1 <<EVSYNC_ODD_IRQ_SHIFT ; + } + } + +if (lcd2_en) + irq_mask |=1 <<FRAMEDONE2_IRQ_SHIFT ; + +/* +* clear any previous FRAMEDONE, FRAMEDONETV, +* EVSYNC_EVEN/ODD or FRAMEDONE2 interrupts +*/ +omap_hwmod_write (irq_mask, oh,DISPC_IRQSTATUS ); + +/* disable LCD and TV managers */ + v =omap_hwmod_read (oh,DISPC_CONTROL ); + v &= ~(LCD_EN_MASK |DIGIT_EN_MASK ); +omap_hwmod_write (v, oh,DISPC_CONTROL ); + +/* disable LCD2 manager */ +if (da->manager_count >2 ) { + v =omap_hwmod_read (oh,DISPC_CONTROL2 ); + v &= ~LCD_EN_MASK ; +omap_hwmod_write (v, oh,DISPC_CONTROL2 ); + } + + i =0 ; +while ((omap_hwmod_read (oh,DISPC_IRQSTATUS ) & irq_mask) != + irq_mask) { + i++; +if (i >FRAMEDONE_IRQ_TIMEOUT ) { +pr_err ("didn't get FRAMEDONE1/2 or TV interrupt\n" ); +break ; + } +mdelay (1 ); + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {
@@ -144,6 +248,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs (oh); + +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c);
arch/arm/mach-omap2/display.h new file +29 -0
@@ -0,0 +1,29 @@ +/* +* display.h - OMAP2+ integration-specific DSS header +* +* Copyright (C) 2011 Texas Instruments, Inc. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License version 2 as published by +* the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License along with +* this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#ifndef __ARCH_ARM_MACH_OMAP2_DISPLAY_H +#define __ARCH_ARM_MACH_OMAP2_DISPLAY_H + +#include <linux/kernel.h> + +struct omap_dss_dispc_dev_attr { +u8 manager_count; +bool has_framedonetv_irq; +}; + +#endif
arch/arm/mach-omap2/omap_hwmod_2420_data.c +1 -0
@@ -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 };/* l4_core -> dss_rfbi */
arch/arm/mach-omap2/omap_hwmod_2430_data.c +1 -0
@@ -1005,6 +1005,7 @@ static struct omap_hwmod omap2430_dss_dispc_hwmod = { .slaves_cnt = ARRAY_SIZE(omap2430_dss_dispc_slaves), .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2430), .flags = HWMOD_NO_IDLEST, + .dev_attr = &omap2_3_dss_dispc_dev_attr };/* l4_core -> dss_rfbi */
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +1 -0
@@ -1465,6 +1465,7 @@ static struct omap_hwmod omap3xxx_dss_dispc_hwmod = {CHIP_GE_OMAP3430ES2 |CHIP_IS_OMAP3630ES1 | CHIP_GE_OMAP3630ES1_1), .flags = HWMOD_NO_IDLEST, + .dev_attr = &omap2_3_dss_dispc_dev_attr };/*
arch/arm/mach-omap2/omap_hwmod_44xx_data.c +6 -0
@@ -1345,6 +1345,11 @@ static struct omap_hwmod_addr_space omap44xx_dss_dispc_addrs[] = { { } }; +static struct omap_dss_dispc_dev_attr omap44xx_dss_dispc_dev_attr = { + .manager_count =3 , + .has_framedonetv_irq =1 +}; +/* l4_per -> dss_dispc */ static struct omap_hwmod_ocp_if omap44xx_l4_per__dss_dispc = { .master = &omap44xx_l4_per_hwmod,
@@ -1376,6 +1381,7 @@ static struct omap_hwmod omap44xx_dss_dispc_hwmod = { .slaves = omap44xx_dss_dispc_slaves, .slaves_cnt = ARRAY_SIZE(omap44xx_dss_dispc_slaves), .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430), + .dev_attr = &omap44xx_dss_dispc_dev_attr };/*
arch/arm/mach-omap2/omap_hwmod_common_data.c +4 -0
@@ -49,3 +49,7 @@ struct omap_hwmod_sysc_fields omap_hwmod_sysc_type2 = { .srst_shift = SYSC_TYPE2_SOFTRESET_SHIFT, }; +struct omap_dss_dispc_dev_attr omap2_3_dss_dispc_dev_attr = { + .manager_count =2 , + .has_framedonetv_irq =0 +};
arch/arm/mach-omap2/omap_hwmod_common_data.h +4 -0
@@ -16,6 +16,8 @@#include <plat/omap_hwmod.h> +#include "display.h" +/* Common address space across OMAP2xxx */ extern struct omap_hwmod_addr_space omap2xxx_uart1_addr_space[];extern struct omap_hwmod_addr_space omap2xxx_uart2_addr_space[];
@@ -111,4 +113,6 @@ extern struct omap_hwmod_class omap2xxx_dma_hwmod_class;extern struct omap_hwmod_class omap2xxx_mailbox_hwmod_class;extern struct omap_hwmod_class omap2xxx_mcspi_class; +extern struct omap_dss_dispc_dev_attr omap2_3_dss_dispc_dev_attr; +#endif
Signature · 1 line
1.7.6.3
PATCH v2 1/1 OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Paul,
Quoted 44 lines · Paul Walmsley
Hi
some comments:
Quoted 38 lines · Archit Taneja
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 clearDSS_CONTROL,DSS_SDI_CONTROLandDSS_PLL_CONTROLso 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<[email protected]>Tested-by R, Sricharan<[email protected]>Signed-off-by Archit Taneja<[email protected]>---
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(-)arch/arm/mach-omap2/display.c +0
-0@@ -30,6 +30,30 @@
Quoted 8 lines
arch/arm/mach-omap2/display.c line 34
Quoted 4 lines
arch/arm/mach-omap2/display.c +2
-0#include "control.h" +#define DISPC_BASE_OMAP2 0x48050400 +#define DISPC_BASE_OMAP4 0x48041000These 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
Quoted 481 lines
Quoted 33 lines
arch/arm/mach-omap2/display.c +25
-0+ +#define DISPC_REG (base, offset) (base + offset) + +#define DISPC_CONTROL 0x0040 +#define DISPC_CONTROL2 0x0238 +#define DISPC_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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;arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 221
Quoted 10 lines
arch/arm/mach-omap2/display.c +10
-0+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_mgrsshould come from the hwmod data. We're trying to get rid of cpu_is_omap*() functions wherever possible.arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data)
Quoted 3 lines
arch/arm/mach-omap2/display.c +3
-0+ +/* 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.Quoted 4 lines
arch/arm/mach-omap2/display.c +4
-0+ lcd_en = val&LCD_EN_MASK ; + digit_en = val&DIGIT_EN_MASK ; + +/* store value of LCDENABLE for LCD2 */ arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 238
Quoted 10 lines
arch/arm/mach-omap2/display.c +10
-0+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.
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 245
Quoted 7 lines
arch/arm/mach-omap2/display.c +7
-0+ 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.
Quoted 16 lines
arch/arm/mach-omap2/display.c +16
-0+ + 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 ) {arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data)
Quoted 10 lines
arch/arm/mach-omap2/display.c +10
-0+ 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 ) {Please hoist this constant up to the top of this file, and use a macro with a comment.
arch/arm/mach-omap2/display.c int __init omap_display_init(struct omap_dss_board_info *board_data) · line 273
Quoted 2 lines
arch/arm/mach-omap2/display.c +2
-0+printk (KERN_ERR "didn't get FRAMEDONE1/2 or TV" +" interrupt\n" );
pr_err()is shorter and better here.Quoted 11 lines
arch/arm/mach-omap2/display.c +7
-0+break ; + } +mdelay (1 ); + } + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {arch/arm/mach-omap2/display.c int omap_dss_reset(struct omap_hwmod *oh) · line 297
Quoted 4 lines
arch/arm/mach-omap2/display.c +1
-0@@ -198,6 +294,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs ();Pass the struct
omap_hwmod*oh in here.Quoted 17 lines
arch/arm/mach-omap2/display.c +13
-1+ +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c); --Quoted 1 line
1.7.1
In the interest of expediency, I've made the above changes to the patch -- updated patch below. The following Compile-tested only, so could you review it please and make sure I haven't broken anything? For future patches, please keep the comments above in mind.
thanks,
- Paul
From: Archit Taneja<[email protected]>
Date: Mon, 12 Sep 2011 12:38:26 +0530
Subject: [PATCH 1/2] ARM: OMAP2PLUS: DSS: Ensure DSS works correctly if
display is enabled in bootloaderResetting 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 clearDSS_CONTROL,DSS_SDI_CONTROLandDSS_PLL_CONTROLso 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<[email protected]>Tested-by R, Sricharan<[email protected]>Signed-off-by Archit Taneja<[email protected]>[[email protected]: restructured code, removed omap_{read,write}l(), removed
cpu_is_omap*() calls and converted todev_attr]Signed-off-by Paul Walmsley<[email protected]>--- arch/arm/mach-omap2/display.c | 118 ++++++++++++++++++++++++++ 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, 164 insertions(+), 0 deletions(-)create mode 100644
arch/arm/mach-omap2/display.harch/arm/mach-omap2/display.c +118
-0@@ -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_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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,83 @@ int __init omap_display_init(struct omap_dss_board_info *board_data)return r; } +static void dispc_disable_outputs (struct omap_hwmod *oh) +{ +u32 v, irq_mask =0 ; +bool lcd_en, digit_en, lcd2_en = false; +int i; +struct omap_dss_dispc_dev_attr *da; + +if (!oh->dev_attr) { +pr_err ("display: could not disable outputs during reset due to missing dev_attr\n" ); +return ; + } + + da = (struct omap_dss_dispc_dev_attr *)oh->dev_attr; + +/* store value of LCDENABLE and DIGITENABLE bits */ + v =omap_hwmod_read (oh,DISPC_CONTROL ); + lcd_en = v&LCD_EN_MASK ; + digit_en = v&DIGIT_EN_MASK ; + +/* store value of LCDENABLE for LCD2 */ +if (da->manager_count>2 ) { + v =omap_hwmod_read (oh,DISPC_CONTROL2 ); + lcd2_en = v&LCD_EN_MASK ; + } + +if (!(lcd_en | digit_en | lcd2_en)) +return ;/* no managers currently enabled */ + +/* +* If any manager was enabled, we need to disable it before +* DSS clocks are disabled or DISPC module is reset +*/ +if (lcd_en) + irq_mask |=1 <<FRAMEDONE_IRQ_SHIFT ; + +if (digit_en) { +if (da->has_framedonetv_irq) { + irq_mask |=1 <<FRAMEDONETV_IRQ_SHIFT ; + }else { + irq_mask |=1 <<EVSYNC_EVEN_IRQ_SHIFT | +1 <<EVSYNC_ODD_IRQ_SHIFT ; + } + } + +if (lcd2_en) + irq_mask |=1 <<FRAMEDONE2_IRQ_SHIFT ; + +/* +* clear any previous FRAMEDONE, FRAMEDONETV, +* EVSYNC_EVEN/ODD or FRAMEDONE2 interrupts +*/ +omap_hwmod_write (irq_mask, oh,DISPC_IRQSTATUS ); + +/* disable LCD and TV managers */ + v =omap_hwmod_read (oh,DISPC_CONTROL ); + v&= ~(LCD_EN_MASK |DIGIT_EN_MASK ); +omap_hwmod_write (v, oh,DISPC_CONTROL ); + +/* disable LCD2 manager */ +if (da->manager_count>2 ) { + v =omap_hwmod_read (oh,DISPC_CONTROL2 ); + v&= ~LCD_EN_MASK ; +omap_hwmod_write (v, oh,DISPC_CONTROL2 ); + } + + i =0 ; +while ((omap_hwmod_read (oh,DISPC_IRQSTATUS )& irq_mask) != + irq_mask) { + i++; +if (i>FRAMEDONE_IRQ_TIMEOUT ) { +pr_err ("didn't get FRAMEDONE1/2 or TV interrupt\n" ); +break ; + } +mdelay (1 ); + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {@@ -144,6 +248,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs (oh); + +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c);arch/arm/mach-omap2/display.h new file +29
-0@@ -0,0 +1,29 @@ +/* +* display.h - OMAP2+ integration-specific DSS header +* +* Copyright (C) 2011 Texas Instruments, Inc. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License version 2 as published by +* the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License along with +* this program. If not, see<http://www.gnu.org/licenses/>. +*/ + +#ifndef __ARCH_ARM_MACH_OMAP2_DISPLAY_H +#define __ARCH_ARM_MACH_OMAP2_DISPLAY_H + +#include <linux/kernel.h> + +struct omap_dss_dispc_dev_attr { +u8 manager_count; +bool has_framedonetv_irq; +}; + +#endif arch/arm/mach-omap2/omap_hwmod_2420_data.c +1
-0@@ -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 };/* l4_core -> dss_rfbi */ arch/arm/mach-omap2/omap_hwmod_2430_data.c +1
-0@@ -1005,6 +1005,7 @@ static struct omap_hwmod omap2430_dss_dispc_hwmod = { .slaves_cnt = ARRAY_SIZE(omap2430_dss_dispc_slaves), .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2430), .flags = HWMOD_NO_IDLEST, + .dev_attr =&omap2_3_dss_dispc_dev_attr };/* l4_core -> dss_rfbi */ arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +1
-0@@ -1465,6 +1465,7 @@ static struct omap_hwmod omap3xxx_dss_dispc_hwmod = {CHIP_GE_OMAP3430ES2 |CHIP_IS_OMAP3630ES1 | CHIP_GE_OMAP3630ES1_1), .flags = HWMOD_NO_IDLEST, + .dev_attr =&omap2_3_dss_dispc_dev_attr };/* arch/arm/mach-omap2/omap_hwmod_44xx_data.c +6
-0@@ -1345,6 +1345,11 @@ static struct omap_hwmod_addr_space omap44xx_dss_dispc_addrs[] = { { } }; +static struct omap_dss_dispc_dev_attr omap44xx_dss_dispc_dev_attr = { + .manager_count =3 , + .has_framedonetv_irq =1 +}; +/* l4_per -> dss_dispc */ static struct omap_hwmod_ocp_if omap44xx_l4_per__dss_dispc = { .master =&omap44xx_l4_per_hwmod,@@ -1376,6 +1381,7 @@ static struct omap_hwmod omap44xx_dss_dispc_hwmod = { .slaves = omap44xx_dss_dispc_slaves, .slaves_cnt = ARRAY_SIZE(omap44xx_dss_dispc_slaves), .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430), + .dev_attr =&omap44xx_dss_dispc_dev_attr };/* arch/arm/mach-omap2/omap_hwmod_common_data.c +4
-0@@ -49,3 +49,7 @@ struct omap_hwmod_sysc_fields omap_hwmod_sysc_type2 = { .srst_shift = SYSC_TYPE2_SOFTRESET_SHIFT, }; +struct omap_dss_dispc_dev_attr omap2_3_dss_dispc_dev_attr = { + .manager_count =2 , + .has_framedonetv_irq =0 +};arch/arm/mach-omap2/omap_hwmod_common_data.h +4
-1@@ -16,6 +16,8 @@#include <plat/omap_hwmod.h> +#include "display.h" +/* Common address space across OMAP2xxx */ extern struct omap_hwmod_addr_space omap2xxx_uart1_addr_space[];extern struct omap_hwmod_addr_space omap2xxx_uart2_addr_space[];@@ -111,4 +113,6 @@ extern struct omap_hwmod_class omap2xxx_dma_hwmod_class;extern struct omap_hwmod_class omap2xxx_mailbox_hwmod_class;extern struct omap_hwmod_class omap2xxx_mcspi_class; +extern struct omap_dss_dispc_dev_attr omap2_3_dss_dispc_dev_attr; +#endif --1.7.6.3
Paul Walmsley
maintainerPATCH v2 1/1 OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Archit,
thanks for the quick and informative response -
Quoted 18 lines · Archit Taneja
Quoted 8 lines · Paul Walmsley
Quoted 6 lines · Archit Taneja
arch/arm/mach-omap2/display.c +0
-0@@ -30,6 +30,30 @@Quoted 8 lines
arch/arm/mach-omap2/display.c line 34
Quoted 4 lines
arch/arm/mach-omap2/display.c +2
-0#include "control.h" +#define DISPC_BASE_OMAP2 0x48050400 +#define DISPC_BASE_OMAP4 0x48041000These 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.
Quoted 4 lines
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 throughomap_dss_reset().
You're right. My replacement patch is broken in that regard.
Quoted 2 lines
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.
Quoted 3 lines
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 <[email protected]>
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.
[paul at pwsan.com: restructured code, removed omap_{read,write}l(), removed
cpu_is_omap*() calls and converted to dev_attr]
--- 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
arch/arm/mach-omap2/display.c +125 -0
@@ -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_IRQSTATUS 0x0018 + +#define DSS_SYSCONFIG 0x10 +#define DSS_SYSSTATUS 0x14 +#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_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 =omap_hwmod_lookup ("dss_dispc" ); +if (!oh) { +WARN (1 ,"display: could not disable outputs during reset - could not find dss_dispc hwmod\n" ); +return ; + } + +if (!oh->dev_attr) { +pr_err ("display: could not disable outputs during reset due to missing dev_attr\n" ); +return ; + } + + da = (struct omap_dss_dispc_dev_attr *)oh->dev_attr; + +/* store value of LCDENABLE and DIGITENABLE bits */ + v =omap_hwmod_read (oh,DISPC_CONTROL ); + lcd_en = v &LCD_EN_MASK ; + digit_en = v &DIGIT_EN_MASK ; + +/* store value of LCDENABLE for LCD2 */ +if (da->manager_count >2 ) { + v =omap_hwmod_read (oh,DISPC_CONTROL2 ); + lcd2_en = v &LCD_EN_MASK ; + } + +if (!(lcd_en | digit_en | lcd2_en)) +return ;/* no managers currently enabled */ + +/* +* If any manager was enabled, we need to disable it before +* DSS clocks are disabled or DISPC module is reset +*/ +if (lcd_en) + irq_mask |=1 <<FRAMEDONE_IRQ_SHIFT ; + +if (digit_en) { +if (da->has_framedonetv_irq) { + irq_mask |=1 <<FRAMEDONETV_IRQ_SHIFT ; + }else { + irq_mask |=1 <<EVSYNC_EVEN_IRQ_SHIFT | +1 <<EVSYNC_ODD_IRQ_SHIFT ; + } + } + +if (lcd2_en) + irq_mask |=1 <<FRAMEDONE2_IRQ_SHIFT ; + +/* +* clear any previous FRAMEDONE, FRAMEDONETV, +* EVSYNC_EVEN/ODD or FRAMEDONE2 interrupts +*/ +omap_hwmod_write (irq_mask, oh,DISPC_IRQSTATUS ); + +/* disable LCD and TV managers */ + v =omap_hwmod_read (oh,DISPC_CONTROL ); + v &= ~(LCD_EN_MASK |DIGIT_EN_MASK ); +omap_hwmod_write (v, oh,DISPC_CONTROL ); + +/* disable LCD2 manager */ +if (da->manager_count >2 ) { + v =omap_hwmod_read (oh,DISPC_CONTROL2 ); + v &= ~LCD_EN_MASK ; +omap_hwmod_write (v, oh,DISPC_CONTROL2 ); + } + + i =0 ; +while ((omap_hwmod_read (oh,DISPC_IRQSTATUS ) & irq_mask) != + irq_mask) { + i++; +if (i >FRAMEDONE_IRQ_TIMEOUT ) { +pr_err ("didn't get FRAMEDONE1/2 or TV interrupt\n" ); +break ; + } +mdelay (1 ); + } +} +#define MAX_MODULE_SOFTRESET_WAIT 10000int omap_dss_reset (struct omap_hwmod *oh) {
@@ -144,6 +255,20 @@ int omap_dss_reset(struct omap_hwmod *oh)if (oc->_clk)clk_enable (oc->_clk); +dispc_disable_outputs (); + +/* clear SDI registers */ +if (cpu_is_omap3430 ()) { +omap_hwmod_write (0x0 , oh,DSS_SDI_CONTROL ); +omap_hwmod_write (0x0 , oh,DSS_PLL_CONTROL ); + } + +/* +* clear DSS_CONTROL register to switch DSS clock sources to +* PRCM clock, if any +*/ +omap_hwmod_write (0x0 , oh,DSS_CONTROL ); +omap_test_timeout ((omap_hwmod_read (oh, oh->class->sysc->syss_offs) &SYSS_RESETDONE_MASK ),MAX_MODULE_SOFTRESET_WAIT , c);
arch/arm/mach-omap2/display.h new file +29 -0
@@ -0,0 +1,29 @@ +/* +* display.h - OMAP2+ integration-specific DSS header +* +* Copyright (C) 2011 Texas Instruments, Inc. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License version 2 as published by +* the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License along with +* this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#ifndef __ARCH_ARM_MACH_OMAP2_DISPLAY_H +#define __ARCH_ARM_MACH_OMAP2_DISPLAY_H + +#include <linux/kernel.h> + +struct omap_dss_dispc_dev_attr { +u8 manager_count; +bool has_framedonetv_irq; +}; + +#endif
arch/arm/mach-omap2/omap_hwmod_2420_data.c +1 -0
@@ -944,6 +944,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 };/* l4_core -> dss_rfbi */
arch/arm/mach-omap2/omap_hwmod_2430_data.c +1 -0
@@ -1004,6 +1004,7 @@ static struct omap_hwmod omap2430_dss_dispc_hwmod = { .slaves_cnt = ARRAY_SIZE(omap2430_dss_dispc_slaves), .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2430), .flags = HWMOD_NO_IDLEST, + .dev_attr = &omap2_3_dss_dispc_dev_attr };/* l4_core -> dss_rfbi */
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +1 -0
@@ -1464,6 +1464,7 @@ static struct omap_hwmod omap3xxx_dss_dispc_hwmod = {CHIP_GE_OMAP3430ES2 |CHIP_IS_OMAP3630ES1 | CHIP_GE_OMAP3630ES1_1), .flags = HWMOD_NO_IDLEST, + .dev_attr = &omap2_3_dss_dispc_dev_attr };/*
arch/arm/mach-omap2/omap_hwmod_44xx_data.c +6 -0
@@ -1345,6 +1345,11 @@ static struct omap_hwmod_addr_space omap44xx_dss_dispc_addrs[] = { { } }; +static struct omap_dss_dispc_dev_attr omap44xx_dss_dispc_dev_attr = { + .manager_count =3 , + .has_framedonetv_irq =1 +}; +/* l4_per -> dss_dispc */ static struct omap_hwmod_ocp_if omap44xx_l4_per__dss_dispc = { .master = &omap44xx_l4_per_hwmod,
@@ -1376,6 +1381,7 @@ static struct omap_hwmod omap44xx_dss_dispc_hwmod = { .slaves = omap44xx_dss_dispc_slaves, .slaves_cnt = ARRAY_SIZE(omap44xx_dss_dispc_slaves), .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430), + .dev_attr = &omap44xx_dss_dispc_dev_attr };/*
arch/arm/mach-omap2/omap_hwmod_common_data.c +4 -0
@@ -49,3 +49,7 @@ struct omap_hwmod_sysc_fields omap_hwmod_sysc_type2 = { .srst_shift = SYSC_TYPE2_SOFTRESET_SHIFT, }; +struct omap_dss_dispc_dev_attr omap2_3_dss_dispc_dev_attr = { + .manager_count =2 , + .has_framedonetv_irq =0 +};
arch/arm/mach-omap2/omap_hwmod_common_data.h +4 -0
@@ -16,6 +16,8 @@#include <plat/omap_hwmod.h> +#include "display.h" +/* Common address space across OMAP2xxx */ extern struct omap_hwmod_addr_space omap2xxx_uart1_addr_space[];extern struct omap_hwmod_addr_space omap2xxx_uart2_addr_space[];
@@ -111,4 +113,6 @@ extern struct omap_hwmod_class omap2xxx_dma_hwmod_class;extern struct omap_hwmod_class omap2xxx_mailbox_hwmod_class;extern struct omap_hwmod_class omap2xxx_mcspi_class; +extern struct omap_dss_dispc_dev_attr omap2_3_dss_dispc_dev_attr; +#endif
Signature · 1 line
1.7.6.3
PATCH v2 1/1 OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
In reply to Paul WalmsleyHi Paul,
Quoted 21 lines · Paul Walmsley
Hi
some comments:
Quoted 15 lines · Archit Taneja
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 clearDSS_CONTROL,DSS_SDI_CONTROLandDSS_PLL_CONTROLso 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>
Quoted 11 lines
arch/arm/mach-omap2/display.h new file +6
-0+struct omap_dss_dispc_dev_attr { +u8 manager_count; +bool has_framedonetv_irq; +}; + +#endif arch/arm/mach-omap2/omap_hwmod_2420_data.c +0
-0
arch/arm/mach-omap2/omap_hwmod_2420_data.c static struct omap_hwmod omap2420_dss_dispc_hwmod = { · line 949
Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod_2420_data.c +1
-0@@ -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
PATCH v2 1/1 OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
Hi Tomi,
Quoted 3 lines · Tomi
Hi Paul,
On Sun, 2011-10-02 at 22:45 -0600, Paul Walmsley wrote:
[...]
Quoted 12 lines
Quoted 12 lines
arch/arm/mach-omap2/display.h new file +6
-0+struct omap_dss_dispc_dev_attr { +u8 manager_count; +bool has_framedonetv_irq; +}; + +#endif arch/arm/mach-omap2/omap_hwmod_2420_data.c +0
-0@@ -945,6 +945,7 @@ static struct omap_hwmod omap2420_dss_dispc_hwmod = { .slaves_cnt = ARRAY_SIZE(omap2420_dss_dispc_slaves),
Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod_2420_data.c static struct omap_hwmod omap2420_dss_dispc_hwmod = { · line 949
Quoted 4 lines
arch/arm/mach-omap2/omap_hwmod_2420_data.c +1
-0.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.
Quoted 6 lines
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_featuressystem is currently part of the driver, but should be moved underarch/arm/*omap*at some point, and this hwmoddev_attrsounds 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.
Quoted 3 lines
I looked at how the
dev_attrsare 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 at 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
Paul Walmsley
maintainer+ devicetree-discuss, lkml
Quoted 6 lines · Benoit
But at that time, device tree was not there...
Now, the wholedev_attrstuff 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.
Quoted 6 lines
As an example here is the ongoing GPIO DT migration:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg56505.html3.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
Quoted 24 lines · Paul Walmsley
+ devicetree-discuss, lkml
Quoted 6 lines · Benoit
But at that time, device tree was not there...
Now, the wholedev_attrstuff 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.Quoted 6 lines
As an example here is the ongoing GPIO DT migration:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg56505.html3.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.
Quoted 6 lines
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.
Paul Walmsley
maintainerPATCH v2 1/1 OMAP2PLUS: DSS: Ensure DSS works correctly if display is enabled in bootloader
In reply to Tomi ValkeinenHi Tomi,
Quoted 32 lines · Tomi Valkeinen
Quoted 17 lines · Paul Walmsley
Quoted 15 lines · Archit Taneja
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 clearDSS_CONTROL,DSS_SDI_CONTROLandDSS_PLL_CONTROLso 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>
Quoted 11 lines
arch/arm/mach-omap2/display.h new file +6
-0+struct omap_dss_dispc_dev_attr { +u8 manager_count; +bool has_framedonetv_irq; +}; + +#endif arch/arm/mach-omap2/omap_hwmod_2420_data.c +0
-0@@ -945,6 +945,7 @@ static struct omap_hwmod omap2420_dss_dispc_hwmod = {
Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod_2420_data.c static struct omap_hwmod omap2420_dss_dispc_hwmod = { · line 949
Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod_2420_data.c +1
-0.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.
Quoted 1 line
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.
Quoted 10 lines
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_featuressystem is currently part of the driver, but should be moved underarch/arm/*omap*at some point, and this hwmoddev_attrsounds like it could possibly be a right place to handle these.I looked at how the
dev_attrsare 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