[U-Boot] [PATCH 3/3] rockchip: check download key before bootup

[Andy Yan]

Hi Simon:


On 2017年09月13日 12:26, Simon Glass wrote:
> Hi Andy,
>
> On 12 September 2017 at 07:58, Andy Yan <andy.yan at rock-chips.com> wrote:
>> Enter download mode if the download key pressed.
>>
>> Signed-off-by: Andy Yan <andy.yan at 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
>
>
>