| Message ID | 6C6B28D4DC342643927BEAFCE8707BF6C0B7FD39@DBDE04.ent.ti.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
On Fri, May 12, 2017 at 08:44:31AM +0000, B, Ravi wrote: > Hi Tom > > Sorry for late response, some how missed this mail. > > >> > >> obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o > >> +ifndef CONFIG_SPL_BUILD > >> obj-$(CONFIG_DFU_MMC) += dfu_mmc.o > >> +endif > >> +obj-$(CONFIG_SPL_DFU_MMC) += dfu_mmc.o > > >This becomes obj-$(CONFIG_$(SPL_)DFU_MMC) += dfu_mmc.o > > >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index > >> 926ccbd..ba509db 100644 > >> --- a/drivers/dfu/dfu_mmc.c > >> +++ b/drivers/dfu/dfu_mmc.c > > [snip] > >> @@ -154,7 +155,11 @@ static int mmc_file_op(enum dfu_op op, struct > >> dfu_entity *dfu, > >> > >> debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > >> > >> +#if CONFIG_IS_ENABLED(DFU_MMC) > >> + ret = cli_simple_run_command(cmd_buf, 0); #else > >> ret = run_command(cmd_buf, 0); > >> +#endif > > >This doesn't make sense. CONFIG_IS_ENABLED(DFU_MMC) is true for CONFIG_DFU_MMC or CONFIG_SPL_DFU_MMC. Thanks! > > True, My bad, I realized it lately after posting the patch. > > I will use run_command() only, which abstrace use of both > simple_cli_xx() and hush_parser. > > Since cli_hush.c is compile out for SPL to reduce the size. SPL must > use simple_cli_xx(). > Since by default CONFIG_HUSH_PARSER is defined for both SPL/U-BOOT, > this leads to compile error. I need to fix this way. We keep running into problems due to trying to whack in what to do in DFU via command rather than via API. Can you make an attempt to convert things over, in both SPL and not SPL, in DFU to using ABI instead, to see if we can get the size reduction here still, and not have to try and put fragile to other use cases ifdefs in common code? Thanks!
Hi Tom >> >> obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o >> >> +ifndef CONFIG_SPL_BUILD >> >> obj-$(CONFIG_DFU_MMC) += dfu_mmc.o >> >> +endif >> >> +obj-$(CONFIG_SPL_DFU_MMC) += dfu_mmc.o >> >> >This becomes obj-$(CONFIG_$(SPL_)DFU_MMC) += dfu_mmc.o >> >> >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index >> >> 926ccbd..ba509db 100644 >> >> --- a/drivers/dfu/dfu_mmc.c >> >> +++ b/drivers/dfu/dfu_mmc.c >> > [snip] >> >> @@ -154,7 +155,11 @@ static int mmc_file_op(enum dfu_op op, struct >> >> dfu_entity *dfu, >> >> >> >> debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); >> >> >> >> +#if CONFIG_IS_ENABLED(DFU_MMC) >> >> + ret = cli_simple_run_command(cmd_buf, 0); #else >> >> ret = run_command(cmd_buf, 0); >> >> +#endif >> >> >This doesn't make sense. CONFIG_IS_ENABLED(DFU_MMC) is true for CONFIG_DFU_MMC or CONFIG_SPL_DFU_MMC. Thanks! >> >> True, My bad, I realized it lately after posting the patch. >> >> I will use run_command() only, which abstrace use of both >> simple_cli_xx() and hush_parser. >> >> Since cli_hush.c is compile out for SPL to reduce the size. SPL must >> use simple_cli_xx(). >> Since by default CONFIG_HUSH_PARSER is defined for both SPL/U-BOOT, >> this leads to compile error. I need to fix this way. >We keep running into problems due to trying to whack in what to do in DFU via command rather than via API. Can you make an attempt to convert things over, in both SPL and not SPL, in DFU to using ABI instead, to see if we can get the size reduction here still, and >not have to try and put fragile to other use cases ifdefs in common code? Thanks! Use of direct mmc read/write API instead of using the run_command() is not feasible in this case. As the DFU does read/write to mmc through fat/ext4 files system, uses "fatload/fatwrite" or "ext4load/ext4write" command to perform read/write to specific partition of mmc device. I think use of run_command() is more appropriate. Regards Ravi
On Wed, May 24, 2017 at 12:11:57PM +0000, B, Ravi wrote: > Hi Tom > > >> >> obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o > >> >> +ifndef CONFIG_SPL_BUILD > >> >> obj-$(CONFIG_DFU_MMC) += dfu_mmc.o > >> >> +endif > >> >> +obj-$(CONFIG_SPL_DFU_MMC) += dfu_mmc.o > >> > >> >This becomes obj-$(CONFIG_$(SPL_)DFU_MMC) += dfu_mmc.o > >> > >> >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index > >> >> 926ccbd..ba509db 100644 > >> >> --- a/drivers/dfu/dfu_mmc.c > >> >> +++ b/drivers/dfu/dfu_mmc.c > >> > [snip] > >> >> @@ -154,7 +155,11 @@ static int mmc_file_op(enum dfu_op op, struct > >> >> dfu_entity *dfu, > >> >> > >> >> debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > >> >> > >> >> +#if CONFIG_IS_ENABLED(DFU_MMC) > >> >> + ret = cli_simple_run_command(cmd_buf, 0); #else > >> >> ret = run_command(cmd_buf, 0); > >> >> +#endif > >> > >> >This doesn't make sense. CONFIG_IS_ENABLED(DFU_MMC) is true for CONFIG_DFU_MMC or CONFIG_SPL_DFU_MMC. Thanks! > >> > >> True, My bad, I realized it lately after posting the patch. > >> > >> I will use run_command() only, which abstrace use of both > >> simple_cli_xx() and hush_parser. > >> > >> Since cli_hush.c is compile out for SPL to reduce the size. SPL must > >> use simple_cli_xx(). > >> Since by default CONFIG_HUSH_PARSER is defined for both SPL/U-BOOT, > >> this leads to compile error. I need to fix this way. > > >We keep running into problems due to trying to whack in what to do in DFU via command rather than via API. Can you make an attempt to convert things over, in both SPL and not SPL, in DFU to using ABI instead, to see if we can get the size reduction here still, and >not have to try and put fragile to other use cases ifdefs in common code? Thanks! > > Use of direct mmc read/write API instead of using the run_command() is not feasible in this case. As the DFU does read/write to mmc through fat/ext4 files system, uses "fatload/fatwrite" or "ext4load/ext4write" command to perform read/write to specific partition of mmc device. I think use of run_command() is more appropriate. OK. Can you go back to https://patchwork.ozlabs.org/patch/758485/ and clean that up as I was saying? We should be able to call cli_simple_run_command in all cases and then not need cli_hush.c, yes? And we can then move common/cli.c into being compiled only in the non-SPL case I think (but use travis-ci to confirm). Thanks!
Hi Tom >> >> >This doesn't make sense. CONFIG_IS_ENABLED(DFU_MMC) is true for CONFIG_DFU_MMC or CONFIG_SPL_DFU_MMC. Thanks! >> >> >> >> True, My bad, I realized it lately after posting the patch. >> >> >> >> I will use run_command() only, which abstrace use of both >> >> simple_cli_xx() and hush_parser. >> >> >> >> Since cli_hush.c is compile out for SPL to reduce the size. SPL >> >> must use simple_cli_xx(). >> >> Since by default CONFIG_HUSH_PARSER is defined for both SPL/U-BOOT, >> >> this leads to compile error. I need to fix this way. >> >> >We keep running into problems due to trying to whack in what to do in DFU via command rather than via API. Can you make an attempt to convert things over, in both SPL and not SPL, in DFU to using ABI instead, to see if we can get the size reduction here still, >and >not have to try and put fragile to other use cases ifdefs in common code? Thanks! >> >> Use of direct mmc read/write API instead of using the run_command() is not feasible in this case. As the DFU does read/write to mmc through fat/ext4 files system, uses "fatload/fatwrite" or "ext4load/ext4write" command to perform read/write to specific >partition of mmc device. I think use of run_command() is more appropriate. >OK. Can you go back to https://patchwork.ozlabs.org/patch/758485/ and clean that up as I was saying? We should be able to call cli_simple_run_command in all cases and then not need cli_hush.c, yes? >And we can then move common/cli.c into being compiled only in the non-SPL case I think (but use travis-ci to confirm). Thanks! Ok, will do. Thanks. In latest mainline, dfu has broken due to commit 1a9a5f7a: usb: host: xhci-omap: fix double weak board_usb_init functions. Due to this DFU gadget mode fails, due to weak board_usb_init() gets called which does not initialize the usb controller. I will fix this as well and send the patch. Regards Ravi
diff --git a/common/cli.c b/common/cli.c index a433ef2..5053f8a 100644 --- a/common/cli.c +++ b/common/cli.c @@ -28,7 +28,7 @@ DECLARE_GLOBAL_DATA_PTR; */ int run_command(const char *cmd, int flag) { -#ifndef CONFIG_HUSH_PARSER +#if !defined(CONFIG_HUSH_PARSER) || defined(CONFIG_SPL_BUILD) /* * cli_run_command can return 0 or 1 for success, so clean up * its result.