[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Lukasz Majewski]

Hi Pantelis,

> usbdescriptors.h conflicts with linux/usb/ch9.h
> Remove it.

After applying this patch, I cannot build trats target anymore.

If I remember correctly both files (usbdescriptors.h and
linux/usb/ch9.h) are necessary.

The usbdescriptors.h declares e.g.: struct usb_device_descriptor


Moreover after quick check, (I've applied all patches excluding the
patch 01/10) the dfu is _NOT_ working properly anymore.

It writes u-boot.bin, but in a way that the board is bricked after
flashing.



Regards,
Lukasz

BTW: 
1. What is your target device? What is the output of dfu mmc 0 list
command on your device?

On trats it is:
DFU alt settings list:
dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
dev: eMMC alt: 1 name: uImage layout: FAT

2. Please look into the TRATS board (especially the CONFIG_DFU_ALT
constant) for reference.

3. What is yours dfu-util version? (Mine is 0.1+svnexported)

> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/usb/gadget/f_dfu.c | 1 -
>  include/g_dnl.h            | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
> index 3ec4c65..10547e3 100644
> --- a/drivers/usb/gadget/f_dfu.c
> +++ b/drivers/usb/gadget/f_dfu.c
> @@ -25,7 +25,6 @@
>  #include <malloc.h>
>  
>  #include <linux/usb/ch9.h>
> -#include <usbdescriptors.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/composite.h>
>  
> diff --git a/include/g_dnl.h b/include/g_dnl.h
> index 0ec7440..f47395f 100644
> --- a/include/g_dnl.h
> +++ b/include/g_dnl.h
> @@ -22,7 +22,6 @@
>  #define __G_DOWNLOAD_H_
>  
>  #include <linux/usb/ch9.h>
> -#include <usbdescriptors.h>
>  #include <linux/usb/gadget.h>
>  
>  int g_dnl_register(const char *s);



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Lukasz Majewski]

Hi Pantelis,

> USB initialization shouldn't happen for all the boards.
> 

The board_usb_init() follows u-boot policy, that SoC IPs (USB) are
enabled and configured just before their usage.


> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  common/cmd_dfu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 01d6b3a..327c738 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) goto done;
>  	}
>  
> +#ifdef CONFIG_TRATS
>  	board_usb_init();
> +#endif
> +
In mine opinion this #ifdef shall be removed and each target board
using the DFU shall define board_usb_init() at board file. 

>  	g_dnl_register(s);
>  	while (1) {
>  		if (ctrlc())



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 06/10] dfu: Fix crash when wrong number of arguments given

[Lukasz Majewski]

Hi Pantelis,

> Fix obvious crash when not enough arguments are given to the dfu
> command.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  common/cmd_dfu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 327c738..83ef324 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -50,7 +50,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) if (ret)
>  		return CMD_RET_FAILURE;
>  
> -	if (strcmp(argv[3], "list") == 0) {
> +	if (argc > 3 && strcmp(argv[3], "list") == 0) {

I'd add the (argc > 4) to the very beginning check: 
if (argc < 3 || argc > 4)
	return CMD_RET_USAGE;

and leave below code unchanged:

if (strcmp(argv[3], "list") == 0) {
	dfu_show_entities();
	goto done;
}


>  		dfu_show_entities();
>  		goto done;
>  	}



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 04/10] g_dnl: Properly terminate string list.

[Lukasz Majewski]

Hi Pantelis,

> Well, not terminating the list causes very interesting crashes.
> As in changing the vendor & product ID crashes. Fun.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/usb/gadget/g_dnl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index 25da733..a5a4c1f 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -69,6 +69,7 @@ static struct usb_device_descriptor device_desc = {
>  static struct usb_string g_dnl_string_defs[] = {
>  	{ 0, manufacturer, },
>  	{ 1, product, },
> +	{  }		/* end of list */

Thanks for spotting.

>  };
>  
>  static struct usb_gadget_strings g_dnl_string_tab = {

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 08/10] dfu: Properly zero out timeout value

[Lukasz Majewski]

Hi Pantelis,

> Zero out timeout value; letting it filled with undefined values
> ends up with the dfu host hanging.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/usb/gadget/f_dfu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
> index 6494f5e..c15585c 100644
> --- a/drivers/usb/gadget/f_dfu.c
> +++ b/drivers/usb/gadget/f_dfu.c
> @@ -164,6 +164,9 @@ static void handle_getstatus(struct usb_request
> *req) 
>  	/* send status response */
>  	dstat->bStatus = f_dfu->dfu_status;
> +	dstat->bwPollTimeout[0] = 0;
> +	dstat->bwPollTimeout[1] = 0;
> +	dstat->bwPollTimeout[2] = 0;
>  	dstat->bState = f_dfu->dfu_state;
>  	dstat->iString = 0;
>  }

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/28/12 09:40, Lukasz Majewski wrote:
> Hi Pantelis,
> 
>> usbdescriptors.h conflicts with linux/usb/ch9.h Remove it.
> 
> After applying this patch, I cannot build trats target anymore.

Had you also merged u-boot-usb?  Locally, trats builds now (which is
why this is merged to masgter) but wasn't after the u-boot-usb merge.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQtjDuAAoJENk4IS6UOR1WiooP/3FZJupcB4F64VKFWN2QjO3Y
MN3SnN7NFxL5/DqiZ/kg7YAz5f3Z7eKvGRuTZGfM5nyCgqDFm0IWlzzrHkHUK6ow
Jlj3Gy0X1iQoDGCC5tCNHR84pka9f0NQ6PQrQbR7fSBGRw5iw/sFANiwWfY50FJz
49Z3rJMLbh0ocwepVHmKmXBlnv++kUxpFd7nMi8gIon70A0a/1X/y1/NIN/fZEE/
e40DHSSgocmrGfJjNb49/UNjhDMqyG9I4yvf7/7QTONkV8taaAHurprzcD/aGRpI
zHlxsfVRlZ5TftfEqIyIK3fGHlW4qdCMqjzjiTxjANImlCgeUK3JHdSbafUA9j94
9fDBwE5OZp5v32npxv8d4a9/0JPjMQSAYVq3Lmb64MyWppFkgOKFtr4StbLg9+NE
K443RM/NYhvX+bkIuDytc9OxbxKkfVvalCJow+2LosxJO9GovwZfD5BrOX7zul3j
EwhzgiFV88Vpzn3SiyGtuhauFugkadrX+3hgC6Mmh1DMYG1MxSK/Ombw7G/nKdzx
SUGRL9ij3eEU60QbZFb241TouQ3PZeB1v2P6b6er5veIbS6iUZjd6GdeRNRyWri2
Q9S+h6tHx0CSVSaRudHrAnhdtmbbCGvgzsPY7fHzMzu5hiV7Ng12Sg0Mr1TiDazu
4rd57SIVSIFLBmdNUnK3
=3b1u
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Lukasz Majewski]

Hi Tom,

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/28/12 09:40, Lukasz Majewski wrote:
> > Hi Pantelis,
> > 
> >> usbdescriptors.h conflicts with linux/usb/ch9.h Remove it.
> > 
> > After applying this patch, I cannot build trats target anymore.
> 
> Had you also merged u-boot-usb?  Locally, trats builds now (which is
> why this is merged to masgter) but wasn't after the u-boot-usb merge.
> 

Good point, I've rebased on master. 
I will do the same with u-boot-usb


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/28/12 09:47, Lukasz Majewski wrote:
> Hi Pantelis,
> 
>> USB initialization shouldn't happen for all the boards.
>> 
> 
> The board_usb_init() follows u-boot policy, that SoC IPs (USB) are 
> enabled and configured just before their usage.
> 
> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> 
>> --- common/cmd_dfu.c | 3 +++ 1 file changed, 3 insertions(+)
>> 
>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index
>> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++
>> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t
>> *cmdtp, int flag, int argc, char * const argv[]) goto done; }
>> 
>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif +
> In mine opinion this #ifdef shall be removed and each target board 
> using the DFU shall define board_usb_init() at board file.
> 

But this isn't a called-only-once place.  What are you really doing
here and are you sure it's needed every time DFU is called?

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQtjM2AAoJENk4IS6UOR1WOu8P/3253rY48k6+NgCefiNZf6GH
Sw9ZEh7fNkC3QgSkKA8/Ifa52F455UFpslftjSVHIrGIwIVc+3TCM2lOdbaBZgMi
bL0MsPKRRbujx6U69lY5A6eaFyrhPJJJcCryFoPkfzsYSuvazol/crKCs9BB24Mk
j35nvd2juxmhh4paQ9+7UVkxI50haLPVBHU7A5v8yv3i9/Cig+qwqewt+GWvIhoE
w6frRy4WyczTClWqF+KwlfT4bwJVtnHxzfl5d2qRn4C/McTzUpwVePT8xrGaS4zc
3p+VCQ269Po176sgwoud5EwJdvCNBVfFeHaTORW8UJ2zLzU4iDixx4VY9SQhTfHF
MP7Ch5p2DIJRrlEUWuRAgQwO6pICBHD+f3jw5q06hg35JWmTnltyq53M5UILGyi/
Vz+SN0xF4YnMJRvKGT9lal0OiRxr/rO63k6fl2XybEfTED6AHhvUJKBV+yb1OxV0
CTCiBRqfKQwkProdFtAAJT6+CeexV4Im2WcHQwqcKxVNqgEQhM6MBsM3DkjfE+nj
naz8ITF6Lal1C0K5dUbSPiY8KqgphXre11wJ28BFp5WSM/p/0wCrhImXxuOMeUd3
QtWT/BT2fJfKcr2bGVLTKdHVBHLWziJQK6IjO8rGnot2QbdSh+QS8n9ZrDf9rJYS
+Ha0VT7jIVP/6uF1FXdj
=puWb
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Lukasz Majewski]

Hi Tom,

> Hi Pantelis,
> 
> > usbdescriptors.h conflicts with linux/usb/ch9.h
> > Remove it.
> 
After rebasing on u-boot-usb/next below comment apply:

After applying this patch, I cannot build trats target anymore.
 

With u-boot-usb/master I can compile the u-boot for trats board with no
warnings and errors.

Unfortunately after flashing with dfu, the u-boot is _NOT_ working
properly anymore.
It seems, that some parts of the binary weren't correct.

> It writes u-boot.bin, but in a way that the board is bricked after
> flashing.

I need some time to perform the thorough review of core DFU patches
(patches 7/10, 09/10, 10/10).



> BTW: 
> 1. What is your target device? What is the output of dfu mmc 0 list
> command on your device?
> 
> On trats it is:
> DFU alt settings list:
> dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
> dev: eMMC alt: 1 name: uImage layout: FAT
> 
> 2. Please look into the TRATS board (especially the CONFIG_DFU_ALT
> constant) for reference.
> 
> 3. What is yours dfu-util version? (Mine is 0.1+svnexported)
> 
> > 
> > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> > ---
> >  drivers/usb/gadget/f_dfu.c | 1 -
> >  include/g_dnl.h            | 1 -
> >  2 files changed, 2 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
> > index 3ec4c65..10547e3 100644
> > --- a/drivers/usb/gadget/f_dfu.c
> > +++ b/drivers/usb/gadget/f_dfu.c
> > @@ -25,7 +25,6 @@
> >  #include <malloc.h>
> >  
> >  #include <linux/usb/ch9.h>
> > -#include <usbdescriptors.h>
> >  #include <linux/usb/gadget.h>
> >  #include <linux/usb/composite.h>
> >  
> > diff --git a/include/g_dnl.h b/include/g_dnl.h
> > index 0ec7440..f47395f 100644
> > --- a/include/g_dnl.h
> > +++ b/include/g_dnl.h
> > @@ -22,7 +22,6 @@
> >  #define __G_DOWNLOAD_H_
> >  
> >  #include <linux/usb/ch9.h>
> > -#include <usbdescriptors.h>
> >  #include <linux/usb/gadget.h>
> >  
> >  int g_dnl_register(const char *s);
> 
> 
> 



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Lukasz Majewski]

Hi Tom,

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/28/12 09:47, Lukasz Majewski wrote:
> > Hi Pantelis,
> > 
> >> USB initialization shouldn't happen for all the boards.
> >> 
> > 
> > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are 
> > enabled and configured just before their usage.
> > 
> > 
> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> 
> >> --- common/cmd_dfu.c | 3 +++ 1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index
> >> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++
> >> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t
> >> *cmdtp, int flag, int argc, char * const argv[]) goto done; }
> >> 
> >> +#ifdef CONFIG_TRATS board_usb_init(); +#endif +
> > In mine opinion this #ifdef shall be removed and each target board 
> > using the DFU shall define board_usb_init() at board file.
> > 
> 
> But this isn't a called-only-once place.  What are you really doing
> here and are you sure it's needed every time DFU is called?
> 

Hmm, you are correct here. 

But I don't have a good alternative for this.

One solution would be to define a static flag for it at do_dfu function
to indicate if this was executed once (however I'm reluctant do this). 


Any ideas?

> - -- 
> Tom
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJQtjM2AAoJENk4IS6UOR1WOu8P/3253rY48k6+NgCefiNZf6GH
> Sw9ZEh7fNkC3QgSkKA8/Ifa52F455UFpslftjSVHIrGIwIVc+3TCM2lOdbaBZgMi
> bL0MsPKRRbujx6U69lY5A6eaFyrhPJJJcCryFoPkfzsYSuvazol/crKCs9BB24Mk
> j35nvd2juxmhh4paQ9+7UVkxI50haLPVBHU7A5v8yv3i9/Cig+qwqewt+GWvIhoE
> w6frRy4WyczTClWqF+KwlfT4bwJVtnHxzfl5d2qRn4C/McTzUpwVePT8xrGaS4zc
> 3p+VCQ269Po176sgwoud5EwJdvCNBVfFeHaTORW8UJ2zLzU4iDixx4VY9SQhTfHF
> MP7Ch5p2DIJRrlEUWuRAgQwO6pICBHD+f3jw5q06hg35JWmTnltyq53M5UILGyi/
> Vz+SN0xF4YnMJRvKGT9lal0OiRxr/rO63k6fl2XybEfTED6AHhvUJKBV+yb1OxV0
> CTCiBRqfKQwkProdFtAAJT6+CeexV4Im2WcHQwqcKxVNqgEQhM6MBsM3DkjfE+nj
> naz8ITF6Lal1C0K5dUbSPiY8KqgphXre11wJ28BFp5WSM/p/0wCrhImXxuOMeUd3
> QtWT/BT2fJfKcr2bGVLTKdHVBHLWziJQK6IjO8rGnot2QbdSh+QS8n9ZrDf9rJYS
> +Ha0VT7jIVP/6uF1FXdj
> =puWb
> -----END PGP SIGNATURE-----



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Pantelis Antoniou]

Hi Lukasz,

On Nov 28, 2012, at 6:01 PM, Lukasz Majewski wrote:

> Hi Tom,
> 
>> Hi Pantelis,
>> 
>>> usbdescriptors.h conflicts with linux/usb/ch9.h
>>> Remove it.
>> 
> After rebasing on u-boot-usb/next below comment apply:
> 
> After applying this patch, I cannot build trats target anymore.
> 
> 
> With u-boot-usb/master I can compile the u-boot for trats board with no
> warnings and errors.
> 
> Unfortunately after flashing with dfu, the u-boot is _NOT_ working
> properly anymore.
> It seems, that some parts of the binary weren't correct.
> 

Are you writing to a file in a filesystem? I.e. FAT?

I'm in the middle of doing more extensive tests, but file write to an
FS might have problems. I am using the raw mmc interface.

There could be something there that it's missed. I'm in the middle of
doing more extensive tests.


>> It writes u-boot.bin, but in a way that the board is bricked after
>> flashing.
> 
> I need some time to perform the thorough review of core DFU patches
> (patches 7/10, 09/10, 10/10).
> 

OK.

> 
> 
>> BTW: 
>> 1. What is your target device? What is the output of dfu mmc 0 list
>> command on your device?
>> 

I'm on am335x_evm target, actual board is a beaglebone.


>> On trats it is:
>> DFU alt settings list:
>> dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
>> dev: eMMC alt: 1 name: uImage layout: FAT
>> 

# setenv dfu_alt_info 'test part 0 3'
# mmc part
U-Boot# mmc part

Partition Map for MMC device 0  --   Partition Type: DOS

Part    Start Sector    Num Sectors     UUID            Type
  1     63              112392          7a348599-01     0c Boot
  2     112455          7501331         7a348599-02     83
  3     7613786         12966           7a348599-03     83
# dfu mmc 0 list
DFU alt settings list:
dev: eMMC alt: 0 name: test layout: RAW_ADDR

Are you downloading u-boot.bin to the raw nand?
I.e. there's no boot partition?

All my tests have been downloading a small ext3 image to the mmc.
I'm in the middle of doing more extensive tests but those takes a huge
amount of time... :(

>> 2. Please look into the TRATS board (especially the CONFIG_DFU_ALT
>> constant) for reference.
>> 

Already looked there.

>> 3. What is yours dfu-util version? (Mine is 0.1+svnexported)
>> 

Compiled from source git://gitorious.org/dfu-util/dfu-util.git
Current master.

Regards

-- Pantelis


>>> 
>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>> ---
>>> drivers/usb/gadget/f_dfu.c | 1 -
>>> include/g_dnl.h            | 1 -
>>> 2 files changed, 2 deletions(-)
>>> 
>>> diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
>>> index 3ec4c65..10547e3 100644
>>> --- a/drivers/usb/gadget/f_dfu.c
>>> +++ b/drivers/usb/gadget/f_dfu.c
>>> @@ -25,7 +25,6 @@
>>> #include <malloc.h>
>>> 
>>> #include <linux/usb/ch9.h>
>>> -#include <usbdescriptors.h>
>>> #include <linux/usb/gadget.h>
>>> #include <linux/usb/composite.h>
>>> 
>>> diff --git a/include/g_dnl.h b/include/g_dnl.h
>>> index 0ec7440..f47395f 100644
>>> --- a/include/g_dnl.h
>>> +++ b/include/g_dnl.h
>>> @@ -22,7 +22,6 @@
>>> #define __G_DOWNLOAD_H_
>>> 
>>> #include <linux/usb/ch9.h>
>>> -#include <usbdescriptors.h>
>>> #include <linux/usb/gadget.h>
>>> 
>>> int g_dnl_register(const char *s);
>> 
>> 
>> 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Lukasz Majewski]

Hi Pantelis,

> Hi Lukasz,
> 
> On Nov 28, 2012, at 6:01 PM, Lukasz Majewski wrote:
> 
> > Hi Tom,
> > 
> >> Hi Pantelis,
> >> 
> >>> usbdescriptors.h conflicts with linux/usb/ch9.h
> >>> Remove it.
> >> 
> > After rebasing on u-boot-usb/next below comment apply:
> > 
> > After applying this patch, I cannot build trats target anymore.
> > 
> > 
> > With u-boot-usb/master I can compile the u-boot for trats board
> > with no warnings and errors.
> > 
> > Unfortunately after flashing with dfu, the u-boot is _NOT_ working
> > properly anymore.
> > It seems, that some parts of the binary weren't correct.
> > 
> 
> Are you writing to a file in a filesystem? I.e. FAT?
> 
> I'm in the middle of doing more extensive tests, but file write to an
> FS might have problems. I am using the raw mmc interface.
> 

I've written u-boot to RAW eMMC (based on the LBA addressing). Moreover
I was also able to write data to FAT and EXT4 fs (which uses standard
fat/ext4 load commands).

> There could be something there that it's missed. I'm in the middle of
> doing more extensive tests.
> 
> 
> >> It writes u-boot.bin, but in a way that the board is bricked after
> >> flashing.
> > 
> > I need some time to perform the thorough review of core DFU patches
> > (patches 7/10, 09/10, 10/10).
> > 
> 
> OK.
> 
> > 
> > 
> >> BTW: 
> >> 1. What is your target device? What is the output of dfu mmc 0 list
> >> command on your device?
> >> 
> 
> I'm on am335x_evm target, actual board is a beaglebone.
> 
> 
> >> On trats it is:
> >> DFU alt settings list:
> >> dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
> >> dev: eMMC alt: 1 name: uImage layout: FAT
> >> 
> 
> # setenv dfu_alt_info 'test part 0 3'
> # mmc part
> U-Boot# mmc part
> 
> Partition Map for MMC device 0  --   Partition Type: DOS
> 
> Part    Start Sector    Num Sectors     UUID            Type
>   1     63              112392          7a348599-01     0c Boot
>   2     112455          7501331         7a348599-02     83
>   3     7613786         12966           7a348599-03     83
> # dfu mmc 0 list
> DFU alt settings list:
> dev: eMMC alt: 0 name: test layout: RAW_ADDR
> 
Off topic: 
It would be nice to have all partitions listed with dfu mmc 0
list :-) and then have access to it via dfu-util tool (as a separate
alt settings). But this is a task for the future :-).


> Are you downloading u-boot.bin to the raw nand?
> I.e. there's no boot partition?

Yes, there isn't any partition for u-boot. I write to raw eMMC address.

> 
> All my tests have been downloading a small ext3 image to the mmc.
> I'm in the middle of doing more extensive tests but those takes a huge
> amount of time... :(

This is because of very low DFU transmission speed (It uses only EP0
with EPS=64B , so it is meant to transfer really small files).

Updating rootfs via DFU would take much time. It is OK, to transfer
u-boot, uImage, some log data.

> 
> >> 2. Please look into the TRATS board (especially the CONFIG_DFU_ALT
> >> constant) for reference.
> >> 
> 
> Already looked there.
> 
> >> 3. What is yours dfu-util version? (Mine is 0.1+svnexported)
> >> 
> 
> Compiled from source git://gitorious.org/dfu-util/dfu-util.git
> Current master.
> 
> Regards
> 
> -- Pantelis
> 
> 
> >>> 
> >>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>> ---
> >>> drivers/usb/gadget/f_dfu.c | 1 -
> >>> include/g_dnl.h            | 1 -
> >>> 2 files changed, 2 deletions(-)
> >>> 
> >>> diff --git a/drivers/usb/gadget/f_dfu.c
> >>> b/drivers/usb/gadget/f_dfu.c index 3ec4c65..10547e3 100644
> >>> --- a/drivers/usb/gadget/f_dfu.c
> >>> +++ b/drivers/usb/gadget/f_dfu.c
> >>> @@ -25,7 +25,6 @@
> >>> #include <malloc.h>
> >>> 
> >>> #include <linux/usb/ch9.h>
> >>> -#include <usbdescriptors.h>
> >>> #include <linux/usb/gadget.h>
> >>> #include <linux/usb/composite.h>
> >>> 
> >>> diff --git a/include/g_dnl.h b/include/g_dnl.h
> >>> index 0ec7440..f47395f 100644
> >>> --- a/include/g_dnl.h
> >>> +++ b/include/g_dnl.h
> >>> @@ -22,7 +22,6 @@
> >>> #define __G_DOWNLOAD_H_
> >>> 
> >>> #include <linux/usb/ch9.h>
> >>> -#include <usbdescriptors.h>
> >>> #include <linux/usb/gadget.h>
> >>> 
> >>> int g_dnl_register(const char *s);
> >> 
> >> 
> >> 
> > 
> > 
> > 
> > -- 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > Samsung Poland R&D Center | Linux Platform Group
> 



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Pantelis Antoniou]

Hi Lukasz,

On Nov 28, 2012, at 7:46 PM, Lukasz Majewski wrote:

> Hi Pantelis,
> 
>> Hi Lukasz,
>> 
>> On Nov 28, 2012, at 6:01 PM, Lukasz Majewski wrote:
>> 
>>> Hi Tom,
>>> 
>>>> Hi Pantelis,
>>>> 
>>>>> usbdescriptors.h conflicts with linux/usb/ch9.h
>>>>> Remove it.
>>>> 
>>> After rebasing on u-boot-usb/next below comment apply:
>>> 
>>> After applying this patch, I cannot build trats target anymore.
>>> 
>>> 
>>> With u-boot-usb/master I can compile the u-boot for trats board
>>> with no warnings and errors.
>>> 
>>> Unfortunately after flashing with dfu, the u-boot is _NOT_ working
>>> properly anymore.
>>> It seems, that some parts of the binary weren't correct.
>>> 
>> 
>> Are you writing to a file in a filesystem? I.e. FAT?
>> 
>> I'm in the middle of doing more extensive tests, but file write to an
>> FS might have problems. I am using the raw mmc interface.
>> 
> 
> I've written u-boot to RAW eMMC (based on the LBA addressing). Moreover
> I was also able to write data to FAT and EXT4 fs (which uses standard
> fat/ext4 load commands).
> 

I see. Note that ext4 writes won't work; there is no offset to the
write file command. You will have to have use a large enough buffer
to handle the file's worst case.

>> There could be something there that it's missed. I'm in the middle of
>> doing more extensive tests.
>> 
>> 
>>>> It writes u-boot.bin, but in a way that the board is bricked after
>>>> flashing.
>>> 
>>> I need some time to perform the thorough review of core DFU patches
>>> (patches 7/10, 09/10, 10/10).
>>> 
>> 
>> OK.
>> 
>>> 
>>> 
>>>> BTW: 
>>>> 1. What is your target device? What is the output of dfu mmc 0 list
>>>> command on your device?
>>>> 
>> 
>> I'm on am335x_evm target, actual board is a beaglebone.
>> 
>> 
>>>> On trats it is:
>>>> DFU alt settings list:
>>>> dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
>>>> dev: eMMC alt: 1 name: uImage layout: FAT
>>>> 
>> 
>> # setenv dfu_alt_info 'test part 0 3'
>> # mmc part
>> U-Boot# mmc part
>> 
>> Partition Map for MMC device 0  --   Partition Type: DOS
>> 
>> Part    Start Sector    Num Sectors     UUID            Type
>>  1     63              112392          7a348599-01     0c Boot
>>  2     112455          7501331         7a348599-02     83
>>  3     7613786         12966           7a348599-03     83
>> # dfu mmc 0 list
>> DFU alt settings list:
>> dev: eMMC alt: 0 name: test layout: RAW_ADDR
>> 
> Off topic: 
> It would be nice to have all partitions listed with dfu mmc 0
> list :-) and then have access to it via dfu-util tool (as a separate
> alt settings). But this is a task for the future :-).
> 
> 
>> Are you downloading u-boot.bin to the raw nand?
>> I.e. there's no boot partition?
> 
> Yes, there isn't any partition for u-boot. I write to raw eMMC address.
> 
>> 
>> All my tests have been downloading a small ext3 image to the mmc.
>> I'm in the middle of doing more extensive tests but those takes a huge
>> amount of time... :(
> 
> This is because of very low DFU transmission speed (It uses only EP0
> with EPS=64B , so it is meant to transfer really small files).
> 
> Updating rootfs via DFU would take much time. It is OK, to transfer
> u-boot, uImage, some log data.
> 

We have customers that do want to use DFU for all of their initial programming.

The limitations are known, but the fact is that they find it very convenient
for their use. And it's not like you're limited by the USB interface, most of
the time is spent writing to the medium.

BTW, I've just confirmed that larger transfers get corrupted... :(
Working on a fix now...

>> 
>>>> 2. Please look into the TRATS board (especially the CONFIG_DFU_ALT
>>>> constant) for reference.
>>>> 
>> 
>> Already looked there.
>> 
>>>> 3. What is yours dfu-util version? (Mine is 0.1+svnexported)
>>>> 
>> 
>> Compiled from source git://gitorious.org/dfu-util/dfu-util.git
>> Current master.
>> 
>> Regards
>> 
>> -- Pantelis
>> 
>> 
>>>>> 
>>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>> ---
>>>>> drivers/usb/gadget/f_dfu.c | 1 -
>>>>> include/g_dnl.h            | 1 -
>>>>> 2 files changed, 2 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/usb/gadget/f_dfu.c
>>>>> b/drivers/usb/gadget/f_dfu.c index 3ec4c65..10547e3 100644
>>>>> --- a/drivers/usb/gadget/f_dfu.c
>>>>> +++ b/drivers/usb/gadget/f_dfu.c
>>>>> @@ -25,7 +25,6 @@
>>>>> #include <malloc.h>
>>>>> 
>>>>> #include <linux/usb/ch9.h>
>>>>> -#include <usbdescriptors.h>
>>>>> #include <linux/usb/gadget.h>
>>>>> #include <linux/usb/composite.h>
>>>>> 
>>>>> diff --git a/include/g_dnl.h b/include/g_dnl.h
>>>>> index 0ec7440..f47395f 100644
>>>>> --- a/include/g_dnl.h
>>>>> +++ b/include/g_dnl.h
>>>>> @@ -22,7 +22,6 @@
>>>>> #define __G_DOWNLOAD_H_
>>>>> 
>>>>> #include <linux/usb/ch9.h>
>>>>> -#include <usbdescriptors.h>
>>>>> #include <linux/usb/gadget.h>
>>>>> 
>>>>> int g_dnl_register(const char *s);
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> 
>>> -- 
>>> Best regards,
>>> 
>>> Lukasz Majewski
>>> 
>>> Samsung Poland R&D Center | Linux Platform Group
>> 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 00/10] USB: Gadget & DFU related fixes

[Marek Vasut]

Dear Pantelis Antoniou,

> Various bugfixes, g_dnl & DFU updates.
> 
> Pantelis Antoniou (10):
>   usb: Remove obsolete header file
>   usb: Fix bug when both DFU & ETHER are defined
>   g_dnl: Issue connect/disconnect as appropriate
>   g_dnl: Properly terminate string list.
>   dfu: Only perform DFU board_usb_init() for TRATS
>   dfu: Fix crash when wrong number of arguments given
>   dfu: Send correct DFU response from composite_setup
>   dfu: Properly zero out timeout value
>   dfu: Add a partition type target
>   dfu: Support larger than memory transfers.
[...]

So this is a V2 patchset? Anyway, can you please rebase on top of u-boot-
usb/master and repost? I rebased it on top of u-boot/master and pushed.

Best regards,
Marek Vasut

[U-Boot] [PATCH 00/10] USB: Gadget & DFU related fixes

[Pantelis Antoniou]

Various bugfixes, g_dnl & DFU updates.

Pantelis Antoniou (10):
  usb: Remove obsolete header file
  usb: Fix bug when both DFU & ETHER are defined
  g_dnl: Issue connect/disconnect as appropriate
  g_dnl: Properly terminate string list.
  dfu: Only perform DFU board_usb_init() for TRATS
  dfu: Fix crash when wrong number of arguments given
  dfu: Send correct DFU response from composite_setup
  dfu: Properly zero out timeout value
  dfu: Add a partition type target
  dfu: Support larger than memory transfers.

 common/cmd_dfu.c               |   5 +-
 drivers/dfu/dfu.c              | 206 +++++++++++++++++++++++++++++++----------
 drivers/dfu/dfu_mmc.c          | 106 ++++++++++++++++-----
 drivers/usb/gadget/Makefile    |  13 ++-
 drivers/usb/gadget/composite.c |  27 ++++++
 drivers/usb/gadget/ep0.c       |   1 +
 drivers/usb/gadget/f_dfu.c     |  10 +-
 drivers/usb/gadget/g_dnl.c     |  12 ++-
 include/dfu.h                  |  19 +++-
 include/g_dnl.h                |   1 -
 10 files changed, 314 insertions(+), 86 deletions(-)

-- 
1.7.12

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Pantelis Antoniou]

usbdescriptors.h conflicts with linux/usb/ch9.h
Remove it.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/usb/gadget/f_dfu.c | 1 -
 include/g_dnl.h            | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
index 3ec4c65..10547e3 100644
--- a/drivers/usb/gadget/f_dfu.c
+++ b/drivers/usb/gadget/f_dfu.c
@@ -25,7 +25,6 @@
 #include <malloc.h>
 
 #include <linux/usb/ch9.h>
-#include <usbdescriptors.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/composite.h>
 
diff --git a/include/g_dnl.h b/include/g_dnl.h
index 0ec7440..f47395f 100644
--- a/include/g_dnl.h
+++ b/include/g_dnl.h
@@ -22,7 +22,6 @@
 #define __G_DOWNLOAD_H_
 
 #include <linux/usb/ch9.h>
-#include <usbdescriptors.h>
 #include <linux/usb/gadget.h>
 
 int g_dnl_register(const char *s);
-- 
1.7.12

[U-Boot] [PATCH 02/10] usb: Fix bug when both DFU & ETHER are defined

[Pantelis Antoniou]

When both CONFIG_USB_GADGET & CONFIG_USB_ETHER are defined
the makefile links objects twice.

Detect this and fix it with a not very elegant way in the
makefile. Revisit and clean it later.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/usb/gadget/Makefile | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 040eaba..15206cd 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -26,14 +26,23 @@ include $(TOPDIR)/config.mk
 LIB	:= $(obj)libusb_gadget.o
 
 # new USB gadget layer dependencies
+
+# ugh; ugh; ugh common objects included twice
+ifdef CONFIG_USB_GADGET
+   COBJS-y += epautoconf.o config.o usbstring.o
+else
+  ifdef CONFIG_USB_ETHER
+     COBJS-y += epautoconf.o config.o usbstring.o
+  endif
+endif
+
 ifdef CONFIG_USB_GADGET
-COBJS-y += epautoconf.o config.o usbstring.o
 COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o
 COBJS-$(CONFIG_USBDOWNLOAD_GADGET) += g_dnl.o
 COBJS-$(CONFIG_DFU_FUNCTION) += f_dfu.o
 endif
 ifdef CONFIG_USB_ETHER
-COBJS-y += ether.o epautoconf.o config.o usbstring.o
+COBJS-y += ether.o
 COBJS-$(CONFIG_USB_ETH_RNDIS) += rndis.o
 COBJS-$(CONFIG_MV_UDC)	+= mv_udc.o
 COBJS-$(CONFIG_CPU_PXA25X) += pxa25x_udc.o
-- 
1.7.12

[U-Boot] [PATCH 03/10] g_dnl: Issue connect/disconnect as appropriate

[Pantelis Antoniou]

Call usb_gadget_connect/usb_gadget_disconnect in g_dnl_bind/g_dnl_unbind.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/usb/gadget/g_dnl.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index 7d87050..25da733 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -83,7 +83,12 @@ static struct usb_gadget_strings *g_dnl_composite_strings[] = {
 
 static int g_dnl_unbind(struct usb_composite_dev *cdev)
 {
-	debug("%s\n", __func__);
+	struct usb_gadget *gadget = cdev->gadget;
+
+	debug("%s: calling usb_gadget_disconnect for "
+			"controller '%s'\n", shortname, gadget->name);
+	usb_gadget_disconnect(gadget);
+
 	return 0;
 }
 
@@ -153,6 +158,10 @@ static int g_dnl_bind(struct usb_composite_dev *cdev)
 		device_desc.bcdDevice = __constant_cpu_to_le16(0x9999);
 	}
 
+	debug("%s: calling usb_gadget_connect for "
+			"controller '%s'\n", shortname, gadget->name);
+	usb_gadget_connect(gadget);
+
 	return 0;
 
  error:
-- 
1.7.12

[U-Boot] [PATCH 04/10] g_dnl: Properly terminate string list.

[Pantelis Antoniou]

Well, not terminating the list causes very interesting crashes.
As in changing the vendor & product ID crashes. Fun.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/usb/gadget/g_dnl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index 25da733..a5a4c1f 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -69,6 +69,7 @@ static struct usb_device_descriptor device_desc = {
 static struct usb_string g_dnl_string_defs[] = {
 	{ 0, manufacturer, },
 	{ 1, product, },
+	{  }		/* end of list */
 };
 
 static struct usb_gadget_strings g_dnl_string_tab = {
-- 
1.7.12

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Pantelis Antoniou]

USB initialization shouldn't happen for all the boards.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 common/cmd_dfu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
index 01d6b3a..327c738 100644
--- a/common/cmd_dfu.c
+++ b/common/cmd_dfu.c
@@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		goto done;
 	}
 
+#ifdef CONFIG_TRATS
 	board_usb_init();
+#endif
+
 	g_dnl_register(s);
 	while (1) {
 		if (ctrlc())
-- 
1.7.12

[U-Boot] [PATCH 06/10] dfu: Fix crash when wrong number of arguments given

[Pantelis Antoniou]

Fix obvious crash when not enough arguments are given to the dfu
command.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 common/cmd_dfu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
index 327c738..83ef324 100644
--- a/common/cmd_dfu.c
+++ b/common/cmd_dfu.c
@@ -50,7 +50,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (ret)
 		return CMD_RET_FAILURE;
 
-	if (strcmp(argv[3], "list") == 0) {
+	if (argc > 3 && strcmp(argv[3], "list") == 0) {
 		dfu_show_entities();
 		goto done;
 	}
-- 
1.7.12

[U-Boot] [PATCH 07/10] dfu: Send correct DFU response from composite_setup

[Pantelis Antoniou]

DFU is a bit peculiar. It needs to hook to composite setup and
return it's function descriptor.

So when get-descriptor request comes with a type of DFU_DT_FUNC
we iterate over the configs, and functions, and when we find
the DFU function we call the setup method which is prepared
to return the appropriate function descriptor.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/usb/gadget/composite.c | 27 +++++++++++++++++++++++++++
 drivers/usb/gadget/ep0.c       |  1 +
 drivers/usb/gadget/f_dfu.c     |  6 ++++--
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index ebb5131..6496436 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -773,6 +773,33 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 			if (value >= 0)
 				value = min(w_length, (u16) value);
 			break;
+
+#ifdef CONFIG_DFU_FUNCTION
+		/* DFU is mighty weird */
+		case DFU_DT_FUNC:
+			w_value &= 0xff;
+			list_for_each_entry(c, &cdev->configs, list) {
+				if (w_value != 0) {
+					w_value--;
+					continue;
+				}
+
+				list_for_each_entry(f, &c->functions, list) {
+
+					/* DFU function only */
+					if (strcmp(f->name, "dfu") != 0)
+						continue;
+
+					value = f->setup(f, ctrl);
+					goto dfu_func_done;
+				}
+			}
+dfu_func_done:
+			if (value >= 0)
+				value = min(w_length, (u16) value);
+			break;
+#endif
+
 		default:
 			goto unknown;
 		}
diff --git a/drivers/usb/gadget/ep0.c b/drivers/usb/gadget/ep0.c
index aa8f916..971d846 100644
--- a/drivers/usb/gadget/ep0.c
+++ b/drivers/usb/gadget/ep0.c
@@ -221,6 +221,7 @@ static int ep0_get_descriptor (struct usb_device_instance *device,
 		break;
 
 	case USB_DESCRIPTOR_TYPE_CONFIGURATION:
+	case USB_DESCRIPTOR_TYPE_OTHER_SPEED_CONFIGURATION:
 		{
 			struct usb_configuration_descriptor
 				*configuration_descriptor;
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
index 10547e3..6494f5e 100644
--- a/drivers/usb/gadget/f_dfu.c
+++ b/drivers/usb/gadget/f_dfu.c
@@ -534,8 +534,10 @@ dfu_handle(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 			value = min(len, (u16) sizeof(dfu_func));
 			memcpy(req->buf, &dfu_func, value);
 		}
-	} else /* DFU specific request */
-		value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl, gadget, req);
+		return value;
+	}
+
+	value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl, gadget, req);
 
 	if (value >= 0) {
 		req->length = value;
-- 
1.7.12

[U-Boot] [PATCH 08/10] dfu: Properly zero out timeout value

[Pantelis Antoniou]

Zero out timeout value; letting it filled with undefined values
ends up with the dfu host hanging.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/usb/gadget/f_dfu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
index 6494f5e..c15585c 100644
--- a/drivers/usb/gadget/f_dfu.c
+++ b/drivers/usb/gadget/f_dfu.c
@@ -164,6 +164,9 @@ static void handle_getstatus(struct usb_request *req)
 
 	/* send status response */
 	dstat->bStatus = f_dfu->dfu_status;
+	dstat->bwPollTimeout[0] = 0;
+	dstat->bwPollTimeout[1] = 0;
+	dstat->bwPollTimeout[2] = 0;
 	dstat->bState = f_dfu->dfu_state;
 	dstat->iString = 0;
 }
-- 
1.7.12

[U-Boot] [PATCH 09/10] dfu: Add a partition type target

[Pantelis Antoniou]

Dealing with raw block numbers with the dfu is very annoying.
Introduce a partition method.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/dfu/dfu_mmc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 5d504df..083d745 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -21,6 +21,7 @@
 
 #include <common.h>
 #include <malloc.h>
+#include <errno.h>
 #include <dfu.h>
 
 enum dfu_mmc_op {
@@ -153,6 +154,10 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
 
 int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s)
 {
+	int dev, part;
+	struct mmc *mmc;
+	block_dev_desc_t *blk_dev;
+	disk_partition_t partinfo;
 	char *st;
 
 	dfu->dev_type = DFU_DEV_MMC;
@@ -166,8 +171,34 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s)
 		dfu->layout = DFU_FS_FAT;
 	} else if (!strcmp(st, "ext4")) {
 		dfu->layout = DFU_FS_EXT4;
+	} else if (!strcmp(st, "part")) {
+
+		dfu->layout = DFU_RAW_ADDR;
+
+		dev = simple_strtoul(s, &s, 10);
+		s++;
+		part = simple_strtoul(s, &s, 10);
+
+		mmc = find_mmc_device(dev);
+		if (mmc == NULL || mmc_init(mmc)) {
+			printf("%s: could not find mmc device #%d!\n", __func__, dev);
+			return -ENODEV;
+		}
+
+		blk_dev = &mmc->block_dev;
+		if (get_partition_info(blk_dev, part, &partinfo) != 0) {
+			printf("%s: could not find partition #%d on mmc device #%d!\n",
+					__func__, part, dev);
+			return -ENODEV;
+		}
+
+		dfu->data.mmc.lba_start = partinfo.start;
+		dfu->data.mmc.lba_size = partinfo.size;
+		dfu->data.mmc.lba_blk_size = partinfo.blksz;
+
 	} else {
 		printf("%s: Memory layout (%s) not supported!\n", __func__, st);
+		return -ENODEV;
 	}
 
 	if (dfu->layout == DFU_FS_EXT4 || dfu->layout == DFU_FS_FAT) {
-- 
1.7.12

[U-Boot] [PATCH 10/10] dfu: Support larger than memory transfers.

[Pantelis Antoniou]

We didn't support upload/download larger than available memory.
This is pretty bad when you have to update your root filesystem for
example.

This patch removes the limitation (and the crashes when you transfered
any file larger than 4MB).
On top of that reduces the huge dfu buffer from 4MB to just 64K, which
was over the top.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/dfu/dfu.c     | 206 ++++++++++++++++++++++++++++++++++++++------------
 drivers/dfu/dfu_mmc.c |  79 ++++++++++++-------
 include/dfu.h         |  19 ++++-
 3 files changed, 224 insertions(+), 80 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index e8477fb..1260c55 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -44,90 +44,196 @@ static int dfu_find_alt_num(const char *s)
 static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
 				     dfu_buf[DFU_DATA_BUF_SIZE];
 
+static int dfu_write_buffer_flush(struct dfu_entity *dfu)
+{
+	long w_size;
+	int ret;
+
+	/* flush size? */
+	w_size = dfu->i_buf - dfu->i_buf_start;
+	if (w_size == 0)
+		return 0;
+
+	/* update CRC32 */
+	dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
+
+	ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start, &w_size);
+	if (ret)
+		debug("%s: Write error!\n", __func__);
+
+	/* point back */
+	dfu->i_buf = dfu->i_buf_start;
+
+	/* update offset */
+	dfu->offset += w_size;
+
+	puts("#");
+
+	return ret;
+}
+
 int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 {
-	static unsigned char *i_buf;
-	static int i_blk_seq_num;
-	long w_size = 0;
 	int ret = 0;
+	int tret;
 
-	debug("%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x i_buf: 0x%p\n",
-	       __func__, dfu->name, buf, size, blk_seq_num, i_buf);
+	debug("%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x "
+			"offset: 0x%llx bufoffset: 0x%x\n",
+	       __func__, dfu->name, buf, size, blk_seq_num, dfu->offset,
+	       dfu->i_buf - dfu->i_buf_start);
 
 	if (blk_seq_num == 0) {
-		i_buf = dfu_buf;
-		i_blk_seq_num = 0;
+		/* initial state */
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_blk_seq_num = 0;
+		dfu->i_buf_start = dfu_buf;
+		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf = dfu->i_buf_start;
 	}
 
-	if (i_blk_seq_num++ != blk_seq_num) {
+	if (dfu->i_blk_seq_num != blk_seq_num) {
 		printf("%s: Wrong sequence number! [%d] [%d]\n",
-		       __func__, i_blk_seq_num, blk_seq_num);
+		       __func__, dfu->i_blk_seq_num, blk_seq_num);
 		return -1;
 	}
+	dfu->i_blk_seq_num++;
+
+	/* flush buffer if overflow */
+	if ((dfu->i_buf + size) > dfu->i_buf_end) {
+		tret = dfu_write_buffer_flush(dfu);
+		if (ret == 0)
+			ret = tret;
+	}
 
-	memcpy(i_buf, buf, size);
-	i_buf += size;
+	memcpy(dfu->i_buf, buf, size);
+	dfu->i_buf += size;
 
+	/* if end or if buffer full flush */
+	if (size == 0 || (dfu->i_buf + size) > dfu->i_buf_end) {
+		tret = dfu_write_buffer_flush(dfu);
+		if (ret == 0)
+			ret = tret;
+	}
+
+	/* end? */
 	if (size == 0) {
-		/* Integrity check (if needed) */
-		debug("%s: %s %d [B] CRC32: 0x%x\n", __func__, dfu->name,
-		       i_buf - dfu_buf, crc32(0, dfu_buf, i_buf - dfu_buf));
+		debug("%s: DFU complete CRC32: 0x%x\n", __func__, dfu->crc);
 
-		w_size = i_buf - dfu_buf;
-		ret = dfu->write_medium(dfu, dfu_buf, &w_size);
-		if (ret)
-			debug("%s: Write error!\n", __func__);
+		puts("\nDownload complete\n");
 
-		i_blk_seq_num = 0;
-		i_buf = NULL;
-		return ret;
+		/* clear everything */
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_blk_seq_num = 0;
+		dfu->i_buf_start = dfu_buf;
+		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf = dfu->i_buf_start;
 	}
 
-	return ret;
+	return ret = 0 ? size : ret;
+}
+
+static int dfu_read_buffer_fill(struct dfu_entity *dfu, void *buf, int size)
+{
+	long chunk;
+	int ret, readn;
+
+	readn = 0;
+	while (size > 0) {
+
+		/* get chunk that can be read */
+		chunk = min(size, dfu->b_left);
+		/* consume */
+		if (chunk > 0) {
+			memcpy(buf, dfu->i_buf, chunk);
+			dfu->crc = crc32(dfu->crc, buf, chunk);
+			dfu->i_buf += chunk;
+			dfu->b_left -= chunk;
+			size -= chunk;
+			buf += chunk;
+			readn += chunk;
+		}
+
+		/* all done */
+		if (size > 0) {
+			/* no more to read */
+			if (dfu->r_left == 0)
+				break;
+
+			dfu->i_buf = dfu->i_buf_start;
+			dfu->b_left = dfu->i_buf_end - dfu->i_buf_start;
+
+			/* got to read, but buffer is empty */
+			if (dfu->b_left > dfu->r_left)
+				dfu->b_left = dfu->r_left;
+			ret = dfu->read_medium(dfu, dfu->offset, dfu->i_buf,
+					&dfu->b_left);
+			if (ret != 0) {
+				debug("%s: Read error!\n", __func__);
+				return ret;
+			}
+			dfu->offset += dfu->b_left;
+			dfu->r_left -= dfu->b_left;
+
+			puts("#");
+		}
+	}
+
+	return readn;
 }
 
 int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 {
-	static unsigned char *i_buf;
-	static int i_blk_seq_num;
-	static long r_size;
-	static u32 crc;
 	int ret = 0;
 
 	debug("%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x i_buf: 0x%p\n",
-	       __func__, dfu->name, buf, size, blk_seq_num, i_buf);
+	       __func__, dfu->name, buf, size, blk_seq_num, dfu->i_buf);
 
 	if (blk_seq_num == 0) {
-		i_buf = dfu_buf;
-		ret = dfu->read_medium(dfu, i_buf, &r_size);
-		debug("%s: %s %ld [B]\n", __func__, dfu->name, r_size);
-		i_blk_seq_num = 0;
-		/* Integrity check (if needed) */
-		crc = crc32(0, dfu_buf, r_size);
+		ret = dfu->read_medium(dfu, 0, NULL, &dfu->r_left);
+		if (ret != 0) {
+			debug("%s: failed to get r_left\n", __func__);
+			return ret;
+		}
+
+		debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
+
+		dfu->i_blk_seq_num = 0;
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_buf_start = dfu_buf;
+		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf = dfu->i_buf_start;
+		dfu->b_left = 0;
 	}
 
-	if (i_blk_seq_num++ != blk_seq_num) {
+	if (dfu->i_blk_seq_num != blk_seq_num) {
 		printf("%s: Wrong sequence number! [%d] [%d]\n",
-		       __func__, i_blk_seq_num, blk_seq_num);
+		       __func__, dfu->i_blk_seq_num, blk_seq_num);
 		return -1;
 	}
+	dfu->i_blk_seq_num++;
 
-	if (r_size >= size) {
-		memcpy(buf, i_buf, size);
-		i_buf += size;
-		r_size -= size;
-		return size;
-	} else {
-		memcpy(buf, i_buf, r_size);
-		i_buf += r_size;
-		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name, crc);
-		puts("UPLOAD ... done\nCtrl+C to exit ...\n");
-
-		i_buf = NULL;
-		i_blk_seq_num = 0;
-		crc = 0;
-		return r_size;
+	ret = dfu_read_buffer_fill(dfu, buf, size);
+	if (ret < 0) {
+		printf("%s: Failed to fill buffer\n", __func__);
+		return -1;
 	}
+
+	if (ret < size) {
+		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name, dfu->crc);
+		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
+
+		dfu->i_blk_seq_num = 0;
+		dfu->crc = 0;
+		dfu->offset = 0;
+		dfu->i_buf_start = dfu_buf;
+		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf = dfu->i_buf_start;
+		dfu->b_left = 0;
+	}
+
 	return ret;
 }
 
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 083d745..4c8994b 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -22,6 +22,7 @@
 #include <common.h>
 #include <malloc.h>
 #include <errno.h>
+#include <div64.h>
 #include <dfu.h>
 
 enum dfu_mmc_op {
@@ -30,35 +31,48 @@ enum dfu_mmc_op {
 };
 
 static int mmc_block_op(enum dfu_mmc_op op, struct dfu_entity *dfu,
-			void *buf, long *len)
+			u64 offset, void *buf, long *len)
 {
 	char cmd_buf[DFU_CMD_BUF_SIZE];
+	u32 blk_start, blk_count;
 
-	sprintf(cmd_buf, "mmc %s 0x%x %x %x",
-		op == DFU_OP_READ ? "read" : "write",
-		(unsigned int) buf,
-		dfu->data.mmc.lba_start,
-		dfu->data.mmc.lba_size);
-
-	if (op == DFU_OP_READ)
+	/* if buf == NULL return total size of the area */
+	if (buf == NULL) {
 		*len = dfu->data.mmc.lba_blk_size * dfu->data.mmc.lba_size;
+		return 0;
+	}
+
+	blk_start = dfu->data.mmc.lba_start +
+			(u32)lldiv(offset, dfu->data.mmc.lba_blk_size);
+	blk_count = *len / dfu->data.mmc.lba_blk_size;
+	if (blk_start + blk_count >
+			dfu->data.mmc.lba_start + dfu->data.mmc.lba_size) {
+		debug("%s: block_op out of bounds\n", __func__);
+		return -1;
+	}
+
+	sprintf(cmd_buf, "mmc %s %p %x %x",
+		op == DFU_OP_READ ? "read" : "write",
+		 buf, blk_start, blk_count);
 
 	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
 	return run_command(cmd_buf, 0);
 }
 
-static inline int mmc_block_write(struct dfu_entity *dfu, void *buf, long *len)
+static inline int mmc_block_write(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
 {
-	return mmc_block_op(DFU_OP_WRITE, dfu, buf, len);
+	return mmc_block_op(DFU_OP_WRITE, dfu, offset, buf, len);
 }
 
-static inline int mmc_block_read(struct dfu_entity *dfu, void *buf, long *len)
+static inline int mmc_block_read(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
 {
-	return mmc_block_op(DFU_OP_READ, dfu, buf, len);
+	return mmc_block_op(DFU_OP_READ, dfu, offset, buf, len);
 }
 
 static int mmc_file_op(enum dfu_mmc_op op, struct dfu_entity *dfu,
-			void *buf, long *len)
+			u64 offset, void *buf, long *len)
 {
 	char cmd_buf[DFU_CMD_BUF_SIZE];
 	char *str_env;
@@ -66,12 +80,17 @@ static int mmc_file_op(enum dfu_mmc_op op, struct dfu_entity *dfu,
 
 	switch (dfu->layout) {
 	case DFU_FS_FAT:
-		sprintf(cmd_buf, "fat%s mmc %d:%d 0x%x %s %lx",
+		sprintf(cmd_buf, "fat%s mmc %d:%d 0x%x %s %lx %llx",
 			op == DFU_OP_READ ? "load" : "write",
 			dfu->data.mmc.dev, dfu->data.mmc.part,
-			(unsigned int) buf, dfu->name, *len);
+			(unsigned int) buf, dfu->name, *len, offset);
 		break;
 	case DFU_FS_EXT4:
+		if (offset != 0) {
+			debug("%s: Offset value %llx != 0 not supported!\n",
+					__func__, offset);
+			return -1;
+		}
 		sprintf(cmd_buf, "ext4%s mmc %d:%d /%s 0x%x %ld",
 			op == DFU_OP_READ ? "load" : "write",
 			dfu->data.mmc.dev, dfu->data.mmc.part,
@@ -80,6 +99,7 @@ static int mmc_file_op(enum dfu_mmc_op op, struct dfu_entity *dfu,
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
 		       dfu_get_layout(dfu->layout));
+		return -1;
 	}
 
 	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
@@ -102,27 +122,30 @@ static int mmc_file_op(enum dfu_mmc_op op, struct dfu_entity *dfu,
 	return ret;
 }
 
-static inline int mmc_file_write(struct dfu_entity *dfu, void *buf, long *len)
+static inline int mmc_file_write(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
 {
-	return mmc_file_op(DFU_OP_WRITE, dfu, buf, len);
+	return mmc_file_op(DFU_OP_WRITE, dfu, offset, buf, len);
 }
 
-static inline int mmc_file_read(struct dfu_entity *dfu, void *buf, long *len)
+static inline int mmc_file_read(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
 {
-	return mmc_file_op(DFU_OP_READ, dfu, buf, len);
+	return mmc_file_op(DFU_OP_READ, dfu, offset, buf, len);
 }
 
-int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+int dfu_write_medium_mmc(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
 {
 	int ret = -1;
 
 	switch (dfu->layout) {
 	case DFU_RAW_ADDR:
-		ret = mmc_block_write(dfu, buf, len);
+		ret = mmc_block_write(dfu, offset, buf, len);
 		break;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
-		ret = mmc_file_write(dfu, buf, len);
+		ret = mmc_file_write(dfu, offset, buf, len);
 		break;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
@@ -132,17 +155,17 @@ int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
 	return ret;
 }
 
-int dfu_read_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void *buf, long *len)
 {
 	int ret = -1;
 
 	switch (dfu->layout) {
 	case DFU_RAW_ADDR:
-		ret = mmc_block_read(dfu, buf, len);
+		ret = mmc_block_read(dfu, offset, buf, len);
 		break;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
-		ret = mmc_file_read(dfu, buf, len);
+		ret = mmc_file_read(dfu, offset, buf, len);
 		break;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
@@ -181,13 +204,15 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s)
 
 		mmc = find_mmc_device(dev);
 		if (mmc == NULL || mmc_init(mmc)) {
-			printf("%s: could not find mmc device #%d!\n", __func__, dev);
+			printf("%s: could not find mmc device #%d!\n",
+					__func__, dev);
 			return -ENODEV;
 		}
 
 		blk_dev = &mmc->block_dev;
 		if (get_partition_info(blk_dev, part, &partinfo) != 0) {
-			printf("%s: could not find partition #%d on mmc device #%d!\n",
+			printf("%s: could not find partition #%d "
+					"on mmc device #%d!\n",
 					__func__, part, dev);
 			return -ENODEV;
 		}
diff --git a/include/dfu.h b/include/dfu.h
index 5350d79..b662488 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -59,7 +59,7 @@ static inline unsigned int get_mmc_blk_size(int dev)
 
 #define DFU_NAME_SIZE 32
 #define DFU_CMD_BUF_SIZE 128
-#define DFU_DATA_BUF_SIZE (1024*1024*4) /* 4 MiB */
+#define DFU_DATA_BUF_SIZE (1024*64) /* 64 KB (the huge buffer is overkill) */
 
 struct dfu_entity {
 	char			name[DFU_NAME_SIZE];
@@ -73,10 +73,23 @@ struct dfu_entity {
 		struct mmc_internal_data mmc;
 	} data;
 
-	int (*read_medium)(struct dfu_entity *dfu, void *buf, long *len);
-	int (*write_medium)(struct dfu_entity *dfu, void *buf, long *len);
+	int (*read_medium)(struct dfu_entity *dfu,
+			u64 offset, void *buf, long *len);
+
+	int (*write_medium)(struct dfu_entity *dfu,
+			u64 offset, void *buf, long *len);
 
 	struct list_head list;
+
+	/* on the fly state */
+	u32 crc;
+	u64 offset;
+	int i_blk_seq_num;
+	u8 *i_buf;
+	u8 *i_buf_start;
+	u8 *i_buf_end;
+	long r_left;
+	long b_left;
 };
 
 int dfu_config_entities(char *s, char *interface, int num);
-- 
1.7.12

[U-Boot] [PATCH 00/10] USB: Gadget & DFU related fixes

[Pantelis Antoniou]

Hi Marek,

On Nov 29, 2012, at 8:32 AM, Marek Vasut wrote:

> Dear Pantelis Antoniou,
> 
>> Various bugfixes, g_dnl & DFU updates.
>> 
>> Pantelis Antoniou (10):
>>  usb: Remove obsolete header file
>>  usb: Fix bug when both DFU & ETHER are defined
>>  g_dnl: Issue connect/disconnect as appropriate
>>  g_dnl: Properly terminate string list.
>>  dfu: Only perform DFU board_usb_init() for TRATS
>>  dfu: Fix crash when wrong number of arguments given
>>  dfu: Send correct DFU response from composite_setup
>>  dfu: Properly zero out timeout value
>>  dfu: Add a partition type target
>>  dfu: Support larger than memory transfers.
> [...]
> 
> So this is a V2 patchset? Anyway, can you please rebase on top of u-boot-
> usb/master and repost? I rebased it on top of u-boot/master and pushed.
> 

Yeah, it's V2. I've rebased, and the patch series applies without any 
conflicts or fudges.

> Best regards,
> Marek Vasut

Regards

-- Pantelis

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Pantelis Antoniou]

Hi Lukasz,

I did found out the problem with large transfers, and there is a patch that
fixes it.

I'm pretty sure that the transfers are correct. Perhaps some other problem
with your board?

Can you use the updated patch series and try with that, with debugging
enabled?

Regards

-- Pantelis
 

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Lukasz Majewski]

Hi Pantelis,

> Hi Lukasz,
> 
> On Nov 28, 2012, at 7:46 PM, Lukasz Majewski wrote:
> 
> > Hi Pantelis,
> > 
> >> Hi Lukasz,
> >> 
> >> On Nov 28, 2012, at 6:01 PM, Lukasz Majewski wrote:
> >> 
> >>> Hi Tom,
> >>> 
> >>>> Hi Pantelis,
> >>>> 
> >>>>> usbdescriptors.h conflicts with linux/usb/ch9.h
> >>>>> Remove it.
> >>>> 
> >>> After rebasing on u-boot-usb/next below comment apply:
> >>> 
> >>> After applying this patch, I cannot build trats target anymore.
> >>> 
> >>> 
> >>> With u-boot-usb/master I can compile the u-boot for trats board
> >>> with no warnings and errors.
> >>> 
> >>> Unfortunately after flashing with dfu, the u-boot is _NOT_ working
> >>> properly anymore.
> >>> It seems, that some parts of the binary weren't correct.
> >>> 
> >> 
> >> Are you writing to a file in a filesystem? I.e. FAT?
> >> 
> >> I'm in the middle of doing more extensive tests, but file write to
> >> an FS might have problems. I am using the raw mmc interface.
> >> 
> > 
> > I've written u-boot to RAW eMMC (based on the LBA addressing).
> > Moreover I was also able to write data to FAT and EXT4 fs (which
> > uses standard fat/ext4 load commands).
> > 
> 
> I see. Note that ext4 writes won't work; there is no offset to the
> write file command. You will have to have use a large enough buffer
> to handle the file's worst case.

The 4 MiB buffer size was my wrong simplification. It shall be done as
you proposed (with transfered data chopped to smaller chunks).

The ext4write command will write data from DRAM buffer (e.g.
0x44000000) to partition formatted with ext4 fs. (one just needs to
remember that file size is given with hex number).
I've posted some fixes for ext4 recently to ML.

> 
> >> There could be something there that it's missed. I'm in the middle
> >> of doing more extensive tests.
> >> 
> >> 
> >>>> It writes u-boot.bin, but in a way that the board is bricked
> >>>> after flashing.
> >>> 
> >>> I need some time to perform the thorough review of core DFU
> >>> patches (patches 7/10, 09/10, 10/10).
> >>> 
> >> 
> >> OK.
> >> 
> >>> 
> >>> 
> >>>> BTW: 
> >>>> 1. What is your target device? What is the output of dfu mmc 0
> >>>> list command on your device?
> >>>> 
> >> 
> >> I'm on am335x_evm target, actual board is a beaglebone.
> >> 
> >> 
> >>>> On trats it is:
> >>>> DFU alt settings list:
> >>>> dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
> >>>> dev: eMMC alt: 1 name: uImage layout: FAT
> >>>> 
> >> 
> >> # setenv dfu_alt_info 'test part 0 3'
> >> # mmc part
> >> U-Boot# mmc part
> >> 
> >> Partition Map for MMC device 0  --   Partition Type: DOS
> >> 
> >> Part    Start Sector    Num Sectors     UUID            Type
> >>  1     63              112392          7a348599-01     0c Boot
> >>  2     112455          7501331         7a348599-02     83
> >>  3     7613786         12966           7a348599-03     83
> >> # dfu mmc 0 list
> >> DFU alt settings list:
> >> dev: eMMC alt: 0 name: test layout: RAW_ADDR
> >> 
> > Off topic: 
> > It would be nice to have all partitions listed with dfu mmc 0
> > list :-) and then have access to it via dfu-util tool (as a separate
> > alt settings). But this is a task for the future :-).
> > 
> > 
> >> Are you downloading u-boot.bin to the raw nand?
> >> I.e. there's no boot partition?
> > 
> > Yes, there isn't any partition for u-boot. I write to raw eMMC
> > address.
> > 
> >> 
> >> All my tests have been downloading a small ext3 image to the mmc.
> >> I'm in the middle of doing more extensive tests but those takes a
> >> huge amount of time... :(
> > 
> > This is because of very low DFU transmission speed (It uses only EP0
> > with EPS=64B , so it is meant to transfer really small files).
> > 
> > Updating rootfs via DFU would take much time. It is OK, to transfer
> > u-boot, uImage, some log data.
> > 
> 
> We have customers that do want to use DFU for all of their initial
> programming.

Ok, I see.

> 
> The limitations are known, but the fact is that they find it very
> convenient for their use. And it's not like you're limited by the USB
> interface, most of the time is spent writing to the medium.

So in my opinion there would be nice to have some kind of caching of
received data before writing to memory.

Then we could write memory asynchronously, when we "collect" e.g. 1 MiB
data in a buffer (and in meantime collect next data).


> 
> BTW, I've just confirmed that larger transfers get corrupted... :(
> Working on a fix now...

Ok.

> 
> >> 
> >>>> 2. Please look into the TRATS board (especially the
> >>>> CONFIG_DFU_ALT constant) for reference.
> >>>> 
> >> 
> >> Already looked there.
> >> 
> >>>> 3. What is yours dfu-util version? (Mine is 0.1+svnexported)
> >>>> 
> >> 
> >> Compiled from source git://gitorious.org/dfu-util/dfu-util.git
> >> Current master.
> >> 
> >> Regards
> >> 
> >> -- Pantelis
> >> 
> >> 
> >>>>> 
> >>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>>> ---
> >>>>> drivers/usb/gadget/f_dfu.c | 1 -
> >>>>> include/g_dnl.h            | 1 -
> >>>>> 2 files changed, 2 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/usb/gadget/f_dfu.c
> >>>>> b/drivers/usb/gadget/f_dfu.c index 3ec4c65..10547e3 100644
> >>>>> --- a/drivers/usb/gadget/f_dfu.c
> >>>>> +++ b/drivers/usb/gadget/f_dfu.c
> >>>>> @@ -25,7 +25,6 @@
> >>>>> #include <malloc.h>
> >>>>> 
> >>>>> #include <linux/usb/ch9.h>
> >>>>> -#include <usbdescriptors.h>
> >>>>> #include <linux/usb/gadget.h>
> >>>>> #include <linux/usb/composite.h>
> >>>>> 
> >>>>> diff --git a/include/g_dnl.h b/include/g_dnl.h
> >>>>> index 0ec7440..f47395f 100644
> >>>>> --- a/include/g_dnl.h
> >>>>> +++ b/include/g_dnl.h
> >>>>> @@ -22,7 +22,6 @@
> >>>>> #define __G_DOWNLOAD_H_
> >>>>> 
> >>>>> #include <linux/usb/ch9.h>
> >>>>> -#include <usbdescriptors.h>
> >>>>> #include <linux/usb/gadget.h>
> >>>>> 
> >>>>> int g_dnl_register(const char *s);
> >>>> 
> >>>> 
> >>>> 
> >>> 
> >>> 
> >>> 
> >>> -- 
> >>> Best regards,
> >>> 
> >>> Lukasz Majewski
> >>> 
> >>> Samsung Poland R&D Center | Linux Platform Group
> >> 
> > 
> > 
> > 
> > -- 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > Samsung Poland R&D Center | Linux Platform Group
> 



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 01/10] usb: Remove obsolete header file

[Pantelis Antoniou]

Hi Lukasz,

On Nov 29, 2012, at 10:13 AM, Lukasz Majewski wrote:

> Hi Pantelis,
> 
>> Hi Lukasz,
>> 
>> On Nov 28, 2012, at 7:46 PM, Lukasz Majewski wrote:
>> 
>>> Hi Pantelis,
>>> 
>>>> Hi Lukasz,
>>>> 
>>>> On Nov 28, 2012, at 6:01 PM, Lukasz Majewski wrote:
>>>> 
>>>>> Hi Tom,
>>>>> 
>>>>>> Hi Pantelis,
>>>>>> 
>>>>>>> usbdescriptors.h conflicts with linux/usb/ch9.h
>>>>>>> Remove it.
>>>>>> 
>>>>> After rebasing on u-boot-usb/next below comment apply:
>>>>> 
>>>>> After applying this patch, I cannot build trats target anymore.
>>>>> 
>>>>> 
>>>>> With u-boot-usb/master I can compile the u-boot for trats board
>>>>> with no warnings and errors.
>>>>> 
>>>>> Unfortunately after flashing with dfu, the u-boot is _NOT_ working
>>>>> properly anymore.
>>>>> It seems, that some parts of the binary weren't correct.
>>>>> 
>>>> 
>>>> Are you writing to a file in a filesystem? I.e. FAT?
>>>> 
>>>> I'm in the middle of doing more extensive tests, but file write to
>>>> an FS might have problems. I am using the raw mmc interface.
>>>> 
>>> 
>>> I've written u-boot to RAW eMMC (based on the LBA addressing).
>>> Moreover I was also able to write data to FAT and EXT4 fs (which
>>> uses standard fat/ext4 load commands).
>>> 
>> 
>> I see. Note that ext4 writes won't work; there is no offset to the
>> write file command. You will have to have use a large enough buffer
>> to handle the file's worst case.
> 
> The 4 MiB buffer size was my wrong simplification. It shall be done as
> you proposed (with transfered data chopped to smaller chunks).
> 
> The ext4write command will write data from DRAM buffer (e.g.
> 0x44000000) to partition formatted with ext4 fs. (one just needs to
> remember that file size is given with hex number).
> I've posted some fixes for ext4 recently to ML.
> 

Nice. There is a big fat warning when you try to write an ext4 file at
an offset other than 0; it should be easy to fix.

>> 
>>>> There could be something there that it's missed. I'm in the middle
>>>> of doing more extensive tests.
>>>> 
>>>> 
>>>>>> It writes u-boot.bin, but in a way that the board is bricked
>>>>>> after flashing.
>>>>> 
>>>>> I need some time to perform the thorough review of core DFU
>>>>> patches (patches 7/10, 09/10, 10/10).
>>>>> 
>>>> 
>>>> OK.
>>>> 
>>>>> 
>>>>> 
>>>>>> BTW: 
>>>>>> 1. What is your target device? What is the output of dfu mmc 0
>>>>>> list command on your device?
>>>>>> 
>>>> 
>>>> I'm on am335x_evm target, actual board is a beaglebone.
>>>> 
>>>> 
>>>>>> On trats it is:
>>>>>> DFU alt settings list:
>>>>>> dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
>>>>>> dev: eMMC alt: 1 name: uImage layout: FAT
>>>>>> 
>>>> 
>>>> # setenv dfu_alt_info 'test part 0 3'
>>>> # mmc part
>>>> U-Boot# mmc part
>>>> 
>>>> Partition Map for MMC device 0  --   Partition Type: DOS
>>>> 
>>>> Part    Start Sector    Num Sectors     UUID            Type
>>>> 1     63              112392          7a348599-01     0c Boot
>>>> 2     112455          7501331         7a348599-02     83
>>>> 3     7613786         12966           7a348599-03     83
>>>> # dfu mmc 0 list
>>>> DFU alt settings list:
>>>> dev: eMMC alt: 0 name: test layout: RAW_ADDR
>>>> 
>>> Off topic: 
>>> It would be nice to have all partitions listed with dfu mmc 0
>>> list :-) and then have access to it via dfu-util tool (as a separate
>>> alt settings). But this is a task for the future :-).
>>> 
>>> 
>>>> Are you downloading u-boot.bin to the raw nand?
>>>> I.e. there's no boot partition?
>>> 
>>> Yes, there isn't any partition for u-boot. I write to raw eMMC
>>> address.
>>> 
>>>> 
>>>> All my tests have been downloading a small ext3 image to the mmc.
>>>> I'm in the middle of doing more extensive tests but those takes a
>>>> huge amount of time... :(
>>> 
>>> This is because of very low DFU transmission speed (It uses only EP0
>>> with EPS=64B , so it is meant to transfer really small files).
>>> 
>>> Updating rootfs via DFU would take much time. It is OK, to transfer
>>> u-boot, uImage, some log data.
>>> 
>> 
>> We have customers that do want to use DFU for all of their initial
>> programming.
> 
> Ok, I see.
> 
>> 
>> The limitations are known, but the fact is that they find it very
>> convenient for their use. And it's not like you're limited by the USB
>> interface, most of the time is spent writing to the medium.
> 
> So in my opinion there would be nice to have some kind of caching of
> received data before writing to memory.
> 
> Then we could write memory asynchronously, when we "collect" e.g. 1 MiB
> data in a buffer (and in meantime collect next data).
> 

I doubt that would work in a u-boot environment. There are no threads
and I'm pretty sure most mass memory interfaces are PIO.

I'm completely up to date on latest u-boot happenings, maybe this has changed.

Regards

-- Pantelis

> 
>> 
>> BTW, I've just confirmed that larger transfers get corrupted... :(
>> Working on a fix now...
> 
> Ok.
> 
>> 
>>>> 
>>>>>> 2. Please look into the TRATS board (especially the
>>>>>> CONFIG_DFU_ALT constant) for reference.
>>>>>> 
>>>> 
>>>> Already looked there.
>>>> 
>>>>>> 3. What is yours dfu-util version? (Mine is 0.1+svnexported)
>>>>>> 
>>>> 
>>>> Compiled from source git://gitorious.org/dfu-util/dfu-util.git
>>>> Current master.
>>>> 
>>>> Regards
>>>> 
>>>> -- Pantelis
>>>> 
>>>> 
>>>>>>> 
>>>>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>>>> ---
>>>>>>> drivers/usb/gadget/f_dfu.c | 1 -
>>>>>>> include/g_dnl.h            | 1 -
>>>>>>> 2 files changed, 2 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/drivers/usb/gadget/f_dfu.c
>>>>>>> b/drivers/usb/gadget/f_dfu.c index 3ec4c65..10547e3 100644
>>>>>>> --- a/drivers/usb/gadget/f_dfu.c
>>>>>>> +++ b/drivers/usb/gadget/f_dfu.c
>>>>>>> @@ -25,7 +25,6 @@
>>>>>>> #include <malloc.h>
>>>>>>> 
>>>>>>> #include <linux/usb/ch9.h>
>>>>>>> -#include <usbdescriptors.h>
>>>>>>> #include <linux/usb/gadget.h>
>>>>>>> #include <linux/usb/composite.h>
>>>>>>> 
>>>>>>> diff --git a/include/g_dnl.h b/include/g_dnl.h
>>>>>>> index 0ec7440..f47395f 100644
>>>>>>> --- a/include/g_dnl.h
>>>>>>> +++ b/include/g_dnl.h
>>>>>>> @@ -22,7 +22,6 @@
>>>>>>> #define __G_DOWNLOAD_H_
>>>>>>> 
>>>>>>> #include <linux/usb/ch9.h>
>>>>>>> -#include <usbdescriptors.h>
>>>>>>> #include <linux/usb/gadget.h>
>>>>>>> 
>>>>>>> int g_dnl_register(const char *s);
>>>>>> 
>>>>>> 
>>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> -- 
>>>>> Best regards,
>>>>> 
>>>>> Lukasz Majewski
>>>>> 
>>>>> Samsung Poland R&D Center | Linux Platform Group
>>>> 
>>> 
>>> 
>>> 
>>> -- 
>>> Best regards,
>>> 
>>> Lukasz Majewski
>>> 
>>> Samsung Poland R&D Center | Linux Platform Group
>> 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 02/10] usb: Fix bug when both DFU & ETHER are defined

[Marek Vasut]

Dear Pantelis Antoniou,

> When both CONFIG_USB_GADGET & CONFIG_USB_ETHER are defined
> the makefile links objects twice.
> 
> Detect this and fix it with a not very elegant way in the
> makefile. Revisit and clean it later.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
[...]

This really needs some proper thinking and fixing.

Besides ... can you please annotate the patches with proper V2: and a proper 
changelog, see [1]?

[1] http://www.denx.de/wiki/U-Boot/Patches

Best regards,
Marek Vasut

[U-Boot] [PATCH 04/10] g_dnl: Properly terminate string list.

[Marek Vasut]

Dear Pantelis Antoniou,

> Well, not terminating the list causes very interesting crashes.
> As in changing the vendor & product ID crashes. Fun.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/usb/gadget/g_dnl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index 25da733..a5a4c1f 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -69,6 +69,7 @@ static struct usb_device_descriptor device_desc = {
>  static struct usb_string g_dnl_string_defs[] = {
>  	{ 0, manufacturer, },
>  	{ 1, product, },
> +	{  }		/* end of list */

So you're adding an uninited entry here? How'll this work?

>  };
> 
>  static struct usb_gadget_strings g_dnl_string_tab = {

Best regards,
Marek Vasut

[U-Boot] [PATCH 00/10] USB: Gadget & DFU related fixes

[Marek Vasut]

Dear Pantelis Antoniou,

> Various bugfixes, g_dnl & DFU updates.
> 
> Pantelis Antoniou (10):
>   usb: Remove obsolete header file
>   usb: Fix bug when both DFU & ETHER are defined
>   g_dnl: Issue connect/disconnect as appropriate
>   g_dnl: Properly terminate string list.
>   dfu: Only perform DFU board_usb_init() for TRATS
>   dfu: Fix crash when wrong number of arguments given
>   dfu: Send correct DFU response from composite_setup
>   dfu: Properly zero out timeout value
>   dfu: Add a partition type target
>   dfu: Support larger than memory transfers.
[...]

Please address the few issues I pointed out and send a proper V2. Thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 04/10] g_dnl: Properly terminate string list.

[Pantelis Antoniou]

Hi Marek,

On Nov 29, 2012, at 10:20 AM, Marek Vasut wrote:

> Dear Pantelis Antoniou,
> 
>> Well, not terminating the list causes very interesting crashes.
>> As in changing the vendor & product ID crashes. Fun.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>> drivers/usb/gadget/g_dnl.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
>> index 25da733..a5a4c1f 100644
>> --- a/drivers/usb/gadget/g_dnl.c
>> +++ b/drivers/usb/gadget/g_dnl.c
>> @@ -69,6 +69,7 @@ static struct usb_device_descriptor device_desc = {
>> static struct usb_string[] = {
>> 	{ 0, manufacturer, },
>> 	{ 1, product, },
>> +	{  }		/* end of list */
> 
> So you're adding an uninited entry here? How'll this work?
> 

This is very common idiom. It's not uninitialized, all the members are
set to 0, including the char * members.

It is exactly the same method used in drivers/usb/gadget/ether.c

>> };
>> 
>> static struct usb_gadget_strings g_dnl_string_tab = {
> 
> Best regards,
> Marek Vasut


Regards

-- Pantelis

[U-Boot] [PATCH 04/10] g_dnl: Properly terminate string list.

[Marek Vasut]

Dear Pantelis Antoniou,

> Hi Marek,
> 
> On Nov 29, 2012, at 10:20 AM, Marek Vasut wrote:
> > Dear Pantelis Antoniou,
> > 
> >> Well, not terminating the list causes very interesting crashes.
> >> As in changing the vendor & product ID crashes. Fun.
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >> ---
> >> drivers/usb/gadget/g_dnl.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> >> index 25da733..a5a4c1f 100644
> >> --- a/drivers/usb/gadget/g_dnl.c
> >> +++ b/drivers/usb/gadget/g_dnl.c
> >> @@ -69,6 +69,7 @@ static struct usb_device_descriptor device_desc = {
> >> static struct usb_string[] = {
> >> 
> >> 	{ 0, manufacturer, },
> >> 	{ 1, product, },
> >> 
> >> +	{  }		/* end of list */
> > 
> > So you're adding an uninited entry here? How'll this work?
> 
> This is very common idiom. It's not uninitialized, all the members are
> set to 0, including the char * members.
> 
> It is exactly the same method used in drivers/usb/gadget/ether.c

Blah, it's a matter of taste then (I prefer it explicit, but I don't really 
care).

Thanks for clearing this up.

Best regards,
Marek Vasut

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/28/12 11:08, Lukasz Majewski wrote:
> Hi Tom,
> 
> On 11/28/12 09:47, Lukasz Majewski wrote:
>>>> Hi Pantelis,
>>>> 
>>>>> USB initialization shouldn't happen for all the boards.
>>>>> 
>>>> 
>>>> The board_usb_init() follows u-boot policy, that SoC IPs
>>>> (USB) are enabled and configured just before their usage.
>>>> 
>>>> 
>>>>> Signed-off-by: Pantelis Antoniou
>>>>> <panto@antoniou-consulting.com> --- common/cmd_dfu.c | 3
>>>>> +++ 1 file changed, 3 insertions(+)
>>>>> 
>>>>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 
>>>>> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ 
>>>>> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int
>>>>> do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>>>>> argv[]) goto done; }
>>>>> 
>>>>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif +
>>>> In mine opinion this #ifdef shall be removed and each target
>>>> board using the DFU shall define board_usb_init() at board
>>>> file.
>>>> 
>> 
>> But this isn't a called-only-once place.  What are you really
>> doing here and are you sure it's needed every time DFU is
>> called?
>> 
> 
> Hmm, you are correct here.
> 
> But I don't have a good alternative for this.
> 
> One solution would be to define a static flag for it at do_dfu
> function to indicate if this was executed once (however I'm
> reluctant do this).
> 
> 
> Any ideas?

I think the answer, and it's what we do on am335x is that
arch_misc_init() is what calls the equiv of s3c_udc_probe(...) under
the logic of "if we are built with usb gadget support, we want to use
it, so init it".

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQt56WAAoJENk4IS6UOR1WznYP/23g0QuMB2slIC41OLTeGKfh
11zybSEVZYZmSPfgjEsXqEWh1cYryQNyiNyKIzNfPPyH/ZAA2PuMH7mKMmdp5St6
p7IIhmFwO+phkLGgpLVSJ6PsCGfY68N1r1FU04JJhpteoNmSPtutBWrb2bJ8tib/
5HHSjUEUSYIgE1OHHVouGUx4KzNwWgyr0nds9WyfJ/X9OnQ22WRuVlkOIpy74NCz
r9QSIEOSbmqY6uU+YFFOorgp0Ox97okRJAH0KAsBNxq6PE2NmZard0Qg2m2Ism7L
NFbBvlfeF+/m9cicnrnuygyVkkNRcsX5NjWzVilzXQCfYmwBSH2YKPZbpRb3XGmr
wNSNqbfSEWG3Oxa+g0NnqI8SPqsTNVXR8X1QsF/f7zIOHlYZfXlbqsDEzITm1YoI
S1OEmpYXQQI1kZEOaxfXyJYbMXnA1/y8uItX8Bl/JUMWDQqQMFeJMVS711khGYuR
EUVL8YQam6N7Xgzk89sN8UPyOfAbxxOgB5fNyKeuSL+sz0vBaAkmv69gNsdPsfIr
vFvfyUKwyMtqhWZO+cG0VU4jzI0S0SMHdh52GtrU6P/3r77MC6zrhVja2EylXqvD
p8pSi7eEdeBUMbJ6uMgLd0kxYwh3NWy5NTTR10yKDyTXi8kh/grG89syI5Eiczwj
/CW6UuwG8R7T2l2+d1X3
=mdL4
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Lukasz Majewski]

Hi Tom,

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/28/12 11:08, Lukasz Majewski wrote:
> > Hi Tom,
> > 
> > On 11/28/12 09:47, Lukasz Majewski wrote:
> >>>> Hi Pantelis,
> >>>> 
> >>>>> USB initialization shouldn't happen for all the boards.
> >>>>> 
> >>>> 
> >>>> The board_usb_init() follows u-boot policy, that SoC IPs
> >>>> (USB) are enabled and configured just before their usage.
> >>>> 
> >>>> 
> >>>>> Signed-off-by: Pantelis Antoniou
> >>>>> <panto@antoniou-consulting.com> --- common/cmd_dfu.c | 3
> >>>>> +++ 1 file changed, 3 insertions(+)
> >>>>> 
> >>>>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 
> >>>>> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ 
> >>>>> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int
> >>>>> do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> >>>>> argv[]) goto done; }
> >>>>> 
> >>>>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif +
> >>>> In mine opinion this #ifdef shall be removed and each target
> >>>> board using the DFU shall define board_usb_init() at board
> >>>> file.
> >>>> 
> >> 
> >> But this isn't a called-only-once place.  What are you really
> >> doing here and are you sure it's needed every time DFU is
> >> called?
> >> 
> > 
> > Hmm, you are correct here.
> > 
> > But I don't have a good alternative for this.
> > 
> > One solution would be to define a static flag for it at do_dfu
> > function to indicate if this was executed once (however I'm
> > reluctant do this).
> > 
> > 
> > Any ideas?
> 
> I think the answer, and it's what we do on am335x is that
> arch_misc_init() is what calls the equiv of s3c_udc_probe(...) under
> the logic of "if we are built with usb gadget support, we want to use
> it, so init it".

I've understood the policy differently:

"We are build with gadget support and we _might_ use it, so enable low
level code only when (or just before) we use it". 

What's about the power consumption? Why IP block which will
be used from time to time shall be enabled and operational?

> 
> - -- 
> Tom
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJQt56WAAoJENk4IS6UOR1WznYP/23g0QuMB2slIC41OLTeGKfh
> 11zybSEVZYZmSPfgjEsXqEWh1cYryQNyiNyKIzNfPPyH/ZAA2PuMH7mKMmdp5St6
> p7IIhmFwO+phkLGgpLVSJ6PsCGfY68N1r1FU04JJhpteoNmSPtutBWrb2bJ8tib/
> 5HHSjUEUSYIgE1OHHVouGUx4KzNwWgyr0nds9WyfJ/X9OnQ22WRuVlkOIpy74NCz
> r9QSIEOSbmqY6uU+YFFOorgp0Ox97okRJAH0KAsBNxq6PE2NmZard0Qg2m2Ism7L
> NFbBvlfeF+/m9cicnrnuygyVkkNRcsX5NjWzVilzXQCfYmwBSH2YKPZbpRb3XGmr
> wNSNqbfSEWG3Oxa+g0NnqI8SPqsTNVXR8X1QsF/f7zIOHlYZfXlbqsDEzITm1YoI
> S1OEmpYXQQI1kZEOaxfXyJYbMXnA1/y8uItX8Bl/JUMWDQqQMFeJMVS711khGYuR
> EUVL8YQam6N7Xgzk89sN8UPyOfAbxxOgB5fNyKeuSL+sz0vBaAkmv69gNsdPsfIr
> vFvfyUKwyMtqhWZO+cG0VU4jzI0S0SMHdh52GtrU6P/3r77MC6zrhVja2EylXqvD
> p8pSi7eEdeBUMbJ6uMgLd0kxYwh3NWy5NTTR10yKDyTXi8kh/grG89syI5Eiczwj
> /CW6UuwG8R7T2l2+d1X3
> =mdL4
> -----END PGP SIGNATURE-----
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/29/12 18:14, Lukasz Majewski wrote:
> Hi Tom,
> 
> On 11/28/12 11:08, Lukasz Majewski wrote:
>>>> Hi Tom,
>>>> 
>>>> On 11/28/12 09:47, Lukasz Majewski wrote:
>>>>>>> Hi Pantelis,
>>>>>>> 
>>>>>>>> USB initialization shouldn't happen for all the 
>>>>>>>> boards.
>>>>>>>> 
>>>>>>> 
>>>>>>> The board_usb_init() follows u-boot policy, that SoC 
>>>>>>> IPs (USB) are enabled and configured just before their
>>>>>>>  usage.
>>>>>>> 
>>>>>>> 
>>>>>>>> Signed-off-by: Pantelis Antoniou 
>>>>>>>> <panto@antoniou-consulting.com> --- common/cmd_dfu.c
>>>>>>>>  | 3 +++ 1 file changed, 3 insertions(+)
>>>>>>>> 
>>>>>>>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c 
>>>>>>>> index 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c
>>>>>>>>  +++ b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static
>>>>>>>> int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char
>>>>>>>> * const argv[]) goto done; }
>>>>>>>> 
>>>>>>>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif +
>>>>>>> In mine opinion this #ifdef shall be removed and each 
>>>>>>> target board using the DFU shall define 
>>>>>>> board_usb_init() at board file.
>>>>>>> 
>>>>> 
>>>>> But this isn't a called-only-once place.  What are you 
>>>>> really doing here and are you sure it's needed every time 
>>>>> DFU is called?
>>>>> 
>>>> 
>>>> Hmm, you are correct here.
>>>> 
>>>> But I don't have a good alternative for this.
>>>> 
>>>> One solution would be to define a static flag for it at 
>>>> do_dfu function to indicate if this was executed once 
>>>> (however I'm reluctant do this).
>>>> 
>>>> 
>>>> Any ideas?
> 
> I think the answer, and it's what we do on am335x is that 
> arch_misc_init() is what calls the equiv of s3c_udc_probe(...) 
> under the logic of "if we are built with usb gadget support, we 
> want to use it, so init it".
> 
>> I've understood the policy differently:
> 
>> "We are build with gadget support and we _might_ use it, so 
>> enable low level code only when (or just before) we use it".
> 
>> What's about the power consumption? Why IP block which will be 
>> used from time to time shall be enabled and operational?

Frankly, I think this shows we don't have a good setup right now, in
general.  Saying the gadget needs to whack and re-whack the device
into on state (but isn't really turning it off once done) isn't ideal.
 There are also indeed drawbacks to saying "gadget support enabled at
boot-time, let me set it up".  Right now, I don't really have a good
answer.  It also needs to cover things like gadget ethernet where we
don't really want to disappear immediately after each command.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQuBJzAAoJENk4IS6UOR1WWncP/29E2kP1ErctSmBPUkJZASEk
qmW+rYCwxpz3aesC6Dcnc0Urad+MwO9SPbzUFfQcB4CMVXXAXMcahH+4FY8FzTnV
p8QqQjZ0o54xYxEI/evkaEWsTyM3nBBXtMq9II2gwYPMxP88i2JwL1NMOF6o2z+Y
VVNyGO1685JIGU+WMzpoXp4WYjfd/8qv8JHhQ+moGoFe0p7p5c5kokYsbXdcxvyk
rkeJrog8SV+vJQLnu0nUh8Q/pBYftiMDD+U8upQOiYNF1CPgGugX33XJydtYMoA8
rv6an/xh4PXUSJkAJbwp8vrxQqrHD0MYWtZEPwTEToFmTxvvaS2zJ0uOnNLkjanv
dp8RrD38HIczsgMtAVXR9nhFsW89MB2EiXc7N/EoO0BA4E0pac1/+OUfUMwELj16
IKJIHa4BXkHfg/ybQXCxr+GDZEc8YwLGXxzyIw8cS3PffNSkR1fkJcjZwFg190y0
1IjmlF0Dhg0CGweej0WTFuzi3OmHQOvMJcvT0doiI35iVbFvGwhjNUt8dJeDr0Yr
xkhWPhR3+iMXmzle+me+ZUxUe5g2vGqPC6HeGUO/fo0ntqZfITjzznMHKpN8Uxwv
D6Njg2e785ZIgTxcMXKn/Q2xtPBF78/stWeStyIwIAxYOEDw/C+NipWAHEpJdGTp
k8mhP+GKWgatyZ5yqDBA
=01FA
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Tom Rini]

On Wed, Nov 28, 2012 at 03:47:43PM +0100, Lukasz Majewski wrote:

> Hi Pantelis,
> 
> > USB initialization shouldn't happen for all the boards.
> > 
> 
> The board_usb_init() follows u-boot policy, that SoC IPs (USB) are
> enabled and configured just before their usage.

Going back to this old thread as I dig at things again.  How much of
s3c_udc_probe is actually enabling hardware as opposed to registering
that such hardware exists and can be enabled when we call
usb_gadget_register_driver ?  It looks like all of the clocking (and pin
muxing?) is already being done.

-- 
Tom

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Lukasz Majewski]

Hi Tom,

> On Wed, Nov 28, 2012 at 03:47:43PM +0100, Lukasz Majewski wrote:
> 
> > Hi Pantelis,
> > 
> > > USB initialization shouldn't happen for all the boards.
> > > 
> > 
> > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are
> > enabled and configured just before their usage.
> 
> Going back to this old thread as I dig at things again.  How much of
> s3c_udc_probe is actually enabling hardware as opposed to registering
> that such hardware exists and can be enabled when we call
> usb_gadget_register_driver ?  It looks like all of the clocking (and
> pin muxing?) is already being done.
> 

I will check if s3c_udc_probe can be removed totally and replaced by
usb_gadget_register_driver (since some time has passed from original
UDC driver posting).

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 05/10] dfu: Only perform DFU board_usb_init() for TRATS

[Tom Rini]

On Mon, Dec 17, 2012 at 12:37 PM, Lukasz Majewski
<l.majewski@samsung.com> wrote:
> Hi Tom,
>
>> On Wed, Nov 28, 2012 at 03:47:43PM +0100, Lukasz Majewski wrote:
>>
>> > Hi Pantelis,
>> >
>> > > USB initialization shouldn't happen for all the boards.
>> > >
>> >
>> > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are
>> > enabled and configured just before their usage.
>>
>> Going back to this old thread as I dig at things again.  How much of
>> s3c_udc_probe is actually enabling hardware as opposed to registering
>> that such hardware exists and can be enabled when we call
>> usb_gadget_register_driver ?  It looks like all of the clocking (and
>> pin muxing?) is already being done.
>>
>
> I will check if s3c_udc_probe can be removed totally and replaced by
> usb_gadget_register_driver (since some time has passed from original
> UDC driver posting).

Well, not so much replace but just, if there's no actual HW init, move
the "probe" (which isn't a probe, it's registering board infos) call
into arch_misc_init or similar.

--
Tom