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

[Przemyslaw Marczak <p.marczak@samsung.com>]

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
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com