Hi Krishna,

Hi,

Thanks for your time,

Please find following comments:

...

1. Current solution (u-boot):

• It is possible to specify your own "dfu_alt_info" environment variable when you call dfu command.

2. Speed improvement - section "Making it faster" from:

http://codelectron.wordpress.com/2014/02/28/flexible-firmware-upgrade/

• I've noticed that you are referring to following dfu-util version:

dfu-util - (C) 2007 by Openmoko Inc.

It seems to be a bit old one.

I just used the content from my old documentation, But im referring to current implementation.

...

The newest GIT repo can be found at: git://gitorious.org/dfu-util/dfu-util.git

I'm referring to following SHA1 (master branch): bda43a52a6c5e9dcd159236553b0d5c511616e77

The code at dfuload_do_dnload() function is already rewritten to only wait: milli_sleep(dst.bwPollTimeout);

which is correct, since non-zero values are explicitly specified in u-boot (when e.g. target expects that eMMC data will be written).

It seems that the proposed improvement is already implemented.

I dont think its implemented, you can refer here, its the block of code what I am referring to,

https://gitorious.org/dfu-util/dfu-util/source/bda43a52a6c5e9dcd159236553b0d... https://gitorious.org/dfu-util/dfu-util/source/bda43a52a6c5e9dcd159236553b0d...

Ok, so there is the current code:

do { ret = dfu_get_status(dif, &dst); if (ret < 0) { errx(EX_IOERR, "Error during download get_status"); goto out_free; }

if (dst.bState == DFU_STATE_dfuDNLOAD_IDLE || dst.bState == DFU_STATE_dfuERROR) break;

/* Wait while device executes flashing */ milli_sleep(dst.bwPollTimeout); } while (1);

And the code you refer in your webpage: http://codelectron.wordpress.com/2014/02/28/flexible-firmware-upgrade/ do { ret = dfu_get_status(dif->dev_handle, dif->interface, &dst); if (ret < 0) { fprintf(stderr, “Error during download get_status\n”); goto out_free; } if (dst.bState == DFU_STATE_dfuDNLOAD_IDLE || dst.bState == DFU_STATE_dfuERROR) break; /* Wait while device executes flashing */ if (quirks & QUIRK_POLLTIMEOUT) milli_sleep(DEFAULT_POLLTIMEOUT); else milli_sleep(dst.bwPollTimeout); } while (1);

I'm a bit confused here, since it looks like the milli_sleep(DEFAULT_POLLTIMEOUT); is already removed.

Am I missing something?

...

As a side note: To improve transmission speed we were even trying to increase the EP0 packet size on HOST machine. Unfortunately it has some limitations (4 KiB if I'm not wrong).

Yes I do think so about the limitation, as a standard EP0 should support atleast 64 bytes and DFU is based on that.

...

Anyway, I'm open for potential DFU transmission speed improvements.

3. Flexibility improvement - section "Making it flexible"

The idea of adding headers seems appealing - but there are as usual some corner cases:

• Backward compatibility - dfu-util and dfu use number of alt

settings to chose the memory region to be written. That is why you observe a lot of alt=X when you type dfu-util -l. We would need some extra switch in the dfu-util to support yours header based approach.

• In u-boot we would like to have some degree of control of where

and if we flash data. I personally like to specify beforehand (in envs) what parts of memory are available for flashing.

I developed it keeping in mind a situation in field where there can be a complete change in memory organisation.

Maybe some special alt setting could be defined for such a behavior in u-boot? We can think about that if it solves a real problem.

...

• Some more complicated schemes shall be considered as well. I've

got some pending patches to support eMMC's bootparts flashing via DFU.

• Also the header itself only assumes NAND being the backing

memory. But we now also alter content of eMMC and RAM with DFU. This is missing.

• The information provided by headers is already stored at "dfu_alt_info" environment variable. It is also used by other

gadgets.

It was an example code based on what I had implemented some time back. It was based on nand flash, but the idea will be the same for any other type.

I just wanted to share the idea of a different type of implementation of USB DFU.

I'm happy, that you had shared this fresh view.

Do you have any other ideas for improvements?

Krishna www.codelectron.com

[Cc'ing the dfu-util list instead of myself, full thread on http://u-boot.10912.n7.nabble.com/A-proposal-hack-for-an-efficient-USB-DFU-f...]

On Fri, Mar 14, 2014 at 11:42 AM, Krishna Pattabiraman wrote:

On Fri, Mar 14, 2014 at 9:52 AM, Lukasz Majewski wrote:

...

Ok, so there is the current code:

    do {
        ret = dfu_get_status(dif, &dst);
        if (ret < 0) {
            errx(EX_IOERR, "Error during download get_status");
            goto out_free;
        }

        if (dst.bState == DFU_STATE_dfuDNLOAD_IDLE ||
                dst.bState == DFU_STATE_dfuERROR)
            break;

        /* Wait while device executes flashing */
        milli_sleep(dst.bwPollTimeout);
    } while (1);

And the code you refer in your webpage: http://codelectron.wordpress.com/2014/02/28/flexible-firmware-upgrade/ do { ret = dfu_get_status(dif->dev_handle, dif->interface, &dst); if (ret < 0) { fprintf(stderr, "Error during download get_status\n"); goto out_free; } if (dst.bState == DFU_STATE_dfuDNLOAD_IDLE || dst.bState == DFU_STATE_dfuERROR) break; /* Wait while device executes flashing */ if (quirks & QUIRK_POLLTIMEOUT) milli_sleep(DEFAULT_POLLTIMEOUT); else milli_sleep(dst.bwPollTimeout); } while (1);

I'm a bit confused here, since it looks like the milli_sleep(DEFAULT_POLLTIMEOUT); is already removed.

Am I missing something?

The DEFAULT_POLLTIMEOUT is now applied inside dfu_get_status() so that is as before. However this default value is only used for a few devices with explicit quirks applied in dfu-util. For u-boot the bwPollTimeout value returned by the boot loader is used.

No not in the code. Yes they are different code but what I am speaking about in the blog is the removal of the whole block and removing the corresponding states in u-boot also. I have also given the reasons for it in the blog.

Then it would not be DFU any longer. The DFU standard mandates a DFU_GETSTATUS request after the DFU_DNLOAD request, and that the host should wait bwPollTimeout milliseconds before sending requests to the device again*. If u-boot does not need any time on its own here, it should return a zero bwPollTimeout.

*) The standard says "wait between the status phase of the next DFU_DNLOAD and the subsequent solicitation of the device's status via DFU_GETSTATUS", however many devices only initiates the actual flashing/erasing/etc after the DFU_GETSTATUS requests and can therefore set this value specifically for the current request.

Regards, Tormod

...

I developed it keeping in mind a situation in field where there can be a complete change in memory organisation.

...

Maybe some special alt setting could be defined for such a behavior in u-boot? We can think about that if it solves a real problem.

Its a difference between a partition table style vs free style. My reason for choosing the latter was

1. The partition table is actually inside the linux kernel and to change

partition, we need upgrade to u-boot-env as well as Linux kernel. Risk of bricking was higher. Like here http://lxr.free-electrons.com/source/arch/arm/mach-omap2/board-omap3beagle.c...

2. I used SAM9G45 and for normal booting of Linux and U-boot for upgrade and

other maintanence purpose. http://free-electrons.com/blog/at91bootstrap-linux/ .

I haven't considered other types of storage devices and boards, but I think that the concept is very same for other devices too.

...

I'm happy, that you had shared this fresh view.

Do you have any other ideas for improvements?

Yes, When you plug the DFU enabled device to PC as per standard it should show as Device Firware Updrade in the Device class. Device manager in windows and any similar appplication in other OS will show that. I haven't checked the current implementation in U-boot. It can be done by changing the configuration descriptor in the USB driver. The class is FE and sub class is 01 you can refer here http://www.usb.org/developers/defined_class/#BaseClassFEh .

Thank you for your time.

Krishna www.codelectron.com