[U-Boot] [PATCH v3 09/12] samsung: misc: Add LCD download menu.
Przemyslaw Marczak
p.marczak at samsung.com
Tue Jan 7 13:46:50 CET 2014
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 at 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
More information about the U-Boot
mailing list