| Message ID | 20240920063204.1049202-2-martin.stolpe@gmail.com |
|---|---|
| State | New |
| Delegated to: | Peter Robinson |
| Headers | show |
| Series | rpi: Fix compilation when CONFIG_VIDEO is disabled | expand |
Hi, Am Fr., 20. Sept. 2024 um 10:10 Uhr schrieb Ivan T. Ivanov <iivanov@suse.de >: > Hi, > > On 09-20 08:32, Martin Stolpe wrote: > > > > When the CONFIG_VIDEO option is set to disabled for Raspberry Pi devices > > the build will fail with the following error message: > > "undefined reference to `fdt_simplefb_enable_and_mem_rsv'." > > > > Signed-off-by: Martin Stolpe <martin.stolpe@gmail.com> > > --- > > > > board/raspberrypi/rpi/rpi.c | 2 +- > > boot/fdt_simplefb.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > > index ab5ea85cf9..bc49708f85 100644 > > --- a/board/raspberrypi/rpi/rpi.c > > +++ b/board/raspberrypi/rpi/rpi.c > > @@ -572,7 +572,7 @@ int ft_board_setup(void *blob, struct bd_info *bd) > > node = fdt_node_offset_by_compatible(blob, -1, > "simple-framebuffer"); > > if (node < 0) > > fdt_simplefb_add_node(blob); > > - else > > + else if (IS_ENABLED(CONFIG_VIDEO)) > > fdt_simplefb_enable_and_mem_rsv(blob); > > I think there is one more user of this function which end of same > situation stm32mp1. > Would it make sense to replace the config option CONFIG_FDT_SIMPLEFB with CONFIG_VIDEO? CONFIG_FDT_SIMPLEFB is only used in the Raspberry Pi default configurations but in no other board configuration and I don't see it being used in the code besides in stm32mp1.c. Regards Martin
Hi Martin, Ivan On 24/09/24 11:56, Martin Stolpe wrote: > Hi, > > Am Fr., 20. Sept. 2024 um 10:10 Uhr schrieb Ivan T. Ivanov <iivanov@suse.de >> : > >> Hi, >> >> On 09-20 08:32, Martin Stolpe wrote: >>> >>> When the CONFIG_VIDEO option is set to disabled for Raspberry Pi devices >>> the build will fail with the following error message: >>> "undefined reference to `fdt_simplefb_enable_and_mem_rsv'." >>> Thanks for the patch. >>> Signed-off-by: Martin Stolpe <martin.stolpe@gmail.com> >>> --- >>> >>> board/raspberrypi/rpi/rpi.c | 2 +- >>> boot/fdt_simplefb.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >>> index ab5ea85cf9..bc49708f85 100644 >>> --- a/board/raspberrypi/rpi/rpi.c >>> +++ b/board/raspberrypi/rpi/rpi.c >>> @@ -572,7 +572,7 @@ int ft_board_setup(void *blob, struct bd_info *bd) >>> node = fdt_node_offset_by_compatible(blob, -1, >> "simple-framebuffer"); >>> if (node < 0) >>> fdt_simplefb_add_node(blob); >>> - else >>> + else if (IS_ENABLED(CONFIG_VIDEO)) >>> fdt_simplefb_enable_and_mem_rsv(blob); >> >> I think there is one more user of this function which end of same >> situation stm32mp1. >> > > Would it make sense to replace the config option CONFIG_FDT_SIMPLEFB with > CONFIG_VIDEO? CONFIG_FDT_SIMPLEFB is only used in the Raspberry Pi default > configurations but in no other board configuration and I don't see it being > used in the code besides in stm32mp1.c. > CONFIG_FDT_SIMPLEFB is only used in splash-screen context which in-turn depends on CONFIG_VIDEO. So CONFIG_FDT_SIMPLEFB is in a way dependent on CONFIG_VIDEO. We had fixed similar issue in past in vendor tree and by making CONFIG_FDT_SIMPLEFB dependent on CONFIG_VIDEO using below set of patches [1] which I was planning to post upstream too. Kindly let me know If these patches look good to you and fix your problem too, I can post same set of patches to upstream too. [1] : https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=1199800505f11f2162030cb641c6d0c9276d5c9c [2] : https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=5b4b8eac243cbd86286ff2cf57ca0469c4d86345 Regards Devarsh > Regards > Martin >
Hi, Am Di., 24. Sept. 2024 um 08:38 Uhr schrieb Devarsh Thakkar <devarsht@ti.com >: > > CONFIG_FDT_SIMPLEFB is only used in splash-screen context which in-turn > depends on CONFIG_VIDEO. So CONFIG_FDT_SIMPLEFB is in a way dependent on > CONFIG_VIDEO. We had fixed similar issue in past in vendor tree and by > making > CONFIG_FDT_SIMPLEFB dependent on CONFIG_VIDEO using below set of patches > [1] > which I was planning to post upstream too. > > Kindly let me know If these patches look good to you and fix your problem > too, > I can post same set of patches to upstream too. > > [1] : > > https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=1199800505f11f2162030cb641c6d0c9276d5c9c > [2] : > > https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=5b4b8eac243cbd86286ff2cf57ca0469c4d86345 > > These patches look good to me. If I understand the code correctly the frame buffer node should only be created if CONFIG_FDT_SIMPLEFB is enabled. Thus I would change the code like this: Signed-off-by: Martin Stolpe <martin.stolpe@gmail.com> --- board/raspberrypi/rpi/rpi.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c index ab5ea85cf9..47db441696 100644 --- a/board/raspberrypi/rpi/rpi.c +++ b/board/raspberrypi/rpi/rpi.c @@ -569,11 +569,13 @@ int ft_board_setup(void *blob, struct bd_info *bd) update_fdt_from_fw(blob, (void *)fw_dtb_pointer); - node = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer"); - if (node < 0) - fdt_simplefb_add_node(blob); - else - fdt_simplefb_enable_and_mem_rsv(blob); + if (CONFIG_IS_ENABLED(FDT_SIMPLEFB)) { + node = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer"); + if (node < 0) + fdt_simplefb_add_node(blob); + else + fdt_simplefb_enable_and_mem_rsv(blob); + } #ifdef CONFIG_EFI_LOADER /* Reserve the spin table */
Hi Martin, On 26/09/24 17:56, Martin Stolpe wrote: > Hi, > > Am Di., 24. Sept. 2024 um 08:38 Uhr schrieb Devarsh Thakkar > <devarsht@ti.com <mailto:devarsht@ti.com>>: > > > CONFIG_FDT_SIMPLEFB is only used in splash-screen context which in-turn > depends on CONFIG_VIDEO. So CONFIG_FDT_SIMPLEFB is in a way dependent on > CONFIG_VIDEO. We had fixed similar issue in past in vendor tree and > by making > CONFIG_FDT_SIMPLEFB dependent on CONFIG_VIDEO using below set of > patches [1] > which I was planning to post upstream too. > > Kindly let me know If these patches look good to you and fix your > problem too, > I can post same set of patches to upstream too. > > [1] : > https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=1199800505f11f2162030cb641c6d0c9276d5c9c <https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=1199800505f11f2162030cb641c6d0c9276d5c9c> > [2] : > https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=5b4b8eac243cbd86286ff2cf57ca0469c4d86345 <https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04&id=5b4b8eac243cbd86286ff2cf57ca0469c4d86345> > > > These patches look good to me. > Thanks for taking a look, I have posted them to the list [1], it would be great if you could provide a Reviewed-by or Tested-by too. > If I understand the code correctly the frame buffer node should only be > created if CONFIG_FDT_SIMPLEFB is enabled. Thus I would change the code > like this: > Yes, that's true, I think you also need to conditionally compile ft_board_setup only when CONFIG_OF_BOARD_SETUP is enabled. [1]: https://lore.kernel.org/all/20240925151354.480704-4-devarsht@ti.com/ Regards Devarsh
Hi Devarsh, Am Do., 26. Sept. 2024 um 18:45 Uhr schrieb Devarsh Thakkar <devarsht@ti.com >: > > Thanks for taking a look, I have posted them to the list [1], it would > be great if you could provide a Reviewed-by or Tested-by too. > I've tried to compile the project for am64x_evm_a53 with the patches applied and I've added the following line to am64x_evm_a53_defconfig: CONFIG_OF_BOARD_SETUP=y When I then try to compile U-Boot using the following command: make am64x_evm_a53_defconfig; make I get the following error message: aarch64-linux-gnu-ld.bfd: boot/image-fdt.o: in function `image_setup_libfdt': /home/mstolpe/repositories/build_base_station/build/workspace/sources/u-boot-copy/boot/image-fdt.c:633:(.text.image_setup_libfdt+0x150): undefined reference to `ft_board_setup' aarch64-linux-gnu-ld.bfd: cmd/fdt.o: in function `do_fdt': /home/mstolpe/repositories/build_base_station/build/workspace/sources/u-boot-copy/cmd/fdt.c:687:(.text.do_fdt+0xf98): undefined reference to `ft_board_setup' Segmentation fault (core dumped) make: *** [Makefile:1801: u-boot] Error 139 make: *** Deleting file 'u-boot' I guess this shouldn't happen? BR Martin
Hi Martin, On 07/10/24 18:24, Martin Stolpe wrote: > Hi Devarsh, > > Am Do., 26. Sept. 2024 um 18:45 Uhr schrieb Devarsh Thakkar <devarsht@ti.com >> : > >> >> Thanks for taking a look, I have posted them to the list [1], it would >> be great if you could provide a Reviewed-by or Tested-by too. >> > > I've tried to compile the project for am64x_evm_a53 with the patches > applied and I've added the following line to > am64x_evm_a53_defconfig: CONFIG_OF_BOARD_SETUP=y > When I then try to compile U-Boot using the following command: > make am64x_evm_a53_defconfig; make > > I get the following error message: > aarch64-linux-gnu-ld.bfd: boot/image-fdt.o: in function > `image_setup_libfdt': > /home/mstolpe/repositories/build_base_station/build/workspace/sources/u-boot-copy/boot/image-fdt.c:633:(.text.image_setup_libfdt+0x150): > undefined reference to `ft_board_setup' > aarch64-linux-gnu-ld.bfd: cmd/fdt.o: in function `do_fdt': > /home/mstolpe/repositories/build_base_station/build/workspace/sources/u-boot-copy/cmd/fdt.c:687:(.text.do_fdt+0xf98): > undefined reference to `ft_board_setup' > Segmentation fault (core dumped) > make: *** [Makefile:1801: u-boot] Error 139 > make: *** Deleting file 'u-boot' > > I guess this shouldn't happen? I think this is expected as you are trying to enable CONFIG_OF_BOARD_SETUP but AM64 platform does not implement ft_board_setup. Platforms need to implement ft_board_setup for platform specific knobs (for e.g. simplefb for splash etc) if using CONFIG_OF_BOARD_SETUP as per below snippet in boot/image-fdt.c : if (IS_ENABLED(CONFIG_OF_BOARD_SETUP)) { const char *skip_board_fixup; skip_board_fixup = env_get("skip_board_fixup"); if (skip_board_fixup && ((int)simple_strtol(skip_board_fixup, NULL, 10) == 1)) { printf("skip board fdt fixup\n"); } else { fdt_ret = ft_board_setup(blob, gd->bd); if (fdt_ret) { printf("ERROR: board-specific fdt fixup failed: %s\n", fdt_strerror(fdt_ret)); goto err; } } } Regards Devarsh > > BR > Martin >
Hi, Sorry if this is a stupid question but what do I have to do to add a " Reviewed-by" from myself to this patch series? BR Martin
On Fri, 11 Oct 2024 at 13:22, Martin Stolpe <martinstolpe@gmail.com> wrote: > > Hi, > > Sorry if this is a stupid question but what do I have to do to add a " Reviewed-by" from myself to this patch series? Just reply all with the reviewed-by and patchwork will automatically apply it when I accept it, I think there needs to be a new v2 of this patch.
diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c index ab5ea85cf9..bc49708f85 100644 --- a/board/raspberrypi/rpi/rpi.c +++ b/board/raspberrypi/rpi/rpi.c @@ -572,7 +572,7 @@ int ft_board_setup(void *blob, struct bd_info *bd) node = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer"); if (node < 0) fdt_simplefb_add_node(blob); - else + else if (IS_ENABLED(CONFIG_VIDEO)) fdt_simplefb_enable_and_mem_rsv(blob); #ifdef CONFIG_EFI_LOADER diff --git a/boot/fdt_simplefb.c b/boot/fdt_simplefb.c index 5341554845..b6e916ff7d 100644 --- a/boot/fdt_simplefb.c +++ b/boot/fdt_simplefb.c @@ -86,6 +86,7 @@ int fdt_simplefb_add_node(void *blob) return fdt_simplefb_configure_node(blob, off); } +#if IS_ENABLED(CONFIG_VIDEO) /** * fdt_simplefb_enable_existing_node() - enable simple-framebuffer DT node * @@ -103,7 +104,6 @@ static int fdt_simplefb_enable_existing_node(void *blob) return fdt_simplefb_configure_node(blob, off); } -#if IS_ENABLED(CONFIG_VIDEO) int fdt_simplefb_enable_and_mem_rsv(void *blob) { int ret;
When the CONFIG_VIDEO option is set to disabled for Raspberry Pi devices the build will fail with the following error message: "undefined reference to `fdt_simplefb_enable_and_mem_rsv'." Signed-off-by: Martin Stolpe <martin.stolpe@gmail.com> --- board/raspberrypi/rpi/rpi.c | 2 +- boot/fdt_simplefb.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)