| Message ID | 1493295321-15498-3-git-send-email-ravibabu@ti.com |
|---|---|
| State | Accepted |
| Commit | 66928afb6b55647a446560d32427a032e674301f |
| Delegated to: | Łukasz Majewski |
| Headers | show |
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
>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
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) >1) default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO size is 129998 This is with no patches. >2) 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). >3) 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) > > >1) default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO size is 129998 > This is with no patches. > > >2) 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). > > >3) 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) >> >> >1) default dra7xx_evm_defconfig and use cli_simple_runcommand - MLO >> >size is 129998 >> This is with no patches. >> >> >2) 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). >> >> >3) 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
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)
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(-)