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

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

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

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 <hs@denx.de>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>

---

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

diff --git a/drivers/usb/musb-new/musb_gadget_ep0.c b/drivers/usb/musb-new/musb_gadget_ep0.c
index 6599d38..8c3b0a1 100644
--- a/drivers/usb/musb-new/musb_gadget_ep0.c
+++ b/drivers/usb/musb-new/musb_gadget_ep0.c
@@ -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);
 }
 
 /*
-- 
1.8.3.1

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[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 <hs@denx.de>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/dfu/dfu_nand.c     | 13 +++++++++++++
 drivers/usb/gadget/f_dfu.c | 14 +++++++++++++-
 include/dfu.h              |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
index 2d07097..ccdbef6 100644
--- a/drivers/dfu/dfu_nand.c
+++ b/drivers/dfu/dfu_nand.c
@@ -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;
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
index de75ff1..9128add 100644
--- a/drivers/usb/gadget/f_dfu.c
+++ b/drivers/usb/gadget/f_dfu.c
@@ -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;
 	}
diff --git a/include/dfu.h b/include/dfu.h
index 6c71ecb..c7fd270 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -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;
 
-- 
1.8.3.1

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Marek Vasut]

On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
> 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 <hs@denx.de>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>

[...]

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

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

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Pantelis Antoniou]

Hi Marek,

On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:

> On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
>> 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 <hs@denx.de>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> [...]
> 
>> @@ -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);
>> +}
> 
> 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.

> 
> [...]
> 
> Best regards,
> Marek Vasut

Regards

-- Pantelis

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Lukasz Majewski]

Hi Pantelis,

> Hi Marek,
> 
> On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:
> 
> > On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
> >> 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 <hs@denx.de>
> >> Cc: Lukasz Majewski <l.majewski@samsung.com>
> >> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >> Cc: Marek Vasut <marex@denx.de>
> >> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> > 
> > [...]
> > 
> >> @@ -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); +}
> > 
> > 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.

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.


> 
> > 
> > [...]
> > 
> > Best regards,
> > Marek Vasut
> 
> Regards
> 
> -- Pantelis



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Heiko Schocher]

Hello Lukasz,

Am 10.04.2014 12:08, schrieb Lukasz Majewski:
> Hi Pantelis,
>
>> Hi Marek,
>>
>> On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:
>>
>>> On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
>>>> 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<hs@denx.de>
>>>> Cc: Lukasz Majewski<l.majewski@samsung.com>
>>>> Cc: Kyungmin Park<kyungmin.park@samsung.com>
>>>> Cc: Marek Vasut<marex@denx.de>
>>>> Cc: Pantelis Antoniou<panto@antoniou-consulting.com>
>>>
>>> [...]
>>>
>>>> @@ -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); +}
>>>
>>> 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.
>
> Seems reasonable for me: +1

Yep, good idea, I change this.

> 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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Lukasz Majewski]

Hi Heiko,

> Hello Lukasz,
> 
> Am 10.04.2014 12:08, schrieb Lukasz Majewski:
> > Hi Pantelis,
> >
> >> Hi Marek,
> >>
> >> On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:
> >>
> >>> On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
> >>>> 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<hs@denx.de>
> >>>> Cc: Lukasz Majewski<l.majewski@samsung.com>
> >>>> Cc: Kyungmin Park<kyungmin.park@samsung.com>
> >>>> Cc: Marek Vasut<marex@denx.de>
> >>>> Cc: Pantelis Antoniou<panto@antoniou-consulting.com>
> >>>
> >>> [...]
> >>>
> >>>> @@ -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); +}
> >>>
> >>> 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.
> >
> > Seems reasonable for me: +1
> 
> Yep, good idea, I change this.
> 
> > 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.

> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Heiko Schocher]

Hello Lukasz,

Am 10.04.2014 16:31, schrieb Lukasz Majewski:
> Hi Heiko,
>
>> Hello Lukasz,
>>
>> Am 10.04.2014 12:08, schrieb Lukasz Majewski:
>>> Hi Pantelis,
>>>
>>>> Hi Marek,
>>>>
>>>> On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:
>>>>
>>>>> On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher wrote:
>>>>>> 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<hs@denx.de>
>>>>>> Cc: Lukasz Majewski<l.majewski@samsung.com>
>>>>>> Cc: Kyungmin Park<kyungmin.park@samsung.com>
>>>>>> Cc: Marek Vasut<marex@denx.de>
>>>>>> Cc: Pantelis Antoniou<panto@antoniou-consulting.com>
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -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); +}
>>>>>
>>>>> 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.
>>>
>>> Seems reasonable for me: +1
>>
>> Yep, good idea, I change this.
>>
>>> 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.

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: <hs@denx.de>
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 <hs@denx.de>
To: u-boot at lists.denx.de
Cc: Heiko Schocher <hs@denx.de>,
	Lukasz Majewski <l.majewski@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Marek Vasut <marex@denx.de>,
	Pantelis Antoniou <panto@antoniou-consulting.com>
Subject: [PATCH 2/2] dfu, nand: add medium specific polltimeout function
Date: Thu, 10 Apr 2014 07:08:06 +0200
Message-Id: <1397106486-1233-2-git-send-email-hs@denx.de>
X-Mailer: git-send-email 1.8.3.1
In-Reply-To: <1397106486-1233-1-git-send-email-hs@denx.de>
References: <1397106486-1233-1-git-send-email-hs@denx.de>

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

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

(click on "Headers show")
[...]
Message-Id: <1397106486-1233-2-git-send-email-hs@denx.de>
X-Mailer: git-send-email 1.8.3.1
In-Reply-To: <1397106486-1233-1-git-send-email-hs@denx.de>
References: <1397106486-1233-1-git-send-email-hs@denx.de>
Cc: Marek Vasut <marex@denx.de>,
	Pantelis Antoniou <panto@antoniou-consulting.com>,
	Kyungmin Park <kyungmin.park@samsung.com>
Subject: [U-Boot] [PATCH 2/2] dfu,
	nand: add medium specific polltimeout function

There you are missing ... ?

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

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Lukasz Majewski]

Hi Heiko,

> Hello Lukasz,
> 
> Am 10.04.2014 16:31, schrieb Lukasz Majewski:
> > Hi Heiko,
> >
> >> Hello Lukasz,
> >>
> >> Am 10.04.2014 12:08, schrieb Lukasz Majewski:
> >>> Hi Pantelis,
> >>>
> >>>> Hi Marek,
> >>>>
> >>>> On Apr 10, 2014, at 10:54 AM, Marek Vasut wrote:
> >>>>
> >>>>> On Thursday, April 10, 2014 at 07:08:06 AM, Heiko Schocher
> >>>>> wrote:
> >>>>>> 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<hs@denx.de>
> >>>>>> Cc: Lukasz Majewski<l.majewski@samsung.com>
> >>>>>> Cc: Kyungmin Park<kyungmin.park@samsung.com>
> >>>>>> Cc: Marek Vasut<marex@denx.de>
> >>>>>> Cc: Pantelis Antoniou<panto@antoniou-consulting.com>
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> @@ -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); +}
> >>>>>
> >>>>> 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.
> >>>
> >>> Seems reasonable for me: +1
> >>
> >> Yep, good idea, I change this.
> >>
> >>> 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.
> 
> 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: <hs@denx.de>
> 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 <hs@denx.de>
> To: u-boot at lists.denx.de
> Cc: Heiko Schocher <hs@denx.de>,
> 	Lukasz Majewski <l.majewski@samsung.com>,
> 	Kyungmin Park <kyungmin.park@samsung.com>,
> 	Marek Vasut <marex@denx.de>,
> 	Pantelis Antoniou <panto@antoniou-consulting.com>
> Subject: [PATCH 2/2] dfu, nand: add medium specific polltimeout
> function Date: Thu, 10 Apr 2014 07:08:06 +0200
> Message-Id: <1397106486-1233-2-git-send-email-hs@denx.de>
> X-Mailer: git-send-email 1.8.3.1
> In-Reply-To: <1397106486-1233-1-git-send-email-hs@denx.de>
> References: <1397106486-1233-1-git-send-email-hs@denx.de>
> 
> There you are in the cc list ... but I see, in patchwork:
> 
> http://patchwork.ozlabs.org/patch/337981/
> 
> (click on "Headers show")
> [...]
> Message-Id: <1397106486-1233-2-git-send-email-hs@denx.de>
> X-Mailer: git-send-email 1.8.3.1
> In-Reply-To: <1397106486-1233-1-git-send-email-hs@denx.de>
> References: <1397106486-1233-1-git-send-email-hs@denx.de>
> Cc: Marek Vasut <marex@denx.de>,
> 	Pantelis Antoniou <panto@antoniou-consulting.com>,
> 	Kyungmin Park <kyungmin.park@samsung.com>
> Subject: [U-Boot] [PATCH 2/2] dfu,
> 	nand: add medium specific polltimeout function
> 
> 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.

> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH 2/2] dfu, nand: add medium specific polltimeout function

[Marek Vasut]

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

[...]

> 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

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

[Marek Vasut]

On Thursday, April 10, 2014 at 07:08:05 AM, Heiko Schocher wrote:
> 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 <hs@denx.de>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>

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

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

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

[Lukasz Majewski]

Hi Heiko,

> 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 <hs@denx.de>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>

Applied to u-boot-dfu. Thanks.

-- 
Best regards,

Lukasz Majewski

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