| Message ID | 5eb509735b5bd634043bbb8f56d088073773b7b8.1379055112.git.afzal.mohd.ma@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Afzal, > DFU spec mentions it as a method to upgrade firmware (software stored > in writable non-volatile memory). It also says other potential uses of > DFU is beyond scope of the spec. > > Here such a beyond the scope use is being attempted - directly pumping > binary images from host via USB to RAM. This facility is a developer > centric one in that it gives advantage over upgrading non-volatile > memory for testing new images every time during development and/or > testing. > > Directly putting image onto RAM would speed up upgrade process. This > and convenience was the initial thoughts that led to doing this, speed > improvement over MMC was only 1 second though - 6 sec on RAM as > opposed to 7 sec on MMC in beagle bone, perhaps enabling cache and/or > optimizing DFU framework to avoid multiple copy for ram (if worth) > may help, and on other platforms and other boot media like NAND maybe > improvement would be higher. > > And for a platform that doesn't yet have proper DFU suppport for > non-volatile media's, DFU to RAM can be used. > > Another minor advantage would be to increase life of mmc/nand as it > would be less used during development/testing. > > usage: <image name> ram <start address> <size> > eg. kernel ram 0x81000000 0x1000000 > > Downloading images to RAM using DFU is not something new, this is > acheived in openmoko also. > > DFU on RAM can be used for extracting RAM contents to host using dfu > upload. Perhaps this can be extended to io for squeezing out register > dump through usb, if it is worth. > > Signed-off-by: Afzal Mohammed <afzal.mohd.ma@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > > v3: error used instead of printf > v2: remove read/write enumerator define's, instead use new common ones Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Dear Afzal Mohammed, [...] > +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) > +{ > + char *st; > + > + dfu->dev_type = DFU_DEV_RAM; > + st = strsep(&s, " "); > + if (strcmp(st, "ram")) { > + error("unsupported device: %s\n", st); > + return -ENODEV; > + } > + > + dfu->layout = DFU_RAM_ADDR; > + dfu->data.ram.start = (void *)simple_strtoul(s, &s, 16); > + dfu->data.ram.size = simple_strtoul(++s, &s, 16); This ++s, &s is quite crazy ;-) Otheriwse Acked-by: Marek Vasut <marex@denx.de> Best regards, Marek Vasut
On Fri, Sep 13, 2013 at 15:51 +0200, Marek Vasut wrote: > > Dear Afzal Mohammed, > > [...] > > > +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) > > +{ > > + char *st; > > + > > + dfu->dev_type = DFU_DEV_RAM; > > + st = strsep(&s, " "); > > + if (strcmp(st, "ram")) { > > + error("unsupported device: %s\n", st); > > + return -ENODEV; > > + } > > + > > + dfu->layout = DFU_RAM_ADDR; > > + dfu->data.ram.start = (void *)simple_strtoul(s, &s, 16); > > + dfu->data.ram.size = simple_strtoul(++s, &s, 16); > > This ++s, &s is quite crazy ;-) Actually it's not just a neat trick, but instead it's not guaranteed to work. It's an inviation for problems that you don't need. The order in which arguments for the call get evaluated isn't specified, so behaviour is uncertain here. Read: the construction happens to work by coincidence for individual setups, but isn't correct and need not work elsewhere, and may break in arbitrary ways at any occasion, and then is hard to track down since things may "seem to work" often enough or the bug won't show up when you are diagnosing the problem. The code needs to get fixed before getting picked up. Doing the increment in a separate instruction is cheap and simple, the terse form is wrong. virtually yours Gerhard Sittig
Hi Gerhard, > On Fri, Sep 13, 2013 at 15:51 +0200, Marek Vasut wrote: > > > > Dear Afzal Mohammed, > > > > [...] > > > > > +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) > > > +{ > > > + char *st; > > > + > > > + dfu->dev_type = DFU_DEV_RAM; > > > + st = strsep(&s, " "); > > > + if (strcmp(st, "ram")) { > > > + error("unsupported device: %s\n", st); > > > + return -ENODEV; > > > + } > > > + > > > + dfu->layout = DFU_RAM_ADDR; > > > + dfu->data.ram.start = (void *)simple_strtoul(s, &s, 16); > > > + dfu->data.ram.size = simple_strtoul(++s, &s, 16); > > > > This ++s, &s is quite crazy ;-) > > Actually it's not just a neat trick, but instead it's not > guaranteed to work. It's an inviation for problems that you > don't need. > > The order in which arguments for the call get evaluated isn't > specified, so behaviour is uncertain here. Read: the > construction happens to work by coincidence for individual > setups, but isn't correct and need not work elsewhere, and may > break in arbitrary ways at any occasion, and then is hard to > track down since things may "seem to work" often enough or the > bug won't show up when you are diagnosing the problem. > > The code needs to get fixed before getting picked up. Doing the > increment in a separate instruction is cheap and simple, the > terse form is wrong. It was me, who by some obscure reason introduced this faulty (non standard) code. And probably Afzal copied it from dfu_mmc.c file. So f...g embarrassing... However the fix for that code at dfu_mmc.c file was already posted to ML. > > > virtually yours > Gerhard Sittig
Dear Gerhard Sittig, > On Fri, Sep 13, 2013 at 15:51 +0200, Marek Vasut wrote: > > Dear Afzal Mohammed, > > > > [...] > > > > > +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) > > > +{ > > > + char *st; > > > + > > > + dfu->dev_type = DFU_DEV_RAM; > > > + st = strsep(&s, " "); > > > + if (strcmp(st, "ram")) { > > > + error("unsupported device: %s\n", st); > > > + return -ENODEV; > > > + } > > > + > > > + dfu->layout = DFU_RAM_ADDR; > > > + dfu->data.ram.start = (void *)simple_strtoul(s, &s, 16); > > > + dfu->data.ram.size = simple_strtoul(++s, &s, 16); > > > > This ++s, &s is quite crazy ;-) > > Actually it's not just a neat trick, but instead it's not > guaranteed to work. It's an inviation for problems that you > don't need. > > The order in which arguments for the call get evaluated isn't > specified, so behaviour is uncertain here. Read: the > construction happens to work by coincidence for individual > setups, but isn't correct and need not work elsewhere, and may > break in arbitrary ways at any occasion, and then is hard to > track down since things may "seem to work" often enough or the > bug won't show up when you are diagnosing the problem. > > The code needs to get fixed before getting picked up. Doing the > increment in a separate instruction is cheap and simple, the > terse form is wrong. Full ACK. Best regards, Marek Vasut
Hi Gerhard, Marek, On Fri, Sep 13, 2013 at 05:37:36PM +0200, Gerhard Sittig wrote: > On Fri, Sep 13, 2013 at 15:51 +0200, Marek Vasut wrote: > > > + dfu->data.ram.size = simple_strtoul(++s, &s, 16); > > > > This ++s, &s is quite crazy ;-) > Actually it's not just a neat trick, but instead it's not > guaranteed to work. It's an inviation for problems that you > don't need. > > The order in which arguments for the call get evaluated isn't > specified, so behaviour is uncertain here. Read: the > construction happens to work by coincidence for individual > setups, but isn't correct and need not work elsewhere, and may > break in arbitrary ways at any occasion, and then is hard to > track down since things may "seem to work" often enough or the > bug won't show up when you are diagnosing the problem. > > The code needs to get fixed before getting picked up. Doing the > increment in a separate instruction is cheap and simple, the > terse form is wrong. Yes wrong me, I realize that result is compiler dependent and not guaranteed to work always, v4 posted taking care of the above. Regards Afzal
+ Gerhard On Fri, Sep 13, 2013 at 10:35:45PM +0530, Afzal Mohammed wrote: > Hi Gerhard, Marek, > > On Fri, Sep 13, 2013 at 05:37:36PM +0200, Gerhard Sittig wrote: > > On Fri, Sep 13, 2013 at 15:51 +0200, Marek Vasut wrote: > > > > > + dfu->data.ram.size = simple_strtoul(++s, &s, 16); > > > > > > This ++s, &s is quite crazy ;-) > > > Actually it's not just a neat trick, but instead it's not > > guaranteed to work. It's an inviation for problems that you > > don't need. > > > > The order in which arguments for the call get evaluated isn't > > specified, so behaviour is uncertain here. Read: the > > construction happens to work by coincidence for individual > > setups, but isn't correct and need not work elsewhere, and may > > break in arbitrary ways at any occasion, and then is hard to > > track down since things may "seem to work" often enough or the > > bug won't show up when you are diagnosing the problem. > > > > The code needs to get fixed before getting picked up. Doing the > > increment in a separate instruction is cheap and simple, the > > terse form is wrong. > > Yes wrong me, I realize that result is compiler dependent and not > guaranteed to work always, v4 posted taking care of the above. > > Regards > Afzal
diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile index fca370a..de9e44e 100644 --- a/drivers/dfu/Makefile +++ b/drivers/dfu/Makefile @@ -12,6 +12,7 @@ LIB = $(obj)libdfu.o COBJS-$(CONFIG_DFU_FUNCTION) += dfu.o COBJS-$(CONFIG_DFU_MMC) += dfu_mmc.o COBJS-$(CONFIG_DFU_NAND) += dfu_nand.o +COBJS-$(CONFIG_DFU_RAM) += dfu_ram.o SRCS := $(COBJS-y:.o=.c) OBJS := $(addprefix $(obj),$(COBJS-y)) diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 689f5db..56b21c7 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -348,6 +348,9 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt, } else if (strcmp(interface, "nand") == 0) { if (dfu_fill_entity_nand(dfu, s)) return -1; + } else if (strcmp(interface, "ram") == 0) { + if (dfu_fill_entity_ram(dfu, s)) + return -1; } else { printf("%s: Device %s not (yet) supported!\n", __func__, interface); @@ -397,14 +400,14 @@ int dfu_config_entities(char *env, char *interface, int num) const char *dfu_get_dev_type(enum dfu_device_type t) { - const char *dev_t[] = {NULL, "eMMC", "OneNAND", "NAND" }; + const char *dev_t[] = {NULL, "eMMC", "OneNAND", "NAND", "RAM" }; return dev_t[t]; } const char *dfu_get_layout(enum dfu_layout l) { const char *dfu_layout[] = {NULL, "RAW_ADDR", "FAT", "EXT2", - "EXT3", "EXT4" }; + "EXT3", "EXT4", "RAM_ADDR" }; return dfu_layout[l]; } diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c new file mode 100644 index 0000000..2e3520c --- /dev/null +++ b/drivers/dfu/dfu_ram.c @@ -0,0 +1,76 @@ +/* + * (C) Copyright 2013 + * Afzal Mohammed <afzal.mohd.ma@gmail.com> + * + * Reference: dfu_mmc.c + * Copyright (C) 2012 Samsung Electronics + * author: Lukasz Majewski <l.majewski@samsung.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <malloc.h> +#include <errno.h> +#include <dfu.h> + +static int dfu_transfer_medium_ram(enum dfu_op op, struct dfu_entity *dfu, + u64 offset, void *buf, long *len) +{ + if (dfu->layout != DFU_RAM_ADDR) { + error("unsupported layout: %s\n", dfu_get_layout(dfu->layout)); + return -EINVAL; + } + + if (offset > dfu->data.ram.size) { + error("request exceeds allowed area\n"); + return -EINVAL; + } + + if (op == DFU_OP_WRITE) + memcpy(dfu->data.ram.start + offset, buf, *len); + else + memcpy(buf, dfu->data.ram.start + offset, *len); + + return 0; +} + +static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset, + void *buf, long *len) +{ + return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len); +} + +static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset, + void *buf, long *len) +{ + if (!*len) { + *len = dfu->data.ram.size; + return 0; + } + + return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset, buf, len); +} + +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) +{ + char *st; + + dfu->dev_type = DFU_DEV_RAM; + st = strsep(&s, " "); + if (strcmp(st, "ram")) { + error("unsupported device: %s\n", st); + return -ENODEV; + } + + dfu->layout = DFU_RAM_ADDR; + dfu->data.ram.start = (void *)simple_strtoul(s, &s, 16); + dfu->data.ram.size = simple_strtoul(++s, &s, 16); + + dfu->write_medium = dfu_write_medium_ram; + dfu->read_medium = dfu_read_medium_ram; + + dfu->inited = 0; + + return 0; +} diff --git a/include/dfu.h b/include/dfu.h index 6a3e253..6f4bba4 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -19,6 +19,7 @@ enum dfu_device_type { DFU_DEV_MMC = 1, DFU_DEV_ONENAND, DFU_DEV_NAND, + DFU_DEV_RAM, }; enum dfu_layout { @@ -27,6 +28,7 @@ enum dfu_layout { DFU_FS_EXT2, DFU_FS_EXT3, DFU_FS_EXT4, + DFU_RAM_ADDR, }; enum dfu_op { @@ -56,6 +58,11 @@ struct nand_internal_data { unsigned int ubi; }; +struct ram_internal_data { + void *start; + unsigned int size; +}; + static inline unsigned int get_mmc_blk_size(int dev) { return find_mmc_device(dev)->read_bl_len; @@ -81,6 +88,7 @@ struct dfu_entity { union { struct mmc_internal_data mmc; struct nand_internal_data nand; + struct ram_internal_data ram; } data; int (*read_medium)(struct dfu_entity *dfu, @@ -143,4 +151,14 @@ static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *s) } #endif +#ifdef CONFIG_DFU_RAM +extern int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s); +#else +static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s) +{ + puts("RAM support not available!\n"); + return -1; +} +#endif + #endif /* __DFU_ENTITY_H_ */
DFU spec mentions it as a method to upgrade firmware (software stored in writable non-volatile memory). It also says other potential uses of DFU is beyond scope of the spec. Here such a beyond the scope use is being attempted - directly pumping binary images from host via USB to RAM. This facility is a developer centric one in that it gives advantage over upgrading non-volatile memory for testing new images every time during development and/or testing. Directly putting image onto RAM would speed up upgrade process. This and convenience was the initial thoughts that led to doing this, speed improvement over MMC was only 1 second though - 6 sec on RAM as opposed to 7 sec on MMC in beagle bone, perhaps enabling cache and/or optimizing DFU framework to avoid multiple copy for ram (if worth) may help, and on other platforms and other boot media like NAND maybe improvement would be higher. And for a platform that doesn't yet have proper DFU suppport for non-volatile media's, DFU to RAM can be used. Another minor advantage would be to increase life of mmc/nand as it would be less used during development/testing. usage: <image name> ram <start address> <size> eg. kernel ram 0x81000000 0x1000000 Downloading images to RAM using DFU is not something new, this is acheived in openmoko also. DFU on RAM can be used for extracting RAM contents to host using dfu upload. Perhaps this can be extended to io for squeezing out register dump through usb, if it is worth. Signed-off-by: Afzal Mohammed <afzal.mohd.ma@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Lukasz Majewski <l.majewski@samsung.com> Cc: Pantelis Antoniou <panto@antoniou-consulting.com> --- v3: error used instead of printf v2: remove read/write enumerator define's, instead use new common ones drivers/dfu/Makefile | 1 + drivers/dfu/dfu.c | 7 +++-- drivers/dfu/dfu_ram.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dfu.h | 18 ++++++++++++ 4 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 drivers/dfu/dfu_ram.c