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

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Ravi,

> Hi Lukasz
> 
> >> Adding support functions to run dfu spl commands.
> >> 
> >> Signed-off-by: Ravi Babu <ravibabu@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@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.

But is it necessary to store "dfu_alt_info" env variable to memory
medium (e.g. eMMC) or is it enough to use the default value stored in
RAM?

> 
> >
> >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 
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group