[U-Boot] [PATCH v2 0/3] spl: dfu: misc fixes and reduce MLO foot print
The patch series spl-dfu fixes includes - select spl-dfu only spl-ram supported - ignore the dfu-reset for spl-dfu - reduce the spl-dfu MLO foot print
buildman ran for arm targets
Change history:
v1: - Kconfig fix for dfu-reset for SPL-DFU - compile out CONFIG_DFU_MMC for SPL-DFU
Ravi Babu (3): spl: Kconfig: dfu: spl-dfu depends on SPL_RAM_SUPPORT common: dfu: ignore reset for spl-dfu spl: dfu: reduce spl-dfu MLO size
common/Makefile | 3 +-- common/dfu.c | 2 +- common/spl/Kconfig | 5 +++++ drivers/dfu/Makefile | 2 ++ drivers/dfu/dfu.c | 4 ++++ include/dfu.h | 2 +- 6 files changed, 14 insertions(+), 4 deletions(-)
Since SPL_DFU_SUPPORT is depends on SPL_RAM_SUPPORT, hence select SPL_DFU_SUPPORT only when SPL_RAM_SUPPORT is chosen.
Signed-off-by: Ravi Babu ravibabu@ti.com Reviewed-by: Tom Rini trini@konsulko.com --- common/spl/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index ea6fbb6..1231351 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -646,6 +646,7 @@ config SPL_USBETH_SUPPORT config SPL_DFU_SUPPORT bool "Support DFU (Device Firmware Upgarde)" select SPL_HASH_SUPPORT + depends on SPL_RAM_SUPPORT help This feature enables the DFU (Device Firmware Upgarde) in SPL with RAM memory device support. The ROM code will load and execute
The SPL-DFU feature enable to load and execute u-boot over usb from PC using dfu-util. Hence dfu-reset should not be issued when dfu-util -R switch is issued.
Signed-off-by: Ravi Babu ravibabu@ti.com --- common/dfu.c | 2 +- common/spl/Kconfig | 4 ++++ drivers/dfu/dfu.c | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..546a1ab 100644 --- a/common/dfu.c +++ b/common/dfu.c @@ -88,7 +88,7 @@ exit: board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
if (dfu_reset) - run_command("reset", 0); + do_reset(NULL, 0, 0, NULL);
g_dnl_clear_detach();
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 1231351..f51ae2c 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -6,6 +6,9 @@ config SUPPORT_SPL config SUPPORT_TPL bool
+config SPL_DFU_NO_RESET + bool + config SPL bool depends on SUPPORT_SPL @@ -646,6 +649,7 @@ config SPL_USBETH_SUPPORT config SPL_DFU_SUPPORT bool "Support DFU (Device Firmware Upgarde)" select SPL_HASH_SUPPORT + select SPL_DFU_NO_RESET depends on SPL_RAM_SUPPORT help This feature enables the DFU (Device Firmware Upgarde) in SPL with diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 8dacc1a..ceb33e3 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -35,7 +35,11 @@ static struct hash_algo *dfu_hash_algo; */ __weak bool dfu_usb_get_reset(void) { +#ifdef CONFIG_SPL_DFU_NO_RESET + return false; +#else return true; +#endif }
static int dfu_find_alt_num(const char *s)
On Thu, Apr 27, 2017 at 05:45:20PM +0530, Ravi Babu wrote:
The SPL-DFU feature enable to load and execute u-boot over usb from PC using dfu-util. Hence dfu-reset should not be issued when dfu-util -R switch is issued.
Signed-off-by: Ravi Babu ravibabu@ti.com
common/dfu.c | 2 +- common/spl/Kconfig | 4 ++++ drivers/dfu/dfu.c | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..546a1ab 100644 --- a/common/dfu.c +++ b/common/dfu.c @@ -88,7 +88,7 @@ exit: board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
if (dfu_reset)
run_command("reset", 0);
do_reset(NULL, 0, 0, NULL);g_dnl_clear_detach();
So this hunk drops out the need for cli stuff.
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 1231351..f51ae2c 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -6,6 +6,9 @@ config SUPPORT_SPL config SUPPORT_TPL bool
+config SPL_DFU_NO_RESET
- bool
config SPL bool depends on SUPPORT_SPL @@ -646,6 +649,7 @@ config SPL_USBETH_SUPPORT config SPL_DFU_SUPPORT bool "Support DFU (Device Firmware Upgarde)" select SPL_HASH_SUPPORT
- select SPL_DFU_NO_RESET depends on SPL_RAM_SUPPORT help This feature enables the DFU (Device Firmware Upgarde) in SPL with
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 8dacc1a..ceb33e3 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -35,7 +35,11 @@ static struct hash_algo *dfu_hash_algo; */ __weak bool dfu_usb_get_reset(void) { +#ifdef CONFIG_SPL_DFU_NO_RESET
- return false;
+#else return true; +#endif }
static int dfu_find_alt_num(const char *s)
So do we still need the above, in order to save space? How much are we saving here even, now? Thanks!
Hi Tom
diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..546a1ab 100644 --- a/common/dfu.c +++ b/common/dfu.c @@ -88,7 +88,7 @@ exit: board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
if (dfu_reset)
run_command("reset", 0);
do_reset(NULL, 0, 0, NULL);g_dnl_clear_detach();
So this hunk drops out the need for cli stuff.
Yes.
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 1231351..f51ae2c 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -6,6 +6,9 @@ config SUPPORT_SPL config SUPPORT_TPL bool
+config SPL_DFU_NO_RESET
- bool
config SPL bool depends on SUPPORT_SPL @@ -646,6 +649,7 @@ config SPL_USBETH_SUPPORT config SPL_DFU_SUPPORT bool "Support DFU (Device Firmware Upgarde)" select SPL_HASH_SUPPORT
- select SPL_DFU_NO_RESET depends on SPL_RAM_SUPPORT help This feature enables the DFU (Device Firmware Upgarde) in SPL with
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 8dacc1a..ceb33e3 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -35,7 +35,11 @@ static struct hash_algo *dfu_hash_algo; */ __weak bool dfu_usb_get_reset(void)
{
+#ifdef CONFIG_SPL_DFU_NO_RESET
- return false;
+#else return true; +#endif }
static int dfu_find_alt_num(const char *s)
So do we still need the above, in order to save space? How much are we saving here even, now? Thanks!
I observed around 7K reduced.
Regards Ravi
On Thu, Apr 27, 2017 at 05:24:09PM +0000, B, Ravi wrote:
Hi Tom
diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..546a1ab 100644 --- a/common/dfu.c +++ b/common/dfu.c @@ -88,7 +88,7 @@ exit: board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
if (dfu_reset)
run_command("reset", 0);
do_reset(NULL, 0, 0, NULL);g_dnl_clear_detach();
So this hunk drops out the need for cli stuff.
Yes.
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 1231351..f51ae2c 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -6,6 +6,9 @@ config SUPPORT_SPL config SUPPORT_TPL bool
+config SPL_DFU_NO_RESET
- bool
config SPL bool depends on SUPPORT_SPL @@ -646,6 +649,7 @@ config SPL_USBETH_SUPPORT config SPL_DFU_SUPPORT bool "Support DFU (Device Firmware Upgarde)" select SPL_HASH_SUPPORT
- select SPL_DFU_NO_RESET depends on SPL_RAM_SUPPORT help This feature enables the DFU (Device Firmware Upgarde) in SPL with
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 8dacc1a..ceb33e3 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -35,7 +35,11 @@ static struct hash_algo *dfu_hash_algo; */ __weak bool dfu_usb_get_reset(void)
{
+#ifdef CONFIG_SPL_DFU_NO_RESET
- return false;
+#else return true; +#endif }
static int dfu_find_alt_num(const char *s)
So do we still need the above, in order to save space? How much are we saving here even, now? Thanks!
I observed around 7K reduced.
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Tom
static int dfu_find_alt_num(const char *s)
So do we still need the above, in order to save space? How much are we saving here even, now? Thanks!
I observed around 7K reduced.
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Without this fix, with cli_simple_run_command(), size of MLO with default dra7xx_evm_defconfig is 130K. With this fix, compile out cli.c, the MLO size is 126K. Around 4K is space saved.
Regards Ravi
On Tue, May 02, 2017 at 12:41:48PM +0000, B, Ravi wrote:
Tom
static int dfu_find_alt_num(const char *s)
So do we still need the above, in order to save space? How much are we saving here even, now? Thanks!
I observed around 7K reduced.
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Without this fix, with cli_simple_run_command(), size of MLO with default dra7xx_evm_defconfig is 130K. With this fix, compile out cli.c, the MLO size is 126K. Around 4K is space saved.
OK. And dropping out CLI and leaving in reset, unconditionally vs dropping out CLI and also dropping reset via DFU, what is the savings there? Is that 3K?
Tom
I observed around 7K reduced.
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Without this fix, with cli_simple_run_command(), size of MLO with default dra7xx_evm_defconfig is 130K. With this fix, compile out cli.c, the MLO size is 126K. Around 4K is space saved.
OK. And dropping out CLI and leaving in reset, unconditionally vs dropping out CLI and also dropping reset via DFU, what is the savings there? Is that 3K?
7K provided earlier was wrong calculation. Sorry for confusion.
If unconditionally dropping CLI and use do_reset instead of run_command, I will save around 4K. (with this patch v2 series) If unconditionally dropping CLI and dropping do_reset in SPL-DFU, I will save around 5K. (with this patch series + drop do_reset in SPL-DFU unconditionally)
Regards Ravi
On Tue, May 02, 2017 at 01:02:24PM +0000, B, Ravi wrote:
Tom
I observed around 7K reduced.
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Without this fix, with cli_simple_run_command(), size of MLO with default dra7xx_evm_defconfig is 130K. With this fix, compile out cli.c, the MLO size is 126K. Around 4K is space saved.
OK. And dropping out CLI and leaving in reset, unconditionally vs dropping out CLI and also dropping reset via DFU, what is the savings there? Is that 3K?
7K provided earlier was wrong calculation. Sorry for confusion.
OK.
If unconditionally dropping CLI and use do_reset instead of run_command, I will save around 4K. (with this patch v2 series) If unconditionally dropping CLI and dropping do_reset in SPL-DFU, I will save around 5K. (with this patch series + drop do_reset in SPL-DFU unconditionally)
Can you give the exact bytes saved in each case, with your specific compiler? I ask since I'm surprised it's more than a function being dropped by the linker in this case. diff'ing the u-boot-spl.map files would also say what is dropped and I'd be interested in that. (And yes, I'm asking for more details to justify adding a Kconfig option here). Thanks!
Tom
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Without this fix, with cli_simple_run_command(), size of MLO with default dra7xx_evm_defconfig is 130K. With this fix, compile out cli.c, the MLO size is 126K. Around 4K is space saved.
OK. And dropping out CLI and leaving in reset, unconditionally vs dropping out CLI and also dropping reset via DFU, what is the savings there? Is that 3K?
7K provided earlier was wrong calculation. Sorry for confusion.
OK.
If unconditionally dropping CLI and use do_reset instead of run_command, I will save around 4K. (with this patch v2 series) If unconditionally dropping CLI and dropping do_reset in SPL-DFU, I will save around 5K. (with this patch series + drop do_reset in SPL-DFU unconditionally)
Can you give the exact bytes saved in each case, with your specific compiler? I ask since I'm surprised it's more than a function being dropped by the linker in this case. diff'ing the u-boot-spl.map files would also say what is dropped and I'd be interested in that. (And >yes, I'm asking for more details to justify adding a Kconfig option here). Thanks
Compiler : arm-linu-gnueabihif-gcc, version: 6.2-2016.11)
1) default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO size is 129998 2) default dra7xx_evm_defconfig and dropping CLI and use do_reset - MLO size is 126130, saving is ~4K (129998-126130 = 3878 bytes). 3) default dra7xx_evm_defconfig and dropping CLI and dropping do_reset - MLO size is 125298, saving is ~5K (129998-125298 = 4708 bytes).
Regards Ravi
Tom
Can you give the exact bytes saved in each case, with your specific compiler? I ask since I'm surprised it's more than a function being dropped by the linker in this case. diff'ing the u-boot-spl.map files would also say what is dropped and I'd be interested in that. > (And >yes, I'm asking for more details to justify adding a Kconfig option here). Thanks
Compiler : arm-linu-gnueabihif-gcc, version: 6.2-2016.11)
- default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO size is 129998
This is with no patches.
- default dra7xx_evm_defconfig and dropping CLI and use do_reset - MLO size is 126130, saving is ~4K (129998-126130 = 3878 bytes).
This 4K saving is based on this V2 patches series (excludes only CONFIG_DFU_MMC in SPL-DFU).
- default dra7xx_evm_defconfig and dropping CLI and dropping do_reset - MLO size is 125298, saving is ~5K (129998-125298 = 4708 bytes).
(My bad, I changed to V1 initial series in between while taking this data) This 5K saving is based on this V1 patches (basically, which excludes all CONFIG_DFU_<MMC/NAND/SF/TFTP> in SPL-DFU) Dropping do_reset in SPL does not reduce the MLO size. I observe do_reset is always included in spl-uboot.map whether exclude and include.
Regards Ravi
On Tue, May 02, 2017 at 01:56:45PM +0000, B, Ravi wrote:
Tom
Can you give the exact bytes saved in each case, with your specific compiler? I ask since I'm surprised it's more than a function being dropped by the linker in this case. diff'ing the u-boot-spl.map files would also say what is dropped and I'd be interested in that. > (And >yes, I'm asking for more details to justify adding a Kconfig option here). Thanks
Compiler : arm-linu-gnueabihif-gcc, version: 6.2-2016.11)
- default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO size is 129998
This is with no patches.
- default dra7xx_evm_defconfig and dropping CLI and use do_reset - MLO size is 126130, saving is ~4K (129998-126130 = 3878 bytes).
This 4K saving is based on this V2 patches series (excludes only CONFIG_DFU_MMC in SPL-DFU).
- default dra7xx_evm_defconfig and dropping CLI and dropping do_reset - MLO size is 125298, saving is ~5K (129998-125298 = 4708 bytes).
(My bad, I changed to V1 initial series in between while taking this data) This 5K saving is based on this V1 patches (basically, which excludes all CONFIG_DFU_<MMC/NAND/SF/TFTP> in SPL-DFU) Dropping do_reset in SPL does not reduce the MLO size. I observe do_reset is always included in spl-uboot.map whether exclude and include.
So in other words, we don't save any space by making DFU-reset be conditionally included? Thanks!
-----Original Message----- From: Tom Rini [mailto:trini@konsulko.com] Sent: Tuesday, May 02, 2017 8:24 PM To: B, Ravi Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v2 2/3] common: dfu: ignore reset for spl-dfu
On Tue, May 02, 2017 at 01:56:45PM +0000, B, Ravi wrote:
Tom
Can you give the exact bytes saved in each case, with your specific compiler? I ask since I'm surprised it's more than a function being dropped by the linker in this case. diff'ing the u-boot-spl.map files would also say what is dropped and I'd be interested in that. >> (And >yes, I'm asking for more details to justify adding a Kconfig option here). Thanks
Compiler : arm-linu-gnueabihif-gcc, version: 6.2-2016.11)
- default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO
size is 129998
This is with no patches.
- default dra7xx_evm_defconfig and dropping CLI and use do_reset - MLO size is 126130, saving is ~4K (129998-126130 = 3878 bytes).
This 4K saving is based on this V2 patches series (excludes only CONFIG_DFU_MMC in SPL-DFU).
- default dra7xx_evm_defconfig and dropping CLI and dropping do_reset - MLO size is 125298, saving is ~5K (129998-125298 = 4708 bytes).
(My bad, I changed to V1 initial series in between while taking this data) This 5K saving is based on this V1 patches (basically, which excludes all CONFIG_DFU_<MMC/NAND/SF/TFTP> in SPL-DFU) Dropping do_reset in SPL does not reduce the MLO size. I observe do_reset is always included in spl-uboot.map whether exclude and include.
So in other words, we don't save any space by making DFU-reset be conditionally included? Thanks!
Yes, you are correct Tom.
Thanks & Regards Ravi
Tom
static int dfu_find_alt_num(const char *s)
So do we still need the above, in order to save space? How much are we saving here even, now? Thanks!
I observed around 7K reduced.
Ignore 7K figure provided, that's wrong calculation.
I don't just mean dropping out CLI, I mean after dropping out CLI but leaving in the reset logic. That's _still_ 7k?
Without this fix, with cli_simple_run_command(), size of MLO with default dra7xx_evm_defconfig is 130K. With this fix, compile out cli.c, the MLO size is 126K. Around 4K is space saved.
do_reset() used instead of run_command() for dfu_reset().
Regards Ravi
Since spl-dfu does not dfu-reset, there is no need of run_command_cli, hence compiling out cli.c and cli_hush.c to reduce the spl-dfu memory foot print.
Signed-off-by: Ravi Babu ravibabu@ti.com --- need better way for how to compile out CONFIG_DFU_MMC
common/Makefile | 3 +-- drivers/dfu/Makefile | 2 ++ include/dfu.h | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/common/Makefile b/common/Makefile index 86225f1..8976cbc 100644 --- a/common/Makefile +++ b/common/Makefile @@ -11,6 +11,7 @@ obj-y += init/ obj-y += main.o obj-y += exports.o obj-y += hash.o +obj-y += cli.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o
@@ -90,7 +91,6 @@ endif # !CONFIG_SPL_BUILD
ifdef CONFIG_SPL_BUILD obj-$(CONFIG_SPL_DFU_SUPPORT) += dfu.o -obj-$(CONFIG_SPL_DFU_SUPPORT) += cli_hush.o obj-$(CONFIG_SPL_HASH_SUPPORT) += hash.o obj-$(CONFIG_ENV_IS_IN_FLASH) += env_flash.o obj-$(CONFIG_SPL_YMODEM_SUPPORT) += xyzModem.o @@ -171,7 +171,6 @@ endif # We always have this since drivers/ddr/fs/interactive.c needs it obj-$(CONFIG_CMDLINE) += cli_simple.o
-obj-y += cli.o obj-$(CONFIG_CMDLINE) += cli_readline.o obj-$(CONFIG_CMD_DFU) += dfu.o obj-y += command.o diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile index 61f2b71..5628734 100644 --- a/drivers/dfu/Makefile +++ b/drivers/dfu/Makefile @@ -6,7 +6,9 @@ #
obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o +ifndef CONFIG_SPL_BUILD obj-$(CONFIG_DFU_MMC) += dfu_mmc.o +endif obj-$(CONFIG_DFU_NAND) += dfu_nand.o obj-$(CONFIG_DFU_RAM) += dfu_ram.o obj-$(CONFIG_DFU_SF) += dfu_sf.o diff --git a/include/dfu.h b/include/dfu.h index f39d3f1..70eeaef 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -203,7 +203,7 @@ static inline void dfu_set_defer_flush(struct dfu_entity *dfu) int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);
/* Device specific */ -#ifdef CONFIG_DFU_MMC +#if defined(CONFIG_DFU_MMC) && !defined(CONFIG_SPL_BUILD) extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s); #else static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr,
participants (3)
-
B, Ravi -
Ravi Babu
-
Tom Rini