[U-Boot,v3,09/12] samsung: misc: Add LCD download menu.

This simple LCD menu allows run one of download mode on device
without writing on console or for fast and easy upgrade.
This feature check user keys combination at boot:
- power key + volume up - download menu
- power key + volume down - thor mode (without menu)

New configs:
- CONFIG_LCD_MENU
- CONFIG_LCD_MENU_BOARD
which depends on: CONFIG_MISC_INIT_R

For proper effect this feature needs following definitions:

Power key:
- KEY_PWR_PMIC_NAME - (string) pmic which supports power key check

Register address:
- KEY_PWR_STATUS_REG
- KEY_PWR_INTERRUPT_REG

Register power key mask:
- KEY_PWR_STATUS_MASK
- KEY_PWR_INTERRUPT_MASK

Gpio numbers:
- KEY_PWR_INTERRUPT_MASK
- KEY_VOL_DOWN_GPIO

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>

---
Changes v2:
- remove keys.h  - definitions should be in boards headers
- add misc.h
- code cleanup
- extend commit msg by more informations

Changes v3:
- none

 board/samsung/common/misc.c |  340 +++++++++++++++++++++++++++++++++++++++++++
 board/samsung/common/misc.h |   18 +++
 2 files changed, 358 insertions(+)
 create mode 100644 board/samsung/common/misc.h

\ No newline at end of file

On 04/01/14 01:43, Przemyslaw Marczak wrote:
> This simple LCD menu allows run one of download mode on device
> without writing on console or for fast and easy upgrade.
> This feature check user keys combination at boot:
> - power key + volume up - download menu
> - power key + volume down - thor mode (without menu)
> 
> New configs:
> - CONFIG_LCD_MENU
> - CONFIG_LCD_MENU_BOARD
> which depends on: CONFIG_MISC_INIT_R
> 
> For proper effect this feature needs following definitions:
> 
> Power key:
> - KEY_PWR_PMIC_NAME - (string) pmic which supports power key check
> 
> Register address:
> - KEY_PWR_STATUS_REG
> - KEY_PWR_INTERRUPT_REG
> 
> Register power key mask:
> - KEY_PWR_STATUS_MASK
> - KEY_PWR_INTERRUPT_MASK
> 
> Gpio numbers:
> - KEY_PWR_INTERRUPT_MASK
> - KEY_VOL_DOWN_GPIO
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> 
> ---
> Changes v2:
> - remove keys.h  - definitions should be in boards headers
> - add misc.h
> - code cleanup
> - extend commit msg by more informations
> 
> Changes v3:
> - none
> 
>  board/samsung/common/misc.c |  340 +++++++++++++++++++++++++++++++++++++++++++
>  board/samsung/common/misc.h |   18 +++
>  2 files changed, 358 insertions(+)
>  create mode 100644 board/samsung/common/misc.h
> 
> diff --git a/board/samsung/common/misc.c b/board/samsung/common/misc.c
> index 3a91d62..93f766c 100644
> --- a/board/samsung/common/misc.c
> +++ b/board/samsung/common/misc.c
> @@ -8,6 +8,341 @@
>  #include <common.h>
>  #include <lcd.h>
>  #include <libtizen.h>
> +#include <errno.h>
> +#include <version.h>
> +#include <asm/sizes.h>
> +#include <asm/arch/cpu.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/gpio.h>
> +#include <linux/input.h>
> +#include <lcd.h>
> +#include <libtizen.h>
> +#include <power/pmic.h>
> +#include <mmc.h>
> +#include "misc.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#ifdef CONFIG_LCD_MENU
> +static int power_key_pressed(int reg)

u32 reg?

> +{
> +	struct pmic *pmic = pmic_get(KEY_PWR_PMIC_NAME);
> +	u32 status = 0;

= 0 do not need.
please do not use initial value, if possible.

> +	u32 mask;

please check return value of pmic_get

	pmic = pmic_get(KEY_PWR_PMIC_NAME);
	if !(pmic)
		return 0;

> +
> +	if (pmic_probe(pmic))
> +		return 0;
> +
> +	if (!pmic) {
> +		printf("%s: Not found\n", KEY_PWR_PMIC_NAME);
> +		return 0;
> +	}
> +
> +	if (reg == KEY_PWR_STATUS_REG)
> +		mask = KEY_PWR_STATUS_MASK;
> +	else
> +		mask = KEY_PWR_INTERRUPT_MASK;
> +
> +	if (pmic_reg_read(pmic, reg, &status))
> +		return 0;
> +
> +	return !!(status & mask);
> +}
> +
> +static int key_pressed(int key)
> +{
> +	int value = 0;

please do not use initial value, if possible.
instead, please add "value = 0" at default statement.

> +
> +	switch (key) {
> +	case KEY_POWER:
> +		value = power_key_pressed(KEY_PWR_INTERRUPT_REG);
> +		break;
> +	case KEY_VOLUMEUP:
> +		value = !gpio_get_value(KEY_VOL_UP_GPIO);
> +		break;
> +	case KEY_VOLUMEDOWN:
> +		value = !gpio_get_value(KEY_VOL_DOWN_GPIO);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return value;
> +}
> +
> +static int check_keys(void)
> +{
> +	int keys = 0;
> +
> +	if (key_pressed(KEY_POWER))
> +		keys += KEY_POWER;
> +	if (key_pressed(KEY_VOLUMEUP))
> +		keys += KEY_VOLUMEUP;
> +	if (key_pressed(KEY_VOLUMEDOWN))
> +		keys += KEY_VOLUMEDOWN;
> +
> +	return keys;
> +}
> +
> +/*
> + * 0 BOOT_MODE_INFO
> + * 1 BOOT_MODE_THOR
> + * 2 BOOT_MODE_UMS
> + * 3 BOOT_MODE_DFU
> + * 4 BOOT_MODE_EXIT
> + */
> +static char *
> +mode_name[BOOT_MODE_EXIT + 1] = {"DEVICE",

please move the "DEVICE" to next line.

> +				"THOR",
> +				"UMS",
> +				"DFU",
> +				"EXIT"};

please move the brace to next line.

> +
> +static char *
> +mode_info[BOOT_MODE_EXIT + 1] = {"info",
> +				 "downloader",
> +				 "mass storage",
> +				 "firmware update",
> +				 "and run normal boot"};

ditto.

> +
> +static char *
> +mode_cmd[BOOT_MODE_EXIT + 1] = {"",
> +				"thor 0 mmc 0",
> +				"ums 0 mmc 0",
> +				"dfu 0 mmc 0",
> +				""};

ditto.

> +
> +static void display_board_info(void)
> +{

#ifdef CONFIG_GENERIC_MMC
> +	struct mmc *mmc = find_mmc_device(0);
#endif

> +	vidinfo_t *vid = &panel_info;
> +
> +	lcd_position_cursor(4, 4);
> +
> +	lcd_printf("%s\n\t", U_BOOT_VERSION);
> +	lcd_puts("\n\t\tBoard Info:\n");
> +#ifdef CONFIG_SYS_BOARD
> +	lcd_printf("\tBoard name: %s\n", CONFIG_SYS_BOARD);
> +#endif
> +#ifdef CONFIG_REVISION_TAG
> +	lcd_printf("\tBoard rev: %u\n", get_board_rev());
> +#endif
> +	lcd_printf("\tDRAM banks: %u\n", CONFIG_NR_DRAM_BANKS);
> +	lcd_printf("\tDRAM size: %u MB\n", gd->ram_size / SZ_1M);
> +

#ifdef CONFIG_GENERIC_MMC
> +	if (mmc)
> +		lcd_printf("\teMMC size: %llu MB\n", mmc->capacity / SZ_1M);
#endif

maybe, you can access mmc->capacity after do mmc_init.

> +
> +	if (vid)
> +		lcd_printf("\tDisplay resolution: %u x % u\n",
> +			   vid->vl_col, vid->vl_row);
> +
> +	lcd_printf("\tDisplay BPP: %u\n", 1 << vid->vl_bpix);
> +}
> +
> +static int mode_leave_menu(int mode)
> +{
> +	int mode_supported = 1;
> +	int leave = 0;
> +	char *exit_option;
> +	char *exit_boot = "boot";
> +	char *exit_back = "back";
> +
> +	switch (mode) {
> +	case BOOT_MODE_INFO:
> +#if !defined(CONFIG_LCD_MENU_BOARD)
> +		mode_supported = 0;
> +#endif
> +		break;
> +	case BOOT_MODE_THOR:
> +#if !defined(CONFIG_CMD_THOR_DOWNLOAD)
> +		mode_supported = 0;
> +#endif
> +		break;
> +	case BOOT_MODE_UMS:
> +#if !defined(CONFIG_CMD_USB_MASS_STORAGE)
> +		mode_supported = 0;
> +#endif
> +		break;
> +	case BOOT_MODE_DFU:
> +#if !defined(CONFIG_CMD_DFU)
> +		mode_supported = 0;
> +#endif
> +		break;
> +	case BOOT_MODE_EXIT:
> +		leave = 1;
> +		goto exit;

why don't you return directly at here.
goto statement is used here only.

> +	default:

then, mode_supported = 0; maybe?

> +		break;
> +	}
> +
> +	lcd_clear();
> +
> +	if (mode_supported) {
> +		if (mode) {
> +			lcd_printf("\n\n\t%s %s\n", mode_name[mode],
> +						    mode_info[mode]);
> +			lcd_puts("\n\tDo not turn off device before finish!\n");
> +
> +			run_command(mode_cmd[mode], 0);

do not need to check a return value?

> +			printf("Command finished\n");
> +			lcd_clear();
> +			lcd_printf("\n\n\t%s finished\n", mode_name[mode]);
> +			exit_option = exit_boot;
> +			leave = 1;
> +		} else {
> +			display_board_info();
> +			exit_option = exit_back;
> +			leave = 0;
> +		}
> +	} else {
> +		lcd_puts("\n\n\tThis mode is not supported.\n");
> +		exit_option = exit_back;
> +		leave = 0;
> +	}
> +
> +	lcd_printf("\n\n\tPress POWER KEY to %s\n", exit_option);
> +
> +	/* Wait for PWR key */
> +	while (!key_pressed(KEY_POWER))
> +		udelay(1000);
> +exit:
> +	lcd_clear();
> +	return leave;
> +}
> +
> +static void display_download_menu(int mode)
> +{
> +	char *menu_name = "Download Mode Menu";
> +	char *indicator = "[=>]";
> +	char *blank = "[  ]";

Do we need those three variable?

> +	char *selection[BOOT_MODE_EXIT + 1];
> +	int i;
> +
> +	for (i = 0; i <= BOOT_MODE_EXIT; i++)
> +		selection[i] = blank;
> +
> +	selection[mode] = indicator;
> +
> +	lcd_clear();
> +	lcd_printf("\n\t\t%s\n", menu_name);
> +
> +	for (i = 0; i <= BOOT_MODE_EXIT; i++)
> +		lcd_printf("\t%s  %s - %s\n\n", selection[i],
> +						mode_name[i],
> +						mode_info[i]);
> +}
> +
> +static void download_menu(void)
> +{
> +	int mode = 0;
> +	int last_mode = 0;
> +	int run;
> +	int key;
> +
> +	display_download_menu(mode);
> +
> +	while (1) {
> +		run = 0;
> +
> +		if (mode != last_mode)
> +			display_download_menu(mode);
> +
> +		last_mode = mode;
> +		udelay(100000);
> +
> +		key = check_keys();
> +		switch (key) {
> +		case KEY_POWER:
> +			run = 1;
> +			break;
> +		case KEY_VOLUMEUP:
> +			if (mode > 0)
> +				mode--;
> +			break;
> +		case KEY_VOLUMEDOWN:
> +			if (mode < BOOT_MODE_EXIT)
> +				mode++;
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (run) {
> +			if (mode_leave_menu(mode))
> +				break;
> +
> +			display_download_menu(mode);
> +		}
> +	}
> +
> +	lcd_clear();
> +}
> +
> +static void display_mode_info(void)
> +{
> +	lcd_position_cursor(4, 4);
> +	lcd_printf("%s\n", U_BOOT_VERSION);
> +	lcd_puts("\nDownload Mode Menu\n");
> +#ifdef CONFIG_SYS_BOARD
> +	lcd_printf("Board name: %s\n", CONFIG_SYS_BOARD);
> +#endif
> +	lcd_printf("Press POWER KEY to display MENU options.");
> +}
> +
> +static int boot_menu(void)
> +{
> +	int key = 0;
> +	int timeout = 10;
> +
> +	display_mode_info();
> +
> +	while (timeout--) {
> +		lcd_printf("\rNormal boot will start in: %d seconds.", timeout);
> +		udelay(1000000);

I think.. user input can be ignored because too long delay.
1 second? it's too long.
and how about use mdelay instead?

> +
> +		key = key_pressed(KEY_POWER);
> +		if (key)
> +			break;
> +	}
> +
> +	lcd_clear();
> +
> +	/* If PWR pressed - show download menu */
> +	if (key) {
> +		printf("Power pressed - go to download menu\n");
> +		udelay(1000000);

delay for 1 second? is it need?

> +		download_menu();
> +		printf("Download mode exit.\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static void check_boot_mode(void)
> +{
> +	int pwr_key;
> +
> +	pwr_key = power_key_pressed(KEY_PWR_STATUS_REG);
> +	if (!pwr_key)
> +		return;
> +
> +	/* Clear PWR button Rising edge interrupt status flag */
> +	power_key_pressed(KEY_PWR_INTERRUPT_REG);
> +
> +	if (key_pressed(KEY_VOLUMEUP))
> +		boot_menu();
> +	else if (key_pressed(KEY_VOLUMEDOWN))
> +		mode_leave_menu(BOOT_MODE_THOR);
> +}
> +
> +static void keys_init(void)
> +{
> +	/* Set direction to input */
> +	gpio_direction_input(KEY_VOL_UP_GPIO);
> +	gpio_direction_input(KEY_VOL_DOWN_GPIO);
> +}
> +#endif /* CONFIG_LCD_MENU */
>  
>  #ifdef CONFIG_CMD_BMP
>  static void draw_logo(void)
> @@ -50,9 +385,14 @@ static void draw_logo(void)
>  /* Common for Samsung boards */
>  int misc_init_r(void)
>  {
> +#ifdef CONFIG_LCD_MENU
> +	keys_init();
> +	check_boot_mode();
> +#endif
>  #ifdef CONFIG_CMD_BMP
>  	if (panel_info.logo_on)
>  		draw_logo();
>  #endif
> +
>  	return 0;
>  }
> diff --git a/board/samsung/common/misc.h b/board/samsung/common/misc.h
> new file mode 100644
> index 0000000..f583552
> --- /dev/null
> +++ b/board/samsung/common/misc.h
> @@ -0,0 +1,18 @@
> +#ifndef __SAMSUNG_COMMON_MISC_H__
> +#define __SAMSUNG_COMMON_MISC_H__
> +
> +#ifdef CONFIG_LCD_MENU
> +enum {
> +	BOOT_MODE_INFO,
> +	BOOT_MODE_THOR,
> +	BOOT_MODE_UMS,
> +	BOOT_MODE_DFU,
> +	BOOT_MODE_EXIT,
> +};
> +
> +#ifdef CONFIG_REVISION_TAG
> +u32 get_board_rev(void);
> +#endif
> +#endif /* CONFIG_LCD_MENU */
> +
> +#endif /* __SAMSUNG_COMMON_MISC_H__ */
> \ No newline at end of file
> 

Thanks,
Minkyu Kang.

Hello Minkyu,

Thank you for review.

On 01/06/2014 12:38 PM, Minkyu Kang wrote:
> On 04/01/14 01:43, Przemyslaw Marczak wrote:
>> This simple LCD menu allows run one of download mode on device
>> without writing on console or for fast and easy upgrade.
>> This feature check user keys combination at boot:
>> - power key + volume up - download menu
>> - power key + volume down - thor mode (without menu)
>>
>> New configs:
>> - CONFIG_LCD_MENU
>> - CONFIG_LCD_MENU_BOARD
>> which depends on: CONFIG_MISC_INIT_R
>>
>> For proper effect this feature needs following definitions:
>>
>> Power key:
>> - KEY_PWR_PMIC_NAME - (string) pmic which supports power key check
>>
>> Register address:
>> - KEY_PWR_STATUS_REG
>> - KEY_PWR_INTERRUPT_REG
>>
>> Register power key mask:
>> - KEY_PWR_STATUS_MASK
>> - KEY_PWR_INTERRUPT_MASK
>>
>> Gpio numbers:
>> - KEY_PWR_INTERRUPT_MASK
>> - KEY_VOL_DOWN_GPIO
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>
>> ---
>> Changes v2:
>> - remove keys.h  - definitions should be in boards headers
>> - add misc.h
>> - code cleanup
>> - extend commit msg by more informations
>>
>> Changes v3:
>> - none
>>
>>   board/samsung/common/misc.c |  340 +++++++++++++++++++++++++++++++++++++++++++
>>   board/samsung/common/misc.h |   18 +++
>>   2 files changed, 358 insertions(+)
>>   create mode 100644 board/samsung/common/misc.h
>>
>> diff --git a/board/samsung/common/misc.c b/board/samsung/common/misc.c
>> index 3a91d62..93f766c 100644
>> --- a/board/samsung/common/misc.c
>> +++ b/board/samsung/common/misc.c
>> @@ -8,6 +8,341 @@
>>   #include <common.h>
>>   #include <lcd.h>
>>   #include <libtizen.h>
>> +#include <errno.h>
>> +#include <version.h>
>> +#include <asm/sizes.h>
>> +#include <asm/arch/cpu.h>
>> +#include <asm/arch/gpio.h>
>> +#include <asm/gpio.h>
>> +#include <linux/input.h>
>> +#include <lcd.h>
>> +#include <libtizen.h>
>> +#include <power/pmic.h>
>> +#include <mmc.h>
>> +#include "misc.h"
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#ifdef CONFIG_LCD_MENU
>> +static int power_key_pressed(int reg)
>
> u32 reg?
>
>> +{
>> +	struct pmic *pmic = pmic_get(KEY_PWR_PMIC_NAME);
>> +	u32 status = 0;
>
> = 0 do not need.
> please do not use initial value, if possible.
>
>> +	u32 mask;
>
> please check return value of pmic_get
>
> 	pmic = pmic_get(KEY_PWR_PMIC_NAME);
> 	if !(pmic)
> 		return 0;
>
>> +
>> +	if (pmic_probe(pmic))
>> +		return 0;
>> +
>> +	if (!pmic) {
>> +		printf("%s: Not found\n", KEY_PWR_PMIC_NAME);
>> +		return 0;
>> +	}
>> +
>> +	if (reg == KEY_PWR_STATUS_REG)
>> +		mask = KEY_PWR_STATUS_MASK;
>> +	else
>> +		mask = KEY_PWR_INTERRUPT_MASK;
>> +
>> +	if (pmic_reg_read(pmic, reg, &status))
>> +		return 0;
>> +
>> +	return !!(status & mask);
>> +}
>> +
>> +static int key_pressed(int key)
>> +{
>> +	int value = 0;
>
> please do not use initial value, if possible.
> instead, please add "value = 0" at default statement.
>
>> +
>> +	switch (key) {
>> +	case KEY_POWER:
>> +		value = power_key_pressed(KEY_PWR_INTERRUPT_REG);
>> +		break;
>> +	case KEY_VOLUMEUP:
>> +		value = !gpio_get_value(KEY_VOL_UP_GPIO);
>> +		break;
>> +	case KEY_VOLUMEDOWN:
>> +		value = !gpio_get_value(KEY_VOL_DOWN_GPIO);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return value;
>> +}
>> +
>> +static int check_keys(void)
>> +{
>> +	int keys = 0;
>> +
>> +	if (key_pressed(KEY_POWER))
>> +		keys += KEY_POWER;
>> +	if (key_pressed(KEY_VOLUMEUP))
>> +		keys += KEY_VOLUMEUP;
>> +	if (key_pressed(KEY_VOLUMEDOWN))
>> +		keys += KEY_VOLUMEDOWN;
>> +
>> +	return keys;
>> +}
>> +
>> +/*
>> + * 0 BOOT_MODE_INFO
>> + * 1 BOOT_MODE_THOR
>> + * 2 BOOT_MODE_UMS
>> + * 3 BOOT_MODE_DFU
>> + * 4 BOOT_MODE_EXIT
>> + */
>> +static char *
>> +mode_name[BOOT_MODE_EXIT + 1] = {"DEVICE",
>
> please move the "DEVICE" to next line.
>
>> +				"THOR",
>> +				"UMS",
>> +				"DFU",
>> +				"EXIT"};
>
> please move the brace to next line.
>
>> +
>> +static char *
>> +mode_info[BOOT_MODE_EXIT + 1] = {"info",
>> +				 "downloader",
>> +				 "mass storage",
>> +				 "firmware update",
>> +				 "and run normal boot"};
>
> ditto.
>
>> +
>> +static char *
>> +mode_cmd[BOOT_MODE_EXIT + 1] = {"",
>> +				"thor 0 mmc 0",
>> +				"ums 0 mmc 0",
>> +				"dfu 0 mmc 0",
>> +				""};
>
> ditto.
>
>> +
>> +static void display_board_info(void)
>> +{
>
> #ifdef CONFIG_GENERIC_MMC
>> +	struct mmc *mmc = find_mmc_device(0);
> #endif
>
>> +	vidinfo_t *vid = &panel_info;
>> +
>> +	lcd_position_cursor(4, 4);
>> +
>> +	lcd_printf("%s\n\t", U_BOOT_VERSION);
>> +	lcd_puts("\n\t\tBoard Info:\n");
>> +#ifdef CONFIG_SYS_BOARD
>> +	lcd_printf("\tBoard name: %s\n", CONFIG_SYS_BOARD);
>> +#endif
>> +#ifdef CONFIG_REVISION_TAG
>> +	lcd_printf("\tBoard rev: %u\n", get_board_rev());
>> +#endif
>> +	lcd_printf("\tDRAM banks: %u\n", CONFIG_NR_DRAM_BANKS);
>> +	lcd_printf("\tDRAM size: %u MB\n", gd->ram_size / SZ_1M);
>> +
>
> #ifdef CONFIG_GENERIC_MMC
>> +	if (mmc)
>> +		lcd_printf("\teMMC size: %llu MB\n", mmc->capacity / SZ_1M);
> #endif
>
> maybe, you can access mmc->capacity after do mmc_init.
>

I will add mmc_init for sure here. But in most cases mmc is initialized 
at misc_init_r stage because of environment initialization.

>> +
>> +	if (vid)
>> +		lcd_printf("\tDisplay resolution: %u x % u\n",
>> +			   vid->vl_col, vid->vl_row);
>> +
>> +	lcd_printf("\tDisplay BPP: %u\n", 1 << vid->vl_bpix);
>> +}
>> +
>> +static int mode_leave_menu(int mode)
>> +{
>> +	int mode_supported = 1;
>> +	int leave = 0;
>> +	char *exit_option;
>> +	char *exit_boot = "boot";
>> +	char *exit_back = "back";
>> +
>> +	switch (mode) {
>> +	case BOOT_MODE_INFO:
>> +#if !defined(CONFIG_LCD_MENU_BOARD)
>> +		mode_supported = 0;
>> +#endif
>> +		break;
>> +	case BOOT_MODE_THOR:
>> +#if !defined(CONFIG_CMD_THOR_DOWNLOAD)
>> +		mode_supported = 0;
>> +#endif
>> +		break;
>> +	case BOOT_MODE_UMS:
>> +#if !defined(CONFIG_CMD_USB_MASS_STORAGE)
>> +		mode_supported = 0;
>> +#endif
>> +		break;
>> +	case BOOT_MODE_DFU:
>> +#if !defined(CONFIG_CMD_DFU)
>> +		mode_supported = 0;
>> +#endif
>> +		break;
>> +	case BOOT_MODE_EXIT:
>> +		leave = 1;
>> +		goto exit;
>
> why don't you return directly at here.
> goto statement is used here only.
>

Ok, will be changed.

>> +	default:
>
> then, mode_supported = 0; maybe?
>
>> +		break;
>> +	}
>> +
>> +	lcd_clear();
>> +
>> +	if (mode_supported) {
>> +		if (mode) {
>> +			lcd_printf("\n\n\t%s %s\n", mode_name[mode],
>> +						    mode_info[mode]);
>> +			lcd_puts("\n\tDo not turn off device before finish!\n");
>> +
>> +			run_command(mode_cmd[mode], 0);
>
> do not need to check a return value?
>

You're right,  I will add checking here.

>> +			printf("Command finished\n");
>> +			lcd_clear();
>> +			lcd_printf("\n\n\t%s finished\n", mode_name[mode]);
>> +			exit_option = exit_boot;
>> +			leave = 1;
>> +		} else {
>> +			display_board_info();
>> +			exit_option = exit_back;
>> +			leave = 0;
>> +		}
>> +	} else {
>> +		lcd_puts("\n\n\tThis mode is not supported.\n");
>> +		exit_option = exit_back;
>> +		leave = 0;
>> +	}
>> +
>> +	lcd_printf("\n\n\tPress POWER KEY to %s\n", exit_option);
>> +
>> +	/* Wait for PWR key */
>> +	while (!key_pressed(KEY_POWER))
>> +		udelay(1000);
>> +exit:
>> +	lcd_clear();
>> +	return leave;
>> +}
>> +
>> +static void display_download_menu(int mode)
>> +{
>> +	char *menu_name = "Download Mode Menu";
>> +	char *indicator = "[=>]";
>> +	char *blank = "[  ]";
>
> Do we need those three variable?
>

Actually we don't. Will be removed.


>> +	char *selection[BOOT_MODE_EXIT + 1];
>> +	int i;
>> +
>> +	for (i = 0; i <= BOOT_MODE_EXIT; i++)
>> +		selection[i] = blank;
>> +
>> +	selection[mode] = indicator;
>> +
>> +	lcd_clear();
>> +	lcd_printf("\n\t\t%s\n", menu_name);
>> +
>> +	for (i = 0; i <= BOOT_MODE_EXIT; i++)
>> +		lcd_printf("\t%s  %s - %s\n\n", selection[i],
>> +						mode_name[i],
>> +						mode_info[i]);
>> +}
>> +
>> +static void download_menu(void)
>> +{
>> +	int mode = 0;
>> +	int last_mode = 0;
>> +	int run;
>> +	int key;
>> +
>> +	display_download_menu(mode);
>> +
>> +	while (1) {
>> +		run = 0;
>> +
>> +		if (mode != last_mode)
>> +			display_download_menu(mode);
>> +
>> +		last_mode = mode;
>> +		udelay(100000);
>> +
>> +		key = check_keys();
>> +		switch (key) {
>> +		case KEY_POWER:
>> +			run = 1;
>> +			break;
>> +		case KEY_VOLUMEUP:
>> +			if (mode > 0)
>> +				mode--;
>> +			break;
>> +		case KEY_VOLUMEDOWN:
>> +			if (mode < BOOT_MODE_EXIT)
>> +				mode++;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +
>> +		if (run) {
>> +			if (mode_leave_menu(mode))
>> +				break;
>> +
>> +			display_download_menu(mode);
>> +		}
>> +	}
>> +
>> +	lcd_clear();
>> +}
>> +
>> +static void display_mode_info(void)
>> +{
>> +	lcd_position_cursor(4, 4);
>> +	lcd_printf("%s\n", U_BOOT_VERSION);
>> +	lcd_puts("\nDownload Mode Menu\n");
>> +#ifdef CONFIG_SYS_BOARD
>> +	lcd_printf("Board name: %s\n", CONFIG_SYS_BOARD);
>> +#endif
>> +	lcd_printf("Press POWER KEY to display MENU options.");
>> +}
>> +
>> +static int boot_menu(void)
>> +{
>> +	int key = 0;
>> +	int timeout = 10;
>> +
>> +	display_mode_info();
>> +
>> +	while (timeout--) {
>> +		lcd_printf("\rNormal boot will start in: %d seconds.", timeout);
>> +		udelay(1000000);
>
> I think.. user input can be ignored because too long delay.
> 1 second? it's too long.
> and how about use mdelay instead?
>

User input can't be ignored because key_pressed(KEY_POWER) is using pmic 
interrupt register. It is no matter when user press the power key, if he 
does then he will wait at most 1 second.

>> +
>> +		key = key_pressed(KEY_POWER);
>> +		if (key)
>> +			break;
>> +	}
>> +
>> +	lcd_clear();
>> +
>> +	/* If PWR pressed - show download menu */
>> +	if (key) {
>> +		printf("Power pressed - go to download menu\n");
>> +		udelay(1000000);
>
> delay for 1 second? is it need?
You're right It is unneeded here.

>
>> +		download_menu();
>> +		printf("Download mode exit.\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void check_boot_mode(void)
>> +{
>> +	int pwr_key;
>> +
>> +	pwr_key = power_key_pressed(KEY_PWR_STATUS_REG);
>> +	if (!pwr_key)
>> +		return;
>> +
>> +	/* Clear PWR button Rising edge interrupt status flag */
>> +	power_key_pressed(KEY_PWR_INTERRUPT_REG);
>> +
>> +	if (key_pressed(KEY_VOLUMEUP))
>> +		boot_menu();
>> +	else if (key_pressed(KEY_VOLUMEDOWN))
>> +		mode_leave_menu(BOOT_MODE_THOR);
>> +}
>> +
>> +static void keys_init(void)
>> +{
>> +	/* Set direction to input */
>> +	gpio_direction_input(KEY_VOL_UP_GPIO);
>> +	gpio_direction_input(KEY_VOL_DOWN_GPIO);
>> +}
>> +#endif /* CONFIG_LCD_MENU */
>>
>>   #ifdef CONFIG_CMD_BMP
>>   static void draw_logo(void)
>> @@ -50,9 +385,14 @@ static void draw_logo(void)
>>   /* Common for Samsung boards */
>>   int misc_init_r(void)
>>   {
>> +#ifdef CONFIG_LCD_MENU
>> +	keys_init();
>> +	check_boot_mode();
>> +#endif
>>   #ifdef CONFIG_CMD_BMP
>>   	if (panel_info.logo_on)
>>   		draw_logo();
>>   #endif
>> +
>>   	return 0;
>>   }
>> diff --git a/board/samsung/common/misc.h b/board/samsung/common/misc.h
>> new file mode 100644
>> index 0000000..f583552
>> --- /dev/null
>> +++ b/board/samsung/common/misc.h
>> @@ -0,0 +1,18 @@
>> +#ifndef __SAMSUNG_COMMON_MISC_H__
>> +#define __SAMSUNG_COMMON_MISC_H__
>> +
>> +#ifdef CONFIG_LCD_MENU
>> +enum {
>> +	BOOT_MODE_INFO,
>> +	BOOT_MODE_THOR,
>> +	BOOT_MODE_UMS,
>> +	BOOT_MODE_DFU,
>> +	BOOT_MODE_EXIT,
>> +};
>> +
>> +#ifdef CONFIG_REVISION_TAG
>> +u32 get_board_rev(void);
>> +#endif
>> +#endif /* CONFIG_LCD_MENU */
>> +
>> +#endif /* __SAMSUNG_COMMON_MISC_H__ */
>> \ No newline at end of file
>>
>
> Thanks,
> Minkyu Kang.
>

Comments will be applied to next version.
Regards