From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 07 Jan 2014 13:46:50 +0100 Subject: [U-Boot] [PATCH v3 09/12] samsung: misc: Add LCD download menu. In-Reply-To: <52CA95CE.2040602@samsung.com> References: <1386093806-2948-1-git-send-email-p.marczak@samsung.com> <1388767393-16173-1-git-send-email-p.marczak@samsung.com> <1388767393-16173-9-git-send-email-p.marczak@samsung.com> <52CA95CE.2040602@samsung.com> Message-ID: <52CBF73A.3040600@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> >> --- >> 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 >> #include >> #include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#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