usb: dfu: introduce dfuMANIFEST state

PATCH Superseded Heiko Schocher 3 participants 14 messages Mar 12, 2014 – Mar 17, 2014 USB

Share & tools

Heiko Schocher

author

2014-03-12 10:01 UTC #0 lore raw

cover usb: dfu: introduce dfuMANIFEST state

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 dfu_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 [1] 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.

Patch 0002-usb-dfu-introduce-dfuMANIFEST-state.patch shows following checkpatch errors, but I think they are OK, as the hole file uses CamelCase for the statenames as this in sync with [1]

CHECK: Avoid CamelCase: <DFU_STATE_dfuMANIFEST>
#103: FILE: drivers/usb/gadget/f_dfu.c:190:
+ f_dfu->dfu_state = DFU_STATE_dfuMANIFEST;

CHECK: Avoid CamelCase: <DFU_STATE_dfuIDLE>
#156: FILE: drivers/usb/gadget/f_dfu.c:491:
+ f_dfu->dfu_state = DFU_STATE_dfuIDLE;

CHECK: Avoid CamelCase: <DFU_STATE_dfuERROR>
#166: FILE: drivers/usb/gadget/f_dfu.c:501:
+ f_dfu->dfu_state = DFU_STATE_dfuERROR;

total: 0 errors, 0 warnings, 3 checks, 133 lines checked

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE USLEEP_RANGE

[1]:
http://www.usb.org/developers/devclass_docs/usbdfu10.pdf

Heiko Schocher (3):
usb, dfu: extract flush code into seperate function
usb: dfu: introduce dfuMANIFEST state
am335x, dfu: add DFU_MANIFEST_POLL_TIMEOUT to the siemens boards

 README                                 |  5 +++
 drivers/dfu/dfu.c                      | 45 +++++++++++++------------
 drivers/usb/gadget/f_dfu.c             | 60 +++++++++++++++++++++++++++++-----
 include/configs/siemens-am33x-common.h |  1 +
 include/dfu.h                          |  4 +++
 5 files changed, 86 insertions(+), 29 deletions(-)

Cc Lukasz Majewski <[email protected]>

Cc Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

Signature · 1 line

1.8.3.1

Heiko Schocher

author

2014-03-12 10:01 UTC #1 lore raw

PATCH 1/3 usb, dfu: extract flush code into seperate function

move the flushing code into an extra function dfu_flush(), so it can be used from other code.

Signed-off-by Heiko Schocher <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/dfu/dfu.c | 46 ++++++++++++++++++++++++++--------------------
 include/dfu.h     |  1 +
 2 files changed, 27 insertions(+), 20 deletions(-)

drivers/dfu/dfu.c +26 ~~-20~~

index 56e69fd..193e047 100644
@@ -127,6 +127,31 @@ static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 	return ret;
 }
 
+int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
+{
+	int ret = 0;
+
+	/* end? */
+	if (size == 0) {
+		/* Now try and flush to the medium if needed. */
+		if (dfu->flush_medium)
+			ret = dfu->flush_medium(dfu);
+		printf("\nDFU complete CRC32: 0x%08x\n", dfu->crc);
+
+		/* clear everything */
+		dfu_free_buf();
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_blk_seq_num = 0;
+		dfu->i_buf_start = dfu_buf;
+		dfu->i_buf_end = dfu_buf;
+		dfu->i_buf = dfu->i_buf_start;
+
+		dfu->inited = 0;
+	}
+	return ret;
+}
+
 int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 {
 	int ret = 0;

@@ -197,26 +222,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 			ret = tret;
 	}
 
-	/* end? */
-	if (size == 0) {
-		/* Now try and flush to the medium if needed. */
-		if (dfu->flush_medium)
-			ret = dfu->flush_medium(dfu);
-		printf("\nDFU complete CRC32: 0x%08x\n", dfu->crc);
-
-		/* clear everything */
-		dfu_free_buf();
-		dfu->crc = 0;
-		dfu->offset = 0;
-		dfu->i_blk_seq_num = 0;
-		dfu->i_buf_start = dfu_buf;
-		dfu->i_buf_end = dfu_buf;
-		dfu->i_buf = dfu->i_buf_start;
-
-		dfu->inited = 0;
-
-	}
-
+	ret = dfu_flush(dfu, buf, size, blk_seq_num);
 	return ret = 0 ? size : ret;
 }

include/dfu.h +1 -0

index f973426..272a245 100644
@@ -138,6 +138,7 @@ unsigned long dfu_get_buf_size(void);
 
 int dfu_read(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
 int dfu_write(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
+int dfu_flush(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
 /* Device specific */
 #ifdef CONFIG_DFU_MMC
 extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s);

Signature · 1 line

1.8.3.1

Marek Vasut

2014-03-12 11:43 UTC #2 lore raw

PATCH 1/3 usb, dfu: extract flush code into seperate function

Quoted 17 lines · Heiko Schocher

move the flushing code into an extra function dfu_flush(), so it can be used from other code.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
---
 drivers/dfu/dfu.c | 46 ++++++++++++++++++++++++++--------------------
 include/dfu.h     |  1 +
 2 files changed, 27 insertions(+), 20 deletions(-)
drivers/dfu/dfu.c +0 -0
index 56e69fd..193e047 100644

drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity · line 134

Quoted 8 lines

drivers/dfu/dfu.c +4 -0

@@ -127,6 +127,31 @@ static int dfu_write_buffer_drain(struct dfu_entity
 *dfu) return ret;
 }
 
+int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int
 blk_seq_num) +{
+	int ret = 0;
+
+	/* end? */

What does this comment mean ? I don't understand it ...

drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 135

Quoted 1 line

drivers/dfu/dfu.c +1 -0
+	if (size == 0) {

You can check this like so:

if (size)
return;

code
code
code

This would cut down the indent, no ?

Please let's fix this when this popped up, either in separate patch or wrap it into this one.

Quoted 18 lines

drivers/dfu/dfu.c +18 -0

+		/* Now try and flush to the medium if needed. */
+		if (dfu->flush_medium)
+			ret = dfu->flush_medium(dfu);
+		printf("\nDFU complete CRC32: 0x%08x\n", dfu->crc);
+
+		/* clear everything */
+		dfu_free_buf();
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_blk_seq_num = 0;
+		dfu->i_buf_start = dfu_buf;
+		dfu->i_buf_end = dfu_buf;
+		dfu->i_buf = dfu->i_buf_start;
+
+		dfu->inited = 0;
+	}
+	return ret;
+}

[...]

Heiko Schocher

author

2014-03-13 05:31 UTC #3 lore raw

PATCH 1/3 usb, dfu: extract flush code into seperate function

Hello Marek,

Am 12.03.2014 12:43, schrieb Marek Vasut:

Quoted 21 lines · Heiko Schocher

Quoted 20 lines
move the flushing code into an extra function dfu_flush(), so it can be used from other code.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
  drivers/dfu/dfu.c | 46 ++++++++++++++++++++++++++--------------------
  include/dfu.h     |  1 +
  2 files changed, 27 insertions(+), 20 deletions(-)
drivers/dfu/dfu.c +0 -0
index 56e69fd..193e047 100644
@@ -127,6 +127,31 @@ static int dfu_write_buffer_drain(struct dfu_entity
 *dfu) return ret;
  }
 

Quoted 7 lines

drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 134
Quoted 5 lines
drivers/dfu/dfu.c +4 -0
+int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int
 blk_seq_num) +{
+	int ret = 0;
+
+	/* end? */
What does this comment mean ? I don't understand it ...

Comes from original code... Thinking about it, it seems better to let this comment and the below "if" in the dfu_write function...

Quoted 6 lines

drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 135
Quoted 1 line
drivers/dfu/dfu.c +1 -0
+	if (size == 0) {
You can check this like so:
if (size)
return;

... as when moving this "if" back to dfu_write(), this "if" kindly disappears in the patch 2/3 of this series, as calling dfu_flush() is only in the new dfuMANIFEST state necessary, which is called at the end of the dfu transfer, so no need for checking, if end of size is reached!

Quoted 6 lines

code
code
code
This would cut down the indent, no ?

Yes. Did the above change and this "if" is no longer necessary ;-)

I will wait for comments from Lukasz before posting v2.

Quoted 14 lines

Please let's fix this when this popped up, either in separate patch or wrap it into this one.

Quoted 11 lines

drivers/dfu/dfu.c +11 -0

+		/* Now try and flush to the medium if needed. */
+		if (dfu->flush_medium)
+			ret = dfu->flush_medium(dfu);
+		printf("\nDFU complete CRC32: 0x%08x\n", dfu->crc);
+
+		/* clear everything */
+		dfu_free_buf();
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_blk_seq_num = 0;
+		dfu->i_buf_start = dfu_buf;

Quoted 8 lines

Quoted 7 lines

drivers/dfu/dfu.c +7 -0

+		dfu->i_buf_end = dfu_buf;
+		dfu->i_buf = dfu->i_buf_start;
+
+		dfu->inited = 0;
+	}
+	return ret;
+}

[...]

Thanks!

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Marek Vasut

2014-03-13 12:38 UTC #4 lore raw

PATCH 1/3 usb, dfu: extract flush code into seperate function

Quoted 43 lines · Heiko Schocher

Hello Marek,
Am 12.03.2014 12:43, schrieb Marek Vasut:
Quoted 23 lines · Heiko Schocher
Quoted 22 lines
move the flushing code into an extra function dfu_flush(), so it can be used from other code.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
  drivers/dfu/dfu.c | 46 ++++++++++++++++++++++++++--------------------
  include/dfu.h     |  1 +
  2 files changed, 27 insertions(+), 20 deletions(-)
drivers/dfu/dfu.c +0 -0
index 56e69fd..193e047 100644
@@ -127,6 +127,31 @@ static int dfu_write_buffer_drain(struct dfu_entity
 *dfu) return ret;
 
  }
 
Quoted 7 lines
drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 134
Quoted 5 lines
drivers/dfu/dfu.c +4 -0
+int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int
 blk_seq_num) +{
+	int ret = 0;
+
+	/* end? */
What does this comment mean ? I don't understand it ...
Comes from original code... Thinking about it, it seems better to let this comment and the below "if" in the dfu_write function...
Quoted 6 lines
drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 135
Quoted 1 line
drivers/dfu/dfu.c +1 -0
+	if (size == 0) {
You can check this like so:
if (size)

Quoted 7 lines

Quoted 1 line
   return;
... as when moving this "if" back to dfu_write(), this "if" kindly disappears in the patch 2/3 of this series, as calling dfu_flush() is only in the new dfuMANIFEST state necessary, which is called at the end of the dfu transfer, so no need for checking, if end of size is reached!

OK, this makes sense. Thanks for pointing this out.

Lukasz Majewski

2014-03-14 10:36 UTC #5 lore raw

PATCH 1/3 usb, dfu: extract flush code into seperate function

In reply to Heiko Schocher

Hi Heiko,

I'm also CCing Pantelis to the loop.

Quoted 32 lines

Hello Marek,
Am 12.03.2014 12:43, schrieb Marek Vasut:
Quoted 21 lines · Heiko Schocher
Quoted 20 lines
move the flushing code into an extra function dfu_flush(), so it can be used from other code.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
  drivers/dfu/dfu.c | 46
++++++++++++++++++++++++++-------------------- include/dfu.h | 1 + 2 files changed, 27 insertions(+), 20 deletions(-)
drivers/dfu/dfu.c +0 -0
index 56e69fd..193e047 100644
@@ -127,6 +127,31 @@ static int dfu_write_buffer_drain(struct
 dfu_entity *dfu) return ret;
  }
 
Quoted 7 lines
drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 134
Quoted 5 lines
drivers/dfu/dfu.c +4 -0
+int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int
 blk_seq_num) +{
+	int ret = 0;
+
+	/* end? */
What does this comment mean ? I don't understand it ...

Quoted 2 lines

Comes from original code... Thinking about it, it seems better to let this comment and the below "if" in the dfu_write function...

This comment doesn't bring any meaningful information, so can be removed.

Quoted 5 lines

Quoted 5 lines
drivers/dfu/dfu.c static int dfu_write_buffer_drain(struct dfu_entity *dfu) · line 135
Quoted 2 lines
drivers/dfu/dfu.c +1 -0
+	if (size == 0) {
You can check this like so:

Quoted 8 lines

Quoted 2 lines
if (size)
return;
... as when moving this "if" back to dfu_write(), this "if" kindly disappears in the patch 2/3 of this series, as calling dfu_flush() is only in the new dfuMANIFEST state necessary, which is called at the end of the dfu transfer, so no need for checking, if end of size is reached!

So, in my opinion, this if can be safely removed.

Quoted 35 lines

Quoted 7 lines
code
code
code
This would cut down the indent, no ?
Yes. Did the above change and this "if" is no longer necessary ;-)
I will wait for comments from Lukasz before posting v2.
Quoted 14 lines
Please let's fix this when this popped up, either in separate patch or wrap it into this one.
Quoted 11 lines
drivers/dfu/dfu.c +4 -0
+		/* Now try and flush to the medium if needed. */
+		if (dfu->flush_medium)
+			ret = dfu->flush_medium(dfu);
+		printf("\nDFU complete CRC32: 0x%08x\n",
dfu->crc); +
drivers/dfu/dfu.c +6 -0
+		/* clear everything */
+		dfu_free_buf();
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_blk_seq_num = 0;
+		dfu->i_buf_start = dfu_buf;
Quoted 8 lines
Quoted 7 lines
drivers/dfu/dfu.c +7 -0
+		dfu->i_buf_end = dfu_buf;
+		dfu->i_buf = dfu->i_buf_start;
+
+		dfu->inited = 0;
+	}
+	return ret;
+}
[...]

Quoted 4 lines

Thanks!
bye,
Heiko

BTW: Thanks for proper extension of the DFU state machine in u-boot :-).

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Heiko Schocher

author

2014-03-12 10:01 UTC #6 lore raw

PATCH 2/3 usb: dfu: introduce dfuMANIFEST state

In reply to Heiko Schocher

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 <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

---
 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(-)

README +5 -0

index 216f0c7..5076248 100644
@@ -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

drivers/dfu/dfu.c +0 -1

index 193e047..f61e166 100644
@@ -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;
 }

drivers/usb/gadget/f_dfu.c +51 -9

106 lines not on this page · raw message

include/dfu.h +3 -0

index 272a245..6c71ecb 100644
@@ -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];

Signature · 1 line

1.8.3.1

Lukasz Majewski

2014-03-14 10:47 UTC #7 lore raw

PATCH 2/3 usb: dfu: introduce dfuMANIFEST state

Hi Heiko,

Quoted 51 lines

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 <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
---
 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(-)
README +0 -0
index 216f0c7..5076248 100644
@@ -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.
README +2 -0
+		DFU_MANIFEST_POLL_TIMEOUT
+		Poll timeout [ms], which the device sends to the
host when
README +1 -0
+		entering dfuMANIFEST state. Host waits this timeout,
before

README The following options need to be configured: · line 1532

Quoted 2 lines

README +2 -0

+		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)?

Quoted 31 lines

- Journaling Flash filesystem support:
CONFIG_JFFS2_NAND, CONFIG_JFFS2_NAND_OFF,
CONFIG_JFFS2_NAND_SIZE, CONFIG_JFFS2_NAND_DEV

drivers/dfu/dfu.c +0 -1

index 193e047..f61e166 100644
@@ -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;
 }

drivers/usb/gadget/f_dfu.c +4 -2

index a045864..f4bf918 100644
@@ -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;
+

drivers/usb/gadget/f_dfu.c static void dnload_request_complete(struct · line 182

Quoted 10 lines

drivers/usb/gadget/f_dfu.c +4 -0

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

Quoted 23 lines

drivers/usb/gadget/f_dfu.c +6 -3

 	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 %

drivers/usb/gadget/f_dfu.c static void handle_getstatus(struct usb_request · line 210

Quoted 10 lines

drivers/usb/gadget/f_dfu.c +6 -2

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

Quoted 20 lines

drivers/usb/gadget/f_dfu.c +2 -1

 	/* 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;
 }

drivers/usb/gadget/f_dfu.c static int state_dfu_manifest_sync(struct f_dfu · line 490

Quoted 10 lines

drivers/usb/gadget/f_dfu.c +10 -0

+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

Quoted 34 lines

drivers/usb/gadget/f_dfu.c +18 -1

+		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 */

include/dfu.h +0 -0

index 272a245..6c71ecb 100644

include/dfu.h static inline unsigned int get_mmc_blk_size(int dev) · line 88

Quoted 9 lines

include/dfu.h +3 -0

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

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Heiko Schocher

author

2014-03-17 06:34 UTC #8 lore raw

PATCH 2/3 usb: dfu: introduce dfuMANIFEST state

Hello Lukasz,

Am 14.03.2014 11:47, schrieb Lukasz Majewski:

Quoted 48 lines

Hi Heiko,
Quoted 46 lines
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<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
  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(-)
README +0 -0
index 216f0c7..5076248 100644
@@ -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.

Quoted 10 lines

README The following options need to be configured: · line 1532
Quoted 7 lines
README +2 -0
+		DFU_MANIFEST_POLL_TIMEOUT
+		Poll timeout [ms], which the device sends to the
host when
README +1 -0
+		entering dfuMANIFEST state. Host waits this timeout,
before
README +2 -0
+		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.

[...]

Quoted 86 lines

Quoted 16 lines

drivers/usb/gadget/f_dfu.c +4 -2

index a045864..f4bf918 100644
@@ -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;
+

drivers/usb/gadget/f_dfu.c static void dnload_request_complete(struct · line 182

Quoted 10 lines

drivers/usb/gadget/f_dfu.c +4 -0

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

Quoted 23 lines

drivers/usb/gadget/f_dfu.c +6 -3

  	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 %

drivers/usb/gadget/f_dfu.c static void handle_getstatus(struct usb_request · line 210

Quoted 10 lines

drivers/usb/gadget/f_dfu.c +6 -2

-		      (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);

Quoted 1 line

Then we could avoid setpolltimeout flag.

Done.

Quoted 19 lines

  	/* send status response */
  	dstat->bStatus = f_dfu->dfu_status;

drivers/usb/gadget/f_dfu.c +2 -1

@@ -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;
  }

Quoted 13 lines

drivers/usb/gadget/f_dfu.c static int state_dfu_manifest_sync(struct f_dfu *f_dfu, · line 490
Quoted 10 lines
drivers/usb/gadget/f_dfu.c +10 -0
+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?

Quoted 41 lines

drivers/usb/gadget/f_dfu.c +18 -1

+		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 */

include/dfu.h +3 -0

index 272a245..6c71ecb 100644
@@ -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

Quoted 5 lines

include/dfu.h static inline unsigned int get_mmc_blk_size(int dev) · line 88
Quoted 2 lines
include/dfu.h +0 -0
  struct dfu_entity {
  	char			name[DFU_NAME_SIZE];
I did some preliminary DFU testing and it seems to not introduce any regressions.

Thanks!

Quoted 1 line

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

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Lukasz Majewski

2014-03-17 09:46 UTC #9 lore raw

PATCH 2/3 usb: dfu: introduce dfuMANIFEST state

Hi Heiko,

Quoted 190 lines

Hello Lukasz,
Am 14.03.2014 11:47, schrieb Lukasz Majewski:
Quoted 49 lines
Hi Heiko,
Quoted 47 lines
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<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
  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(-)
README +0 -0
index 216f0c7..5076248 100644
@@ -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.
Quoted 10 lines
README The following options need to be configured: · line 1532
Quoted 7 lines
README +2 -0
+		DFU_MANIFEST_POLL_TIMEOUT
+		Poll timeout [ms], which the device sends to the
host when
README +1 -0
+		entering dfuMANIFEST state. Host waits this
timeout, before
README +2 -0
+		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.
[...]
Quoted 87 lines
Quoted 16 lines
(quoted diff) +0 -0
b/drivers/usb/gadget/f_dfu.c index a045864..f4bf918 100644
drivers/usb/gadget/f_dfu.c +4 -2
@@ -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;
+
drivers/usb/gadget/f_dfu.c static void dnload_request_complete(struct · line 182
Quoted 10 lines
drivers/usb/gadget/f_dfu.c +4 -0
+	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.
Quoted 33 lines
drivers/usb/gadget/f_dfu.c +11 -5
  	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);
drivers/usb/gadget/f_dfu.c
Quoted 1 line
drivers/usb/gadget/f_dfu.c +1 -0
+	}
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);
Quoted 1 line
Then we could avoid setpolltimeout flag.
Done.
Quoted 19 lines
Quoted 19 lines
  	/* send status response */
  	dstat->bStatus = f_dfu->dfu_status;
drivers/usb/gadget/f_dfu.c +2 -1
@@ -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;
  }
 
Quoted 13 lines
drivers/usb/gadget/f_dfu.c static int state_dfu_manifest_sync(struct f_dfu *f_dfu, · line 490
Quoted 10 lines
drivers/usb/gadget/f_dfu.c +10 -0
+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

Quoted 2 lines

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

Quoted 48 lines

Quoted 42 lines

drivers/usb/gadget/f_dfu.c +18 -1

+		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

include/dfu.h +0 -0

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

include/dfu.h +3 -0

+#ifndef DFU_MANIFEST_POLL_TIMEOUT
+#define DFU_MANIFEST_POLL_TIMEOUT	DFU_DEFAULT_POLL_TIMEOUT
+#endif

Quoted 5 lines

Quoted 2 lines
  struct dfu_entity {
  	char			name[DFU_NAME_SIZE];
I did some preliminary DFU testing and it seems to not introduce any regressions.

Quoted 6 lines

Thanks!
Quoted 1 line
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.

Quoted 8 lines

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?

Quoted 5 lines

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

Quoted 4 lines

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?

Quoted 3 lines

bye,
Heiko

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Heiko Schocher

author

2014-03-17 10:11 UTC #10 lore raw

PATCH 2/3 usb: dfu: introduce dfuMANIFEST state

Hello Lukasz,

Am 17.03.2014 10:46, schrieb Lukasz Majewski:

Quoted 46 lines

Hi Heiko,
Quoted 44 lines
Hello Lukasz,
Am 14.03.2014 11:47, schrieb Lukasz Majewski:
Quoted 41 lines
Hi Heiko,
Quoted 39 lines
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<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
   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(-)

[...]

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
b/drivers/usb/gadget/f_dfu.c index a045864..f4bf918 100644
drivers/usb/gadget/f_dfu.c +0 -0

[...]

Quoted 25 lines

Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
drivers/usb/gadget/f_dfu.c +0 -0
@@ -463,6 +478,33 @@ static int state_dfu_manifest_sync(struct
 f_dfu *f_dfu, return value;
   }
 
Quoted 13 lines
drivers/usb/gadget/f_dfu.c static int state_dfu_manifest_sync(struct f_dfu *f_dfu, · line 490
Quoted 10 lines
drivers/usb/gadget/f_dfu.c +10 -0
+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
Quoted 2 lines
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'".

[...]

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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 ...

Quoted 10 lines

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

Quoted 13 lines

Quoted 4 lines
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).
Quoted 1 line

Quoted 7 lines

Quoted 3 lines
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...

Quoted 5 lines

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

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Lukasz Majewski

2014-03-17 10:42 UTC #11 lore raw

PATCH 2/3 usb: dfu: introduce dfuMANIFEST state

Hi Heiko,

Quoted 108 lines

Hello Lukasz,
Am 17.03.2014 10:46, schrieb Lukasz Majewski:
Quoted 48 lines
Hi Heiko,
Quoted 46 lines
Hello Lukasz,
Am 14.03.2014 11:47, schrieb Lukasz Majewski:
Quoted 43 lines
Hi Heiko,
Quoted 41 lines
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<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
---
   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(-)
[...]
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
b/drivers/usb/gadget/f_dfu.c index a045864..f4bf918 100644
drivers/usb/gadget/f_dfu.c +0 -0
[...]
Quoted 27 lines
Quoted 19 lines
Quoted 4 lines
Quoted 4 lines
drivers/usb/gadget/f_dfu.c +0 -0
@@ -463,6 +478,33 @@ static int state_dfu_manifest_sync(struct
 f_dfu *f_dfu, return value;
   }
 
Quoted 14 lines
drivers/usb/gadget/f_dfu.c static int state_dfu_manifest_sync(struct f_dfu *f_dfu, · line 490
Quoted 11 lines
drivers/usb/gadget/f_dfu.c +10 -0
+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
Quoted 2 lines
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'".
[...]
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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 ...

Quoted 9 lines

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.

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
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 ...

Quoted 7 lines

Quoted 2 lines
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.

Quoted 29 lines

Quoted 14 lines
Quoted 5 lines
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).
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
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...
Quoted 4 lines
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.

Quoted 5 lines

Quoted 1 line
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.

Quoted 3 lines

bye,
Heiko

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Heiko Schocher

author

2014-03-12 10:01 UTC #12 lore raw

PATCH 3/3 am335x, dfu: add DFU_MANIFEST_POLL_TIMEOUT to the siemens boards

In reply to Heiko Schocher

as the siemens boards use dfu for updating a nand ubi partition add DFU_MANIFEST_POLL_TIMEOUT to them, so dfu host waits after complete transfer of the new image for DFU_MANIFEST_POLL_TIMEOUT ms before sending again an usb request. So the board have enough time to erase rest of the nand sectors.

Signed-off-by Heiko Schocher <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Tom Rini <[email protected]>

---
 include/configs/siemens-am33x-common.h | 1 +
 1 file changed, 1 insertion(+)

include/configs/siemens-am33x-common.h +1 -0

index 98b6e72..721c4e6 100644
@@ -265,6 +265,7 @@
 #define CONFIG_DFU_NAND
 #define CONFIG_CMD_DFU
 #define CONFIG_SYS_DFU_DATA_BUF_SIZE	(1 << 20)
+#define DFU_MANIFEST_POLL_TIMEOUT	25000
 
 #endif /* CONFIG_SPL_BUILD */

Signature · 1 line

1.8.3.1

Lukasz Majewski

2014-03-14 10:48 UTC #13 lore raw

PATCH 3/3 am335x, dfu: add DFU_MANIFEST_POLL_TIMEOUT to the siemens boards

Hi Heiko,

Quoted 20 lines

as the siemens boards use dfu for updating a nand ubi partition add DFU_MANIFEST_POLL_TIMEOUT to them, so dfu host waits after complete transfer of the new image for DFU_MANIFEST_POLL_TIMEOUT ms before sending again an usb request. So the board have enough time to erase rest of the nand sectors.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Tom Rini <[email protected]>
---
 include/configs/siemens-am33x-common.h | 1 +
 1 file changed, 1 insertion(+)
(quoted diff) +0 -0
b/include/configs/siemens-am33x-common.h index 98b6e72..721c4e6 100644

include/configs/siemens-am33x-common.h line 271

Quoted 7 lines

include/configs/siemens-am33x-common.h +1 -0

@@ -265,6 +265,7 @@
 #define CONFIG_DFU_NAND
 #define CONFIG_CMD_DFU
 #define CONFIG_SYS_DFU_DATA_BUF_SIZE	(1 << 20)
+#define DFU_MANIFEST_POLL_TIMEOUT	25000
 
 #endif /* CONFIG_SPL_BUILD */

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

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