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

[Simon Glass]

Hi Andy,

On 13 September 2017 at 00:14, Andy Yan <andy.yan at 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 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.

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