[U-Boot] [RFC PATCH v2 3/5] spl: dfu: adding dfu support functions for SPL-DFU

[B, Ravi]

Hi Lukasz

>> Adding support functions to run dfu spl commands.
>> 
>> Signed-off-by: Ravi Babu <ravibabu at ti.com>
>> ---
>>  common/spl/Makefile  |    1 +
>>  common/spl/spl_dfu.c |   57
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/spl.h        |    8 +++++++ 3 files changed, 66 insertions(+)
>>  create mode 100644 common/spl/spl_dfu.c
>> 
>> diff --git a/common/spl/Makefile b/common/spl/Makefile index 
>> 2e0f695..0850da0 100644
>> --- a/common/spl/Makefile
>> +++ b/common/spl/Makefile
>> @@ -21,4 +21,5 @@ obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
>>  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
>>  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
>>  obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>> +obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>>  endif
>> diff --git a/common/spl/spl_dfu.c b/common/spl/spl_dfu.c new file mode 
>> 100644 index 0000000..e8d0ba1
>> --- /dev/null
>> +++ b/common/spl/spl_dfu.c
>> @@ -0,0 +1,57 @@
>> +/*
>> + * (C) Copyright 2016
>> + * Texas Instruments, <www.ti.com>
>> + *
>> + * Ravi B <ravibabu at ti.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +#include <common.h>
>> +#include <spl.h>
>> +#include <linux/compiler.h>
>> +#include <errno.h>
>> +#include <watchdog.h>
>> +#include <console.h>
>> +#include <g_dnl.h>
>> +#include <usb.h>
>> +#include <dfu.h>
>> +#include <environment.h>
>> +
>> +static int run_dfu(int usb_index, char *interface, char *devstring) {
>> +	int ret;
>> +
>> +	ret = dfu_init_env_entities(interface, devstring);
>> +	if (ret) {
>> +		dfu_free_entities();
>> +		goto exit;
>> +	}
>> +
>> +	run_usb_dnl_gadget(usb_index, "usb_dnl_dfu");
>> +exit:
>> +	dfu_free_entities();
>> +	return ret;
>> +}
>> +
>> +int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, 
>				     ^^^^^^^^^^^^^^ this is a bit
>				     misleading.
>				     I would rename it to "alt_info_str"

Ok. Will change to alt_info_str.

>
>> char *devstr) +{
>> +	char *str_env;
>> +	int ret;
>> +
>> +	/* set default environment */
>> +	set_default_env(0);

>set_default_env() accepts const char *s as the argument. Please replace
>0 -> NULL

Fine.

>
>> +	str_env = getenv(dfu_alt_info);
>> +	if (!str_env) {
>> +		error("\"dfu_alt_info\" env variable not
>> defined!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = setenv("dfu_alt_info", str_env);
>> +	if (ret) {
>> +		error("unable to set env variable
>> \"dfu_alt_info\"!\n");
>> +		return -EINVAL;
>> +	}

>I'm a bit confused with this env flow.
>
>Is it required on your platform to:
>
>1. set_default_env(NULL) - which sets default envs (one which are mostly defined at ./include/<board_config> file) in RAM

Actually not required, I felt it is better to use default environment inorder to get the latest 
environment from SPL which is being loaded. I will check once without set_default_env().

>
>2. call getenv with "dfu_alt_info_ram", which reads this value from RAM
>
>3. call setenv() to store already in ram available env variable to some medium?

This step is required, "dfu_alt_info" is one referred by dfu driver.

>
>If you want to store default envs in the medium (e.g. eMMC), I think that it would be easier to call "saveenv".

Ok. 

>Otherwise, I would stay with default envs, since we only use this u-boot for flashing other binaries.

Right.

Regards
Ravi