[PATCH] common: update_tftp: remove unnecessary build check
Logically, the current update_tftp() should and does compile and work correctly even without satisfying the following condition:
#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) #error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" #endif
It would be better to just drop it so that this function will be used on wider range of platforms.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- common/update.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/common/update.c b/common/update.c index c8dd346a0956..ade029851dbd 100644 --- a/common/update.c +++ b/common/update.c @@ -14,10 +14,6 @@ #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature" #endif
-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" -#endif - #include <command.h> #include <env.h> #include <flash.h> @@ -210,8 +206,9 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size) printf("Error: could not protect flash sectors\n"); return 1; } +#else + return -1; #endif - return 0; }
static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
On 2/28/20 1:06 AM, AKASHI Takahiro wrote:
Logically, the current update_tftp() should and does compile and work correctly even without satisfying the following condition:
#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) #error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" #endif
It would be better to just drop it so that this function will be used on wider range of platforms.
Function update_tftp() calls update_flash(). update_flash() does nothing if CONFIG_MTD_NOR_FLASH=n.
Please, describe which configuration would be additionally usable if your patch is applied.
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
common/update.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/common/update.c b/common/update.c index c8dd346a0956..ade029851dbd 100644 --- a/common/update.c +++ b/common/update.c @@ -14,10 +14,6 @@ #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature" #endif
-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" -#endif
- #include <command.h> #include <env.h> #include <flash.h>
@@ -210,8 +206,9 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size) printf("Error: could not protect flash sectors\n"); return 1; } +#else
- return -1; #endif
return 0; }
static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
On Fri, Feb 28, 2020 at 07:26:25PM +0100, Heinrich Schuchardt wrote:
On 2/28/20 1:06 AM, AKASHI Takahiro wrote:
Logically, the current update_tftp() should and does compile and work correctly even without satisfying the following condition:
#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) #error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" #endif
It would be better to just drop it so that this function will be used on wider range of platforms.
Function update_tftp() calls update_flash(). update_flash() does nothing if CONFIG_MTD_NOR_FLASH=n.
Please, describe which configuration would be additionally usable if your patch is applied.
update_tftp() takes additional two parameters, interface and devstring, since the commit c7ff5528439a ("update: tftp: dfu: Extend update_tftp() function to support DFU"). CONFIG_DFU and COFIG_DFU_XXX, without MTD_NOR_FLASH, does work.
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
common/update.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/common/update.c b/common/update.c index c8dd346a0956..ade029851dbd 100644 --- a/common/update.c +++ b/common/update.c @@ -14,10 +14,6 @@ #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature" #endif
-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" -#endif
- #include <command.h> #include <env.h> #include <flash.h>
@@ -210,8 +206,9 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size) printf("Error: could not protect flash sectors\n"); return 1; } +#else
- return -1; #endif
return 0; }
static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
On 3/2/20 1:11 AM, AKASHI Takahiro wrote:
On Fri, Feb 28, 2020 at 07:26:25PM +0100, Heinrich Schuchardt wrote:
On 2/28/20 1:06 AM, AKASHI Takahiro wrote:
Logically, the current update_tftp() should and does compile and work correctly even without satisfying the following condition:
#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) #error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" #endif
It would be better to just drop it so that this function will be used on wider range of platforms.
Function update_tftp() calls update_flash(). update_flash() does nothing if CONFIG_MTD_NOR_FLASH=n.
Please, describe which configuration would be additionally usable if your patch is applied.
update_tftp() takes additional two parameters, interface and devstring, since the commit c7ff5528439a ("update: tftp: dfu: Extend update_tftp() function to support DFU"). CONFIG_DFU and COFIG_DFU_XXX, without MTD_NOR_FLASH, does work.
But would you use CONFIG_UPDATE_TFTP in conjunction with CONFIG_DFU*?
common/Kconfig describes UPDATE_TFTP as: "This option allows performing update of NOR with data in fitImage sent via TFTP boot."
doc/README.dfutftp says: "* Update TFTP (CONFIG_UPDATE_TFTP) only supports writing code to NAND memory via TFTP. * DFU supports writing data to the variety of mediums (NAND, eMMC, SD, partitions, RAM, etc) via USB."
I assume README.dfutftp saying "NAND" and not "NOR" is a documentation error.
So why do you specifically need to allow CONFIG_UPDATE_TFTP=y && CONFIG_MTD_NOR_FLASH=n ?
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
common/update.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/common/update.c b/common/update.c index c8dd346a0956..ade029851dbd 100644 --- a/common/update.c +++ b/common/update.c @@ -14,10 +14,6 @@ #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature" #endif
-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" -#endif
- #include <command.h> #include <env.h> #include <flash.h>
@@ -210,8 +206,9 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size) printf("Error: could not protect flash sectors\n"); return 1; } +#else
- return -1; #endif
return 0; }
static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
On Mon, Mar 02, 2020 at 08:12:09AM +0100, Heinrich Schuchardt wrote:
On 3/2/20 1:11 AM, AKASHI Takahiro wrote:
On Fri, Feb 28, 2020 at 07:26:25PM +0100, Heinrich Schuchardt wrote:
On 2/28/20 1:06 AM, AKASHI Takahiro wrote:
Logically, the current update_tftp() should and does compile and work correctly even without satisfying the following condition:
#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) #error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" #endif
It would be better to just drop it so that this function will be used on wider range of platforms.
Function update_tftp() calls update_flash(). update_flash() does nothing if CONFIG_MTD_NOR_FLASH=n.
Please, describe which configuration would be additionally usable if your patch is applied.
update_tftp() takes additional two parameters, interface and devstring, since the commit c7ff5528439a ("update: tftp: dfu: Extend update_tftp() function to support DFU"). CONFIG_DFU and COFIG_DFU_XXX, without MTD_NOR_FLASH, does work.
But would you use CONFIG_UPDATE_TFTP in conjunction with CONFIG_DFU*?
common/update.c is only compiled under CONFIG_UPDATE_TFTP. So without CNFIG_DFU*, the commit c7ff5528439a doens't make any sense.
common/Kconfig describes UPDATE_TFTP as: "This option allows performing update of NOR with data in fitImage sent via TFTP boot."
doc/README.dfutftp says: "* Update TFTP (CONFIG_UPDATE_TFTP) only supports writing code to NAND memory via TFTP.
- DFU supports writing data to the variety of mediums (NAND,
eMMC, SD, partitions, RAM, etc) via USB."
I assume README.dfutftp saying "NAND" and not "NOR" is a documentation error.
I believe that the document should be overhauled.
So why do you specifically need to allow CONFIG_UPDATE_TFTP=y && CONFIG_MTD_NOR_FLASH=n ?
Do you remember that I'm now working on capsule update? That is why I included you in Cc.
Anyhow, Lukasz should have a comment here.
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
common/update.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/common/update.c b/common/update.c index c8dd346a0956..ade029851dbd 100644 --- a/common/update.c +++ b/common/update.c @@ -14,10 +14,6 @@ #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature" #endif
-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" -#endif
- #include <command.h> #include <env.h> #include <flash.h>
@@ -210,8 +206,9 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size) printf("Error: could not protect flash sectors\n"); return 1; } +#else
- return -1; #endif
return 0; }
static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
On 2/28/20 1:06 AM, AKASHI Takahiro wrote:
Logically, the current update_tftp() should and does compile and work correctly even without satisfying the following condition:
#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) #error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" #endif
It would be better to just drop it so that this function will be used on wider range of platforms.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
In common/main.c:37:void main_loop(void) we have a call:
if (IS_ENABLED(CONFIG_UPDATE_TFTP)) update_tftp(0UL, NULL, NULL);
When you implement UEFI capsule updates you probably are not interested in this call.
So I would suggest that you take the code after the label got_update_file and put it into a new function in a new file drivers/dfu/dfu_fit.c which is only compiled if both CONFIG_DFU and CONFIG_FIT are specified.
This way we get a clean separation between tFTP updates and capsule updates.
---
We should get rid of any config checks in code. Those checks should be in Kconfig. This should also cover the CONFIG_FIT && CONFIG_OF_LIBFDT test.
@Takahiro: Why do you want to allow CONFIG_UPDATE_TFTP=y and CONFIG_MTD_NOR_FLASH=y? In your capsule update series you use DFU. DFU_MTD depends on DFU and DM_MTD. So the flash driver called from DFU is not in common/flash.c but somewhere in drivers/mtd.
Best regards
Heinrich
common/update.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/common/update.c b/common/update.c index c8dd346a0956..ade029851dbd 100644 --- a/common/update.c +++ b/common/update.c @@ -14,10 +14,6 @@ #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature" #endif
-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH) -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour" -#endif
#include <command.h> #include <env.h> #include <flash.h> @@ -210,8 +206,9 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size) printf("Error: could not protect flash sectors\n"); return 1; } +#else
- return -1;
#endif
- return 0;
}
static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt