musb-new, dfu: first send request answer then call completions

PATCH New Heiko Schocher 4 participants 12 messages Apr 10, 2014 – May 8, 2014 USB

lore

Share & tools

lore

Heiko Schocher

author

2014-04-10 05:08 UTC #0 lore raw

PATCH 1/2 musb-new, dfu: first send request answer then call completions

comment in ep0_txstate() states:

"report completions as soon as the fifo's loaded; there's no win
in waiting till this last packet gets acked".

This is wrong for using dfu. In the dfu usecase we must send a PollTimeout to the host, so the host can wait until the U-Boot Code is ready for answering new usb requests. So the answer which contains the PollTimeout must send *before* U-Boot calls req->complete.

The req->complete is used in the dfu case for flushing the medium, when entering DFU_STATE_dfuMANIFEST_SYNC state.

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

Cc Lukasz Majewski <[email protected]>

Cc Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

---

Tested on the dxr2 and pxm2 boards. If dfu_flush() needs longer then 5 sec, dfu-util breaks with current mainline code:

[...]
finished!
unable to read DFU status
$

With this patch, it shows again:

[...]
finished!
state(7) = dfuMANIFEST, status(0) = No error condition is present state(2) = dfuIDLE, status(0) = No error condition is present Done!
$

---
 drivers/usb/musb-new/musb_gadget_ep0.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/musb-new/musb_gadget_ep0.c +4 -4

index 6599d38..8c3b0a1 100644
@@ -576,6 +576,10 @@ static void ep0_txstate(struct musb *musb)
 	} else
 		request = NULL;
 
+	/* send it out, triggering a "txpktrdy cleared" irq */
+	musb_ep_select(musb->mregs, 0);
+	musb_writew(regs, MUSB_CSR0, csr);
+
 	/* report completions as soon as the fifo's loaded; there's no
 	 * win in waiting till this last packet gets acked.  (other than
 	 * very precise fault reporting, needed by USB TMC; possible with

@@ -588,10 +592,6 @@ static void ep0_txstate(struct musb *musb)
 			return;
 		musb->ackpend = 0;
 	}
-
-	/* send it out, triggering a "txpktrdy cleared" irq */
-	musb_ep_select(musb->mregs, 0);
-	musb_writew(regs, MUSB_CSR0, csr);
 }
 
 /*

Signature · 1 line

1.8.3.1

Heiko Schocher

author

2014-04-10 05:08 UTC #1 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.

Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.

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

Cc Lukasz Majewski <[email protected]>

Cc Kyungmin Park <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Pantelis Antoniou <[email protected]>

---
 drivers/dfu/dfu_nand.c     | 13 +++++++++++++
 drivers/usb/gadget/f_dfu.c | 14 +++++++++++++-
 include/dfu.h              |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

drivers/dfu/dfu_nand.c +13 -0

index 2d07097..ccdbef6 100644
@@ -163,6 +163,18 @@ static int dfu_flush_medium_nand(struct dfu_entity *dfu)
 	return ret;
 }
 
+unsigned int dfu_polltimeout_nand(struct dfu_entity *dfu)
+{
+	/*
+	 * Currently, Poll Timeout != 0 is only needed on nand
+	 * ubi partition, as the not used sectors need an erase
+	 */
+	if (dfu->data.nand.ubi)
+		return DFU_MANIFEST_POLL_TIMEOUT;
+
+	return DFU_DEFAULT_POLL_TIMEOUT;
+}
+
 int dfu_fill_entity_nand(struct dfu_entity *dfu, char *s)
 {
 	char *st;

@@ -211,6 +223,7 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, char *s)
 	dfu->read_medium = dfu_read_medium_nand;
 	dfu->write_medium = dfu_write_medium_nand;
 	dfu->flush_medium = dfu_flush_medium_nand;
+	dfu->poll_timeout = dfu_polltimeout_nand;
 
 	/* initial state */
 	dfu->inited = 0;

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

index de75ff1..9128add 100644
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct usb_ep *ep, struct usb_request *req)
 		  req->length, f_dfu->blk_seq_num);
 }
 
+static void dfu_set_poll_timeout_manifest(struct dfu_status *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu = dfu_get_entity(f_dfu->altsetting);
+
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat, dfu->poll_timeout(dfu));
+	else
+		dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT);
+}
+
 static void handle_getstatus(struct usb_request *req)
 {
 	struct dfu_status *dstat = (struct dfu_status *)req->buf;

@@ -190,7 +201,8 @@ static void handle_getstatus(struct usb_request *req)
 		f_dfu->dfu_state = DFU_STATE_dfuMANIFEST;
 		break;
 	case DFU_STATE_dfuMANIFEST:
-		dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT);
+		dfu_set_poll_timeout_manifest(dstat, f_dfu);
+		break;
 	default:
 		break;
 	}

include/dfu.h +1 -0

index 6c71ecb..c7fd270 100644
@@ -105,6 +105,7 @@ struct dfu_entity {
 			u64 offset, void *buf, long *len);
 
 	int (*flush_medium)(struct dfu_entity *dfu);
+	unsigned int (*poll_timeout)(struct dfu_entity *dfu);
 
 	struct list_head list;

Signature · 1 line

1.8.3.1

Marek Vasut

2014-04-10 07:54 UTC #2 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Quoted 13 lines · Heiko Schocher

add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Pantelis Antoniou <[email protected]>

[...]

Quoted 4 lines

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

@@ -174,6 +174,17 @@ static void dnload_request_flush(struct usb_ep *ep,
 struct usb_request *req) req->length, f_dfu->blk_seq_num);
 }

drivers/usb/gadget/f_dfu.c static void dnload_request_flush(struct usb_ep *ep, · line 186

Quoted 10 lines

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

+static void dfu_set_poll_timeout_manifest(struct dfu_status *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu = dfu_get_entity(f_dfu->altsetting);
+
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat, dfu->poll_timeout(dfu));
+	else
+		dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT);
+}

Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.

[...]

Best regards,
Marek Vasut

Pantelis Antoniou

2014-04-10 08:29 UTC #3 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Hi Marek,

Quoted 21 lines · Marek Vasut

Quoted 13 lines · Heiko Schocher
add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Pantelis Antoniou <[email protected]>
[...]
Quoted 4 lines
(quoted diff) +0 -0
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct usb_ep *ep,
 struct usb_request *req) req->length, f_dfu->blk_seq_num);
 }
 

Quoted 15 lines

drivers/usb/gadget/f_dfu.c static void dnload_request_flush(struct usb_ep *ep, struct usb_request *req) · line 186
Quoted 10 lines
drivers/usb/gadget/f_dfu.c +10 -0
+static void dfu_set_poll_timeout_manifest(struct dfu_status *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu = dfu_get_entity(f_dfu->altsetting);
+
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat, dfu->poll_timeout(dfu));
+	else
+		dfu_set_poll_timeout(dstat, DFU_MANIFEST_POLL_TIMEOUT);
+}
Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.

Could work, but why not a simple accessor like this:

static inline unsigned int dfu_get_poll_timeout(struct dfu_entity *dfu) {

	return dfu->poll_timeout ? dfu->poll_timeout(dfu); 
		DFU_MANIFEST_POLL_TIMEOUT);
}

and dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));

You even get the benefit of have a method to read the timeout value if we ever needed sometime in the future.

Quoted 5 lines

[...]
Best regards,
Marek Vasut

Regards

-- Pantelis

Lukasz Majewski

2014-04-10 10:08 UTC #4 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Hi Pantelis,

Quoted 49 lines

Hi Marek,
Quoted 30 lines · Marek Vasut
Quoted 13 lines · Heiko Schocher
add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Pantelis Antoniou <[email protected]>
[...]
Quoted 13 lines
(quoted diff) +4 -0
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct
 usb_ep *ep, struct usb_request *req) req->length,
 f_dfu->blk_seq_num); }
 
+static void dfu_set_poll_timeout_manifest(struct dfu_status
 *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu =
dfu_get_entity(f_dfu->altsetting); +
(quoted diff) +2 -0
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat,
dfu->poll_timeout(dfu));
Quoted 9 lines
Quoted 3 lines
(quoted diff) +2 -0
+	else
+		dfu_set_poll_timeout(dstat,
DFU_MANIFEST_POLL_TIMEOUT); +}
Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.
Could work, but why not a simple accessor like this:
static inline unsigned int dfu_get_poll_timeout(struct dfu_entity *dfu) {

Quoted 8 lines

	return dfu->poll_timeout ? dfu->poll_timeout(dfu); 
		DFU_MANIFEST_POLL_TIMEOUT);
}
and dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));
You even get the benefit of have a method to read the timeout value if we ever needed sometime in the future.

Seems reasonable for me: +1

Some comment:

Guys, please be consistent with CCing people. I didn't receive this thread. Also this original reply from Pantelis was not CCed to Heiko.

Quoted 10 lines

Quoted 6 lines
[...]
Best regards,
Marek Vasut
Regards
-- Pantelis

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Heiko Schocher

author

2014-04-10 10:21 UTC #5 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Hello Lukasz,

Am 10.04.2014 12:08, schrieb Lukasz Majewski:

Quoted 55 lines

Hi Pantelis,
Quoted 53 lines
Hi Marek,
Quoted 30 lines · Marek Vasut
Quoted 13 lines · Heiko Schocher
add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
Cc Pantelis Antoniou<[email protected]>
[...]
Quoted 13 lines
(quoted diff) +4 -0
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct
 usb_ep *ep, struct usb_request *req) req->length,
 f_dfu->blk_seq_num); }
 
+static void dfu_set_poll_timeout_manifest(struct dfu_status
 *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu =
dfu_get_entity(f_dfu->altsetting); +
(quoted diff) +2 -0
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat,
dfu->poll_timeout(dfu));
Quoted 9 lines
Quoted 3 lines
(quoted diff) +2 -0
+	else
+		dfu_set_poll_timeout(dstat,
DFU_MANIFEST_POLL_TIMEOUT); +}
Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.
Could work, but why not a simple accessor like this:
static inline unsigned int dfu_get_poll_timeout(struct dfu_entity *dfu) {
	return dfu->poll_timeout ? dfu->poll_timeout(dfu);
		DFU_MANIFEST_POLL_TIMEOUT);
}

Quoted 6 lines

Quoted 4 lines
and dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));
You even get the benefit of have a method to read the timeout value if we ever needed sometime in the future.
Seems reasonable for me: +1

Yep, good idea, I change this.

Quoted 4 lines

Some comment:
Guys, please be consistent with CCing people. I didn't receive this thread. Also this original reply from Pantelis was not CCed to Heiko.

Hmm.. I lloked in my received EMails, and I see you always on cc ... ?

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-04-10 14:31 UTC #6 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Hi Heiko,

Quoted 67 lines

Hello Lukasz,
Am 10.04.2014 12:08, schrieb Lukasz Majewski:
Quoted 55 lines
Hi Pantelis,
Quoted 53 lines
Hi Marek,
Quoted 30 lines · Marek Vasut
Quoted 13 lines · Heiko Schocher
add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
Cc Pantelis Antoniou<[email protected]>
[...]
Quoted 13 lines
(quoted diff) +4 -0
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct
 usb_ep *ep, struct usb_request *req) req->length,
 f_dfu->blk_seq_num); }
 
+static void dfu_set_poll_timeout_manifest(struct dfu_status
 *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu =
dfu_get_entity(f_dfu->altsetting); +
(quoted diff) +2 -0
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat,
dfu->poll_timeout(dfu));
Quoted 9 lines
Quoted 3 lines
(quoted diff) +2 -0
+	else
+		dfu_set_poll_timeout(dstat,
DFU_MANIFEST_POLL_TIMEOUT); +}
Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.
Could work, but why not a simple accessor like this:
static inline unsigned int dfu_get_poll_timeout(struct dfu_entity *dfu) {
	return dfu->poll_timeout ? dfu->poll_timeout(dfu);
		DFU_MANIFEST_POLL_TIMEOUT);
}
Quoted 6 lines
Quoted 4 lines
and dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));
You even get the benefit of have a method to read the timeout value if we ever needed sometime in the future.
Seems reasonable for me: +1
Yep, good idea, I change this.

Quoted 7 lines

Quoted 5 lines
Some comment:
Guys, please be consistent with CCing people. I didn't receive this thread. Also this original reply from Pantelis was not CCed to Heiko.
Hmm.. I lloked in my received EMails, and I see you always on cc ... ?

I wasn't added to CC in the original patch 2/2.

I was only added to Cc below the Signed-of-by, but then I was missing in the CC of the message itself.

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-04-11 04:22 UTC #7 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Hello Lukasz,

Am 10.04.2014 16:31, schrieb Lukasz Majewski:

Quoted 75 lines

Hi Heiko,
Quoted 73 lines
Hello Lukasz,
Am 10.04.2014 12:08, schrieb Lukasz Majewski:
Quoted 55 lines
Hi Pantelis,
Quoted 53 lines
Hi Marek,
Quoted 30 lines · Marek Vasut
Quoted 13 lines · Heiko Schocher
add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
Cc Pantelis Antoniou<[email protected]>
[...]
Quoted 13 lines
(quoted diff) +4 -0
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct
 usb_ep *ep, struct usb_request *req) req->length,
 f_dfu->blk_seq_num); }
 
+static void dfu_set_poll_timeout_manifest(struct dfu_status
 *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu =
dfu_get_entity(f_dfu->altsetting); +
(quoted diff) +2 -0
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat,
dfu->poll_timeout(dfu));
Quoted 9 lines
Quoted 3 lines
(quoted diff) +2 -0
+	else
+		dfu_set_poll_timeout(dstat,
DFU_MANIFEST_POLL_TIMEOUT); +}
Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.
Could work, but why not a simple accessor like this:
static inline unsigned int dfu_get_poll_timeout(struct dfu_entity *dfu) {
	return dfu->poll_timeout ? dfu->poll_timeout(dfu);
		DFU_MANIFEST_POLL_TIMEOUT);
}
Quoted 6 lines
Quoted 4 lines
and dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));
You even get the benefit of have a method to read the timeout value if we ever needed sometime in the future.
Seems reasonable for me: +1
Yep, good idea, I change this.
Quoted 5 lines
Some comment:
Guys, please be consistent with CCing people. I didn't receive this thread. Also this original reply from Pantelis was not CCed to Heiko.

Quoted 6 lines

Quoted 1 line
Hmm.. I lloked in my received EMails, and I see you always on cc ... ?
I wasn't added to CC in the original patch 2/2.
I was only added to Cc below the Signed-of-by, but then I was missing in the CC of the message itself.

Yes, I see ... Hmm.. I sent patches always with git send-mail ...

Header I got:

From - Thu Apr 10 07:12:33 2014
X-Account-Key: account2
X-UIDL: 1130185039.262989
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
X-Mozilla-Keys:
Return-Path: <[email protected]>
Received: from murder ([192.168.8.180])

	 by backend11 (Cyrus v2.2.12) with LMTPA;
	 Thu, 10 Apr 2014 07:08:28 +0200
X-Sieve: CMU Sieve 2.2
Received: from mail.m-online.net (localhost [127.0.0.1])
	 by frontend1.mail.m-online.net (Cyrus v2.2.12) with LMTPA;
	 Thu, 10 Apr 2014 07:08:27 +0200
[...]
Received: from pollux.denx.de (pollux [192.168.1.1])
	by mail.denx.de (Postfix) with ESMTP id D0851342155;
	Thu, 10 Apr 2014 07:08:08 +0200 (CEST)
Received: by pollux.denx.de (Postfix, from userid 515)
	id 98C9BF6E; Thu, 10 Apr 2014 07:08:08 +0200 (CEST)

From: Heiko Schocher <[email protected]>
To: u-boot at lists.denx.de
Cc: Heiko Schocher <[email protected]>,
Lukasz Majewski <[email protected]>,
Kyungmin Park <[email protected]>,
Marek Vasut <[email protected]>,
Pantelis Antoniou <[email protected]>
Subject: [PATCH 2/2] dfu, nand: add medium specific polltimeout function
Date: Thu, 10 Apr 2014 07:08:06 +0200
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1
In-Reply-To: <[email protected]>
References: <[email protected]>

There you are in the cc list ... but I see, in patchwork:

http://patchwork.ozlabs.org/patch/337981/

(click on "Headers show")
[...]
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1
In-Reply-To: <[email protected]>
References: <[email protected]>
Cc: Marek Vasut <[email protected]>,
Pantelis Antoniou <[email protected]>, Kyungmin Park <[email protected]>
Subject: [U-Boot] [PATCH 2/2] dfu,
nand: add medium specific polltimeout function

There you are missing ... ?

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-04-11 07:20 UTC #8 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

Hi Heiko,

Quoted 141 lines

Hello Lukasz,
Am 10.04.2014 16:31, schrieb Lukasz Majewski:
Quoted 76 lines
Hi Heiko,
Quoted 74 lines
Hello Lukasz,
Am 10.04.2014 12:08, schrieb Lukasz Majewski:
Quoted 56 lines
Hi Pantelis,
Quoted 54 lines
Hi Marek,
Quoted 31 lines · Marek Vasut
Quoted 13 lines · Heiko Schocher
add a possibility to add a medium specific polltimeout function. So it is possible to define different poll timeouts.
Used on nand medium, for setting the DFU_MANIFEST_POLL_TIMEOUT only on nand ubi partitions, which is currently the only usecase.
Signed-off-by Heiko Schocher<[email protected]>
Cc Lukasz Majewski<[email protected]>
Cc Kyungmin Park<[email protected]>
Cc Marek Vasut<[email protected]>
Cc Pantelis Antoniou<[email protected]>
[...]
Quoted 13 lines
(quoted diff) +4 -0
@@ -174,6 +174,17 @@ static void dnload_request_flush(struct
 usb_ep *ep, struct usb_request *req) req->length,
 f_dfu->blk_seq_num); }
 
+static void dfu_set_poll_timeout_manifest(struct dfu_status
 *dstat,
+					  struct f_dfu *f_dfu)
+{
+	struct dfu_entity *dfu =
dfu_get_entity(f_dfu->altsetting); +
(quoted diff) +2 -0
+	if (dfu->poll_timeout)
+		dfu_set_poll_timeout(dstat,
dfu->poll_timeout(dfu));
Quoted 9 lines
Quoted 3 lines
(quoted diff) +2 -0
+	else
+		dfu_set_poll_timeout(dstat,
DFU_MANIFEST_POLL_TIMEOUT); +}
Don't you think it'd be better (yet more intrusive) to have all the DFU users have default implementation of dfu->poll_timeout() ? Then you'd be able to avoid this if and even get rid of this dfu_set_poll_timeout_manifest() function.
Could work, but why not a simple accessor like this:
static inline unsigned int dfu_get_poll_timeout(struct dfu_entity *dfu) {
	return dfu->poll_timeout ? dfu->poll_timeout(dfu);
		DFU_MANIFEST_POLL_TIMEOUT);
}
Quoted 6 lines
Quoted 4 lines
and dfu_set_poll_timeout(dstat, dfu_get_poll_timeout(dfu));
You even get the benefit of have a method to read the timeout value if we ever needed sometime in the future.
Seems reasonable for me: +1
Yep, good idea, I change this.
Quoted 5 lines
Some comment:
Guys, please be consistent with CCing people. I didn't receive this thread. Also this original reply from Pantelis was not CCed to Heiko.
Quoted 7 lines
Quoted 2 lines
Hmm.. I lloked in my received EMails, and I see you always on cc ... ?
I wasn't added to CC in the original patch 2/2.
I was only added to Cc below the Signed-of-by, but then I was missing in the CC of the message itself.
Yes, I see ... Hmm.. I sent patches always with git send-mail ...
Header I got:
From - Thu Apr 10 07:12:33 2014
X-Account-Key: account2
X-UIDL: 1130185039.262989
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
X-Mozilla-Keys:
Return-Path: <[email protected]>
Received: from murder ([192.168.8.180])
	 by backend11 (Cyrus v2.2.12) with LMTPA;
	 Thu, 10 Apr 2014 07:08:28 +0200
X-Sieve: CMU Sieve 2.2
Received: from mail.m-online.net (localhost [127.0.0.1])
	 by frontend1.mail.m-online.net (Cyrus v2.2.12) with LMTPA;
	 Thu, 10 Apr 2014 07:08:27 +0200
[...]
Received: from pollux.denx.de (pollux [192.168.1.1])
	by mail.denx.de (Postfix) with ESMTP id D0851342155;
	Thu, 10 Apr 2014 07:08:08 +0200 (CEST)
Received: by pollux.denx.de (Postfix, from userid 515)
	id 98C9BF6E; Thu, 10 Apr 2014 07:08:08 +0200 (CEST)
From: Heiko Schocher <[email protected]>
To: u-boot at lists.denx.de
Cc: Heiko Schocher <[email protected]>,
Lukasz Majewski <[email protected]>, Kyungmin Park <[email protected]>, Marek Vasut <[email protected]>,
Pantelis Antoniou <[email protected]>
Subject: [PATCH 2/2] dfu, nand: add medium specific polltimeout function Date: Thu, 10 Apr 2014 07:08:06 +0200
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1
In-Reply-To: <[email protected]>
References: <[email protected]>
There you are in the cc list ... but I see, in patchwork:
http://patchwork.ozlabs.org/patch/337981/
(click on "Headers show")
[...]
Message-Id: <[email protected]>
X-Mailer: git-send-email 1.8.3.1
In-Reply-To: <[email protected]>
References: <[email protected]>
Cc: Marek Vasut <[email protected]>,
Pantelis Antoniou <[email protected]>, Kyungmin Park <[email protected]>
Subject: [U-Boot] [PATCH 2/2] dfu,
nand: add medium specific polltimeout function

Quoted 1 line

There you are missing ... ?

Yes. Here is the problem. When you reply to patch, then Cc from the commit message is not taken into account.

This is why I've asked for being consistent with CC list.

For this reason (also the cover letter apply to this), when I send patches with git-send-email, I add all concerned people with explicit --cc/--to option.

Quoted 3 lines

bye,
Heiko

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Marek Vasut

2014-04-10 11:10 UTC #9 lore raw

PATCH 2/2 dfu, nand: add medium specific polltimeout function

In reply to Lukasz Majewski

On Thursday, April 10, 2014 at 12:08:44 PM, Lukasz Majewski wrote:

[...]

Quoted 6 lines

Seems reasonable for me: +1
Some comment:
Guys, please be consistent with CCing people. I didn't receive this thread. Also this original reply from Pantelis was not CCed to Heiko.

For mutt, I have the key 'a' bound to:

bind index,pager a group-reply

Best regards,
Marek Vasut

Marek Vasut

2014-04-10 07:50 UTC #10 lore raw

PATCH 1/2 musb-new, dfu: first send request answer then call completions

In reply to Heiko Schocher

Quoted 11 lines · Heiko Schocher

comment in ep0_txstate() states:
"report completions as soon as the fifo's loaded; there's no win
in waiting till this last packet gets acked".
This is wrong for using dfu. In the dfu usecase we must send a PollTimeout to the host, so the host can wait until the U-Boot Code is ready for answering new usb requests. So the answer which contains the PollTimeout must send *before* U-Boot calls req->complete.

Quoted 8 lines

The req->complete is used in the dfu case for flushing the medium, when entering DFU_STATE_dfuMANIFEST_SYNC state.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Pantelis Antoniou <[email protected]>

To me, this looks OK, yes. We need to "commit" the packet into the hardware before calling ->complete.

Acked-by Marek Vasut <[email protected]>

Best regards,
Marek Vasut

Lukasz Majewski

2014-05-08 08:47 UTC #11 lore raw

PATCH 1/2 musb-new, dfu: first send request answer then call completions

In reply to Heiko Schocher

Hi Heiko,

Quoted 11 lines

comment in ep0_txstate() states:
"report completions as soon as the fifo's loaded; there's no win
in waiting till this last packet gets acked".
This is wrong for using dfu. In the dfu usecase we must send a PollTimeout to the host, so the host can wait until the U-Boot Code is ready for answering new usb requests. So the answer which contains the PollTimeout must send *before* U-Boot calls req->complete.

Quoted 8 lines

The req->complete is used in the dfu case for flushing the medium, when entering DFU_STATE_dfuMANIFEST_SYNC state.
Signed-off-by Heiko Schocher <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Kyungmin Park <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Pantelis Antoniou <[email protected]>

Applied to u-boot-dfu. Thanks.

Signature · 5 lines

Best regards,

Lukasz Majewski

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