[U-Boot] [PATCH 07/10] samsung: misc: Add LCD download menu.

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

Dear Minkyu,

On 12/11/2013 09:15 AM, Minkyu Kang wrote:
> Dear Przemyslaw Marczak,
>
> On 04/12/13 03:03, Przemyslaw Marczak wrote:
>> New configs:
>> - CONFIG_LCD_MENU
>> - CONFIG_LCD_MENU_BOARD
>> which depends on: CONFIG_MISC_INIT_R
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> ---
>>   board/samsung/common/keys.h |   78 ++++++++++
>>   board/samsung/common/misc.c |  354 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 432 insertions(+)
>>   create mode 100644 board/samsung/common/keys.h
>>
>> diff --git a/board/samsung/common/keys.h b/board/samsung/common/keys.h
>> new file mode 100644
>> index 0000000..48822d1
>> --- /dev/null
>> +++ b/board/samsung/common/keys.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Copyright (C) 2013 Samsung Electronics
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +#ifndef __SAMSUNG_KEYS__
>> +#define __SAMSUNG_KEYS__
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#include <config.h>
>> +#include <common.h>
>> +#include <power/pmic.h>
>> +#include <asm/arch/gpio.h>
>> +
>> +/* PMIC PWR ON key */
>> +#if defined(CONFIG_MACH_GONI) || defined(CONFIG_UNIVERSAL)
>> +
>> +#include <power/max8998_pmic.h>
>> +
>> +#define KEY_PWR_PMIC_NAME		"MAX8998_PMIC"
>> +
>> +#define KEY_PWR_STATUS_REG		MAX8998_REG_STATUS1
>> +#define KEY_PWR_STATUS_MASK		(1 << 7)
>> +
>> +#define KEY_PWR_INTERRUPT_REG		MAX8998_REG_IRQ1
>> +#define KEY_PWR_INTERRUPT_MASK		(1 << 7)
>> +
>> +#elif defined(CONFIG_TRATS)
>> +
>> +#include <power/max8997_pmic.h>
>> +
>> +#define KEY_PWR_PMIC_NAME		"MAX8997_PMIC"
>> +
>> +#define KEY_PWR_STATUS_REG		MAX8997_REG_STATUS1
>> +#define KEY_PWR_STATUS_MASK		(1 << 0)
>> +
>> +#define KEY_PWR_INTERRUPT_REG		MAX8997_REG_INT1
>> +#define KEY_PWR_INTERRUPT_MASK		(1 << 0)
>> +
>> +#elif defined(CONFIG_TRATS2)
>> +
>> +#include <power/max77686_pmic.h>
>> +
>> +#define KEY_PWR_PMIC_NAME		"MAX77686_PMIC"
>> +
>> +#define KEY_PWR_STATUS_REG		MAX77686_REG_PMIC_STATUS1
>> +#define KEY_PWR_STATUS_MASK		(1 << 0)
>> +
>> +#define KEY_PWR_INTERRUPT_REG		MAX77686_REG_PMIC_INT1
>> +#define KEY_PWR_INTERRUPT_MASK		(1 << 1)
>> +
>> +#endif /* PMIC PWR ON key */
>
> Hm no. it's a board specific feature so it should be go to each boards.
> Maybe we need some.. framework?
>

Ok, I will move it to boards configs headers.
And one more about some framework. I think this is good idea but I 
prefer to use this simple version first.
We can extend pmic framework with some "ops" to "struct pmic", eg.:
- get_power_key() - status or interrupt
- check_usb_cable()
the same as low_power_mode() which exists.
So at this time we can add two pmic extensions.

Also such pmic framework should provide some generic function to just 
get power key state and usb cable connection with no worry about 
specified pmic name like it is now.
This also adding new problem - what if board has few pmic instances...

This things requires other patch set and adding it here will increase 
this patch set apply time.
So I prefer to just put this code to boards headers.

>> +
>> +/* GPIO for Vol Up and Vol Down */
>> +#if defined(CONFIG_MACH_GONI)
>> +
>> +#define KEY_VOL_UP_GPIO			s5pc110_gpio_get(h3, 1)
>> +#define KEY_VOL_DOWN_GPIO		s5pc110_gpio_get(h3, 2)
>> +
>> +#elif defined(CONFIG_UNIVERSAL) || defined(CONFIG_TRATS)
>> +
>> +#define KEY_VOL_UP_GPIO			exynos4_gpio_get(2, x2, 0)
>> +#define KEY_VOL_DOWN_GPIO		exynos4_gpio_get(2, x2, 1)
>> +
>> +#elif defined(CONFIG_TRATS2)
>> +
>> +#define KEY_VOL_UP_GPIO			exynos4x12_gpio_get(2, x2, 2)
>> +#define KEY_VOL_DOWN_GPIO		exynos4x12_gpio_get(2, x3, 3)
>> +
>> +#else
>> +#ifdef CONFIG_MISC_INIT_R
>> +#warning Vol UP and Vol DOWN GPIO are undefined!
>> +#endif
>> +#endif /* GPIO for Vol Up and Vol Down */
>
> ditto.
> Will you add ifdef when you add new boards?
> It doesn't make sense.
>
>> +
>> +#endif /* __ASSEMBLY__ */
>> +#endif /* __SAMSUNG_KEYS__ */
>> diff --git a/board/samsung/common/misc.c b/board/samsung/common/misc.c
>> index fa97644..c792b87 100644
>> --- a/board/samsung/common/misc.c
>> +++ b/board/samsung/common/misc.c
>> @@ -8,8 +8,357 @@
>>   #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 <mmc.h>
>> +#include "keys.h"
>>
>>   #ifdef CONFIG_MISC_INIT_R
>> +struct s5p_gpio_bank *s5p_gpio_get_bank(unsigned);
>> +int s5p_gpio_get_pin(unsigned);
>> +#ifdef CONFIG_REVISION_TAG
>> +u32 get_board_rev(void);
>> +#endif
>> +
>> +#ifdef CONFIG_LCD_MENU
>> +enum {
>> +	BOOT_MODE_INFO,
>> +	BOOT_MODE_THOR,
>> +	BOOT_MODE_UMS,
>> +	BOOT_MODE_DFU,
>> +	BOOT_MODE_EXIT,
>> +};
>> +
>> +static int power_key_pressed(int reg)
>> +{
>> +	struct pmic *pmic = pmic_get(KEY_PWR_PMIC_NAME);
>> +	u32 status = 0;
>> +	u32 mask;
>> +
>> +	if (pmic_probe(pmic))
>> +		return 0;
>> +
>> +	if (!pmic) {
>> +		printf("%s: Not found\n", KEY_PWR_PMIC_NAME);
>> +		return -ENODEV;
>> +	}
>> +
>> +	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 -EIO;
>> +
>> +	return !!(status & mask);
>> +}
>> +
>> +static int key_pressed(int key)
>> +{
>> +	int value = 0;
>> +
>> +	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;
>> +}
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>
> please move it to top of this file.
>

OK.

>> +
>> +/*
>> + * 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",
>> +				"THOR",
>> +				"UMS",
>> +				"DFU",
>> +				"EXIT"};
>> +
>> +static char *
>> +mode_info[BOOT_MODE_EXIT + 1] = {"info",
>> +				 "downloader",
>> +				 "mass storage",
>> +				 "firmware update",
>> +				 "and run normal boot"};
>> +
>> +static char *
>> +mode_cmd[BOOT_MODE_EXIT + 1] = {"",
>> +				"thor 0 mmc 0",
>> +				"ums 0 mmc 0",
>> +				"dfu 0 mmc 0",
>> +				""};
>> +
>> +static void display_board_info(void)
>> +{
>> +	struct mmc *mmc = find_mmc_device(0);
>> +	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_BOARD_NAME
>> +	lcd_printf("\tBoard name: %s\n", CONFIG_BOARD_NAME);
>> +#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);
>> +
>> +	if (mmc)
>> +		lcd_printf("\teMMC size: %llu MB\n", mmc->capacity / SZ_1M);
>> +
>> +	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;
>> +	default:
>> +		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);
>> +			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 = "[  ]";
>> +	char *selection[BOOT_MODE_EXIT + 1];
>> +	int i;
>> +
>> +	for (i = 0; i <= BOOT_MODE_EXIT; i++)
>> +			selection[i] = blank;
>
> indentation error.
>
Right
>> +
>> +	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]);
>> +}
>> +
>> +void download_menu(void)
>> +{
>> +	int mode = 0;
>> +	int last_mode = 0;
>> +	int run;
>> +	int key;
>> +	int menu_exit = 0;
>> +
>> +	display_download_menu(mode);
>> +
>> +	while (1) {
>> +		run = 0;
>> +		menu_exit = 0;
>> +
>> +		if (mode != last_mode)
>> +			display_download_menu(mode);
>> +
>> +		last_mode = mode;
>> +		udelay(100000);
>> +
>> +		key = check_keys();
>
> as your implementation of check_keys function.
> you never catch if we pressed multi keys.
> What is the expected result of such a situation?
>

Multi key was not implemented as any functional meaning.
In this case it is the same as no key was pressed, so it is "default" 
case, and function will keep in loop.
I only use power key as "enter key" - it is intuitive.

>> +		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_BOARD_NAME
>> +	lcd_printf("Board name: %s\n", CONFIG_BOARD_NAME);
>> +#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);
>> +
>> +		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);
>> +		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) {
>
> if (!pwr_key) then 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)
>>   {
>> @@ -46,10 +395,15 @@ 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;
>>   }
>>   #endif /* CONFIG_MISC_INIT_R */
>>
>
> Thanks,
> Minkyu Kang.
>

I will apply comments and send next patch set soon.

Regards.
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com