[U-Boot] [PATCH 2/3] usb: dfu: introduce dfuMANIFEST state

[Heiko Schocher <hs@denx.de>]

Hello Lukasz,

Am 17.03.2014 10:46, schrieb Lukasz Majewski:
> Hi Heiko,
>
>> Hello Lukasz,
>>
>> Am 14.03.2014 11:47, schrieb Lukasz Majewski:
>>> Hi Heiko,
>>>
>>>> on nand flash using ubi, after the download of the new image into
>>>> the flash, the "rest" of the nand sectors get erased while flushing
>>>> the medium. With current u-boot version dfu-util may show:
>>>>
>>>> Starting download:
>>>> [##################################################] finished!
>>>> state(7) = dfuMANIFEST, status(0) = No error condition is present
>>>> unable to read DFU status
>>>>
>>>> as get_status is not answered while erasing sectors, if erasing
>>>> needs some time.
>>>>
>>>> So do the following changes to prevent this:
>>>>
>>>> - introduce dfuManifest state
>>>>     According to dfu specification
>>>>     ( http://www.usb.org/developers/devclass_docs/usbdfu10.pdf )
>>>> section 7: "the device enters the dfuMANIFEST-SYNC state and awaits
>>>> the solicitation of the status report by the host. Upon receipt of
>>>> the anticipated DFU_GETSTATUS, the device enters the dfuMANIFEST
>>>> state, where it completes its reprogramming operations."
>>>>
>>>> - when stepping into dfuManifest state, sending a PollTimeout
>>>>     DFU_MANIFEST_POLL_TIMEOUT in ms, to the host, so the host
>>>>     (dfu-util) waits the PollTimeout before sending a get_status
>>>> again.
>>>>
>>>> Signed-off-by: Heiko Schocher<hs@denx.de>
>>>> Cc: Lukasz Majewski<l.majewski@samsung.com>
>>>> Cc: Kyungmin Park<kyungmin.park@samsung.com>
>>>> Cc: Marek Vasut<marex@denx.de>
>>>> ---
>>>>    README                     |  5 ++++
>>>>    drivers/dfu/dfu.c          |  1 -
>>>>    drivers/usb/gadget/f_dfu.c | 60
>>>> +++++++++++++++++++++++++++++++++++++++-------
>>>> include/dfu.h              |  3 +++ 4 files changed, 59
>>>> insertions(+), 10 deletions(-)
>>>>
[...]
>>>> diff --git a/drivers/usb/gadget/f_dfu.c
>>>> b/drivers/usb/gadget/f_dfu.c index a045864..f4bf918 100644
>>>> --- a/drivers/usb/gadget/f_dfu.c
>>>> +++ b/drivers/usb/gadget/f_dfu.c
[...]
>>>> @@ -463,6 +478,33 @@ static int state_dfu_manifest_sync(struct
>>>> f_dfu *f_dfu, return value;
>>>>    }
>>>>
>>>> +static int state_dfu_manifest(struct f_dfu *f_dfu,
>>>> +			      const struct usb_ctrlrequest *ctrl,
>>>> +			      struct usb_gadget *gadget,
>>>> +			      struct usb_request *req)
>>>> +{
>>>> +	int value = 0;
>>>> +
>>>> +	switch (ctrl->bRequest) {
>>>> +	case USB_REQ_DFU_GETSTATUS:
>>>> +		/* We're MainfestationTolerant */
>>>
>>> Please look into the comments globally - we do now support the
>>> DFU_STATE_dfuMANIFEST_* states
>>
>> Hmm.. Yes, but we are MainfestationTolerant ... so the comment is
>> Ok, or?
>
> My understanding of "ManifestationTolerant" is as follow:
>
> "We didn't used the dfuMANIFEST state and immediately went to dfuIDLE
> state from dfuDOWNLOAD-IDLE, so we are 'ManifestationTolerant'".

[...]
>>> I'm looking forward for v2.
>>
>> I am ready for sending it, but need response from you as I think the
>> comment is correct ...
>
> Please explain why it is correct in your opinion.

Hmm..

http://www.usb.org/developers/devclass_docs/usbdfu10.pdf on page 26
"Figure A.1 Interface state transition diagram":

State 7 bitManifestationTolerant = 1 (=ManifestationTolerant?), if
PollTimeout is reached go back to state 6, and after DFU_GET_STATUS
go to state 2

if State 7 and "bitManifestationTolerant = 0" and PollTimeout reached,
go to state 8 ... and wait there forever?

So we are in the "bitManifestationTolerant = 1" case as before my
changes ... nothing changed ...

before my changes there was immediately the transition from state 6
to state 2 and this is only possible if "bitManifestationTolerant = 1"
is set ... I only added the dfuMANIFEST state, not changed the
"bitManifestationTolerant = 1"

I interpretate 'ManifestationTolerant' as:
multiple dfu downloads are possible, no need for reseting the device
after a download was successful ...

>> BTW:
>> With the DFU_MANIFEST_POLL_TIMEOUT solution, we have a static timeout,
>> which is suboptimal, as we have a big MTD partittion and we must have
>> a "big" timeout for erasing (in the worst case) the complete
>> partition. Also this big timeout is (on nand using ubi) only needed
>> for the nand ubi partiton, not for the raw partitions containing for
>> example u-boot ... where it could be 0 ...
>
> So, the problem is with setting a worst case value for all kinds of
> your dfu entities?

Yes. At least each dfu entity needs an own Timeout ... think of
an 1MiB Partition and a 900MiB Partition on one device ... we
currently use the Polltimeout for the 900MiB Partition also when
updating the 1 MiB Partition ...

>> Better would be, to have the information how long does it take to
>> erase one sector (define?) and how many sectors we have to erase, and
>> sending this value to the host... Do you see a chance to get this
>> information within the dfu code?
>
> We have f_dfu->poll_timeout field in the dfu function. Value from
> it sets the poll timeout to be waited by the host.
>
> Maybe it would be better to reuse this field to set it accordingly for
> MANIFEST_POLL_TIMEOUT. Now it is used for setting timeout when we store
> big chunks of data (32 MiB).
>
>>
>> Thinking about it, possibly we need a callback from the mtd layer,
>> which gives back the info, how long the flush would take, as this
>> is a media/partition size dependent timeout ...
>
> I think that we could add this callback - called e.g. (*poll_timeout)
> to the struct [mmc|nand]_internal_data. Then some helper functions
> would be defined at dfu_[mmc|nand].c and used at f_dfu.c

Yep ... I look into this...

> Other question is if we accept current patches and wait for above
> improvement to follow of drop them and wait for solution addressing
> those issues.
>
> What do you think?

I prefer to accept current patches (I can post v2), as they are an
improvement. On top of that the optimzation of the PollTimeout
settings is a seperate patch.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany