[U-Boot] [PATCH 0/5] Add splash screen for CM-T35

[U-Boot] [PATCH 0/5] Add splash screen for CM-T35

[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 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

-- 
1.7.10.4

[U-Boot] [PATCH 1/5] omap3: add useful dss defines

[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 <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 arch/arm/include/asm/arch-omap3/dss.h |   35 +++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/include/asm/arch-omap3/dss.h b/arch/arm/include/asm/arch-omap3/dss.h
index ffaffbb..cb6d746 100644
--- a/arch/arm/include/asm/arch-omap3/dss.h
+++ b/arch/arm/include/asm/arch-omap3/dss.h
@@ -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;
-- 
1.7.10.4

[U-Boot] [PATCH 1/5] omap3: add useful dss defines

[Jeroen Hofstee]

Hello Nikita,

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> Add useful omap3 dss defines for: polarity, TFT data lines, lcd
> display type, gfx burst size, and gfx format
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   arch/arm/include/asm/arch-omap3/dss.h |   35 +++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-omap3/dss.h b/arch/arm/include/asm/arch-omap3/dss.h
> index ffaffbb..cb6d746 100644
> --- a/arch/arm/include/asm/arch-omap3/dss.h
> +++ b/arch/arm/include/asm/arch-omap3/dss.h
> @@ -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;
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

[U-Boot] [PATCH 1/5] omap3: add useful dss defines

[Nikita Kiryanov]

Hi Jeroen,

On 01/20/2013 11:42 PM, Jeroen Hofstee wrote:
> Hello Nikita,
>
[...]
>> +#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;
> 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].

> Regards,
> Jeroen

-- 
Regards,
Nikita.

[U-Boot] [PATCH 1/5] omap3: add useful dss defines

[Jeroen Hofstee]

Hello Nikita,

+#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;
>> 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].
>

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

[U-Boot] [PATCH 1/5] omap3: add useful dss defines

[Nikita Kiryanov]

On 01/21/2013 08:38 PM, Jeroen Hofstee wrote:
> Hello Nikita,
>
> +#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;
>>> 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].
>>
>
> 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.

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).

>
> Regards,
> Jeroen
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[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
(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.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 README        |    8 ++++++++
 common/lcd.c  |   15 +++++++++++++++
 include/lcd.h |    1 +
 3 files changed, 24 insertions(+)

diff --git a/README b/README
index 78f40df..cffc016 100644
--- a/README
+++ b/README
@@ -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
diff --git a/common/lcd.c b/common/lcd.c
index 66d4f94..129cf7e 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -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");
diff --git a/include/lcd.h b/include/lcd.h
index c24164a..4ac4ddd 100644
--- a/include/lcd.h
+++ b/include/lcd.h
@@ -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,
-- 
1.7.10.4

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Jeroen Hofstee]

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> 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
> (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.
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   README        |    8 ++++++++
>   common/lcd.c  |   15 +++++++++++++++
>   include/lcd.h |    1 +
>   3 files changed, 24 insertions(+)
>
> diff --git a/README b/README
> index 78f40df..cffc016 100644
> --- a/README
> +++ b/README
> @@ -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
> diff --git a/common/lcd.c b/common/lcd.c
> index 66d4f94..129cf7e 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -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");
> diff --git a/include/lcd.h b/include/lcd.h
> index c24164a..4ac4ddd 100644
> --- a/include/lcd.h
> +++ b/include/lcd.h
> @@ -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

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Nikita Kiryanov]

Hi Jeroen,

On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
[...]
>> diff --git a/include/lcd.h b/include/lcd.h
>> index c24164a..4ac4ddd 100644
>> --- a/include/lcd.h
>> +++ b/include/lcd.h
>> @@ -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.

>
> Regards,
> Jeroen


-- 
Regards,
Nikita.

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Jeroen Hofstee]

Hello Nikita,

On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:
> Hi Jeroen,
>
> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
> [...]
>>> diff --git a/include/lcd.h b/include/lcd.h
>>> index c24164a..4ac4ddd 100644
>>> --- a/include/lcd.h
>>> +++ b/include/lcd.h
>>> @@ -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.
>
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

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Nikita Kiryanov]

On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
> Hello Nikita,
>
> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:
>> Hi Jeroen,
>>
>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
>> [...]
>>>> diff --git a/include/lcd.h b/include/lcd.h
>>>> index c24164a..4ac4ddd 100644
>>>> --- a/include/lcd.h
>>>> +++ b/include/lcd.h
>>>> @@ -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.
>>
> 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_.

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.

>
> But anyway, can't this at least be changed to a __weak function, 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.

>
> Regards,
> Jeroen
>
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Jeroen Hofstee]

Hello Nikita,

On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:
> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
>> Hello Nikita,
>>
>> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:
>>> Hi Jeroen,
>>>
>>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
>>> [...]
>>>>> diff --git a/include/lcd.h b/include/lcd.h
>>>>> index c24164a..4ac4ddd 100644
>>>>> --- a/include/lcd.h
>>>>> +++ b/include/lcd.h
>>>>> @@ -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.
>>>
>> 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_.
>
> 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).

>>
>> But anyway, can't this at least be changed to a __weak function, 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

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Igor Grinberg]

On 01/24/13 00:13, Jeroen Hofstee wrote:
> Hello Nikita,
> 
> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:
>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
>>> Hello Nikita,
>>>
>>> On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:
>>>> Hi Jeroen,
>>>>
>>>> On 01/20/2013 10:34 PM, Jeroen Hofstee wrote:
>>>> [...]
>>>>>> diff --git a/include/lcd.h b/include/lcd.h
>>>>>> index c24164a..4ac4ddd 100644
>>>>>> --- a/include/lcd.h
>>>>>> +++ b/include/lcd.h
>>>>>> @@ -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.
>>>>
>>> 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_.
>>
>> 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).

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...

> 
>>>
>>> But anyway, can't this at least be changed to a __weak function, 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...

> 
> 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.


-- 
Regards,
Igor.

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Jeroen Hofstee]

Hello Igor,

On 01/24/2013 09:35 AM, Igor Grinberg wrote:
> On 01/24/13 00:13, Jeroen Hofstee wrote:
>> Hello Nikita,
>>
>> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:
>>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
>>>>
>>>> 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_.
>>> 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).
> So what's your point? Do you think we should add a splash screen specific
> callback inside the board.c U-Boot boot flow?
no.
> 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.

>
>>>> But anyway, can't this at least be changed to a __weak function, 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...
>
>> 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....
> 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

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Igor Grinberg]

On 01/25/13 00:34, Jeroen Hofstee wrote:
> Hello Igor,
> 
> On 01/24/2013 09:35 AM, Igor Grinberg wrote:
>> On 01/24/13 00:13, Jeroen Hofstee wrote:
>>> Hello Nikita,
>>>
>>> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:
>>>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
>>>>>
>>>>> 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_.
>>>> 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).
>> So what's your point? Do you think we should add a splash screen specific
>> callback inside the board.c U-Boot boot flow?
> no.
>> 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.

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.

> 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.

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.

> 
>>
>>>>> But anyway, can't this at least be changed to a __weak function, 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...
>>
>>> 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.

> 
> 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....

Ok. So we agree that this can't be done in lcd_enable().

>> 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.

Usually, I also like the __weak approach, but this time the intention was
to document the feature and hopefully stop the lcd_*() API abuse.


-- 
Regards,
Igor.

[U-Boot] [PATCH 2/5] lcd: add option for board specific splash screen preparation

[Jeroen Hofstee]

Hello Igor,

On 01/25/2013 07:45 AM, Igor Grinberg wrote:
> On 01/25/13 00:34, Jeroen Hofstee wrote:
>> Hello Igor,
>>
>> On 01/24/2013 09:35 AM, Igor Grinberg wrote:
>>> On 01/24/13 00:13, Jeroen Hofstee wrote:
>>>> Hello Nikita,
>>>>
>>>> On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:
>>>>> On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:
>>>>>> 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_.
>>>>> 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).
>>> So what's your point? Do you think we should add a splash screen specific
>>> callback inside the board.c U-Boot boot flow?
>> no.
>>> 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.
> 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.
>
>> 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.
> 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.
>

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.

>> 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....
> Ok. So we agree that this can't be done in lcd_enable().
>
yes.
>>> 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.
> Usually, I also like the __weak approach, but this time the intention was
> to document the feature and hopefully stop the lcd_*() API abuse.
>
>
Regards,
Jeroen

[U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays

[Nikita Kiryanov]

Add support for dvi displays with user selectable dvi presets.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 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

diff --git a/board/cm_t35/Makefile b/board/cm_t35/Makefile
index 894fa09..bde56e6 100644
--- a/board/cm_t35/Makefile
+++ b/board/cm_t35/Makefile
@@ -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)
 
diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
index edbb941..8f3d735 100644
--- a/board/cm_t35/cm_t35.c
+++ b/board/cm_t35/cm_t35.c
@@ -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*/
diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c
new file mode 100644
index 0000000..11b8ed9
--- /dev/null
+++ b/board/cm_t35/display.c
@@ -0,0 +1,213 @@
+/*
+ * (C) Copyright 2012 CompuLab, Ltd. <www.compulab.co.il>
+ *
+ * Authors: Nikita Kiryanov <nikita@compulab.co.il>
+ *
+ * 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,
+};
+
+/*
+ * 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 */
+	.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))
+		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;
+}
+
+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;
+
+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
+	 * to 16 by default, we need to set it to a smaller value, like 3
+	 * (chosen via trial and error).
+	 */
+	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);
+}
+
+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) {}
diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
index fa6eb4e..8544b15 100644
--- a/include/configs/cm_t35.h
+++ b/include/configs/cm_t35.h
@@ -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 */
-- 
1.7.10.4

[U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays

[Jeroen Hofstee]

Hello Nikita,

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> Add support for dvi displays with user selectable dvi presets.
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   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
>
> diff --git a/board/cm_t35/Makefile b/board/cm_t35/Makefile
> index 894fa09..bde56e6 100644
> --- a/board/cm_t35/Makefile
> +++ b/board/cm_t35/Makefile
> @@ -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)
>   
> diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
> index edbb941..8f3d735 100644
> --- a/board/cm_t35/cm_t35.c
> +++ b/board/cm_t35/cm_t35.c
> @@ -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*/
> diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c
> new file mode 100644
> index 0000000..11b8ed9
> --- /dev/null
> +++ b/board/cm_t35/display.c
> @@ -0,0 +1,213 @@
> +/*
> + * (C) Copyright 2012 CompuLab, Ltd. <www.compulab.co.il>
> + *
> + * Authors: Nikita Kiryanov <nikita@compulab.co.il>
> + *
> + * 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,
> +};
> +
> +/*
> + * 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
> +	.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))
> +		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..
> +
> +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.
> +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
> +	 * 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.
> +	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.
> +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) {}
> diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
> index fa6eb4e..8544b15 100644
> --- a/include/configs/cm_t35.h
> +++ b/include/configs/cm_t35.h
> @@ -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

[U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays

[Nikita Kiryanov]

Hi Jeroen,

On 01/20/2013 10:59 PM, Jeroen Hofstee wrote:
> Hello Nikita,
>
> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
>> Add support for dvi displays with user selectable dvi presets.
>>
>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
>> ---

[...]

>> +
>> +/*
>> + * 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

Good point.

>> +    .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,
>> +};
>> +

[...]

>> +    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))
>> +        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..

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.

>> +
>> +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.

Yes that should be the extent of the "damage".

>> +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
>> +     * 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)

Thanks.
I'll consider it for future extensions to this code, but for the time
being the guess serves its purpose.

>
> 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.


>> +    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?

Yes, we do want 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.

Addressed above.

>> +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) {}
>> diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
>> index fa6eb4e..8544b15 100644
>> --- a/include/configs/cm_t35.h
>> +++ b/include/configs/cm_t35.h
>> @@ -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
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays

[Jeroen Hofstee]

On 01/21/2013 09:12 AM, Nikita Kiryanov wrote:
>
>>> +    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))
>>> +        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..
>
> 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.

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.

>
>>> +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
>>> +     * 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)
>
> 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.."
>>
>> 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.
>
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).
>
>>> + 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?
>
> Yes, we do want 16 bit support.
Why?
>
>> 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.
>
> Addressed above.
well the same why... you drive them as 24 bit panels, why do you want a 
lower
resolution in software?

Regards,
Jeroen

[U-Boot] [PATCH 3/5] cm-t35: add support for dvi displays

[Igor Grinberg]

On 01/23/13 23:39, Jeroen Hofstee wrote:
> On 01/21/2013 09:12 AM, Nikita Kiryanov wrote:
>>
>>>> +    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))
>>>> +        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..
>>
>> 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.
> 
> 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.

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.

> 
>>
>>>> +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
>>>> +     * 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)
>>
>> 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!

>>>
>>> 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.
>>
> 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.

> 
> 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.

>>
>>>> + 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?
>>
>> Yes, we do want 16 bit support.
> Why?
>>
>>> 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.
>>
>> 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.

-- 
Regards,
Igor.

[U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters

[Nikita Kiryanov]

Add support for user defined lcd parameters for cm-t35 splash screen.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 board/cm_t35/display.c |  213 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 210 insertions(+), 3 deletions(-)

diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c
index 11b8ed9..7b09bce 100644
--- a/board/cm_t35/display.c
+++ b/board/cm_t35/display.c
@@ -3,6 +3,8 @@
  *
  * Authors: Nikita Kiryanov <nikita@compulab.co.il>
  *
+ * 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();
 	}
-- 
1.7.10.4

[U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters

[Jeroen Hofstee]

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> Add support for user defined lcd parameters for cm-t35 splash screen.
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   board/cm_t35/display.c |  213 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 210 insertions(+), 3 deletions(-)
>
> diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c
> index 11b8ed9..7b09bce 100644
> --- a/board/cm_t35/display.c
> +++ b/board/cm_t35/display.c
> @@ -3,6 +3,8 @@
>    *
>    * Authors: Nikita Kiryanov <nikita@compulab.co.il>
>    *
> + * 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);
I fail to understand why you want to copy this.
> +	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?
> +	return 0;
> +}
> +
Is above really board specific or should it be in omap_videomodes.c or 
whatever?
> +/*
>    * 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();
>   	}
Regards,
Jeroen

[U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters

[Nikita Kiryanov]

Hi Jeroen,

On 01/20/2013 11:08 PM, Jeroen Hofstee wrote:
> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
[...]
>> + * 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.

strtok modifies the string it operates on. The documentation for
getenv states that you must not modify the string it returns.

>> +    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?

Can you please elaborate on this comment? I'm not entirely sure what
inconsistencies you are referring to.

>> +    return 0;
>> +}
>> +
> Is above really board specific or should it be in omap_videomodes.c or
> whatever?

Well, most of it is parsing for a custom feature, so I would say this is
board specific.

>> +/*
>>    * 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();
>>       }
> Regards,
> Jeroen
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters

[Jeroen Hofstee]

Hi Nikita,

On 01/21/2013 09:25 AM, Nikita Kiryanov wrote:
> Hi Jeroen,
>
> On 01/20/2013 11:08 PM, Jeroen Hofstee wrote:
>> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> [...]
>>> + * 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.
>
> 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...
>>> +    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?
>
> 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?
>
>>> +    return 0;
>>> +}
>>> +
>> Is above really board specific or should it be in omap_videomodes.c or
>> 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

[U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters

[Igor Grinberg]

On 01/24/13 00:36, Jeroen Hofstee wrote:
> Hi Nikita,
> 
> On 01/21/2013 09:25 AM, Nikita Kiryanov wrote:
>> Hi Jeroen,
>>
>> On 01/20/2013 11:08 PM, Jeroen Hofstee wrote:
>>> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
>> [...]
>>>> + * 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.
>>
>> 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...
>>>> +    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?
>>
>> 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?
>>
>>>> +    return 0;
>>>> +}
>>>> +
>>> Is above really board specific or should it be in omap_videomodes.c or
>>> 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....).

I fail to understand which timer are you talking about...
Probably, you mean the DPLLs and the clocks?

-- 
Regards,
Igor.

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Nikita Kiryanov]

Add support for loading splash image from NAND

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 board/cm_t35/cm_t35.c    |   61 ++++++++++++++++++++++++++++++++++++++++++++++
 include/configs/cm_t35.h |    4 +++
 2 files changed, 65 insertions(+)

diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
index 8f3d735..8dbdb44 100644
--- a/board/cm_t35/cm_t35.c
+++ b/board/cm_t35/cm_t35.c
@@ -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.
diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
index 8544b15..5e0d261 100644
--- a/include/configs/cm_t35.h
+++ b/include/configs/cm_t35.h
@@ -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 */
-- 
1.7.10.4

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Jeroen Hofstee]

Hi Nikita,

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> Add support for loading splash image from NAND
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   board/cm_t35/cm_t35.c    |   61 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/configs/cm_t35.h |    4 +++
>   2 files changed, 65 insertions(+)
>
> diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
> index 8f3d735..8dbdb44 100644
> --- a/board/cm_t35/cm_t35.c
> +++ b/board/cm_t35/cm_t35.c
> @@ -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 */
> +
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

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Nikita Kiryanov]

Hi Jeroen,

On 12/24/2012 10:55 AM, Jeroen Hofstee wrote:
> Hi Nikita,
>
> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
>> Add support for loading splash image from NAND
>>
>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
>> ---
>>   board/cm_t35/cm_t35.c    |   61
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/configs/cm_t35.h |    4 +++
>>   2 files changed, 65 insertions(+)
>>
>> diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
>> index 8f3d735..8dbdb44 100644
>> --- a/board/cm_t35/cm_t35.c
>> +++ b/board/cm_t35/cm_t35.c
>> @@ -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 */
>> +
> 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.

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?

>
> Regards,
> Jeroen
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Jeroen Hofstee]

Hello Nikita,

On 12/25/2012 09:56 AM, Nikita Kiryanov wrote:
>
>> 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.
>
> 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?
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

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Nikita Kiryanov]

On 12/26/2012 04:27 PM, Jeroen Hofstee wrote:
> Hello Nikita,
>
> On 12/25/2012 09:56 AM, Nikita Kiryanov wrote:
>>
>>> 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.
>>
>> 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?
> 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).

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.

>
> Regards,
> Jeroen
>
>
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Albert ARIBAUD]

Hi Nikita,

On Sun, 30 Dec 2012 16:39:06 +0200, Nikita Kiryanov
<nikita@compulab.co.il> wrote:

> On 12/26/2012 04:27 PM, Jeroen Hofstee wrote:
> > Hello Nikita,
> >
> > On 12/25/2012 09:56 AM, Nikita Kiryanov wrote:
> >>
> >>> 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.
> >>
> >> 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?
> > 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).
> 
> 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?

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.

> 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.

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,
-- 
Albert.

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Nikita Kiryanov]

Hi Albert,

On 01/22/2013 09:37 AM, Albert ARIBAUD wrote:
> Hi Nikita,
>

[...]

>> 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.
>
> 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.

>
> 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.

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.

>
> 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.

>
> 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

>
> 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,
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Nikita Kiryanov]

On 01/23/2013 12:47 PM, Nikita Kiryanov wrote:
> Hi Albert,
>
> On 01/22/2013 09:37 AM, Albert ARIBAUD wrote:
>> Hi Nikita,
>>
>
> [...]
>
>> 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.

Correction:
"I would prefer a more global solution."

>
>>
>> 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,
>>
>
>


-- 
Regards,
Nikita.

[U-Boot] [U-Boot, 5/5] cm-t35: add support for loading splash image from NAND

[Tom Rini]

On Sat, Dec 22, 2012 at 09:03:48PM -0000, Nikita Kiryanov wrote:

> Add support for loading splash image from NAND
> 
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>

Applied to u-boot-ti/master (and already pulled into u-boot-arm),
thanks!

-- 
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>

[U-Boot] [PATCH 0/5] Add splash screen for CM-T35

[Nikita Kiryanov]

Hi all,

this series is awaiting review for almost a month.
Can someone take a look at it?

On 12/23/2012 09:03 AM, Nikita Kiryanov wrote:
> 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
>


-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/5] Add splash screen for CM-T35

[Jeroen Hofstee]

Hello Nikita,

On 01/20/2013 01:25 PM, Nikita Kiryanov wrote:
> Hi all,
>
> this series is awaiting review for almost a month.
> Can someone take a look at it?
>
> On 12/23/2012 09:03 AM, Nikita Kiryanov wrote:
>> 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
>>
>
>
I am not a u-boot developer, but will at some comments.

Regards,
Jeroen

[U-Boot] [PATCH 0/5] Add splash screen for CM-T35

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/20/2013 07:25 AM, Nikita Kiryanov wrote:

> 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-----