spl: dfu: misc fixes and reduce MLO foot print

PATCH Superseded Ravi Babu 4 participants 21 messages Apr 26, 2017 – May 3, 2017

Share & tools

Ravi Babu

author

2017-04-26 13:14 UTC #0 lore raw

cover 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

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         | 3 +++
 common/spl/Kconfig   | 1 +
 drivers/dfu/Makefile | 4 +++-
 include/dfu.h        | 8 ++++----
 5 files changed, 12 insertions(+), 7 deletions(-)

Signature · 1 line

1.9.1

Ravi Babu

author

2017-04-26 13:14 UTC #1 lore raw

PATCH 1/3 spl: Kconfig: dfu: spl-dfu depends on SPL_RAM_SUPPORT

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 <[email protected]>

---
 common/spl/Kconfig | 1 +
 1 file changed, 1 insertion(+)

common/spl/Kconfig +1 -0

index ea6fbb6..1231351 100644
@@ -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

Signature · 1 line

1.9.1

Tom Rini

2017-04-26 13:36 UTC #2 lore raw

PATCH 1/3 spl: Kconfig: dfu: spl-dfu depends on SPL_RAM_SUPPORT

Quoted 5 lines · Ravi Babu

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 <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170426/fa77d648/attachment.sig>

Ravi Babu

author

2017-04-26 13:14 UTC #3 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

In reply to Ravi Babu

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 <[email protected]>

---
 common/dfu.c | 3 +++
 1 file changed, 3 insertions(+)

common/dfu.c +3 -0

index 0e9f5f5..fa77526 100644
@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
 
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
 	if (dfu_reset)
 		run_command("reset", 0);

Signature · 1 line

1.9.1

Tom Rini

2017-04-26 13:40 UTC #4 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

Quoted 16 lines · Ravi Babu

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 <[email protected]>
---
 common/dfu.c | 3 +++
 1 file changed, 3 insertions(+)
common/dfu.c +0 -0
index 0e9f5f5..fa77526 100644

common/dfu.c exit: · line 94

Quoted 8 lines

common/dfu.c +3 -0

@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
 
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
 	if (dfu_reset)
 		run_command("reset", 0);

So we "fix" some of the problems we see by saying that you can't reset the board in SPL via DFU. I think maybe we should instead drop run_command here and make reset-via-DFU call do_reset() directly like some other small-size-required cases do. This will let us drop the command requirement here but still allow for "use DFU to flash and reset the board with just SPL" as a use-case. Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170426/b0bf380c/attachment.sig>

B

2017-04-26 15:58 UTC #5 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

Hi Tom

Quoted 17 lines

Quoted 17 lines
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 <[email protected]>
---
 common/dfu.c | 3 +++
 1 file changed, 3 insertions(+)
common/dfu.c +0 -0
@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
 

Quoted 8 lines

common/dfu.c exit: · line 94
Quoted 5 lines
common/dfu.c +3 -0
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
 	if (dfu_reset)
 		run_command("reset", 0);
So we "fix" some of the problems we see by saying that you can't reset the board in SPL via DFU.
I think maybe we should instead drop run_command here and make reset-via-DFU call do_reset() directly like some other small-size-required cases do. This will let us drop the command >requirement here but still allow for "use DFU to flash and reset the board with just SPL" as a use-case. Thanks!

The SPL-DFU will load and execute u-boot.img from RAM. If we issue dfu-reset (-R switch), this leads to cpu-reset and we lost the purpose of SPL-DFU itself. Hence dfu-reset issue shall not be issued for SPL-DFU.

I agree, the dfu-reset is needed in u-boot, after flashing images to QSPI/eMMC/SD using the DFU to execute newly loaded image. So, dfu-reset is needed for u-boot, but not required for SPL-DFU.

For u-boot, we can continue to use run_command() for dfu-reset.

Regards
Ravi

Tom Rini

2017-04-26 16:24 UTC #6 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

Quoted 37 lines · Ravi

Hi Tom
Quoted 17 lines
Quoted 17 lines
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 <[email protected]>
---
 common/dfu.c | 3 +++
 1 file changed, 3 insertions(+)
common/dfu.c +0 -0
@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
 
Quoted 12 lines
common/dfu.c exit: · line 94
Quoted 5 lines
common/dfu.c +3 -0
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
 	if (dfu_reset)
 		run_command("reset", 0);
So we "fix" some of the problems we see by saying that you can't reset the board in SPL via DFU. I think maybe we should instead drop run_command here and make reset-via-DFU call do_reset() directly like some other small-size-required cases do. This will let us drop the command >requirement here but still allow for "use DFU to flash and reset the board with just SPL" as a use-case. Thanks!
The SPL-DFU will load and execute u-boot.img from RAM. If we issue dfu-reset (-R switch), this leads to cpu-reset and we lost the purpose of SPL-DFU itself. Hence dfu-reset issue shall not be issued for SPL-DFU.

Quoted 5 lines

I agree, the dfu-reset is needed in u-boot, after flashing images to QSPI/eMMC/SD using the DFU to execute newly loaded image. So, dfu-reset is needed for u-boot, but not required for SPL-DFU.
For u-boot, we can continue to use run_command() for dfu-reset.

OK. I guess if someone else wants to try and use SPL for DFU flashing that requires more work and they can address the above then, thanks!

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170426/f8088a03/attachment.sig>

B

2017-04-26 16:25 UTC #7 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

Hi Tom

Quoted 10 lines

Quoted 10 lines
The SPL-DFU will load and execute u-boot.img from RAM. If we issue dfu-reset (-R switch), this leads to cpu-reset and we lost the purpose of SPL-DFU itself. Hence dfu-reset issue shall not be issued for SPL-DFU.
I agree, the dfu-reset is needed in u-boot, after flashing images to QSPI/eMMC/SD using the DFU to execute newly loaded image. So, dfu-reset is needed for u-boot, but not required for SPL-DFU.

Quoted 5 lines

Quoted 1 line
For u-boot, we can continue to use run_command() for dfu-reset.
OK. I guess if someone else wants to try and use SPL for DFU flashing that requires more work and they can address the above then, thanks!
Reviewed-by Tom Rini <[email protected]>

Thanks.

Any comments on [PATCH 3/3]?

Regards
Ravi

Lukasz Majewski

2017-04-27 08:06 UTC #8 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

In reply to Tom Rini

Quoted 34 lines · On Wed, 26 Apr 2017 12:24:06 -0400 Tom Rini <[email protected]> wrote:

Quoted 33 lines · Ravi
Hi Tom
Quoted 30 lines
Quoted 14 lines
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 <[email protected]>
---
 common/dfu.c | 3 +++
 1 file changed, 3 insertions(+)
common/dfu.c index 0e9f5f5..fa77526 renamed from common/dfu.c +0 -0
100644 --- a/common/dfu.c
+++ b/common/dfu.c
common/dfu.c index 0e9f5f5..fa77526 exit: · line 94
Quoted 8 lines
common/dfu.c index 0e9f5f5..fa77526 renamed from common/dfu.c +3 -0
@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
 
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
 	if (dfu_reset)
 		run_command("reset", 0);
So we "fix" some of the problems we see by saying that you can't reset the board in SPL via DFU. I think maybe we should instead drop run_command here and make reset-via-DFU call do_reset() directly like some other small-size-required cases do. This will let us drop the command >requirement here but still allow for "use DFU to flash and reset the board with just SPL" as a use-case. Thanks!

Quoted 4 lines

Quoted 4 lines
The SPL-DFU will load and execute u-boot.img from RAM. If we issue dfu-reset (-R switch), this leads to cpu-reset and we lost the purpose of SPL-DFU itself. Hence dfu-reset issue shall not be issued for SPL-DFU.

It seems like a valid use case - maybe it would be beneficial to add Kconfig option (CONFIG_DFU_SPL_NO_RESET) to give the user possibility to decide (and in this way document it?).

Quoted 5 lines

Quoted 5 lines
I agree, the dfu-reset is needed in u-boot, after flashing images to QSPI/eMMC/SD using the DFU to execute newly loaded image. So, dfu-reset is needed for u-boot, but not required for SPL-DFU.

Quoted 7 lines

Quoted 1 line
For u-boot, we can continue to use run_command() for dfu-reset.
OK. I guess if someone else wants to try and use SPL for DFU flashing that requires more work and they can address the above then, thanks!
Reviewed-by Tom Rini <[email protected]>

Best regards,

Lukasz Majewski

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170427/f2c24af4/attachment.sig>

B

2017-04-27 08:37 UTC #9 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

Lukasz

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
The SPL-DFU will load and execute u-boot.img from RAM. If we issue dfu-reset (-R switch), this leads to cpu-reset and we lost the purpose of SPL-DFU itself. Hence dfu-reset issue shall not be issued for SPL-DFU.
It seems like a valid use case - maybe it would be beneficial to add Kconfig option (CONFIG_DFU_SPL_NO_RESET) to give the user possibility to decide (and in this way document it?).

Yes, make sense, to differentiate dfu-reset for SPL-DFU. Ok, I will include CONFIG_SPL_DFU_NO_RESET in next version of patch.

Thanks.

Quoted 11 lines

Quoted 11 lines
Quoted 6 lines
I agree, the dfu-reset is needed in u-boot, after flashing images to QSPI/eMMC/SD using the DFU to execute newly loaded image. So, dfu-reset is needed for u-boot, but not required for SPL-DFU.
For u-boot, we can continue to use run_command() for dfu-reset.
OK. I guess if someone else wants to try and use SPL for DFU flashing that requires more work and they can address the above then, thanks!
Reviewed-by Tom Rini <[email protected]>

Regards
Ravi

B

2017-04-27 08:37 UTC #10 lore raw

PATCH 2/3 common: dfu: ignore reset for spl-dfu

In reply to Lukasz Majewski

Lukasz

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
The SPL-DFU will load and execute u-boot.img from RAM. If we issue dfu-reset (-R switch), this leads to cpu-reset and we lost the purpose of SPL-DFU itself. Hence dfu-reset issue shall not be issued for SPL-DFU.
It seems like a valid use case - maybe it would be beneficial to add Kconfig option (CONFIG_DFU_SPL_NO_RESET) to give the user possibility to decide (and in this way document it?).

Yes, make sense, to differentiate dfu-reset for SPL-DFU. Ok, I will include CONFIG_SPL_DFU_NO_RESET in next version of patch.

Thanks.

Quoted 11 lines

Quoted 11 lines
Quoted 6 lines
I agree, the dfu-reset is needed in u-boot, after flashing images to QSPI/eMMC/SD using the DFU to execute newly loaded image. So, dfu-reset is needed for u-boot, but not required for SPL-DFU.
For u-boot, we can continue to use run_command() for dfu-reset.
OK. I guess if someone else wants to try and use SPL for DFU flashing that requires more work and they can address the above then, thanks!
Reviewed-by Tom Rini <[email protected]>

Regards
Ravi

Ravi Babu

author

2017-04-26 13:14 UTC #11 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

In reply to Ravi Babu

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 <[email protected]>

---
 common/Makefile      | 3 +--
 drivers/dfu/Makefile | 4 +++-
 include/dfu.h        | 8 ++++----
 3 files changed, 8 insertions(+), 7 deletions(-)

common/Makefile +1 -2

index 86225f1..8976cbc 100644
@@ -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

drivers/dfu/Makefile +3 -1

index 61f2b71..ef48f36 100644
@@ -6,8 +6,10 @@
 #
 
 obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
+ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
-obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
 obj-$(CONFIG_DFU_TFTP) += dfu_tftp.o
+endif
+obj-$(CONFIG_DFU_RAM) += dfu_ram.o

include/dfu.h +4 -4

index f39d3f1..b53ae80 100644
@@ -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,

@@ -214,7 +214,7 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr,
 }
 #endif
 
-#ifdef CONFIG_DFU_NAND
+#if defined(CONFIG_DFU_NAND) && !defined(CONFIG_SPL_BUILD)
 extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s);
 #else
 static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr,

@@ -236,7 +236,7 @@ static inline int dfu_fill_entity_ram(struct dfu_entity *dfu, char *devstr,
 }
 #endif
 
-#ifdef CONFIG_DFU_SF
+#if defined(CONFIG_DFU_SF) && !defined(CONFIG_SPL_BUILD)
 extern int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr, char *s);
 #else
 static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr,

@@ -260,7 +260,7 @@ static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr,
  *
  * @return 0 on success, otherwise error code
  */
-#ifdef CONFIG_DFU_TFTP
+#if defined(CONFIG_DFU_TFTP) && !defined(CONFIG_SPL_BUILD)
 int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len,
 		   char *interface, char *devstring);
 #else

Signature · 1 line

1.9.1

Tom Rini

2017-04-26 13:35 UTC #12 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Quoted 5 lines · Ravi Babu

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 <[email protected]>

[snip]

Quoted 6 lines

drivers/dfu/Makefile +0 -0

index 61f2b71..ef48f36 100644
@@ -6,8 +6,10 @@
 #

drivers/dfu/Makefile line 15

Quoted 10 lines

drivers/dfu/Makefile +3 -1

 
 obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
+ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
-obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
 obj-$(CONFIG_DFU_TFTP) += dfu_tftp.o
+endif
+obj-$(CONFIG_DFU_RAM) += dfu_ram.o

We should discard at link time the unreachable parts here in SPL, no?

include/dfu.h static inline void dfu_set_defer_flush(struct dfu_entity *dfu) · line 206

Quoted 10 lines

include/dfu.h +1 -1

index f39d3f1..b53ae80 100644
@@ -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)

I don't like the initial condition we have here, adding the !SPL_BUILD test makes this even worse, lets not do that.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170426/0c1fef39/attachment.sig>

B

2017-04-27 07:22 UTC #13 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Hi Tom

Quoted 5 lines

Quoted 5 lines
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 <[email protected]>

[snip]

Quoted 7 lines

Quoted 7 lines
drivers/dfu/Makefile index renamed from drivers/dfu/Makefile +0 -0
61f2b71..ef48f36 100644
drivers/dfu/Makefile +0 -0
@@ -6,8 +6,10 @@
 #
 

Quoted 11 lines

drivers/dfu/Makefile line 15

Quoted 9 lines

drivers/dfu/Makefile +3 -1

 obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
+ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
-obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
 obj-$(CONFIG_DFU_TFTP) += dfu_tftp.o
+endif
+obj-$(CONFIG_DFU_RAM) += dfu_ram.o

We should discard at link time the unreachable parts here in SPL, no?

Yes you are correct.
But what is happening here is, the CONFIG_DFU_<MMC/NAND/SF/TFTP> selected through Kconfig/Menuconfig is applicable for both SPL and U-Boot. Hence CONFIG_DFU_MMC/NAND/SF gets compiled for SPL as well, which needs run_command(). Actually CONFIG_DFU_MMC/NAND/etc is not scoped for SPL-DFU. As we have aligned, not to increase the SPL size, user shall use SPL-DFU feature to boot to u-boot, then utilize the full featured DFU to flash MMC/NAND/SF.

I get undefined reference to common function run_command(), "dfu_fill_entitiy_<mmc/nand/sf>" in driver/dfu/dfu.c. The dfu.c is common for both SPL-DFU and U-boot.

Quoted 8 lines

Quoted 8 lines
include/dfu.h index f39d3f1..b53ae80 renamed from include/dfu.h +0 -0
100644
include/dfu.h +0 -0
@@ -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);
 

Quoted 5 lines

include/dfu.h static inline void dfu_set_defer_flush(struct dfu_entity *dfu) · line 206
Quoted 3 lines
include/dfu.h +1 -1
 /* Device specific */
-#ifdef CONFIG_DFU_MMC
+#if defined(CONFIG_DFU_MMC) && !defined(CONFIG_SPL_BUILD)
I don't like the initial condition we have here, adding the !SPL_BUILD test makes this even worse, lets not do that.

I did not find better solution to reduce the SPL size by removing cli.c/cli_hush.c and compiling out CONFIG_DFU_<MMC/NAND/SF> for SPL altogether.

Regards
Ravi

Tom Rini

2017-04-27 12:31 UTC #14 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Quoted 27 lines · Ravi

Hi Tom
Quoted 5 lines
Quoted 5 lines
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 <[email protected]>
[snip]
Quoted 7 lines
Quoted 7 lines
drivers/dfu/Makefile index renamed from drivers/dfu/Makefile +0 -0
61f2b71..ef48f36 100644
drivers/dfu/Makefile +0 -0
@@ -6,8 +6,10 @@
 #
 
Quoted 11 lines
drivers/dfu/Makefile line 15
Quoted 9 lines
drivers/dfu/Makefile +3 -1
 obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
+ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
-obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
 obj-$(CONFIG_DFU_TFTP) += dfu_tftp.o
+endif
+obj-$(CONFIG_DFU_RAM) += dfu_ram.o
We should discard at link time the unreachable parts here in SPL, no?

Quoted 7 lines

Yes you are correct.
But what is happening here is, the CONFIG_DFU_<MMC/NAND/SF/TFTP> selected through Kconfig/Menuconfig is applicable for both SPL and U-Boot. Hence CONFIG_DFU_MMC/NAND/SF gets compiled for SPL as well, which needs run_command(). Actually CONFIG_DFU_MMC/NAND/etc is not scoped for SPL-DFU. As we have aligned, not to increase the SPL size, user shall use SPL-DFU feature to boot to u-boot, then utilize the full featured DFU to flash MMC/NAND/SF.
I get undefined reference to common function run_command(), "dfu_fill_entitiy_<mmc/nand/sf>" in driver/dfu/dfu.c. The dfu.c is common for both SPL-DFU and U-boot.

OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and use CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170427/615ba049/attachment.sig>

B

2017-04-27 17:25 UTC #15 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Hi Tom

Quoted 11 lines

Quoted 8 lines
Yes you are correct.
But what is happening here is, the CONFIG_DFU_<MMC/NAND/SF/TFTP> selected through Kconfig/Menuconfig is applicable for both SPL and U-Boot. Hence CONFIG_DFU_MMC/NAND/SF gets compiled for SPL as well, which needs run_command(). Actually CONFIG_DFU_MMC/NAND/etc is not scoped for SPL-DFU. As we have aligned, not to increase the SPL size, user shall use SPL-DFU feature to boot to u-boot, then utilize the full featured DFU to flash MMC/NAND/SF.
I get undefined reference to common function run_command(), "dfu_fill_entitiy_<mmc/nand/sf>" in driver/dfu/dfu.c. The dfu.c is common for both SPL-DFU and U-boot.
OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and use CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.

Ok, will do.

Regards
Ravi

B

2017-05-03 08:36 UTC #16 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

In reply to Tom Rini

Tom

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Yes you are correct.
But what is happening here is, the CONFIG_DFU_<MMC/NAND/SF/TFTP> selected through Kconfig/Menuconfig is applicable for both SPL and U-Boot. Hence CONFIG_DFU_MMC/NA
ND/SF gets compiled for SPL as well, which needs run_command(). Actually CONFIG_DFU_MMC/NAND/etc is not scoped for SPL-DFU.
Quoted 2 lines
Quoted 2 lines
As we have aligned, not to increase the SPL size, user shall use SPL-DFU feature to boot to u-boot, then utilize the full featured DFU to flash MMC/NAND/SF.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I get undefined reference to common function run_command(), "dfu_fill_entitiy_<mmc/nand/sf>" in driver/dfu/dfu.c. The dfu.c is common for both SPL-DFU and U-boot.
OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and use CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.
Ok, will do.

Correct me if I am wrong, I need understand if we introduce say SPL_DFU_MMC Kconfig options, then whether need to support DFU_MMC in SPL ? Again this will increase the SPL-size, and also DFU_MMC uses run_command() again, there is dependency of cli.c, hush etc.

Regards
Ravi

Tom Rini

2017-05-03 12:42 UTC #17 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Quoted 15 lines · Ravi

Tom
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Yes you are correct.
But what is happening here is, the CONFIG_DFU_<MMC/NAND/SF/TFTP> selected through Kconfig/Menuconfig is applicable for both SPL and U-Boot. Hence CONFIG_DFU_MMC/NA
ND/SF gets compiled for SPL as well, which needs run_command(). Actually CONFIG_DFU_MMC/NAND/etc is not scoped for SPL-DFU.
Quoted 7 lines
Quoted 4 lines
As we have aligned, not to increase the SPL size, user shall use SPL-DFU feature to boot to u-boot, then utilize the full featured DFU to flash MMC/NAND/SF.
I get undefined reference to common function run_command(), "dfu_fill_entitiy_<mmc/nand/sf>" in driver/dfu/dfu.c. The dfu.c is common for both SPL-DFU and U-boot.
OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and use CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.

Quoted 7 lines

Quoted 1 line
Ok, will do.
Correct me if I am wrong, I need understand if we introduce say SPL_DFU_MMC Kconfig options, then whether need to support DFU_MMC in SPL ?
Again this will increase the SPL-size, and also DFU_MMC uses run_command() again, there is dependency of cli.c, hush etc.

SPL_DFU_MMC will only increase the size of SPL if it's enabled. Being able to switch to testing with CONFIG_IS_ENABLED(DFU_xxx) means that we'll be able to keep the space savings while also not making various parts of the code harder to read with more #ifdef tests.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170503/f66352f4/attachment.sig>

B

2017-05-03 12:45 UTC #18 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Hi Tom

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and
use
CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.
Ok, will do.

Quoted 7 lines

Quoted 5 lines
Correct me if I am wrong, I need understand if we introduce say SPL_DFU_MMC Kconfig options, then whether need to support DFU_MMC in SPL ?
Again this will increase the SPL-size, and also DFU_MMC uses run_command() again, there is dependency of cli.c, hush etc.
SPL_DFU_MMC will only increase the size of SPL if it's enabled. Being able to switch to testing with CONFIG_IS_ENABLED(DFU_xxx) means that we'll be able to keep the space savings while also not making various parts of the code harder to read with more #ifdef tests.

Ok, since SPL_DFU_MMC will be added in Kconfig, if SPL_DFU_MMC is selected it will increase the SPL size, it means SPL_DFU_MMC dependency code shall be included (like cli,c, hush etc). When SPL_DFU_MMC is not selected then automatically SPL size will be reduced.

Thanks.

Regards
Ravi

Lukasz Majewski

2017-05-03 20:53 UTC #19 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

Quoted 22 lines · Ravi

Hi Tom
Quoted 14 lines
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and use
CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.
Quoted 7 lines
Quoted 1 line
Ok, will do.
Correct me if I am wrong, I need understand if we introduce say SPL_DFU_MMC Kconfig options, then whether need to support DFU_MMC in SPL ?
Again this will increase the SPL-size, and also DFU_MMC uses run_command() again, there is dependency of cli.c, hush etc.
Quoted 5 lines
SPL_DFU_MMC will only increase the size of SPL if it's enabled. Being able to switch to testing with CONFIG_IS_ENABLED(DFU_xxx) means that we'll be able to keep the space savings while also not making various parts of the code harder to read with more #ifdef tests.

Quoted 5 lines

Ok, since SPL_DFU_MMC will be added in Kconfig, if SPL_DFU_MMC is selected it will increase the SPL size, it means SPL_DFU_MMC dependency code shall be included (like cli,c, hush etc). When SPL_DFU_MMC is not selected then automatically SPL size will be reduced.

My impression here is that CONFIG_IS_ENABLED() will help us to make the code looking better and document changes by Kconfig variables.

And to be honest - I do not feel like adding hush to SPL is a good conceptual solution.

Quoted 5 lines

Thanks.
Regards
Ravi

Best regards,

Lukasz Majewski

2017-05-03 20:48 UTC #20 lore raw

PATCH 3/3 spl: dfu: reduce spl-dfu MLO size

In reply to Tom Rini

Hi Tom,

Quoted 31 lines · Ravi

Quoted 22 lines
Tom
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
Yes you are correct.
But what is happening here is, the CONFIG_DFU_<MMC/NAND/SF/TFTP> selected through Kconfig/Menuconfig is applicable for both SPL and U-Boot. Hence CONFIG_DFU_MMC/NA
ND/SF gets compiled for SPL as well, which needs run_command(). Actually CONFIG_DFU_MMC/NAND/etc is not scoped for SPL-DFU.
Quoted 11 lines
Quoted 7 lines
As we have aligned, not to increase the SPL size, user shall use SPL-DFU feature to boot to u-boot, then utilize the full featured DFU to flash MMC/NAND/SF.
I get undefined reference to common function run_command(), "dfu_fill_entitiy_<mmc/nand/sf>" in driver/dfu/dfu.c. The dfu.c is common for both SPL-DFU and U-boot.
OK. I think we need to introduce SPL_DFU_xxx Kconfig options, and use CONFIG_IS_ENABLED(DFU_xxx) so that we will get things enabled/disabled (and discarded) as needed.
Quoted 7 lines
Quoted 1 line
Ok, will do.
Correct me if I am wrong, I need understand if we introduce say SPL_DFU_MMC Kconfig options, then whether need to support DFU_MMC in SPL ?
Again this will increase the SPL-size, and also DFU_MMC uses run_command() again, there is dependency of cli.c, hush etc.

Quoted 5 lines

SPL_DFU_MMC will only increase the size of SPL if it's enabled. Being able to switch to testing with CONFIG_IS_ENABLED(DFU_xxx) means that we'll be able to keep the space savings while also not making various parts of the code harder to read with more #ifdef tests.

(and I would like to see the code soon :-) )

Best regards,

Lukasz Majewski