[U-Boot] [PATCH 0/2] Enable DFU for RAM on Allwinner devices
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(-)
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 \
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 \
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);
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
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,
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)
Ian.
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,
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
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.
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
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.
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_ot... https://github.com/u-boot/u-boot/blob/v2015.10/drivers/usb/gadget/ci_udc.c#L...
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.
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,
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.
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,
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).
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#L64It might be also a bit racy (the RESET interrupt could be potentially missed sometimes).
That's a question for Lukasz .
Best regards, Marek Vasut
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,
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,
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?
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,
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).
participants (6)
-
Albert ARIBAUD -
Hans de Goede -
Ian Campbell -
Marek Vasut -
Piotr Król -
Siarhei Siamashka