From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Mon, 17 Mar 2014 11:11:15 +0100 Subject: [U-Boot] [PATCH 2/3] usb: dfu: introduce dfuMANIFEST state In-Reply-To: <20140317104643.6698797f@amdc2363> References: <1394618481-9572-1-git-send-email-hs@denx.de> <1394618481-9572-3-git-send-email-hs@denx.de> <20140314114749.285b3a09@amdc2363> <5326978B.40304@denx.de> <20140317104643.6698797f@amdc2363> Message-ID: <5326CA43.3080805@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 >>>> Cc: Lukasz Majewski >>>> Cc: Kyungmin Park >>>> Cc: Marek Vasut >>>> --- >>>> 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