[PATCH 1/3] rockchip: Fix TPL link error

[PATCH 1/3] rockchip: Fix TPL link error

[FUKAUMI Naoki]

Rockchip RK3399 targets build TPL. If CONFIG_SPL_USB_DWC3_GENERIC and
CONFIG_SPL_USB_GADGET are enabled, linking u-boot-tpl will fail.

  LD      tpl/u-boot-tpl
ld.bfd: drivers/usb/dwc3/dwc3-generic.o: in function `dwc3_glue_bind_common':
/home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:527:(.text.dwc3_glue_bind_common+0x20): undefined reference to `usb_get_dr_mode'
/home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:527:(.text.dwc3_glue_bind_common+0x20): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
ld.bfd: /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:529:(.text.dwc3_glue_bind_common+0x2c): undefined reference to `usb_get_dr_mode'
/home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:529:(.text.dwc3_glue_bind_common+0x2c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
ld.bfd: drivers/usb/dwc3/dwc3-generic.o: in function `dwc3_glue_probe':
/home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:683:(.text.dwc3_glue_probe+0xb0): undefined reference to `usb_get_dr_mode'
/home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:683:(.text.dwc3_glue_probe+0xb0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'

Do not compile drivers/usb/ for TPL.

Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
 scripts/Makefile.xpl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.xpl b/scripts/Makefile.xpl
index abc49fbe6c9..e4ca4d395ac 100644
--- a/scripts/Makefile.xpl
+++ b/scripts/Makefile.xpl
@@ -125,8 +125,8 @@ endif
 
 libs-y += drivers/
 libs-$(CONFIG_SPL_MEMORY) += drivers/memory/
-libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/
-libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/
+libs-$(CONFIG_$(PHASE_)USB_GADGET) += drivers/usb/dwc3/
+libs-$(CONFIG_$(PHASE_)USB_GADGET) += drivers/usb/cdns3/
 libs-y += dts/
 libs-y += fs/
 libs-$(CONFIG_SPL_POST_MEM_SUPPORT) += post/drivers/
-- 
2.43.0

[PATCH 2/3] rockchip: Fix SPL link error for Radxa ROCK Pi 4

[FUKAUMI Naoki]

If CONFIG_EFI_CAPSULE_FIRMWARE_RAW, CONFIG_SPL_ENV_SUPPORT, and
CONFIG_SPL_DFU are enabled, linking u-boot-spl will fail.

  LD      spl/u-boot-spl
ld.bfd: drivers/dfu/dfu.o: in function `dfu_init_env_entities':
/home/radxa/u-boot/drivers/dfu/dfu.c:173:(.text.dfu_init_env_entities+0x24): undefined reference to `set_dfu_alt_info'

Compile mach-rockchip/board.c for SPL.

Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
 arch/arm/mach-rockchip/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
index 5e7edc99cdc..e697df7ebcb 100644
--- a/arch/arm/mach-rockchip/Makefile
+++ b/arch/arm/mach-rockchip/Makefile
@@ -22,10 +22,10 @@ ifeq ($(CONFIG_XPL_BUILD)$(CONFIG_TPL_BUILD),)
 # we can have the preprocessor correctly recognise both 0x0 and 0
 # meaning "turn it off".
 obj-y += boot_mode.o
-obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o
 endif
 
 ifeq ($(CONFIG_TPL_BUILD),)
+obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o
 obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o
 endif
 
-- 
2.43.0

Re: [PATCH 2/3] rockchip: Fix SPL link error for Radxa ROCK Pi 4

[Quentin Schulz]

Hi Naoki,

On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
> If CONFIG_EFI_CAPSULE_FIRMWARE_RAW, CONFIG_SPL_ENV_SUPPORT, and
> CONFIG_SPL_DFU are enabled, linking u-boot-spl will fail.
> 
>    LD      spl/u-boot-spl
> ld.bfd: drivers/dfu/dfu.o: in function `dfu_init_env_entities':
> /home/radxa/u-boot/drivers/dfu/dfu.c:173:(.text.dfu_init_env_entities+0x24): undefined reference to `set_dfu_alt_info'
> 
> Compile mach-rockchip/board.c for SPL.
> 
> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
> ---
>   arch/arm/mach-rockchip/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
> index 5e7edc99cdc..e697df7ebcb 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -22,10 +22,10 @@ ifeq ($(CONFIG_XPL_BUILD)$(CONFIG_TPL_BUILD),)
>   # we can have the preprocessor correctly recognise both 0x0 and 0
>   # meaning "turn it off".
>   obj-y += boot_mode.o
> -obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o
>   endif
>   
>   ifeq ($(CONFIG_TPL_BUILD),)
> +obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o

This is probably a tiny bit too enthusiastic.

This means the whole content of board.c will be compiled for SPL whereas 
it currently is compiled only for proper.

This means we need to be careful about what's in board.c.

Most IS_ENABLED(CONFIG_<config>) should probably be changed to 
CONFIG_IS_ENABLED(<config>) so that it handles SPL symbols too and 
doesn't attempt to build stuff for SPL when only proper symbols are 
selected (and not their SPL counterpart).

E.g.

- EFI_PARTITION
- SYS_DCACHE_OFF
- USB_GADGET
- DM_USB_GADGET

seems to have SPL symbols too, but

- EFI_HAVE_CAPSULE_SUPPORT
- ROCKCHIP_USB2_PHY
- FASTBOOT
- MISC_INIT_R
- ROCKCHIP_EFUSE (though does only build for proper (see the use of 
$(XPL_) in Makefile))
- ROCKCHIP_OTP (though does only build for proper (see the use of 
$(XPL_) in Makefile))
- BOARD_RNG_SEED
- RNG_ROCKCHIP

only have proper symbols.

Maybe it makes more sense to move the efi-specific part of board.c into 
efi.c and build that one whenever we actually need it?

What do you think?

Cheers,
Quentin

Re: [PATCH 2/3] rockchip: Fix SPL link error for Radxa ROCK Pi 4

[Jonas Karlman]

Hi Quentin and Naoki,

On 2025-02-05 12:37, Quentin Schulz wrote:
> Hi Naoki,
> 
> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
>> If CONFIG_EFI_CAPSULE_FIRMWARE_RAW, CONFIG_SPL_ENV_SUPPORT, and
>> CONFIG_SPL_DFU are enabled, linking u-boot-spl will fail.
>>
>>    LD      spl/u-boot-spl
>> ld.bfd: drivers/dfu/dfu.o: in function `dfu_init_env_entities':
>> /home/radxa/u-boot/drivers/dfu/dfu.c:173:(.text.dfu_init_env_entities+0x24): undefined reference to `set_dfu_alt_info'
>>
>> Compile mach-rockchip/board.c for SPL.
>>
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> ---
>>   arch/arm/mach-rockchip/Makefile | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
>> index 5e7edc99cdc..e697df7ebcb 100644
>> --- a/arch/arm/mach-rockchip/Makefile
>> +++ b/arch/arm/mach-rockchip/Makefile
>> @@ -22,10 +22,10 @@ ifeq ($(CONFIG_XPL_BUILD)$(CONFIG_TPL_BUILD),)
>>   # we can have the preprocessor correctly recognise both 0x0 and 0
>>   # meaning "turn it off".
>>   obj-y += boot_mode.o
>> -obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o
>>   endif
>>   
>>   ifeq ($(CONFIG_TPL_BUILD),)
>> +obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o
> 
> This is probably a tiny bit too enthusiastic.
> 
> This means the whole content of board.c will be compiled for SPL whereas 
> it currently is compiled only for proper.
> 
> This means we need to be careful about what's in board.c.
> 
> Most IS_ENABLED(CONFIG_<config>) should probably be changed to 
> CONFIG_IS_ENABLED(<config>) so that it handles SPL symbols too and 
> doesn't attempt to build stuff for SPL when only proper symbols are 
> selected (and not their SPL counterpart).
> 
> E.g.
> 
> - EFI_PARTITION
> - SYS_DCACHE_OFF
> - USB_GADGET
> - DM_USB_GADGET
> 
> seems to have SPL symbols too, but
> 
> - EFI_HAVE_CAPSULE_SUPPORT
> - ROCKCHIP_USB2_PHY
> - FASTBOOT
> - MISC_INIT_R
> - ROCKCHIP_EFUSE (though does only build for proper (see the use of 
> $(XPL_) in Makefile))
> - ROCKCHIP_OTP (though does only build for proper (see the use of 
> $(XPL_) in Makefile))
> - BOARD_RNG_SEED
> - RNG_ROCKCHIP
> 
> only have proper symbols.
> 
> Maybe it makes more sense to move the efi-specific part of board.c into 
> efi.c and build that one whenever we actually need it?
> 
> What do you think?

I fully agree, please move the efi-specific parts away from board.c.

In current state it is only the rock-pi-4c-rk3399 target that have use
of set_dfu_alt_info(), rockchip_capsule_update_board_setup() is broken
for rock-pi-4-rk3399, the code check for radxa,rockpi4b, and board
target use radxa,rockpi4a :-)

Regards,
Jonas

> 
> Cheers,
> Quentin

[PATCH 3/3] rockchip: Fix binman error

[FUKAUMI Naoki]

After adding the below configs, binman produces an error.

 CONFIG_SPL_DFU
 CONFIG_SPL_ENV_SUPPORT
 CONFIG_SPL_USB_DWC3_GENERIC
 CONFIG_SPL_USB_GADGET

binman: Section '/binman/simple-bin-spi': Symbol '_binman_u_boot_any_prop_size'
   in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl-nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot-tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,mkimage,fit,simple-bin-spi)

Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.

Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
 arch/arm/dts/rk3399-u-boot.dtsi   | 2 ++
 arch/arm/dts/rockchip-u-boot.dtsi | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
index 70f35b6c197..0e3143f8235 100644
--- a/arch/arm/dts/rk3399-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-u-boot.dtsi
@@ -45,9 +45,11 @@
 			};
 #elif defined(CONFIG_TPL)
 			u-boot-tpl {
+				no-write-symbols;
 			};
 #endif
 			u-boot-spl {
+				no-write-symbols;
 			};
 		};
 		fit {
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index c8c928c7e50..fd005a24890 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -27,9 +27,11 @@
 			};
 #elif defined(CONFIG_TPL)
 			u-boot-tpl {
+				no-write-symbols;
 			};
 #endif
 			u-boot-spl {
+				no-write-symbols;
 			};
 		};
 
@@ -187,9 +189,11 @@
 			};
 #elif defined(CONFIG_TPL)
 			u-boot-tpl {
+				no-write-symbols;
 			};
 #endif
 			u-boot-spl {
+				no-write-symbols;
 			};
 		};
 
-- 
2.43.0

Re: [PATCH 3/3] rockchip: Fix binman error

[Quentin Schulz]

Hi Naoki,

On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
> After adding the below configs, binman produces an error.
> 
>   CONFIG_SPL_DFU
>   CONFIG_SPL_ENV_SUPPORT
>   CONFIG_SPL_USB_DWC3_GENERIC
>   CONFIG_SPL_USB_GADGET
> 
> binman: Section '/binman/simple-bin-spi': Symbol '_binman_u_boot_any_prop_size'
>     in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl-nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot-tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,mkimage,fit,simple-bin-spi)
> 
> Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.
> 

I'll be frank with you, I have no clue what's happening :)

But, it seems this only applies to the SPI image (disable 
ROCKCHIP_SPI_IMAGE and it'll built just fine). Why? No clue.

I've never liked how the simple-bin-spi was built because we should be 
reusing artifacts from simple-bin instead of rebuilding stuff. That's 
why you have /binman/simple-bin-spi/mkimage/fit being a blob, because we 
want to reuse the fitImage from /binman/simple-bin/mkimage/fit but we 
didn't do this for the other ones.

Can you try to revert your patch here and use this instead:

"""
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi 
b/arch/arm/dts/rk3399-u-boot.dtsi
index 70f35b6c197..0a5771b706f 100644
--- a/arch/arm/dts/rk3399-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-u-boot.dtsi
@@ -45,9 +45,15 @@
  			};
  #elif defined(CONFIG_TPL)
  			u-boot-tpl {
+				/* sync with /binman/simple-bin/mkimage/u-boot-tpl */
+				type = "blob";
+				filename = "tpl/u-boot-tpl.bin";
  			};
  #endif
  			u-boot-spl {
+				/* sync with /binman/simple-bin/mkimage/u-boot-spl */
+				type = "blob";
+				filename = "spl/u-boot-spl.bin";
  			};
  		};
  		fit {
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi 
b/arch/arm/dts/rockchip-u-boot.dtsi
index c8c928c7e50..fea49eb3145 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -187,9 +187,15 @@
  			};
  #elif defined(CONFIG_TPL)
  			u-boot-tpl {
+				/* sync with /binman/simple-bin/mkimage/u-boot-tpl */
+				type = "blob";
+				filename = "tpl/u-boot-tpl.bin";
  			};
  #endif
  			u-boot-spl {
+				/* sync with /binman/simple-bin/mkimage/u-boot-spl */
+				type = "blob";
+				filename = "spl/u-boot-spl.bin";
  			};
  		};
"""

This seems to work for me on RK3399 Puma and Rock-Pi-4 builds with the 
symbols you listed enabled. I also believe this should be "better" than 
what we have today.

If that works, I'll send a proper patch for this :)

Cheers,
Quentin

Re: [PATCH 3/3] rockchip: Fix binman error

[Simon Glass]

Hi Quentin,

On Wed, 5 Feb 2025 at 06:00, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Naoki,
>
> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
> > After adding the below configs, binman produces an error.
> >
> >   CONFIG_SPL_DFU
> >   CONFIG_SPL_ENV_SUPPORT
> >   CONFIG_SPL_USB_DWC3_GENERIC
> >   CONFIG_SPL_USB_GADGET
> >
> > binman: Section '/binman/simple-bin-spi': Symbol '_binman_u_boot_any_prop_size'
> >     in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl-nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot-tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,mkimage,fit,simple-bin-spi)
> >
> > Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.
> >
>
> I'll be frank with you, I have no clue what's happening :)

I means that simple-bin-spi has a binary inside it, u-boot-spl, which
uses binman_sym_declare(ulong, u_boot_any, size). Most likely it is
the one at the top of common/spl.c

This symbol is filled in (by binman, at build-time) with the size of
the U-Boot binary. If there is no such U-Boot image then it cannot do
this.

There is more information at [1]

>
> But, it seems this only applies to the SPI image (disable
> ROCKCHIP_SPI_IMAGE and it'll built just fine). Why? No clue.
>
> I've never liked how the simple-bin-spi was built because we should be
> reusing artifacts from simple-bin instead of rebuilding stuff. That's
> why you have /binman/simple-bin-spi/mkimage/fit being a blob, because we
> want to reuse the fitImage from /binman/simple-bin/mkimage/fit but we
> didn't do this for the other ones.
>
> Can you try to revert your patch here and use this instead:
>
> """
> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi
> b/arch/arm/dts/rk3399-u-boot.dtsi
> index 70f35b6c197..0a5771b706f 100644
> --- a/arch/arm/dts/rk3399-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
> @@ -45,9 +45,15 @@
>                         };
>   #elif defined(CONFIG_TPL)
>                         u-boot-tpl {
> +                               /* sync with /binman/simple-bin/mkimage/u-boot-tpl */
> +                               type = "blob";
> +                               filename = "tpl/u-boot-tpl.bin";
>                         };
>   #endif
>                         u-boot-spl {
> +                               /* sync with /binman/simple-bin/mkimage/u-boot-spl */
> +                               type = "blob";
> +                               filename = "spl/u-boot-spl.bin";
>                         };
>                 };
>                 fit {
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi
> b/arch/arm/dts/rockchip-u-boot.dtsi
> index c8c928c7e50..fea49eb3145 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -187,9 +187,15 @@
>                         };
>   #elif defined(CONFIG_TPL)
>                         u-boot-tpl {
> +                               /* sync with /binman/simple-bin/mkimage/u-boot-tpl */
> +                               type = "blob";
> +                               filename = "tpl/u-boot-tpl.bin";
>                         };
>   #endif
>                         u-boot-spl {
> +                               /* sync with /binman/simple-bin/mkimage/u-boot-spl */
> +                               type = "blob";
> +                               filename = "spl/u-boot-spl.bin";
>                         };
>                 };
> """
>
> This seems to work for me on RK3399 Puma and Rock-Pi-4 builds with the
> symbols you listed enabled. I also believe this should be "better" than
> what we have today.
>
> If that works, I'll send a proper patch for this :)

Regards,
Simon

[1] https://docs.u-boot.org/en/latest/develop/package/binman.html#access-to-binman-entry-offsets-at-run-time-symbols

Re: [PATCH 3/3] rockchip: Fix binman error

[Quentin Schulz]

Hi Simon,

On 2/6/25 1:46 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 5 Feb 2025 at 06:00, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Naoki,
>>
>> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
>>> After adding the below configs, binman produces an error.
>>>
>>>    CONFIG_SPL_DFU
>>>    CONFIG_SPL_ENV_SUPPORT
>>>    CONFIG_SPL_USB_DWC3_GENERIC
>>>    CONFIG_SPL_USB_GADGET
>>>
>>> binman: Section '/binman/simple-bin-spi': Symbol '_binman_u_boot_any_prop_size'
>>>      in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl-nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot-tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,mkimage,fit,simple-bin-spi)
>>>
>>> Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.
>>>
>>
>> I'll be frank with you, I have no clue what's happening :)
> 
> I means that simple-bin-spi has a binary inside it, u-boot-spl, which
> uses binman_sym_declare(ulong, u_boot_any, size). Most likely it is
> the one at the top of common/spl.c
> 
> This symbol is filled in (by binman, at build-time) with the size of
> the U-Boot binary. If there is no such U-Boot image then it cannot do
> this.
> 

Why is it not failing right now? Why does it fail when you enable 
CONFIG_SPL_DFU, CONFIG_SPL_ENV_SUPPORT, CONFIG_SPL_USB_DWC3_GENERIC, 
CONFIG_SPL_USB_GADGET for Rock-Pi-4? This doesn't generate the 
u-boot.itb file we need for simple-bin-spi as we anyway take the fit 
blob generated by simple-bin instead.

It would make sense to always fail if this relies on u-boot.itb to be 
built for simple-bin-spi as well?

I suggested to use only blobs from simple-bin in simple-bin-spi but 
according to 
https://docs.u-boot.org/en/latest/develop/package/binman.html#image-dependencies 
this is undefined behavior. Maybe we should use a template 
(https://docs.u-boot.org/en/latest/develop/package/binman.html#templates) 
and rebuild everything for the SPI image as well. I saw such a patch in 
your VBE part H series I believe, maybe it makes sense to have this 
merged separately?

This feels somewhat wrong though, if I'm not mistaken every binary used 
in simple-bin and simple-bin-spi is the same, to the exception of how 
the idbloader.img is generated (-T rkspi/rksd), so it doesn't make a lot 
of sense to me to go through a complete rebuild of all binaries, instead 
of just running mkimage a second time with the same set of binaries. But 
if that's the only way to have something with defined behavior AND 
working, then so be it.

Cheers,
Quentin

Re: [PATCH 3/3] rockchip: Fix binman error

[Simon Glass]

Hi Quentin,

On Thu, 6 Feb 2025 at 09:54, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 2/6/25 1:46 PM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 5 Feb 2025 at 06:00, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Naoki,
> >>
> >> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
> >>> After adding the below configs, binman produces an error.
> >>>
> >>>    CONFIG_SPL_DFU
> >>>    CONFIG_SPL_ENV_SUPPORT
> >>>    CONFIG_SPL_USB_DWC3_GENERIC
> >>>    CONFIG_SPL_USB_GADGET
> >>>
> >>> binman: Section '/binman/simple-bin-spi': Symbol '_binman_u_boot_any_prop_size'
> >>>      in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl-nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot-tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,mkimage,fit,simple-bin-spi)
> >>>
> >>> Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.
> >>>
> >>
> >> I'll be frank with you, I have no clue what's happening :)
> >
> > I means that simple-bin-spi has a binary inside it, u-boot-spl, which
> > uses binman_sym_declare(ulong, u_boot_any, size). Most likely it is
> > the one at the top of common/spl.c
> >
> > This symbol is filled in (by binman, at build-time) with the size of
> > the U-Boot binary. If there is no such U-Boot image then it cannot do
> > this.
> >
>
> Why is it not failing right now? Why does it fail when you enable
> CONFIG_SPL_DFU, CONFIG_SPL_ENV_SUPPORT, CONFIG_SPL_USB_DWC3_GENERIC,
> CONFIG_SPL_USB_GADGET for Rock-Pi-4?

I added these to the end of configs/rock-pi-4-rk3399_defconfig

CONFIG_SPL_DFU=y
CONFIG_SPL_ENV_SUPPORT=y
CONFIG_SPL_USB_DWC3_GENERIC=y
CONFIG_SPL_USB_GADGET=y
CONFIG_DM_USB=y

then build and don't see these errors. Can you please give a little
more detail so I can repeat this?

> This doesn't generate the
> u-boot.itb file we need for simple-bin-spi as we anyway take the fit
> blob generated by simple-bin instead.
>
> It would make sense to always fail if this relies on u-boot.itb to be
> built for simple-bin-spi as well?
>
> I suggested to use only blobs from simple-bin in simple-bin-spi but
> according to
> https://docs.u-boot.org/en/latest/develop/package/binman.html#image-dependencies
> this is undefined behavior. Maybe we should use a template
> (https://docs.u-boot.org/en/latest/develop/package/binman.html#templates)
> and rebuild everything for the SPI image as well. I saw such a patch in
> your VBE part H series I believe, maybe it makes sense to have this
> merged separately?

Yes I am adding a template in that series, but I would rather get that
submitted, since Binman does currently process images in series.

>
> This feels somewhat wrong though, if I'm not mistaken every binary used
> in simple-bin and simple-bin-spi is the same, to the exception of how
> the idbloader.img is generated (-T rkspi/rksd), so it doesn't make a lot
> of sense to me to go through a complete rebuild of all binaries, instead
> of just running mkimage a second time with the same set of binaries. But
> if that's the only way to have something with defined behavior AND
> working, then so be it.

For now at least I think it is best to keep the images independent,
with a template to reduce the duplication.

Regards.

Simon

Re: [PATCH 3/3] rockchip: Fix binman error

[FUKAUMI Naoki]

Hi,

On 2/9/25 23:27, Simon Glass wrote:
> Hi Quentin,
> 
> On Thu, 6 Feb 2025 at 09:54, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Simon,
>>
>> On 2/6/25 1:46 PM, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Wed, 5 Feb 2025 at 06:00, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Naoki,
>>>>
>>>> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
>>>>> After adding the below configs, binman produces an error.
>>>>>
>>>>>     CONFIG_SPL_DFU
>>>>>     CONFIG_SPL_ENV_SUPPORT
>>>>>     CONFIG_SPL_USB_DWC3_GENERIC
>>>>>     CONFIG_SPL_USB_GADGET
>>>>>
>>>>> binman: Section '/binman/simple-bin-spi': Symbol '_binman_u_boot_any_prop_size'
>>>>>       in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl-nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot-tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,mkimage,fit,simple-bin-spi)
>>>>>
>>>>> Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.
>>>>>
>>>>
>>>> I'll be frank with you, I have no clue what's happening :)
>>>
>>> I means that simple-bin-spi has a binary inside it, u-boot-spl, which
>>> uses binman_sym_declare(ulong, u_boot_any, size). Most likely it is
>>> the one at the top of common/spl.c
>>>
>>> This symbol is filled in (by binman, at build-time) with the size of
>>> the U-Boot binary. If there is no such U-Boot image then it cannot do
>>> this.
>>>
>>
>> Why is it not failing right now? Why does it fail when you enable
>> CONFIG_SPL_DFU, CONFIG_SPL_ENV_SUPPORT, CONFIG_SPL_USB_DWC3_GENERIC,
>> CONFIG_SPL_USB_GADGET for Rock-Pi-4?
> 
> I added these to the end of configs/rock-pi-4-rk3399_defconfig
> 
> CONFIG_SPL_DFU=y
> CONFIG_SPL_ENV_SUPPORT=y
> CONFIG_SPL_USB_DWC3_GENERIC=y
> CONFIG_SPL_USB_GADGET=y
> CONFIG_DM_USB=y
> 
> then build and don't see these errors. Can you please give a little
> more detail so I can repeat this?

This occurs when adding the above CONFIG to the following defconfig and 
building on today's master and next branch.

  rock-4c-plus-rk3399_defconfig
  rock-4se-rk3399_defconfig
  rock-pi-4-rk3399_defconfig
  rock-pi-4c-rk3399_defconfig

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

>> This doesn't generate the
>> u-boot.itb file we need for simple-bin-spi as we anyway take the fit
>> blob generated by simple-bin instead.
>>
>> It would make sense to always fail if this relies on u-boot.itb to be
>> built for simple-bin-spi as well?
>>
>> I suggested to use only blobs from simple-bin in simple-bin-spi but
>> according to
>> https://docs.u-boot.org/en/latest/develop/package/binman.html#image-dependencies
>> this is undefined behavior. Maybe we should use a template
>> (https://docs.u-boot.org/en/latest/develop/package/binman.html#templates)
>> and rebuild everything for the SPI image as well. I saw such a patch in
>> your VBE part H series I believe, maybe it makes sense to have this
>> merged separately?
> 
> Yes I am adding a template in that series, but I would rather get that
> submitted, since Binman does currently process images in series.
> 
>>
>> This feels somewhat wrong though, if I'm not mistaken every binary used
>> in simple-bin and simple-bin-spi is the same, to the exception of how
>> the idbloader.img is generated (-T rkspi/rksd), so it doesn't make a lot
>> of sense to me to go through a complete rebuild of all binaries, instead
>> of just running mkimage a second time with the same set of binaries. But
>> if that's the only way to have something with defined behavior AND
>> working, then so be it.
> 
> For now at least I think it is best to keep the images independent,
> with a template to reduce the duplication.
> 
> Regards.
> 
> Simon
> 

Re: [PATCH 3/3] rockchip: Fix binman error

[FUKAUMI Naoki]

Hi Quentin,

On 2/5/25 21:59, Quentin Schulz wrote:
> Hi Naoki,
> 
> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
>> After adding the below configs, binman produces an error.
>>
>>   CONFIG_SPL_DFU
>>   CONFIG_SPL_ENV_SUPPORT
>>   CONFIG_SPL_USB_DWC3_GENERIC
>>   CONFIG_SPL_USB_GADGET
>>
>> binman: Section '/binman/simple-bin-spi': Symbol 
>> '_binman_u_boot_any_prop_size'
>>     in entry '/binman/simple-bin-spi/mkimage/u-boot-spl/u-boot-spl- 
>> nodtb': Entry 'u-boot-any' not found in list (u-boot-tpl-nodtb,u-boot- 
>> tpl-dtb,u-boot-tpl,u-boot-spl-nodtb,u-boot-spl-dtb,u-boot- 
>> spl,mkimage,fit,simple-bin-spi)
>>
>> Add "no-write-symbols;" to u-boot-tpl and u-boot-spl.
>>
> 
> I'll be frank with you, I have no clue what's happening :)
> 
> But, it seems this only applies to the SPI image (disable 
> ROCKCHIP_SPI_IMAGE and it'll built just fine). Why? No clue.
> 
> I've never liked how the simple-bin-spi was built because we should be 
> reusing artifacts from simple-bin instead of rebuilding stuff. That's 
> why you have /binman/simple-bin-spi/mkimage/fit being a blob, because we 
> want to reuse the fitImage from /binman/simple-bin/mkimage/fit but we 
> didn't do this for the other ones.
> 
> Can you try to revert your patch here and use this instead:
> 
> """
> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u- 
> boot.dtsi
> index 70f35b6c197..0a5771b706f 100644
> --- a/arch/arm/dts/rk3399-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
> @@ -45,9 +45,15 @@
>               };
>   #elif defined(CONFIG_TPL)
>               u-boot-tpl {
> +                /* sync with /binman/simple-bin/mkimage/u-boot-tpl */
> +                type = "blob";
> +                filename = "tpl/u-boot-tpl.bin";
>               };
>   #endif
>               u-boot-spl {
> +                /* sync with /binman/simple-bin/mkimage/u-boot-spl */
> +                type = "blob";
> +                filename = "spl/u-boot-spl.bin";
>               };
>           };
>           fit {
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip- 
> u-boot.dtsi
> index c8c928c7e50..fea49eb3145 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -187,9 +187,15 @@
>               };
>   #elif defined(CONFIG_TPL)
>               u-boot-tpl {
> +                /* sync with /binman/simple-bin/mkimage/u-boot-tpl */
> +                type = "blob";
> +                filename = "tpl/u-boot-tpl.bin";
>               };
>   #endif
>               u-boot-spl {
> +                /* sync with /binman/simple-bin/mkimage/u-boot-spl */
> +                type = "blob";
> +                filename = "spl/u-boot-spl.bin";
>               };
>           };
> """
> 
> This seems to work for me on RK3399 Puma and Rock-Pi-4 builds with the 
> symbols you listed enabled. I also believe this should be "better" than 
> what we have today.
> 
> If that works, I'll send a proper patch for this :)

Proper patch please :)

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Cheers,
> Quentin
> 

Re: [PATCH 1/3] rockchip: Fix TPL link error

[Quentin Schulz]

Hi Naoki,

On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
> Rockchip RK3399 targets build TPL. If CONFIG_SPL_USB_DWC3_GENERIC and
> CONFIG_SPL_USB_GADGET are enabled, linking u-boot-tpl will fail.
> 
>    LD      tpl/u-boot-tpl
> ld.bfd: drivers/usb/dwc3/dwc3-generic.o: in function `dwc3_glue_bind_common':
> /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:527:(.text.dwc3_glue_bind_common+0x20): undefined reference to `usb_get_dr_mode'
> /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:527:(.text.dwc3_glue_bind_common+0x20): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
> ld.bfd: /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:529:(.text.dwc3_glue_bind_common+0x2c): undefined reference to `usb_get_dr_mode'
> /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:529:(.text.dwc3_glue_bind_common+0x2c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
> ld.bfd: drivers/usb/dwc3/dwc3-generic.o: in function `dwc3_glue_probe':
> /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:683:(.text.dwc3_glue_probe+0xb0): undefined reference to `usb_get_dr_mode'
> /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:683:(.text.dwc3_glue_probe+0xb0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
> 
> Do not compile drivers/usb/ for TPL.
> 
> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
> ---
>   scripts/Makefile.xpl | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/Makefile.xpl b/scripts/Makefile.xpl
> index abc49fbe6c9..e4ca4d395ac 100644
> --- a/scripts/Makefile.xpl
> +++ b/scripts/Makefile.xpl
> @@ -125,8 +125,8 @@ endif
>   
>   libs-y += drivers/
>   libs-$(CONFIG_SPL_MEMORY) += drivers/memory/
> -libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/
> -libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/
> +libs-$(CONFIG_$(PHASE_)USB_GADGET) += drivers/usb/dwc3/
> +libs-$(CONFIG_$(PHASE_)USB_GADGET) += drivers/usb/cdns3/

I don't know what this is actually used for? Like why is it different 
from the typical Makefile we have for other subsystems and drivers?

Can't we simply remove those and add them in drivers/Makefile, like:

obj-$(CONFIG_$(PHASE_)USB_GADGET) += usb/dwc3/
obj-$(CONFIG_$(PHASE_)USB_GADGET) += usb/cdns3/

For example?

Otherwise makes sense to me.

Cheers,
Quentin

Re: [PATCH 1/3] rockchip: Fix TPL link error

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]

On Wed, Jan 29, 2025 at 03:16:13PM +0100, Quentin Schulz wrote:
> Hi Naoki,
> 
> On 1/29/25 2:25 PM, FUKAUMI Naoki wrote:
> > Rockchip RK3399 targets build TPL. If CONFIG_SPL_USB_DWC3_GENERIC and
> > CONFIG_SPL_USB_GADGET are enabled, linking u-boot-tpl will fail.
> > 
> >    LD      tpl/u-boot-tpl
> > ld.bfd: drivers/usb/dwc3/dwc3-generic.o: in function `dwc3_glue_bind_common':
> > /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:527:(.text.dwc3_glue_bind_common+0x20): undefined reference to `usb_get_dr_mode'
> > /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:527:(.text.dwc3_glue_bind_common+0x20): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
> > ld.bfd: /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:529:(.text.dwc3_glue_bind_common+0x2c): undefined reference to `usb_get_dr_mode'
> > /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:529:(.text.dwc3_glue_bind_common+0x2c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
> > ld.bfd: drivers/usb/dwc3/dwc3-generic.o: in function `dwc3_glue_probe':
> > /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:683:(.text.dwc3_glue_probe+0xb0): undefined reference to `usb_get_dr_mode'
> > /home/radxa/u-boot/drivers/usb/dwc3/dwc3-generic.c:683:(.text.dwc3_glue_probe+0xb0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `usb_get_dr_mode'
> > 
> > Do not compile drivers/usb/ for TPL.
> > 
> > Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
> > ---
> >   scripts/Makefile.xpl | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/Makefile.xpl b/scripts/Makefile.xpl
> > index abc49fbe6c9..e4ca4d395ac 100644
> > --- a/scripts/Makefile.xpl
> > +++ b/scripts/Makefile.xpl
> > @@ -125,8 +125,8 @@ endif
> >   libs-y += drivers/
> >   libs-$(CONFIG_SPL_MEMORY) += drivers/memory/
> > -libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/dwc3/
> > -libs-$(CONFIG_SPL_USB_GADGET) += drivers/usb/cdns3/
> > +libs-$(CONFIG_$(PHASE_)USB_GADGET) += drivers/usb/dwc3/
> > +libs-$(CONFIG_$(PHASE_)USB_GADGET) += drivers/usb/cdns3/
> 
> I don't know what this is actually used for? Like why is it different from
> the typical Makefile we have for other subsystems and drivers?
> 
> Can't we simply remove those and add them in drivers/Makefile, like:
> 
> obj-$(CONFIG_$(PHASE_)USB_GADGET) += usb/dwc3/
> obj-$(CONFIG_$(PHASE_)USB_GADGET) += usb/cdns3/
> 
> For example?

The likely answer is that we used to not have consistent CONFIG naming
here between SPL support and non-SPL support, and while that's since
been corrected, this part was never updated to take advantage of that.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]