[U-Boot] [PATCH 1/8] doc: dfu: tftp: README entry for TFTP extension of DFU

[Joe Hershberger]

Hi Lukasz,

On Mon, Jul 20, 2015 at 1:59 PM, Lukasz Majewski <l.majewski at majess.pl> wrote:
> Hi Joe,
>
>> Hi Lukasz,
>>
>> On Thu, Jul 16, 2015 at 2:59 PM, Lukasz Majewski
>> <l.majewski at majess.pl> wrote:
>> > Hi Joe,
>> >
>> > Thank you for your review.
>> >
>> >> Hi Lukasz,
>> >>
>> >> On Sun, Jul 12, 2015 at 10:30 AM, Lukasz Majewski
>> >> <l.majewski at majess.pl> wrote:
>> >> > Documentation file for DFU extension. With this functionality it
>> >> > is now possible to transfer FIT images with firmware updates via
>> >> > TFTP and use DFU backend for storing them.
>> >> >
>> >> > Signed-off-by: Lukasz Majewski <l.majewski at majess.pl>
>> >> > ---
>> >> >  doc/README.dfutftp | 153
>> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
>> >> > changed, 153 insertions(+) create mode 100644 doc/README.dfutftp
>> >> >
>> >> > diff --git a/doc/README.dfutftp b/doc/README.dfutftp
>> >> > new file mode 100644
>> >> > index 0000000..4636321
>> >> > --- /dev/null
>> >> > +++ b/doc/README.dfutftp
>> >> > @@ -0,0 +1,153 @@
>> >> > +Device Firmware Upgrade (DFU) - extension to use TFTP
>> >> > +=====================================================
>> >> > +
>> >> > +Why?
>> >> > +----
>> >> > +
>> >> > +* Update TFTP (CONFIG_UPDATE_TFTP) only supports writing
>> >> > +code to NAND memory via TFTP.
>> >> > +* DFU supports writing data to variety of mediums (NAND,
>> >> > +eMMC, SD, partitions, RAM, etc) via USB.
>> >> > +
>> >> > +Combination of both solves their shortcomings!
>> >> > +
>> >> > +
>> >> > +Overview
>> >> > +--------
>> >> > +
>> >> > +This document briefly describes how to use BF for
>> >>
>> >> BF?
>> >
>> > It should be DFU.
>> >
>> >>
>> >> > +upgrading firmware (e.g. kernel, u-boot, rootfs, etc.)
>> >> > +via TFTP protocol.
>> >> > +
>> >> > +By using Ethernet (TFTP protocol to be precise) it was
>> >> > +possible to overcome the major problem of USB based DFU -
>> >> > +the relatively low transfer speed for large files.
>> >> > +This was caused by DFU standard, which imposed utilization
>> >> > +of only EP0 for transfer. By using Ethernet we can circumvent
>> >> > +this shortcoming.
>> >> > +
>> >> > +Beagle Bone Black (BBB) powered by TI's am335x CPU has been used
>> >> > +as a demo board.
>> >> > +
>> >> > +To utilize this feature, one needs to first enable support
>> >> > +for USB based DFU (CONFIG_DFU_*) and TFTP update
>> >> > +(CONFIG_UPDATE_TFTP) described in ./doc/README.update.
>> >>
>> >> Does it really make sense to reuse this UPDATE_TFTP config? Why not
>> >> DFU_TFTP?
>> >
>> > Enabling CONFIG_UPDATE_TFTP allows reusing parts of the legacy
>> > update_tftp() code.
>>
>> I can understand reusing the code, but that doesn't mean we should
>> force both complete features to be included.
>
> The legacy ./common/update.c was using two flags - namely
> CONFIG_UPDATE_TFTP (to allow compilation of this file) and
> CONFIG_SYS_NO_FLASH. Lack of the latter was using #error to terminate a
> compilation.

It seems the CONFIG_SYS_NO_FLASH is simply making sure that some other
required feature is included. It wasn't actually selecting this
feature.

The CONFIG_UPDATE_TFTP was actually enabling the feature. It seems
that should remain the case.

> That was the old behavior - one needed to define both flags to compile
> in the legacy code.

You actually were required to have *not* defined CONFIG_SYS_NO_FLASH.

> Moreover the CONFIG_UPDATE_TFTP is used to enable running
> update_tftp() in the early boot stage.

Yes... That could probably be left as is for now.

> My changes in the ./common/update.c file have split the code to common
> one (enabled by CONFIG_UPDATE_TFTP) and legacy one (NAND support
> specific) enabled by CONFIG_SYS_NO_FLASH.

I think you need to make the common code be enabled by seeing either
CONFIG_UPDATE_TFTP or CONFIG_DFU_TFTP.

You also need to have a check that fails if CONFIG_UPDATE_TFTP &&
CONFIG_SYS_NO_FLASH. That preserves old behavior.

>> > What I mean is that one should enable legacy CONFIG_UPDATE_TFTP as a
>> > prerequisite for using dfu tftp transfer.
>>
>> What if a new config like DFU_TFTP enabled the shared code, but not
>> the old behaviors.
>
> DFU_TFTP enables dfu specific code - not the shared code
> (at ./common/update.c).

It should also enable the shared code.

> Shared code is enabled by CONFIG_UPDATE_TFTP.

It should enable the shared code as well as the legacy-specific behavior.

>> >> > +New "dfutftp" command has been introduced to comply with present
>> >> > +USB related commands' syntax.
>> >> > +
>> >> > +This code works without "fitupd" command enabled.
>> >> > +
>> >> > +As of this writing
>> >> > (SHA1:241746e618fa725491e9ff689710c5f73ffd9139) the +update.c
>> >> > code is not enabled (CONFIG_UPDATE_TFTP) by any board in the
>> >> > +contemporary u-boot tree.
>> >>
>> >> So maybe we should remove it.
>> >
>> > This is IMHO a tricky issue. On the one hand there hasn't left any
>> > board supporting this feature in mainline (recently some old PPC
>> > boards have been removed from u-boot).
>> >
>> > One the other hand _probably_ there are deployed systems (as a
>> > derivative of the boards supported in u-boot and using this feature)
>> > which depend on this feature.
>>
>> That's fair.
>>
>> > I'd opt for leaving the original code in the tree with a fat big
>> > note about the plan to remove the legacy code in a near future (as
>> > we do it with MAKEALL script).
>>
>> OK
>>
>> >> > +
>> >> > +Environment variables
>> >> > +---------------------
>> >> > +
>> >> > +To preserve legacy behavior of TFTP update (./common/update.c)
>> >> > +code following new environment variables had been introduced:
>> >>
>> >> Another example of why you should use a new config instead of the
>> >> existing one.
>> >
>> > Could you be more specific here?
>>
>> You are adding magic env vars here because you want to have both old
>> and new behaviors at once. I think it would be better to select one at
>> build time and not add any magic.
>
> In theory you are right. In practice it is a bit more complicated - I
> will try to explain it latter in this e-mail.
>
>>
>> >> > +* "update_tftp_exec_at_boot" ["true"|"false"]
>> >> > +
>> >> > +  New usage of update_tftp allows setting the
>> >> > +  "update_tftp_exec_at_boot" env variable to allow it running
>> >> > +  at boot time.
>> >> > +  In this way update_tftp will not be executed at startup and
>> >> > reduce
>> >> > +  boot time.
>> >> > +
>> >> > +  To preserve backward compatibility for legacy boards this
>> >> > variable
>> >> > +  should be set to "true".
>> >> > +
>> >> > +  BBB note:
>> >> > +           When update tftp is not working after boot one need
>> >> > to
>> >> > +           increase values of following two configs:
>> >> > +           CONFIG_UPDATE_TFTP_MSEC_MAX and
>> >> > CONFIG_UPDATE_TFTP_CNT_MAX.
>> >> > +           Values of namely 1000 and 5 solve the issue for BBB.
>> >> > +
>> >> > +* "update_tftp_dfu" ["true"|"false"]
>> >> > +
>> >> > +  By "update_tftp_dfu" env variable we inform update_tftp that
>> >> > +  it should use dfu for writing data - in this way support for
>> >> > +  legacy usage is preserved.
>> >>
>> >> Same here... presumably a user only wants support for one or the
>> >> other compiled in. Please use a different config.
>> >
>> > The most appealing thing about update.c is that I had to add only a
>> > few lines of code to use it for my purpose. For that reason I've
>> > decided to keep as much as possible of the legacy code.
>> >
>> >>
>> >> > +* "update_tftp_dfu_interface" ["mmc"]
>> >> > +* "update_tftp_dfu_devstring" ["1"]
>> >> > +
>> >> > +  Both variables - namely
>> >> > "update_tftp_dfu_{interface|devstring}" are
>> >> > +  only taken into account when "update_tftp_dfu" is defined.
>> >> > +  They store information about interface (e.g. "mmc") and device
>> >> > number
>> >> > +  string (e.g. "1").
>> >>
>> >> It is preferable to make these parameters to a command and not
>> >> magic env variables.
>> >
>> > They can be specified in the 'dfutftp' command - which syntax is
>> > similar to 'dfu' (e.g. dfutftp 0 mmc 1).
>>
>> OK, great.
>>
>> > However, those variables are needed for automatic update after power
>> > up. In other words update_tftp() function is called very early in
>> > boot and env variables are a convenient way to specify the
>> > interface and its number.
>>
>> So this is only called early in the case of the old functionality.
>> Your new DFU features do not need an early automatic feature, right?
>
> I'm afraid that they will require some kind of update in the early
> boot state.

You should not need more than already existed for legacy, I think. So
you should not be adding any magic env vars.

>> In fact, I would argue that the old method didn't need it either and
>> never should have supported that. Things like this should simply be
>> part of the board's preboot script so we don't need this type of magic
>> stuff.
>
> Please correct me if I'm wrong.
>
> I should use another CONFIG_* to enable using common update_tftp()
> function. In this way I would avoid calling it in the early boot code.

Explained above.

> Instead, I should define proper "preboot" env variable with "dfu tftp 0
> mmc 1" command and use it for SW updating.

Correct. The preboot command should call normal commands and pass parameters.

>> >> > +  In the "dfutftp" command they are explicitly overridden.
>> >> > +  It is done deliberately, since in this command we may use
>> >> > arbitrary values. +
>> >> > +  Default values, when available during boot, are used by
>> >> > update_tftp
>> >> > +  (when dfu support is enabled) to properly setup medium device
>> >> > +  (e.g. mmc 1).
>> >> > +
>> >> > +
>> >> > +
>> >> > +Beagle Bone Black (BBB) setup
>> >> > +-----------------------------
>> >> > +
>> >> > +1. Setup tftp env variables:
>> >> > +   *  select desired eth device - 'ethact' variable
>> >> > ["ethact=cpsw"]
>> >> > +      (use "bdinfo" to check current setting)
>> >> > +   *  setup "serverip" and "ipaddr" variables
>> >> > +   *  set "loadaddr" as a fixed buffer where incoming data is
>> >> > placed
>> >> > +      ["loadaddr=0x81000000"]
>> >> > +
>> >> > +#########
>> >> > +# BONUS #
>> >> > +#########
>> >> > +It is possible to use USB interface to emulate ETH connection by
>> >> > setting +"ethact=usb_ether". In this way one can have very fast
>> >> > DFU transfer via USB.
>> >>
>> >> Is thor not faster than DFU?
>> >
>> > Yes, it is. However, DFU is standardized (which is despite of its
>> > low speed its huge advantage) - thor not.
>> >
>> >>
>> >> It seems like DFU should support a bulk endpoint if performance is
>> >> an issue, right?
>> >
>> > Yes, it should, but such modification would be not compliant with
>> > the standard.
>>
>> Who is the standards body? Can they accept suggestions for the next
>> revision? Is it worth trying to improve the standard?
>
> The last revision of DFU standard is from 2006 with Greg KH being a
> notable USB committee board member :-)
>
> I've asked him about the possibility to revise the standard but he
> replied that chances are small since Linux Foundation is not part
> of the USB standard committee anymore.

That's a shame. Oh well.

>> >>That would be more efficient than emulating Ethernet.
>> >
>> > To be more precise - I've combined the ability to use Ethernet with
>> > DFU flashing backend.
>> >
>> > In this way boards only equipped with ETH can (re)use DFU code to
>> > flash data (on MMC, NAND, filesystems, RAM, etc).
>>
>> Yes, that's very good. I was simply talking about the "BONUS" where
>> emulated Ethernet over USB is used. In that case it would be more
>> efficient to actually have a raw bulk endpoint supported by DFU.
>
> Yes, it would. Unfortunately I think that it would be very hard to
> revise the DFU standard.
>
> ETH over USB can be used on devices equipped only with USB (like
> trats/trats2 devel mobile phones). In that way DFU speed would increase.

Sure. Sounds good.

>> >> > +For 33MiB test image the transfer rate is 1MiB/s
>> >> > +
>> >> > +2. Setup update_tftp variables:
>> >> > +   *  set "updatefile" - the file name to be downloaded via TFTP
>> >> > (stored on
>> >> > +      the HOST at e.g. /srv/tftp)
>> >> > +
>> >> > +3. Setup dfutftp specific variables (as explained in
>> >> > "Environment Variables")
>> >> > +   * "update_tftp_exec_at_boot=true"
>> >> > +   * "update_tftp_dfu=true"
>> >> > +
>> >> > +4. Inspect "dfu" specific variables:
>> >> > +   * "dfu_alt_info" - information about available DFU entities
>> >> > +   * "dfu_bufsiz"   - variable to set buffer size [in bytes] -
>> >> > when it is not
>> >> > +                     possible to set large enough default buffer
>> >> > (8 MiB @ BBB) +
>> >> > +
>> >> > +
>> >> > +FIT image format for download
>> >> > +-----------------------------
>> >> > +
>> >> > +To create FIT image for download one should follow the update
>> >> > tftp README file +(./doc/README.update) with one notable
>> >> > difference: +
>> >> > +The original snippet of ./doc/uImage.FIT/update_uboot.its
>> >> > +
>> >> > +       images {
>> >> > +               update at 1 {
>> >> > +                       description = "U-Boot binary";
>> >> > +
>> >> > +should look like
>> >> > +
>> >> > +       images {
>> >> > +               u-boot.bin at 1 {
>> >> > +                       description = "U-Boot binary";
>> >> > +
>> >> > +where "u-boot.bin" is the DFU entity name to be stored.
>> >> > +
>> >> > +
>> >> > +
>> >> > +To do
>> >> > +-----
>> >> > +
>> >> > +* Extend dfu-util command (or write new one - e.g. dfueth-util)
>> >> > to support
>> >> > +  TFTP based transfers
>> >> > +
>> >> > +* Upload support (via TFTP)
>> >> > \ No newline at end of file
>> >> > --
>> >> > 2.1.4
>> >> >
>> >> > _______________________________________________
>> >> > U-Boot mailing list
>> >> > U-Boot at lists.denx.de
>> >> > http://lists.denx.de/mailman/listinfo/u-boot
>> >
>> > Best regards,
>> >
>> > Lukasz Majewski
>
> Best regards,
> Lukasz Majewski