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<h...@denx.de> > >>>> Cc: Lukasz Majewski<l.majew...@samsung.com> > >>>> Cc: Kyungmin Park<kyungmin.p...@samsung.com> > >>>> Cc: Marek Vasut<ma...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
