All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices
@ 2015-10-25  4:44 Siarhei Siamashka
  2015-10-25  4:44 ` [U-Boot] [PATCH 1/2] sunxi: Enable DFU for RAM Siarhei Siamashka
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-25  4:44 UTC (permalink / raw)
  To: u-boot

Hello,

DFU allows to transfer large files (such as initrd images) much
faster than FEL.

Siarhei Siamashka (2):
  sunxi: Enable DFU for RAM
  musb: sunxi: Implement dfu_usb_get_reset()

 drivers/usb/musb-new/sunxi.c   | 12 ++++++++++++
 include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 5 deletions(-)

-- 
2.4.10

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 1/2] sunxi: Enable DFU for RAM
  2015-10-25  4:44 [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Siarhei Siamashka
@ 2015-10-25  4:44 ` Siarhei Siamashka
  2015-11-20 15:21   ` [U-Boot] [U-Boot,1/2] " Hans de Goede
  2015-10-25  4:44 ` [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset() Siarhei Siamashka
  2015-10-26 11:18 ` [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Piotr Król
  2 siblings, 1 reply; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-25  4:44 UTC (permalink / raw)
  To: u-boot

The DFU protocol implementation in U-Boot is much faster than the
FEL protocol implementation in the boot ROM on Allwinner devices.
Using DFU instead of FEL improves the USB transfer speed from
500-900 KB/s to 3.2-3.7 MB/s. This is particularly useful for
reducing the time needed for booting systems with large initrd
images.

FEL is still useful for loading the U-Boot bootloader and a boot
script, which may then activate DFU in the following way:

   setenv dfu_alt_info ${dfu_alt_info_ram}
   dfu 0 ram 0
   bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}

The rest of the files can be transferred to the device using the
"dfu-util" tool.

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
---
 include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index ddcfe94..38c0bc5 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -337,6 +337,7 @@ extern int soft_i2c_gpio_scl;
 #define CONFIG_USB_GADGET_VBUS_DRAW	0
 
 #define CONFIG_USB_GADGET_DOWNLOAD
+#define CONFIG_USB_FUNCTION_DFU
 #define CONFIG_USB_FUNCTION_FASTBOOT
 #define CONFIG_USB_FUNCTION_MASS_STORAGE
 #endif
@@ -347,6 +348,11 @@ extern int soft_i2c_gpio_scl;
 #define CONFIG_G_DNL_MANUFACTURER	"Allwinner Technology"
 #endif
 
+#ifdef CONFIG_USB_FUNCTION_DFU
+#define CONFIG_CMD_DFU
+#define CONFIG_DFU_RAM
+#endif
+
 #ifdef CONFIG_USB_FUNCTION_FASTBOOT
 #define CONFIG_CMD_FASTBOOT
 #define CONFIG_FASTBOOT_BUF_ADDR	CONFIG_SYS_LOAD_ADDR
@@ -394,13 +400,26 @@ extern int soft_i2c_gpio_scl;
  * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
  * 1M script, 1M pxe and the ramdisk at the end.
  */
+
+#define KERNEL_ADDR_R  __stringify(SDRAM_OFFSET(2000000))
+#define FDT_ADDR_R     __stringify(SDRAM_OFFSET(3000000))
+#define SCRIPT_ADDR_R  __stringify(SDRAM_OFFSET(3100000))
+#define PXEFILE_ADDR_R __stringify(SDRAM_OFFSET(3200000))
+#define RAMDISK_ADDR_R __stringify(SDRAM_OFFSET(3300000))
+
 #define MEM_LAYOUT_ENV_SETTINGS \
 	"bootm_size=0xa000000\0" \
-	"kernel_addr_r=" __stringify(SDRAM_OFFSET(2000000)) "\0" \
-	"fdt_addr_r=" __stringify(SDRAM_OFFSET(3000000)) "\0" \
-	"scriptaddr=" __stringify(SDRAM_OFFSET(3100000)) "\0" \
-	"pxefile_addr_r=" __stringify(SDRAM_OFFSET(3200000)) "\0" \
-	"ramdisk_addr_r=" __stringify(SDRAM_OFFSET(3300000)) "\0"
+	"kernel_addr_r=" KERNEL_ADDR_R "\0" \
+	"fdt_addr_r=" FDT_ADDR_R "\0" \
+	"scriptaddr=" SCRIPT_ADDR_R "\0" \
+	"pxefile_addr_r=" PXEFILE_ADDR_R "\0" \
+	"ramdisk_addr_r=" RAMDISK_ADDR_R "\0"
+
+#define DFU_ALT_INFO_RAM \
+	"dfu_alt_info_ram=" \
+	"kernel ram " KERNEL_ADDR_R " 0x1000000;" \
+	"fdt ram " FDT_ADDR_R " 0x100000;" \
+	"ramdisk ram " RAMDISK_ADDR_R " 0x4000000\0"
 
 #ifdef CONFIG_MMC
 #define BOOT_TARGET_DEVICES_MMC(func) func(MMC, mmc, 0)
@@ -482,6 +501,7 @@ extern int soft_i2c_gpio_scl;
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	CONSOLE_ENV_SETTINGS \
 	MEM_LAYOUT_ENV_SETTINGS \
+	DFU_ALT_INFO_RAM \
 	"fdtfile=" CONFIG_DEFAULT_DEVICE_TREE ".dtb\0" \
 	"console=ttyS0,115200\0" \
 	BOOTCMD_SUNXI_COMPAT \
-- 
2.4.10

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25  4:44 [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Siarhei Siamashka
  2015-10-25  4:44 ` [U-Boot] [PATCH 1/2] sunxi: Enable DFU for RAM Siarhei Siamashka
@ 2015-10-25  4:44 ` Siarhei Siamashka
  2015-10-25 11:00   ` Marek Vasut
  2015-10-26 11:18 ` [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Piotr Król
  2 siblings, 1 reply; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-25  4:44 UTC (permalink / raw)
  To: u-boot

This is necessary to distinguish between the "dfu-util --detach" and
the "dfu-util --reset" requests.

The default weak implementation of dfu_usb_get_reset() unconditionally
reboots the device, but we want to be able to continue the boot.scr
execution after writing the kernel, fdt and ramdisk to RAM via DFU.

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
---
 drivers/usb/musb-new/sunxi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index a146c08..5eb8d19 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -166,6 +166,17 @@ static void USBC_ConfigFIFO_Base(void)
 }
 
 /******************************************************************************
+ * Needed for the DFU polling magic
+ ******************************************************************************/
+
+static u8 last_int_usb;
+
+bool dfu_usb_get_reset(void)
+{
+	return !!(last_int_usb & MUSB_INTR_RESET);
+}
+
+/******************************************************************************
  * MUSB Glue code
  ******************************************************************************/
 
@@ -176,6 +187,7 @@ static irqreturn_t sunxi_musb_interrupt(int irq, void *__hci)
 
 	/* read and flush interrupts */
 	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
+	last_int_usb = musb->int_usb;
 	if (musb->int_usb)
 		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
 	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
-- 
2.4.10

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25  4:44 ` [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset() Siarhei Siamashka
@ 2015-10-25 11:00   ` Marek Vasut
  2015-10-25 11:46     ` Albert ARIBAUD
  2015-10-25 14:55     ` Siarhei Siamashka
  0 siblings, 2 replies; 24+ messages in thread
From: Marek Vasut @ 2015-10-25 11:00 UTC (permalink / raw)
  To: u-boot

On Sunday, October 25, 2015 at 05:44:47 AM, Siarhei Siamashka wrote:
> This is necessary to distinguish between the "dfu-util --detach" and
> the "dfu-util --reset" requests.
> 
> The default weak implementation of dfu_usb_get_reset() unconditionally
> reboots the device, but we want to be able to continue the boot.scr
> execution after writing the kernel, fdt and ramdisk to RAM via DFU.
> 
> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index a146c08..5eb8d19 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -166,6 +166,17 @@ static void USBC_ConfigFIFO_Base(void)
>  }
> 
>  /*************************************************************************
> ***** + * Needed for the DFU polling magic
> +
> **************************************************************************
> ****/ +
> +static u8 last_int_usb;
> +
> +bool dfu_usb_get_reset(void)
> +{
> +	return !!(last_int_usb & MUSB_INTR_RESET);

The !! is not needed.

[...]

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 11:00   ` Marek Vasut
@ 2015-10-25 11:46     ` Albert ARIBAUD
  2015-10-25 12:40       ` Ian Campbell
  2015-10-25 14:55     ` Siarhei Siamashka
  1 sibling, 1 reply; 24+ messages in thread
From: Albert ARIBAUD @ 2015-10-25 11:46 UTC (permalink / raw)
  To: u-boot

Hello Marek,

On Sun, 25 Oct 2015 12:00:09 +0100, Marek Vasut <marex@denx.de> wrote:
> On Sunday, October 25, 2015 at 05:44:47 AM, Siarhei Siamashka wrote:
> > This is necessary to distinguish between the "dfu-util --detach" and
> > the "dfu-util --reset" requests.
> > 
> > The default weak implementation of dfu_usb_get_reset() unconditionally
> > reboots the device, but we want to be able to continue the boot.scr
> > execution after writing the kernel, fdt and ramdisk to RAM via DFU.
> > 
> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> > ---
> >  drivers/usb/musb-new/sunxi.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> > index a146c08..5eb8d19 100644
> > --- a/drivers/usb/musb-new/sunxi.c
> > +++ b/drivers/usb/musb-new/sunxi.c
> > @@ -166,6 +166,17 @@ static void USBC_ConfigFIFO_Base(void)
> >  }
> > 
> >  /*************************************************************************
> > ***** + * Needed for the DFU polling magic
> > +
> > **************************************************************************
> > ****/ +
> > +static u8 last_int_usb;
> > +
> > +bool dfu_usb_get_reset(void)
> > +{
> > +	return !!(last_int_usb & MUSB_INTR_RESET);
> 
> The !! is not needed.

Except if you want to be sure that you return 0 or 1 rather than 0 or
(1 << something).

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

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 11:46     ` Albert ARIBAUD
@ 2015-10-25 12:40       ` Ian Campbell
  2015-10-25 13:22         ` Albert ARIBAUD
  0 siblings, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2015-10-25 12:40 UTC (permalink / raw)
  To: u-boot

On Sun, 2015-10-25 at 12:46 +0100, Albert ARIBAUD wrote:
> > > +static u8 last_int_usb;
> > > +
> > > +bool dfu_usb_get_reset(void)
> > > +{
> > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > 
> > The !! is not needed.
> 
> Except if you want to be sure that you return 0 or 1 rather than 0 or
> (1 << something).

Doesn't the bool return type already cause that to happen? (from the
PoV of the caller@least)

Ian.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 12:40       ` Ian Campbell
@ 2015-10-25 13:22         ` Albert ARIBAUD
  2015-10-25 13:29           ` Marek Vasut
  2015-10-25 19:22           ` Ian Campbell
  0 siblings, 2 replies; 24+ messages in thread
From: Albert ARIBAUD @ 2015-10-25 13:22 UTC (permalink / raw)
  To: u-boot

Hello Ian,

On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
<ijc+uboot@hellion.org.uk> wrote:
> On Sun, 2015-10-25 at 12:46 +0100, Albert ARIBAUD wrote:
> > > > +static u8 last_int_usb;
> > > > +
> > > > +bool dfu_usb_get_reset(void)
> > > > +{
> > > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > > 
> > > The !! is not needed.
> > 
> > Except if you want to be sure that you return 0 or 1 rather than 0 or
> > (1 << something).
> 
> Doesn't the bool return type already cause that to happen? (from the
> PoV of the caller at least)

When all is said and done, a C bool is a C int, and anyway C does not
perform value conversion (except for size and possibly sign extension)
on type casts.

So no, types, bool or otherwise, do not cause any implicit '!!' to
happen.

What happens is, wherever C expects a boolean value ('if', 'while'...)
it considers 0 to be false and anything else to be true. But that's
independent of the value's alleged type.

> Ian.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 13:22         ` Albert ARIBAUD
@ 2015-10-25 13:29           ` Marek Vasut
  2015-10-25 14:46             ` Siarhei Siamashka
  2015-10-25 19:22           ` Ian Campbell
  1 sibling, 1 reply; 24+ messages in thread
From: Marek Vasut @ 2015-10-25 13:29 UTC (permalink / raw)
  To: u-boot

On Sunday, October 25, 2015 at 02:22:53 PM, Albert ARIBAUD wrote:
> Hello Ian,

Hi!

> On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> 
> <ijc+uboot@hellion.org.uk> wrote:
> > On Sun, 2015-10-25 at 12:46 +0100, Albert ARIBAUD wrote:
> > > > > +static u8 last_int_usb;
> > > > > +
> > > > > +bool dfu_usb_get_reset(void)
> > > > > +{
> > > > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > > > 
> > > > The !! is not needed.
> > > 
> > > Except if you want to be sure that you return 0 or 1 rather than 0 or
> > > (1 << something).
> > 
> > Doesn't the bool return type already cause that to happen? (from the
> > PoV of the caller at least)
> 
> When all is said and done, a C bool is a C int, and anyway C does not
> perform value conversion (except for size and possibly sign extension)
> on type casts.
> 
> So no, types, bool or otherwise, do not cause any implicit '!!' to
> happen.
> 
> What happens is, wherever C expects a boolean value ('if', 'while'...)
> it considers 0 to be false and anything else to be true. But that's
> independent of the value's alleged type.

Which is the case here -- one is not supposed to test boolean type for
any particular value.

Best grouik,
Marek Vasut

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 13:29           ` Marek Vasut
@ 2015-10-25 14:46             ` Siarhei Siamashka
  2015-10-25 16:08               ` Marek Vasut
  2015-10-25 19:24               ` Ian Campbell
  0 siblings, 2 replies; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-25 14:46 UTC (permalink / raw)
  To: u-boot

On Sun, 25 Oct 2015 14:29:59 +0100
Marek Vasut <marex@denx.de> wrote:

> On Sunday, October 25, 2015 at 02:22:53 PM, Albert ARIBAUD wrote:
> > Hello Ian,
> 
> Hi!
> 
> > On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> > 
> > <ijc+uboot@hellion.org.uk> wrote:
> > > On Sun, 2015-10-25 at 12:46 +0100, Albert ARIBAUD wrote:
> > > > > > +static u8 last_int_usb;
> > > > > > +
> > > > > > +bool dfu_usb_get_reset(void)
> > > > > > +{
> > > > > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > > > > 
> > > > > The !! is not needed.
> > > > 
> > > > Except if you want to be sure that you return 0 or 1 rather than 0 or
> > > > (1 << something).
> > > 
> > > Doesn't the bool return type already cause that to happen? (from the
> > > PoV of the caller at least)
> > 
> > When all is said and done, a C bool is a C int, and anyway C does not
> > perform value conversion (except for size and possibly sign extension)
> > on type casts.
> > 
> > So no, types, bool or otherwise, do not cause any implicit '!!' to
> > happen.
> > 
> > What happens is, wherever C expects a boolean value ('if', 'while'...)
> > it considers 0 to be false and anything else to be true. But that's
> > independent of the value's alleged type.
> 
> Which is the case here -- one is not supposed to test boolean type for
> any particular value.

Sure, this works fine as long as everyone has exactly the same idea
about how this is supposed to work. Please consider the following code:

    if (one_boolean_variable != another_boolean_variable) {
        /* Sanity check failed, features X and Y must be either
           both enabled or both disabled at the same time */
    }

The author of this hypothetical code may claim that a boolean
variable must be always 0 or 1. And both of you will have a long
and entertaining discussion as a result.

One more example:

    #include <stdbool.h>
    #include <stdio.h>

    bool foo(void)
    {
        return 123;
    }

    int main(void)
    {
        printf("%d\n", (int)foo());
        return 0;
    }

Guess what is printed after compiling and executing this code? Then
replace "#include <stdbool.h>" with "typedef int bool;" and try it
again. With the GCC compiler, the former prints "1" and the latter
prints "123".

This stuff is a potential source of non-obvious bugs. Using "!!" is
always safe, but may be in many cases redundant.

-- 
Best regards,
Siarhei Siamashka

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 11:00   ` Marek Vasut
  2015-10-25 11:46     ` Albert ARIBAUD
@ 2015-10-25 14:55     ` Siarhei Siamashka
  2015-10-25 16:09       ` Marek Vasut
  2015-10-25 21:19       ` Albert ARIBAUD
  1 sibling, 2 replies; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-25 14:55 UTC (permalink / raw)
  To: u-boot

On Sun, 25 Oct 2015 12:00:09 +0100
Marek Vasut <marex@denx.de> wrote:

> On Sunday, October 25, 2015 at 05:44:47 AM, Siarhei Siamashka wrote:
> > This is necessary to distinguish between the "dfu-util --detach" and
> > the "dfu-util --reset" requests.
> > 
> > The default weak implementation of dfu_usb_get_reset() unconditionally
> > reboots the device, but we want to be able to continue the boot.scr
> > execution after writing the kernel, fdt and ramdisk to RAM via DFU.
> > 
> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> > ---
> >  drivers/usb/musb-new/sunxi.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> > index a146c08..5eb8d19 100644
> > --- a/drivers/usb/musb-new/sunxi.c
> > +++ b/drivers/usb/musb-new/sunxi.c
> > @@ -166,6 +166,17 @@ static void USBC_ConfigFIFO_Base(void)
> >  }
> > 
> >  /*************************************************************************
> > ***** + * Needed for the DFU polling magic
> > +
> > **************************************************************************
> > ****/ +
> > +static u8 last_int_usb;
> > +
> > +bool dfu_usb_get_reset(void)
> > +{
> > +	return !!(last_int_usb & MUSB_INTR_RESET);
> 
> The !! is not needed.

Thanks for your comment, I can surely change this in the updated
version of the patch.

But I'm more interested to know if the USB people are happy with the
current wacky way of interaction between the DFU code and the USB
drivers. Doing dfu_usb_get_reset() polling up to 1000 times in a loop
does not look exactly beautiful:

    https://github.com/u-boot/u-boot/blob/v2015.10/common/cmd_dfu.c#L64

It might be also a bit racy (the RESET interrupt could be potentially
missed sometimes).

-- 
Best regards,
Siarhei Siamashka

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 14:46             ` Siarhei Siamashka
@ 2015-10-25 16:08               ` Marek Vasut
  2015-10-25 19:24               ` Ian Campbell
  1 sibling, 0 replies; 24+ messages in thread
From: Marek Vasut @ 2015-10-25 16:08 UTC (permalink / raw)
  To: u-boot

On Sunday, October 25, 2015 at 03:46:15 PM, Siarhei Siamashka wrote:
> On Sun, 25 Oct 2015 14:29:59 +0100
> 
> Marek Vasut <marex@denx.de> wrote:
> > On Sunday, October 25, 2015 at 02:22:53 PM, Albert ARIBAUD wrote:
> > > Hello Ian,
> > 
> > Hi!
> > 
> > > On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> > > 
> > > <ijc+uboot@hellion.org.uk> wrote:
> > > > On Sun, 2015-10-25 at 12:46 +0100, Albert ARIBAUD wrote:
> > > > > > > +static u8 last_int_usb;
> > > > > > > +
> > > > > > > +bool dfu_usb_get_reset(void)
> > > > > > > +{
> > > > > > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > > > > > 
> > > > > > The !! is not needed.
> > > > > 
> > > > > Except if you want to be sure that you return 0 or 1 rather than 0
> > > > > or (1 << something).
> > > > 
> > > > Doesn't the bool return type already cause that to happen? (from the
> > > > PoV of the caller at least)
> > > 
> > > When all is said and done, a C bool is a C int, and anyway C does not
> > > perform value conversion (except for size and possibly sign extension)
> > > on type casts.
> > > 
> > > So no, types, bool or otherwise, do not cause any implicit '!!' to
> > > happen.
> > > 
> > > What happens is, wherever C expects a boolean value ('if', 'while'...)
> > > it considers 0 to be false and anything else to be true. But that's
> > > independent of the value's alleged type.
> > 
> > Which is the case here -- one is not supposed to test boolean type for
> > any particular value.
> 
> Sure, this works fine as long as everyone has exactly the same idea
> about how this is supposed to work. Please consider the following code:
> 
>     if (one_boolean_variable != another_boolean_variable) {
>         /* Sanity check failed, features X and Y must be either
>            both enabled or both disabled at the same time */
>     }
> 
> The author of this hypothetical code may claim that a boolean
> variable must be always 0 or 1.

This assumption is wrong.

> And both of you will have a long
> and entertaining discussion as a result.
> 
> One more example:
> 
>     #include <stdbool.h>
>     #include <stdio.h>
> 
>     bool foo(void)
>     {
>         return 123;

This is bloody confusing.

>     }
> 
>     int main(void)
>     {
>         printf("%d\n", (int)foo());

This is wrong -- the cast is outright incorrect.

>         return 0;
>     }
> 
> Guess what is printed after compiling and executing this code? Then
> replace "#include <stdbool.h>" with "typedef int bool;" and try it
> again. With the GCC compiler, the former prints "1" and the latter
> prints "123".

The code is broken, so the result is undefined.

> This stuff is a potential source of non-obvious bugs. Using "!!" is
> always safe, but may be in many cases redundant.

I'd expect that using !! will generate additional code and that's done
for no reason at all.

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 14:55     ` Siarhei Siamashka
@ 2015-10-25 16:09       ` Marek Vasut
  2015-10-25 21:19       ` Albert ARIBAUD
  1 sibling, 0 replies; 24+ messages in thread
From: Marek Vasut @ 2015-10-25 16:09 UTC (permalink / raw)
  To: u-boot

On Sunday, October 25, 2015 at 03:55:43 PM, Siarhei Siamashka wrote:
> On Sun, 25 Oct 2015 12:00:09 +0100
> 
> Marek Vasut <marex@denx.de> wrote:
> > On Sunday, October 25, 2015 at 05:44:47 AM, Siarhei Siamashka wrote:
> > > This is necessary to distinguish between the "dfu-util --detach" and
> > > the "dfu-util --reset" requests.
> > > 
> > > The default weak implementation of dfu_usb_get_reset() unconditionally
> > > reboots the device, but we want to be able to continue the boot.scr
> > > execution after writing the kernel, fdt and ramdisk to RAM via DFU.
> > > 
> > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> > > ---
> > > 
> > >  drivers/usb/musb-new/sunxi.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/usb/musb-new/sunxi.c
> > > b/drivers/usb/musb-new/sunxi.c index a146c08..5eb8d19 100644
> > > --- a/drivers/usb/musb-new/sunxi.c
> > > +++ b/drivers/usb/musb-new/sunxi.c
> > > @@ -166,6 +166,17 @@ static void USBC_ConfigFIFO_Base(void)
> > > 
> > >  }
> > >  
> > >  /*********************************************************************
> > >  ****
> > > 
> > > ***** + * Needed for the DFU polling magic
> > > +
> > > ***********************************************************************
> > > *** ****/ +
> > > +static u8 last_int_usb;
> > > +
> > > +bool dfu_usb_get_reset(void)
> > > +{
> > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > 
> > The !! is not needed.
> 
> Thanks for your comment, I can surely change this in the updated
> version of the patch.
> 
> But I'm more interested to know if the USB people are happy with the
> current wacky way of interaction between the DFU code and the USB
> drivers. Doing dfu_usb_get_reset() polling up to 1000 times in a loop
> does not look exactly beautiful:
> 
>     https://github.com/u-boot/u-boot/blob/v2015.10/common/cmd_dfu.c#L64
> 
> It might be also a bit racy (the RESET interrupt could be potentially
> missed sometimes).

That's a question for Lukasz .

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 13:22         ` Albert ARIBAUD
  2015-10-25 13:29           ` Marek Vasut
@ 2015-10-25 19:22           ` Ian Campbell
  2015-10-25 21:16             ` Albert ARIBAUD
  1 sibling, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2015-10-25 19:22 UTC (permalink / raw)
  To: u-boot

On Sun, 2015-10-25 at 14:22 +0100, Albert ARIBAUD wrote:
> On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> > Doesn't the bool return type already cause that to happen? (from the
> > PoV of the caller at least)
> 
> When all is said and done, a C bool is a C int,

Not if it is a _Bool (via stdbool.h or some other way).

A _Bool is always either 0 or 1, and scalar value which is converted to
a _Bool is converted to either 0 or 1.

> So no, types, bool or otherwise, do not cause any implicit '!!' to
> happen.

I believe this is not correct when _Bool is used.

In u-boot a bool is indeed a _Bool (or at least I don't see any other
typedef's and I can see various includes on stdbool.h, I therefore
didn't feel the need to check how bool is arrived at in this particular
file).

Ian.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 14:46             ` Siarhei Siamashka
  2015-10-25 16:08               ` Marek Vasut
@ 2015-10-25 19:24               ` Ian Campbell
  2015-10-25 20:48                 ` Siarhei Siamashka
  1 sibling, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2015-10-25 19:24 UTC (permalink / raw)
  To: u-boot

On Sun, 2015-10-25 at 16:46 +0200, Siarhei Siamashka wrote:
> replace "#include <stdbool.h>" with "typedef int bool;" and try it
> again.

So don't do that then.

In u-boot the "bool" type comes from stdbool.h and it is therefore
completely reasonable to assume that bool in u-boot has the properties
which this implies.

Ian.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 19:24               ` Ian Campbell
@ 2015-10-25 20:48                 ` Siarhei Siamashka
  0 siblings, 0 replies; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-25 20:48 UTC (permalink / raw)
  To: u-boot

On Sun, 25 Oct 2015 19:24:04 +0000
Ian Campbell <ijc+uboot@hellion.org.uk> wrote:

> On Sun, 2015-10-25 at 16:46 +0200, Siarhei Siamashka wrote:
> > replace "#include <stdbool.h>" with "typedef int bool;" and try it
> > again.
> 
> So don't do that then.

Yes, I'm not going to do this. It was just an artificial example,
demonstrating how things work. I'm only a little bit worried about
a potential rogue typedef unexpectedly coming from one of the
include files one day.

> In u-boot the "bool" type comes from stdbool.h and it is therefore
> completely reasonable to assume that bool in u-boot has the properties
> which this implies.

Right, except that there are still a few bool related typedefs
in the code:
    https://github.com/u-boot/u-boot/blob/v2015.10/lib/lzma/Types.h#L83
    https://github.com/u-boot/u-boot/blob/v2015.10/lib/bzip2/bzlib_private.h#L85

Yes, they come from third-party libraries and don't really redefine
bool or _Bool. But the C language did not always have a native bool
data type. And historically it had been a common practice to typedef
bool to some integer type. One should not be very much surprised upon
encountering such code in the wild even now.

Regarding the code efficiency. The following example:

    #include <stdbool.h>

    bool foo(int x)
    {
        return x & 4;
    }

    bool bar(int x)
    {
        return !!(x & 4);
    }

Compiles into the following code on ARM:

    00000000 <foo>:
       0:	e7e00150 	ubfx	r0, r0, #2, #1
       4:	e12fff1e 	bx	lr

    00000008 <bar>:
       8:	e7e00150 	ubfx	r0, r0, #2, #1
       c:	e12fff1e 	bx	lr

Basically, there is exactly no difference. And removing "!!" only
has a cosmetic value for the code that uses the bool definition
from <stdbool.h>.

As I already said, I'm perfectly fine with either of these variants :-)
The purists are welcome to start a crusade eliminating all of the uses
of the "!!" construct in the U-Boot code. Probably starting with the
other dfu_usb_get_reset() implementations for the sake of consistency:
    https://github.com/u-boot/u-boot/blob/v2015.10/drivers/usb/gadget/s3c_udc_otg.c#L151
    https://github.com/u-boot/u-boot/blob/v2015.10/drivers/usb/gadget/ci_udc.c#L1062

-- 
Best regards,
Siarhei Siamashka

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 19:22           ` Ian Campbell
@ 2015-10-25 21:16             ` Albert ARIBAUD
  2015-10-26 10:07               ` Ian Campbell
  0 siblings, 1 reply; 24+ messages in thread
From: Albert ARIBAUD @ 2015-10-25 21:16 UTC (permalink / raw)
  To: u-boot

Hello Ian,

On Sun, 25 Oct 2015 19:22:00 +0000, Ian Campbell
<ijc+uboot@hellion.org.uk> wrote:
> On Sun, 2015-10-25 at 14:22 +0100, Albert ARIBAUD wrote:
> > On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> > > Doesn't the bool return type already cause that to happen? (from the
> > > PoV of the caller at least)
> > 
> > When all is said and done, a C bool is a C int,
> 
> Not if it is a _Bool (via stdbool.h or some other way).
> 
> A _Bool is always either 0 or 1, and scalar value which is converted to
> a _Bool is converted to either 0 or 1.
>
> > So no, types, bool or otherwise, do not cause any implicit '!!' to
> > happen.
> 
> I believe this is not correct when _Bool is used.
> 
> In u-boot a bool is indeed a _Bool (or at least I don't see any other
> typedef's and I can see various includes on stdbool.h, I therefore
> didn't feel the need to check how bool is arrived at in this particular
> file).

What you write is possibly correct for C++, but certainly not for C,
for which booleans are integers, with no compiler-enforced constraint
on their value domains.

Amicalement,
-- 
Albert.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 14:55     ` Siarhei Siamashka
  2015-10-25 16:09       ` Marek Vasut
@ 2015-10-25 21:19       ` Albert ARIBAUD
  1 sibling, 0 replies; 24+ messages in thread
From: Albert ARIBAUD @ 2015-10-25 21:19 UTC (permalink / raw)
  To: u-boot

Hello Siarhei,

On Sun, 25 Oct 2015 16:55:43 +0200, Siarhei Siamashka
<siarhei.siamashka@gmail.com> wrote:
> On Sun, 25 Oct 2015 12:00:09 +0100
> Marek Vasut <marex@denx.de> wrote:
> 
> > On Sunday, October 25, 2015 at 05:44:47 AM, Siarhei Siamashka wrote:
> > > This is necessary to distinguish between the "dfu-util --detach" and
> > > the "dfu-util --reset" requests.
> > > 
> > > The default weak implementation of dfu_usb_get_reset() unconditionally
> > > reboots the device, but we want to be able to continue the boot.scr
> > > execution after writing the kernel, fdt and ramdisk to RAM via DFU.
> > > 
> > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> > > ---
> > >  drivers/usb/musb-new/sunxi.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> > > index a146c08..5eb8d19 100644
> > > --- a/drivers/usb/musb-new/sunxi.c
> > > +++ b/drivers/usb/musb-new/sunxi.c
> > > @@ -166,6 +166,17 @@ static void USBC_ConfigFIFO_Base(void)
> > >  }
> > > 
> > >  /*************************************************************************
> > > ***** + * Needed for the DFU polling magic
> > > +
> > > **************************************************************************
> > > ****/ +
> > > +static u8 last_int_usb;
> > > +
> > > +bool dfu_usb_get_reset(void)
> > > +{
> > > +	return !!(last_int_usb & MUSB_INTR_RESET);
> > 
> > The !! is not needed.
> 
> Thanks for your comment, I can surely change this in the updated
> version of the patch.

For the sake of consistency, I'd rather you did not remove the '!!'. If
you decide to remove it, then you should change the function's type
to int and mention that it may return zero or non-zero. In C, boolean
operators '!', '||' and '&&' always evaluate to 1 for true.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-25 21:16             ` Albert ARIBAUD
@ 2015-10-26 10:07               ` Ian Campbell
  2015-10-26 11:32                 ` Albert ARIBAUD
  0 siblings, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2015-10-26 10:07 UTC (permalink / raw)
  To: u-boot

On Sun, 2015-10-25 at 22:16 +0100, Albert ARIBAUD wrote:
> Hello Ian,
> 
> On Sun, 25 Oct 2015 19:22:00 +0000, Ian Campbell
> <ijc+uboot@hellion.org.uk> wrote:
> > On Sun, 2015-10-25 at 14:22 +0100, Albert ARIBAUD wrote:
> > > On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> > > > Doesn't the bool return type already cause that to happen?
> (from the
> > > > PoV of the caller at least)
> > > 
> > > When all is said and done, a C bool is a C int,
> > 
> > Not if it is a _Bool (via stdbool.h or some other way).
> > 
> > A _Bool is always either 0 or 1, and scalar value which is
> converted to
> > a _Bool is converted to either 0 or 1.
> >
> > > So no, types, bool or otherwise, do not cause any implicit '!!'
> to
> > > happen.
> > 
> > I believe this is not correct when _Bool is used.
> > 
> > In u-boot a bool is indeed a _Bool (or at least I don't see any
> other
> > typedef's and I can see various includes on stdbool.h, I therefore
> > didn't feel the need to check how bool is arrived at in this
> particular
> > file).
> 
> What you write is possibly correct for C++, but certainly not for C,
> for which booleans are integers, with no compiler-enforced constraint
> on their value domains.

I know next to nothing about C++ so I am certainly not confusing things
with that.

The _Bool type in C99 is an integer which may take on exactly the
values 0 or 1. Since the code which started this subthread was using
"bool" from <stdbool.h> it is _Bool which is being discussed here.

The actual standard costs money (and is therefore unlinkable) but 
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf is a late
draft and I believe this aspect is unchanged. Section 6.3.1.2 is one
relevant part explaining that a _Bool must always be either 0 or 1:

    When any scalar value is converted to _Bool, the result is 0 if the
    value compares equal to 0; otherwise, the result is 1.

Many of the other clauses dealing with Integers (e.g. 6.3.1.3.1) have
had "other than _Bool" or some similar wording added to them since
_Bool does indeed behave a little differently.

So I'm afraid I disagree with your statement, at least for C >= C99 (I
can't recall if _Bool was in C89, but I think the answer is no).

Ian.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices
  2015-10-25  4:44 [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Siarhei Siamashka
  2015-10-25  4:44 ` [U-Boot] [PATCH 1/2] sunxi: Enable DFU for RAM Siarhei Siamashka
  2015-10-25  4:44 ` [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset() Siarhei Siamashka
@ 2015-10-26 11:18 ` Piotr Król
  2015-10-27  4:31   ` Siarhei Siamashka
  2 siblings, 1 reply; 24+ messages in thread
From: Piotr Król @ 2015-10-26 11:18 UTC (permalink / raw)
  To: u-boot

On Sun, Oct 25, 2015 at 06:44:45AM +0200, Siarhei Siamashka wrote:
> Hello,
> 
> DFU allows to transfer large files (such as initrd images) much
> faster than FEL.
> 
> Siarhei Siamashka (2):
>   sunxi: Enable DFU for RAM
>   musb: sunxi: Implement dfu_usb_get_reset()
> 
>  drivers/usb/musb-new/sunxi.c   | 12 ++++++++++++
>  include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
>  2 files changed, 37 insertions(+), 5 deletions(-)

Siarhei,
can you give some pointers how to test those patches. I have
A20-OLinuXino-Micro and Cubietruck and would be glad to give them a try.

I think it would be interested to combine this method with Boris NAND support
and get much better solution then {Live,Phoenix}Suit.

Best Regards,
-- 
Piotr Kr?l
Embedded Systems Consultant
http://3mdeb.com | @3mdeb_com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset()
  2015-10-26 10:07               ` Ian Campbell
@ 2015-10-26 11:32                 ` Albert ARIBAUD
  0 siblings, 0 replies; 24+ messages in thread
From: Albert ARIBAUD @ 2015-10-26 11:32 UTC (permalink / raw)
  To: u-boot

Hello Ian,

On Mon, 26 Oct 2015 10:07:24 +0000, Ian Campbell
<ijc+uboot@hellion.org.uk> wrote:
> On Sun, 2015-10-25 at 22:16 +0100, Albert ARIBAUD wrote:
> > Hello Ian,
> > 
> > On Sun, 25 Oct 2015 19:22:00 +0000, Ian Campbell
> > <ijc+uboot@hellion.org.uk> wrote:
> > > On Sun, 2015-10-25 at 14:22 +0100, Albert ARIBAUD wrote:
> > > > On Sun, 25 Oct 2015 12:40:45 +0000, Ian Campbell
> > > > > Doesn't the bool return type already cause that to happen?
> > (from the
> > > > > PoV of the caller at least)
> > > > 
> > > > When all is said and done, a C bool is a C int,
> > > 
> > > Not if it is a _Bool (via stdbool.h or some other way).
> > > 
> > > A _Bool is always either 0 or 1, and scalar value which is
> > converted to
> > > a _Bool is converted to either 0 or 1.
> > >
> > > > So no, types, bool or otherwise, do not cause any implicit '!!'
> > to
> > > > happen.
> > > 
> > > I believe this is not correct when _Bool is used.
> > > 
> > > In u-boot a bool is indeed a _Bool (or at least I don't see any
> > other
> > > typedef's and I can see various includes on stdbool.h, I therefore
> > > didn't feel the need to check how bool is arrived at in this
> > particular
> > > file).
> > 
> > What you write is possibly correct for C++, but certainly not for C,
> > for which booleans are integers, with no compiler-enforced constraint
> > on their value domains.
> 
> I know next to nothing about C++ so I am certainly not confusing things
> with that.
> 
> The _Bool type in C99 is an integer which may take on exactly the
> values 0 or 1. Since the code which started this subthread was using
> "bool" from <stdbool.h> it is _Bool which is being discussed here.
> 
> The actual standard costs money (and is therefore unlinkable) but 
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf is a late
> draft and I believe this aspect is unchanged. Section 6.3.1.2 is one
> relevant part explaining that a _Bool must always be either 0 or 1:
> 
>     When any scalar value is converted to _Bool, the result is 0 if the
>     value compares equal to 0; otherwise, the result is 1.
> 
> Many of the other clauses dealing with Integers (e.g. 6.3.1.3.1) have
> had "other than _Bool" or some similar wording added to them since
> _Bool does indeed behave a little differently.
> 
> So I'm afraid I disagree with your statement, at least for C >= C99 (I
> can't recall if _Bool was in C89, but I think the answer is no).

I stand corrected: I've just checked it, and the conversion does indeed
happen on return -- at least gcc 5.2.1 -- even when, like in U-Boot
build files, c99 standard compliance is not specified.

If older GCCs (up to a point: how old a gcc are we wanting to
support?) also work this way too, the '!!' is indeed unneeded.

> Ian.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices
  2015-10-26 11:18 ` [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Piotr Król
@ 2015-10-27  4:31   ` Siarhei Siamashka
  2015-10-27 23:27     ` Piotr Król
  0 siblings, 1 reply; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-27  4:31 UTC (permalink / raw)
  To: u-boot

On Mon, 26 Oct 2015 12:18:35 +0100
Piotr Kr?l <piotr.krol@3mdeb.com> wrote:

> On Sun, Oct 25, 2015 at 06:44:45AM +0200, Siarhei Siamashka wrote:
> > Hello,
> > 
> > DFU allows to transfer large files (such as initrd images) much
> > faster than FEL.
> > 
> > Siarhei Siamashka (2):
> >   sunxi: Enable DFU for RAM
> >   musb: sunxi: Implement dfu_usb_get_reset()
> > 
> >  drivers/usb/musb-new/sunxi.c   | 12 ++++++++++++
> >  include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
> >  2 files changed, 37 insertions(+), 5 deletions(-)
> 
> Siarhei,
> can you give some pointers how to test those patches. I have
> A20-OLinuXino-Micro and Cubietruck and would be glad to give them a try.

Hello,

I tried to provide some basic usage instructions as a part of
the commit message:
    https://patchwork.ozlabs.org/patch/535535/

But you also need the "sunxi: cubietruck: Enable the USB OTG
controller" patch from Maxime Ripard to enable USB OTG on the
Cubietruck:
    https://patchwork.ozlabs.org/patch/530656/

> I think it would be interested to combine this method with Boris NAND support
> and get much better solution then {Live,Phoenix}Suit.

At this stage I'm only interested in the DFU usage as a speed
booster for FEL. Booting over USB via FEL allows us to temporarily
run more or less complete system (kernel and rootfs on ramdisk)
on any Allwinner device without modifying existing pre-installed
software on non-volatile storage. This already works fine, but
we were not quite happy about the data transfer speed.

If you are interested in flashing NAND, then you can probably
have a look at the recent fastboot patches from Maxime.

DFU means "Device Firmware Upgrade" and it can be also used for
flashing NAND or writing images to SD cards over USB (if we
hook up this part of the DFU functionality). The main question
is how many alternative NAND flashing methods do we need?

-- 
Best regards,
Siarhei Siamashka

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices
  2015-10-27  4:31   ` Siarhei Siamashka
@ 2015-10-27 23:27     ` Piotr Król
  2015-10-28  4:49       ` Siarhei Siamashka
  0 siblings, 1 reply; 24+ messages in thread
From: Piotr Król @ 2015-10-27 23:27 UTC (permalink / raw)
  To: u-boot

On Tue, Oct 27, 2015 at 06:31:24AM +0200, Siarhei Siamashka wrote:
> On Mon, 26 Oct 2015 12:18:35 +0100
> Piotr Kr?l <piotr.krol@3mdeb.com> wrote:
> 
> > On Sun, Oct 25, 2015 at 06:44:45AM +0200, Siarhei Siamashka wrote:
> > > Hello,
> > > 
> > > DFU allows to transfer large files (such as initrd images) much
> > > faster than FEL.
> > > 
> > > Siarhei Siamashka (2):
> > >   sunxi: Enable DFU for RAM
> > >   musb: sunxi: Implement dfu_usb_get_reset()
> > > 
> > >  drivers/usb/musb-new/sunxi.c   | 12 ++++++++++++
> > >  include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
> > >  2 files changed, 37 insertions(+), 5 deletions(-)
> > 
> > Siarhei,
> > can you give some pointers how to test those patches. I have
> > A20-OLinuXino-Micro and Cubietruck and would be glad to give them a try.
> 
> Hello,
> 
> I tried to provide some basic usage instructions as a part of
> the commit message:
>     https://patchwork.ozlabs.org/patch/535535/
> 

Hi Siarhei,
unfortunately I'm not able to compile even clean master with enabled
CONFIG_USB_MUSB_GADGET. I'm using Linaro toolchain and get this error:

arm-linux-gnueabihf-ld.bfd: error:
/home/pietrushnic/storage/wdc/projects/3mdeb/cubietruck/toolchains/gcc-linaro-4.9-2015.05-x86_64_arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/4.9.3/libgcc.a(_udivmoddi4.o) uses VFP register arguments, u-boot does not
arm-linux-gnueabihf-ld.bfd: failed to merge target specific data of file
/home/pietrushnic/storage/wdc/projects/3mdeb/cubietruck/toolchains/gcc-linaro-4.9-2015.05-x86_64_arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/4.9.3/libgcc.a(_udivmoddi4.o)
Makefile:1183: recipe for target 'u-boot' failed
make: *** [u-boot] Error 1

CONFIG_USB_MUSB_GADGET automatically enable CONFIG_USB_MUSB_SUNXI. When
I try with CONFIG_USB_MUSB_SUNXI not set I'm getting other failure:

(...)
drivers/usb/musb-new/musb_gadget.c:133:8: error: 'DMA_TO_DEVICE' undeclared (first use in this function)
(...)
drivers/usb/musb-new/musb_gadget.c:134:8: error: 'DMA_FROM_DEVICE' undeclared (first use in this function)
      : DMA_FROM_DEVICE);
        ^
(...)
drivers/usb/musb-new/musb_gadget.c:164:7: error: 'DMA_TO_DEVICE' undeclared (first use in this function)
     ? DMA_TO_DEVICE
       ^
drivers/usb/musb-new/musb_gadget.c:165:7: error: 'DMA_FROM_DEVICE' undeclared (first use in this function)
     : DMA_FROM_DEVICE);

My steps:
make CROSS_COMPILE=arm-linux-gnueabihf- Cubietruck_defconfig
make CROSS_COMPILE=arm-linux-gnueabihf- menuconfig 
# enable CONFIG_USB_MUSB_GADGET
make CROSS_COMPILE=arm-linux-gnueabihf- -j$(nproc)

Any idea how to narrow VFP problem ?

> But you also need the "sunxi: cubietruck: Enable the USB OTG
> controller" patch from Maxime Ripard to enable USB OTG on the
> Cubietruck:
>     https://patchwork.ozlabs.org/patch/530656/
> 

Cannot apply this series to master cleanly. Should I try different tree ?

> > I think it would be interested to combine this method with Boris NAND support
> > and get much better solution then {Live,Phoenix}Suit.
> 
> At this stage I'm only interested in the DFU usage as a speed
> booster for FEL. Booting over USB via FEL allows us to temporarily
> run more or less complete system (kernel and rootfs on ramdisk)
> on any Allwinner device without modifying existing pre-installed
> software on non-volatile storage. This already works fine, but
> we were not quite happy about the data transfer speed.
> 
> If you are interested in flashing NAND, then you can probably
> have a look at the recent fastboot patches from Maxime.
> 
> DFU means "Device Firmware Upgrade" and it can be also used for
> flashing NAND or writing images to SD cards over USB (if we
> hook up this part of the DFU functionality). The main question
> is how many alternative NAND flashing methods do we need?

IMHO one method that works would be great.

Best Regards,
-- 
Piotr Kr?l
Embedded Systems Consultant
http://3mdeb.com | @3mdeb_com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices
  2015-10-27 23:27     ` Piotr Król
@ 2015-10-28  4:49       ` Siarhei Siamashka
  0 siblings, 0 replies; 24+ messages in thread
From: Siarhei Siamashka @ 2015-10-28  4:49 UTC (permalink / raw)
  To: u-boot

On Wed, 28 Oct 2015 00:27:48 +0100
Piotr Kr?l <piotr.krol@3mdeb.com> wrote:

> On Tue, Oct 27, 2015 at 06:31:24AM +0200, Siarhei Siamashka wrote:
> > On Mon, 26 Oct 2015 12:18:35 +0100
> > Piotr Kr?l <piotr.krol@3mdeb.com> wrote:
> > 
> > > On Sun, Oct 25, 2015 at 06:44:45AM +0200, Siarhei Siamashka wrote:
> > > > Hello,
> > > > 
> > > > DFU allows to transfer large files (such as initrd images) much
> > > > faster than FEL.
> > > > 
> > > > Siarhei Siamashka (2):
> > > >   sunxi: Enable DFU for RAM
> > > >   musb: sunxi: Implement dfu_usb_get_reset()
> > > > 
> > > >  drivers/usb/musb-new/sunxi.c   | 12 ++++++++++++
> > > >  include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
> > > >  2 files changed, 37 insertions(+), 5 deletions(-)
> > > 
> > > Siarhei,
> > > can you give some pointers how to test those patches. I have
> > > A20-OLinuXino-Micro and Cubietruck and would be glad to give them a try.
> > 
> > Hello,
> > 
> > I tried to provide some basic usage instructions as a part of
> > the commit message:
> >     https://patchwork.ozlabs.org/patch/535535/
> > 
> 
> Hi Siarhei,
> unfortunately I'm not able to compile even clean master with enabled
> CONFIG_USB_MUSB_GADGET. I'm using Linaro toolchain and get this error:
> 
> arm-linux-gnueabihf-ld.bfd: error:
> /home/pietrushnic/storage/wdc/projects/3mdeb/cubietruck/toolchains/gcc-linaro-4.9-2015.05-x86_64_arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/4.9.3/libgcc.a(_udivmoddi4.o) uses VFP register arguments, u-boot does not
> arm-linux-gnueabihf-ld.bfd: failed to merge target specific data of file
> /home/pietrushnic/storage/wdc/projects/3mdeb/cubietruck/toolchains/gcc-linaro-4.9-2015.05-x86_64_arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/4.9.3/libgcc.a(_udivmoddi4.o)
> Makefile:1183: recipe for target 'u-boot' failed
> make: *** [u-boot] Error 1

[...]

> My steps:
> make CROSS_COMPILE=arm-linux-gnueabihf- Cubietruck_defconfig
> make CROSS_COMPILE=arm-linux-gnueabihf- menuconfig 
> # enable CONFIG_USB_MUSB_GADGET
> make CROSS_COMPILE=arm-linux-gnueabihf- -j$(nproc)
> 
> Any idea how to narrow VFP problem ?

Thanks for reporting this. I have also confirmed the problem with
a hardfloat toolchain and just sent a patch:

    https://patchwork.ozlabs.org/patch/537185/

Debugging this involved just checking all the symbols in the object
files generated by a softfloat toolchain (it does not fail), then
filtering out all the symbols which also exist in a successful
hardfloat build (with fastboot config options disabled) and
comparing the remaining symbols with libgcc.a

The culprit was "__aeabi_uldivmod".

> > But you also need the "sunxi: cubietruck: Enable the USB OTG
> > controller" patch from Maxime Ripard to enable USB OTG on the
> > Cubietruck:
> >     https://patchwork.ozlabs.org/patch/530656/
> > 
> 
> Cannot apply this series to master cleanly. Should I try different tree ?

This whole series is not needed if you are only interested in DFU.

We just need to ensure that USB OTG is eventually enabled on all
sunxi boards (and also properly handles the ID pin to switch
between the device and host roles).

-- 
Best regards,
Siarhei Siamashka

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [U-Boot] [U-Boot,1/2] sunxi: Enable DFU for RAM
  2015-10-25  4:44 ` [U-Boot] [PATCH 1/2] sunxi: Enable DFU for RAM Siarhei Siamashka
@ 2015-11-20 15:21   ` Hans de Goede
  0 siblings, 0 replies; 24+ messages in thread
From: Hans de Goede @ 2015-11-20 15:21 UTC (permalink / raw)
  To: u-boot

Hi,

On 25-10-15 05:44, Siarhei Siamashka wrote:
> The DFU protocol implementation in U-Boot is much faster than the
> FEL protocol implementation in the boot ROM on Allwinner devices.
> Using DFU instead of FEL improves the USB transfer speed from
> 500-900 KB/s to 3.2-3.7 MB/s. This is particularly useful for
> reducing the time needed for booting systems with large initrd
> images.
>
> FEL is still useful for loading the U-Boot bootloader and a boot
> script, which may then activate DFU in the following way:
>
>     setenv dfu_alt_info ${dfu_alt_info_ram}
>     dfu 0 ram 0
>     bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}
>
> The rest of the files can be transferred to the device using the
> "dfu-util" tool.
>
> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

Thanks, I've applied this series to my tree, and it will be part
of the next pull-req.

Regards,

Hans


> ---
>   include/configs/sunxi-common.h | 30 +++++++++++++++++++++++++-----
>   1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index ddcfe94..38c0bc5 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -337,6 +337,7 @@ extern int soft_i2c_gpio_scl;
>   #define CONFIG_USB_GADGET_VBUS_DRAW	0
>
>   #define CONFIG_USB_GADGET_DOWNLOAD
> +#define CONFIG_USB_FUNCTION_DFU
>   #define CONFIG_USB_FUNCTION_FASTBOOT
>   #define CONFIG_USB_FUNCTION_MASS_STORAGE
>   #endif
> @@ -347,6 +348,11 @@ extern int soft_i2c_gpio_scl;
>   #define CONFIG_G_DNL_MANUFACTURER	"Allwinner Technology"
>   #endif
>
> +#ifdef CONFIG_USB_FUNCTION_DFU
> +#define CONFIG_CMD_DFU
> +#define CONFIG_DFU_RAM
> +#endif
> +
>   #ifdef CONFIG_USB_FUNCTION_FASTBOOT
>   #define CONFIG_CMD_FASTBOOT
>   #define CONFIG_FASTBOOT_BUF_ADDR	CONFIG_SYS_LOAD_ADDR
> @@ -394,13 +400,26 @@ extern int soft_i2c_gpio_scl;
>    * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
>    * 1M script, 1M pxe and the ramdisk at the end.
>    */
> +
> +#define KERNEL_ADDR_R  __stringify(SDRAM_OFFSET(2000000))
> +#define FDT_ADDR_R     __stringify(SDRAM_OFFSET(3000000))
> +#define SCRIPT_ADDR_R  __stringify(SDRAM_OFFSET(3100000))
> +#define PXEFILE_ADDR_R __stringify(SDRAM_OFFSET(3200000))
> +#define RAMDISK_ADDR_R __stringify(SDRAM_OFFSET(3300000))
> +
>   #define MEM_LAYOUT_ENV_SETTINGS \
>   	"bootm_size=0xa000000\0" \
> -	"kernel_addr_r=" __stringify(SDRAM_OFFSET(2000000)) "\0" \
> -	"fdt_addr_r=" __stringify(SDRAM_OFFSET(3000000)) "\0" \
> -	"scriptaddr=" __stringify(SDRAM_OFFSET(3100000)) "\0" \
> -	"pxefile_addr_r=" __stringify(SDRAM_OFFSET(3200000)) "\0" \
> -	"ramdisk_addr_r=" __stringify(SDRAM_OFFSET(3300000)) "\0"
> +	"kernel_addr_r=" KERNEL_ADDR_R "\0" \
> +	"fdt_addr_r=" FDT_ADDR_R "\0" \
> +	"scriptaddr=" SCRIPT_ADDR_R "\0" \
> +	"pxefile_addr_r=" PXEFILE_ADDR_R "\0" \
> +	"ramdisk_addr_r=" RAMDISK_ADDR_R "\0"
> +
> +#define DFU_ALT_INFO_RAM \
> +	"dfu_alt_info_ram=" \
> +	"kernel ram " KERNEL_ADDR_R " 0x1000000;" \
> +	"fdt ram " FDT_ADDR_R " 0x100000;" \
> +	"ramdisk ram " RAMDISK_ADDR_R " 0x4000000\0"
>
>   #ifdef CONFIG_MMC
>   #define BOOT_TARGET_DEVICES_MMC(func) func(MMC, mmc, 0)
> @@ -482,6 +501,7 @@ extern int soft_i2c_gpio_scl;
>   #define CONFIG_EXTRA_ENV_SETTINGS \
>   	CONSOLE_ENV_SETTINGS \
>   	MEM_LAYOUT_ENV_SETTINGS \
> +	DFU_ALT_INFO_RAM \
>   	"fdtfile=" CONFIG_DEFAULT_DEVICE_TREE ".dtb\0" \
>   	"console=ttyS0,115200\0" \
>   	BOOTCMD_SUNXI_COMPAT \
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2015-11-20 15:21 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-25  4:44 [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Siarhei Siamashka
2015-10-25  4:44 ` [U-Boot] [PATCH 1/2] sunxi: Enable DFU for RAM Siarhei Siamashka
2015-11-20 15:21   ` [U-Boot] [U-Boot,1/2] " Hans de Goede
2015-10-25  4:44 ` [U-Boot] [PATCH 2/2] musb: sunxi: Implement dfu_usb_get_reset() Siarhei Siamashka
2015-10-25 11:00   ` Marek Vasut
2015-10-25 11:46     ` Albert ARIBAUD
2015-10-25 12:40       ` Ian Campbell
2015-10-25 13:22         ` Albert ARIBAUD
2015-10-25 13:29           ` Marek Vasut
2015-10-25 14:46             ` Siarhei Siamashka
2015-10-25 16:08               ` Marek Vasut
2015-10-25 19:24               ` Ian Campbell
2015-10-25 20:48                 ` Siarhei Siamashka
2015-10-25 19:22           ` Ian Campbell
2015-10-25 21:16             ` Albert ARIBAUD
2015-10-26 10:07               ` Ian Campbell
2015-10-26 11:32                 ` Albert ARIBAUD
2015-10-25 14:55     ` Siarhei Siamashka
2015-10-25 16:09       ` Marek Vasut
2015-10-25 21:19       ` Albert ARIBAUD
2015-10-26 11:18 ` [U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices Piotr Król
2015-10-27  4:31   ` Siarhei Siamashka
2015-10-27 23:27     ` Piotr Król
2015-10-28  4:49       ` Siarhei Siamashka

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.