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

[Lukasz Majewski]

Hi Heiko,

> 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 at denx.de>
> >>>> Cc: Lukasz Majewski<l.majewski at samsung.com>
> >>>> Cc: Kyungmin Park<kyungmin.park at samsung.com>
> >>>> Cc: Marek Vasut<marex at 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 ...
> 

Thanks for very in depth explanation. You seem to be right :-). Lets
not change the comment.

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

Yes, it is a pain... to wait.

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

Ok, lets proceed in this way.

> 
> bye,
> Heiko


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group