| Message ID | 1505224713-30097-1-git-send-email-andy.yan@rock-chips.com |
|---|---|
| State | Superseded |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | support enter download mode on rockchip platform | expand |
Hi Andy, On 12 September 2017 at 07:58, Andy Yan <andy.yan@rock-chips.com> wrote: > Enter download mode if the download key pressed. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > --- > > arch/arm/mach-rockchip/boot_mode.c | 27 ++++++++++++++++++++++++++- > board/rockchip/evb_rk3399/evb-rk3399.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-rockchip/boot_mode.c b/arch/arm/mach-rockchip/boot_mode.c > index 7b3cbc5..bff1cbc 100644 > --- a/arch/arm/mach-rockchip/boot_mode.c > +++ b/arch/arm/mach-rockchip/boot_mode.c > @@ -8,11 +8,36 @@ > #include <asm/io.h> > #include <asm/arch/boot_mode.h> > > +void set_back_to_bootrom_dnl_flag(void) > +{ > + writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG); > +} > + > +/* > + * some boards use a adc key, but some use gpio > + */ > +__weak int rockchip_dnl_key_pressed(void) Can you please declare this in a header file and add a full comment as to what this does? > +{ > + return false; > +} > + > +void rockchip_dnl_mode_check(void) > +{ > + if (rockchip_dnl_key_pressed()) { > + printf("download key pressed, enter download mode..."); should that be 'entering'? > + set_back_to_bootrom_dnl_flag(); > + do_reset(NULL, 0, 0, NULL); > + } > +} > + > int setup_boot_mode(void) > { > void* reg = (void *)CONFIG_ROCKCHIP_BOOT_MODE_REG; > - int boot_mode = readl(reg); > + int boot_mode; > + > + rockchip_dnl_mode_check(); > > + boot_mode = readl(reg); > debug("boot mode %x.\n", boot_mode); > > /* Clear boot mode */ > diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c > index d50c59d..738d942 100644 > --- a/board/rockchip/evb_rk3399/evb-rk3399.c > +++ b/board/rockchip/evb_rk3399/evb-rk3399.c > @@ -4,6 +4,7 @@ > * SPDX-License-Identifier: GPL-2.0+ > */ > #include <common.h> > +#include <adc.h> > #include <dm.h> > #include <ram.h> > #include <dm/pinctrl.h> > @@ -13,6 +14,33 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +#define KEY_DOWN_MIN_VAL 1 > +#define KEY_DOWN_MAX_VAL 20 > + > +int rockchip_dnl_key_pressed(void) > +{ > + unsigned int ret; > + unsigned int i; > + unsigned int val; > + int cnt = 0; > + Please document what is going on here - why the loop? What is the check against min/max value? > + for (i = 0; i < 10; i++) { > + ret = adc_channel_single_shot("saradc", 1, &val); > + if (ret) { > + printf("%s adc_channel_single_shot fail!\n", __func__); > + break; > + } > + > + if ((val >= KEY_DOWN_MIN_VAL) && (val <= KEY_DOWN_MAX_VAL)) > + cnt++; > + } > + > + if (cnt >= 8) > + return true; > + else > + return false; > +} > + > int board_init(void) > { > struct udevice *pinctrl, *regulator; > -- > 2.7.4 > > Regards, Simon
Hi Simon: On 2017年09月13日 12:26, Simon Glass wrote: > Hi Andy, > > On 12 September 2017 at 07:58, Andy Yan <andy.yan@rock-chips.com> wrote: >> Enter download mode if the download key pressed. >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> --- >> >> arch/arm/mach-rockchip/boot_mode.c | 27 ++++++++++++++++++++++++++- >> board/rockchip/evb_rk3399/evb-rk3399.c | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 54 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-rockchip/boot_mode.c b/arch/arm/mach-rockchip/boot_mode.c >> index 7b3cbc5..bff1cbc 100644 >> --- a/arch/arm/mach-rockchip/boot_mode.c >> +++ b/arch/arm/mach-rockchip/boot_mode.c >> @@ -8,11 +8,36 @@ >> #include <asm/io.h> >> #include <asm/arch/boot_mode.h> >> >> +void set_back_to_bootrom_dnl_flag(void) >> +{ >> + writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG); >> +} >> + >> +/* >> + * some boards use a adc key, but some use gpio >> + */ >> +__weak int rockchip_dnl_key_pressed(void) > Can you please declare this in a header file and add a full comment as > to what this does? This function is only used in this file and can be override by other boards. So I don't think this is a necessary to declare this in a header file. Of course, I will add a full comment for it. >> +{ >> + return false; >> +} >> + >> +void rockchip_dnl_mode_check(void) >> +{ >> + if (rockchip_dnl_key_pressed()) { >> + printf("download key pressed, enter download mode..."); > should that be 'entering'? Okay, should be 'entering' >> + set_back_to_bootrom_dnl_flag(); >> + do_reset(NULL, 0, 0, NULL); >> + } >> +} >> + >> int setup_boot_mode(void) >> { >> void* reg = (void *)CONFIG_ROCKCHIP_BOOT_MODE_REG; >> - int boot_mode = readl(reg); >> + int boot_mode; >> + >> + rockchip_dnl_mode_check(); >> >> + boot_mode = readl(reg); >> debug("boot mode %x.\n", boot_mode); >> >> /* Clear boot mode */ >> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c >> index d50c59d..738d942 100644 >> --- a/board/rockchip/evb_rk3399/evb-rk3399.c >> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c >> @@ -4,6 +4,7 @@ >> * SPDX-License-Identifier: GPL-2.0+ >> */ >> #include <common.h> >> +#include <adc.h> >> #include <dm.h> >> #include <ram.h> >> #include <dm/pinctrl.h> >> @@ -13,6 +14,33 @@ >> >> DECLARE_GLOBAL_DATA_PTR; >> >> +#define KEY_DOWN_MIN_VAL 1 >> +#define KEY_DOWN_MAX_VAL 20 >> + >> +int rockchip_dnl_key_pressed(void) >> +{ >> + unsigned int ret; >> + unsigned int i; >> + unsigned int val; >> + int cnt = 0; >> + > Please document what is going on here - why the loop? What is the > check against min/max value? This if for voltage debounce, according to my experience on the rk3399 evb board, in the 10 adc sample results, we will met 2 ~ 4 result vals are out of the MIN~MAX range. > >> + for (i = 0; i < 10; i++) { >> + ret = adc_channel_single_shot("saradc", 1, &val); >> + if (ret) { >> + printf("%s adc_channel_single_shot fail!\n", __func__); >> + break; >> + } >> + >> + if ((val >= KEY_DOWN_MIN_VAL) && (val <= KEY_DOWN_MAX_VAL)) >> + cnt++; >> + } >> + >> + if (cnt >= 8) >> + return true; >> + else >> + return false; >> +} >> + >> int board_init(void) >> { >> struct udevice *pinctrl, *regulator; >> -- >> 2.7.4 >> >> > Regards, > Simon > > >
Hi Andy, On 13 September 2017 at 00:14, Andy Yan <andy.yan@rock-chips.com> wrote: > Hi Simon: > > > > On 2017年09月13日 12:26, Simon Glass wrote: >> >> Hi Andy, >> >> On 12 September 2017 at 07:58, Andy Yan <andy.yan@rock-chips.com> wrote: >>> >>> Enter download mode if the download key pressed. >>> >>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >>> --- >>> >>> arch/arm/mach-rockchip/boot_mode.c | 27 ++++++++++++++++++++++++++- >>> board/rockchip/evb_rk3399/evb-rk3399.c | 28 >>> ++++++++++++++++++++++++++++ >>> 2 files changed, 54 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mach-rockchip/boot_mode.c >>> b/arch/arm/mach-rockchip/boot_mode.c >>> index 7b3cbc5..bff1cbc 100644 >>> --- a/arch/arm/mach-rockchip/boot_mode.c >>> +++ b/arch/arm/mach-rockchip/boot_mode.c >>> @@ -8,11 +8,36 @@ >>> #include <asm/io.h> >>> #include <asm/arch/boot_mode.h> >>> >>> +void set_back_to_bootrom_dnl_flag(void) >>> +{ >>> + writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG); >>> +} >>> + >>> +/* >>> + * some boards use a adc key, but some use gpio >>> + */ >>> +__weak int rockchip_dnl_key_pressed(void) >> >> Can you please declare this in a header file and add a full comment as >> to what this does? > > > This function is only used in this file and can be override by other > boards. So I don't > think this is a necessary to declare this in a header file. The reason is that all exported functions should be declared somewhere. Otherwise the function can be used / overridden somewhere else without any argument checking. > Of course, I will add a full comment for it. >>> >>> +{ >>> + return false; >>> +} >>> + >>> +void rockchip_dnl_mode_check(void) >>> +{ >>> + if (rockchip_dnl_key_pressed()) { >>> + printf("download key pressed, enter download mode..."); >> >> should that be 'entering'? > > > Okay, should be 'entering' > >>> + set_back_to_bootrom_dnl_flag(); >>> + do_reset(NULL, 0, 0, NULL); >>> + } >>> +} >>> + >>> int setup_boot_mode(void) >>> { >>> void* reg = (void *)CONFIG_ROCKCHIP_BOOT_MODE_REG; >>> - int boot_mode = readl(reg); >>> + int boot_mode; >>> + >>> + rockchip_dnl_mode_check(); >>> >>> + boot_mode = readl(reg); >>> debug("boot mode %x.\n", boot_mode); >>> >>> /* Clear boot mode */ >>> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c >>> b/board/rockchip/evb_rk3399/evb-rk3399.c >>> index d50c59d..738d942 100644 >>> --- a/board/rockchip/evb_rk3399/evb-rk3399.c >>> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c >>> @@ -4,6 +4,7 @@ >>> * SPDX-License-Identifier: GPL-2.0+ >>> */ >>> #include <common.h> >>> +#include <adc.h> >>> #include <dm.h> >>> #include <ram.h> >>> #include <dm/pinctrl.h> >>> @@ -13,6 +14,33 @@ >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> +#define KEY_DOWN_MIN_VAL 1 >>> +#define KEY_DOWN_MAX_VAL 20 >>> + >>> +int rockchip_dnl_key_pressed(void) >>> +{ >>> + unsigned int ret; >>> + unsigned int i; >>> + unsigned int val; >>> + int cnt = 0; >>> + >> >> Please document what is going on here - why the loop? What is the >> check against min/max value? > > > This if for voltage debounce, according to my experience on the rk3399 > evb board, in the 10 adc sample results, > we will met 2 ~ 4 result vals are out of the MIN~MAX range. OK great, please add a comment here about this. > >> >>> + for (i = 0; i < 10; i++) { >>> + ret = adc_channel_single_shot("saradc", 1, &val); >>> + if (ret) { >>> + printf("%s adc_channel_single_shot fail!\n", >>> __func__); >>> + break; >>> + } >>> + >>> + if ((val >= KEY_DOWN_MIN_VAL) && (val <= >>> KEY_DOWN_MAX_VAL)) >>> + cnt++; >>> + } >>> + >>> + if (cnt >= 8) >>> + return true; >>> + else >>> + return false; >>> +} >>> + >>> int board_init(void) >>> { >>> struct udevice *pinctrl, *regulator; >>> -- >>> 2.7.4Regards, Simon
diff --git a/arch/arm/mach-rockchip/boot_mode.c b/arch/arm/mach-rockchip/boot_mode.c index 7b3cbc5..bff1cbc 100644 --- a/arch/arm/mach-rockchip/boot_mode.c +++ b/arch/arm/mach-rockchip/boot_mode.c @@ -8,11 +8,36 @@ #include <asm/io.h> #include <asm/arch/boot_mode.h> +void set_back_to_bootrom_dnl_flag(void) +{ + writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG); +} + +/* + * some boards use a adc key, but some use gpio + */ +__weak int rockchip_dnl_key_pressed(void) +{ + return false; +} + +void rockchip_dnl_mode_check(void) +{ + if (rockchip_dnl_key_pressed()) { + printf("download key pressed, enter download mode..."); + set_back_to_bootrom_dnl_flag(); + do_reset(NULL, 0, 0, NULL); + } +} + int setup_boot_mode(void) { void* reg = (void *)CONFIG_ROCKCHIP_BOOT_MODE_REG; - int boot_mode = readl(reg); + int boot_mode; + + rockchip_dnl_mode_check(); + boot_mode = readl(reg); debug("boot mode %x.\n", boot_mode); /* Clear boot mode */ diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c index d50c59d..738d942 100644 --- a/board/rockchip/evb_rk3399/evb-rk3399.c +++ b/board/rockchip/evb_rk3399/evb-rk3399.c @@ -4,6 +4,7 @@ * SPDX-License-Identifier: GPL-2.0+ */ #include <common.h> +#include <adc.h> #include <dm.h> #include <ram.h> #include <dm/pinctrl.h> @@ -13,6 +14,33 @@ DECLARE_GLOBAL_DATA_PTR; +#define KEY_DOWN_MIN_VAL 1 +#define KEY_DOWN_MAX_VAL 20 + +int rockchip_dnl_key_pressed(void) +{ + unsigned int ret; + unsigned int i; + unsigned int val; + int cnt = 0; + + for (i = 0; i < 10; i++) { + ret = adc_channel_single_shot("saradc", 1, &val); + if (ret) { + printf("%s adc_channel_single_shot fail!\n", __func__); + break; + } + + if ((val >= KEY_DOWN_MIN_VAL) && (val <= KEY_DOWN_MAX_VAL)) + cnt++; + } + + if (cnt >= 8) + return true; + else + return false; +} + int board_init(void) { struct udevice *pinctrl, *regulator;
Enter download mode if the download key pressed. Signed-off-by: Andy Yan <andy.yan@rock-chips.com> --- arch/arm/mach-rockchip/boot_mode.c | 27 ++++++++++++++++++++++++++- board/rockchip/evb_rk3399/evb-rk3399.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-)