Re: [U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
Dear Mike Frysinger,
On Thursday 02 August 2012 09:55:24 Lukasz Majewski wrote:
Dear Mike Frysinger,
On Tuesday 31 July 2012 02:36:59 Lukasz Majewski wrote:
+{
+ int i = 0;
+
+ for (; *s; s++)
+ if (*s == ';')
+ i++;
+
+ return ++i;
+}
In this function I count how many times the ';' separator appears.
well, to be more specific, i think it's counting the number of elements if
you were to split on ';'. so if there was one ';', this would return 2.
but you're correct of course that my suggested replacement is not
equivalent.
+int dfu_config_entities(char *env, char *interface, int num)
+{
+ struct dfu_entity *dfu;
+ int i, ret;
+ char *s;
+
+ dfu_alt_num = dfu_find_alt_num(env);
+ debug(%s: dfu_alt_num=%d\n, __func__, dfu_alt_num);
+
+ for (i = 0; i dfu_alt_num; i++) {
+ dfu = calloc(sizeof(struct dfu_entity), 1);
seems like you can do this in a single call outside of the for loop:
dfu = calloc(sizeof(*dfu), dfu_alt_num);
if (!dfu)
return -1;
for (i = 0; i dfu_alt_num; i++) {
s = strsep(env, ;);
ret = dfu_fill_entity(dfu[i], s, i, interface, num);
if (ret)
return -1;
list_add_tail(dfu[i].list, dfu_list);
}
I'd prefer to leave it as it is (since IMHO is more readable) with the
addition of following code:
it might be more slightly more readable, but doing a single function call
results in better runtime performance
Doesn't the compiler optimize it as it sees fit?
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
On Saturday 04 August 2012 03:47:34 Marek Vasut wrote:
Dear Mike Frysinger,
On Thursday 02 August 2012 09:55:24 Lukasz Majewski wrote:
Dear Mike Frysinger,
On Tuesday 31 July 2012 02:36:59 Lukasz Majewski wrote:
+int dfu_config_entities(char *env, char *interface, int num)
+{
+ struct dfu_entity *dfu;
+ int i, ret;
+ char *s;
+
+ dfu_alt_num = dfu_find_alt_num(env);
+ debug(%s: dfu_alt_num=%d\n, __func__, dfu_alt_num);
+
+ for (i = 0; i dfu_alt_num; i++) {
+ dfu = calloc(sizeof(struct dfu_entity), 1);
seems like you can do this in a single call outside of the for loop:
dfu = calloc(sizeof(*dfu), dfu_alt_num);
if (!dfu)
return -1;
for (i = 0; i dfu_alt_num; i++) {
s = strsep(env, ;);
ret = dfu_fill_entity(dfu[i], s, i, interface, num);
if (ret)
return -1;
list_add_tail(dfu[i].list, dfu_list);
}
I'd prefer to leave it as it is (since IMHO is more readable) with the
addition of following code:
it might be more slightly more readable, but doing a single function call
results in better runtime performance
Doesn't the compiler optimize it as it sees fit?
gcc can't know to optimize malloc calls like this
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
On Thursday 02 August 2012 09:55:24 Lukasz Majewski wrote:
Dear Mike Frysinger,
On Tuesday 31 July 2012 02:36:59 Lukasz Majewski wrote:
+{
+ int i = 0;
+
+ for (; *s; s++)
+ if (*s == ';')
+ i++;
+
+ return ++i;
+}
In this function I count how many times the ';' separator appears.
well, to be more specific, i think it's counting the number of elements if you
were to split on ';'. so if there was one ';', this would return 2. but
you're correct of course that my suggested replacement is not equivalent.
+int dfu_config_entities(char *env, char *interface, int num)
+{
+ struct dfu_entity *dfu;
+ int i, ret;
+ char *s;
+
+ dfu_alt_num = dfu_find_alt_num(env);
+ debug(%s: dfu_alt_num=%d\n, __func__, dfu_alt_num);
+
+ for (i = 0; i dfu_alt_num; i++) {
+ dfu = calloc(sizeof(struct dfu_entity), 1);
seems like you can do this in a single call outside of the for loop:
dfu = calloc(sizeof(*dfu), dfu_alt_num);
if (!dfu)
return -1;
for (i = 0; i dfu_alt_num; i++) {
s = strsep(env, ;);
ret = dfu_fill_entity(dfu[i], s, i, interface, num);
if (ret)
return -1;
list_add_tail(dfu[i].list, dfu_list);
}
I'd prefer to leave it as it is (since IMHO is more readable) with the
addition of following code:
it might be more slightly more readable, but doing a single function call
results in better runtime performance
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
Dear Mike Frysinger,
On Tuesday 31 July 2012 02:36:59 Lukasz Majewski wrote:
--- /dev/null
+++ b/drivers/dfu/dfu.c
+static int dfu_find_alt_num(char *s)
const char *s
Good point.
+{
+ int i = 0;
+
+ for (; *s; s++)
+ if (*s == ';')
+ i++;
+
+ return ++i;
+}
In this function I count how many times the ';' separator appears.
I didn't found proper function at ./lib/string.c.
looks kind of like:
return (strrchr(s, ';') - s) + 1;
The above code returns position of the last occurrence of the ';'
separator.
+int dfu_write(struct dfu_entity *dfu, void *buf, int size, int
blk_seq_num) +{
+ static unsigned char *i_buf;
+ static int i_blk_seq_num;
+ long w_size = 0;
+ int ret = 0;
+
+ if (blk_seq_num == 0) {
+ memset(dfu_buf, '\0', sizeof(dfu_buf));
...
+ memcpy(i_buf, buf, size);
+ i_buf += size;
why bother clearing it ? since right below we memcpy() in the data
we care about from buf, i'd skip the memset() completely.
You are right, removed.
+int dfu_read(struct dfu_entity *dfu, void *buf, int size, int
blk_seq_num) +{
+ static unsigned char *i_buf;
+ static int i_blk_seq_num;
+ static long r_size;
+ static u32 crc;
+ int ret = 0;
+
+ if (blk_seq_num == 0) {
+ i_buf = dfu_buf;
+ memset(dfu_buf, '\0', sizeof(dfu_buf));
+ ret = dfu-read_medium(dfu, i_buf, r_size);
+ debug(%s: %s %ld [B]\n, __func__, dfu-name,
r_size);
+ i_blk_seq_num = 0;
+ /* Integrity check (if needed) */
+ crc = crc32(0, dfu_buf, r_size);
+ }
same here -- punt the memset()
OK,
+static int dfu_fill_entity(struct dfu_entity *dfu, char* s, int
alt,
char *s, not char* s
OK,
+int dfu_config_entities(char *env, char *interface, int num)
+{
+ struct dfu_entity *dfu;
+ int i, ret;
+ char *s;
+
+ dfu_alt_num = dfu_find_alt_num(env);
+ debug(%s: dfu_alt_num=%d\n, __func__, dfu_alt_num);
+
+ for (i = 0; i dfu_alt_num; i++) {
+ dfu = calloc(sizeof(struct dfu_entity), 1);
seems like you can do this in a single call outside of the for loop:
dfu = calloc(sizeof(*dfu), dfu_alt_num);
if (!dfu)
return -1;
for (i = 0; i dfu_alt_num; i++) {
s = strsep(env, ;);
ret = dfu_fill_entity(dfu[i], s, i, interface, num);
if (ret)
return -1;
list_add_tail(dfu[i].list, dfu_list);
}
I'd prefer to leave it as it is (since IMHO is more readable) with the
addition of following code:
for (i = 0; i dfu_alt_num; i++) {
dfu = calloc(sizeof(struct dfu_entity), 1);
if (!dfu) {
dfu_free_entities();
return -1;
}
}
--- /dev/null
+++ b/include/dfu.h
+char *dfu_extract_token(char** e, int *n);
+extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s);
+static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu,
char* s)
char *s, not char* s
OK
-mike
--
Best regards,
Lukasz Majewski
Samsung Poland RD Center | Linux Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
On Tuesday 31 July 2012 02:36:59 Lukasz Majewski wrote:
--- /dev/null
+++ b/drivers/dfu/dfu.c
+static int dfu_find_alt_num(char *s)
const char *s
+{
+ int i = 0;
+
+ for (; *s; s++)
+ if (*s == ';')
+ i++;
+
+ return ++i;
+}
looks kind of like:
return (strrchr(s, ';') - s) + 1;
+int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
+{
+ static unsigned char *i_buf;
+ static int i_blk_seq_num;
+ long w_size = 0;
+ int ret = 0;
+
+ if (blk_seq_num == 0) {
+ memset(dfu_buf, '\0', sizeof(dfu_buf));
...
+ memcpy(i_buf, buf, size);
+ i_buf += size;
why bother clearing it ? since right below we memcpy() in the data we care
about from buf, i'd skip the memset() completely.
+int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
+{
+ static unsigned char *i_buf;
+ static int i_blk_seq_num;
+ static long r_size;
+ static u32 crc;
+ int ret = 0;
+
+ if (blk_seq_num == 0) {
+ i_buf = dfu_buf;
+ memset(dfu_buf, '\0', sizeof(dfu_buf));
+ ret = dfu-read_medium(dfu, i_buf, r_size);
+ debug(%s: %s %ld [B]\n, __func__, dfu-name, r_size);
+ i_blk_seq_num = 0;
+ /* Integrity check (if needed) */
+ crc = crc32(0, dfu_buf, r_size);
+ }
same here -- punt the memset()
+static int dfu_fill_entity(struct dfu_entity *dfu, char* s, int alt,
char *s, not char* s
+int dfu_config_entities(char *env, char *interface, int num)
+{
+ struct dfu_entity *dfu;
+ int i, ret;
+ char *s;
+
+ dfu_alt_num = dfu_find_alt_num(env);
+ debug(%s: dfu_alt_num=%d\n, __func__, dfu_alt_num);
+
+ for (i = 0; i dfu_alt_num; i++) {
+ dfu = calloc(sizeof(struct dfu_entity), 1);
seems like you can do this in a single call outside of the for loop:
dfu = calloc(sizeof(*dfu), dfu_alt_num);
if (!dfu)
return -1;
for (i = 0; i dfu_alt_num; i++) {
s = strsep(env, ;);
ret = dfu_fill_entity(dfu[i], s, i, interface, num);
if (ret)
return -1;
list_add_tail(dfu[i].list, dfu_list);
}
--- /dev/null
+++ b/include/dfu.h
+char *dfu_extract_token(char** e, int *n);
+extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s);
+static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s)
char *s, not char* s
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
New, separate driver at ./drivers/dfu has been added. It allows platform
and storage independent operation of DFU.
It has been extended to use new MMC level of command abstraction.
Signed-off-by: Lukasz Majewski l.majew...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Marek Vasut ma...@denx.de
---
Changes for v2:
- None
Changes for v3:
- Remove unnecessary NULL and 0 initialization of dynamic variables
- Combine two puts to one
- Adding const qualifier to device and layout definitions
- Remove unnecessary casting at dfu-name passing
- Provide more meaningful names for dfu layouts and device types
- Removal of dfu_extract_{token|entity} functions and replace them
with strsep calls
---
Makefile |1 +
drivers/dfu/Makefile | 43 +
drivers/dfu/dfu.c| 237 ++
include/dfu.h| 103 ++
4 files changed, 384 insertions(+), 0 deletions(-)
create mode 100644 drivers/dfu/Makefile
create mode 100644 drivers/dfu/dfu.c
create mode 100644 include/dfu.h
diff --git a/Makefile b/Makefile
index d57c15e..bd469f4 100644
--- a/Makefile
+++ b/Makefile
@@ -271,6 +271,7 @@ LIBS += drivers/pci/libpci.o
LIBS += drivers/pcmcia/libpcmcia.o
LIBS += drivers/power/libpower.o
LIBS += drivers/spi/libspi.o
+LIBS += drivers/dfu/libdfu.o
ifeq ($(CPU),mpc83xx)
LIBS += drivers/qe/libqe.o
LIBS += arch/powerpc/cpu/mpc8xxx/ddr/libddr.o
diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile
new file mode 100644
index 000..7736485
--- /dev/null
+++ b/drivers/dfu/Makefile
@@ -0,0 +1,43 @@
+#
+# Copyright (C) 2012 Samsung Electronics
+# Lukasz Majewski l.majew...@samsung.com
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+
+include $(TOPDIR)/config.mk
+
+LIB= $(obj)libdfu.o
+
+COBJS-$(CONFIG_DFU_FUNCTION) += dfu.o
+
+SRCS:= $(COBJS-y:.o=.c)
+OBJS := $(addprefix $(obj),$(COBJS-y))
+
+$(LIB):$(obj).depend $(OBJS)
+ $(call cmd_link_o_target, $(OBJS))
+
+#
+
+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk
+
+sinclude $(obj).depend
+
+#
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
new file mode 100644
index 000..8f48b49
--- /dev/null
+++ b/drivers/dfu/dfu.c
@@ -0,0 +1,237 @@
+/*
+ * dfu.c -- DFU back-end routines
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * author: Lukasz Majewski l.majew...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#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 unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
+dfu_buf[DFU_DATA_BUF_SIZE];
+
+int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
+{
+ static unsigned char *i_buf;
+ static int i_blk_seq_num;
+ long w_size = 0;
+ int ret = 0;
+
+ debug(%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x i_buf: 0x%p\n,
+ __func__, dfu-name, buf, size, blk_seq_num, i_buf);
+
+ if (blk_seq_num == 0) {
+ memset(dfu_buf, '\0', sizeof(dfu_buf));
+ i_buf = dfu_buf;
+
