common: dfu: ignore reset for spl-dfu

PATCH Superseded B 2 participants 7 messages Apr 27, 2017 5 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20170427095527.2a3992fe@jawa

B

2017-04-27 08:26 UTC #3 lore raw

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

Lukasz

Quoted 7 lines

common/dfu.c +0 -0

@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);

Quoted 5 lines

common/dfu.c exit: · line 92
Quoted 3 lines
common/dfu.c +3 -0
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
Why do you only ifdef this part? What problem does this solve?

Common/dfu.c is common code for SPL and U-boot, for SPL-DFU dfu_reset should not be given. This is must fix. Also this avoid use of run_command for SPL-DFU altogether, SPL size also will reduce by removing cli.c/cli_hush.c

Regards
Ravi

Lukasz Majewski

2017-04-27 10:33 UTC #4 lore raw

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

Quoted 15 lines · Ravi

Lukasz
Quoted 7 lines
Quoted 7 lines
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 renamed from common/dfu.c +0 -0
@@ -87,6 +87,9 @@ exit:
 	g_dnl_unregister();
 	board_usb_cleanup(usbctrl_index, USB_INIT_DEVICE);
 
Quoted 5 lines
common/dfu.c index 0e9f5f5..fa77526 exit: · line 92
Quoted 3 lines
common/dfu.c index 0e9f5f5..fa77526 renamed from common/dfu.c +3 -0
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
Why do you only ifdef this part? What problem does this solve?

Quoted 4 lines

Common/dfu.c is common code for SPL and U-boot, for SPL-DFU dfu_reset should not be given. This is must fix. Also this avoid use of run_command for SPL-DFU altogether, SPL size also will reduce by removing cli.c/cli_hush.c

As I've metioned in the other mail. Kconfig option would be OK.

Please look into the dfu_usb_get_reset() __weak function definition.

It is by default set to true.

You can extend this function to take into account a Kconfig option to return false during SPL builds.

Quoted 3 lines

Regards
Ravi

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

Lukasz Majewski

2017-04-27 10:34 UTC #5 lore raw

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

Hi,

Quoted 16 lines

Quoted 13 lines · Ravi
Lukasz
Quoted 10 lines
Quoted 10 lines
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 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

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Why do you only ifdef this part? What problem does this solve?
Common/dfu.c is common code for SPL and U-boot, for SPL-DFU dfu_reset should not be given. This is must fix. Also this avoid use of run_command for SPL-DFU altogether, SPL size also will reduce by removing cli.c/cli_hush.c
As I've metioned in the other mail. Kconfig option would be OK.

And this Kconfig should be _only_ enabled for your SPL-DFU support enabled (also in Kconfig).

Quoted 17 lines

Please look into the dfu_usb_get_reset() __weak function definition.
It is by default set to true.
You can extend this function to take into account a Kconfig option to return false during SPL builds.
Quoted 3 lines
Regards
Ravi
Best regards,

Quoted 7 lines

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

Best regards,

Lukasz Majewski

B

2017-04-27 11:19 UTC #6 lore raw

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

Hi Lukasz

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
Why do you only ifdef this part? What problem does this solve?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Common/dfu.c is common code for SPL and U-boot, for SPL-DFU dfu_reset should not be given. This is must fix. Also this avoid use of run_command for SPL-DFU altogether, SPL size also will reduce by removing cli.c/cli_hush.c
As I've metioned in the other mail. Kconfig option would be OK.
And this Kconfig should be _only_ enabled for your SPL-DFU support enabled (also in Kconfig).

I have already added CONFIG_SPL_DFU_NO_RESET option for SPL-DFU in Kconfig.

Quoted 8 lines

Quoted 8 lines
Please look into the dfu_usb_get_reset() __weak function definition.
It is by default set to true.
You can extend this function to take into account a Kconfig option to return false during SPL builds.

As suggested by you, I feel this is best option.

I have test verified this option, will post in next patch version.

__weak bool dfu_usb_get_reset(void)
{
+#ifdef CONFIG_SPL_DFU_NO_RESET
+ return false
+#else

	return true;

+#endif
}

Also changing run_command() to do_reset().

If (dfu_reset)

	do_reset(NULL, 0, 0, NULL);

Regards
Ravi

Lukasz Majewski

2017-04-27 13:09 UTC #7 lore raw

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

Hi Ravi,

Quoted 41 lines

Hi Lukasz
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+#ifdef CONFIG_SPL_BUILD
+	dfu_reset = 0;
+#endif
Why do you only ifdef this part? What problem does this solve?
Common/dfu.c is common code for SPL and U-boot, for SPL-DFU dfu_reset should not be given. This is must fix. Also this avoid use of run_command for SPL-DFU altogether, SPL size also will reduce by removing cli.c/cli_hush.c
Quoted 4 lines
Quoted 1 line
As I've metioned in the other mail. Kconfig option would be OK.
And this Kconfig should be _only_ enabled for your SPL-DFU support enabled (also in Kconfig).
I have already added CONFIG_SPL_DFU_NO_RESET option for SPL-DFU in Kconfig.
Quoted 9 lines
Quoted 9 lines
Please look into the dfu_usb_get_reset() __weak function definition.
It is by default set to true.
You can extend this function to take into account a Kconfig option to return false during SPL builds.
As suggested by you, I feel this is best option.
I have test verified this option, will post in next patch version.
__weak bool dfu_usb_get_reset(void)
{
(quoted diff) +3 -0
+#ifdef CONFIG_SPL_DFU_NO_RESET
+	return false
+#else
	return true;

Quoted 7 lines

+#endif
}
Also changing run_command() to do_reset().
If (dfu_reset)
	do_reset(NULL, 0, 0, NULL);

One question - could you write some numbers before SPL dfu tinification and afterwards?

I'm just curious how much we can save up.

Quoted 4 lines

Regards
Ravi

Best regards,

Lukasz Majewski

B

2017-04-27 17:30 UTC #8 lore raw

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

Hi Lukasz,

Quoted 11 lines

Quoted 5 lines
Also changing run_command() to do_reset().
If (dfu_reset)
	do_reset(NULL, 0, 0, NULL);
+1
One question - could you write some numbers before SPL dfu tinification and afterwards?
I'm just curious how much we can save up.

I am OOO till May 2nd. Will provide more details once I am back.

Regards
Ravi

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20170427095808.4a87f63c@jawa

B

2017-04-27 11:21 UTC #11 lore raw

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

{Corrected typo u-boot mailing list.}

Hi Lukasz

Quoted 4 lines

Quoted 4 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.

[..]

Quoted 8 lines

Review
Quoted 3 lines
*/
(quoted diff) +1 -1
-#ifdef CONFIG_DFU_TFTP
+#if defined(CONFIG_DFU_TFTP) && !defined(CONFIG_SPL_BUILD)
And then somebody would like to use DFU TFTP in the SPL (like you use DFU) and we would have new set of problems.
Sorry, but this is a wrong approach.

I agree with you for TFTP use-case.
But as aligned, in order to reduce SPL size, use SPL-DFU to load and execute u-boot from RAM. And then use complete DFU facility in u-boot for flashing MMC/SF/NAND etc.

I don't see any harm to allow for DFU_TFTP in SPL.
And SPL shall not support CONFIG_DFU_{MMC/NAND/SF} or any memory device except DFU_RAM.

Regards
Ravi