| Message ID | 1493212449-26863-3-git-send-email-ravibabu@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Łukasz Majewski |
| Headers | show |
On Wed, Apr 26, 2017 at 06:44:08PM +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 | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/common/dfu.c b/common/dfu.c > index 0e9f5f5..fa77526 100644 > --- a/common/dfu.c > +++ b/common/dfu.c > @@ -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!
Hi Tom >> 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 | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..fa77526 100644 >> --- a/common/dfu.c >> +++ b/common/dfu.c >> @@ -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! 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
On Wed, Apr 26, 2017 at 03:58:27PM +0000, B, Ravi wrote: > Hi Tom > > >> 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 | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..fa77526 100644 > >> --- a/common/dfu.c > >> +++ b/common/dfu.c > >> @@ -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! > > 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. 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 <trini@konsulko.com>
Hi Tom >> >> 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. >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 <trini@konsulko.com> Thanks. Any comments on [PATCH 3/3]? Regards Ravi
On Wed, 26 Apr 2017 12:24:06 -0400 Tom Rini <trini@konsulko.com> wrote: > On Wed, Apr 26, 2017 at 03:58:27PM +0000, B, Ravi wrote: > > Hi Tom > > > > >> 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 | 3 +++ > > >> 1 file changed, 3 insertions(+) > > >> > > >> diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..fa77526 > > >> 100644 --- a/common/dfu.c > > >> +++ b/common/dfu.c > > >> @@ -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! > > > > 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?). > > > > 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 <trini@konsulko.com> > 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@denx.de
Lukasz >> > >> > 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. >> > >> > 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 <trini@konsulko.com> Regards Ravi
Lukasz >> > >> > 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. >> > >> > 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 <trini@konsulko.com> Regards Ravi
diff --git a/common/dfu.c b/common/dfu.c index 0e9f5f5..fa77526 100644 --- a/common/dfu.c +++ b/common/dfu.c @@ -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);
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 | 3 +++ 1 file changed, 3 insertions(+)