* [PATCH] common: update_tftp: remove unnecessary build check
@ 2020-02-28 0:06 AKASHI Takahiro
2020-02-28 18:26 ` Heinrich Schuchardt
2020-05-26 16:16 ` Heinrich Schuchardt
0 siblings, 2 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2020-02-28 0:06 UTC (permalink / raw)
To: u-boot
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,
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] common: update_tftp: remove unnecessary build check
2020-02-28 0:06 [PATCH] common: update_tftp: remove unnecessary build check AKASHI Takahiro
@ 2020-02-28 18:26 ` Heinrich Schuchardt
2020-03-02 0:11 ` AKASHI Takahiro
2020-05-26 16:16 ` Heinrich Schuchardt
1 sibling, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2020-02-28 18:26 UTC (permalink / raw)
To: u-boot
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] common: update_tftp: remove unnecessary build check
2020-02-28 18:26 ` Heinrich Schuchardt
@ 2020-03-02 0:11 ` AKASHI Takahiro
2020-03-02 7:12 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2020-03-02 0:11 UTC (permalink / raw)
To: u-boot
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,
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] common: update_tftp: remove unnecessary build check
2020-03-02 0:11 ` AKASHI Takahiro
@ 2020-03-02 7:12 ` Heinrich Schuchardt
2020-03-02 7:53 ` AKASHI Takahiro
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2020-03-02 7:12 UTC (permalink / raw)
To: u-boot
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,
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] common: update_tftp: remove unnecessary build check
2020-03-02 7:12 ` Heinrich Schuchardt
@ 2020-03-02 7:53 ` AKASHI Takahiro
0 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2020-03-02 7:53 UTC (permalink / raw)
To: u-boot
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,
> > > >
> > >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] common: update_tftp: remove unnecessary build check
2020-02-28 0:06 [PATCH] common: update_tftp: remove unnecessary build check AKASHI Takahiro
2020-02-28 18:26 ` Heinrich Schuchardt
@ 2020-05-26 16:16 ` Heinrich Schuchardt
1 sibling, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2020-05-26 16:16 UTC (permalink / raw)
To: u-boot
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-26 16:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 0:06 [PATCH] common: update_tftp: remove unnecessary build check AKASHI Takahiro
2020-02-28 18:26 ` Heinrich Schuchardt
2020-03-02 0:11 ` AKASHI Takahiro
2020-03-02 7:12 ` Heinrich Schuchardt
2020-03-02 7:53 ` AKASHI Takahiro
2020-05-26 16:16 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.