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

[Lukasz Majewski <l.majewski@samsung.com>]

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.

-- 
Best regards,

Lukasz Majewski

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