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

[Joe Hershberger]

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.

> 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.

>> > +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.

>> > +* "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?

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.

>> > +  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?

>>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.

>> > +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