* [U-Boot] [RFC PATCH v4 0/3] Enable FIT image to be loaded beyond 32-bit space
@ 2016-02-12 20:59 York Sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host York Sun
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: York Sun @ 2016-02-12 20:59 UTC (permalink / raw)
To: u-boot
Please comment on these changes. Don't apply yet. I think there may
be compiling warnings on some platforms. Want to get some feedback
before going too far on this path.
This set fixes compiling warnings for sandbox on 32-bit host (Ubuntu),
convert ulong to phys_addr_t for image handling. The purpose is to fix
image addresses so FIT image can be put beyond 32-bit space.
The challenge is to keep 32-bit host tool (eg mkimage) working. Using
unsigned long long as phys_addr_t for host is the trick.
This patchset is only tested on powerpc and arm for selected platforms.
More test (at least compiling) is needed for other platforms.
Changes in v4:
New patch to fix compiling warnings for sandbox built on 32-bit host
Still need to change CONFIG_SANDBOX_BITS_PER_LONG to 32 to avoid
these warnings
include/asm-generic/bitops/__fls.h:17:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:19:3: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:22:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:26:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:30:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:34:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:38:2: warning: left shift
count >= width of type [enabled by default]
New patch, separated from fixing FIT image.
Separate ulong to phys_addr_t change to another patch.
Changes in v3:
Define PRIpa for host and target in common/image-fit.c so printf works
properly for 32-, 64-bit targets and host tools.
Changes in v2:
Make a common function for both load and entry addresses.
Simplify calculation of addresses in a similar way as fdtdec_get_number()
fdtdec_get_number() is not used, or too many files need to be included
and/or twisted for host tool
Continue to use %08llx for print format for load and entry addresses
because %pa does not always work for host tool (mkimage)
York Sun (3):
sandbox: Fix compiling warning on 32-bit host
common: Convert ulong to phys_addr_t for image addresses
common: Fix load and entry addresses in FIT image
arch/powerpc/lib/bootm.c | 4 +-
arch/sandbox/cpu/cpu.c | 3 +-
arch/sandbox/include/asm/types.h | 4 +-
arch/sandbox/lib/bootm.c | 3 +-
arch/sandbox/lib/pci_io.c | 2 +-
cmd/bootm.c | 4 +-
cmd/lzmadec.c | 5 ++-
cmd/ximg.c | 9 +++--
common/bootm.c | 21 +++++-----
common/bootm_os.c | 14 ++++---
common/image-android.c | 6 +--
common/image-fdt.c | 16 ++++----
common/image-fit.c | 81 +++++++++++++++++++++-----------------
common/image.c | 17 ++++----
disk/part_efi.c | 2 +-
include/bootm.h | 6 +--
include/image.h | 36 +++++++++++------
lib/hashtable.c | 2 +-
tools/default_image.c | 2 +-
19 files changed, 134 insertions(+), 103 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host
2016-02-12 20:59 [U-Boot] [RFC PATCH v4 0/3] Enable FIT image to be loaded beyond 32-bit space York Sun
@ 2016-02-12 20:59 ` York Sun
2016-02-16 16:01 ` Simon Glass
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses York Sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image York Sun
2 siblings, 1 reply; 13+ messages in thread
From: York Sun @ 2016-02-12 20:59 UTC (permalink / raw)
To: u-boot
Fix the following compiling warning on 32-bit host
disk/part_efi.c: In function 'alloc_read_gpt_entries':
disk/part_efi.c:894:2: warning: format '%zu' expects argument of
type 'size_t', but argument 5 has type 'long unsigned int'
[-Wformat]
disk/part_efi.c:907:4: warning: format '%zX' expects argument of
type 'size_t', but argument 3 has type 'long unsigned int'
[-Wformat]
cmd/lzmadec.c: In function 'do_lzmadec':
cmd/lzmadec.c:39:12: warning: passing argument 2 of
'lzmaBuffToBuffDecompress' from incompatible pointer type
[enabled by default]
include/lzma/../../lib/lzma/LzmaTools.h:17:12: note: expected
'SizeT *' but argument is of type 'long unsigned int *'
lib/hashtable.c: In function 'hexport_r':
lib/hashtable.c:605:2: warning: format '%zu' expects argument of
type 'size_t', but argument 5 has type 'long unsigned int'
[-Wformat]
lib/hashtable.c:661:5: warning: format '%zu' expects argument of
type 'size_t', but argument 2 has type 'long unsigned int'
[-Wformat]
lib/hashtable.c:661:5: warning: format '%zu' expects argument of
type 'size_t', but argument 3 has type 'long unsigned int'
[-Wformat]
lib/hashtable.c: In function 'himport_r':
lib/hashtable.c:793:3: warning: format '%zu' expects argument of
type 'size_t', but argument 2 has type 'long unsigned int'
[-Wformat]
Signed-off-by: York Sun <york.sun@nxp.com>
---
Changes in v4:
New patch to fix compiling warnings for sandbox built on 32-bit host
Still need to change CONFIG_SANDBOX_BITS_PER_LONG to 32 to avoid
these warnings
include/asm-generic/bitops/__fls.h:17:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:19:3: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:22:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:26:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:30:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:34:2: warning: left shift
count >= width of type [enabled by default]
include/asm-generic/bitops/__fls.h:38:2: warning: left shift
count >= width of type [enabled by default]
Changes in v3: None
Changes in v2: None
arch/sandbox/cpu/cpu.c | 3 ++-
arch/sandbox/include/asm/types.h | 4 ++--
arch/sandbox/lib/bootm.c | 3 ++-
arch/sandbox/lib/pci_io.c | 2 +-
cmd/bootm.c | 4 ++--
cmd/lzmadec.c | 5 +++--
disk/part_efi.c | 2 +-
include/image.h | 6 ++++++
lib/hashtable.c | 2 +-
9 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 196f3e1..7aad876 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -62,7 +62,8 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
map_dev = NULL;
if (enable_pci_map && !pci_map_physmem(paddr, &len, &map_dev, &ptr)) {
if (plen != len) {
- printf("%s: Warning: partial map at %x, wanted %lx, got %lx\n",
+ printf("%s: Warning: partial map at " PRIpa
+ ", wanted %lx, got %lx\n",
__func__, paddr, len, plen);
}
map_len = len;
diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
index 42c09e2..c3bb76e 100644
--- a/arch/sandbox/include/asm/types.h
+++ b/arch/sandbox/include/asm/types.h
@@ -53,8 +53,8 @@ typedef __UINT64_TYPE__ u64;
#define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
typedef unsigned long dma_addr_t;
-typedef u32 phys_addr_t;
-typedef u32 phys_size_t;
+typedef unsigned long phys_addr_t;
+typedef unsigned long phys_size_t;
#endif /* __KERNEL__ */
diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c
index d49c927..9ca89b5 100644
--- a/arch/sandbox/lib/bootm.c
+++ b/arch/sandbox/lib/bootm.c
@@ -54,7 +54,8 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
{
if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
bootstage_mark(BOOTSTAGE_ID_RUN_OS);
- printf("## Transferring control to Linux (at address %08lx)...\n",
+ printf("## Transferring control to Linux (at address " PRIpa
+ ")...\n",
images->ep);
reset_cpu(0);
}
diff --git a/arch/sandbox/lib/pci_io.c b/arch/sandbox/lib/pci_io.c
index 0de124f..3ada286 100644
--- a/arch/sandbox/lib/pci_io.c
+++ b/arch/sandbox/lib/pci_io.c
@@ -35,7 +35,7 @@ int pci_map_physmem(phys_addr_t paddr, unsigned long *lenp,
return 0;
}
- debug("%s: failed: addr=%x\n", __func__, paddr);
+ debug("%s: failed: addr=" PRIpa "\n", __func__, paddr);
return -ENOSYS;
}
diff --git a/cmd/bootm.c b/cmd/bootm.c
index 48738ac..5493c5d 100644
--- a/cmd/bootm.c
+++ b/cmd/bootm.c
@@ -566,8 +566,8 @@ static int bootz_start(cmd_tbl_t *cmdtp, int flag, int argc,
load_addr);
} else {
images->ep = simple_strtoul(argv[0], NULL, 16);
- debug("* kernel: cmdline image address = 0x%08lx\n",
- images->ep);
+ debug("* kernel: cmdline image address = 0x" PRIpa "\n",
+ images->ep);
}
ret = bootz_setup(images->ep, &zi_start, &zi_end);
diff --git a/cmd/lzmadec.c b/cmd/lzmadec.c
index 1ad9ed6..8e370ef 100644
--- a/cmd/lzmadec.c
+++ b/cmd/lzmadec.c
@@ -20,7 +20,7 @@
static int do_lzmadec(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
{
unsigned long src, dst;
- unsigned long src_len = ~0UL, dst_len = ~0UL;
+ SizeT src_len = ~0UL, dst_len = ~0UL;
int ret;
switch (argc) {
@@ -40,7 +40,8 @@ static int do_lzmadec(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
if (ret != SZ_OK)
return 1;
- printf("Uncompressed size: %ld = 0x%lX\n", src_len, src_len);
+ printf("Uncompressed size: %ld = 0x%lX\n",
+ (ulong)src_len, (ulong)src_len);
setenv_hex("filesize", src_len);
return 0;
diff --git a/disk/part_efi.c b/disk/part_efi.c
index e1b58c5..a59527b 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -10,7 +10,6 @@
* when CONFIG_SYS_64BIT_LBA is not defined, lbaint_t is 32 bits; this
* limits the maximum size of addressable storage to < 2 Terra Bytes
*/
-#include <asm/unaligned.h>
#include <common.h>
#include <command.h>
#include <ide.h>
@@ -19,6 +18,7 @@
#include <memalign.h>
#include <part_efi.h>
#include <linux/ctype.h>
+#include <asm/unaligned.h>
DECLARE_GLOBAL_DATA_PTR;
diff --git a/include/image.h b/include/image.h
index 299d6d2..d77f46c 100644
--- a/include/image.h
+++ b/include/image.h
@@ -38,6 +38,12 @@ struct lmb;
#include <lmb.h>
#include <asm/u-boot.h>
#include <command.h>
+#include <linux/types.h>
+#if defined(CONFIG_PHYS_64BIT) || (BITS_PER_LONG > 32)
+#define PRIpa "0x%08llx"
+#else
+#define PRIpa "0x%08lx"
+#endif
/* Take notice of the 'ignore' property for hashes */
#define IMAGE_ENABLE_IGNORE 1
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 02b4105..ddc5541 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -13,6 +13,7 @@
* SPDX-License-Identifier: LGPL-2.1+
*/
+# include <common.h>
#include <errno.h>
#include <malloc.h>
@@ -29,7 +30,6 @@
# endif
# endif
#else /* U-Boot build */
-# include <common.h>
# include <linux/string.h>
# include <linux/ctype.h>
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses
2016-02-12 20:59 [U-Boot] [RFC PATCH v4 0/3] Enable FIT image to be loaded beyond 32-bit space York Sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host York Sun
@ 2016-02-12 20:59 ` York Sun
2016-02-16 16:01 ` Simon Glass
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image York Sun
2 siblings, 1 reply; 13+ messages in thread
From: York Sun @ 2016-02-12 20:59 UTC (permalink / raw)
To: u-boot
When dealing with image addresses, ulong has been used. Some files
are used by both host and target. It is OK for the target, but not
always enough for host tools including mkimage. This patch replaces
"ulong" with "phys_addr_t" to make sure addresses are correct for
both the target and the host.
This issue was found on 32-bit host when compiling for 64-bit target
to support images with address higher than 32-bit space.
Signed-off-by: York Sun <york.sun@nxp.com>
---
Changes in v4:
New patch, separated from fixing FIT image.
Changes in v3: None
Changes in v2: None
arch/powerpc/lib/bootm.c | 4 ++--
cmd/ximg.c | 9 +++++----
common/bootm.c | 21 +++++++++++----------
common/bootm_os.c | 14 ++++++++------
common/image-android.c | 6 +++---
common/image-fdt.c | 16 ++++++++--------
common/image-fit.c | 27 ++++++++++++++-------------
common/image.c | 17 ++++++++++-------
include/bootm.h | 6 +++---
include/image.h | 30 ++++++++++++++++++------------
tools/default_image.c | 2 +-
11 files changed, 83 insertions(+), 69 deletions(-)
diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
index ef15e7a..794382a 100644
--- a/arch/powerpc/lib/bootm.c
+++ b/arch/powerpc/lib/bootm.c
@@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
#endif
kernel = (void (*)(bd_t *, ulong, ulong, ulong,
- ulong, ulong, ulong))images->ep;
+ ulong, ulong, ulong))(uintptr_t)images->ep;
debug ("## Transferring control to Linux (at address %08lx) ...\n",
(ulong)kernel);
@@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
WATCHDOG_RESET();
((void (*)(void *, ulong, ulong, ulong,
- ulong, ulong, ulong))images->ep)(images->ft_addr,
+ ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
}
#endif
diff --git a/cmd/ximg.c b/cmd/ximg.c
index d033c15..70d6d14 100644
--- a/cmd/ximg.c
+++ b/cmd/ximg.c
@@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
{
ulong addr = load_addr;
ulong dest = 0;
- ulong data, len;
+ phys_addr_t data;
+ ulong len;
int verify;
int part = 0;
#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
@@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
return 1;
}
- data = (ulong)fit_data;
+ data = (phys_addr_t)(uintptr_t)fit_data;
len = (ulong)fit_len;
break;
#endif
@@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
}
#else /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
printf(" Loading part %d ... ", part);
- memmove((char *) dest, (char *)data, len);
+ memmove((char *)dest, (char *)(uintptr_t)data, len);
#endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
break;
#ifdef CONFIG_GZIP
case IH_COMP_GZIP:
printf(" Uncompressing part %d ... ", part);
if (gunzip((void *) dest, unc_len,
- (uchar *) data, &len) != 0) {
+ (uchar *)(uintptr_t)data, &len) != 0) {
puts("GUNZIP ERROR - image not loaded\n");
return 1;
}
diff --git a/common/bootm.c b/common/bootm.c
index 99d574d..785858c 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -43,7 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[], bootm_headers_t *images,
- ulong *os_data, ulong *os_len);
+ phys_addr_t *os_data, ulong *os_len);
#ifdef CONFIG_LMB
static void boot_start_lmb(bootm_headers_t *images)
@@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size,
return BOOTM_ERR_RESET;
}
-int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
- void *load_buf, void *image_buf, ulong image_len,
- uint unc_len, ulong *load_end)
+int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
+ int type, void *load_buf, void *image_buf,
+ ulong image_len, uint unc_len, ulong *load_end)
{
int ret = 0;
@@ -767,7 +767,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
/**
* boot_get_kernel - find kernel image
- * @os_data: pointer to a ulong variable, will hold os data start address
+ * @os_data: pointer to a phys_addr_t variable, will hold os data start address
* @os_len: pointer to a ulong variable, will hold os data length
*
* boot_get_kernel() tries to find a kernel image, verifies its integrity
@@ -779,7 +779,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
*/
static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[], bootm_headers_t *images,
- ulong *os_data, ulong *os_len)
+ phys_addr_t *os_data, ulong *os_len)
{
#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
image_header_t *hdr;
@@ -879,7 +879,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
return NULL;
}
- debug(" kernel data at 0x%08lx, len = 0x%08lx (%ld)\n",
+ debug(" kernel data at " PRIpa ", len = 0x%08lx (%ld)\n",
*os_data, *os_len, *os_len);
return buf;
@@ -894,7 +894,8 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
static int bootm_host_load_image(const void *fit, int req_image_type)
{
const char *fit_uname_config = NULL;
- ulong data, len;
+ phys_addr_t *data = NULL;
+ ulong len;
bootm_headers_t images;
int noffset;
ulong load_end;
@@ -908,7 +909,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
noffset = fit_image_load(&images, (ulong)fit,
NULL, &fit_uname_config,
IH_ARCH_DEFAULT, req_image_type, -1,
- FIT_LOAD_IGNORED, &data, &len);
+ FIT_LOAD_IGNORED, data, &len);
if (noffset < 0)
return noffset;
if (fit_image_get_type(fit, noffset, &image_type)) {
@@ -923,7 +924,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
/* Allow the image to expand by a factor of 4, should be safe */
load_buf = malloc((1 << 20) + len * 4);
- ret = bootm_decomp_image(imape_comp, 0, data, image_type, load_buf,
+ ret = bootm_decomp_image(imape_comp, 0, *data, image_type, load_buf,
(void *)data, len, CONFIG_SYS_BOOTM_LEN,
&load_end);
free(load_buf);
diff --git a/common/bootm_os.c b/common/bootm_os.c
index cb83f4a..74276f6 100644
--- a/common/bootm_os.c
+++ b/common/bootm_os.c
@@ -26,7 +26,7 @@ static int do_bootm_standalone(int flag, int argc, char * const argv[],
setenv_hex("filesize", images->os.image_len);
return 0;
}
- appl = (int (*)(int, char * const []))images->ep;
+ appl = (int (*)(int, char * const []))(uintptr_t)images->ep;
appl(argc, argv);
return 0;
}
@@ -55,7 +55,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
{
void (*loader)(bd_t *, image_header_t *, char *, char *);
image_header_t *os_hdr, *hdr;
- ulong kernel_data, kernel_len;
+ phys_addr_t kernel_data;
+ ulong kernel_len;
char *consdev;
char *cmdline;
@@ -113,7 +114,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
cmdline = "";
}
- loader = (void (*)(bd_t *, image_header_t *, char *, char *))images->ep;
+ loader = (void (*)(bd_t *, image_header_t *, char *, char *))
+ (uintptr_t)images->ep;
printf("## Transferring control to NetBSD stage-2 loader (at address %08lx) ...\n",
(ulong)loader);
@@ -171,7 +173,7 @@ static int do_bootm_rtems(int flag, int argc, char * const argv[],
}
#endif
- entry_point = (void (*)(bd_t *))images->ep;
+ entry_point = (void (*)(bd_t *))(uintptr_t)images->ep;
printf("## Transferring control to RTEMS (at address %08lx) ...\n",
(ulong)entry_point);
@@ -252,7 +254,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[],
}
}
- entry_point = (void (*)(void))images->ep;
+ entry_point = (void (*)(void))(uintptr_t)images->ep;
printf("## Transferring control to Plan 9 (at address %08lx) ...\n",
(ulong)entry_point);
@@ -364,7 +366,7 @@ static int do_bootm_qnxelf(int flag, int argc, char * const argv[],
}
#endif
- sprintf(str, "%lx", images->ep); /* write entry-point into string */
+ sprintf(str, PRIpa, images->ep); /* write entry-point into string */
local_args[0] = argv[0];
local_args[1] = str; /* and provide it via the arguments */
do_bootelf(NULL, 0, 2, local_args);
diff --git a/common/image-android.c b/common/image-android.c
index b6a94b3..7c574f8 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -38,7 +38,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
* @hdr: Pointer to image header, which is at the start
* of the image.
* @verify: Checksum verification flag. Currently unimplemented.
- * @os_data: Pointer to a ulong variable, will hold os data start
+ * @os_data: Pointer to a phys_addr_t variable, will hold os data start
* address.
* @os_len: Pointer to a ulong variable, will hold os data length.
*
@@ -49,7 +49,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
* otherwise on failure.
*/
int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
- ulong *os_data, ulong *os_len)
+ phys_addr_t *os_data, ulong *os_len)
{
u32 kernel_addr = android_image_get_kernel_addr(hdr);
@@ -93,7 +93,7 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
setenv("bootargs", newbootargs);
if (os_data) {
- *os_data = (ulong)hdr;
+ *os_data = (phys_addr_t)hdr;
*os_data += hdr->page_size;
}
if (os_len)
diff --git a/common/image-fdt.c b/common/image-fdt.c
index 5e4e5bd..bb637d7 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -223,11 +223,14 @@ error:
int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
bootm_headers_t *images, char **of_flat_tree, ulong *of_size)
{
+ phys_addr_t load;
#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
const image_header_t *fdt_hdr;
- ulong load, load_end;
+ phys_addr_t load_end;
ulong image_start, image_data, image_end;
#endif
+ phys_addr_t fdt_data;
+ ulong fdt_len;
ulong fdt_addr;
char *fdt_blob = NULL;
void *buf;
@@ -236,6 +239,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
const char *fit_uname_fdt = NULL;
ulong default_addr;
int fdt_noffset;
+ ulong len;
#endif
const char *select = NULL;
int ok_no_fdt = 0;
@@ -335,10 +339,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
goto error;
}
- debug(" Loading FDT from 0x%08lx to 0x%08lx\n",
+ debug(" Loading FDT from 0x%08lx to " PRIpa "\n",
image_data, load);
- memmove((void *)load,
+ memmove((void *)(uintptr_t)load,
(void *)image_data,
image_get_data_size(fdt_hdr));
@@ -354,8 +358,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
#if defined(CONFIG_FIT)
/* check FDT blob vs FIT blob */
if (fit_check_format(buf)) {
- ulong load, len;
-
fdt_noffset = fit_image_load(images,
fdt_addr, &fit_uname_fdt,
&fit_uname_config,
@@ -389,8 +391,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
} else if (images->legacy_hdr_valid &&
image_check_type(&images->legacy_hdr_os_copy,
IH_TYPE_MULTI)) {
- ulong fdt_data, fdt_len;
-
/*
* Now check if we have a legacy multi-component image,
* get second entry data start address and len.
@@ -401,7 +401,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
image_multi_getimg(images->legacy_hdr_os, 2, &fdt_data,
&fdt_len);
if (fdt_len) {
- fdt_blob = (char *)fdt_data;
+ fdt_blob = (char *)(uintptr_t)fdt_data;
printf(" Booting using the fdt at 0x%p\n", fdt_blob);
if (fdt_check_header(fdt_blob) != 0) {
diff --git a/common/image-fit.c b/common/image-fit.c
index c531ee7..bfa76a2 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -358,7 +358,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
char *desc;
uint8_t type, arch, os, comp;
size_t size;
- ulong load, entry;
+ phys_addr_t load, entry;
const void *data;
int noffset;
int ndepth;
@@ -428,7 +428,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
if (ret)
printf("unavailable\n");
else
- printf("0x%08lx\n", load);
+ printf(PRIpa "\n", load);
}
if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
@@ -438,7 +438,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
if (ret)
printf("unavailable\n");
else
- printf("0x%08lx\n", entry);
+ printf(PRIpa "\n", entry);
}
/* Process all hash subnodes of the component image node */
@@ -679,7 +679,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
* fit_image_get_load() - get load addr property for given component image node
* @fit: pointer to the FIT format image header
* @noffset: component image node offset
- * @load: pointer to the uint32_t, will hold load address
+ * @load: pointer to the phys_addr_t, will hold load address
*
* fit_image_get_load() finds load address property in a given component
* image node. If the property is found, its value is returned to the caller.
@@ -688,7 +688,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
* 0, on success
* -1, on failure
*/
-int fit_image_get_load(const void *fit, int noffset, ulong *load)
+int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
{
int len;
const uint32_t *data;
@@ -707,7 +707,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
* fit_image_get_entry() - get entry point address property
* @fit: pointer to the FIT format image header
* @noffset: component image node offset
- * @entry: pointer to the uint32_t, will hold entry point address
+ * @entry: pointer to the phys_addr_t, will hold entry point address
*
* This gets the entry point address property for a given component image
* node.
@@ -720,7 +720,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
* 0, on success
* -1, on failure
*/
-int fit_image_get_entry(const void *fit, int noffset, ulong *entry)
+int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
{
int len;
const uint32_t *data;
@@ -1556,7 +1556,7 @@ static const char *fit_get_image_type_property(int type)
int fit_image_load(bootm_headers_t *images, ulong addr,
const char **fit_unamep, const char **fit_uname_configp,
int arch, int image_type, int bootstage_id,
- enum fit_load_op load_op, ulong *datap, ulong *lenp)
+ enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp)
{
int cfg_noffset, noffset;
const char *fit_uname;
@@ -1565,7 +1565,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
const void *buf;
size_t size;
int type_ok, os_ok;
- ulong load, data, len;
+ phys_addr_t load;
+ ulong data, len;
uint8_t os;
const char *prop_name;
int ret;
@@ -1721,7 +1722,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
}
} else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
ulong image_start, image_end;
- ulong load_end;
+ phys_addr_t load_end;
void *dst;
/*
@@ -1738,8 +1739,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
return -EXDEV;
}
- printf(" Loading %s from 0x%08lx to 0x%08lx\n",
- prop_name, data, load);
+ printf(" Loading %s from 0x%08lx to %08llx\n",
+ prop_name, data, (uint64_t)load);
dst = map_sysmem(load, len);
memmove(dst, buf, len);
@@ -1758,7 +1759,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
}
int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
- ulong *setup_start, ulong *setup_len)
+ phys_addr_t *setup_start, ulong *setup_len)
{
int noffset;
ulong addr;
diff --git a/common/image.c b/common/image.c
index f4a1dc8..b54de68 100644
--- a/common/image.c
+++ b/common/image.c
@@ -232,7 +232,7 @@ ulong image_multi_count(const image_header_t *hdr)
* image_multi_getimg - get component data address and size
* @hdr: pointer to the header of the multi component image
* @idx: index of the requested component
- * @data: pointer to a ulong variable, will hold component data address
+ * @data: pointer to a phys_addr_t variable, will hold component data address
* @len: pointer to a ulong variable, will hold component size
*
* image_multi_getimg() returns size and data address for the requested
@@ -246,7 +246,7 @@ ulong image_multi_count(const image_header_t *hdr)
* 0 in data and len, if idx is out of range
*/
void image_multi_getimg(const image_header_t *hdr, ulong idx,
- ulong *data, ulong *len)
+ phys_addr_t *data, ulong *len)
{
int i;
uint32_t *size;
@@ -326,7 +326,8 @@ void image_print_contents(const void *ptr)
if (image_check_type(hdr, IH_TYPE_MULTI) ||
image_check_type(hdr, IH_TYPE_SCRIPT)) {
int i;
- ulong data, len;
+ phys_addr_t data;
+ ulong len;
ulong count = image_multi_count(hdr);
printf("%sContents:\n", p);
@@ -342,7 +343,7 @@ void image_print_contents(const void *ptr)
* if planning to do something with
* multiple files
*/
- printf("%s Offset = 0x%08lx\n", p, data);
+ printf("%s Offset = " PRIpa, p, data);
}
}
}
@@ -890,7 +891,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
uint8_t arch, ulong *rd_start, ulong *rd_end)
{
ulong rd_addr, rd_load;
- ulong rd_data, rd_len;
+ phys_addr_t rd_data;
+ ulong rd_len;
#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
const image_header_t *rd_hdr;
#endif
@@ -1177,7 +1179,7 @@ error:
#endif /* CONFIG_SYS_BOOT_RAMDISK_HIGH */
int boot_get_setup(bootm_headers_t *images, uint8_t arch,
- ulong *setup_start, ulong *setup_len)
+ phys_addr_t *setup_start, ulong *setup_len)
{
#if defined(CONFIG_FIT)
return boot_get_setup_fit(images, arch, setup_start, setup_len);
@@ -1199,7 +1201,8 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
* These two variables are requirements for fit_image_load, but
* their values are not used
*/
- ulong img_data, img_len;
+ phys_addr_t img_data;
+ ulong img_len;
void *buf;
int loadables_index;
int conf_noffset;
diff --git a/include/bootm.h b/include/bootm.h
index 4981377..f280ace 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -69,8 +69,8 @@ void arch_preboot_os(void);
* @unc_len: Available space for decompression
* @return 0 if OK, -ve on error (BOOTM_ERR_...)
*/
-int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
- void *load_buf, void *image_buf, ulong image_len,
- uint unc_len, ulong *load_end);
+int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
+ int type, void *load_buf, void *image_buf,
+ ulong image_len, uint unc_len, ulong *load_end);
#endif
diff --git a/include/image.h b/include/image.h
index d77f46c..07e301e 100644
--- a/include/image.h
+++ b/include/image.h
@@ -33,6 +33,11 @@ struct lmb;
#define IMAGE_ENABLE_IGNORE 0
#define IMAGE_INDENT_STRING ""
+/* Be able to hold physical address */
+typedef unsigned long long phys_addr_t;
+typedef unsigned long long phys_size_t;
+#define PRIpa "0x%08llx"
+
#else
#include <lmb.h>
@@ -294,9 +299,10 @@ typedef struct image_header {
} image_header_t;
typedef struct image_info {
- ulong start, end; /* start/end of blob */
- ulong image_start, image_len; /* start of image within blob, len of image */
- ulong load; /* load addr for the image */
+ phys_addr_t start, end; /* start/end of blob */
+ phys_addr_t image_start; /* start of image within blob */
+ ulong image_len; /* len of image */
+ phys_addr_t load; /* load addr for the image */
uint8_t comp, type, os; /* compression, type of image, os type */
uint8_t arch; /* CPU architecture */
} image_info_t;
@@ -337,7 +343,7 @@ typedef struct bootm_headers {
#ifndef USE_HOSTCC
image_info_t os; /* os image info */
- ulong ep; /* entry point of OS */
+ phys_addr_t ep; /* entry point of OS */
ulong rd_start, rd_end;/* ramdisk start/end */
@@ -456,8 +462,8 @@ enum fit_load_op {
FIT_LOAD_REQUIRED, /* Must be provided */
};
-int boot_get_setup(bootm_headers_t *images, uint8_t arch, ulong *setup_start,
- ulong *setup_len);
+int boot_get_setup(bootm_headers_t *images, uint8_t arch,
+ phys_addr_t *setup_start, ulong *setup_len);
#ifndef USE_HOSTCC
/* Image format types, returned by _get_format() routine */
@@ -505,7 +511,7 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
#endif /* !USE_HOSTCC */
int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
- ulong *setup_start, ulong *setup_len);
+ phys_addr_t *setup_start, ulong *setup_len);
/**
* fit_image_load() - load an image from a FIT
@@ -540,7 +546,7 @@ int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
int fit_image_load(bootm_headers_t *images, ulong addr,
const char **fit_unamep, const char **fit_uname_configp,
int arch, int image_type, int bootstage_id,
- enum fit_load_op load_op, ulong *datap, ulong *lenp);
+ enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp);
#ifndef USE_HOSTCC
/**
@@ -709,7 +715,7 @@ static inline int image_check_os(const image_header_t *hdr, uint8_t os)
ulong image_multi_count(const image_header_t *hdr);
void image_multi_getimg(const image_header_t *hdr, ulong idx,
- ulong *data, ulong *len);
+ phys_addr_t *data, ulong *len);
void image_print_contents(const void *hdr);
@@ -851,8 +857,8 @@ int fit_image_get_os(const void *fit, int noffset, uint8_t *os);
int fit_image_get_arch(const void *fit, int noffset, uint8_t *arch);
int fit_image_get_type(const void *fit, int noffset, uint8_t *type);
int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp);
-int fit_image_get_load(const void *fit, int noffset, ulong *load);
-int fit_image_get_entry(const void *fit, int noffset, ulong *entry);
+int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load);
+int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry);
int fit_image_get_data(const void *fit, int noffset,
const void **data, size_t *size);
@@ -1128,7 +1134,7 @@ static inline int fit_image_check_target_arch(const void *fdt, int node)
struct andr_img_hdr;
int android_image_check_header(const struct andr_img_hdr *hdr);
int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
- ulong *os_data, ulong *os_len);
+ phys_addr_t *os_data, ulong *os_len);
int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
ulong *rd_data, ulong *rd_len);
ulong android_image_get_end(const struct andr_img_hdr *hdr);
diff --git a/tools/default_image.c b/tools/default_image.c
index 3ed7014..ecb77f4 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -134,7 +134,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
static int image_extract_subimage(void *ptr, struct image_tool_params *params)
{
const image_header_t *hdr = (const image_header_t *)ptr;
- ulong file_data;
+ phys_addr_t file_data;
ulong file_len;
if (image_check_type(hdr, IH_TYPE_MULTI)) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image
2016-02-12 20:59 [U-Boot] [RFC PATCH v4 0/3] Enable FIT image to be loaded beyond 32-bit space York Sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host York Sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses York Sun
@ 2016-02-12 20:59 ` York Sun
2016-02-16 16:02 ` Simon Glass
2 siblings, 1 reply; 13+ messages in thread
From: York Sun @ 2016-02-12 20:59 UTC (permalink / raw)
To: u-boot
FIT image supports more than 32 bits in addresses by using #address-cell
field. However the address length is not handled when parsing FIT images.
Signed-off-by: York Sun <york.sun@nxp.com>
---
Changes in v4:
Separate ulong to phys_addr_t change to another patch.
Changes in v3:
Define PRIpa for host and target in common/image-fit.c so printf works
properly for 32-, 64-bit targets and host tools.
Changes in v2:
Make a common function for both load and entry addresses.
Simplify calculation of addresses in a similar way as fdtdec_get_number()
fdtdec_get_number() is not used, or too many files need to be included
and/or twisted for host tool
Continue to use %08llx for print format for load and entry addresses
because %pa does not always work for host tool (mkimage)
common/image-fit.c | 54 ++++++++++++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index bfa76a2..c000475 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -433,7 +433,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
(type == IH_TYPE_RAMDISK)) {
- fit_image_get_entry(fit, image_noffset, &entry);
+ ret = fit_image_get_entry(fit, image_noffset, &entry);
printf("%s Entry Point: ", p);
if (ret)
printf("unavailable\n");
@@ -675,6 +675,34 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
return 0;
}
+static int fit_image_get_address(const void *fit, int noffset, char *name,
+ phys_addr_t *load)
+{
+ int len, cell_len;
+ const fdt32_t *cell;
+ unsigned long long load64 = 0;
+
+ cell = fdt_getprop(fit, noffset, name, &len);
+ if (cell == NULL) {
+ fit_get_debug(fit, noffset, name, len);
+ return -1;
+ }
+
+ if (len > sizeof(phys_addr_t)) {
+ printf("Unsupported %s address size\n", name);
+ return -1;
+ }
+
+ cell_len = len >> 2;
+ /* Use load64 to avoid compiling warning for 32-bit target */
+ while (cell_len--) {
+ load64 = (load64 << 32) | uimage_to_cpu(*cell);
+ cell++;
+ }
+ *load = (phys_addr_t)load64;
+
+ return 0;
+}
/**
* fit_image_get_load() - get load addr property for given component image node
* @fit: pointer to the FIT format image header
@@ -690,17 +718,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
*/
int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
{
- int len;
- const uint32_t *data;
-
- data = fdt_getprop(fit, noffset, FIT_LOAD_PROP, &len);
- if (data == NULL) {
- fit_get_debug(fit, noffset, FIT_LOAD_PROP, len);
- return -1;
- }
-
- *load = uimage_to_cpu(*data);
- return 0;
+ return fit_image_get_address(fit, noffset, FIT_LOAD_PROP, load);
}
/**
@@ -722,17 +740,7 @@ int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
*/
int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
{
- int len;
- const uint32_t *data;
-
- data = fdt_getprop(fit, noffset, FIT_ENTRY_PROP, &len);
- if (data == NULL) {
- fit_get_debug(fit, noffset, FIT_ENTRY_PROP, len);
- return -1;
- }
-
- *entry = uimage_to_cpu(*data);
- return 0;
+ return fit_image_get_address(fit, noffset, FIT_ENTRY_PROP, entry);
}
/**
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host York Sun
@ 2016-02-16 16:01 ` Simon Glass
2016-02-23 16:48 ` york sun
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2016-02-16 16:01 UTC (permalink / raw)
To: u-boot
+Masahiro
Hi York,
On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
> Fix the following compiling warning on 32-bit host
>
> disk/part_efi.c: In function 'alloc_read_gpt_entries':
> disk/part_efi.c:894:2: warning: format '%zu' expects argument of
> type 'size_t', but argument 5 has type 'long unsigned int'
> [-Wformat]
> disk/part_efi.c:907:4: warning: format '%zX' expects argument of
> type 'size_t', but argument 3 has type 'long unsigned int'
> [-Wformat]
> cmd/lzmadec.c: In function 'do_lzmadec':
> cmd/lzmadec.c:39:12: warning: passing argument 2 of
> 'lzmaBuffToBuffDecompress' from incompatible pointer type
> [enabled by default]
> include/lzma/../../lib/lzma/LzmaTools.h:17:12: note: expected
> 'SizeT *' but argument is of type 'long unsigned int *'
> lib/hashtable.c: In function 'hexport_r':
> lib/hashtable.c:605:2: warning: format '%zu' expects argument of
> type 'size_t', but argument 5 has type 'long unsigned int'
> [-Wformat]
> lib/hashtable.c:661:5: warning: format '%zu' expects argument of
> type 'size_t', but argument 2 has type 'long unsigned int'
> [-Wformat]
> lib/hashtable.c:661:5: warning: format '%zu' expects argument of
> type 'size_t', but argument 3 has type 'long unsigned int'
> [-Wformat]
> lib/hashtable.c: In function 'himport_r':
> lib/hashtable.c:793:3: warning: format '%zu' expects argument of
> type 'size_t', but argument 2 has type 'long unsigned int'
> [-Wformat]
>
> Signed-off-by: York Sun <york.sun@nxp.com>
>
> ---
>
> Changes in v4:
> New patch to fix compiling warnings for sandbox built on 32-bit host
> Still need to change CONFIG_SANDBOX_BITS_PER_LONG to 32 to avoid
> these warnings
> include/asm-generic/bitops/__fls.h:17:2: warning: left shift
> count >= width of type [enabled by default]
> include/asm-generic/bitops/__fls.h:19:3: warning: left shift
> count >= width of type [enabled by default]
> include/asm-generic/bitops/__fls.h:22:2: warning: left shift
> count >= width of type [enabled by default]
> include/asm-generic/bitops/__fls.h:26:2: warning: left shift
> count >= width of type [enabled by default]
> include/asm-generic/bitops/__fls.h:30:2: warning: left shift
> count >= width of type [enabled by default]
> include/asm-generic/bitops/__fls.h:34:2: warning: left shift
> count >= width of type [enabled by default]
> include/asm-generic/bitops/__fls.h:38:2: warning: left shift
> count >= width of type [enabled by default]
>
> Changes in v3: None
> Changes in v2: None
>
> arch/sandbox/cpu/cpu.c | 3 ++-
> arch/sandbox/include/asm/types.h | 4 ++--
> arch/sandbox/lib/bootm.c | 3 ++-
> arch/sandbox/lib/pci_io.c | 2 +-
> cmd/bootm.c | 4 ++--
> cmd/lzmadec.c | 5 +++--
> disk/part_efi.c | 2 +-
> include/image.h | 6 ++++++
> lib/hashtable.c | 2 +-
> 9 files changed, 20 insertions(+), 11 deletions(-)
I think this makes sense. But one nit - the % is not included in the
macros in inttypes,h. What do you think about doing the same with your
macro?
>
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index 196f3e1..7aad876 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -62,7 +62,8 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
> map_dev = NULL;
> if (enable_pci_map && !pci_map_physmem(paddr, &len, &map_dev, &ptr)) {
> if (plen != len) {
> - printf("%s: Warning: partial map at %x, wanted %lx, got %lx\n",
> + printf("%s: Warning: partial map at " PRIpa
> + ", wanted %lx, got %lx\n",
> __func__, paddr, len, plen);
> }
> map_len = len;
> diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
> index 42c09e2..c3bb76e 100644
> --- a/arch/sandbox/include/asm/types.h
> +++ b/arch/sandbox/include/asm/types.h
> @@ -53,8 +53,8 @@ typedef __UINT64_TYPE__ u64;
> #define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
>
> typedef unsigned long dma_addr_t;
> -typedef u32 phys_addr_t;
> -typedef u32 phys_size_t;
> +typedef unsigned long phys_addr_t;
> +typedef unsigned long phys_size_t;
>
> #endif /* __KERNEL__ */
>
> diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c
> index d49c927..9ca89b5 100644
> --- a/arch/sandbox/lib/bootm.c
> +++ b/arch/sandbox/lib/bootm.c
> @@ -54,7 +54,8 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
> {
> if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
> bootstage_mark(BOOTSTAGE_ID_RUN_OS);
> - printf("## Transferring control to Linux (at address %08lx)...\n",
> + printf("## Transferring control to Linux (at address " PRIpa
> + ")...\n",
> images->ep);
> reset_cpu(0);
> }
> diff --git a/arch/sandbox/lib/pci_io.c b/arch/sandbox/lib/pci_io.c
> index 0de124f..3ada286 100644
> --- a/arch/sandbox/lib/pci_io.c
> +++ b/arch/sandbox/lib/pci_io.c
> @@ -35,7 +35,7 @@ int pci_map_physmem(phys_addr_t paddr, unsigned long *lenp,
> return 0;
> }
>
> - debug("%s: failed: addr=%x\n", __func__, paddr);
> + debug("%s: failed: addr=" PRIpa "\n", __func__, paddr);
> return -ENOSYS;
> }
>
> diff --git a/cmd/bootm.c b/cmd/bootm.c
> index 48738ac..5493c5d 100644
> --- a/cmd/bootm.c
> +++ b/cmd/bootm.c
> @@ -566,8 +566,8 @@ static int bootz_start(cmd_tbl_t *cmdtp, int flag, int argc,
> load_addr);
> } else {
> images->ep = simple_strtoul(argv[0], NULL, 16);
> - debug("* kernel: cmdline image address = 0x%08lx\n",
> - images->ep);
> + debug("* kernel: cmdline image address = 0x" PRIpa "\n",
> + images->ep);
> }
>
> ret = bootz_setup(images->ep, &zi_start, &zi_end);
> diff --git a/cmd/lzmadec.c b/cmd/lzmadec.c
> index 1ad9ed6..8e370ef 100644
> --- a/cmd/lzmadec.c
> +++ b/cmd/lzmadec.c
> @@ -20,7 +20,7 @@
> static int do_lzmadec(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> {
> unsigned long src, dst;
> - unsigned long src_len = ~0UL, dst_len = ~0UL;
> + SizeT src_len = ~0UL, dst_len = ~0UL;
> int ret;
>
> switch (argc) {
> @@ -40,7 +40,8 @@ static int do_lzmadec(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>
> if (ret != SZ_OK)
> return 1;
> - printf("Uncompressed size: %ld = 0x%lX\n", src_len, src_len);
> + printf("Uncompressed size: %ld = 0x%lX\n",
> + (ulong)src_len, (ulong)src_len);
> setenv_hex("filesize", src_len);
>
> return 0;
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index e1b58c5..a59527b 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -10,7 +10,6 @@
> * when CONFIG_SYS_64BIT_LBA is not defined, lbaint_t is 32 bits; this
> * limits the maximum size of addressable storage to < 2 Terra Bytes
> */
> -#include <asm/unaligned.h>
> #include <common.h>
> #include <command.h>
> #include <ide.h>
> @@ -19,6 +18,7 @@
> #include <memalign.h>
> #include <part_efi.h>
> #include <linux/ctype.h>
> +#include <asm/unaligned.h>
What is this for?
>
> DECLARE_GLOBAL_DATA_PTR;
>
> diff --git a/include/image.h b/include/image.h
> index 299d6d2..d77f46c 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -38,6 +38,12 @@ struct lmb;
> #include <lmb.h>
> #include <asm/u-boot.h>
> #include <command.h>
> +#include <linux/types.h>
> +#if defined(CONFIG_PHYS_64BIT) || (BITS_PER_LONG > 32)
> +#define PRIpa "0x%08llx"
> +#else
> +#define PRIpa "0x%08lx"
Can you use "%#08lx" ?
> +#endif
>
> /* Take notice of the 'ignore' property for hashes */
> #define IMAGE_ENABLE_IGNORE 1
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index 02b4105..ddc5541 100644
> --- a/lib/hashtable.c
> +++ b/lib/hashtable.c
> @@ -13,6 +13,7 @@
> * SPDX-License-Identifier: LGPL-2.1+
> */
>
> +# include <common.h>
> #include <errno.h>
> #include <malloc.h>
>
> @@ -29,7 +30,6 @@
> # endif
> # endif
> #else /* U-Boot build */
> -# include <common.h>
> # include <linux/string.h>
> # include <linux/ctype.h>
> #endif
> --
> 1.7.9.5
>
I guess it is safe to include <common.h> here...
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses York Sun
@ 2016-02-16 16:01 ` Simon Glass
2016-02-23 20:36 ` york sun
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2016-02-16 16:01 UTC (permalink / raw)
To: u-boot
Hi York,
On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
> When dealing with image addresses, ulong has been used. Some files
> are used by both host and target. It is OK for the target, but not
> always enough for host tools including mkimage. This patch replaces
> "ulong" with "phys_addr_t" to make sure addresses are correct for
> both the target and the host.
>
> This issue was found on 32-bit host when compiling for 64-bit target
> to support images with address higher than 32-bit space.
>
> Signed-off-by: York Sun <york.sun@nxp.com>
>
> ---
>
> Changes in v4:
> New patch, separated from fixing FIT image.
>
> Changes in v3: None
> Changes in v2: None
>
> arch/powerpc/lib/bootm.c | 4 ++--
> cmd/ximg.c | 9 +++++----
> common/bootm.c | 21 +++++++++++----------
> common/bootm_os.c | 14 ++++++++------
> common/image-android.c | 6 +++---
> common/image-fdt.c | 16 ++++++++--------
> common/image-fit.c | 27 ++++++++++++++-------------
> common/image.c | 17 ++++++++++-------
> include/bootm.h | 6 +++---
> include/image.h | 30 ++++++++++++++++++------------
> tools/default_image.c | 2 +-
> 11 files changed, 83 insertions(+), 69 deletions(-)
>
> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
> index ef15e7a..794382a 100644
> --- a/arch/powerpc/lib/bootm.c
> +++ b/arch/powerpc/lib/bootm.c
> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
> #endif
>
> kernel = (void (*)(bd_t *, ulong, ulong, ulong,
> - ulong, ulong, ulong))images->ep;
> + ulong, ulong, ulong))(uintptr_t)images->ep;
> debug ("## Transferring control to Linux (at address %08lx) ...\n",
> (ulong)kernel);
>
> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
> WATCHDOG_RESET();
>
> ((void (*)(void *, ulong, ulong, ulong,
> - ulong, ulong, ulong))images->ep)(images->ft_addr,
> + ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
> }
> #endif
> diff --git a/cmd/ximg.c b/cmd/ximg.c
> index d033c15..70d6d14 100644
> --- a/cmd/ximg.c
> +++ b/cmd/ximg.c
> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
> {
> ulong addr = load_addr;
> ulong dest = 0;
> - ulong data, len;
> + phys_addr_t data;
> + ulong len;
> int verify;
> int part = 0;
> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
> return 1;
> }
>
> - data = (ulong)fit_data;
> + data = (phys_addr_t)(uintptr_t)fit_data;
> len = (ulong)fit_len;
> break;
> #endif
> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
> }
> #else /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
> printf(" Loading part %d ... ", part);
> - memmove((char *) dest, (char *)data, len);
> + memmove((char *)dest, (char *)(uintptr_t)data, len);
> #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
> break;
> #ifdef CONFIG_GZIP
> case IH_COMP_GZIP:
> printf(" Uncompressing part %d ... ", part);
> if (gunzip((void *) dest, unc_len,
> - (uchar *) data, &len) != 0) {
> + (uchar *)(uintptr_t)data, &len) != 0) {
The uintptr_t cast presumably defeats the warning. Is the intention to
convert a 64-bit value to 32-bit?
Is it true that sizeof(phys_addr_t) is always sizeof(ulong) on the target?
> puts("GUNZIP ERROR - image not loaded\n");
> return 1;
> }
> diff --git a/common/bootm.c b/common/bootm.c
> index 99d574d..785858c 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -43,7 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
>
> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[], bootm_headers_t *images,
> - ulong *os_data, ulong *os_len);
> + phys_addr_t *os_data, ulong *os_len);
>
> #ifdef CONFIG_LMB
> static void boot_start_lmb(bootm_headers_t *images)
> @@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size,
> return BOOTM_ERR_RESET;
> }
>
> -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
> - void *load_buf, void *image_buf, ulong image_len,
> - uint unc_len, ulong *load_end)
> +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
> + int type, void *load_buf, void *image_buf,
> + ulong image_len, uint unc_len, ulong *load_end)
> {
> int ret = 0;
>
> @@ -767,7 +767,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
>
> /**
> * boot_get_kernel - find kernel image
> - * @os_data: pointer to a ulong variable, will hold os data start address
> + * @os_data: pointer to a phys_addr_t variable, will hold os data start address
> * @os_len: pointer to a ulong variable, will hold os data length
> *
> * boot_get_kernel() tries to find a kernel image, verifies its integrity
> @@ -779,7 +779,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
> */
> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[], bootm_headers_t *images,
> - ulong *os_data, ulong *os_len)
> + phys_addr_t *os_data, ulong *os_len)
> {
> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> image_header_t *hdr;
> @@ -879,7 +879,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
> return NULL;
> }
>
> - debug(" kernel data at 0x%08lx, len = 0x%08lx (%ld)\n",
> + debug(" kernel data at " PRIpa ", len = 0x%08lx (%ld)\n",
> *os_data, *os_len, *os_len);
>
> return buf;
> @@ -894,7 +894,8 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
> static int bootm_host_load_image(const void *fit, int req_image_type)
> {
> const char *fit_uname_config = NULL;
> - ulong data, len;
> + phys_addr_t *data = NULL;
This looks suspicious. Why is it changing to a pointer?
> + ulong len;
> bootm_headers_t images;
> int noffset;
> ulong load_end;
> @@ -908,7 +909,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
> noffset = fit_image_load(&images, (ulong)fit,
> NULL, &fit_uname_config,
> IH_ARCH_DEFAULT, req_image_type, -1,
> - FIT_LOAD_IGNORED, &data, &len);
> + FIT_LOAD_IGNORED, data, &len);
Won't this pass a NULL pointer?
> if (noffset < 0)
> return noffset;
> if (fit_image_get_type(fit, noffset, &image_type)) {
> @@ -923,7 +924,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>
> /* Allow the image to expand by a factor of 4, should be safe */
> load_buf = malloc((1 << 20) + len * 4);
> - ret = bootm_decomp_image(imape_comp, 0, data, image_type, load_buf,
> + ret = bootm_decomp_image(imape_comp, 0, *data, image_type, load_buf,
> (void *)data, len, CONFIG_SYS_BOOTM_LEN,
> &load_end);
> free(load_buf);
> diff --git a/common/bootm_os.c b/common/bootm_os.c
> index cb83f4a..74276f6 100644
> --- a/common/bootm_os.c
> +++ b/common/bootm_os.c
> @@ -26,7 +26,7 @@ static int do_bootm_standalone(int flag, int argc, char * const argv[],
> setenv_hex("filesize", images->os.image_len);
> return 0;
> }
> - appl = (int (*)(int, char * const []))images->ep;
> + appl = (int (*)(int, char * const []))(uintptr_t)images->ep;
> appl(argc, argv);
> return 0;
> }
> @@ -55,7 +55,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
> {
> void (*loader)(bd_t *, image_header_t *, char *, char *);
> image_header_t *os_hdr, *hdr;
> - ulong kernel_data, kernel_len;
> + phys_addr_t kernel_data;
> + ulong kernel_len;
> char *consdev;
> char *cmdline;
>
> @@ -113,7 +114,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
> cmdline = "";
> }
>
> - loader = (void (*)(bd_t *, image_header_t *, char *, char *))images->ep;
> + loader = (void (*)(bd_t *, image_header_t *, char *, char *))
> + (uintptr_t)images->ep;
>
> printf("## Transferring control to NetBSD stage-2 loader (at address %08lx) ...\n",
> (ulong)loader);
> @@ -171,7 +173,7 @@ static int do_bootm_rtems(int flag, int argc, char * const argv[],
> }
> #endif
>
> - entry_point = (void (*)(bd_t *))images->ep;
> + entry_point = (void (*)(bd_t *))(uintptr_t)images->ep;
>
> printf("## Transferring control to RTEMS (at address %08lx) ...\n",
> (ulong)entry_point);
> @@ -252,7 +254,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[],
> }
> }
>
> - entry_point = (void (*)(void))images->ep;
> + entry_point = (void (*)(void))(uintptr_t)images->ep;
>
> printf("## Transferring control to Plan 9 (at address %08lx) ...\n",
> (ulong)entry_point);
> @@ -364,7 +366,7 @@ static int do_bootm_qnxelf(int flag, int argc, char * const argv[],
> }
> #endif
>
> - sprintf(str, "%lx", images->ep); /* write entry-point into string */
> + sprintf(str, PRIpa, images->ep); /* write entry-point into string */
> local_args[0] = argv[0];
> local_args[1] = str; /* and provide it via the arguments */
> do_bootelf(NULL, 0, 2, local_args);
> diff --git a/common/image-android.c b/common/image-android.c
> index b6a94b3..7c574f8 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -38,7 +38,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
> * @hdr: Pointer to image header, which is at the start
> * of the image.
> * @verify: Checksum verification flag. Currently unimplemented.
> - * @os_data: Pointer to a ulong variable, will hold os data start
> + * @os_data: Pointer to a phys_addr_t variable, will hold os data start
> * address.
> * @os_len: Pointer to a ulong variable, will hold os data length.
> *
> @@ -49,7 +49,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
> * otherwise on failure.
> */
> int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
> - ulong *os_data, ulong *os_len)
> + phys_addr_t *os_data, ulong *os_len)
> {
> u32 kernel_addr = android_image_get_kernel_addr(hdr);
>
> @@ -93,7 +93,7 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
> setenv("bootargs", newbootargs);
>
> if (os_data) {
> - *os_data = (ulong)hdr;
> + *os_data = (phys_addr_t)hdr;
> *os_data += hdr->page_size;
> }
> if (os_len)
> diff --git a/common/image-fdt.c b/common/image-fdt.c
> index 5e4e5bd..bb637d7 100644
> --- a/common/image-fdt.c
> +++ b/common/image-fdt.c
> @@ -223,11 +223,14 @@ error:
> int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> bootm_headers_t *images, char **of_flat_tree, ulong *of_size)
> {
> + phys_addr_t load;
> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> const image_header_t *fdt_hdr;
> - ulong load, load_end;
> + phys_addr_t load_end;
> ulong image_start, image_data, image_end;
> #endif
> + phys_addr_t fdt_data;
> + ulong fdt_len;
> ulong fdt_addr;
> char *fdt_blob = NULL;
> void *buf;
> @@ -236,6 +239,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> const char *fit_uname_fdt = NULL;
> ulong default_addr;
> int fdt_noffset;
> + ulong len;
> #endif
> const char *select = NULL;
> int ok_no_fdt = 0;
> @@ -335,10 +339,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> goto error;
> }
>
> - debug(" Loading FDT from 0x%08lx to 0x%08lx\n",
> + debug(" Loading FDT from 0x%08lx to " PRIpa "\n",
> image_data, load);
>
> - memmove((void *)load,
> + memmove((void *)(uintptr_t)load,
> (void *)image_data,
> image_get_data_size(fdt_hdr));
>
> @@ -354,8 +358,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> #if defined(CONFIG_FIT)
> /* check FDT blob vs FIT blob */
> if (fit_check_format(buf)) {
> - ulong load, len;
> -
> fdt_noffset = fit_image_load(images,
> fdt_addr, &fit_uname_fdt,
> &fit_uname_config,
> @@ -389,8 +391,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> } else if (images->legacy_hdr_valid &&
> image_check_type(&images->legacy_hdr_os_copy,
> IH_TYPE_MULTI)) {
> - ulong fdt_data, fdt_len;
> -
> /*
> * Now check if we have a legacy multi-component image,
> * get second entry data start address and len.
> @@ -401,7 +401,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> image_multi_getimg(images->legacy_hdr_os, 2, &fdt_data,
> &fdt_len);
> if (fdt_len) {
> - fdt_blob = (char *)fdt_data;
> + fdt_blob = (char *)(uintptr_t)fdt_data;
> printf(" Booting using the fdt at 0x%p\n", fdt_blob);
>
> if (fdt_check_header(fdt_blob) != 0) {
> diff --git a/common/image-fit.c b/common/image-fit.c
> index c531ee7..bfa76a2 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -358,7 +358,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
> char *desc;
> uint8_t type, arch, os, comp;
> size_t size;
> - ulong load, entry;
> + phys_addr_t load, entry;
> const void *data;
> int noffset;
> int ndepth;
> @@ -428,7 +428,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
> if (ret)
> printf("unavailable\n");
> else
> - printf("0x%08lx\n", load);
> + printf(PRIpa "\n", load);
> }
>
> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
> @@ -438,7 +438,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
> if (ret)
> printf("unavailable\n");
> else
> - printf("0x%08lx\n", entry);
> + printf(PRIpa "\n", entry);
> }
>
> /* Process all hash subnodes of the component image node */
> @@ -679,7 +679,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
> * fit_image_get_load() - get load addr property for given component image node
> * @fit: pointer to the FIT format image header
> * @noffset: component image node offset
> - * @load: pointer to the uint32_t, will hold load address
> + * @load: pointer to the phys_addr_t, will hold load address
> *
> * fit_image_get_load() finds load address property in a given component
> * image node. If the property is found, its value is returned to the caller.
> @@ -688,7 +688,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
> * 0, on success
> * -1, on failure
> */
> -int fit_image_get_load(const void *fit, int noffset, ulong *load)
> +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
> {
> int len;
> const uint32_t *data;
> @@ -707,7 +707,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
> * fit_image_get_entry() - get entry point address property
> * @fit: pointer to the FIT format image header
> * @noffset: component image node offset
> - * @entry: pointer to the uint32_t, will hold entry point address
> + * @entry: pointer to the phys_addr_t, will hold entry point address
> *
> * This gets the entry point address property for a given component image
> * node.
> @@ -720,7 +720,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
> * 0, on success
> * -1, on failure
> */
> -int fit_image_get_entry(const void *fit, int noffset, ulong *entry)
> +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
> {
> int len;
> const uint32_t *data;
> @@ -1556,7 +1556,7 @@ static const char *fit_get_image_type_property(int type)
> int fit_image_load(bootm_headers_t *images, ulong addr,
> const char **fit_unamep, const char **fit_uname_configp,
> int arch, int image_type, int bootstage_id,
> - enum fit_load_op load_op, ulong *datap, ulong *lenp)
> + enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp)
> {
> int cfg_noffset, noffset;
> const char *fit_uname;
> @@ -1565,7 +1565,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> const void *buf;
> size_t size;
> int type_ok, os_ok;
> - ulong load, data, len;
> + phys_addr_t load;
> + ulong data, len;
> uint8_t os;
> const char *prop_name;
> int ret;
> @@ -1721,7 +1722,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> }
> } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
> ulong image_start, image_end;
> - ulong load_end;
> + phys_addr_t load_end;
> void *dst;
>
> /*
> @@ -1738,8 +1739,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> return -EXDEV;
> }
>
> - printf(" Loading %s from 0x%08lx to 0x%08lx\n",
> - prop_name, data, load);
> + printf(" Loading %s from 0x%08lx to %08llx\n",
> + prop_name, data, (uint64_t)load);
Do you need to cast? Why not use your magic printf() string #define?
>
> dst = map_sysmem(load, len);
> memmove(dst, buf, len);
> @@ -1758,7 +1759,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> }
>
> int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
> - ulong *setup_start, ulong *setup_len)
> + phys_addr_t *setup_start, ulong *setup_len)
> {
> int noffset;
> ulong addr;
> diff --git a/common/image.c b/common/image.c
> index f4a1dc8..b54de68 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -232,7 +232,7 @@ ulong image_multi_count(const image_header_t *hdr)
> * image_multi_getimg - get component data address and size
> * @hdr: pointer to the header of the multi component image
> * @idx: index of the requested component
> - * @data: pointer to a ulong variable, will hold component data address
> + * @data: pointer to a phys_addr_t variable, will hold component data address
> * @len: pointer to a ulong variable, will hold component size
> *
> * image_multi_getimg() returns size and data address for the requested
> @@ -246,7 +246,7 @@ ulong image_multi_count(const image_header_t *hdr)
> * 0 in data and len, if idx is out of range
> */
> void image_multi_getimg(const image_header_t *hdr, ulong idx,
> - ulong *data, ulong *len)
> + phys_addr_t *data, ulong *len)
> {
> int i;
> uint32_t *size;
> @@ -326,7 +326,8 @@ void image_print_contents(const void *ptr)
> if (image_check_type(hdr, IH_TYPE_MULTI) ||
> image_check_type(hdr, IH_TYPE_SCRIPT)) {
> int i;
> - ulong data, len;
> + phys_addr_t data;
> + ulong len;
> ulong count = image_multi_count(hdr);
>
> printf("%sContents:\n", p);
> @@ -342,7 +343,7 @@ void image_print_contents(const void *ptr)
> * if planning to do something with
> * multiple files
> */
> - printf("%s Offset = 0x%08lx\n", p, data);
> + printf("%s Offset = " PRIpa, p, data);
> }
> }
> }
> @@ -890,7 +891,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
> uint8_t arch, ulong *rd_start, ulong *rd_end)
> {
> ulong rd_addr, rd_load;
> - ulong rd_data, rd_len;
> + phys_addr_t rd_data;
> + ulong rd_len;
> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> const image_header_t *rd_hdr;
> #endif
> @@ -1177,7 +1179,7 @@ error:
> #endif /* CONFIG_SYS_BOOT_RAMDISK_HIGH */
>
> int boot_get_setup(bootm_headers_t *images, uint8_t arch,
> - ulong *setup_start, ulong *setup_len)
> + phys_addr_t *setup_start, ulong *setup_len)
> {
> #if defined(CONFIG_FIT)
> return boot_get_setup_fit(images, arch, setup_start, setup_len);
> @@ -1199,7 +1201,8 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
> * These two variables are requirements for fit_image_load, but
> * their values are not used
> */
> - ulong img_data, img_len;
> + phys_addr_t img_data;
> + ulong img_len;
> void *buf;
> int loadables_index;
> int conf_noffset;
> diff --git a/include/bootm.h b/include/bootm.h
> index 4981377..f280ace 100644
> --- a/include/bootm.h
> +++ b/include/bootm.h
> @@ -69,8 +69,8 @@ void arch_preboot_os(void);
> * @unc_len: Available space for decompression
> * @return 0 if OK, -ve on error (BOOTM_ERR_...)
> */
> -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
> - void *load_buf, void *image_buf, ulong image_len,
> - uint unc_len, ulong *load_end);
> +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
> + int type, void *load_buf, void *image_buf,
> + ulong image_len, uint unc_len, ulong *load_end);
>
> #endif
> diff --git a/include/image.h b/include/image.h
> index d77f46c..07e301e 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -33,6 +33,11 @@ struct lmb;
> #define IMAGE_ENABLE_IGNORE 0
> #define IMAGE_INDENT_STRING ""
>
> +/* Be able to hold physical address */
> +typedef unsigned long long phys_addr_t;
> +typedef unsigned long long phys_size_t;
> +#define PRIpa "0x%08llx"
> +
> #else
>
> #include <lmb.h>
> @@ -294,9 +299,10 @@ typedef struct image_header {
> } image_header_t;
>
> typedef struct image_info {
> - ulong start, end; /* start/end of blob */
> - ulong image_start, image_len; /* start of image within blob, len of image */
> - ulong load; /* load addr for the image */
> + phys_addr_t start, end; /* start/end of blob */
> + phys_addr_t image_start; /* start of image within blob */
> + ulong image_len; /* len of image */
> + phys_addr_t load; /* load addr for the image */
> uint8_t comp, type, os; /* compression, type of image, os type */
> uint8_t arch; /* CPU architecture */
> } image_info_t;
> @@ -337,7 +343,7 @@ typedef struct bootm_headers {
>
> #ifndef USE_HOSTCC
> image_info_t os; /* os image info */
> - ulong ep; /* entry point of OS */
> + phys_addr_t ep; /* entry point of OS */
>
> ulong rd_start, rd_end;/* ramdisk start/end */
>
> @@ -456,8 +462,8 @@ enum fit_load_op {
> FIT_LOAD_REQUIRED, /* Must be provided */
> };
>
> -int boot_get_setup(bootm_headers_t *images, uint8_t arch, ulong *setup_start,
> - ulong *setup_len);
> +int boot_get_setup(bootm_headers_t *images, uint8_t arch,
> + phys_addr_t *setup_start, ulong *setup_len);
>
> #ifndef USE_HOSTCC
> /* Image format types, returned by _get_format() routine */
> @@ -505,7 +511,7 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
> #endif /* !USE_HOSTCC */
>
> int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
> - ulong *setup_start, ulong *setup_len);
> + phys_addr_t *setup_start, ulong *setup_len);
>
> /**
> * fit_image_load() - load an image from a FIT
> @@ -540,7 +546,7 @@ int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
> int fit_image_load(bootm_headers_t *images, ulong addr,
> const char **fit_unamep, const char **fit_uname_configp,
> int arch, int image_type, int bootstage_id,
> - enum fit_load_op load_op, ulong *datap, ulong *lenp);
> + enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp);
>
> #ifndef USE_HOSTCC
> /**
> @@ -709,7 +715,7 @@ static inline int image_check_os(const image_header_t *hdr, uint8_t os)
>
> ulong image_multi_count(const image_header_t *hdr);
> void image_multi_getimg(const image_header_t *hdr, ulong idx,
> - ulong *data, ulong *len);
> + phys_addr_t *data, ulong *len);
>
> void image_print_contents(const void *hdr);
>
> @@ -851,8 +857,8 @@ int fit_image_get_os(const void *fit, int noffset, uint8_t *os);
> int fit_image_get_arch(const void *fit, int noffset, uint8_t *arch);
> int fit_image_get_type(const void *fit, int noffset, uint8_t *type);
> int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp);
> -int fit_image_get_load(const void *fit, int noffset, ulong *load);
> -int fit_image_get_entry(const void *fit, int noffset, ulong *entry);
> +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load);
> +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry);
> int fit_image_get_data(const void *fit, int noffset,
> const void **data, size_t *size);
>
> @@ -1128,7 +1134,7 @@ static inline int fit_image_check_target_arch(const void *fdt, int node)
> struct andr_img_hdr;
> int android_image_check_header(const struct andr_img_hdr *hdr);
> int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
> - ulong *os_data, ulong *os_len);
> + phys_addr_t *os_data, ulong *os_len);
> int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
> ulong *rd_data, ulong *rd_len);
> ulong android_image_get_end(const struct andr_img_hdr *hdr);
> diff --git a/tools/default_image.c b/tools/default_image.c
> index 3ed7014..ecb77f4 100644
> --- a/tools/default_image.c
> +++ b/tools/default_image.c
> @@ -134,7 +134,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
> static int image_extract_subimage(void *ptr, struct image_tool_params *params)
> {
> const image_header_t *hdr = (const image_header_t *)ptr;
> - ulong file_data;
> + phys_addr_t file_data;
> ulong file_len;
>
> if (image_check_type(hdr, IH_TYPE_MULTI)) {
> --
> 1.7.9.5
>
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image York Sun
@ 2016-02-16 16:02 ` Simon Glass
2016-02-24 22:55 ` york sun
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2016-02-16 16:02 UTC (permalink / raw)
To: u-boot
Hi York,
On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
> FIT image supports more than 32 bits in addresses by using #address-cell
> field. However the address length is not handled when parsing FIT images.
>
nit: How about saying "fix this by adding support for 64-bit
addresses" or similar
> Signed-off-by: York Sun <york.sun@nxp.com>
>
> ---
>
> Changes in v4:
> Separate ulong to phys_addr_t change to another patch.
>
> Changes in v3:
> Define PRIpa for host and target in common/image-fit.c so printf works
> properly for 32-, 64-bit targets and host tools.
>
> Changes in v2:
> Make a common function for both load and entry addresses.
> Simplify calculation of addresses in a similar way as fdtdec_get_number()
> fdtdec_get_number() is not used, or too many files need to be included
> and/or twisted for host tool
> Continue to use %08llx for print format for load and entry addresses
> because %pa does not always work for host tool (mkimage)
>
> common/image-fit.c | 54 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index bfa76a2..c000475 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -433,7 +433,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>
> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
> (type == IH_TYPE_RAMDISK)) {
> - fit_image_get_entry(fit, image_noffset, &entry);
> + ret = fit_image_get_entry(fit, image_noffset, &entry);
> printf("%s Entry Point: ", p);
> if (ret)
> printf("unavailable\n");
> @@ -675,6 +675,34 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
> return 0;
> }
>
> +static int fit_image_get_address(const void *fit, int noffset, char *name,
> + phys_addr_t *load)
> +{
> + int len, cell_len;
> + const fdt32_t *cell;
> + unsigned long long load64 = 0;
> +
> + cell = fdt_getprop(fit, noffset, name, &len);
> + if (cell == NULL) {
> + fit_get_debug(fit, noffset, name, len);
> + return -1;
> + }
> +
> + if (len > sizeof(phys_addr_t)) {
> + printf("Unsupported %s address size\n", name);
> + return -1;
> + }
> +
> + cell_len = len >> 2;
> + /* Use load64 to avoid compiling warning for 32-bit target */
> + while (cell_len--) {
> + load64 = (load64 << 32) | uimage_to_cpu(*cell);
> + cell++;
> + }
> + *load = (phys_addr_t)load64;
> +
> + return 0;
> +}
> /**
> * fit_image_get_load() - get load addr property for given component image node
> * @fit: pointer to the FIT format image header
> @@ -690,17 +718,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
> */
> int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
> {
> - int len;
> - const uint32_t *data;
> -
> - data = fdt_getprop(fit, noffset, FIT_LOAD_PROP, &len);
> - if (data == NULL) {
> - fit_get_debug(fit, noffset, FIT_LOAD_PROP, len);
> - return -1;
> - }
> -
> - *load = uimage_to_cpu(*data);
> - return 0;
> + return fit_image_get_address(fit, noffset, FIT_LOAD_PROP, load);
I think it would make sense to have your new fit_image_get_address()
in one patch, and the enhancement to support more address sizes in
another.
> }
>
> /**
> @@ -722,17 +740,7 @@ int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
> */
> int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
> {
> - int len;
> - const uint32_t *data;
> -
> - data = fdt_getprop(fit, noffset, FIT_ENTRY_PROP, &len);
> - if (data == NULL) {
> - fit_get_debug(fit, noffset, FIT_ENTRY_PROP, len);
> - return -1;
> - }
> -
> - *entry = uimage_to_cpu(*data);
> - return 0;
> + return fit_image_get_address(fit, noffset, FIT_ENTRY_PROP, entry);
> }
>
> /**
> --
> 1.7.9.5
>
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host
2016-02-16 16:01 ` Simon Glass
@ 2016-02-23 16:48 ` york sun
0 siblings, 0 replies; 13+ messages in thread
From: york sun @ 2016-02-23 16:48 UTC (permalink / raw)
To: u-boot
On 02/16/2016 08:01 AM, Simon Glass wrote:
> +Masahiro
>
> Hi York,
>
> On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
>> Fix the following compiling warning on 32-bit host
>>
>> disk/part_efi.c: In function 'alloc_read_gpt_entries':
>> disk/part_efi.c:894:2: warning: format '%zu' expects argument of
>> type 'size_t', but argument 5 has type 'long unsigned int'
>> [-Wformat]
>> disk/part_efi.c:907:4: warning: format '%zX' expects argument of
>> type 'size_t', but argument 3 has type 'long unsigned int'
>> [-Wformat]
>> cmd/lzmadec.c: In function 'do_lzmadec':
>> cmd/lzmadec.c:39:12: warning: passing argument 2 of
>> 'lzmaBuffToBuffDecompress' from incompatible pointer type
>> [enabled by default]
>> include/lzma/../../lib/lzma/LzmaTools.h:17:12: note: expected
>> 'SizeT *' but argument is of type 'long unsigned int *'
>> lib/hashtable.c: In function 'hexport_r':
>> lib/hashtable.c:605:2: warning: format '%zu' expects argument of
>> type 'size_t', but argument 5 has type 'long unsigned int'
>> [-Wformat]
>> lib/hashtable.c:661:5: warning: format '%zu' expects argument of
>> type 'size_t', but argument 2 has type 'long unsigned int'
>> [-Wformat]
>> lib/hashtable.c:661:5: warning: format '%zu' expects argument of
>> type 'size_t', but argument 3 has type 'long unsigned int'
>> [-Wformat]
>> lib/hashtable.c: In function 'himport_r':
>> lib/hashtable.c:793:3: warning: format '%zu' expects argument of
>> type 'size_t', but argument 2 has type 'long unsigned int'
>> [-Wformat]
>>
>> Signed-off-by: York Sun <york.sun@nxp.com>
>>
>> ---
>>
>> Changes in v4:
>> New patch to fix compiling warnings for sandbox built on 32-bit host
>> Still need to change CONFIG_SANDBOX_BITS_PER_LONG to 32 to avoid
>> these warnings
>> include/asm-generic/bitops/__fls.h:17:2: warning: left shift
>> count >= width of type [enabled by default]
>> include/asm-generic/bitops/__fls.h:19:3: warning: left shift
>> count >= width of type [enabled by default]
>> include/asm-generic/bitops/__fls.h:22:2: warning: left shift
>> count >= width of type [enabled by default]
>> include/asm-generic/bitops/__fls.h:26:2: warning: left shift
>> count >= width of type [enabled by default]
>> include/asm-generic/bitops/__fls.h:30:2: warning: left shift
>> count >= width of type [enabled by default]
>> include/asm-generic/bitops/__fls.h:34:2: warning: left shift
>> count >= width of type [enabled by default]
>> include/asm-generic/bitops/__fls.h:38:2: warning: left shift
>> count >= width of type [enabled by default]
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> arch/sandbox/cpu/cpu.c | 3 ++-
>> arch/sandbox/include/asm/types.h | 4 ++--
>> arch/sandbox/lib/bootm.c | 3 ++-
>> arch/sandbox/lib/pci_io.c | 2 +-
>> cmd/bootm.c | 4 ++--
>> cmd/lzmadec.c | 5 +++--
>> disk/part_efi.c | 2 +-
>> include/image.h | 6 ++++++
>> lib/hashtable.c | 2 +-
>> 9 files changed, 20 insertions(+), 11 deletions(-)
>
> I think this makes sense. But one nit - the % is not included in the
> macros in inttypes,h. What do you think about doing the same with your
> macro?
Sure. I can drop the % sign in the macro.
>
>>
>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>> index 196f3e1..7aad876 100644
>> --- a/arch/sandbox/cpu/cpu.c
>> +++ b/arch/sandbox/cpu/cpu.c
>> @@ -62,7 +62,8 @@ void *map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
>> map_dev = NULL;
>> if (enable_pci_map && !pci_map_physmem(paddr, &len, &map_dev, &ptr)) {
>> if (plen != len) {
>> - printf("%s: Warning: partial map at %x, wanted %lx, got %lx\n",
>> + printf("%s: Warning: partial map at " PRIpa
>> + ", wanted %lx, got %lx\n",
>> __func__, paddr, len, plen);
>> }
>> map_len = len;
>> diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h
>> index 42c09e2..c3bb76e 100644
>> --- a/arch/sandbox/include/asm/types.h
>> +++ b/arch/sandbox/include/asm/types.h
>> @@ -53,8 +53,8 @@ typedef __UINT64_TYPE__ u64;
>> #define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG
>>
>> typedef unsigned long dma_addr_t;
>> -typedef u32 phys_addr_t;
>> -typedef u32 phys_size_t;
>> +typedef unsigned long phys_addr_t;
>> +typedef unsigned long phys_size_t;
>>
>> #endif /* __KERNEL__ */
>>
>> diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c
>> index d49c927..9ca89b5 100644
>> --- a/arch/sandbox/lib/bootm.c
>> +++ b/arch/sandbox/lib/bootm.c
>> @@ -54,7 +54,8 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>> {
>> if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
>> bootstage_mark(BOOTSTAGE_ID_RUN_OS);
>> - printf("## Transferring control to Linux (at address %08lx)...\n",
>> + printf("## Transferring control to Linux (at address " PRIpa
>> + ")...\n",
>> images->ep);
>> reset_cpu(0);
>> }
>> diff --git a/arch/sandbox/lib/pci_io.c b/arch/sandbox/lib/pci_io.c
>> index 0de124f..3ada286 100644
>> --- a/arch/sandbox/lib/pci_io.c
>> +++ b/arch/sandbox/lib/pci_io.c
>> @@ -35,7 +35,7 @@ int pci_map_physmem(phys_addr_t paddr, unsigned long *lenp,
>> return 0;
>> }
>>
>> - debug("%s: failed: addr=%x\n", __func__, paddr);
>> + debug("%s: failed: addr=" PRIpa "\n", __func__, paddr);
>> return -ENOSYS;
>> }
>>
>> diff --git a/cmd/bootm.c b/cmd/bootm.c
>> index 48738ac..5493c5d 100644
>> --- a/cmd/bootm.c
>> +++ b/cmd/bootm.c
>> @@ -566,8 +566,8 @@ static int bootz_start(cmd_tbl_t *cmdtp, int flag, int argc,
>> load_addr);
>> } else {
>> images->ep = simple_strtoul(argv[0], NULL, 16);
>> - debug("* kernel: cmdline image address = 0x%08lx\n",
>> - images->ep);
>> + debug("* kernel: cmdline image address = 0x" PRIpa "\n",
>> + images->ep);
>> }
>>
>> ret = bootz_setup(images->ep, &zi_start, &zi_end);
>> diff --git a/cmd/lzmadec.c b/cmd/lzmadec.c
>> index 1ad9ed6..8e370ef 100644
>> --- a/cmd/lzmadec.c
>> +++ b/cmd/lzmadec.c
>> @@ -20,7 +20,7 @@
>> static int do_lzmadec(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>> {
>> unsigned long src, dst;
>> - unsigned long src_len = ~0UL, dst_len = ~0UL;
>> + SizeT src_len = ~0UL, dst_len = ~0UL;
>> int ret;
>>
>> switch (argc) {
>> @@ -40,7 +40,8 @@ static int do_lzmadec(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>>
>> if (ret != SZ_OK)
>> return 1;
>> - printf("Uncompressed size: %ld = 0x%lX\n", src_len, src_len);
>> + printf("Uncompressed size: %ld = 0x%lX\n",
>> + (ulong)src_len, (ulong)src_len);
>> setenv_hex("filesize", src_len);
>>
>> return 0;
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index e1b58c5..a59527b 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -10,7 +10,6 @@
>> * when CONFIG_SYS_64BIT_LBA is not defined, lbaint_t is 32 bits; this
>> * limits the maximum size of addressable storage to < 2 Terra Bytes
>> */
>> -#include <asm/unaligned.h>
>> #include <common.h>
>> #include <command.h>
>> #include <ide.h>
>> @@ -19,6 +18,7 @@
>> #include <memalign.h>
>> #include <part_efi.h>
>> #include <linux/ctype.h>
>> +#include <asm/unaligned.h>
>
> What is this for?
Not really my concern. I simply rearrange the order of include. <common.h> needs
to be on top to activate some macros for other header files to use.
>
>>
>> DECLARE_GLOBAL_DATA_PTR;
>>
>> diff --git a/include/image.h b/include/image.h
>> index 299d6d2..d77f46c 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -38,6 +38,12 @@ struct lmb;
>> #include <lmb.h>
>> #include <asm/u-boot.h>
>> #include <command.h>
>> +#include <linux/types.h>
>> +#if defined(CONFIG_PHYS_64BIT) || (BITS_PER_LONG > 32)
>> +#define PRIpa "0x%08llx"
>> +#else
>> +#define PRIpa "0x%08lx"
>
> Can you use "%#08lx" ?
Yes.
>
>> +#endif
>>
>> /* Take notice of the 'ignore' property for hashes */
>> #define IMAGE_ENABLE_IGNORE 1
>> diff --git a/lib/hashtable.c b/lib/hashtable.c
>> index 02b4105..ddc5541 100644
>> --- a/lib/hashtable.c
>> +++ b/lib/hashtable.c
>> @@ -13,6 +13,7 @@
>> * SPDX-License-Identifier: LGPL-2.1+
>> */
>>
>> +# include <common.h>
>> #include <errno.h>
>> #include <malloc.h>
>>
>> @@ -29,7 +30,6 @@
>> # endif
>> # endif
>> #else /* U-Boot build */
>> -# include <common.h>
>> # include <linux/string.h>
>> # include <linux/ctype.h>
>> #endif
>> --
>> 1.7.9.5
>>
>
> I guess it is safe to include <common.h> here...
Same thing. I moved <common.h> up. I will drop the white space in next version
though.
York
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses
2016-02-16 16:01 ` Simon Glass
@ 2016-02-23 20:36 ` york sun
2016-02-23 23:28 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: york sun @ 2016-02-23 20:36 UTC (permalink / raw)
To: u-boot
On 02/16/2016 08:01 AM, Simon Glass wrote:
> Hi York,
>
> On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
>> When dealing with image addresses, ulong has been used. Some files
>> are used by both host and target. It is OK for the target, but not
>> always enough for host tools including mkimage. This patch replaces
>> "ulong" with "phys_addr_t" to make sure addresses are correct for
>> both the target and the host.
>>
>> This issue was found on 32-bit host when compiling for 64-bit target
>> to support images with address higher than 32-bit space.
>>
>> Signed-off-by: York Sun <york.sun@nxp.com>
>>
>> ---
>>
>> Changes in v4:
>> New patch, separated from fixing FIT image.
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> arch/powerpc/lib/bootm.c | 4 ++--
>> cmd/ximg.c | 9 +++++----
>> common/bootm.c | 21 +++++++++++----------
>> common/bootm_os.c | 14 ++++++++------
>> common/image-android.c | 6 +++---
>> common/image-fdt.c | 16 ++++++++--------
>> common/image-fit.c | 27 ++++++++++++++-------------
>> common/image.c | 17 ++++++++++-------
>> include/bootm.h | 6 +++---
>> include/image.h | 30 ++++++++++++++++++------------
>> tools/default_image.c | 2 +-
>> 11 files changed, 83 insertions(+), 69 deletions(-)
>>
>> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
>> index ef15e7a..794382a 100644
>> --- a/arch/powerpc/lib/bootm.c
>> +++ b/arch/powerpc/lib/bootm.c
>> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
>> #endif
>>
>> kernel = (void (*)(bd_t *, ulong, ulong, ulong,
>> - ulong, ulong, ulong))images->ep;
>> + ulong, ulong, ulong))(uintptr_t)images->ep;
>> debug ("## Transferring control to Linux (at address %08lx) ...\n",
>> (ulong)kernel);
>>
>> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
>> WATCHDOG_RESET();
>>
>> ((void (*)(void *, ulong, ulong, ulong,
>> - ulong, ulong, ulong))images->ep)(images->ft_addr,
>> + ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
>> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
>> }
>> #endif
>> diff --git a/cmd/ximg.c b/cmd/ximg.c
>> index d033c15..70d6d14 100644
>> --- a/cmd/ximg.c
>> +++ b/cmd/ximg.c
>> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>> {
>> ulong addr = load_addr;
>> ulong dest = 0;
>> - ulong data, len;
>> + phys_addr_t data;
>> + ulong len;
>> int verify;
>> int part = 0;
>> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>> return 1;
>> }
>>
>> - data = (ulong)fit_data;
>> + data = (phys_addr_t)(uintptr_t)fit_data;
>> len = (ulong)fit_len;
>> break;
>> #endif
>> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>> }
>> #else /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
>> printf(" Loading part %d ... ", part);
>> - memmove((char *) dest, (char *)data, len);
>> + memmove((char *)dest, (char *)(uintptr_t)data, len);
>> #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
>> break;
>> #ifdef CONFIG_GZIP
>> case IH_COMP_GZIP:
>> printf(" Uncompressing part %d ... ", part);
>> if (gunzip((void *) dest, unc_len,
>> - (uchar *) data, &len) != 0) {
>> + (uchar *)(uintptr_t)data, &len) != 0) {
>
> The uintptr_t cast presumably defeats the warning. Is the intention to
> convert a 64-bit value to 32-bit?
Yes. Before this patch, all these addresses are ulong. It is enough to hold the
addresses _used_ for images on 32- and 64-bit targets. Please note, the key here
is the address used. On system with 36 or more bits for physical addresses, the
phys_addr_t is "unsigned long long". If the core is in 32-bit, the images are
actually put in 32-bit space. So it is safe to case the 64-bit phys_addr_t to a
32-bit pointer.
The idea behind this change is to make the host tool (such as mkimge) to support
64-bit address, even on a 32-bit host. My solution is to always use 64-bit
variable on host. I didn't find a better way to deal with this issue.
>
> Is it true that sizeof(phys_addr_t) is always sizeof(ulong) on the target?
No. On some arch (eg arm, mips, powerpc), phys_addr_t can be either "unsigned
long long", or "unsigned long".
>
>> puts("GUNZIP ERROR - image not loaded\n");
>> return 1;
>> }
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 99d574d..785858c 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -43,7 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>> char * const argv[], bootm_headers_t *images,
>> - ulong *os_data, ulong *os_len);
>> + phys_addr_t *os_data, ulong *os_len);
>>
>> #ifdef CONFIG_LMB
>> static void boot_start_lmb(bootm_headers_t *images)
>> @@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size,
>> return BOOTM_ERR_RESET;
>> }
>>
>> -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
>> - void *load_buf, void *image_buf, ulong image_len,
>> - uint unc_len, ulong *load_end)
>> +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
>> + int type, void *load_buf, void *image_buf,
>> + ulong image_len, uint unc_len, ulong *load_end)
>> {
>> int ret = 0;
>>
>> @@ -767,7 +767,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
>>
>> /**
>> * boot_get_kernel - find kernel image
>> - * @os_data: pointer to a ulong variable, will hold os data start address
>> + * @os_data: pointer to a phys_addr_t variable, will hold os data start address
>> * @os_len: pointer to a ulong variable, will hold os data length
>> *
>> * boot_get_kernel() tries to find a kernel image, verifies its integrity
>> @@ -779,7 +779,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
>> */
>> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>> char * const argv[], bootm_headers_t *images,
>> - ulong *os_data, ulong *os_len)
>> + phys_addr_t *os_data, ulong *os_len)
>> {
>> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>> image_header_t *hdr;
>> @@ -879,7 +879,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>> return NULL;
>> }
>>
>> - debug(" kernel data at 0x%08lx, len = 0x%08lx (%ld)\n",
>> + debug(" kernel data at " PRIpa ", len = 0x%08lx (%ld)\n",
>> *os_data, *os_len, *os_len);
>>
>> return buf;
>> @@ -894,7 +894,8 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
>> static int bootm_host_load_image(const void *fit, int req_image_type)
>> {
>> const char *fit_uname_config = NULL;
>> - ulong data, len;
>> + phys_addr_t *data = NULL;
>
> This looks suspicious. Why is it changing to a pointer?
It does look suspicious. I must carried the change from an earlier patch.
>
>> + ulong len;
>> bootm_headers_t images;
>> int noffset;
>> ulong load_end;
>> @@ -908,7 +909,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>> noffset = fit_image_load(&images, (ulong)fit,
>> NULL, &fit_uname_config,
>> IH_ARCH_DEFAULT, req_image_type, -1,
>> - FIT_LOAD_IGNORED, &data, &len);
>> + FIT_LOAD_IGNORED, data, &len);
>
> Won't this pass a NULL pointer?
This must be wrong. Let me debug it.
>
>> if (noffset < 0)
>> return noffset;
>> if (fit_image_get_type(fit, noffset, &image_type)) {
>> @@ -923,7 +924,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>>
>> /* Allow the image to expand by a factor of 4, should be safe */
>> load_buf = malloc((1 << 20) + len * 4);
>> - ret = bootm_decomp_image(imape_comp, 0, data, image_type, load_buf,
>> + ret = bootm_decomp_image(imape_comp, 0, *data, image_type, load_buf,
>> (void *)data, len, CONFIG_SYS_BOOTM_LEN,
>> &load_end);
>> free(load_buf);
>> diff --git a/common/bootm_os.c b/common/bootm_os.c
>> index cb83f4a..74276f6 100644
>> --- a/common/bootm_os.c
>> +++ b/common/bootm_os.c
>> @@ -26,7 +26,7 @@ static int do_bootm_standalone(int flag, int argc, char * const argv[],
>> setenv_hex("filesize", images->os.image_len);
>> return 0;
>> }
>> - appl = (int (*)(int, char * const []))images->ep;
>> + appl = (int (*)(int, char * const []))(uintptr_t)images->ep;
>> appl(argc, argv);
>> return 0;
>> }
>> @@ -55,7 +55,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
>> {
>> void (*loader)(bd_t *, image_header_t *, char *, char *);
>> image_header_t *os_hdr, *hdr;
>> - ulong kernel_data, kernel_len;
>> + phys_addr_t kernel_data;
>> + ulong kernel_len;
>> char *consdev;
>> char *cmdline;
>>
>> @@ -113,7 +114,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
>> cmdline = "";
>> }
>>
>> - loader = (void (*)(bd_t *, image_header_t *, char *, char *))images->ep;
>> + loader = (void (*)(bd_t *, image_header_t *, char *, char *))
>> + (uintptr_t)images->ep;
>>
>> printf("## Transferring control to NetBSD stage-2 loader (at address %08lx) ...\n",
>> (ulong)loader);
>> @@ -171,7 +173,7 @@ static int do_bootm_rtems(int flag, int argc, char * const argv[],
>> }
>> #endif
>>
>> - entry_point = (void (*)(bd_t *))images->ep;
>> + entry_point = (void (*)(bd_t *))(uintptr_t)images->ep;
>>
>> printf("## Transferring control to RTEMS (at address %08lx) ...\n",
>> (ulong)entry_point);
>> @@ -252,7 +254,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[],
>> }
>> }
>>
>> - entry_point = (void (*)(void))images->ep;
>> + entry_point = (void (*)(void))(uintptr_t)images->ep;
>>
>> printf("## Transferring control to Plan 9 (at address %08lx) ...\n",
>> (ulong)entry_point);
>> @@ -364,7 +366,7 @@ static int do_bootm_qnxelf(int flag, int argc, char * const argv[],
>> }
>> #endif
>>
>> - sprintf(str, "%lx", images->ep); /* write entry-point into string */
>> + sprintf(str, PRIpa, images->ep); /* write entry-point into string */
>> local_args[0] = argv[0];
>> local_args[1] = str; /* and provide it via the arguments */
>> do_bootelf(NULL, 0, 2, local_args);
>> diff --git a/common/image-android.c b/common/image-android.c
>> index b6a94b3..7c574f8 100644
>> --- a/common/image-android.c
>> +++ b/common/image-android.c
>> @@ -38,7 +38,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
>> * @hdr: Pointer to image header, which is at the start
>> * of the image.
>> * @verify: Checksum verification flag. Currently unimplemented.
>> - * @os_data: Pointer to a ulong variable, will hold os data start
>> + * @os_data: Pointer to a phys_addr_t variable, will hold os data start
>> * address.
>> * @os_len: Pointer to a ulong variable, will hold os data length.
>> *
>> @@ -49,7 +49,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
>> * otherwise on failure.
>> */
>> int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>> - ulong *os_data, ulong *os_len)
>> + phys_addr_t *os_data, ulong *os_len)
>> {
>> u32 kernel_addr = android_image_get_kernel_addr(hdr);
>>
>> @@ -93,7 +93,7 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>> setenv("bootargs", newbootargs);
>>
>> if (os_data) {
>> - *os_data = (ulong)hdr;
>> + *os_data = (phys_addr_t)hdr;
>> *os_data += hdr->page_size;
>> }
>> if (os_len)
>> diff --git a/common/image-fdt.c b/common/image-fdt.c
>> index 5e4e5bd..bb637d7 100644
>> --- a/common/image-fdt.c
>> +++ b/common/image-fdt.c
>> @@ -223,11 +223,14 @@ error:
>> int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>> bootm_headers_t *images, char **of_flat_tree, ulong *of_size)
>> {
>> + phys_addr_t load;
>> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>> const image_header_t *fdt_hdr;
>> - ulong load, load_end;
>> + phys_addr_t load_end;
>> ulong image_start, image_data, image_end;
>> #endif
>> + phys_addr_t fdt_data;
>> + ulong fdt_len;
>> ulong fdt_addr;
>> char *fdt_blob = NULL;
>> void *buf;
>> @@ -236,6 +239,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>> const char *fit_uname_fdt = NULL;
>> ulong default_addr;
>> int fdt_noffset;
>> + ulong len;
>> #endif
>> const char *select = NULL;
>> int ok_no_fdt = 0;
>> @@ -335,10 +339,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>> goto error;
>> }
>>
>> - debug(" Loading FDT from 0x%08lx to 0x%08lx\n",
>> + debug(" Loading FDT from 0x%08lx to " PRIpa "\n",
>> image_data, load);
>>
>> - memmove((void *)load,
>> + memmove((void *)(uintptr_t)load,
>> (void *)image_data,
>> image_get_data_size(fdt_hdr));
>>
>> @@ -354,8 +358,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>> #if defined(CONFIG_FIT)
>> /* check FDT blob vs FIT blob */
>> if (fit_check_format(buf)) {
>> - ulong load, len;
>> -
>> fdt_noffset = fit_image_load(images,
>> fdt_addr, &fit_uname_fdt,
>> &fit_uname_config,
>> @@ -389,8 +391,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>> } else if (images->legacy_hdr_valid &&
>> image_check_type(&images->legacy_hdr_os_copy,
>> IH_TYPE_MULTI)) {
>> - ulong fdt_data, fdt_len;
>> -
>> /*
>> * Now check if we have a legacy multi-component image,
>> * get second entry data start address and len.
>> @@ -401,7 +401,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>> image_multi_getimg(images->legacy_hdr_os, 2, &fdt_data,
>> &fdt_len);
>> if (fdt_len) {
>> - fdt_blob = (char *)fdt_data;
>> + fdt_blob = (char *)(uintptr_t)fdt_data;
>> printf(" Booting using the fdt at 0x%p\n", fdt_blob);
>>
>> if (fdt_check_header(fdt_blob) != 0) {
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index c531ee7..bfa76a2 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -358,7 +358,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>> char *desc;
>> uint8_t type, arch, os, comp;
>> size_t size;
>> - ulong load, entry;
>> + phys_addr_t load, entry;
>> const void *data;
>> int noffset;
>> int ndepth;
>> @@ -428,7 +428,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>> if (ret)
>> printf("unavailable\n");
>> else
>> - printf("0x%08lx\n", load);
>> + printf(PRIpa "\n", load);
>> }
>>
>> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>> @@ -438,7 +438,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>> if (ret)
>> printf("unavailable\n");
>> else
>> - printf("0x%08lx\n", entry);
>> + printf(PRIpa "\n", entry);
>> }
>>
>> /* Process all hash subnodes of the component image node */
>> @@ -679,7 +679,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>> * fit_image_get_load() - get load addr property for given component image node
>> * @fit: pointer to the FIT format image header
>> * @noffset: component image node offset
>> - * @load: pointer to the uint32_t, will hold load address
>> + * @load: pointer to the phys_addr_t, will hold load address
>> *
>> * fit_image_get_load() finds load address property in a given component
>> * image node. If the property is found, its value is returned to the caller.
>> @@ -688,7 +688,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>> * 0, on success
>> * -1, on failure
>> */
>> -int fit_image_get_load(const void *fit, int noffset, ulong *load)
>> +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>> {
>> int len;
>> const uint32_t *data;
>> @@ -707,7 +707,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>> * fit_image_get_entry() - get entry point address property
>> * @fit: pointer to the FIT format image header
>> * @noffset: component image node offset
>> - * @entry: pointer to the uint32_t, will hold entry point address
>> + * @entry: pointer to the phys_addr_t, will hold entry point address
>> *
>> * This gets the entry point address property for a given component image
>> * node.
>> @@ -720,7 +720,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>> * 0, on success
>> * -1, on failure
>> */
>> -int fit_image_get_entry(const void *fit, int noffset, ulong *entry)
>> +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
>> {
>> int len;
>> const uint32_t *data;
>> @@ -1556,7 +1556,7 @@ static const char *fit_get_image_type_property(int type)
>> int fit_image_load(bootm_headers_t *images, ulong addr,
>> const char **fit_unamep, const char **fit_uname_configp,
>> int arch, int image_type, int bootstage_id,
>> - enum fit_load_op load_op, ulong *datap, ulong *lenp)
>> + enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp)
>> {
>> int cfg_noffset, noffset;
>> const char *fit_uname;
>> @@ -1565,7 +1565,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>> const void *buf;
>> size_t size;
>> int type_ok, os_ok;
>> - ulong load, data, len;
>> + phys_addr_t load;
>> + ulong data, len;
>> uint8_t os;
>> const char *prop_name;
>> int ret;
>> @@ -1721,7 +1722,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>> }
>> } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
>> ulong image_start, image_end;
>> - ulong load_end;
>> + phys_addr_t load_end;
>> void *dst;
>>
>> /*
>> @@ -1738,8 +1739,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>> return -EXDEV;
>> }
>>
>> - printf(" Loading %s from 0x%08lx to 0x%08lx\n",
>> - prop_name, data, load);
>> + printf(" Loading %s from 0x%08lx to %08llx\n",
>> + prop_name, data, (uint64_t)load);
>
> Do you need to cast? Why not use your magic printf() string #define?
Sure. Let me respin the patch and double confirm on an ARMv8 board.
York
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses
2016-02-23 20:36 ` york sun
@ 2016-02-23 23:28 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2016-02-23 23:28 UTC (permalink / raw)
To: u-boot
Hi York,
On 23 February 2016 at 13:36, york sun <york.sun@nxp.com> wrote:
> On 02/16/2016 08:01 AM, Simon Glass wrote:
>> Hi York,
>>
>> On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
>>> When dealing with image addresses, ulong has been used. Some files
>>> are used by both host and target. It is OK for the target, but not
>>> always enough for host tools including mkimage. This patch replaces
>>> "ulong" with "phys_addr_t" to make sure addresses are correct for
>>> both the target and the host.
>>>
>>> This issue was found on 32-bit host when compiling for 64-bit target
>>> to support images with address higher than 32-bit space.
>>>
>>> Signed-off-by: York Sun <york.sun@nxp.com>
>>>
>>> ---
>>>
>>> Changes in v4:
>>> New patch, separated from fixing FIT image.
>>>
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>> arch/powerpc/lib/bootm.c | 4 ++--
>>> cmd/ximg.c | 9 +++++----
>>> common/bootm.c | 21 +++++++++++----------
>>> common/bootm_os.c | 14 ++++++++------
>>> common/image-android.c | 6 +++---
>>> common/image-fdt.c | 16 ++++++++--------
>>> common/image-fit.c | 27 ++++++++++++++-------------
>>> common/image.c | 17 ++++++++++-------
>>> include/bootm.h | 6 +++---
>>> include/image.h | 30 ++++++++++++++++++------------
>>> tools/default_image.c | 2 +-
>>> 11 files changed, 83 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
>>> index ef15e7a..794382a 100644
>>> --- a/arch/powerpc/lib/bootm.c
>>> +++ b/arch/powerpc/lib/bootm.c
>>> @@ -47,7 +47,7 @@ static void boot_jump_linux(bootm_headers_t *images)
>>> #endif
>>>
>>> kernel = (void (*)(bd_t *, ulong, ulong, ulong,
>>> - ulong, ulong, ulong))images->ep;
>>> + ulong, ulong, ulong))(uintptr_t)images->ep;
>>> debug ("## Transferring control to Linux (at address %08lx) ...\n",
>>> (ulong)kernel);
>>>
>>> @@ -335,7 +335,7 @@ void boot_jump_vxworks(bootm_headers_t *images)
>>> WATCHDOG_RESET();
>>>
>>> ((void (*)(void *, ulong, ulong, ulong,
>>> - ulong, ulong, ulong))images->ep)(images->ft_addr,
>>> + ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
>>> 0, 0, EPAPR_MAGIC, getenv_bootm_mapsize(), 0, 0);
>>> }
>>> #endif
>>> diff --git a/cmd/ximg.c b/cmd/ximg.c
>>> index d033c15..70d6d14 100644
>>> --- a/cmd/ximg.c
>>> +++ b/cmd/ximg.c
>>> @@ -33,7 +33,8 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>>> {
>>> ulong addr = load_addr;
>>> ulong dest = 0;
>>> - ulong data, len;
>>> + phys_addr_t data;
>>> + ulong len;
>>> int verify;
>>> int part = 0;
>>> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>>> @@ -173,7 +174,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>>> return 1;
>>> }
>>>
>>> - data = (ulong)fit_data;
>>> + data = (phys_addr_t)(uintptr_t)fit_data;
>>> len = (ulong)fit_len;
>>> break;
>>> #endif
>>> @@ -205,14 +206,14 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>>> }
>>> #else /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
>>> printf(" Loading part %d ... ", part);
>>> - memmove((char *) dest, (char *)data, len);
>>> + memmove((char *)dest, (char *)(uintptr_t)data, len);
>>> #endif /* CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG */
>>> break;
>>> #ifdef CONFIG_GZIP
>>> case IH_COMP_GZIP:
>>> printf(" Uncompressing part %d ... ", part);
>>> if (gunzip((void *) dest, unc_len,
>>> - (uchar *) data, &len) != 0) {
>>> + (uchar *)(uintptr_t)data, &len) != 0) {
>>
>> The uintptr_t cast presumably defeats the warning. Is the intention to
>> convert a 64-bit value to 32-bit?
>
> Yes. Before this patch, all these addresses are ulong. It is enough to hold the
> addresses _used_ for images on 32- and 64-bit targets. Please note, the key here
> is the address used. On system with 36 or more bits for physical addresses, the
> phys_addr_t is "unsigned long long". If the core is in 32-bit, the images are
> actually put in 32-bit space. So it is safe to case the 64-bit phys_addr_t to a
> 32-bit pointer.
>
> The idea behind this change is to make the host tool (such as mkimge) to support
> 64-bit address, even on a 32-bit host. My solution is to always use 64-bit
> variable on host. I didn't find a better way to deal with this issue.
>
>>
>> Is it true that sizeof(phys_addr_t) is always sizeof(ulong) on the target?
>
> No. On some arch (eg arm, mips, powerpc), phys_addr_t can be either "unsigned
> long long", or "unsigned long".
OK - can you please add a comment in the code here? I understand what
you are saying but it will not be obvious from the code.
>
>>
>>> puts("GUNZIP ERROR - image not loaded\n");
>>> return 1;
>>> }
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index 99d574d..785858c 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -43,7 +43,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>
>>> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>>> char * const argv[], bootm_headers_t *images,
>>> - ulong *os_data, ulong *os_len);
>>> + phys_addr_t *os_data, ulong *os_len);
>>>
>>> #ifdef CONFIG_LMB
>>> static void boot_start_lmb(bootm_headers_t *images)
>>> @@ -325,9 +325,9 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size,
>>> return BOOTM_ERR_RESET;
>>> }
>>>
>>> -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
>>> - void *load_buf, void *image_buf, ulong image_len,
>>> - uint unc_len, ulong *load_end)
>>> +int bootm_decomp_image(int comp, phys_addr_t load, phys_addr_t image_start,
>>> + int type, void *load_buf, void *image_buf,
>>> + ulong image_len, uint unc_len, ulong *load_end)
>>> {
>>> int ret = 0;
>>>
>>> @@ -767,7 +767,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
>>>
>>> /**
>>> * boot_get_kernel - find kernel image
>>> - * @os_data: pointer to a ulong variable, will hold os data start address
>>> + * @os_data: pointer to a phys_addr_t variable, will hold os data start address
>>> * @os_len: pointer to a ulong variable, will hold os data length
>>> *
>>> * boot_get_kernel() tries to find a kernel image, verifies its integrity
>>> @@ -779,7 +779,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int verify)
>>> */
>>> static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>>> char * const argv[], bootm_headers_t *images,
>>> - ulong *os_data, ulong *os_len)
>>> + phys_addr_t *os_data, ulong *os_len)
>>> {
>>> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>>> image_header_t *hdr;
>>> @@ -879,7 +879,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>>> return NULL;
>>> }
>>>
>>> - debug(" kernel data at 0x%08lx, len = 0x%08lx (%ld)\n",
>>> + debug(" kernel data at " PRIpa ", len = 0x%08lx (%ld)\n",
>>> *os_data, *os_len, *os_len);
>>>
>>> return buf;
>>> @@ -894,7 +894,8 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
>>> static int bootm_host_load_image(const void *fit, int req_image_type)
>>> {
>>> const char *fit_uname_config = NULL;
>>> - ulong data, len;
>>> + phys_addr_t *data = NULL;
>>
>> This looks suspicious. Why is it changing to a pointer?
>
> It does look suspicious. I must carried the change from an earlier patch.
>
>>
>>> + ulong len;
>>> bootm_headers_t images;
>>> int noffset;
>>> ulong load_end;
>>> @@ -908,7 +909,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>>> noffset = fit_image_load(&images, (ulong)fit,
>>> NULL, &fit_uname_config,
>>> IH_ARCH_DEFAULT, req_image_type, -1,
>>> - FIT_LOAD_IGNORED, &data, &len);
>>> + FIT_LOAD_IGNORED, data, &len);
>>
>> Won't this pass a NULL pointer?
>
> This must be wrong. Let me debug it.
>
>>
>>> if (noffset < 0)
>>> return noffset;
>>> if (fit_image_get_type(fit, noffset, &image_type)) {
>>> @@ -923,7 +924,7 @@ static int bootm_host_load_image(const void *fit, int req_image_type)
>>>
>>> /* Allow the image to expand by a factor of 4, should be safe */
>>> load_buf = malloc((1 << 20) + len * 4);
>>> - ret = bootm_decomp_image(imape_comp, 0, data, image_type, load_buf,
>>> + ret = bootm_decomp_image(imape_comp, 0, *data, image_type, load_buf,
>>> (void *)data, len, CONFIG_SYS_BOOTM_LEN,
>>> &load_end);
>>> free(load_buf);
>>> diff --git a/common/bootm_os.c b/common/bootm_os.c
>>> index cb83f4a..74276f6 100644
>>> --- a/common/bootm_os.c
>>> +++ b/common/bootm_os.c
>>> @@ -26,7 +26,7 @@ static int do_bootm_standalone(int flag, int argc, char * const argv[],
>>> setenv_hex("filesize", images->os.image_len);
>>> return 0;
>>> }
>>> - appl = (int (*)(int, char * const []))images->ep;
>>> + appl = (int (*)(int, char * const []))(uintptr_t)images->ep;
>>> appl(argc, argv);
>>> return 0;
>>> }
>>> @@ -55,7 +55,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
>>> {
>>> void (*loader)(bd_t *, image_header_t *, char *, char *);
>>> image_header_t *os_hdr, *hdr;
>>> - ulong kernel_data, kernel_len;
>>> + phys_addr_t kernel_data;
>>> + ulong kernel_len;
>>> char *consdev;
>>> char *cmdline;
>>>
>>> @@ -113,7 +114,8 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[],
>>> cmdline = "";
>>> }
>>>
>>> - loader = (void (*)(bd_t *, image_header_t *, char *, char *))images->ep;
>>> + loader = (void (*)(bd_t *, image_header_t *, char *, char *))
>>> + (uintptr_t)images->ep;
>>>
>>> printf("## Transferring control to NetBSD stage-2 loader (at address %08lx) ...\n",
>>> (ulong)loader);
>>> @@ -171,7 +173,7 @@ static int do_bootm_rtems(int flag, int argc, char * const argv[],
>>> }
>>> #endif
>>>
>>> - entry_point = (void (*)(bd_t *))images->ep;
>>> + entry_point = (void (*)(bd_t *))(uintptr_t)images->ep;
>>>
>>> printf("## Transferring control to RTEMS (at address %08lx) ...\n",
>>> (ulong)entry_point);
>>> @@ -252,7 +254,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[],
>>> }
>>> }
>>>
>>> - entry_point = (void (*)(void))images->ep;
>>> + entry_point = (void (*)(void))(uintptr_t)images->ep;
>>>
>>> printf("## Transferring control to Plan 9 (at address %08lx) ...\n",
>>> (ulong)entry_point);
>>> @@ -364,7 +366,7 @@ static int do_bootm_qnxelf(int flag, int argc, char * const argv[],
>>> }
>>> #endif
>>>
>>> - sprintf(str, "%lx", images->ep); /* write entry-point into string */
>>> + sprintf(str, PRIpa, images->ep); /* write entry-point into string */
>>> local_args[0] = argv[0];
>>> local_args[1] = str; /* and provide it via the arguments */
>>> do_bootelf(NULL, 0, 2, local_args);
>>> diff --git a/common/image-android.c b/common/image-android.c
>>> index b6a94b3..7c574f8 100644
>>> --- a/common/image-android.c
>>> +++ b/common/image-android.c
>>> @@ -38,7 +38,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
>>> * @hdr: Pointer to image header, which is at the start
>>> * of the image.
>>> * @verify: Checksum verification flag. Currently unimplemented.
>>> - * @os_data: Pointer to a ulong variable, will hold os data start
>>> + * @os_data: Pointer to a phys_addr_t variable, will hold os data start
>>> * address.
>>> * @os_len: Pointer to a ulong variable, will hold os data length.
>>> *
>>> @@ -49,7 +49,7 @@ static ulong android_image_get_kernel_addr(const struct andr_img_hdr *hdr)
>>> * otherwise on failure.
>>> */
>>> int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>>> - ulong *os_data, ulong *os_len)
>>> + phys_addr_t *os_data, ulong *os_len)
>>> {
>>> u32 kernel_addr = android_image_get_kernel_addr(hdr);
>>>
>>> @@ -93,7 +93,7 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>>> setenv("bootargs", newbootargs);
>>>
>>> if (os_data) {
>>> - *os_data = (ulong)hdr;
>>> + *os_data = (phys_addr_t)hdr;
>>> *os_data += hdr->page_size;
>>> }
>>> if (os_len)
>>> diff --git a/common/image-fdt.c b/common/image-fdt.c
>>> index 5e4e5bd..bb637d7 100644
>>> --- a/common/image-fdt.c
>>> +++ b/common/image-fdt.c
>>> @@ -223,11 +223,14 @@ error:
>>> int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>>> bootm_headers_t *images, char **of_flat_tree, ulong *of_size)
>>> {
>>> + phys_addr_t load;
>>> #if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>>> const image_header_t *fdt_hdr;
>>> - ulong load, load_end;
>>> + phys_addr_t load_end;
>>> ulong image_start, image_data, image_end;
>>> #endif
>>> + phys_addr_t fdt_data;
>>> + ulong fdt_len;
>>> ulong fdt_addr;
>>> char *fdt_blob = NULL;
>>> void *buf;
>>> @@ -236,6 +239,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>>> const char *fit_uname_fdt = NULL;
>>> ulong default_addr;
>>> int fdt_noffset;
>>> + ulong len;
>>> #endif
>>> const char *select = NULL;
>>> int ok_no_fdt = 0;
>>> @@ -335,10 +339,10 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>>> goto error;
>>> }
>>>
>>> - debug(" Loading FDT from 0x%08lx to 0x%08lx\n",
>>> + debug(" Loading FDT from 0x%08lx to " PRIpa "\n",
>>> image_data, load);
>>>
>>> - memmove((void *)load,
>>> + memmove((void *)(uintptr_t)load,
>>> (void *)image_data,
>>> image_get_data_size(fdt_hdr));
>>>
>>> @@ -354,8 +358,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>>> #if defined(CONFIG_FIT)
>>> /* check FDT blob vs FIT blob */
>>> if (fit_check_format(buf)) {
>>> - ulong load, len;
>>> -
>>> fdt_noffset = fit_image_load(images,
>>> fdt_addr, &fit_uname_fdt,
>>> &fit_uname_config,
>>> @@ -389,8 +391,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>>> } else if (images->legacy_hdr_valid &&
>>> image_check_type(&images->legacy_hdr_os_copy,
>>> IH_TYPE_MULTI)) {
>>> - ulong fdt_data, fdt_len;
>>> -
>>> /*
>>> * Now check if we have a legacy multi-component image,
>>> * get second entry data start address and len.
>>> @@ -401,7 +401,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>>> image_multi_getimg(images->legacy_hdr_os, 2, &fdt_data,
>>> &fdt_len);
>>> if (fdt_len) {
>>> - fdt_blob = (char *)fdt_data;
>>> + fdt_blob = (char *)(uintptr_t)fdt_data;
>>> printf(" Booting using the fdt at 0x%p\n", fdt_blob);
>>>
>>> if (fdt_check_header(fdt_blob) != 0) {
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index c531ee7..bfa76a2 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -358,7 +358,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>> char *desc;
>>> uint8_t type, arch, os, comp;
>>> size_t size;
>>> - ulong load, entry;
>>> + phys_addr_t load, entry;
>>> const void *data;
>>> int noffset;
>>> int ndepth;
>>> @@ -428,7 +428,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>> if (ret)
>>> printf("unavailable\n");
>>> else
>>> - printf("0x%08lx\n", load);
>>> + printf(PRIpa "\n", load);
>>> }
>>>
>>> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>>> @@ -438,7 +438,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>> if (ret)
>>> printf("unavailable\n");
>>> else
>>> - printf("0x%08lx\n", entry);
>>> + printf(PRIpa "\n", entry);
>>> }
>>>
>>> /* Process all hash subnodes of the component image node */
>>> @@ -679,7 +679,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>>> * fit_image_get_load() - get load addr property for given component image node
>>> * @fit: pointer to the FIT format image header
>>> * @noffset: component image node offset
>>> - * @load: pointer to the uint32_t, will hold load address
>>> + * @load: pointer to the phys_addr_t, will hold load address
>>> *
>>> * fit_image_get_load() finds load address property in a given component
>>> * image node. If the property is found, its value is returned to the caller.
>>> @@ -688,7 +688,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>>> * 0, on success
>>> * -1, on failure
>>> */
>>> -int fit_image_get_load(const void *fit, int noffset, ulong *load)
>>> +int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>>> {
>>> int len;
>>> const uint32_t *data;
>>> @@ -707,7 +707,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>>> * fit_image_get_entry() - get entry point address property
>>> * @fit: pointer to the FIT format image header
>>> * @noffset: component image node offset
>>> - * @entry: pointer to the uint32_t, will hold entry point address
>>> + * @entry: pointer to the phys_addr_t, will hold entry point address
>>> *
>>> * This gets the entry point address property for a given component image
>>> * node.
>>> @@ -720,7 +720,7 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load)
>>> * 0, on success
>>> * -1, on failure
>>> */
>>> -int fit_image_get_entry(const void *fit, int noffset, ulong *entry)
>>> +int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
>>> {
>>> int len;
>>> const uint32_t *data;
>>> @@ -1556,7 +1556,7 @@ static const char *fit_get_image_type_property(int type)
>>> int fit_image_load(bootm_headers_t *images, ulong addr,
>>> const char **fit_unamep, const char **fit_uname_configp,
>>> int arch, int image_type, int bootstage_id,
>>> - enum fit_load_op load_op, ulong *datap, ulong *lenp)
>>> + enum fit_load_op load_op, phys_addr_t *datap, ulong *lenp)
>>> {
>>> int cfg_noffset, noffset;
>>> const char *fit_uname;
>>> @@ -1565,7 +1565,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>> const void *buf;
>>> size_t size;
>>> int type_ok, os_ok;
>>> - ulong load, data, len;
>>> + phys_addr_t load;
>>> + ulong data, len;
>>> uint8_t os;
>>> const char *prop_name;
>>> int ret;
>>> @@ -1721,7 +1722,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>> }
>>> } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
>>> ulong image_start, image_end;
>>> - ulong load_end;
>>> + phys_addr_t load_end;
>>> void *dst;
>>>
>>> /*
>>> @@ -1738,8 +1739,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>> return -EXDEV;
>>> }
>>>
>>> - printf(" Loading %s from 0x%08lx to 0x%08lx\n",
>>> - prop_name, data, load);
>>> + printf(" Loading %s from 0x%08lx to %08llx\n",
>>> + prop_name, data, (uint64_t)load);
>>
>> Do you need to cast? Why not use your magic printf() string #define?
>
> Sure. Let me respin the patch and double confirm on an ARMv8 board.
OK
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image
2016-02-16 16:02 ` Simon Glass
@ 2016-02-24 22:55 ` york sun
2016-02-25 0:30 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: york sun @ 2016-02-24 22:55 UTC (permalink / raw)
To: u-boot
On 02/16/2016 08:02 AM, Simon Glass wrote:
> Hi York,
>
> On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
>> FIT image supports more than 32 bits in addresses by using #address-cell
>> field. However the address length is not handled when parsing FIT images.
>>
>
> nit: How about saying "fix this by adding support for 64-bit
> addresses" or similar
>
Sure. I can fix that.
>> Signed-off-by: York Sun <york.sun@nxp.com>
>>
>> ---
>>
>> Changes in v4:
>> Separate ulong to phys_addr_t change to another patch.
>>
>> Changes in v3:
>> Define PRIpa for host and target in common/image-fit.c so printf works
>> properly for 32-, 64-bit targets and host tools.
>>
>> Changes in v2:
>> Make a common function for both load and entry addresses.
>> Simplify calculation of addresses in a similar way as fdtdec_get_number()
>> fdtdec_get_number() is not used, or too many files need to be included
>> and/or twisted for host tool
>> Continue to use %08llx for print format for load and entry addresses
>> because %pa does not always work for host tool (mkimage)
>>
>> common/image-fit.c | 54 ++++++++++++++++++++++++++++++----------------------
>> 1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index bfa76a2..c000475 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -433,7 +433,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>
>> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>> (type == IH_TYPE_RAMDISK)) {
>> - fit_image_get_entry(fit, image_noffset, &entry);
>> + ret = fit_image_get_entry(fit, image_noffset, &entry);
>> printf("%s Entry Point: ", p);
>> if (ret)
>> printf("unavailable\n");
>> @@ -675,6 +675,34 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>> return 0;
>> }
>>
>> +static int fit_image_get_address(const void *fit, int noffset, char *name,
>> + phys_addr_t *load)
>> +{
>> + int len, cell_len;
>> + const fdt32_t *cell;
>> + unsigned long long load64 = 0;
>> +
>> + cell = fdt_getprop(fit, noffset, name, &len);
>> + if (cell == NULL) {
>> + fit_get_debug(fit, noffset, name, len);
>> + return -1;
>> + }
>> +
>> + if (len > sizeof(phys_addr_t)) {
>> + printf("Unsupported %s address size\n", name);
>> + return -1;
>> + }
>> +
>> + cell_len = len >> 2;
>> + /* Use load64 to avoid compiling warning for 32-bit target */
>> + while (cell_len--) {
>> + load64 = (load64 << 32) | uimage_to_cpu(*cell);
>> + cell++;
>> + }
>> + *load = (phys_addr_t)load64;
>> +
>> + return 0;
>> +}
>> /**
>> * fit_image_get_load() - get load addr property for given component image node
>> * @fit: pointer to the FIT format image header
>> @@ -690,17 +718,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>> */
>> int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>> {
>> - int len;
>> - const uint32_t *data;
>> -
>> - data = fdt_getprop(fit, noffset, FIT_LOAD_PROP, &len);
>> - if (data == NULL) {
>> - fit_get_debug(fit, noffset, FIT_LOAD_PROP, len);
>> - return -1;
>> - }
>> -
>> - *load = uimage_to_cpu(*data);
>> - return 0;
>> + return fit_image_get_address(fit, noffset, FIT_LOAD_PROP, load);
>
> I think it would make sense to have your new fit_image_get_address()
> in one patch, and the enhancement to support more address sizes in
> another.
The new fit_image_get_address() gets correct address. The rest of change is to
use the new function. I don't think they can be separated. Maybe I don't
understand your comment.
I am preparing a new version. Please comment on that if you still feel the same.
York
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image
2016-02-24 22:55 ` york sun
@ 2016-02-25 0:30 ` Simon Glass
2016-02-25 16:54 ` york sun
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2016-02-25 0:30 UTC (permalink / raw)
To: u-boot
Hi York,
On 24 February 2016 at 15:55, york sun <york.sun@nxp.com> wrote:
> On 02/16/2016 08:02 AM, Simon Glass wrote:
>> Hi York,
>>
>> On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
>>> FIT image supports more than 32 bits in addresses by using #address-cell
>>> field. However the address length is not handled when parsing FIT images.
>>>
>>
>> nit: How about saying "fix this by adding support for 64-bit
>> addresses" or similar
>>
>
> Sure. I can fix that.
>
>>> Signed-off-by: York Sun <york.sun@nxp.com>
>>>
>>> ---
>>>
>>> Changes in v4:
>>> Separate ulong to phys_addr_t change to another patch.
>>>
>>> Changes in v3:
>>> Define PRIpa for host and target in common/image-fit.c so printf works
>>> properly for 32-, 64-bit targets and host tools.
>>>
>>> Changes in v2:
>>> Make a common function for both load and entry addresses.
>>> Simplify calculation of addresses in a similar way as fdtdec_get_number()
>>> fdtdec_get_number() is not used, or too many files need to be included
>>> and/or twisted for host tool
>>> Continue to use %08llx for print format for load and entry addresses
>>> because %pa does not always work for host tool (mkimage)
>>>
>>> common/image-fit.c | 54 ++++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 31 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index bfa76a2..c000475 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -433,7 +433,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>>
>>> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>>> (type == IH_TYPE_RAMDISK)) {
>>> - fit_image_get_entry(fit, image_noffset, &entry);
>>> + ret = fit_image_get_entry(fit, image_noffset, &entry);
>>> printf("%s Entry Point: ", p);
>>> if (ret)
>>> printf("unavailable\n");
>>> @@ -675,6 +675,34 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>>> return 0;
>>> }
>>>
>>> +static int fit_image_get_address(const void *fit, int noffset, char *name,
>>> + phys_addr_t *load)
>>> +{
>>> + int len, cell_len;
>>> + const fdt32_t *cell;
>>> + unsigned long long load64 = 0;
>>> +
>>> + cell = fdt_getprop(fit, noffset, name, &len);
>>> + if (cell == NULL) {
>>> + fit_get_debug(fit, noffset, name, len);
>>> + return -1;
>>> + }
>>> +
>>> + if (len > sizeof(phys_addr_t)) {
>>> + printf("Unsupported %s address size\n", name);
>>> + return -1;
>>> + }
>>> +
>>> + cell_len = len >> 2;
>>> + /* Use load64 to avoid compiling warning for 32-bit target */
>>> + while (cell_len--) {
>>> + load64 = (load64 << 32) | uimage_to_cpu(*cell);
>>> + cell++;
>>> + }
>>> + *load = (phys_addr_t)load64;
>>> +
>>> + return 0;
>>> +}
>>> /**
>>> * fit_image_get_load() - get load addr property for given component image node
>>> * @fit: pointer to the FIT format image header
>>> @@ -690,17 +718,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>>> */
>>> int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>>> {
>>> - int len;
>>> - const uint32_t *data;
>>> -
>>> - data = fdt_getprop(fit, noffset, FIT_LOAD_PROP, &len);
>>> - if (data == NULL) {
>>> - fit_get_debug(fit, noffset, FIT_LOAD_PROP, len);
>>> - return -1;
>>> - }
>>> -
>>> - *load = uimage_to_cpu(*data);
>>> - return 0;
>>> + return fit_image_get_address(fit, noffset, FIT_LOAD_PROP, load);
>>
>> I think it would make sense to have your new fit_image_get_address()
>> in one patch, and the enhancement to support more address sizes in
>> another.
>
> The new fit_image_get_address() gets correct address. The rest of change is to
> use the new function. I don't think they can be separated. Maybe I don't
> understand your comment.
>
> I am preparing a new version. Please comment on that if you still feel the same.
I mean:
- patch 1: take the existing 32-bit-only code and put it in a new
fit_image_get_address() functoin
- patch 2: enhance your new function to support 64-bit
At present you have these two things co-mingled which I don't think is ideal.
if you don't agree or this doesn't make sense, that is fine too. For that case:
Reviewed-by: Simon Glass <sjg@chromium.org>
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image
2016-02-25 0:30 ` Simon Glass
@ 2016-02-25 16:54 ` york sun
0 siblings, 0 replies; 13+ messages in thread
From: york sun @ 2016-02-25 16:54 UTC (permalink / raw)
To: u-boot
On 02/24/2016 04:30 PM, Simon Glass wrote:
> Hi York,
>
> On 24 February 2016 at 15:55, york sun <york.sun@nxp.com> wrote:
>> On 02/16/2016 08:02 AM, Simon Glass wrote:
>>> Hi York,
>>>
>>> On 12 February 2016 at 13:59, York Sun <york.sun@nxp.com> wrote:
>>>> FIT image supports more than 32 bits in addresses by using #address-cell
>>>> field. However the address length is not handled when parsing FIT images.
>>>>
>>>
>>> nit: How about saying "fix this by adding support for 64-bit
>>> addresses" or similar
>>>
>>
>> Sure. I can fix that.
>>
>>>> Signed-off-by: York Sun <york.sun@nxp.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v4:
>>>> Separate ulong to phys_addr_t change to another patch.
>>>>
>>>> Changes in v3:
>>>> Define PRIpa for host and target in common/image-fit.c so printf works
>>>> properly for 32-, 64-bit targets and host tools.
>>>>
>>>> Changes in v2:
>>>> Make a common function for both load and entry addresses.
>>>> Simplify calculation of addresses in a similar way as fdtdec_get_number()
>>>> fdtdec_get_number() is not used, or too many files need to be included
>>>> and/or twisted for host tool
>>>> Continue to use %08llx for print format for load and entry addresses
>>>> because %pa does not always work for host tool (mkimage)
>>>>
>>>> common/image-fit.c | 54 ++++++++++++++++++++++++++++++----------------------
>>>> 1 file changed, 31 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>>> index bfa76a2..c000475 100644
>>>> --- a/common/image-fit.c
>>>> +++ b/common/image-fit.c
>>>> @@ -433,7 +433,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>>>>
>>>> if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>>>> (type == IH_TYPE_RAMDISK)) {
>>>> - fit_image_get_entry(fit, image_noffset, &entry);
>>>> + ret = fit_image_get_entry(fit, image_noffset, &entry);
>>>> printf("%s Entry Point: ", p);
>>>> if (ret)
>>>> printf("unavailable\n");
>>>> @@ -675,6 +675,34 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>>>> return 0;
>>>> }
>>>>
>>>> +static int fit_image_get_address(const void *fit, int noffset, char *name,
>>>> + phys_addr_t *load)
>>>> +{
>>>> + int len, cell_len;
>>>> + const fdt32_t *cell;
>>>> + unsigned long long load64 = 0;
>>>> +
>>>> + cell = fdt_getprop(fit, noffset, name, &len);
>>>> + if (cell == NULL) {
>>>> + fit_get_debug(fit, noffset, name, len);
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (len > sizeof(phys_addr_t)) {
>>>> + printf("Unsupported %s address size\n", name);
>>>> + return -1;
>>>> + }
>>>> +
>>>> + cell_len = len >> 2;
>>>> + /* Use load64 to avoid compiling warning for 32-bit target */
>>>> + while (cell_len--) {
>>>> + load64 = (load64 << 32) | uimage_to_cpu(*cell);
>>>> + cell++;
>>>> + }
>>>> + *load = (phys_addr_t)load64;
>>>> +
>>>> + return 0;
>>>> +}
>>>> /**
>>>> * fit_image_get_load() - get load addr property for given component image node
>>>> * @fit: pointer to the FIT format image header
>>>> @@ -690,17 +718,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>>>> */
>>>> int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>>>> {
>>>> - int len;
>>>> - const uint32_t *data;
>>>> -
>>>> - data = fdt_getprop(fit, noffset, FIT_LOAD_PROP, &len);
>>>> - if (data == NULL) {
>>>> - fit_get_debug(fit, noffset, FIT_LOAD_PROP, len);
>>>> - return -1;
>>>> - }
>>>> -
>>>> - *load = uimage_to_cpu(*data);
>>>> - return 0;
>>>> + return fit_image_get_address(fit, noffset, FIT_LOAD_PROP, load);
>>>
>>> I think it would make sense to have your new fit_image_get_address()
>>> in one patch, and the enhancement to support more address sizes in
>>> another.
>>
>> The new fit_image_get_address() gets correct address. The rest of change is to
>> use the new function. I don't think they can be separated. Maybe I don't
>> understand your comment.
>>
>> I am preparing a new version. Please comment on that if you still feel the same.
>
> I mean:
>
> - patch 1: take the existing 32-bit-only code and put it in a new
> fit_image_get_address() functoin
> - patch 2: enhance your new function to support 64-bit
>
> At present you have these two things co-mingled which I don't think is ideal.
>
I see. Let me work on it.
York
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-02-25 16:54 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12 20:59 [U-Boot] [RFC PATCH v4 0/3] Enable FIT image to be loaded beyond 32-bit space York Sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 1/3] sandbox: Fix compiling warning on 32-bit host York Sun
2016-02-16 16:01 ` Simon Glass
2016-02-23 16:48 ` york sun
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 2/3] common: Convert ulong to phys_addr_t for image addresses York Sun
2016-02-16 16:01 ` Simon Glass
2016-02-23 20:36 ` york sun
2016-02-23 23:28 ` Simon Glass
2016-02-12 20:59 ` [U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image York Sun
2016-02-16 16:02 ` Simon Glass
2016-02-24 22:55 ` york sun
2016-02-25 0:30 ` Simon Glass
2016-02-25 16:54 ` york sun
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.