Add splash screen for CM-T35
cover Add splash screen for CM-T35
This patchset adds splash screen support for CM-T35.
It includes the ability to initialize the display subsystem either using predefines (selected via env variable "displaytype"), or user supplied configuration options, also stored in an environment variables and pointed to by displaytype. The splash image data is currently read from NAND.
As a preparation for the above functionality, this patch set introduces new DSS #defines and an option for board-specific splash screen preparation, which can be invoked in lcd_logo() right before displaying the splash screen (typical use
case: load the image data in time for it to be displayed).
Nikita Kiryanov (5):
omap3: add useful dss defines
lcd: add option for board specific splash screen preparation
cm-t35: add support for dvi displays
cm-t35: add support for user defined lcd parameters
cm-t35: add support for loading splash image from NAND
README | 8 + arch/arm/include/asm/arch-omap3/dss.h | 35 +++ board/cm_t35/Makefile | 1 + board/cm_t35/cm_t35.c | 64 +++++ board/cm_t35/display.c | 420 +++++++++++++++++++++++++++++++++ common/lcd.c | 15 ++ include/configs/cm_t35.h | 10 + include/lcd.h | 1 + 8 files changed, 554 insertions(+) create mode 100644 board/cm_t35/display.c
Signature · 1 line
1.7.10.4
PATCH 1/5 omap3: add useful dss defines
Add useful omap3 dss defines for: polarity, TFT data lines, lcd display type, gfx burst size, and gfx format
--- arch/arm/include/asm/arch-omap3/dss.h | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
arch/arm/include/asm/arch-omap3/dss.h +35 -0
@@ -167,6 +167,41 @@ struct venc_regs {#define VENC_OUT_SEL (1 << 6)#define DIG_LPP_SHIFT 16 +/* LCD display type */ +#define PASSIVE_DISPLAY 0 +#define ACTIVE_DISPLAY 1 + +/* TFTDATALINES */ +#define LCD_INTERFACE_12_BIT 0 +#define LCD_INTERFACE_16_BIT 1 +#define LCD_INTERFACE_18_BIT 2 +#define LCD_INTERFACE_24_BIT 3 + +/* Polarity */ +#define DSS_IVS (1 << 12) +#define DSS_IHS (1 << 13) +#define DSS_IPC (1 << 14) +#define DSS_IEO (1 << 15) + +/* GFX format */ +#define GFXFORMAT_BITMAP1 0x0 +#define GFXFORMAT_BITMAP2 0x1 +#define GFXFORMAT_BITMAP4 0x2 +#define GFXFORMAT_BITMAP8 0x3 +#define GFXFORMAT_RGB12 0x4 +#define GFXFORMAT_ARGB16 0x5 +#define GFXFORMAT_RGB16 0x6 +#define GFXFORMAT_RGB24_UNPACKED 0x8 +#define GFXFORMAT_RGB24_PACKED 0x9 +#define GFXFORMAT_ARGB32 0xC +#define GFXFORMAT_RGBA32 0xD +#define GFXFORMAT_RGBx32 0xE + +/* GFX burst size */ +#define GFXBURSTSIZE4 0 +#define GFXBURSTSIZE8 1 +#define GFXBURSTSIZE16 2 +/* Panel Configuration */ struct panel_config {u32 timing_h;
Signature · 1 line
1.7.10.4
PATCH 1/5 omap3: add useful dss defines
Hello Nikita,
Quoted 45 lines · Nikita Kiryanov
Add useful omap3 dss defines for: polarity, TFT data lines, lcd display type, gfx burst size, and gfx format
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>--- arch/arm/include/asm/arch-omap3/dss.h | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)arch/arm/include/asm/arch-omap3/dss.h +28
-0@@ -167,6 +167,41 @@ struct venc_regs {#define VENC_OUT_SEL (1 << 6)#define DIG_LPP_SHIFT 16 +/* LCD display type */ +#define PASSIVE_DISPLAY 0 +#define ACTIVE_DISPLAY 1 + +/* TFTDATALINES */ +#define LCD_INTERFACE_12_BIT 0 +#define LCD_INTERFACE_16_BIT 1 +#define LCD_INTERFACE_18_BIT 2 +#define LCD_INTERFACE_24_BIT 3 + +/* Polarity */ +#define DSS_IVS (1 << 12) +#define DSS_IHS (1 << 13) +#define DSS_IPC (1 << 14) +#define DSS_IEO (1 << 15) + +/* GFX format */ +#define GFXFORMAT_BITMAP1 0x0 +#define GFXFORMAT_BITMAP2 0x1 +#define GFXFORMAT_BITMAP4 0x2 +#define GFXFORMAT_BITMAP8 0x3 +#define GFXFORMAT_RGB12 0x4 +#define GFXFORMAT_ARGB16 0x5 +#define GFXFORMAT_RGB16 0x6 +#define GFXFORMAT_RGB24_UNPACKED 0x8 +#define GFXFORMAT_RGB24_PACKED 0x9 +#define GFXFORMAT_ARGB32 0xC +#define GFXFORMAT_RGBA32 0xD
arch/arm/include/asm/arch-omap3/dss.h struct venc_regs { · line 207
Quoted 10 lines
arch/arm/include/asm/arch-omap3/dss.h +7
-0+#define GFXFORMAT_RGBx32 0xE + +/* GFX burst size */ +#define GFXBURSTSIZE4 0 +#define GFXBURSTSIZE8 1 +#define GFXBURSTSIZE16 2 +/* Panel Configuration */ struct panel_config {u32 timing_h;
most defines in omap dss use the location in the silicon itself. For consistency you might want to shift these values to the appropriate place. (or just use 32 mode so you can drop most if not all of them)
Regards,
Jeroen
PATCH 1/5 omap3: add useful dss defines
Hi Jeroen,
Quoted 2 lines · Jeroen Hofstee
Hello Nikita,
[...]
Quoted 8 lines
Quoted 8 lines
arch/arm/include/asm/arch-omap3/dss.h +8
-0+#define GFXFORMAT_ARGB32 0xC +#define GFXFORMAT_RGBA32 0xD +#define GFXFORMAT_RGBx32 0xE + +/* GFX burst size */ +#define GFXBURSTSIZE4 0 +#define GFXBURSTSIZE8 1 +#define GFXBURSTSIZE16 2
Quoted 9 lines
Quoted 4 lines
+
/* Panel Configuration */struct panel_config { u32 timing_h;most defines in omap dss use the location in the silicon itself. For consistency you might want to shift these values to the appropriate place. (or just use 32 mode so you can drop most if not all of them)
These aren't offsets against a base address. These are input values for the various sections of the dss registers. For example the /* GFX burst size */ defines are values for DISPC_GFX_ATTRIBUTES[7:6].
Quoted 2 lines
Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 1/5 omap3: add useful dss defines
Hello Nikita,
+#define GFXFORMAT_ARGB32 0xC
Quoted 17 lines
Quoted 7 lines
Quoted 7 lines
arch/arm/include/asm/arch-omap3/dss.h +7
-0+#define GFXFORMAT_RGBA32 0xD +#define GFXFORMAT_RGBx32 0xE + +/* GFX burst size */ +#define GFXBURSTSIZE4 0 +#define GFXBURSTSIZE8 1 +#define GFXBURSTSIZE16 2Quoted 9 lines
Quoted 4 lines
+
/* Panel Configuration */struct panel_config { u32 timing_h;most defines in omap dss use the location in the silicon itself. For consistency you might want to shift these values to the appropriate place. (or just use 32 mode so you can drop most if not all of them)
Quoted 5 lines
These aren't offsets against a base address. These are input values for the various sections of the dss registers. For example the /* GFX burst size */ defines are values for DISPC_GFX_ATTRIBUTES[7:6].
What I mean is that the defines currently in dss.h already shift the values to the location where the hardware expects them, e.g..
/* Configure VENC DSS Params */
#define VENC_CLK_ENABLE (1 << 3)
#define DAC_DEMEN (1 << 4)
#define DAC_POWERDN (1 << 5)
#define VENC_OUT_SEL (1 << 6)
The defines you add are not shifted however, so after this patch half of the defines need shifting, the other half does not. Thats confusing, so macro's like
#define GFXBURSTSIZE8 (1 << 6)
is a better option in my opinion.
Regards,
Jeroen
PATCH 1/5 omap3: add useful dss defines
Quoted 35 lines · Jeroen Hofstee
Hello Nikita,
+#define
GFXFORMAT_ARGB320xCQuoted 17 lines
Quoted 7 lines
Quoted 7 lines
arch/arm/include/asm/arch-omap3/dss.h +7
-0+#define GFXFORMAT_RGBA32 0xD +#define GFXFORMAT_RGBx32 0xE + +/* GFX burst size */ +#define GFXBURSTSIZE4 0 +#define GFXBURSTSIZE8 1 +#define GFXBURSTSIZE16 2Quoted 9 lines
Quoted 4 lines
+
/* Panel Configuration */struct panel_config { u32 timing_h;most defines in omap dss use the location in the silicon itself. For consistency you might want to shift these values to the appropriate place. (or just use 32 mode so you can drop most if not all of them)
Quoted 5 lines
These aren't offsets against a base address. These are input values for the various sections of the dss registers. For example the /* GFX burst size */ defines are values for DISPC_GFX_ATTRIBUTES[7:6].
What I mean is that the defines currently in
dss.halready shift the values to the location where the hardware expects them, e.g../* Configure VENC DSS Params */
#defineVENC_CLK_ENABLE(1 << 3)
#defineDAC_DEMEN(1 << 4)
#defineDAC_POWERDN(1 << 5)
#defineVENC_OUT_SEL(1 << 6)
Quoted 7 lines
The defines you add are not shifted however, so after this patch half of the defines need shifting, the other half does not. Thats confusing, so macro's like
#define GFXBURSTSIZE8 (1 << 6)
is a better option in my opinion.
OK now I understand. Some of these could indeed be shifted, and I'll do that in a V2, but LCD_INTERFACE_* and *_DISPLAY cannot be shifted, because they are passed to a function that expects them to be unshifted (omap3_dss_panel_config).
Quoted 4 lines
Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 2/5 lcd: add option for board specific splash screen preparation
In reply to Nikita KiryanovCurrently there is no logical place to put the code that prepares the splash image data. The splash image data should be ready in memory before bmp_display() is called, and after the environment is ready (since lcd.c looks for the splash image in an address specified by the environment variable "splashimage").
Our window of opportunity in board_init_r() is therefore: between env_relocate() and bmp_display(), and from the available options only the lcd related functions in drv_lcd_init() seem appropriate for such lcd oriented code.
Add the option to prepare the splash image data in lcd_logo() right before it is sent to be displayed.
--- README | 8 ++++++++ common/lcd.c | 15 +++++++++++++++ include/lcd.h | 1 + 3 files changed, 24 insertions(+)
README +8 -0
@@ -1541,6 +1541,14 @@ CBFS (Coreboot Filesystem) support => vertically centered image at x = dspWidth - bmpWidth - 9 + CONFIG_SPLASH_SCREEN_PREPARE + + If this option is set then the board_splash_screen_prepare() + function, which must be defined in your code, is called as part + of the splash screen display sequence. It gives the board an + opportunity to prepare the splash image data before it is + processed and sent to the frame buffer by U-Boot. + - Gzip compressed BMP image support: CONFIG_VIDEO_BMP_GZIP If this option is set, additionally to standard BMP
common/lcd.c +15 -0
@@ -1034,6 +1034,18 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) }#endif +#ifdef CONFIG_SPLASH_SCREEN_PREPARE +static inline int splash_screen_prepare (void ) +{ +return board_splash_screen_prepare (); +} +#else +static inline int splash_screen_prepare (void ) +{ +return 0 ; +} +#endif +static void *lcd_logo (void ) {#ifdef CONFIG_SPLASH_SCREEN
@@ -1045,6 +1057,9 @@ static void *lcd_logo(void)int x =0 , y =0 ; do_splash =0 ; +if (splash_screen_prepare ()) +return (void *)lcd_base; + addr =simple_strtoul (s,NULL ,16 );#ifdef CONFIG_SPLASH_SCREEN_ALIGN s =getenv ("splashpos" );
include/lcd.h +1 -0
@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,
Signature · 1 line
1.7.10.4
PATCH 2/5 lcd: add option for board specific splash screen preparation
Quoted 79 lines · Nikita Kiryanov
Currently there is no logical place to put the code that prepares the splash image data. The splash image data should be ready in memory before
bmp_display()is called, and after the environment is ready (sincelcd.clooks for the splash image in an address specified by the environment variable "splashimage").Our window of opportunity in
board_init_r()is therefore: betweenenv_relocate()andbmp_display(), and from the available options only the lcd related functions indrv_lcd_init()seem appropriate for such lcd oriented code.Add the option to prepare the splash image data in
lcd_logo()right before it is sent to be displayed.Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>--- README | 8 ++++++++ common/lcd.c | 15 +++++++++++++++ include/lcd.h | 1 + 3 files changed, 24 insertions(+)README +8
-0@@ -1541,6 +1541,14 @@ CBFS (Coreboot Filesystem) support => vertically centered image at x = dspWidth - bmpWidth - 9 + CONFIG_SPLASH_SCREEN_PREPARE + + If this option is set then the board_splash_screen_prepare() + function, which must be defined in your code, is called as part + of the splash screen display sequence. It gives the board an + opportunity to prepare the splash image data before it is + processed and sent to the frame buffer by U-Boot. + - Gzip compressed BMP image support: CONFIG_VIDEO_BMP_GZIP If this option is set, additionally to standard BMPcommon/lcd.c +15
-0@@ -1034,6 +1034,18 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y) }#endif +#ifdef CONFIG_SPLASH_SCREEN_PREPARE +static inline int splash_screen_prepare (void ) +{ +return board_splash_screen_prepare (); +} +#else +static inline int splash_screen_prepare (void ) +{ +return 0 ; +} +#endif +static void *lcd_logo (void ) {#ifdef CONFIG_SPLASH_SCREEN @@ -1045,6 +1057,9 @@ static void *lcd_logo(void)int x =0 , y =0 ; do_splash =0 ; +if (splash_screen_prepare ()) +return (void *)lcd_base; + addr =simple_strtoul (s,NULL ,16 );#ifdef CONFIG_SPLASH_SCREEN_ALIGN s =getenv ("splashpos" );include/lcd.h +0
-0
include/lcd.h extern struct vidinfo panel_info; · line 53
Quoted 7 lines
include/lcd.h +1
-0@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,
Other boards seem to do this in lcd_enable. Perhaps that is also an option.
Regards,
Jeroen
PATCH 2/5 lcd: add option for board specific splash screen preparation
Hi Jeroen,
On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
[...]
Quoted 11 lines
include/lcd.h extern struct vidinfo panel_info; · line 51
Quoted 10 lines
include/lcd.h +1
-0@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,Other boards seem to do this in
lcd_enable. Perhaps that is also an option.
The problem with doing it in lcd_enable is that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.
Quoted 3 lines
Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 2/5 lcd: add option for board specific splash screen preparation
Hello Nikita,
Quoted 17 lines · Nikita Kiryanov
Hi Jeroen,
On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
[...]Quoted 4 lines
Quoted 4 lines
include/lcd.h +0
-0Quoted 8 lines
include/lcd.h extern struct vidinfo panel_info; · line 51
Quoted 6 lines
include/lcd.h +1
-0@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,Other boards seem to do this in
lcd_enable. Perhaps that is also an option.
Quoted 4 lines
The problem with doing it in
lcd_enableis that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.
mmm, I am not so sure I agree that loading a bitmap in lcd_enable is a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.
But anyway, can't this at least be changed to a __weak function, so the CONFIG and ifdef stuff can be dropped?
Regards,
Jeroen
PATCH 2/5 lcd: add option for board specific splash screen preparation
Quoted 20 lines · Jeroen Hofstee
Hello Nikita,
Quoted 17 lines · Nikita Kiryanov
Hi Jeroen,
On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
[...]Quoted 12 lines
include/lcd.h extern struct vidinfo panel_info; · line 51
Quoted 10 lines
include/lcd.h +1
-0@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,Other boards seem to do this in
lcd_enable. Perhaps that is also an option.
Quoted 7 lines
Quoted 4 lines
The problem with doing it in
lcd_enableis that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.mmm, I am not so sure I agree that loading a bitmap in
lcd_enableis a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.
Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.
Quoted 3 lines
But anyway, can't this at least be changed to a
__weakfunction, so the CONFIG and ifdef stuff can be dropped?
The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code.
Quoted 5 lines
Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 2/5 lcd: add option for board specific splash screen preparation
Hello Nikita,
Quoted 25 lines · Nikita Kiryanov
Quoted 24 lines · Jeroen Hofstee
Hello Nikita,
Quoted 21 lines · Nikita Kiryanov
Hi Jeroen,
On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
[...]Quoted 4 lines
Quoted 4 lines
include/lcd.h +0
-0Quoted 8 lines
include/lcd.h extern struct vidinfo panel_info; · line 51
Quoted 6 lines
include/lcd.h +1
-0@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,Other boards seem to do this in
lcd_enable. Perhaps that is also an option.The problem with doing it in
lcd_enableis that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.
Quoted 9 lines
Quoted 3 lines
mmm, I am not so sure I agree that loading a bitmap in
lcd_enableis a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "
lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.
my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it,
it seems you're make a side effect of a function only called once a side effect
of another function (which could be called multiple times). So you make things
even worse (loading an bitmap while the function is just named to display it).
Quoted 8 lines
Quoted 3 lines
But anyway, can't this at least be changed to a
__weakfunction, so the CONFIG and ifdef stuff can be dropped?The motivation behind the CONFIG was to make it a documentable user setting,
rather than an undocumented feature buried in the code.
then document the callback...
I don't see the improvement of this patch..
Regards,
Jeroen
PATCH 2/5 lcd: add option for board specific splash screen preparation
Quoted 37 lines · Jeroen Hofstee
Hello Nikita,
Quoted 25 lines · Nikita Kiryanov
Quoted 24 lines · Jeroen Hofstee
Hello Nikita,
Quoted 21 lines · Nikita Kiryanov
Hi Jeroen,
On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
[...]Quoted 4 lines
Quoted 4 lines
include/lcd.h +0
-0Quoted 8 lines
include/lcd.h extern struct vidinfo panel_info; · line 51
Quoted 6 lines
include/lcd.h +1
-0@@ -47,6 +47,7 @@ extern struct vidinfo panel_info;extern void lcd_ctrl_init (void *lcdbase);extern void lcd_enable (void ); +extern int board_splash_screen_prepare (void );/* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,Other boards seem to do this in
lcd_enable. Perhaps that is also an option.The problem with doing it in
lcd_enableis that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.Quoted 9 lines
Quoted 3 lines
mmm, I am not so sure I agree that loading a bitmap in
lcd_enableis a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "
lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.
Quoted 4 lines
my point is that
lcd_clearwill e.g. calllcd_logo. Although I haven't tested it, it seems you're make a side effect of a function only called once a side effect of another function (which could be called multiple times). So you make things even worse (loading an bitmap while the function is just named to display it).
So what's your point? Do you think we should add a splash screen specific callback inside the board.c U-Boot boot flow?
Please, be more specific, as both approaches are not suitable according to what was said above...
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
But anyway, can't this at least be changed to a
__weakfunction, so the CONFIG and ifdef stuff can be dropped?The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code.
then document the callback...
Sorry, may be I've missed something, but I can't see any callback being documented in the README file...
Quoted 2 lines
I don't see the improvement of this patch..
What does that suppose to mean? Either be constructive or don't bother... I'd like to hear Anatolij's opinion on this.
Signature · 2 lines
Regards, Igor.
PATCH 2/5 lcd: add option for board specific splash screen preparation
Hello Igor,
Quoted 14 lines · Igor Grinberg
Quoted 13 lines · Jeroen Hofstee
Hello Nikita,
Quoted 10 lines · Nikita Kiryanov
Quoted 4 lines · Jeroen Hofstee
mmm, I am not so sure I agree that loading a bitmap in
lcd_enableis a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "
lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.
Quoted 6 lines
Quoted 4 lines
my point is that
lcd_clearwill e.g. calllcd_logo. Although I haven't tested it, it seems you're make a side effect of a function only called once a side effect of another function (which could be called multiple times). So you make things even worse (loading an bitmap while the function is just named to display it).So what's your point? Do you think we should add a splash screen specific callback inside the
board.cU-Boot boot flow?
no.
Quoted 2 lines
Please, be more specific, as both approaches are not suitable according to what was said above...
lets see, drv_lcd_init calls lcd_init. which does
lcd_ctrl_init(lcdbase);lcd_is_enabled = 1;lcd_clear();lcd_enable();
After this patch, lcd_clear calls lcd_logo which calls board_splash_screen_prepare in its turn. In my mind this still leaves allot of side effects. If you want to prepare for displaying and not have it as a side effect of a function which is named to do something else, it should be in between lcd_ctrl_init and lcd_clear in my mind.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
But anyway, can't this at least be changed to a
__weakfunction, so the CONFIG and ifdef stuff can be dropped?The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code.
Quoted 6 lines
Quoted 1 line
then document the callback...
Sorry, may be I've missed something, but I can't see any callback being documented in the README file...
Quoted 1 line
I don't see the improvement of this patch..
What does that suppose to mean? Either be constructive or don't bother...
This means, as I hopefully explained a bit more clearly now, that the patch makes the loading of a bitmap a side effect of lcd_clear, while the intention was to make it a more natural call sequence. (which can simply be done by putting it somewhere else as mentioned above)
btw, I think, loading the image in lcd_enable() won't even work since lcd_enable is actually before lcd_clear. Scanning some boards which load in lcd_enable, they seem to call bmp_display manually. So that makes this patch no longer optional, but is actually required and is an improvement....
Quoted 2 lines
I'd like to hear Anatolij's opinion on this.
yes, me too. I like the __weak far more than requiring a CONFIG_to enable a callback. I cannot think of a reason why these __weak functions cannot be documented. So that's up to Anatolij.
Regards,
Jeroen
PATCH 2/5 lcd: add option for board specific splash screen preparation
Quoted 29 lines · Jeroen Hofstee
Hello Igor,
Quoted 14 lines · Igor Grinberg
Quoted 13 lines · Jeroen Hofstee
Hello Nikita,
Quoted 10 lines · Nikita Kiryanov
Quoted 4 lines · Jeroen Hofstee
mmm, I am not so sure I agree that loading a bitmap in
lcd_enableis a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "
lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.Quoted 6 lines
Quoted 4 lines
my point is that
lcd_clearwill e.g. calllcd_logo. Although I haven't tested it, it seems you're make a side effect of a function only called once a side effect of another function (which could be called multiple times). So you make things even worse (loading an bitmap while the function is just named to display it).So what's your point? Do you think we should add a splash screen specific callback inside the
board.cU-Boot boot flow?no.
Quoted 2 lines
Please, be more specific, as both approaches are not suitable according to what was said above...
lets see,
drv_lcd_initcallslcd_init. which does
Quoted 7 lines
lcd_ctrl_init(lcdbase);lcd_is_enabled= 1;lcd_clear();lcd_enable();After this patch,
lcd_clearcallslcd_logowhich callsboard_splash_screen_preparein its turn.
That said, lcd_clear() calls lcd_logo()...
This is the real source of confusion here - the side effect, and not the fact that lcd_logo() calls the board_splash_screen_prepare()... Being that a problem not directly related to Nikita's patch set, I don't think we should delay it any further.
Quoted 5 lines
In my mind this
still leaves allot of side effects. If you want to prepare for displaying and not have it as a side effect of a function which is named to do something else, it should be in betweenlcd_ctrl_initandlcd_clearin my mind.
I think not, lcd_logo() fits perfectly for loading the splash screen. The fact that lcd_logo() is called from lcd_clear(), IMO, would be the one that should be dealt with if you want to remove the confusion ("the side effect"). But that is not related to Nikita's patch set.
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
But anyway, can't this at least be changed to a
__weakfunction, so the CONFIG and ifdef stuff can be dropped?The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code.
then document the callback...
Sorry, may be I've missed something, but I can't see any callback being documented in the README file...
Quoted 7 lines
Quoted 2 lines
Quoted 1 line
I don't see the improvement of this patch..
What does that suppose to mean? Either be constructive or don't bother...
This means, as I hopefully explained a bit more clearly now, that the patch makes the loading of a bitmap a side effect of
lcd_clear, while the intention was to make it a more natural call sequence. (which can simply be done by putting it somewhere else as mentioned above)
As explained above, the patch only uses lcd_logo(), which is meant to display the splash screen, and add the loading functionality. The fact that the lcd_logo() is called from lcd_clear() is the one you should blame. And as already said, not related to this patch.
Quoted 6 lines
btw, I think, loading the image in
lcd_enable()won't even work sincelcd_enableis actually beforelcd_clear. Scanning some boards which load inlcd_enable, they seem to callbmp_displaymanually. So that makes this patch no longer optional, but is actually required and is an improvement....
Ok. So we agree that this can't be done in lcd_enable().
Quoted 5 lines
Quoted 2 lines
I'd like to hear Anatolij's opinion on this.
yes, me too. I like the
__weakfar more than requiring aCONFIG_toenable a callback. I cannot think of a reason why these__weakfunctions cannot be documented. So that's up to Anatolij.
Usually, I also like the __weak approach, but this time the intention was to document the feature and hopefully stop the lcd_*() API abuse.
Signature · 2 lines
Regards, Igor.
PATCH 2/5 lcd: add option for board specific splash screen preparation
Hello Igor,
Quoted 46 lines · Igor Grinberg
Quoted 27 lines · Jeroen Hofstee
Hello Igor,
Quoted 13 lines · Igor Grinberg
Quoted 12 lines · Jeroen Hofstee
Hello Nikita,
Quoted 9 lines · Nikita Kiryanov
Quoted 3 lines · Jeroen Hofstee
mmm, I am not so sure I agree that loading a bitmap in
lcd_enableis a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "
lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.Quoted 6 lines
Quoted 4 lines
my point is that
lcd_clearwill e.g. calllcd_logo. Although I haven't tested it, it seems you're make a side effect of a function only called once a side effect of another function (which could be called multiple times). So you make things even worse (loading an bitmap while the function is just named to display it).So what's your point? Do you think we should add a splash screen specific callback inside the
board.cU-Boot boot flow?no.
Quoted 2 lines
Please, be more specific, as both approaches are not suitable according to what was said above...
lets see,
drv_lcd_initcallslcd_init. which doesQuoted 7 lines
lcd_ctrl_init(lcdbase);lcd_is_enabled= 1;lcd_clear();lcd_enable();After this patch,
lcd_clearcallslcd_logowhich callsboard_splash_screen_preparein its turn.That said,
lcd_clear()callslcd_logo()...
This is the real source of confusion here - the side effect, and not the fact thatlcd_logo()calls theboard_splash_screen_prepare()... Being that a problem not directly related to Nikita's patch set, I don't think we should delay it any further.Quoted 5 lines
In my mind this
still leaves allot of side effects. If you want to prepare for displaying and not have it as a side effect of a function which is named to do something else, it should be in betweenlcd_ctrl_initandlcd_clearin my mind.
Quoted 6 lines
I think not,
lcd_logo()fits perfectly for loading the splash screen. The fact thatlcd_logo()is called fromlcd_clear(), IMO, would be the one that should be dealt with if you want to remove the confusion ("the side effect"). But that is not related to Nikita's patch set.
Given that I now know that lcd_enable is not an alternative to this patch, but adding a callback is the only method to load a bitmap and have it shown, I understand now that this patch is not just a formal / cleanup change, but adds an actually missing feature. So I am fine with having it inside lcd_logo.
Quoted 7 lines
Quoted 5 lines
btw, I think, loading the image in
lcd_enable()won't even work sincelcd_enableis actually beforelcd_clear. Scanning some boards which load inlcd_enable, they seem to callbmp_displaymanually. So that makes this patch no longer optional, but is actually required and is an improvement....Ok. So we agree that this can't be done in
lcd_enable().
yes.
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
I'd like to hear Anatolij's opinion on this.
yes, me too. I like the
__weakfar more than requiring aCONFIG_toenable a callback. I cannot think of a reason why these__weakfunctions cannot be documented. So that's up to Anatolij.Usually, I also like the
__weakapproach, but this time the intention was to document the feature and hopefully stop the lcd_*() API abuse.
Regards,
Jeroen
PATCH 3/5 cm-t35: add support for dvi displays
In reply to Nikita KiryanovAdd support for dvi displays with user selectable dvi presets.
--- board/cm_t35/Makefile | 1 + board/cm_t35/cm_t35.c | 3 + board/cm_t35/display.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/cm_t35.h | 6 ++ 4 files changed, 223 insertions(+) create mode 100644 board/cm_t35/display.c
board/cm_t35/Makefile +1 -0
@@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(BOARD).o COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += eeprom.o +COBJS-$(CONFIG_LCD) += display.o COBJS := cm_t35.o leds.o $(COBJS-y)
board/cm_t35/cm_t35.c +3 -0
@@ -216,6 +216,9 @@ static void cm_t3x_set_common_muxconf(void)/* SB-T35 Ethernet */ MUX_VAL (CP (GPMC_NCS4 ), (IEN |PTU |EN |M0 ));/*GPMC_nCS4*/ +/* DVI enable */ +MUX_VAL (CP (GPMC_NCS3 ), (IDIS |PTU |DIS |M4 ));/*GPMC_nCS3*/ +/* CM-T3x Ethernet */ MUX_VAL (CP (GPMC_NCS5 ), (IDIS |PTU |DIS |M0 ));/*GPMC_nCS5*/ MUX_VAL (CP (GPMC_CLK ), (IEN |PTD |DIS |M4 ));/*GPIO_59*/
board/cm_t35/display.c new file +213 -0
213 lines not on this page · raw message
include/configs/cm_t35.h +6 -0
@@ -339,4 +339,10 @@#define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ #endif +/* Display Configuration */ +#define CONFIG_OMAP3_GPIO_2 +#define CONFIG_VIDEO_OMAP3 + +#define CONFIG_LCD +#endif /* __CONFIG_H */
Signature · 1 line
1.7.10.4
PATCH 3/5 cm-t35: add support for dvi displays
Hello Nikita,
Quoted 80 lines · Nikita Kiryanov
Add support for dvi displays with user selectable dvi presets.
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>--- board/cm_t35/Makefile | 1 + board/cm_t35/cm_t35.c | 3 + board/cm_t35/display.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/cm_t35.h | 6 ++ 4 files changed, 223 insertions(+)create mode 100644
board/cm_t35/display.cboard/cm_t35/Makefile +1
-0@@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(BOARD).o COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += eeprom.o +COBJS-$(CONFIG_LCD) += display.o COBJS := cm_t35.o leds.o $(COBJS-y)board/cm_t35/cm_t35.c +3
-0@@ -216,6 +216,9 @@ static void cm_t3x_set_common_muxconf(void)/* SB-T35 Ethernet */ MUX_VAL (CP (GPMC_NCS4 ), (IEN |PTU |EN |M0 ));/*GPMC_nCS4*/ +/* DVI enable */ +MUX_VAL (CP (GPMC_NCS3 ), (IDIS |PTU |DIS |M4 ));/*GPMC_nCS3*/ +/* CM-T3x Ethernet */ MUX_VAL (CP (GPMC_NCS5 ), (IDIS |PTU |DIS |M0 ));/*GPMC_nCS5*/ MUX_VAL (CP (GPMC_CLK ), (IEN |PTD |DIS |M4 ));/*GPIO_59*/ board/cm_t35/display.c new file +36
-0@@ -0,0 +1,213 @@ +/* +* (C) Copyright 2012 CompuLab, Ltd. <www.compulab.co.il> +* +* Authors: Nikita Kiryanov <[email protected]> +* +* See file CREDITS for list of people who contributed to this +* project. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* 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, write to the Free Software +* Foundation, Inc. +*/ +#include <common.h> +#include <asm/gpio.h> +#include <asm/io.h> +#include <stdio_dev.h> +#include <asm/arch/dss.h> +#include <lcd.h> +#include <asm/arch-omap3/dss.h> + +DECLARE_GLOBAL_DATA_PTR ; + +enum display_type { +NONE , +DVI , +};
board/cm_t35/display.c line 46
Quoted 10 lines
board/cm_t35/display.c new file +10
-0+ +/* +* The frame buffer is allocated before we have the chance to parse user input. +* To make sure enough memory is allocated for all resolutions, we define +* vl_{col | row} to the maximal resolution supported by OMAP3. +*/ +vidinfo_t panel_info = { + .vl_col =1400 , + .vl_row =1050 , + .vl_bpix =4 ,/* 16-bits pixel data */
There is no need to hardcode the magic 4, just use LCD_BPP
Quoted 109 lines
board/cm_t35/display.c new file +109
-0+ .cmap = (ushort *)0x80100000, +}; + +static struct panel_config panel_cfg; +static enum display_type lcd_def; + +static const struct panel_config preset_dvi_640X480 = { + .lcd_size =PANEL_LCD_SIZE (640 ,480 ), + .timing_h =DSS_HBP (48 ) |DSS_HFP (16 ) |DSS_HSW (96 ), + .timing_v =DSS_VBP (33 ) |DSS_VFP (10 ) |DSS_VSW (2 ), + .divisor =12 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; + +static const struct panel_config preset_dvi_800X600 = { + .lcd_size =PANEL_LCD_SIZE (800 ,600 ), + .timing_h =DSS_HBP (88 ) |DSS_HFP (40 ) |DSS_HSW (128 ), + .timing_v =DSS_VBP (23 ) |DSS_VFP (1 ) |DSS_VSW (4 ), + .divisor =8 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; + +static const struct panel_config preset_dvi_1024X768 = { + .lcd_size =PANEL_LCD_SIZE (1024 ,768 ), + .timing_h =DSS_HBP (160 ) |DSS_HFP (24 ) |DSS_HSW (136 ), + .timing_v =DSS_VBP (29 ) |DSS_VFP (3 ) |DSS_VSW (6 ), + .divisor =5 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; + +static const struct panel_config preset_dvi_1152X864 = { + .lcd_size =PANEL_LCD_SIZE (1152 ,864 ), + .timing_h =DSS_HBP (256 ) |DSS_HFP (64 ) |DSS_HSW (128 ), + .timing_v =DSS_VBP (32 ) |DSS_VFP (1 ) |DSS_VSW (3 ), + .divisor =3 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; + +static const struct panel_config preset_dvi_1280X960 = { + .lcd_size =PANEL_LCD_SIZE (1280 ,960 ), + .timing_h =DSS_HBP (312 ) |DSS_HFP (96 ) |DSS_HSW (112 ), + .timing_v =DSS_VBP (36 ) |DSS_VFP (1 ) |DSS_VSW (3 ), + .divisor =3 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; + +static const struct panel_config preset_dvi_1280X1024 = { + .lcd_size =PANEL_LCD_SIZE (1280 ,1024 ), + .timing_h =DSS_HBP (248 ) |DSS_HFP (48 ) |DSS_HSW (112 ), + .timing_v =DSS_VBP (38 ) |DSS_VFP (1 ) |DSS_VSW (3 ), + .divisor =3 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; + +/* +* set_resolution_params() +* +* Due to usage of multiple display related APIs resolution data is located in +* more than one place. This function updates them all. +*/ +static void set_resolution_params (int x,int y) +{ + panel_cfg.lcd_size =PANEL_LCD_SIZE (x, y); + panel_info.vl_col = x; + panel_info.vl_row = y; + lcd_line_length = (panel_info.vl_col *NBITS (panel_info.vl_bpix)) /8 ; +} + +static void set_preset (const struct panel_config preset,int x_res,int y_res) +{ + panel_cfg = preset; +set_resolution_params (x_res, y_res); +} + +static enum display_type set_dvi_preset (const struct panel_config preset, +int x_res,int y_res) +{ +set_preset (preset, x_res, y_res); +return DVI ; +} + +/* +* env_parse_displaytype() - parse display type. +* +* Parses the environment variable "displaytype", which contains the +* name of the display type or preset, in which case it applies its +* configurations. +* +* Returns the type of display that was specified. +*/ +static enum display_type env_parse_displaytype (char *displaytype) +{ +if (!strncmp (displaytype,"dvi640x480" ,10 )) +return set_dvi_preset (preset_dvi_640X480,640 ,480 ); +else if (!strncmp (displaytype,"dvi800x600" ,10 )) +return set_dvi_preset (preset_dvi_800X600,800 ,600 ); +else if (!strncmp (displaytype,"dvi1024x768" ,11 ))
board/cm_t35/display.c line 165
Quoted 10 lines
board/cm_t35/display.c new file +10
-0+return set_dvi_preset (preset_dvi_1024X768,1024 ,768 ); + else if (!strncmp(displaytype, "dvi1152x864", 11)) +return set_dvi_preset (preset_dvi_1152X864,1152 ,864 ); + else if (!strncmp(displaytype, "dvi1280x960", 11)) +return set_dvi_preset (preset_dvi_1280X960,1280 ,960 ); + else if (!strncmp(displaytype, "dvi1280x1024", 12)) +return set_dvi_preset (preset_dvi_1280X1024,1280 ,1024 ); + +return NONE ; +}
I think the lcd_line_length is no longer needed. ( but I haven't checked) Wondering if this should be board specific..
board/cm_t35/display.c line 174
Quoted 9 lines
board/cm_t35/display.c new file +9
-0+ +int lcd_line_length; +int lcd_color_fg; +int lcd_color_bg; +void *lcd_base; +short console_col; +short console_row; +void *lcd_console_address; +
fyi, I try to get rid of those, see
http://patchwork.ozlabs.org/patch/211562/.
Will repost v2 after this... No idea how this gets merged, but you might end with some unused globals.
Quoted 11 lines
board/cm_t35/display.c new file +11
-0+void lcd_ctrl_init (void *lcdbase) +{ +struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ; +struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *displaytype =getenv ("displaytype" ); + +if (displaytype ==NULL ) +return ; + + lcd_def =env_parse_displaytype (displaytype); +if (lcd_def ==NONE )
board/cm_t35/display.c line 195
Quoted 10 lines
board/cm_t35/display.c new file +10
-0+return ; + + panel_cfg.frame_buffer = lcdbase; +omap3_dss_panel_config (&panel_cfg); +/* +* Pixel clock is defined with many divisions and only few +* multiplications of the system clock. Since DSS FCLK divisor is set +* to 16 by default, we need to set it to a smaller value, like 3 +* (chosen via trial and error). +*/
bah, guessing timer values, this might help you
https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c
(a bit old, but simply downloading the file should work, and yes it might warn a bit)
The whole routine should go to the video driver in my opinion.
board/cm_t35/display.c line 203
Quoted 8 lines
board/cm_t35/display.c new file +8
-0+clrsetbits_le32 (&prcm->clksel_dss,0xF ,3 ); +/* +* Some of what the omap3_dss_panel_config() function did, needs to be +* adjusted, otherwise the image will be messed up/not appear at all. +*/ +clrsetbits_le32 (&dispc->gfx_attributes,0xF <<1 ,GFXFORMAT_RGB16 <<1 ); +clrsetbits_le32 (&dispc->gfx_attributes,0x3 <<6 ,GFXBURSTSIZE16 <<6 ); +}
mmm, do you really need 16 bit support? lcd.c is easily extended to 32 bit support (I have a patch for it) and then you can drop the driver adjustment. Anyway, if you want 16 bit support it should go into the driver and not in your board in my opinion.
Quoted 14 lines
board/cm_t35/display.c new file +9
-0+void lcd_enable (void ) +{ +if (lcd_def ==DVI ) { +gpio_direction_output (54 ,0 );/* Turn on DVI */ +omap3_dss_enable (); + } +} + +void lcd_setcolreg (ushort regno,ushort red,ushort green,ushort blue) {}include/configs/cm_t35.h +0
-0
include/configs/cm_t35.h line 348
Quoted 10 lines
include/configs/cm_t35.h +6
-0@@ -339,4 +339,10 @@#define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ #endif +/* Display Configuration */ +#define CONFIG_OMAP3_GPIO_2 +#define CONFIG_VIDEO_OMAP3 + +#define CONFIG_LCD +#endif /* __CONFIG_H */
Regards,
Jeroen
PATCH 3/5 cm-t35: add support for dvi displays
Hi Jeroen,
Quoted 8 lines · Jeroen Hofstee
Hello Nikita,
Quoted 5 lines · Nikita Kiryanov
Add support for dvi displays with user selectable dvi presets.
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>---
[...]
Quoted 5 lines
Quoted 5 lines
board/cm_t35/display.c new file +3
-0+ +/* +* The frame buffer is allocated before we have the chance to parse user input.
+ * To make sure enough memory is allocated for all resolutions, we
Quoted 8 lines
board/cm_t35/display.c line 46
Quoted 7 lines
define
+ * vl_{col | row} to the maximal resolution supported by OMAP3.
+ */board/cm_t35/display.c new file +4
-0+vidinfo_t panel_info = { + .vl_col =1400 , + .vl_row =1050 , + .vl_bpix =4 ,/* 16-bits pixel data */ There is no need to hardcode the magic 4, just use
LCD_BPP
Good point.
Quoted 16 lines
Quoted 16 lines
board/cm_t35/display.c new file +16
-0+ .cmap = (ushort *)0x80100000, +}; + +static struct panel_config panel_cfg; +static enum display_type lcd_def; + +static const struct panel_config preset_dvi_640X480 = { + .lcd_size =PANEL_LCD_SIZE (640 ,480 ), + .timing_h =DSS_HBP (48 ) |DSS_HFP (16 ) |DSS_HSW (96 ), + .timing_v =DSS_VBP (33 ) |DSS_VFP (10 ) |DSS_VSW (2 ), + .divisor =12 | (1 <<16 ), + .data_lines =LCD_INTERFACE_24_BIT , + .panel_type =ACTIVE_DISPLAY , + .load_mode =2 , +}; +
[...]
Quoted 7 lines
Quoted 7 lines
board/cm_t35/display.c new file +7
-0+else if (!strncmp (displaytype,"dvi800x600" ,10 )) +return set_dvi_preset (preset_dvi_800X600,800 ,600 ); +else if (!strncmp (displaytype,"dvi1024x768" ,11 )) +return set_dvi_preset (preset_dvi_1024X768,1024 ,768 ); +else if (!strncmp (displaytype,"dvi1152x864" ,11 )) +return set_dvi_preset (preset_dvi_1152X864,1152 ,864 ); +else if (!strncmp (displaytype,"dvi1280x960" ,11 ))
Quoted 8 lines
board/cm_t35/display.c line 165
Quoted 6 lines
board/cm_t35/display.c new file +6
-0+return set_dvi_preset (preset_dvi_1280X960,1280 ,960 ); + else if (!strncmp(displaytype, "dvi1280x1024", 12)) +return set_dvi_preset (preset_dvi_1280X1024,1280 ,1024 ); + +return NONE ; +}I think the
lcd_line_lengthis no longer needed. ( but I haven't checked) Wondering if this should be board specific..
These variables are here because at the time the implementation of lcd.c required them to be defined by the board. If you succeed in removing them it will be a simple matter of a clean up patch.
Quoted 6 lines
Quoted 6 lines
board/cm_t35/display.c new file +6
-0+ +int lcd_line_length; +int lcd_color_fg; +int lcd_color_bg; +void *lcd_base; +short console_col;
Quoted 8 lines
board/cm_t35/display.c line 174
Quoted 3 lines
board/cm_t35/display.c new file +3
-0+short console_row; +void *lcd_console_address; +fyi, I try to get rid of those, see
http://patchwork.ozlabs.org/patch/211562/.
Will repost v2 after this... No idea how this gets merged, but you might end
with some unused globals.
Yes that should be the extent of the "damage".
Quoted 25 lines
Quoted 19 lines
board/cm_t35/display.c new file +18
-0+void lcd_ctrl_init (void *lcdbase) +{ +struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ; +struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *displaytype =getenv ("displaytype" ); + +if (displaytype ==NULL ) +return ; + + lcd_def =env_parse_displaytype (displaytype); +if (lcd_def ==NONE ) +return ; + + panel_cfg.frame_buffer = lcdbase; +omap3_dss_panel_config (&panel_cfg); +/* +* Pixel clock is defined with many divisions and only few +* multiplications of the system clock. Since DSS FCLK divisor is set
board/cm_t35/display.c line 195
Quoted 3 lines
board/cm_t35/display.c new file +3
-0+* to 16 by default, we need to set it to a smaller value, like 3 +* (chosen via trial and error). +*/ bah, guessing timer values, this might help you
https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c
Quoted 2 lines
(a bit old, but simply downloading the file should work, and yes it might warn a bit)
Thanks.
I'll consider it for future extensions to this code, but for the time being the guess serves its purpose.
Quoted 2 lines
The whole routine should go to the video driver in my opinion.
What this function is, is predefines + call to omap3_dss_panel_config()
+ some corrections.
Anything related to the predefines is not generic. They have many assumptions in them (such as whether the screen is active or passive) and they are selected using a user interface that is also specific to our board.
The adjustment after the call to omap3_dss_panel_config() is, once again, specific to our own choices.
So overall, I don't think this is fit to be a generic driver function.
Quoted 12 lines
Quoted 8 lines
board/cm_t35/display.c new file +3
-0+clrsetbits_le32 (&prcm->clksel_dss,0xF ,3 ); +/* +* Some of what the omap3_dss_panel_config() function did, needs to be
+ * adjusted, otherwise the image will be messed up/not appear at
all.(quoted diff) +2
-0+ */ + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16Quoted 4 lines
<< 1);
+ clrsetbits_le32(&dispc->gfx_attributes,0x3<< 6, GFXBURSTSIZE16
<< 6);
+}
Quoted 1 line
mmm, do you really need 16 bit support?
Yes, we do want 16 bit support.
Quoted 4 lines
lcd.cis easily extended
to 32 bit support (I have a patch for it) and then you can drop the driver adjustment. Anyway, if you want 16 bit support it should go into the driver and not in your board in my opinion.
Addressed above.
Quoted 27 lines
Quoted 15 lines
board/cm_t35/display.c new file +9
-0+void lcd_enable (void ) +{ +if (lcd_def ==DVI ) { +gpio_direction_output (54 ,0 );/* Turn on DVI */ +omap3_dss_enable (); + } +} + +void lcd_setcolreg(ushort regno, ushort red, ushort green, ushortblue) {}
include/configs/cm_t35.h +0
-0include/configs/cm_t35.h line 347
Quoted 9 lines
include/configs/cm_t35.h +6
-0@@ -339,4 +339,10 @@#define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ #endif +/* Display Configuration */ +#define CONFIG_OMAP3_GPIO_2 +#define CONFIG_VIDEO_OMAP3 + +#define CONFIG_LCD +#endif /* __CONFIG_H */ Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 3/5 cm-t35: add support for dvi displays
Quoted 18 lines · Nikita Kiryanov
Quoted 17 lines
Quoted 4 lines
board/cm_t35/display.c new file +3
-0+else if (!strncmp (displaytype,"dvi800x600" ,10 )) +return set_dvi_preset (preset_dvi_800X600,800 ,600 ); +else if (!strncmp (displaytype,"dvi1024x768" ,11 ))board/cm_t35/display.c line 165
Quoted 10 lines
board/cm_t35/display.c new file +10
-0+return set_dvi_preset (preset_dvi_1024X768,1024 ,768 ); + else if (!strncmp(displaytype, "dvi1152x864", 11)) +return set_dvi_preset (preset_dvi_1152X864,1152 ,864 ); + else if (!strncmp(displaytype, "dvi1280x960", 11)) +return set_dvi_preset (preset_dvi_1280X960,1280 ,960 ); + else if (!strncmp(displaytype, "dvi1280x1024", 12)) +return set_dvi_preset (preset_dvi_1280X1024,1280 ,1024 ); + +return NONE ; +}I think the
lcd_line_lengthis no longer needed. ( but I haven't checked)
Wondering if this should be board specific..
Quoted 3 lines
These variables are here because at the time the implementation of
lcd.crequired them to be defined by the board. If you succeed in removing them it will be a simple matter of a clean up patch.
What I meant is that Stephan Warren posted a patch to fix the lcd_line_length
update in general, see http://patchwork.ozlabs.org/patch/212378/. (Which I thought was picked up, but isn't yet), instead of fixing it in boards, like you do.
Quoted 27 lines
Quoted 27 lines
Quoted 20 lines
board/cm_t35/display.c new file +18
-0+void lcd_ctrl_init (void *lcdbase) +{ +struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ; +struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *displaytype =getenv ("displaytype" ); + +if (displaytype ==NULL ) +return ; + + lcd_def =env_parse_displaytype (displaytype); +if (lcd_def ==NONE ) +return ; + + panel_cfg.frame_buffer = lcdbase; +omap3_dss_panel_config (&panel_cfg); +/* +* Pixel clock is defined with many divisions and only few +* multiplications of the system clock. Since DSS FCLK divisor is set
board/cm_t35/display.c line 195
Quoted 3 lines
board/cm_t35/display.c new file +3
-0+* to 16 by default, we need to set it to a smaller value, like 3 +* (chosen via trial and error). +*/ bah, guessing timer values, this might help you
https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c
Quoted 7 lines
Quoted 2 lines
(a bit old, but simply downloading the file should work, and yes it might warn a bit)
Thanks.
I'll consider it for future extensions to this code, but for the time being the guess serves its purpose.
What purpose does it serve exactly? The link I mentioned is not to update the
code per definition, but to be a bit more explicit why the timer adjustments are
needed instead of "this seems to work.."
Quoted 10 lines
Quoted 2 lines
The whole routine should go to the video driver in my opinion.
What this function is, is predefines + call to
omap3_dss_panel_config()
+ some corrections.
Anything related to the predefines is not generic. They have many assumptions in them (such as whether the screen is active or passive) and they are selected using a user interface that is also specific to our board.
Quoted 5 lines
The adjustment after the call to
omap3_dss_panel_config()is, once again, specific to our own choices.So overall, I don't think this is fit to be a generic driver function.
The idea would be that the driver could optionally check the env for a display configuration. If not found or not enabled call board_video_init. So there is a single driver for video and lcd and configurable by the environment and you can also have all control of it in the board. I don't have the time currently to check if this would actually work, but I don't see a reason why not (perhaps I can check something during the weekend).
And I didn't mean the predefined lcd configs. I am fine with you having them in you board / tested to work / delivered with them etc. But you add custom omap frame buffers settings, set by the user and I don't think that part should go into a board, since it is at least common to omap(3/4?) (or at least should be in my opinion).
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
+
clrsetbits_le32(&prcm->clksel_dss, 0xF, 3);(quoted diff) +2
-0+ /* + * Some of what the omap3_dss_panel_config() function did, needsto be
+ * adjusted, otherwise the image will be messed up/not appear at
all.(quoted diff) +2
-0+ */ + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16<< 1);
+ clrsetbits_le32(&dispc->gfx_attributes,0x3<< 6, GFXBURSTSIZE16
<< 6);
+}
Quoted 3 lines
Quoted 1 line
mmm, do you really need 16 bit support?
Yes, we do want 16 bit support.
Why?
Quoted 9 lines
Quoted 7 lines
lcd.cis easily extended
to 32 bit support (I have a patch for it) and then you can drop the driver
adjustment. Anyway, if you want 16 bit support it should go into the driver
and not in your board in my opinion.Addressed above.
well the same why... you drive them as 24 bit panels, why do you want a lower
resolution in software?
Regards,
Jeroen
PATCH 3/5 cm-t35: add support for dvi displays
Quoted 18 lines · Jeroen Hofstee
Quoted 17 lines · Nikita Kiryanov
Quoted 16 lines
Quoted 4 lines
board/cm_t35/display.c new file +3
-0+else if (!strncmp (displaytype,"dvi800x600" ,10 )) +return set_dvi_preset (preset_dvi_800X600,800 ,600 ); +else if (!strncmp (displaytype,"dvi1024x768" ,11 ))board/cm_t35/display.c line 165
Quoted 10 lines
board/cm_t35/display.c new file +10
-0+return set_dvi_preset (preset_dvi_1024X768,1024 ,768 ); + else if (!strncmp(displaytype, "dvi1152x864", 11)) +return set_dvi_preset (preset_dvi_1152X864,1152 ,864 ); + else if (!strncmp(displaytype, "dvi1280x960", 11)) +return set_dvi_preset (preset_dvi_1280X960,1280 ,960 ); + else if (!strncmp(displaytype, "dvi1280x1024", 12)) +return set_dvi_preset (preset_dvi_1280X1024,1280 ,1024 ); + +return NONE ; +}I think the
lcd_line_lengthis no longer needed. ( but I haven't checked) Wondering if this should be board specific..
Quoted 7 lines
Quoted 3 lines
These variables are here because at the time the implementation of
lcd.crequired them to be defined by the board. If you succeed in removing them it will be a simple matter of a clean up patch.What I meant is that Stephan Warren posted a patch to fix the
lcd_line_lengthupdate in general, see http://patchwork.ozlabs.org/patch/212378/. (Which I thought was picked up, but isn't yet), instead of fixing it in boards, like you do.
Unless. explicitly, requested by Anatolij,
we should not wait for specific patches to be merged or not merged. This does not work like this.
After improvements are made, the code can be adjusted - that's what the -rc cycle is for.
Quoted 30 lines
Quoted 30 lines
Quoted 29 lines
Quoted 21 lines
board/cm_t35/display.c new file +18
-0+void lcd_ctrl_init (void *lcdbase) +{ +struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ; +struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *displaytype =getenv ("displaytype" ); + +if (displaytype ==NULL ) +return ; + + lcd_def =env_parse_displaytype (displaytype); +if (lcd_def ==NONE ) +return ; + + panel_cfg.frame_buffer = lcdbase; +omap3_dss_panel_config (&panel_cfg); +/* +* Pixel clock is defined with many divisions and only few +* multiplications of the system clock. Since DSS FCLK divisor is set
board/cm_t35/display.c line 195
Quoted 3 lines
board/cm_t35/display.c new file +3
-0+* to 16 by default, we need to set it to a smaller value, like 3 +* (chosen via trial and error). +*/ bah, guessing timer values, this might help you
https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_timing.c(a bit old, but simply downloading the file should work, and yes it might warn a bit)
Quoted 7 lines
Quoted 4 lines
Thanks.
I'll consider it for future extensions to this code, but for the time being the guess serves its purpose.What purpose does it serve exactly? The link I mentioned is not to update the code per definition, but to be a bit more explicit why the timer adjustments are needed instead of "this seems to work.."
Thanks for the explanation, we will look into this. Currently, I don't see a good reason we should miss the merge window just to adopt those above. We will put it on our TODO list. Thanks!
Quoted 15 lines
Quoted 10 lines
Quoted 2 lines
The whole routine should go to the video driver in my opinion.
What this function is, is predefines + call to
omap3_dss_panel_config()
+ some corrections.
Anything related to the predefines is not generic. They have many assumptions in them (such as whether the screen is active or passive) and they are selected using a user interface that is also specific to our board.Quoted 5 lines
The adjustment after the call to
omap3_dss_panel_config()is, once again, specific to our own choices.So overall, I don't think this is fit to be a generic driver function.
Quoted 7 lines
The idea would be that the driver could optionally check the env for a display configuration. If not found or not enabled call
board_video_init. So there is a single driver for video and lcd and configurable by the environment and you can also have all control of it in the board. I don't have the time currently to check if this would actually work, but I don't see a reason why not (perhaps I can check something during the weekend).
It should work, but first we should decide if it is suitable for all OMAP DSS users.
So I would suggest we see how it works and if people decide this is good enough to be in the generic DSS code, we can move it to live there.
Quoted 6 lines
And I didn't mean the predefined lcd configs. I am fine with you having them in you board / tested to work / delivered with them etc. But you add custom omap frame buffers settings, set by the user and I don't think that part should go into a board, since it is at least common to omap(3/4?) (or at least should be in my opinion).
If I get you correctly,
you are suggesting to parametrize the GFXFORMAT_RGB16 and GFXBURSTSIZE16? This can be done.
Quoted 18 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
+
clrsetbits_le32(&prcm->clksel_dss, 0xF, 3);(quoted diff) +2
-0+ /* + * Some of what the omap3_dss_panel_config() function did, needsto be
+ * adjusted, otherwise the image will be messed up/not appear at
all.(quoted diff) +2
-0+ */ + clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16<< 1);
+ clrsetbits_le32(&dispc->gfx_attributes,0x3<< 6, GFXBURSTSIZE16
<< 6);
+}Quoted 3 lines
Quoted 1 line
mmm, do you really need 16 bit support?
Yes, we do want 16 bit support.
Why?
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
lcd.cis easily extended
to 32 bit support (I have a patch for it) and then you can drop the driver adjustment. Anyway, if you want 16 bit support it should go into the driver and not in your board in my opinion.Addressed above.
well the same why... you drive them as 24 bit panels, why do you want a lower resolution in software?
Actually, we also have 16 and 18 bit panels, just not in this patch set. We want the basic stuff to go in and then other stuff.
But, you are right, probably parameterizing these or deriving from some other setting should be done. We will look into this.
Signature · 2 lines
Regards, Igor.
PATCH 4/5 cm-t35: add support for user defined lcd parameters
In reply to Nikita KiryanovAdd support for user defined lcd parameters for cm-t35 splash screen.
--- board/cm_t35/display.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 210 insertions(+), 3 deletions(-)
board/cm_t35/display.c +210 -3
@@ -3,6 +3,8 @@* * Authors: Nikita Kiryanov <[email protected]> * +* Parsing code based on linux/drivers/video/pxafb.c +* * See file CREDITS for list of people who contributed to this * project. *
@@ -33,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR;enum display_type {NONE ,DVI , +DVI_CUSTOM , };/*
@@ -138,6 +141,205 @@ static enum display_type set_dvi_preset(const struct panel_config preset, }/* +* parse_mode() - parse the mode parameter of custom lcd settings +* +* @mode: <res_x>x<res_y> +* +* Returns -1 on error, 0 on success. +*/ +static int parse_mode (const char *mode) +{ +unsigned int modelen =strlen (mode); +int res_specified =0 ; +unsigned int xres =0 , yres =0 ; +int yres_specified =0 ; +int i; + +for (i = modelen -1 ; i >=0 ; i--) { +switch (mode[i]) { +case 'x' : +if (!yres_specified) { + yres =simple_strtoul (&mode[i +1 ],NULL ,0 ); + yres_specified =1 ; + }else { + goto done_parsing; + } + +break ; + case '0' ... '9': +break ; +default : + goto done_parsing; + } + } + +if (i <0 && yres_specified) { + xres =simple_strtoul (mode,NULL ,0 ); + res_specified =1 ; + } + +done_parsing: +if (res_specified) { +set_resolution_params (xres, yres); + }else { +printf ("LCD: invalid mode: %s\n" , mode); +return -1 ; + } + +return 0 ; +} + +#define PIXEL_CLK_NUMERATOR (26 * 432 / 39) +/* +* parse_pixclock() - Parse the pixclock parameter of custom lcd settings +* +* @pixclock: the desired pixel clock +* +* Returns -1 on error, 0 on success. +* +* Handling the pixel_clock: +* +* Pixel clock is defined in the OMAP35x TRM as follows: +* pixel_clock = +* (SYS_CLK * 2 * PRCM.CM_CLKSEL2_PLL[18:8]) / +* (DSS.DISPC_DIVISOR[23:16] * DSS.DISPC_DIVISOR[6:0] * +* PRCM.CM_CLKSEL_DSS[4:0] * (PRCM.CM_CLKSEL2_PLL[6:0] + 1)) +* +* In practice, this means that in order to set the +* divisor for the desired pixel clock one needs to +* solve the following equation: +* +* 26 * 432 / (39 * <pixel_clock>) = DSS.DISPC_DIVISOR[6:0] +* +* NOTE: the explicit equation above is reduced. Do not +* try to infer anything from these numbers. +*/ +static int parse_pixclock (char *pixclock) +{ +int divisor, pixclock_val; +char *pixclk_start = pixclock; + + pixclock_val =simple_strtoul (pixclock, &pixclock,10 ); + divisor =DIV_ROUND_UP (PIXEL_CLK_NUMERATOR , pixclock_val); +/* 0 and 1 are illegal values for PCD */ +if (divisor <=1 ) + divisor =2 ; + + panel_cfg.divisor = divisor | (1 <<16 ); +if (pixclock[0 ] !='\0' ) { +printf ("LCD: invalid value for pixclock:%s\n" , pixclk_start); +return -1 ; + } + +return 0 ; +} + +/* +* parse_setting() - parse a single setting of custom lcd parameters +* +* @setting: The custom lcd setting <name>:<value> +* +* Returns -1 on failure, 0 on success. +*/ +static int parse_setting (char *setting) +{ +int num_val; +char *setting_start = setting; + +if (!strncmp (setting,"mode:" ,5 )) { +return parse_mode (setting +5 ); + }else if (!strncmp (setting,"pixclock:" ,9 )) { +return parse_pixclock (setting +9 ); + }else if (!strncmp (setting,"left:" ,5 )) { + num_val =simple_strtoul (setting +5 , &setting,0 ); + panel_cfg.timing_h |=DSS_HBP (num_val); + }else if (!strncmp (setting,"right:" ,6 )) { + num_val =simple_strtoul (setting +6 , &setting,0 ); + panel_cfg.timing_h |=DSS_HFP (num_val); + }else if (!strncmp (setting,"upper:" ,6 )) { + num_val =simple_strtoul (setting +6 , &setting,0 ); + panel_cfg.timing_v |=DSS_VBP (num_val); + }else if (!strncmp (setting,"lower:" ,6 )) { + num_val =simple_strtoul (setting +6 , &setting,0 ); + panel_cfg.timing_v |=DSS_VFP (num_val); + }else if (!strncmp (setting,"hsynclen:" ,9 )) { + num_val =simple_strtoul (setting +9 , &setting,0 ); + panel_cfg.timing_h |=DSS_HSW (num_val); + }else if (!strncmp (setting,"vsynclen:" ,9 )) { + num_val =simple_strtoul (setting +9 , &setting,0 ); + panel_cfg.timing_v |=DSS_VSW (num_val); + }else if (!strncmp (setting,"hsync:" ,6 )) { +if (simple_strtoul (setting +6 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IHS ; +else + panel_cfg.pol_freq &= ~DSS_IHS ; + }else if (!strncmp (setting,"vsync:" ,6 )) { +if (simple_strtoul (setting +6 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IVS ; +else + panel_cfg.pol_freq &= ~DSS_IVS ; + }else if (!strncmp (setting,"outputen:" ,9 )) { +if (simple_strtoul (setting +9 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IEO ; +else + panel_cfg.pol_freq &= ~DSS_IEO ; + }else if (!strncmp (setting,"pixclockpol:" ,12 )) { +if (simple_strtoul (setting +12 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IPC ; +else + panel_cfg.pol_freq &= ~DSS_IPC ; + }else if (!strncmp (setting,"active" ,6 )) { + panel_cfg.panel_type =ACTIVE_DISPLAY ; +return 0 ;/* Avoid sanity check below */ + }else if (!strncmp (setting,"passive" ,7 )) { + panel_cfg.panel_type =PASSIVE_DISPLAY ; +return 0 ;/* Avoid sanity check below */ + }else if (!strncmp (setting,"display:" ,8 )) { +if (!strncmp (setting +8 ,"dvi" ,3 )) { + lcd_def =DVI_CUSTOM ; +return 0 ;/* Avoid sanity check below */ + } + }else { +printf ("LCD: unknown option %s\n" , setting_start); +return -1 ; + } + +if (setting[0 ] !='\0' ) { +printf ("LCD: invalid value for %s\n" , setting_start); +return -1 ; + } + +return 0 ; +} + +/* +* env_parse_customlcd() - parse custom lcd params from an environment variable. +* +* @custom_lcd_params: The environment variable containing the lcd params. +* +* Returns -1 on failure, 0 on success. +*/ +static int parse_customlcd (char *custom_lcd_params) +{ +char params_cpy[160 ]; +char *setting; + +strncpy (params_cpy, custom_lcd_params,160 ); + setting =strtok (params_cpy,"," ); +while (setting) { +if (parse_setting (setting) <0 ) +return -1 ; + + setting =strtok (NULL ,"," ); + } + +/* Currently we don't support changing this via custom lcd params */ + panel_cfg.data_lines =LCD_INTERFACE_24_BIT ; + +return 0 ; +} + +/* * env_parse_displaytype() - parse display type. * * Parses the environment variable "displaytype", which contains the
@@ -176,14 +378,19 @@ void lcd_ctrl_init(void *lcdbase) {struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ;struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *custom_lcd;char *displaytype =getenv ("displaytype" );if (displaytype ==NULL )return ; lcd_def =env_parse_displaytype (displaytype); -if (lcd_def ==NONE ) -return ; +/* If we did not recognize the preset, check if it's an env variable */ +if (lcd_def ==NONE ) { + custom_lcd =getenv (displaytype); +if (custom_lcd ==NULL ||parse_customlcd (custom_lcd) <0 ) +return ; + } panel_cfg.frame_buffer = lcdbase;omap3_dss_panel_config (&panel_cfg);
@@ -204,7 +411,7 @@ void lcd_ctrl_init(void *lcdbase)void lcd_enable (void ) { -if (lcd_def ==DVI ) { +if (lcd_def ==DVI || lcd_def ==DVI_CUSTOM ) {gpio_direction_output (54 ,0 );/* Turn on DVI */ omap3_dss_enable (); }
Signature · 1 line
1.7.10.4
PATCH 4/5 cm-t35: add support for user defined lcd parameters
Quoted 207 lines · Nikita Kiryanov
Add support for user defined lcd parameters for cm-t35 splash screen.
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>--- board/cm_t35/display.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 210 insertions(+), 3 deletions(-)board/cm_t35/display.c +177
-0@@ -3,6 +3,8 @@* * Authors: Nikita Kiryanov <[email protected]> * +* Parsing code based on linux/drivers/video/pxafb.c +* * See file CREDITS for list of people who contributed to this * project. * @@ -33,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR;enum display_type {NONE ,DVI , +DVI_CUSTOM , };/* @@ -138,6 +141,205 @@ static enum display_type set_dvi_preset(const struct panel_config preset, }/* +* parse_mode() - parse the mode parameter of custom lcd settings +* +* @mode: <res_x>x<res_y> +* +* Returns -1 on error, 0 on success. +*/ +static int parse_mode (const char *mode) +{ +unsigned int modelen =strlen (mode); +int res_specified =0 ; +unsigned int xres =0 , yres =0 ; +int yres_specified =0 ; +int i; + +for (i = modelen -1 ; i >=0 ; i--) { +switch (mode[i]) { +case 'x' : +if (!yres_specified) { + yres =simple_strtoul (&mode[i +1 ],NULL ,0 ); + yres_specified =1 ; + }else { + goto done_parsing; + } + +break ; + case '0' ... '9': +break ; +default : + goto done_parsing; + } + } + +if (i <0 && yres_specified) { + xres =simple_strtoul (mode,NULL ,0 ); + res_specified =1 ; + } + +done_parsing: +if (res_specified) { +set_resolution_params (xres, yres); + }else { +printf ("LCD: invalid mode: %s\n" , mode); +return -1 ; + } + +return 0 ; +} + +#define PIXEL_CLK_NUMERATOR (26 * 432 / 39) +/* +* parse_pixclock() - Parse the pixclock parameter of custom lcd settings +* +* @pixclock: the desired pixel clock +* +* Returns -1 on error, 0 on success. +* +* Handling the pixel_clock: +* +* Pixel clock is defined in the OMAP35x TRM as follows: +* pixel_clock = +* (SYS_CLK * 2 * PRCM.CM_CLKSEL2_PLL[18:8]) / +* (DSS.DISPC_DIVISOR[23:16] * DSS.DISPC_DIVISOR[6:0] * +* PRCM.CM_CLKSEL_DSS[4:0] * (PRCM.CM_CLKSEL2_PLL[6:0] + 1)) +* +* In practice, this means that in order to set the +* divisor for the desired pixel clock one needs to +* solve the following equation: +* +* 26 * 432 / (39 * <pixel_clock>) = DSS.DISPC_DIVISOR[6:0] +* +* NOTE: the explicit equation above is reduced. Do not +* try to infer anything from these numbers. +*/ +static int parse_pixclock (char *pixclock) +{ +int divisor, pixclock_val; +char *pixclk_start = pixclock; + + pixclock_val =simple_strtoul (pixclock, &pixclock,10 ); + divisor =DIV_ROUND_UP (PIXEL_CLK_NUMERATOR , pixclock_val); +/* 0 and 1 are illegal values for PCD */ +if (divisor <=1 ) + divisor =2 ; + + panel_cfg.divisor = divisor | (1 <<16 ); +if (pixclock[0 ] !='\0' ) { +printf ("LCD: invalid value for pixclock:%s\n" , pixclk_start); +return -1 ; + } + +return 0 ; +} + +/* +* parse_setting() - parse a single setting of custom lcd parameters +* +* @setting: The custom lcd setting <name>:<value> +* +* Returns -1 on failure, 0 on success. +*/ +static int parse_setting (char *setting) +{ +int num_val; +char *setting_start = setting; + +if (!strncmp (setting,"mode:" ,5 )) { +return parse_mode (setting +5 ); + }else if (!strncmp (setting,"pixclock:" ,9 )) { +return parse_pixclock (setting +9 ); + }else if (!strncmp (setting,"left:" ,5 )) { + num_val =simple_strtoul (setting +5 , &setting,0 ); + panel_cfg.timing_h |=DSS_HBP (num_val); + }else if (!strncmp (setting,"right:" ,6 )) { + num_val =simple_strtoul (setting +6 , &setting,0 ); + panel_cfg.timing_h |=DSS_HFP (num_val); + }else if (!strncmp (setting,"upper:" ,6 )) { + num_val =simple_strtoul (setting +6 , &setting,0 ); + panel_cfg.timing_v |=DSS_VBP (num_val); + }else if (!strncmp (setting,"lower:" ,6 )) { + num_val =simple_strtoul (setting +6 , &setting,0 ); + panel_cfg.timing_v |=DSS_VFP (num_val); + }else if (!strncmp (setting,"hsynclen:" ,9 )) { + num_val =simple_strtoul (setting +9 , &setting,0 ); + panel_cfg.timing_h |=DSS_HSW (num_val); + }else if (!strncmp (setting,"vsynclen:" ,9 )) { + num_val =simple_strtoul (setting +9 , &setting,0 ); + panel_cfg.timing_v |=DSS_VSW (num_val); + }else if (!strncmp (setting,"hsync:" ,6 )) { +if (simple_strtoul (setting +6 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IHS ; +else + panel_cfg.pol_freq &= ~DSS_IHS ; + }else if (!strncmp (setting,"vsync:" ,6 )) { +if (simple_strtoul (setting +6 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IVS ; +else + panel_cfg.pol_freq &= ~DSS_IVS ; + }else if (!strncmp (setting,"outputen:" ,9 )) { +if (simple_strtoul (setting +9 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IEO ; +else + panel_cfg.pol_freq &= ~DSS_IEO ; + }else if (!strncmp (setting,"pixclockpol:" ,12 )) { +if (simple_strtoul (setting +12 , &setting,0 ) ==0 ) + panel_cfg.pol_freq |=DSS_IPC ; +else + panel_cfg.pol_freq &= ~DSS_IPC ; + }else if (!strncmp (setting,"active" ,6 )) { + panel_cfg.panel_type =ACTIVE_DISPLAY ; +return 0 ;/* Avoid sanity check below */ + }else if (!strncmp (setting,"passive" ,7 )) { + panel_cfg.panel_type =PASSIVE_DISPLAY ; +return 0 ;/* Avoid sanity check below */ + }else if (!strncmp (setting,"display:" ,8 )) { +if (!strncmp (setting +8 ,"dvi" ,3 )) { + lcd_def =DVI_CUSTOM ; +return 0 ;/* Avoid sanity check below */ + } + }else { +printf ("LCD: unknown option %s\n" , setting_start); +return -1 ; + } + +if (setting[0 ] !='\0' ) { +printf ("LCD: invalid value for %s\n" , setting_start); +return -1 ; + } + +return 0 ; +} + +/* +* env_parse_customlcd() - parse custom lcd params from an environment variable. +*
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 327
Quoted 10 lines
board/cm_t35/display.c +10
-0+* @custom_lcd_params: The environment variable containing the lcd params. +* +* Returns -1 on failure, 0 on success. +*/ +static int parse_customlcd (char *custom_lcd_params) +{ +char params_cpy[160 ]; +char *setting; + +strncpy (params_cpy, custom_lcd_params,160 );
I fail to understand why you want to copy this.
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 338
Quoted 11 lines
board/cm_t35/display.c +11
-0+ setting =strtok (params_cpy,"," ); +while (setting) { +if (parse_setting (setting) <0 ) +return -1 ; + + setting =strtok (NULL ,"," ); + } + +/* Currently we don't support changing this via custom lcd params */ + panel_cfg.data_lines =LCD_INTERFACE_24_BIT ; +
again, if you only support 24 panels, why not drive them as such?
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 191
Quoted 3 lines
board/cm_t35/display.c +3
-0+return 0 ; +} +
Is above really board specific or should it be in omap_videomodes.c or whatever?
Quoted 27 lines
board/cm_t35/display.c +8
-2+/* * env_parse_displaytype() - parse display type. * * Parses the environment variable "displaytype", which contains the @@ -176,14 +378,19 @@ void lcd_ctrl_init(void *lcdbase) {struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ;struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *custom_lcd;char *displaytype =getenv ("displaytype" );if (displaytype ==NULL )return ; lcd_def =env_parse_displaytype (displaytype); -if (lcd_def ==NONE ) -return ; +/* If we did not recognize the preset, check if it's an env variable */ +if (lcd_def ==NONE ) { + custom_lcd =getenv (displaytype); +if (custom_lcd ==NULL ||parse_customlcd (custom_lcd) <0 ) +return ; + } panel_cfg.frame_buffer = lcdbase;omap3_dss_panel_config (&panel_cfg);
board/cm_t35/display.c void lcd_ctrl_init(void *lcdbase) · line 417
Quoted 8 lines
board/cm_t35/display.c +1
-1@@ -204,7 +411,7 @@ void lcd_ctrl_init(void *lcdbase)void lcd_enable (void ) { -if (lcd_def ==DVI ) { +if (lcd_def ==DVI || lcd_def ==DVI_CUSTOM ) {gpio_direction_output (54 ,0 );/* Turn on DVI */ omap3_dss_enable (); }
Regards,
Jeroen
PATCH 4/5 cm-t35: add support for user defined lcd parameters
Hi Jeroen,
Quoted 1 line · Jeroen Hofstee
On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
[...]
Quoted 9 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 327
Quoted 8 lines
+ * Returns -1 on failure, 0 on success.
+ */board/cm_t35/display.c +6
-0+static int parse_customlcd (char *custom_lcd_params) +{ +char params_cpy[160 ]; +char *setting; + +strncpy (params_cpy, custom_lcd_params,160 );I fail to understand why you want to copy this.
strtok modifies the string it operates on. The documentation for getenv states that you must not modify the string it returns.
Quoted 4 lines
Quoted 4 lines
board/cm_t35/display.c +4
-0+ setting =strtok (params_cpy,"," ); +while (setting) { +if (parse_setting (setting) <0 ) +return -1 ;
Quoted 8 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 338
Quoted 7 lines
board/cm_t35/display.c +7
-0+ + setting =strtok (NULL ,"," ); + } + +/* Currently we don't support changing this via custom lcd params */ + panel_cfg.data_lines =LCD_INTERFACE_24_BIT ; +again, if you only support 24 panels, why not drive them as such?
Can you please elaborate on this comment? I'm not entirely sure what inconsistencies you are referring to.
Quoted 5 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 191
Quoted 3 lines
board/cm_t35/display.c +3
-0+return 0 ; +} +Is above really board specific or should it be in
omap_videomodes.cor whatever?
Well, most of it is parsing for a custom feature, so I would say this is board specific.
Quoted 35 lines
Quoted 25 lines
board/cm_t35/display.c +3
-2+/* * env_parse_displaytype() - parse display type. * * Parses the environment variable "displaytype", which contains the @@ -176,14 +378,19 @@ void lcd_ctrl_init(void *lcdbase) {struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE ;struct prcm *prcm = (struct prcm *)PRCM_BASE ; +char *custom_lcd;char *displaytype =getenv ("displaytype" );if (displaytype ==NULL )return ; lcd_def =env_parse_displaytype (displaytype); -if (lcd_def ==NONE ) -return ; +/* If we did not recognize the preset, check if it's an env variable */
board/cm_t35/display.c +5
-0+if (lcd_def ==NONE ) { + custom_lcd =getenv (displaytype); +if (custom_lcd ==NULL ||parse_customlcd (custom_lcd) <0 ) +return ; + } panel_cfg.frame_buffer = lcdbase;omap3_dss_panel_config (&panel_cfg);board/cm_t35/display.c void lcd_ctrl_init(void *lcdbase) · line 416
Quoted 7 lines
board/cm_t35/display.c +1
-1@@ -204,7 +411,7 @@ void lcd_ctrl_init(void *lcdbase)void lcd_enable (void ) { -if (lcd_def ==DVI ) { +if (lcd_def ==DVI || lcd_def ==DVI_CUSTOM ) {gpio_direction_output (54 ,0 );/* Turn on DVI */ omap3_dss_enable (); }Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 4/5 cm-t35: add support for user defined lcd parameters
Hi Nikita,
Quoted 15 lines · Nikita Kiryanov
Hi Jeroen,
Quoted 1 line · Jeroen Hofstee
On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
[...]
Quoted 9 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 327
Quoted 8 lines
+ * Returns -1 on failure, 0 on success.
+ */board/cm_t35/display.c +6
-0+static int parse_customlcd (char *custom_lcd_params) +{ +char params_cpy[160 ]; +char *setting; + +strncpy (params_cpy, custom_lcd_params,160 );I fail to understand why you want to copy this.
Quoted 3 lines
strtok modifies the string it operates on. The documentation for getenv states that you must not modify the string it returns.
that seems to make sense...
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
board/cm_t35/display.c +5
-0+ setting =strtok (params_cpy,"," ); +while (setting) { +if (parse_setting (setting) <0 ) +return -1 ; +Quoted 8 lines
Review
Quoted 7 lines
board/cm_t35/display.c +4
-0+ setting =strtok (NULL ,"," ); + } + +/* Currently we don't support changing this via custom lcd params */
(quoted diff) +2
-0+ panel_cfg.data_lines = LCD_INTERFACE_24_BIT; +again, if you only support 24 panels, why not drive them as such?
Quoted 2 lines
Can you please elaborate on this comment? I'm not entirely sure what inconsistencies you are referring to.
You're driving only 24 bit panels, yet you want the software to drive 16 bit panels.
Why not keep the software frame buffer at 32 bits?
Quoted 9 lines
Quoted 6 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 191
Quoted 4 lines
board/cm_t35/display.c +3
-0+return 0 ; +} +Is above really board specific or should it be in
omap_videomodes.cor whatever?Well, most of it is parsing for a custom feature, so I would say this is board specific.
I can't understand how custom settings can be board specific, unless you mess with timer of course (but even then....).
Regards,
Jeroen
PATCH 4/5 cm-t35: add support for user defined lcd parameters
Quoted 46 lines · Jeroen Hofstee
Hi Nikita,
Quoted 15 lines · Nikita Kiryanov
Hi Jeroen,
Quoted 1 line · Jeroen Hofstee
On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
[...]
Quoted 9 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 327
Quoted 8 lines
+ * Returns -1 on failure, 0 on success.
+ */board/cm_t35/display.c +6
-0+static int parse_customlcd (char *custom_lcd_params) +{ +char params_cpy[160 ]; +char *setting; + +strncpy (params_cpy, custom_lcd_params,160 );I fail to understand why you want to copy this.
Quoted 3 lines
strtok modifies the string it operates on. The documentation for getenv states that you must not modify the string it returns.
that seems to make sense...
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
board/cm_t35/display.c +4
-0+ setting =strtok (params_cpy,"," ); +while (setting) { +if (parse_setting (setting) <0 ) +return -1 ;Quoted 8 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 338
Quoted 7 lines
board/cm_t35/display.c +7
-0+ + setting =strtok (NULL ,"," ); + } + +/* Currently we don't support changing this via custom lcd params */ + panel_cfg.data_lines =LCD_INTERFACE_24_BIT ; +again, if you only support 24 panels, why not drive them as such?
Quoted 2 lines
Can you please elaborate on this comment? I'm not entirely sure what inconsistencies you are referring to.
You're driving only 24 bit panels, yet you want the software to drive 16 bit panels. Why not keep the software frame buffer at 32 bits?
Quoted 7 lines
Quoted 6 lines
board/cm_t35/display.c static enum display_type set_dvi_preset(const struct panel_config preset, · line 191
Quoted 4 lines
board/cm_t35/display.c +3
-0+return 0 ; +} +Is above really board specific or should it be in
omap_videomodes.cor whatever?
Quoted 4 lines
Quoted 2 lines
Well, most of it is parsing for a custom feature, so I would say this is board specific.
I can't understand how custom settings can be board specific, unless you mess with timer of course (but even then....).
I fail to understand which timer are you talking about... Probably, you mean the DPLLs and the clocks?
Signature · 2 lines
Regards, Igor.
PATCH 5/5 cm-t35: add support for loading splash image from NAND
In reply to Nikita KiryanovAdd support for loading splash image from NAND
--- board/cm_t35/cm_t35.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/cm_t35.h | 4 +++ 2 files changed, 65 insertions(+)
board/cm_t35/cm_t35.c +61 -0
@@ -33,7 +33,9 @@#include <net.h> #include <i2c.h> #include <usb.h> +#include <nand.h> #include <twl4030.h> +#include <bmp_layout.h> #include <linux/compiler.h> #include <asm/io.h>
@@ -75,6 +77,65 @@ static u32 gpmc_nand_config[GPMC_MAX_REG] = { 0, }; +#ifdef CONFIG_LCD +#ifdef CONFIG_CMD_NAND +static int splash_load_from_nand (u32 bmp_load_addr) +{ +struct bmp_header *bmp_hdr; +int res, splash_screen_nand_offset =0x100000 ; +size_t bmp_size, bmp_header_size =sizeof (struct bmp_header ); + +if (bmp_load_addr + bmp_header_size >= gd->start_addr_sp) + goto splash_address_too_high; + + res =nand_read_skip_bad (&nand_info[nand_curr_device], + splash_screen_nand_offset, &bmp_header_size, + (u_char *)bmp_load_addr); +if (res <0 ) +return res; + + bmp_hdr = (struct bmp_header *)bmp_load_addr; + bmp_size =le32_to_cpu (bmp_hdr->file_size); + +if (bmp_load_addr + bmp_size >= gd->start_addr_sp) + goto splash_address_too_high; + +return nand_read_skip_bad (&nand_info[nand_curr_device], + splash_screen_nand_offset, &bmp_size, + (u_char *)bmp_load_addr); + +splash_address_too_high: +printf ("Error: splashimage address too high. Data overwrites U-Boot " +"and/or placed beyond DRAM boundaries.\n" ); + +return -1 ; +} +#else +static inline int splash_load_from_nand (void ) +{ +return -1 ; +} +#endif /* CONFIG_CMD_NAND */ + +int board_splash_screen_prepare (void ) +{ +char *env_splashimage_value; +u32 bmp_load_addr; + + env_splashimage_value =getenv ("splashimage" ); +if (env_splashimage_value ==NULL ) +return -1 ; + + bmp_load_addr =simple_strtoul (env_splashimage_value,0 ,16 ); +if (bmp_load_addr ==0 ) { +printf ("Error: bad splashimage address specified\n" ); +return -1 ; + } + +return splash_load_from_nand (bmp_load_addr); +} +#endif /* CONFIG_LCD */ +/* * Routine: board_init * Description: hardware init.
include/configs/cm_t35.h +4 -0
@@ -344,5 +344,9 @@#define CONFIG_VIDEO_OMAP3 #define CONFIG_LCD +#define CONFIG_SPLASH_SCREEN +#define CONFIG_CMD_BMP +#define CONFIG_BMP_16BPP +#define CONFIG_SPLASH_SCREEN_PREPARE #endif /* __CONFIG_H */
Signature · 1 line
1.7.10.4
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Hi Nikita,
Quoted 76 lines · Nikita Kiryanov
Add support for loading splash image from NAND
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>--- board/cm_t35/cm_t35.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/cm_t35.h | 4 +++ 2 files changed, 65 insertions(+)board/cm_t35/cm_t35.c +51
-0@@ -33,7 +33,9 @@#include <net.h> #include <i2c.h> #include <usb.h> +#include <nand.h> #include <twl4030.h> +#include <bmp_layout.h> #include <linux/compiler.h> #include <asm/io.h> @@ -75,6 +77,65 @@ static u32 gpmc_nand_config[GPMC_MAX_REG] = { 0, }; +#ifdef CONFIG_LCD +#ifdef CONFIG_CMD_NAND +static int splash_load_from_nand (u32 bmp_load_addr) +{ +struct bmp_header *bmp_hdr; +int res, splash_screen_nand_offset =0x100000 ; +size_t bmp_size, bmp_header_size =sizeof (struct bmp_header ); + +if (bmp_load_addr + bmp_header_size >= gd->start_addr_sp) + goto splash_address_too_high; + + res =nand_read_skip_bad (&nand_info[nand_curr_device], + splash_screen_nand_offset, &bmp_header_size, + (u_char *)bmp_load_addr); +if (res <0 ) +return res; + + bmp_hdr = (struct bmp_header *)bmp_load_addr; + bmp_size =le32_to_cpu (bmp_hdr->file_size); + +if (bmp_load_addr + bmp_size >= gd->start_addr_sp) + goto splash_address_too_high; + +return nand_read_skip_bad (&nand_info[nand_curr_device], + splash_screen_nand_offset, &bmp_size, + (u_char *)bmp_load_addr); + +splash_address_too_high: +printf ("Error: splashimage address too high. Data overwrites U-Boot " +"and/or placed beyond DRAM boundaries.\n" ); + +return -1 ; +} +#else +static inline int splash_load_from_nand (void ) +{ +return -1 ; +} +#endif /* CONFIG_CMD_NAND */ + +int board_splash_screen_prepare (void ) +{ +char *env_splashimage_value; +u32 bmp_load_addr; + + env_splashimage_value =getenv ("splashimage" ); +if (env_splashimage_value ==NULL ) +return -1 ; +
board/cm_t35/cm_t35.c static u32 gpmc_nand_config[GPMC_MAX_REG] = { · line 138
Quoted 10 lines
board/cm_t35/cm_t35.c +10
-0+ bmp_load_addr =simple_strtoul (env_splashimage_value,0 ,16 ); +if (bmp_load_addr ==0 ) { +printf ("Error: bad splashimage address specified\n" ); +return -1 ; + } + + return splash_load_from_nand(bmp_load_addr); +} +#endif /* CONFIG_LCD */ +
fyi, I noticed that my board compiled with gcc 4.7.3 from ELDK 5.3 will trap if the bitmap is not aligned. Aligned is a bit tricky though since the bitmap
has the signature, e.g. "BM" prepended and is thereafter 32 bit aligned (or at least combined fields of 32 bits). In my case displaying the bitmap now only works when loaded to an aligned address - 2. Since you accept the value from the user, which has no ability to restore it once set "incorrectly", you might want to check the load address (well obviously only if it is a problem in your case as well).
Regards,
Jeroen
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Hi Jeroen,
Quoted 91 lines · Jeroen Hofstee
Hi Nikita,
Quoted 76 lines · Nikita Kiryanov
Add support for loading splash image from NAND
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>--- board/cm_t35/cm_t35.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/cm_t35.h | 4 +++ 2 files changed, 65 insertions(+)board/cm_t35/cm_t35.c +31
-0@@ -33,7 +33,9 @@#include <net.h> #include <i2c.h> #include <usb.h> +#include <nand.h> #include <twl4030.h> +#include <bmp_layout.h> #include <linux/compiler.h> #include <asm/io.h> @@ -75,6 +77,65 @@ static u32 gpmc_nand_config[GPMC_MAX_REG] = { 0, }; +#ifdef CONFIG_LCD +#ifdef CONFIG_CMD_NAND +static int splash_load_from_nand(u32 bmp_load_addr) +{ + struct bmp_header *bmp_hdr; + int res, splash_screen_nand_offset = 0x100000; + size_t bmp_size, bmp_header_size = sizeof(struct bmp_header); + + if (bmp_load_addr + bmp_header_size >= gd->start_addr_sp) + goto splash_address_too_high; + + res = nand_read_skip_bad(&nand_info[nand_curr_device], + splash_screen_nand_offset, &bmp_header_size, + (u_char *)bmp_load_addr); + if (res < 0) + return res; + + bmp_hdr = (struct bmp_header *)bmp_load_addr; + bmp_size = le32_to_cpu(bmp_hdr->file_size); + + if (bmp_load_addr + bmp_size >= gd->start_addr_sp) + goto splash_address_too_high; + + return nand_read_skip_bad(&nand_info[nand_curr_device], + splash_screen_nand_offset, &bmp_size, + (u_char *)bmp_load_addr); + +splash_address_too_high: + printf("Error: splashimage address too high. Data overwritesU-Boot "
board/cm_t35/cm_t35.c +20
-0+ "and/or placed beyond DRAM boundaries.\n"); + + return -1; +} +#else +static inline int splash_load_from_nand (void ) +{ +return -1 ; +} +#endif /* CONFIG_CMD_NAND */ + +int board_splash_screen_prepare (void ) +{ +char *env_splashimage_value; +u32 bmp_load_addr; + + env_splashimage_value =getenv ("splashimage" ); +if (env_splashimage_value ==NULL ) +return -1 ; +board/cm_t35/cm_t35.c static u32 gpmc_nand_config[GPMC_MAX_REG] = { · line 138
Quoted 10 lines
board/cm_t35/cm_t35.c +10
-0+ bmp_load_addr =simple_strtoul (env_splashimage_value,0 ,16 ); +if (bmp_load_addr ==0 ) { +printf ("Error: bad splashimage address specified\n" ); +return -1 ; + } + + return splash_load_from_nand(bmp_load_addr); +} +#endif /* CONFIG_LCD */ +fyi, I noticed that my board compiled with gcc 4.7.3 from ELDK 5.3 will trap
Quoted 8 lines
if the bitmap is not aligned. Aligned is a bit tricky though since the bitmap
has the signature, e.g. "BM" prepended and is thereafter 32 bit aligned (or at least combined fields of 32 bits). In my case displaying the bitmap now only works when loaded to an aligned address - 2. Since you accept the value from the user, which has no ability to restore it once set "incorrectly", you might want to check the load address (well obviously only if it is a problem in your case as well).
Thanks for verifying this issue. I did encounter it during testing, but I assumed it to be a compiler problem because it worked when compiling with a different version.
Loading to aligned address - 2 works for me as well when compiling with the problematic compiler, but this is strange to me. Isn't the "packed" attribute that is applied to bmp_header_t supposed to prevent these types of problems by effectively forcing the compiler to assume byte alignment?
Albert, can you shed some light on this?
Quoted 4 lines
Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Hello Nikita,
Quoted 18 lines · Nikita Kiryanov
Quoted 13 lines
fyi, I noticed that my board compiled with gcc 4.7.3 from ELDK 5.3 will trap if the bitmap is not aligned. Aligned is a bit tricky though since the
bitmap has the signature, e.g. "BM" prepended and is thereafter 32 bit aligned (or at least combined fields of 32 bits). In my case displaying the
bitmap now only works when loaded to an aligned address - 2. Since you accept the value from the user, which has no ability to restore it once
set "incorrectly", you might want to check the load address (well obviously
only if it is a problem in your case as well).Thanks for verifying this issue. I did encounter it during testing, but I assumed it to be a compiler problem because it worked when compiling with a different version.
Quoted 5 lines
Loading to aligned address - 2 works for me as well when compiling with the problematic compiler, but this is strange to me. Isn't the "packed" attribute that is applied to
bmp_header_tsupposed to prevent these types of problems by effectively forcing the compiler to assume byte alignment?
Not per definition, while the pack does place most members at unaligned addresses, it does not control if the data can be loaded from it. Since the 4.7+ compilers by default assume that the chip does support unaligned accesses, it just generates ldr instruction to get the uint32_t members (and thus trap on this mcu). Older versions (or 4.7 with -mno-unaligned-access)
will generate byte fetches due to the pack since it assumes the chip does not support unaligned accesses.
So when compiled with a older compiler the bitmap could be loaded anywhere. With 4.7+ it is faster but needs to be aligned (in a bit weird manner).
Regards,
Jeroen
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Quoted 35 lines · Jeroen Hofstee
Hello Nikita,
Quoted 18 lines · Nikita Kiryanov
Quoted 13 lines
fyi, I noticed that my board compiled with gcc 4.7.3 from ELDK 5.3 will trap if the bitmap is not aligned. Aligned is a bit tricky though since the
bitmap has the signature, e.g. "BM" prepended and is thereafter 32 bit aligned (or at least combined fields of 32 bits). In my case displaying the
bitmap now only works when loaded to an aligned address - 2. Since you accept the value from the user, which has no ability to restore it once
set "incorrectly", you might want to check the load address (well obviously
only if it is a problem in your case as well).Thanks for verifying this issue. I did encounter it during testing, but I assumed it to be a compiler problem because it worked when compiling with a different version.
Quoted 5 lines
Loading to aligned address - 2 works for me as well when compiling with the problematic compiler, but this is strange to me. Isn't the "packed" attribute that is applied to
bmp_header_tsupposed to prevent these types of problems by effectively forcing the compiler to assume byte alignment?Not per definition, while the pack does place most members at unaligned addresses, it does not control if the data can be loaded from it. Since the 4.7+ compilers by default assume that the chip does support unaligned accesses, it just generates ldr instruction to get the
uint32_tmembers (and thus trap on this mcu). Older versions (or 4.7 with -mno-unaligned-access)
will generate byte fetches due to the pack since it assumes the chip does not support unaligned accesses.
Quoted 2 lines
So when compiled with a older compiler the bitmap could be loaded anywhere. With 4.7+ it is faster but needs to be aligned (in a bit weird manner).
Hmm... Then this means that lcd.c is similarly broken; it makes the same accesses and the same assumptions about the load address.
Personally, I don't like the idea that board users should be aware of the architecture's capabilities or the internal structure of BMP files in order to select a correct load address, so requiring them to load it to aligned address - 2 really irks me.
README.arm-unaligned-accesses does list standard compliance as a possible reason for allowing emulated unaligned accesses, and the format of the BMP header is certainly a standard of the BMP file format, so I wonder if this constitutes a good reason to allow emulated unaligned accesses for lcd.c?
Barring that, we should at least protect lcd.c from this issue by making some sort of check for affected targets, or maybe limit the possible values for splashimage... This issue makes it way too easy to accidentally break the boot process in a way that's hard to recover from.
Quoted 6 lines
Regards,
Jeroen
Signature · 2 lines
Regards, Nikita.
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Hi Nikita,
Quoted 47 lines · Nikita Kiryanov
Quoted 35 lines · Jeroen Hofstee
Hello Nikita,
Quoted 18 lines · Nikita Kiryanov
Quoted 13 lines
fyi, I noticed that my board compiled with gcc 4.7.3 from ELDK 5.3 will trap if the bitmap is not aligned. Aligned is a bit tricky though since the
bitmap has the signature, e.g. "BM" prepended and is thereafter 32 bit aligned (or at least combined fields of 32 bits). In my case displaying the
bitmap now only works when loaded to an aligned address - 2. Since you accept the value from the user, which has no ability to restore it once
set "incorrectly", you might want to check the load address (well obviously
only if it is a problem in your case as well).Thanks for verifying this issue. I did encounter it during testing, but I assumed it to be a compiler problem because it worked when compiling with a different version.
Quoted 5 lines
Loading to aligned address - 2 works for me as well when compiling with the problematic compiler, but this is strange to me. Isn't the "packed" attribute that is applied to
bmp_header_tsupposed to prevent these types of problems by effectively forcing the compiler to assume byte alignment?Not per definition, while the pack does place most members at unaligned addresses, it does not control if the data can be loaded from it. Since the 4.7+ compilers by default assume that the chip does support unaligned accesses, it just generates ldr instruction to get the
uint32_tmembers (and thus trap on this mcu). Older versions (or 4.7 with -mno-unaligned-access)
will generate byte fetches due to the pack since it assumes the chip does not support unaligned accesses.Quoted 2 lines
So when compiled with a older compiler the bitmap could be loaded anywhere. With 4.7+ it is faster but needs to be aligned (in a bit weird manner).
Hmm... Then this means that
lcd.cis similarly broken; it makes the same accesses and the same assumptions about the load address.Personally, I don't like the idea that board users should be aware of the architecture's capabilities or the internal structure of BMP files in order to select a correct load address, so requiring them to load it to aligned address - 2 really irks me.
Quoted 5 lines
README.arm-unaligned-accesses does list standard compliance as a possible reason for allowing emulated unaligned accesses, and the format of the BMP header is certainly a standard of the BMP file format, so I wonder if this constitutes a good reason to allow emulated unaligned accesses for
lcd.c?
IMO no, it does not, for two reasons:
- generally speaking, standard formats and known formats should not be confused, and here, we are dealing with a known, but not standard, format;
- concerning the exception in README.arm-unaligned-accesses exception, it is about cases where the structure is going to be used by hardware (either local or external to the system) which requires conformance, for instance, network frames. Here, the BMB header is not used by any hardware.
Quoted 5 lines
Barring that, we should at least protect
lcd.cfrom this issue by making some sort of check for affected targets, or maybe limit the possible values for splashimage... This issue makes it way too easy to accidentally break the boot process in a way that's hard to recover from.
I suggest a few solutions:
1) enforce given load address alignment so that BMP header fields are natively aligned, with a clear error message. Simple to implement, difficut for users to understand and accept.
2) once the address provided by the user is known, if it is not properly aligned, then the next properly aligned address should be used, and the byte at given address should contain the offset from the given address to the used address. This is a general solution that works for any given load address, odd ones included:
Given address: First bytes: Used address: 10000000 2 x 'B' 'M' 10000002 10000001 1 'B' 'M' 10000002 10000002 'B' 'M' 10000002 10000003 3 x x 'B' 'M' 10000006 10000004 2 x 'B' 'M' 10000006
...
Note that if the user address is constrained to be 4-byte-aligned, then only the "2 x 'B' 'M'" case would apply.
3) define an internal 'BMP holder' structure which contains a two-byte prefix before the 'BMP file' header and data. This way, if the overall structure is aligned, then the fields in the BMP header are aligned too.
4) Build a time machine and tell the designers of the BMP header format, in great inventive and colorful detail, what horrible things will happen to them if they don't order and size their fields so that they naturally land on aligned offsets from the header start. This solution gives the best results IMO.
5) if none the above (including 4) is feasible for some reason, then use unaligned accessors for this BMP fields, with a Big Fat Comment about why this is so.
Note that all solutions except 2 (and 4) depend on the given address being constrained in some way -- such a constraint does not seem excessive to me.
Amicalement,
Signature · 1 line
Albert.
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Hi Albert,
Quoted 2 lines · Albert ARIBAUD
Hi Nikita,
[...]
Quoted 11 lines
Quoted 5 lines
Barring that, we should at least protect
lcd.cfrom this issue by making some sort of check for affected targets, or maybe limit the possible values for splashimage... This issue makes it way too easy to accidentally break the boot process in a way that's hard to recover from.I suggest a few solutions:
1) enforce given load address alignment so that BMP header fields are natively aligned, with a clear error message. Simple to implement, difficut for users to understand and accept.
Yes I agree that from a user point of view this looks terrible, which is why I prefer not to do something like this.
Quoted 13 lines
2) once the address provided by the user is known, if it is not properly aligned, then the next properly aligned address should be used, and the byte at given address should contain the offset from the given address to the used address. This is a general solution that works for any given load address, odd ones included:
Given address: First bytes: Used address: 10000000 2 x 'B' 'M' 10000002 10000001 1 'B' 'M' 10000002 10000002 'B' 'M' 10000002 10000003 3 x x 'B' 'M' 10000006 10000004 2 x 'B' 'M' 10000006
Quoted 4 lines
...
Note that if the user address is constrained to be 4-byte-aligned, then only the "2 x 'B' 'M'" case would apply.
I think a simpler way to implement something like this is to just use modulo 4 to check alignment and fix the address dynamically; perhaps even fixing it in the environment.
This is a localized approach though. We will have to do this from all the code paths that lead to a bmp header being probed in memory. I would prefer a more localized solution.
Quoted 11 lines
3) define an internal 'BMP holder' structure which contains a two-byte prefix before the 'BMP file' header and data. This way, if the overall structure is aligned, then the fields in the BMP header are aligned too.
4) Build a time machine and tell the designers of the BMP header format, in great inventive and colorful detail, what horrible things will happen to them if they don't order and size their fields so that they naturally land on aligned offsets from the header start. This solution gives the best results IMO.
The problem with 3 (and 4) is that it still doesn't protect the user from bricking their board by choosing a non-aligned address for their BMP. This might happen because the user:
- is unaware of the dangers of choosing a non-aligned address
- made a typo
- relies on a script or program that runs under U-Boot to setup stuff
- I"m sure there are other possibilities
In terms of usability it's a *big* regression. If we do not actually prevent the user from setting splashimage to an unaligned address we should make sure that all usable addresses are safe.
Quoted 4 lines
5) if none the above (including 4) is feasible for some reason, then use unaligned accessors for this BMP fields, with a Big Fat Comment about why this is so.
I think, once this feature is merged, I'll try to see if something nice can be done with an approach like this.
For now I'll add a suggestion-#2-style check in lcd.c
Quoted 7 lines
Note that all solutions except 2 (and 4) depend on the given address being constrained in some way -- such a constraint does not seem excessive to me.
Amicalement,
Signature · 2 lines
Regards, Nikita.
PATCH 5/5 cm-t35: add support for loading splash image from NAND
Quoted 11 lines · Nikita Kiryanov
Hi Albert,
Quoted 2 lines · Albert ARIBAUD
Hi Nikita,
[...]
Quoted 2 lines
Note that if the user address is constrained to be 4-byte-aligned, then only the "2 x 'B' 'M'" case would apply.
Quoted 7 lines
I think a simpler way to implement something like this is to just use modulo 4 to check alignment and fix the address dynamically; perhaps even fixing it in the environment.
This is a localized approach though. We will have to do this from all the code paths that lead to a bmp header being probed in memory. I would prefer a more localized solution.
Correction:
"I would prefer a more global solution."
Quoted 12 lines
Quoted 10 lines
3) define an internal 'BMP holder' structure which contains a two-byte prefix before the 'BMP file' header and data. This way, if the overall structure is aligned, then the fields in the BMP header are aligned too.
Amicalement,
Signature · 2 lines
Regards, Nikita.
PATCH 5/5 cm-t35: add support for loading splash image from NAND
In reply to Nikita KiryanovQuoted 4 lines · Nikita Kiryanov
Add support for loading splash image from NAND
Signed-off-by Nikita Kiryanov <[email protected]>Signed-off-by Igor Grinberg <[email protected]>
Applied to u-boot-ti/master (and already pulled into u-boot-arm), thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130326/ec6e6f85/attachment.pgp>
cover Add splash screen for CM-T35
In reply to Nikita KiryanovHi all,
this series is awaiting review for almost a month. Can someone take a look at it?
Quoted 29 lines · Nikita Kiryanov
This patchset adds splash screen support for CM-T35.
It includes the ability to initialize the display subsystem either using predefines (selected via env variable "displaytype"), or user supplied configuration options, also stored in an environment variables and pointed to by displaytype. The splash image data is currently read from NAND.As a preparation for the above functionality, this patch set introduces new DSS #defines and an option for board-specific splash screen preparation, which can be invoked in
lcd_logo()right before displaying the splash screen (typical use
case: load the image data in time for it to be displayed).Nikita Kiryanov (5):
omap3: add useful dss defines
lcd: add option for board specific splash screen preparation
cm-t35: add support for dvi displays
cm-t35: add support for user defined lcd parameters
cm-t35: add support for loading splash image from NANDREADME | 8 + arch/arm/include/asm/arch-omap3/dss.h | 35 +++ board/cm_t35/Makefile | 1 + board/cm_t35/cm_t35.c | 64 +++++ board/cm_t35/display.c | 420 +++++++++++++++++++++++++++++++++ common/lcd.c | 15 ++ include/configs/cm_t35.h | 10 + include/lcd.h | 1 + 8 files changed, 554 insertions(+)create mode 100644
board/cm_t35/display.c
Signature · 2 lines
Regards, Nikita.
cover Add splash screen for CM-T35
Hello Nikita,
Quoted 42 lines · Nikita Kiryanov
Hi all,
this series is awaiting review for almost a month. Can someone take a look at it?
Quoted 34 lines · Nikita Kiryanov
This patchset adds splash screen support for CM-T35.
It includes the ability to initialize the display subsystem either using predefines (selected via env variable "displaytype"), or user supplied configuration options, also stored in an environment variables and pointed to by
displaytype. The splash image data is currently read from NAND.As a preparation for the above functionality, this patch set introduces new DSS
#defines and an option for board-specific splash screen preparation, which can
be invoked inlcd_logo()right before displaying the splash screen (typical use
case: load the image data in time for it to be displayed).Nikita Kiryanov (5):
omap3: add useful dss defines
lcd: add option for board specific splash screen preparation
cm-t35: add support for dvi displays
cm-t35: add support for user defined lcd parameters
cm-t35: add support for loading splash image from NANDREADME | 8 + arch/arm/include/asm/arch-omap3/dss.h | 35 +++ board/cm_t35/Makefile | 1 + board/cm_t35/cm_t35.c | 64 +++++ board/cm_t35/display.c | 420 +++++++++++++++++++++++++++++++++ common/lcd.c | 15 ++ include/configs/cm_t35.h | 10 + include/lcd.h | 1 + 8 files changed, 554 insertions(+)create mode 100644
board/cm_t35/display.c
I am not a u-boot developer, but will at some comments.
Regards,
Jeroen
cover Add splash screen for CM-T35
In reply to Nikita Kiryanov-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 4 lines · Nikita Kiryanov
Hi all,
this series is awaiting review for almost a month. Can someone take a look at it?
In general, I'm waiting for Anatolij to review this code area, thanks!
- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/
iQIcBAEBAgAGBQJQ/UxrAAoJENk4IS6UOR1WzMEP+weJxwguOOlQvRGFKcfUHgdh 0v+lnMCes+MbbKTjWZ+iYA5ksBvdLGSieeL3J/PlViJpwXTgiHA/7wPD6ZKXvkNE ncO/WwU8q8eaJBpjmauC/h7TtILfMYonItdKTjfia6vg1z5nRm3bcieOpdeY7WV3 9iUgcBq4eRHHShm/B5xFIe77VrVb++Xk/gPpOx/TEFeP2q7YNhTcMvc/fdr27TNV n2pEcaYgASFKms0r61dIaffq6xYyjBvtKvIlVvpY8nuBLtnPY8lK310JOGybFFZR ST2GKh4V24MlBYJDhqXXoswmjIIuwj0qh6lHN7tC6U1gHj3MI/EmzrUismT+9qRH f04LDZR2FZooEQE/FMJuH/MId/PntzMxYDAfTQk0vByQ0yunw+ZPttjmif2mqf2x D72fOHjumko0uyixC/PTBx4Nmt2yyo/9oAPymy66SevJ6Sbd9GynpDklewHRWJn6 y+Sjf7t5LbVBex0Tl1Xzj1f56c0DZXwbfY0ckZSc0ndZhcwudusx0lCvengCR9sc rgvTUaKa/pIeiqX8SEPI+WEr8PIAKIsSmr0P/6z9TcSiky2qHOfU8F1eQ4M9z3S9 5tieSBzPepC7V7haQLHNyw2Q19D43VLZkyNIs2CJOhsd4zWincc76CbpsFH59xty wcoY37K/rJf2snQYSKAq
=6npF
-----END PGP SIGNATURE-----