[PATCH 0/1] rpi: Fix compilation when CONFIG_VIDEO is disabled

[PATCH 0/1] rpi: Fix compilation when CONFIG_VIDEO is disabled

[Martin Stolpe]

This patch will fix a compilation error if CONFIG_VIDEO is set to disabled.


Martin Stolpe (1):
  rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

 board/raspberrypi/rpi/rpi.c | 2 +-
 boot/fdt_simplefb.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Martin Stolpe]

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);
 
 #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;
-- 
2.43.0

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Ivan T. Ivanov]

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.

>  
>  #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)
>  {

Perhaps it will be better if above check is moved inside this function
making it empty in case of CONFIG_VIDEO is not set. This way you will
not need to modify it users.

Regards,
Ivan

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Martin Stolpe]

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

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Devarsh Thakkar]

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
> 

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Martin Stolpe]

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 */
-- 
2.43.0

What do you think?

Regards
Martin

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Devarsh Thakkar]

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

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Martin Stolpe]

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

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Devarsh Thakkar]

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
> 

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Martin Stolpe]

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

Re: [PATCH 1/1] rpi: Fix build error when CONFIG_VIDEO is disabled for Raspberry Pi

[Peter Robinson]

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.