From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lukasz Majewski <l.majewski@samsung.com>
Date: Wed, 04 Jul 2012 10:56:10 +0200
Subject: [U-Boot] [PATCH 3/7] dfu: DFU backend implementation
In-Reply-To: <201207032328.19267.marex@denx.de>
References: <1341308291-14663-1-git-send-email-l.majewski@samsung.com>
	<1341308291-14663-4-git-send-email-l.majewski@samsung.com>
	<201207032328.19267.marex@denx.de>
Message-ID: <20120704105610.25deee27@lmajewski.digital.local>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Marek,

> Dear Lukasz Majewski,
> 
> > New, separate driver at ./drivers/dfu has been added. It allows
> > platform and storage independent operation of DFU.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> > ---
> 
> [...]
> 
> > +#include <common.h>
> > +#include <malloc.h>
> > +#include <mmc.h>
> > +#include <fat.h>
> > +#include <dfu.h>
> > +#include <linux/list.h>
> > +#include <linux/compiler.h>
> > +
> > +static LIST_HEAD(dfu_list);
> > +static int dfu_alt_num;
> > +
> > +static int dfu_find_alt_num(char *s)
> > +{
> > +	int i = 0;
> > +
> > +	for (; *s; s++)
> > +		if (*s == ';')
> > +			i++;
> > +
> > +	return ++i;
> > +}
> > +
> > +static char *dfu_extract_entity(char** env)
> > +{
> > +	char *s = *env;
> > +
> > +	strsep(env, ";");
> > +	return s;
> > +}
> 
> Shall we not make these all generic? They seem to be quite helpful
> components.

It is a good topic for a discussion if those functions shall be moved
to ./lib/string.c. 
I regarded them as a "little" helper functions for parsing DFU alt
setting env variable. Those are very short and build with methods
exported from string.c

> 
> > +
> > +char *dfu_extract_token(char** e, int *n)
> > +{
> > +	char *st = *e;
> > +
> > +	debug("%s: %s\n", __func__, st);
> > +
> > +	strsep(e, " ");
> > +	*n = *e - st;
> > +
> > +	return st;
> > +}
> > +
> > +static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
> > +				     dfu_buf[DFU_DATA_BUF_SIZE];
> 
> Can we not stack-allocate it with ALLOC_CACHE_ALIGN_BUFFER()?

The dfu_buf is 4 MiB (this is the size of DFU_DATA_BUF_SIZE). I don't
think, that allocating it on the stack (for stack allocation the
ALLOC_CACHE_ALIGN_BUFFER() is designed) is a good idea.

> 
> [...]
> 
> > diff --git a/include/dfu.h b/include/dfu.h
> > new file mode 100644
> > index 0000000..f7d823b
> > --- /dev/null
> > +++ b/include/dfu.h
> 
> [...]
> 
> > +struct dfu_entity {
> > +	char			name[DFU_NAME_SIZE];
> > +	int                     alt;
> > +	void                    *dev_private;
> > +	int                     dev_num;
> > +	enum dfu_device_type    dev_type;
> > +	enum dfu_layout         layout;
> > +
> > +	union {
> > +		struct mmc_internal_data mmc;
> 
> This union seems redundant ;-)

Good point :-), but I predict, that DFU will be used to program other
memory types (OneNAND, or NAND). To support those, one needs to extend
the union with e.g struct onenand_internal_data onenand.

Since we don't have so many memory types, I think that union usage is
acceptable. 

> 
> [...]



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group