| Message ID | 1394618481-9572-3-git-send-email-hs@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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/README b/README > index 216f0c7..5076248 100644 > --- a/README > +++ b/README > @@ -1525,6 +1525,11 @@ The following options need to be configured: > this to the maximum filesize (in bytes) for the > buffer. Default is 4 MiB if undefined. > > + DFU_MANIFEST_POLL_TIMEOUT > + Poll timeout [ms], which the device sends to the > host when > + entering dfuMANIFEST state. Host waits this timeout, > before > + sending again an USB request to the device. > + Could you also add information about the DFU_DEFAULT_POLL_TIMEOUT to the README, please (I've forgotten to do that)? > - Journaling Flash filesystem support: > CONFIG_JFFS2_NAND, CONFIG_JFFS2_NAND_OFF, > CONFIG_JFFS2_NAND_SIZE, CONFIG_JFFS2_NAND_DEV > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c > index 193e047..f61e166 100644 > --- a/drivers/dfu/dfu.c > +++ b/drivers/dfu/dfu.c > @@ -222,7 +222,6 @@ int dfu_write(struct dfu_entity *dfu, void *buf, > int size, int blk_seq_num) ret = tret; > } > > - ret = dfu_flush(dfu, buf, size, blk_seq_num); > return ret = 0 ? size : ret; > } > > 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 > @@ -164,15 +164,22 @@ static void dnload_request_complete(struct > usb_ep *ep, struct usb_request *req) > dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf, > req->length, f_dfu->blk_seq_num); > +} > > - if (req->length == 0) > - puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); > +static void dnload_request_flush(struct usb_ep *ep, struct > usb_request *req) +{ > + struct f_dfu *f_dfu = req->context; > + > + req->length = 0; > + dfu_flush(dfu_get_entity(f_dfu->altsetting), req->buf, > + req->length, f_dfu->blk_seq_num); > } > > static void handle_getstatus(struct usb_request *req) > { > struct dfu_status *dstat = (struct dfu_status *)req->buf; > struct f_dfu *f_dfu = req->context; > + int setpolltimeout = 1; We can go away with this flag. Please see below. > > switch (f_dfu->dfu_state) { > case DFU_STATE_dfuDNLOAD_SYNC: > @@ -180,17 +187,24 @@ static void handle_getstatus(struct usb_request > *req) f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; > break; > case DFU_STATE_dfuMANIFEST_SYNC: > + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; > break; > + case DFU_STATE_dfuMANIFEST: > + dfu_set_poll_timeout(dstat, > DFU_MANIFEST_POLL_TIMEOUT); > + setpolltimeout = 0; > default: > break; > } > > - dfu_set_poll_timeout(dstat, 0); > + if (setpolltimeout) { > + dfu_set_poll_timeout(dstat, 0); > > - if (f_dfu->poll_timeout) > - if (!(f_dfu->blk_seq_num % > - (dfu_get_buf_size() / DFU_USB_BUFSIZ))) > - dfu_set_poll_timeout(dstat, > f_dfu->poll_timeout); > + if (f_dfu->poll_timeout) > + if (!(f_dfu->blk_seq_num % > + (dfu_get_buf_size() / DFU_USB_BUFSIZ))) > + dfu_set_poll_timeout(dstat, > + > f_dfu->poll_timeout); > + } What about this solution: dfu_set_poll_timeout(dstat, 0); switch (f_dfu->dfu_state) { case DFU_STATE_dfuDNLOAD_SYNC: case DFU_STATE_dfuDNBUSY: f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; break; case DFU_STATE_dfuMANIFEST_SYNC: f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; break; case DFU_STATE_dfuMANIFEST: dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT); default: break; } if (f_dfu->poll_timeout) if (!(f_dfu->blk_seq_num % (dfu_get_buf_size() / DFU_USB_BUFSIZ))) dfu_set_poll_timeout(dstat,f_dfu->poll_timeout); Then we could avoid setpolltimeout flag. > > /* send status response */ > dstat->bStatus = f_dfu->dfu_status; > @@ -446,10 +460,11 @@ static int state_dfu_manifest_sync(struct f_dfu > *f_dfu, switch (ctrl->bRequest) { > case USB_REQ_DFU_GETSTATUS: > /* We're MainfestationTolerant */ > - f_dfu->dfu_state = DFU_STATE_dfuIDLE; > + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; > handle_getstatus(req); > f_dfu->blk_seq_num = 0; > value = RET_STAT_LEN; > + req->complete = dnload_request_flush; > break; > case USB_REQ_DFU_GETSTATE: > handle_getstate(req); > @@ -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 > + f_dfu->dfu_state = DFU_STATE_dfuIDLE; > + handle_getstatus(req); > + f_dfu->blk_seq_num = 0; > + value = RET_STAT_LEN; > + puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); > + break; > + case USB_REQ_DFU_GETSTATE: > + handle_getstate(req); > + break; > + default: > + f_dfu->dfu_state = DFU_STATE_dfuERROR; > + value = RET_STALL; > + break; > + } > + return value; > +} > + > static int state_dfu_upload_idle(struct f_dfu *f_dfu, > const struct usb_ctrlrequest *ctrl, > struct usb_gadget *gadget, > @@ -539,7 +581,7 @@ static dfu_state_fn dfu_state[] = { > state_dfu_dnbusy, /* DFU_STATE_dfuDNBUSY */ > state_dfu_dnload_idle, /* DFU_STATE_dfuDNLOAD_IDLE */ > state_dfu_manifest_sync, /* DFU_STATE_dfuMANIFEST_SYNC */ > - NULL, /* DFU_STATE_dfuMANIFEST */ > + state_dfu_manifest, /* DFU_STATE_dfuMANIFEST */ > NULL, /* DFU_STATE_dfuMANIFEST_WAIT_RST */ > state_dfu_upload_idle, /* DFU_STATE_dfuUPLOAD_IDLE */ > state_dfu_error /* DFU_STATE_dfuERROR */ > diff --git a/include/dfu.h b/include/dfu.h > index 272a245..6c71ecb 100644 > --- a/include/dfu.h > +++ b/include/dfu.h > @@ -80,6 +80,9 @@ static inline unsigned int get_mmc_blk_size(int dev) > #ifndef DFU_DEFAULT_POLL_TIMEOUT > #define DFU_DEFAULT_POLL_TIMEOUT 0 > #endif > +#ifndef DFU_MANIFEST_POLL_TIMEOUT > +#define DFU_MANIFEST_POLL_TIMEOUT DFU_DEFAULT_POLL_TIMEOUT > +#endif > > struct dfu_entity { > char name[DFU_NAME_SIZE]; I did some preliminary DFU testing and it seems to not introduce any regressions. I'm looking forward for v2.
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/README b/README >> index 216f0c7..5076248 100644 >> --- a/README >> +++ b/README >> @@ -1525,6 +1525,11 @@ The following options need to be configured: >> this to the maximum filesize (in bytes) for the >> buffer. Default is 4 MiB if undefined. >> >> + DFU_MANIFEST_POLL_TIMEOUT >> + Poll timeout [ms], which the device sends to the >> host when >> + entering dfuMANIFEST state. Host waits this timeout, >> before >> + sending again an USB request to the device. >> + > > Could you also add information about the DFU_DEFAULT_POLL_TIMEOUT to > the README, please (I've forgotten to do that)? Done. [...] >> 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 >> @@ -164,15 +164,22 @@ static void dnload_request_complete(struct >> usb_ep *ep, struct usb_request *req) >> dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf, >> req->length, f_dfu->blk_seq_num); >> +} >> >> - if (req->length == 0) >> - puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); >> +static void dnload_request_flush(struct usb_ep *ep, struct >> usb_request *req) +{ >> + struct f_dfu *f_dfu = req->context; >> + >> + req->length = 0; >> + dfu_flush(dfu_get_entity(f_dfu->altsetting), req->buf, >> + req->length, f_dfu->blk_seq_num); >> } >> >> static void handle_getstatus(struct usb_request *req) >> { >> struct dfu_status *dstat = (struct dfu_status *)req->buf; >> struct f_dfu *f_dfu = req->context; >> + int setpolltimeout = 1; > > We can go away with this flag. Please see below. > >> >> switch (f_dfu->dfu_state) { >> case DFU_STATE_dfuDNLOAD_SYNC: >> @@ -180,17 +187,24 @@ static void handle_getstatus(struct usb_request >> *req) f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; >> break; >> case DFU_STATE_dfuMANIFEST_SYNC: >> + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; >> break; >> + case DFU_STATE_dfuMANIFEST: >> + dfu_set_poll_timeout(dstat, >> DFU_MANIFEST_POLL_TIMEOUT); >> + setpolltimeout = 0; >> default: >> break; >> } >> >> - dfu_set_poll_timeout(dstat, 0); >> + if (setpolltimeout) { >> + dfu_set_poll_timeout(dstat, 0); >> >> - if (f_dfu->poll_timeout) >> - if (!(f_dfu->blk_seq_num % >> - (dfu_get_buf_size() / DFU_USB_BUFSIZ))) >> - dfu_set_poll_timeout(dstat, >> f_dfu->poll_timeout); >> + if (f_dfu->poll_timeout) >> + if (!(f_dfu->blk_seq_num % >> + (dfu_get_buf_size() / DFU_USB_BUFSIZ))) >> + dfu_set_poll_timeout(dstat, >> + >> f_dfu->poll_timeout); >> + } > > What about this solution: > > dfu_set_poll_timeout(dstat, 0); > > switch (f_dfu->dfu_state) { > case DFU_STATE_dfuDNLOAD_SYNC: > case DFU_STATE_dfuDNBUSY: > f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; > break; > case DFU_STATE_dfuMANIFEST_SYNC: > f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; > break; > case DFU_STATE_dfuMANIFEST: > dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT); > default: > break; > } > > if (f_dfu->poll_timeout) > if (!(f_dfu->blk_seq_num % > (dfu_get_buf_size() / DFU_USB_BUFSIZ))) > dfu_set_poll_timeout(dstat,f_dfu->poll_timeout); > > Then we could avoid setpolltimeout flag. Done. >> /* send status response */ >> dstat->bStatus = f_dfu->dfu_status; >> @@ -446,10 +460,11 @@ static int state_dfu_manifest_sync(struct f_dfu >> *f_dfu, switch (ctrl->bRequest) { >> case USB_REQ_DFU_GETSTATUS: >> /* We're MainfestationTolerant */ >> - f_dfu->dfu_state = DFU_STATE_dfuIDLE; >> + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; >> handle_getstatus(req); >> f_dfu->blk_seq_num = 0; >> value = RET_STAT_LEN; >> + req->complete = dnload_request_flush; >> break; >> case USB_REQ_DFU_GETSTATE: >> handle_getstate(req); >> @@ -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? >> + f_dfu->dfu_state = DFU_STATE_dfuIDLE; >> + handle_getstatus(req); >> + f_dfu->blk_seq_num = 0; >> + value = RET_STAT_LEN; >> + puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); >> + break; >> + case USB_REQ_DFU_GETSTATE: >> + handle_getstate(req); >> + break; >> + default: >> + f_dfu->dfu_state = DFU_STATE_dfuERROR; >> + value = RET_STALL; >> + break; >> + } >> + return value; >> +} >> + >> static int state_dfu_upload_idle(struct f_dfu *f_dfu, >> const struct usb_ctrlrequest *ctrl, >> struct usb_gadget *gadget, >> @@ -539,7 +581,7 @@ static dfu_state_fn dfu_state[] = { >> state_dfu_dnbusy, /* DFU_STATE_dfuDNBUSY */ >> state_dfu_dnload_idle, /* DFU_STATE_dfuDNLOAD_IDLE */ >> state_dfu_manifest_sync, /* DFU_STATE_dfuMANIFEST_SYNC */ >> - NULL, /* DFU_STATE_dfuMANIFEST */ >> + state_dfu_manifest, /* DFU_STATE_dfuMANIFEST */ >> NULL, /* DFU_STATE_dfuMANIFEST_WAIT_RST */ >> state_dfu_upload_idle, /* DFU_STATE_dfuUPLOAD_IDLE */ >> state_dfu_error /* DFU_STATE_dfuERROR */ >> diff --git a/include/dfu.h b/include/dfu.h >> index 272a245..6c71ecb 100644 >> --- a/include/dfu.h >> +++ b/include/dfu.h >> @@ -80,6 +80,9 @@ static inline unsigned int get_mmc_blk_size(int dev) >> #ifndef DFU_DEFAULT_POLL_TIMEOUT >> #define DFU_DEFAULT_POLL_TIMEOUT 0 >> #endif >> +#ifndef DFU_MANIFEST_POLL_TIMEOUT >> +#define DFU_MANIFEST_POLL_TIMEOUT DFU_DEFAULT_POLL_TIMEOUT >> +#endif >> >> struct dfu_entity { >> char name[DFU_NAME_SIZE]; > > I did some preliminary DFU testing and it seems to not introduce any > regressions. Thanks! > I'm looking forward for v2. I am ready for sending it, but need response from you as I think the comment is correct ... 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 ... 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? 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 ... bye, Heiko
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/README b/README > >> index 216f0c7..5076248 100644 > >> --- a/README > >> +++ b/README > >> @@ -1525,6 +1525,11 @@ The following options need to be configured: > >> this to the maximum filesize (in bytes) for the > >> buffer. Default is 4 MiB if undefined. > >> > >> + DFU_MANIFEST_POLL_TIMEOUT > >> + Poll timeout [ms], which the device sends to the > >> host when > >> + entering dfuMANIFEST state. Host waits this > >> timeout, before > >> + sending again an USB request to the device. > >> + > > > > Could you also add information about the DFU_DEFAULT_POLL_TIMEOUT to > > the README, please (I've forgotten to do that)? > > Done. > > [...] > >> 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 > >> @@ -164,15 +164,22 @@ static void dnload_request_complete(struct > >> usb_ep *ep, struct usb_request *req) > >> dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf, > >> req->length, f_dfu->blk_seq_num); > >> +} > >> > >> - if (req->length == 0) > >> - puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); > >> +static void dnload_request_flush(struct usb_ep *ep, struct > >> usb_request *req) +{ > >> + struct f_dfu *f_dfu = req->context; > >> + > >> + req->length = 0; > >> + dfu_flush(dfu_get_entity(f_dfu->altsetting), req->buf, > >> + req->length, f_dfu->blk_seq_num); > >> } > >> > >> static void handle_getstatus(struct usb_request *req) > >> { > >> struct dfu_status *dstat = (struct dfu_status *)req->buf; > >> struct f_dfu *f_dfu = req->context; > >> + int setpolltimeout = 1; > > > > We can go away with this flag. Please see below. > > > >> > >> switch (f_dfu->dfu_state) { > >> case DFU_STATE_dfuDNLOAD_SYNC: > >> @@ -180,17 +187,24 @@ static void handle_getstatus(struct > >> usb_request *req) f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; > >> break; > >> case DFU_STATE_dfuMANIFEST_SYNC: > >> + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; > >> break; > >> + case DFU_STATE_dfuMANIFEST: > >> + dfu_set_poll_timeout(dstat, > >> DFU_MANIFEST_POLL_TIMEOUT); > >> + setpolltimeout = 0; > >> default: > >> break; > >> } > >> > >> - dfu_set_poll_timeout(dstat, 0); > >> + if (setpolltimeout) { > >> + dfu_set_poll_timeout(dstat, 0); > >> > >> - if (f_dfu->poll_timeout) > >> - if (!(f_dfu->blk_seq_num % > >> - (dfu_get_buf_size() / DFU_USB_BUFSIZ))) > >> - dfu_set_poll_timeout(dstat, > >> f_dfu->poll_timeout); > >> + if (f_dfu->poll_timeout) > >> + if (!(f_dfu->blk_seq_num % > >> + (dfu_get_buf_size() / > >> DFU_USB_BUFSIZ))) > >> + dfu_set_poll_timeout(dstat, > >> + > >> f_dfu->poll_timeout); > >> + } > > > > What about this solution: > > > > dfu_set_poll_timeout(dstat, 0); > > > > switch (f_dfu->dfu_state) { > > case DFU_STATE_dfuDNLOAD_SYNC: > > case DFU_STATE_dfuDNBUSY: > > f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; > > break; > > case DFU_STATE_dfuMANIFEST_SYNC: > > f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; > > break; > > case DFU_STATE_dfuMANIFEST: > > dfu_set_poll_timeout(dstat, > > DFU_MANIFEST_POLL_TIMEOUT); default: > > break; > > } > > > > if (f_dfu->poll_timeout) > > if (!(f_dfu->blk_seq_num % > > (dfu_get_buf_size() / DFU_USB_BUFSIZ))) > > dfu_set_poll_timeout(dstat,f_dfu->poll_timeout); > > > > Then we could avoid setpolltimeout flag. > > Done. > > >> /* send status response */ > >> dstat->bStatus = f_dfu->dfu_status; > >> @@ -446,10 +460,11 @@ static int state_dfu_manifest_sync(struct > >> f_dfu *f_dfu, switch (ctrl->bRequest) { > >> case USB_REQ_DFU_GETSTATUS: > >> /* We're MainfestationTolerant */ > >> - f_dfu->dfu_state = DFU_STATE_dfuIDLE; > >> + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; > >> handle_getstatus(req); > >> f_dfu->blk_seq_num = 0; > >> value = RET_STAT_LEN; > >> + req->complete = dnload_request_flush; > >> break; > >> case USB_REQ_DFU_GETSTATE: > >> handle_getstate(req); > >> @@ -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'". > > >> + f_dfu->dfu_state = DFU_STATE_dfuIDLE; > >> + handle_getstatus(req); > >> + f_dfu->blk_seq_num = 0; > >> + value = RET_STAT_LEN; > >> + puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); > >> + break; > >> + case USB_REQ_DFU_GETSTATE: > >> + handle_getstate(req); > >> + break; > >> + default: > >> + f_dfu->dfu_state = DFU_STATE_dfuERROR; > >> + value = RET_STALL; > >> + break; > >> + } > >> + return value; > >> +} > >> + > >> static int state_dfu_upload_idle(struct f_dfu *f_dfu, > >> const struct usb_ctrlrequest > >> *ctrl, struct usb_gadget *gadget, > >> @@ -539,7 +581,7 @@ static dfu_state_fn dfu_state[] = { > >> state_dfu_dnbusy, /* DFU_STATE_dfuDNBUSY */ > >> state_dfu_dnload_idle, /* DFU_STATE_dfuDNLOAD_IDLE */ > >> state_dfu_manifest_sync, /* DFU_STATE_dfuMANIFEST_SYNC */ > >> - NULL, /* DFU_STATE_dfuMANIFEST */ > >> + state_dfu_manifest, /* DFU_STATE_dfuMANIFEST */ > >> NULL, /* > >> DFU_STATE_dfuMANIFEST_WAIT_RST */ state_dfu_upload_idle, /* > >> DFU_STATE_dfuUPLOAD_IDLE */ state_dfu_error /* > >> DFU_STATE_dfuERROR */ diff --git a/include/dfu.h b/include/dfu.h > >> index 272a245..6c71ecb 100644 > >> --- a/include/dfu.h > >> +++ b/include/dfu.h > >> @@ -80,6 +80,9 @@ static inline unsigned int get_mmc_blk_size(int > >> dev) #ifndef DFU_DEFAULT_POLL_TIMEOUT > >> #define DFU_DEFAULT_POLL_TIMEOUT 0 > >> #endif > >> +#ifndef DFU_MANIFEST_POLL_TIMEOUT > >> +#define DFU_MANIFEST_POLL_TIMEOUT DFU_DEFAULT_POLL_TIMEOUT > >> +#endif > >> > >> struct dfu_entity { > >> char name[DFU_NAME_SIZE]; > > > > I did some preliminary DFU testing and it seems to not introduce any > > regressions. > > Thanks! > > > 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. > > 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? > > 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 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? > > bye, > 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@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
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@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 ... > 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
diff --git a/README b/README index 216f0c7..5076248 100644 --- a/README +++ b/README @@ -1525,6 +1525,11 @@ The following options need to be configured: this to the maximum filesize (in bytes) for the buffer. Default is 4 MiB if undefined. + DFU_MANIFEST_POLL_TIMEOUT + Poll timeout [ms], which the device sends to the host when + entering dfuMANIFEST state. Host waits this timeout, before + sending again an USB request to the device. + - Journaling Flash filesystem support: CONFIG_JFFS2_NAND, CONFIG_JFFS2_NAND_OFF, CONFIG_JFFS2_NAND_SIZE, CONFIG_JFFS2_NAND_DEV diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 193e047..f61e166 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -222,7 +222,6 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num) ret = tret; } - ret = dfu_flush(dfu, buf, size, blk_seq_num); return ret = 0 ? size : ret; } 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 @@ -164,15 +164,22 @@ static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req) dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf, req->length, f_dfu->blk_seq_num); +} - if (req->length == 0) - puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); +static void dnload_request_flush(struct usb_ep *ep, struct usb_request *req) +{ + struct f_dfu *f_dfu = req->context; + + req->length = 0; + dfu_flush(dfu_get_entity(f_dfu->altsetting), req->buf, + req->length, f_dfu->blk_seq_num); } static void handle_getstatus(struct usb_request *req) { struct dfu_status *dstat = (struct dfu_status *)req->buf; struct f_dfu *f_dfu = req->context; + int setpolltimeout = 1; switch (f_dfu->dfu_state) { case DFU_STATE_dfuDNLOAD_SYNC: @@ -180,17 +187,24 @@ static void handle_getstatus(struct usb_request *req) f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE; break; case DFU_STATE_dfuMANIFEST_SYNC: + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; break; + case DFU_STATE_dfuMANIFEST: + dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT); + setpolltimeout = 0; default: break; } - dfu_set_poll_timeout(dstat, 0); + if (setpolltimeout) { + dfu_set_poll_timeout(dstat, 0); - if (f_dfu->poll_timeout) - if (!(f_dfu->blk_seq_num % - (dfu_get_buf_size() / DFU_USB_BUFSIZ))) - dfu_set_poll_timeout(dstat, f_dfu->poll_timeout); + if (f_dfu->poll_timeout) + if (!(f_dfu->blk_seq_num % + (dfu_get_buf_size() / DFU_USB_BUFSIZ))) + dfu_set_poll_timeout(dstat, + f_dfu->poll_timeout); + } /* send status response */ dstat->bStatus = f_dfu->dfu_status; @@ -446,10 +460,11 @@ static int state_dfu_manifest_sync(struct f_dfu *f_dfu, switch (ctrl->bRequest) { case USB_REQ_DFU_GETSTATUS: /* We're MainfestationTolerant */ - f_dfu->dfu_state = DFU_STATE_dfuIDLE; + f_dfu->dfu_state = DFU_STATE_dfuMANIFEST; handle_getstatus(req); f_dfu->blk_seq_num = 0; value = RET_STAT_LEN; + req->complete = dnload_request_flush; break; case USB_REQ_DFU_GETSTATE: handle_getstate(req); @@ -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 */ + f_dfu->dfu_state = DFU_STATE_dfuIDLE; + handle_getstatus(req); + f_dfu->blk_seq_num = 0; + value = RET_STAT_LEN; + puts("DOWNLOAD ... OK\nCtrl+C to exit ...\n"); + break; + case USB_REQ_DFU_GETSTATE: + handle_getstate(req); + break; + default: + f_dfu->dfu_state = DFU_STATE_dfuERROR; + value = RET_STALL; + break; + } + return value; +} + static int state_dfu_upload_idle(struct f_dfu *f_dfu, const struct usb_ctrlrequest *ctrl, struct usb_gadget *gadget, @@ -539,7 +581,7 @@ static dfu_state_fn dfu_state[] = { state_dfu_dnbusy, /* DFU_STATE_dfuDNBUSY */ state_dfu_dnload_idle, /* DFU_STATE_dfuDNLOAD_IDLE */ state_dfu_manifest_sync, /* DFU_STATE_dfuMANIFEST_SYNC */ - NULL, /* DFU_STATE_dfuMANIFEST */ + state_dfu_manifest, /* DFU_STATE_dfuMANIFEST */ NULL, /* DFU_STATE_dfuMANIFEST_WAIT_RST */ state_dfu_upload_idle, /* DFU_STATE_dfuUPLOAD_IDLE */ state_dfu_error /* DFU_STATE_dfuERROR */ diff --git a/include/dfu.h b/include/dfu.h index 272a245..6c71ecb 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -80,6 +80,9 @@ static inline unsigned int get_mmc_blk_size(int dev) #ifndef DFU_DEFAULT_POLL_TIMEOUT #define DFU_DEFAULT_POLL_TIMEOUT 0 #endif +#ifndef DFU_MANIFEST_POLL_TIMEOUT +#define DFU_MANIFEST_POLL_TIMEOUT DFU_DEFAULT_POLL_TIMEOUT +#endif struct dfu_entity { char name[DFU_NAME_SIZE];
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(-)