| Message ID | 20200214173819.9277-1-trini@konsulko.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | image.h: Change android_image_get_dtb* to use uint and not u32 | expand |
Hi Tom, Cc: Masahiro Yamada On Fri, Feb 14, 2020 at 12:38:19PM -0500, Tom Rini wrote: > The image.h header can be used fairly widely in U-Boot builds. We > cannot use u32 here as it may be used in cases where we don't have that > typedef available and don't want to expose it either. Use uint instead > here. > > Cc: Eugeniu Rosca <roscaeugeniu@gmail.com> > Cc: Sam Protsenko <joe.skb7@gmail.com> > Signed-off-by: Tom Rini <trini@konsulko.com> > --- > include/image.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/image.h b/include/image.h > index b316d167d8d7..1dc3b48d8689 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, > ulong *rd_data, ulong *rd_len); > int android_image_get_second(const struct andr_img_hdr *hdr, > ulong *second_data, ulong *second_len); > -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > - u32 *size); > +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size); > +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr, > + uint *size); While I think the change is harmless and brings some consistency and visual comfort when reviewing the types employed in 'include/image.h', I can hardly imagine a real-life breakage introduced by u32 in 'include/image.h'. That's because the 'image.h' itself seems to ensure the definition of u32 and friends, as can be understood from below test: --------------------- 8< -------------------------- 1. Create a dummy C file which includes <image.h> $ cat cmd/my-test.c #define FORCE_BREAKAGE #include <image.h> 2. Force a build-time error on including "int-ll64.h", which defines u32 and friends for all architectures, as per commit 3747bdbb2bb83a ("arch: types.h: factor out fixed width typedefs to int-ll64.h") (thanks Yamada-san): $ git diff -- include/asm-generic/int-ll64.h diff --git a/include/asm-generic/int-ll64.h b/include/asm-generic/int-ll64.h index 7451718a61cd..f90dc2826071 100644 --- a/include/asm-generic/int-ll64.h +++ b/include/asm-generic/int-ll64.h @@ -15,6 +15,10 @@ * header files exported to user space */ +#ifdef FORCE_BREAKAGE +#error "Included, thus providing (s,u){8,16,32,64}" +#endif + typedef __signed__ char __s8; typedef unsigned char __u8; 3. Compile the newly created C file for sandbox $ make cmd/my-test.o CC cmd/my-test.o In file included from ./arch/sandbox/include/asm/types.h:9, from include/linux/types.h:5, from include/linux/string.h:4, from include/compiler.h:126, from include/image.h:18, from cmd/my-test.c:3: include/asm-generic/int-ll64.h:19:2: error: #error "Included, thus providing (s,u){8,16,32,64}" --------------------- 8< -------------------------- So, to summarize, I don't fully understand the motivation behind this change, unless there is a long-term goal to keep image.h (and/or other U-Boot headers) free of u32 and Co. Just to contrast the frequency of 'uint' vs 'u32' in v5.5 Linux release: $ git log --oneline -S"\bu32\b" --pickaxe-regex v5.4..v5.5 | wc -l 1020 $ git log --oneline -S"\buint\b" --pickaxe-regex v5.4..v5.5 | wc -l 35 Assuming you still want the change, since there is no regression: Reviewed-by: Eugeniu Rosca <roscaeugeniu@gmail.com>
On Sun, Feb 16, 2020 at 05:23:14PM +0100, Eugeniu Rosca wrote: > Hi Tom, > Cc: Masahiro Yamada > > On Fri, Feb 14, 2020 at 12:38:19PM -0500, Tom Rini wrote: > > The image.h header can be used fairly widely in U-Boot builds. We > > cannot use u32 here as it may be used in cases where we don't have that > > typedef available and don't want to expose it either. Use uint instead > > here. > > > > Cc: Eugeniu Rosca <roscaeugeniu@gmail.com> > > Cc: Sam Protsenko <joe.skb7@gmail.com> > > Signed-off-by: Tom Rini <trini@konsulko.com> > > --- > > include/image.h | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/include/image.h b/include/image.h > > index b316d167d8d7..1dc3b48d8689 100644 > > --- a/include/image.h > > +++ b/include/image.h > > @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, > > ulong *rd_data, ulong *rd_len); > > int android_image_get_second(const struct andr_img_hdr *hdr, > > ulong *second_data, ulong *second_len); > > -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > > -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > > - u32 *size); > > +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size); > > +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr, > > + uint *size); > > While I think the change is harmless and brings some consistency and > visual comfort when reviewing the types employed in 'include/image.h', > I can hardly imagine a real-life breakage introduced by u32 in > 'include/image.h'. I ran in to this in practice with http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* applied.
Hi Tom, On Sun, Feb 16, 2020 at 11:53:23AM -0500, Tom Rini wrote: > On Sun, Feb 16, 2020 at 05:23:14PM +0100, Eugeniu Rosca wrote: > > On Fri, Feb 14, 2020 at 12:38:19PM -0500, Tom Rini wrote: > > > The image.h header can be used fairly widely in U-Boot builds. We > > > cannot use u32 here as it may be used in cases where we don't have that > > > typedef available and don't want to expose it either. Use uint instead > > > here. > > > > > > Cc: Eugeniu Rosca <roscaeugeniu@gmail.com> > > > Cc: Sam Protsenko <joe.skb7@gmail.com> > > > Signed-off-by: Tom Rini <trini@konsulko.com> > > > --- > > > include/image.h | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/image.h b/include/image.h > > > index b316d167d8d7..1dc3b48d8689 100644 > > > --- a/include/image.h > > > +++ b/include/image.h > > > @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, > > > ulong *rd_data, ulong *rd_len); > > > int android_image_get_second(const struct andr_img_hdr *hdr, > > > ulong *second_data, ulong *second_len); > > > -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > > > -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > > > - u32 *size); > > > +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size); > > > +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr, > > > + uint *size); > > > > While I think the change is harmless and brings some consistency and > > visual comfort when reviewing the types employed in 'include/image.h', > > I can hardly imagine a real-life breakage introduced by u32 in > > 'include/image.h'. > > I ran in to this in practice with > http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* > applied. Applying this series to u-boot/master, I am running into below build failure [1], which I believe is something you try to fix in this patch. It looks to me that U-Boot's 'include/image.h' is used not only by files which are compiled for the target device, but also by files located in 'tools/', which are compiled for the host with -DUSE_HOSTCC. After inspecting the 'tools/' path of U-Boot repository, it looks like the definition of 'u32' is indeed missing there, so I believe that's the root cause of the build failure. W.r.t. 'android_image_*' functions, I really doubt that they were designed to be compiled with USE_HOSTCC. If so, then IMHO we shouldn't try to make them compliant with USE_HOSTCC compilation, since this will impose additional constraints/requirements to the development style of those functions. IMHO we should just hide the android_image functions on enabling -DUSE_HOSTCC, as shown in [2]. What's your view on that? [1] Build error after applying to u-boot/master below series: http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* In file included from include/u-boot/rsa-mod-exp.h:10, from ./tools/../lib/rsa/rsa-verify.c:22, from tools/lib/rsa/rsa-verify.c:1: include/image.h:1440:58: error: unknown type name ‘u32’ 1440 | bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); | ^~~ include/image.h:1441:53: error: unknown type name ‘u32’ 1441 | bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, | ^~~ include/image.h:1442:9: error: unknown type name ‘u32’ 1442 | u32 *size); | ^~~ HOSTCC tools/asn1_compiler make[1]: *** [scripts/Makefile.host:114: tools/lib/rsa/rsa-verify.o] Error 1 make[1]: *** Waiting for unfinished jobs.... HOSTLD tools/mkenvimage make: *** [Makefile:1728: tools] Error 2 [2] Hide the android_image_* functions when USE_HOSTCC is enabled diff --git a/include/image.h b/include/image.h index ebec329582eb..0cdb2165fdaf 100644 --- a/include/image.h +++ b/include/image.h @@ -1429,7 +1429,7 @@ struct cipher_algo *image_get_cipher_algo(const char *full_name); #endif /* CONFIG_FIT_VERBOSE */ #endif /* CONFIG_FIT */ -#if defined(CONFIG_ANDROID_BOOT_IMAGE) +#if defined(CONFIG_ANDROID_BOOT_IMAGE) && !defined(USE_HOSTCC) 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, @@ -1449,7 +1449,7 @@ void android_print_contents(const struct andr_img_hdr *hdr); bool android_image_print_dtb_contents(ulong hdr_addr); #endif -#endif /* CONFIG_ANDROID_BOOT_IMAGE */ +#endif /* CONFIG_ANDROID_BOOT_IMAGE && !USE_HOSTCC */ /** * board_fit_config_name_match() - Check for a matching board name
Hi Eugeniu, Tom, On Mon, Feb 17, 2020 at 7:05 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > Hi Tom, > > On Sun, Feb 16, 2020 at 11:53:23AM -0500, Tom Rini wrote: > > On Sun, Feb 16, 2020 at 05:23:14PM +0100, Eugeniu Rosca wrote: > > > On Fri, Feb 14, 2020 at 12:38:19PM -0500, Tom Rini wrote: > > > > The image.h header can be used fairly widely in U-Boot builds. We > > > > cannot use u32 here as it may be used in cases where we don't have that > > > > typedef available and don't want to expose it either. Use uint instead > > > > here. > > > > > > > > Cc: Eugeniu Rosca <roscaeugeniu@gmail.com> > > > > Cc: Sam Protsenko <joe.skb7@gmail.com> > > > > Signed-off-by: Tom Rini <trini@konsulko.com> > > > > --- > > > > include/image.h | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/include/image.h b/include/image.h > > > > index b316d167d8d7..1dc3b48d8689 100644 > > > > --- a/include/image.h > > > > +++ b/include/image.h > > > > @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, > > > > ulong *rd_data, ulong *rd_len); > > > > int android_image_get_second(const struct andr_img_hdr *hdr, > > > > ulong *second_data, ulong *second_len); > > > > -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > > > > -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > > > > - u32 *size); > > > > +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size); > > > > +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr, > > > > + uint *size); > > > > > > While I think the change is harmless and brings some consistency and > > > visual comfort when reviewing the types employed in 'include/image.h', > > > I can hardly imagine a real-life breakage introduced by u32 in > > > 'include/image.h'. > > > > I ran in to this in practice with > > http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* > > applied. > > Applying this series to u-boot/master, I am running into below build > failure [1], which I believe is something you try to fix in this patch. > > It looks to me that U-Boot's 'include/image.h' is used not only by > files which are compiled for the target device, but also by files > located in 'tools/', which are compiled for the host with -DUSE_HOSTCC. > After inspecting the 'tools/' path of U-Boot repository, it looks like > the definition of 'u32' is indeed missing there, so I believe that's > the root cause of the build failure. If you need a fixed-width type, you can use uint32_t if you like. It is already used. See line 183 of include/image.h typedef struct image_header { uint32_t ih_magic; /* Image Header Magic Number */ include/compiler.h includes <stdint.h> when USE_HOSTCC is defined. However, forbidding u32 for tools is questionable to me. u32 and uint32_t should be always interchangeable. Perhaps, does the following patch work? (untested) --------------------->8------------------------ diff --git a/include/compiler.h b/include/compiler.h index ed74c272b8c5..f2a4adfbc7e4 100644 --- a/include/compiler.h +++ b/include/compiler.h @@ -61,6 +61,9 @@ #include <time.h> +typedef uint8_t u8; +typedef uint16_t u16; +typedef uint32_t u32; typedef uint8_t __u8; typedef uint16_t __u16; typedef uint32_t __u32; --------------------->8------------------------ BTW, I think include/compiler.h in U-Boot is ugly. Linux kernel uses tools/include/linux/types.h for defining {u8,u16,u32,u64} for the tools space. Barebox also adopted a similar approach. When compiling files for tools, <linux/types.h> actually includes scripts/include/linux/types.h instead of include/linux/types.h Perhaps, U-Boot could do similar, but I have never got around to it. > > W.r.t. 'android_image_*' functions, I really doubt that they were > designed to be compiled with USE_HOSTCC. If so, then IMHO we shouldn't > try to make them compliant with USE_HOSTCC compilation, since this > will impose additional constraints/requirements to the development style > of those functions. IMHO we should just hide the android_image functions > on enabling -DUSE_HOSTCC, as shown in [2]. What's your view on that? > [1] Build error after applying to u-boot/master below series: > http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* > > In file included from include/u-boot/rsa-mod-exp.h:10, > from ./tools/../lib/rsa/rsa-verify.c:22, > from tools/lib/rsa/rsa-verify.c:1: > include/image.h:1440:58: error: unknown type name ‘u32’ > 1440 | bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > | ^~~ > include/image.h:1441:53: error: unknown type name ‘u32’ > 1441 | bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > | ^~~ > include/image.h:1442:9: error: unknown type name ‘u32’ > 1442 | u32 *size); > | ^~~ > HOSTCC tools/asn1_compiler > make[1]: *** [scripts/Makefile.host:114: tools/lib/rsa/rsa-verify.o] Error 1 > make[1]: *** Waiting for unfinished jobs.... > HOSTLD tools/mkenvimage > make: *** [Makefile:1728: tools] Error 2 > > [2] Hide the android_image_* functions when USE_HOSTCC is enabled > diff --git a/include/image.h b/include/image.h > index ebec329582eb..0cdb2165fdaf 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1429,7 +1429,7 @@ struct cipher_algo *image_get_cipher_algo(const char *full_name); > #endif /* CONFIG_FIT_VERBOSE */ > #endif /* CONFIG_FIT */ > > -#if defined(CONFIG_ANDROID_BOOT_IMAGE) > +#if defined(CONFIG_ANDROID_BOOT_IMAGE) && !defined(USE_HOSTCC) > 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, > @@ -1449,7 +1449,7 @@ void android_print_contents(const struct andr_img_hdr *hdr); > bool android_image_print_dtb_contents(ulong hdr_addr); > #endif > > -#endif /* CONFIG_ANDROID_BOOT_IMAGE */ > +#endif /* CONFIG_ANDROID_BOOT_IMAGE && !USE_HOSTCC */ > > /** > * board_fit_config_name_match() - Check for a matching board name > Maybe U-Boot shares too much code between U-Boot space and tooling space? include/image.h of U-Boot is 1520 lines. include/image.h of Barebox is 258 lines. But, I am not tracking how they diverged. Shrinking the interface between U-Boot space and tooling space will provide a better maintainability. ifdef would work. Perhaps, splitting the header might be even better. That's my random thought. I have not looked into the detail, though. -- Best Regards Masahiro Yamada
Hello Yamada-san, Thank you for your precious thoughts! On Mon, Feb 17, 2020 at 04:45:57PM +0900, Masahiro Yamada wrote: > If you need a fixed-width type, > you can use uint32_t if you like. > > It is already used. See line 183 of include/image.h > > typedef struct image_header { > uint32_t ih_magic; /* Image Header Magic Number */ > > include/compiler.h includes <stdint.h> when USE_HOSTCC is defined. I think this is a safe, simple and non-intrusive solution, so I have pushed https://patchwork.ozlabs.org/patch/1239098/. > However, forbidding u32 for tools is questionable to me. Since Linux has never restricted 'u32' in its in-tree tooling, I totally support this statement. > u32 and uint32_t should be always interchangeable. > > Perhaps, does the following patch work? (untested) > --------------------->8------------------------ > diff --git a/include/compiler.h b/include/compiler.h > index ed74c272b8c5..f2a4adfbc7e4 100644 > --- a/include/compiler.h > +++ b/include/compiler.h > @@ -61,6 +61,9 @@ > > #include <time.h> > > +typedef uint8_t u8; > +typedef uint16_t u16; > +typedef uint32_t u32; > typedef uint8_t __u8; > typedef uint16_t __u16; > typedef uint32_t __u32; > --------------------->8------------------------ > Since this has greater impact compared to s/u32/uint32_t/, I leave up to Tom to decide on the most suitable approach. > > BTW, I think include/compiler.h in U-Boot is ugly. > > Linux kernel uses > tools/include/linux/types.h > for defining {u8,u16,u32,u64} for the tools space. > Agreed, since below v3.16 commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d944c4eebcf4c0 > > Barebox also adopted a similar approach. > > When compiling files for tools, > <linux/types.h> actually includes > scripts/include/linux/types.h > instead of include/linux/types.h > > > Perhaps, U-Boot could do similar, > but I have never got around to it. > > Maybe U-Boot shares too much code > between U-Boot space and tooling space? > > include/image.h of U-Boot is 1520 lines. > include/image.h of Barebox is 258 lines. > > But, I am not tracking how they diverged. > > Shrinking the interface between U-Boot space and > tooling space will provide a better maintainability. > > ifdef would work. Perhaps, splitting the header might be even better. The above sounds like food for thought on how to mitigate the problem long-term. > > That's my random thought. Many thanks!
On Mon, Feb 17, 2020 at 04:45:57PM +0900, Masahiro Yamada wrote: > Hi Eugeniu, Tom, > > > On Mon, Feb 17, 2020 at 7:05 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > > > Hi Tom, > > > > On Sun, Feb 16, 2020 at 11:53:23AM -0500, Tom Rini wrote: > > > On Sun, Feb 16, 2020 at 05:23:14PM +0100, Eugeniu Rosca wrote: > > > > On Fri, Feb 14, 2020 at 12:38:19PM -0500, Tom Rini wrote: > > > > > The image.h header can be used fairly widely in U-Boot builds. We > > > > > cannot use u32 here as it may be used in cases where we don't have that > > > > > typedef available and don't want to expose it either. Use uint instead > > > > > here. > > > > > > > > > > Cc: Eugeniu Rosca <roscaeugeniu@gmail.com> > > > > > Cc: Sam Protsenko <joe.skb7@gmail.com> > > > > > Signed-off-by: Tom Rini <trini@konsulko.com> > > > > > --- > > > > > include/image.h | 6 +++--- > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/include/image.h b/include/image.h > > > > > index b316d167d8d7..1dc3b48d8689 100644 > > > > > --- a/include/image.h > > > > > +++ b/include/image.h > > > > > @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, > > > > > ulong *rd_data, ulong *rd_len); > > > > > int android_image_get_second(const struct andr_img_hdr *hdr, > > > > > ulong *second_data, ulong *second_len); > > > > > -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > > > > > -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > > > > > - u32 *size); > > > > > +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size); > > > > > +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr, > > > > > + uint *size); > > > > > > > > While I think the change is harmless and brings some consistency and > > > > visual comfort when reviewing the types employed in 'include/image.h', > > > > I can hardly imagine a real-life breakage introduced by u32 in > > > > 'include/image.h'. > > > > > > I ran in to this in practice with > > > http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* > > > applied. > > > > Applying this series to u-boot/master, I am running into below build > > failure [1], which I believe is something you try to fix in this patch. > > > > It looks to me that U-Boot's 'include/image.h' is used not only by > > files which are compiled for the target device, but also by files > > located in 'tools/', which are compiled for the host with -DUSE_HOSTCC. > > After inspecting the 'tools/' path of U-Boot repository, it looks like > > the definition of 'u32' is indeed missing there, so I believe that's > > the root cause of the build failure. > > > If you need a fixed-width type, > you can use uint32_t if you like. > > It is already used. See line 183 of include/image.h > > typedef struct image_header { > uint32_t ih_magic; /* Image Header Magic Number */ > > > include/compiler.h includes <stdint.h> when USE_HOSTCC is defined. > > > > However, forbidding u32 for tools is questionable to me. > u32 and uint32_t should be always interchangeable. > > > Perhaps, does the following patch work? (untested) > --------------------->8------------------------ > diff --git a/include/compiler.h b/include/compiler.h > index ed74c272b8c5..f2a4adfbc7e4 100644 > --- a/include/compiler.h > +++ b/include/compiler.h > @@ -61,6 +61,9 @@ > > #include <time.h> > > +typedef uint8_t u8; > +typedef uint16_t u16; > +typedef uint32_t u32; > typedef uint8_t __u8; > typedef uint16_t __u16; > typedef uint32_t __u32; > --------------------->8------------------------ > > > > BTW, I think include/compiler.h in U-Boot is ugly. Yes, we should not further expand that file. We should indeed get rid of it. > Linux kernel uses > tools/include/linux/types.h > for defining {u8,u16,u32,u64} for the tools space. > > > Barebox also adopted a similar approach. > > When compiling files for tools, > <linux/types.h> actually includes > scripts/include/linux/types.h > instead of include/linux/types.h > > > Perhaps, U-Boot could do similar, > but I have never got around to it. Yes, it's just another thing on the TODO list that's not been re-synced in a while. > > W.r.t. 'android_image_*' functions, I really doubt that they were > > designed to be compiled with USE_HOSTCC. If so, then IMHO we shouldn't > > try to make them compliant with USE_HOSTCC compilation, since this > > will impose additional constraints/requirements to the development style > > of those functions. IMHO we should just hide the android_image functions > > on enabling -DUSE_HOSTCC, as shown in [2]. What's your view on that? > > [1] Build error after applying to u-boot/master below series: > > http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=* > > > > In file included from include/u-boot/rsa-mod-exp.h:10, > > from ./tools/../lib/rsa/rsa-verify.c:22, > > from tools/lib/rsa/rsa-verify.c:1: > > include/image.h:1440:58: error: unknown type name ‘u32’ > > 1440 | bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); > > | ^~~ > > include/image.h:1441:53: error: unknown type name ‘u32’ > > 1441 | bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, > > | ^~~ > > include/image.h:1442:9: error: unknown type name ‘u32’ > > 1442 | u32 *size); > > | ^~~ > > HOSTCC tools/asn1_compiler > > make[1]: *** [scripts/Makefile.host:114: tools/lib/rsa/rsa-verify.o] Error 1 > > make[1]: *** Waiting for unfinished jobs.... > > HOSTLD tools/mkenvimage > > make: *** [Makefile:1728: tools] Error 2 > > > > [2] Hide the android_image_* functions when USE_HOSTCC is enabled > > diff --git a/include/image.h b/include/image.h > > index ebec329582eb..0cdb2165fdaf 100644 > > --- a/include/image.h > > +++ b/include/image.h > > @@ -1429,7 +1429,7 @@ struct cipher_algo *image_get_cipher_algo(const char *full_name); > > #endif /* CONFIG_FIT_VERBOSE */ > > #endif /* CONFIG_FIT */ > > > > -#if defined(CONFIG_ANDROID_BOOT_IMAGE) > > +#if defined(CONFIG_ANDROID_BOOT_IMAGE) && !defined(USE_HOSTCC) > > 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, > > @@ -1449,7 +1449,7 @@ void android_print_contents(const struct andr_img_hdr *hdr); > > bool android_image_print_dtb_contents(ulong hdr_addr); > > #endif > > > > -#endif /* CONFIG_ANDROID_BOOT_IMAGE */ > > +#endif /* CONFIG_ANDROID_BOOT_IMAGE && !USE_HOSTCC */ > > > > /** > > * board_fit_config_name_match() - Check for a matching board name > > > > > Maybe U-Boot shares too much code > between U-Boot space and tooling space? > > include/image.h of U-Boot is 1520 lines. > include/image.h of Barebox is 258 lines. > > But, I am not tracking how they diverged. > > Shrinking the interface between U-Boot space and > tooling space will provide a better maintainability. > > ifdef would work. Perhaps, splitting the header might be even better. For this specific problem, yes, the CONFIG_SPL_GUARD around Android stuff should get moved and perhaps a later step of moving it elsewhere. Reducing image.h itself would be largely a matter of moving the "legacy" image format code and FIT image format code into separate headers but keeping the notice about "not a derived work" on it.
diff --git a/include/image.h b/include/image.h index b316d167d8d7..1dc3b48d8689 100644 --- a/include/image.h +++ b/include/image.h @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, ulong *rd_data, ulong *rd_len); int android_image_get_second(const struct andr_img_hdr *hdr, ulong *second_data, ulong *second_len); -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size); -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, - u32 *size); +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size); +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr, + uint *size); ulong android_image_get_end(const struct andr_img_hdr *hdr); ulong android_image_get_kload(const struct andr_img_hdr *hdr); ulong android_image_get_kcomp(const struct andr_img_hdr *hdr);
The image.h header can be used fairly widely in U-Boot builds. We cannot use u32 here as it may be used in cases where we don't have that typedef available and don't want to expose it either. Use uint instead here. Cc: Eugeniu Rosca <roscaeugeniu@gmail.com> Cc: Sam Protsenko <joe.skb7@gmail.com> Signed-off-by: Tom Rini <trini@konsulko.com> --- include/image.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)