diff mbox series

[1/4] boot/arm-trusted-firmware: Disable bin copy for rk3399

Message ID 1581573151-18943-2-git-send-email-sunil@amarulasolutions.com
State Superseded
Headers show
Series Fix ATF v2.2 build for rk3399, add roc-rk3399-pc board | expand

Commit Message

Suniel Mahesh Feb. 13, 2020, 5:52 a.m. UTC 
From: Jagan Teki <jagan@amarulasolutions.com>

Unlike other SoC platforms, rockchip platforms doesn't require a binary
generation on TF-A project.

This is due to rockchip platforms have non-continuous memory areas in
the linker script with a huge gap between them, so generating the binary
would require addition padding which indeed increases the size of the binary.

Interestingly this binary generation is disabled in v2.2 of TF-A on below
commit:
 commit <33218d2a8143> "rockchip: Disable binary generation for all SoCs."

So, select default binary boot images as *.bin only if it's not rk3399.

This fixes the atf build on rk3399 with v2.2.

Note: the same can be applied to rest of rockchip platforms if
they use v2.2 TF-A.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Suniel Mahesh <sunil@amarulasolutions.com>
---
 boot/arm-trusted-firmware/Config.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sergey Matyukevich Feb. 13, 2020, 7:23 a.m. UTC | #1 
Hi, 

> Unlike other SoC platforms, rockchip platforms doesn't require a binary
> generation on TF-A project.
> 
> This is due to rockchip platforms have non-continuous memory areas in
> the linker script with a huge gap between them, so generating the binary
> would require addition padding which indeed increases the size of the binary.
> 
> Interestingly this binary generation is disabled in v2.2 of TF-A on below
> commit:
>  commit <33218d2a8143> "rockchip: Disable binary generation for all SoCs."
> 
> So, select default binary boot images as *.bin only if it's not rk3399.
> 
> This fixes the atf build on rk3399 with v2.2.
> 
> Note: the same can be applied to rest of rockchip platforms if
> they use v2.2 TF-A.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Tested-by: Suniel Mahesh <sunil@amarulasolutions.com>
> ---
>  boot/arm-trusted-firmware/Config.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

...

>  config BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES
>  	string "Binary boot images"
> -	default "*.bin"
> +	default "*.bin" if BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM!=rk3399
>  	help
>  	  Names of generated image files that are installed in the
>  	  output images/ directory.

IIUC, it should be customized in your board defconfig.

Regards,
Sergey
Jagan Teki Feb. 13, 2020, 8:06 a.m. UTC | #2 
On Thu, Feb 13, 2020 at 12:53 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> Hi,
>
> > Unlike other SoC platforms, rockchip platforms doesn't require a binary
> > generation on TF-A project.
> >
> > This is due to rockchip platforms have non-continuous memory areas in
> > the linker script with a huge gap between them, so generating the binary
> > would require addition padding which indeed increases the size of the binary.
> >
> > Interestingly this binary generation is disabled in v2.2 of TF-A on below
> > commit:
> >  commit <33218d2a8143> "rockchip: Disable binary generation for all SoCs."
> >
> > So, select default binary boot images as *.bin only if it's not rk3399.
> >
> > This fixes the atf build on rk3399 with v2.2.
> >
> > Note: the same can be applied to rest of rockchip platforms if
> > they use v2.2 TF-A.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > Tested-by: Suniel Mahesh <sunil@amarulasolutions.com>
> > ---
> >  boot/arm-trusted-firmware/Config.in | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> ...
>
> >  config BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES
> >       string "Binary boot images"
> > -     default "*.bin"
> > +     default "*.bin" if BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM!=rk3399
> >       help
> >         Names of generated image files that are installed in the
> >         output images/ directory.
>
> IIUC, it should be customized in your board defconfig.

Not sure of it. Technically this is how rockchip SoC behaves wrt ATF
build. If we mark BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES="" to all
supporting boards of  rockchip start with rk3399, rk3288 etc.. it
ended up huge and meaningless change, IMHO.

Jagan.
Thomas Petazzoni Feb. 14, 2020, 4:20 a.m. UTC | #3 
On Thu, 13 Feb 2020 13:36:53 +0530
Jagan Teki <jagan@amarulasolutions.com> wrote:

> > >  config BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES
> > >       string "Binary boot images"
> > > -     default "*.bin"
> > > +     default "*.bin" if BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM!=rk3399
> > >       help
> > >         Names of generated image files that are installed in the
> > >         output images/ directory.  
> >
> > IIUC, it should be customized in your board defconfig.  
> 
> Not sure of it. Technically this is how rockchip SoC behaves wrt ATF
> build. If we mark BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES="" to all
> supporting boards of  rockchip start with rk3399, rk3288 etc.. it
> ended up huge and meaningless change, IMHO.

Sergey is right here: I don't think we want to encode platform-specific
stuff like this in Kconfig files.

What is the problem if you leave this to the default of *.bin ?

Best regards,

Thomas
Jagan Teki Feb. 14, 2020, 6:55 a.m. UTC | #4 
On Fri, Feb 14, 2020 at 9:50 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Thu, 13 Feb 2020 13:36:53 +0530
> Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> > > >  config BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES
> > > >       string "Binary boot images"
> > > > -     default "*.bin"
> > > > +     default "*.bin" if BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM!=rk3399
> > > >       help
> > > >         Names of generated image files that are installed in the
> > > >         output images/ directory.
> > >
> > > IIUC, it should be customized in your board defconfig.
> >
> > Not sure of it. Technically this is how rockchip SoC behaves wrt ATF
> > build. If we mark BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES="" to all
> > supporting boards of  rockchip start with rk3399, rk3288 etc.. it
> > ended up huge and meaningless change, IMHO.
>
> Sergey is right here: I don't think we want to encode platform-specific
> stuff like this in Kconfig files.
>
> What is the problem if you leave this to the default of *.bin ?

Is looking for *.bin in build/rk3399/release/*.bin but v2.2 is not
creating any bin since rk3399 (or rockchip) doesn't need bins.

.mk file change:

define ARM_TRUSTED_FIRMWARE_INSTALL_IMAGES_CMDS
        $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
                cp -dpf $(ARM_TRUSTED_FIRMWARE_IMG_DIR)/$(f) $(BINARIES_DIR)/
        )

Build log:

>>> arm-trusted-firmware v2.2 Installing to images directory
cp -dpf /mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin
/mnt/out/images/
cp: cannot stat
'/mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin':
No such file or directory
package/pkg-generic.mk:339: recipe for target
'/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed'
failed
make[1]: *** [/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed]
Error 1
Makefile:23: recipe for target '_all' failed
make: *** [_all] Error 2

So, we have tried 3 set of changes

1. Kconfig (like in this patch)
2. *.mk check for rk3399, and v2.2 to not to execute above foreach loop
3. defined BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES="" board defconfig

Out of the above 3 possibilities, option 1) seem meaningful for me.
Let us know your comments?

Jagan.
Thomas Petazzoni Feb. 14, 2020, 7:26 a.m. UTC | #5 
On Fri, 14 Feb 2020 12:25:37 +0530
Jagan Teki <jagan@amarulasolutions.com> wrote:

> > Sergey is right here: I don't think we want to encode platform-specific
> > stuff like this in Kconfig files.
> >
> > What is the problem if you leave this to the default of *.bin ?  
> 
> Is looking for *.bin in build/rk3399/release/*.bin but v2.2 is not
> creating any bin since rk3399 (or rockchip) doesn't need bins.
> 
> .mk file change:
> 
> define ARM_TRUSTED_FIRMWARE_INSTALL_IMAGES_CMDS
>         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
>                 cp -dpf $(ARM_TRUSTED_FIRMWARE_IMG_DIR)/$(f) $(BINARIES_DIR)/
>         )
> 
> Build log:
> 
> >>> arm-trusted-firmware v2.2 Installing to images directory  
> cp -dpf /mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin
> /mnt/out/images/
> cp: cannot stat
> '/mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin':
> No such file or directory
> package/pkg-generic.mk:339: recipe for target
> '/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed'
> failed
> make[1]: *** [/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed]
> Error 1
> Makefile:23: recipe for target '_all' failed
> make: *** [_all] Error 2

What is your image name then? You don't install any image?

One possibility is to change:

-         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
+         $(foreach f,$(wilard $(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES))), \

But still, it would be nice to understand what is your image name. I
guess I could find that out by doing a build, though :)

Thomas
Jagan Teki Feb. 14, 2020, 7:34 a.m. UTC | #6 
On Fri, Feb 14, 2020 at 12:56 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Fri, 14 Feb 2020 12:25:37 +0530
> Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> > > Sergey is right here: I don't think we want to encode platform-specific
> > > stuff like this in Kconfig files.
> > >
> > > What is the problem if you leave this to the default of *.bin ?
> >
> > Is looking for *.bin in build/rk3399/release/*.bin but v2.2 is not
> > creating any bin since rk3399 (or rockchip) doesn't need bins.
> >
> > .mk file change:
> >
> > define ARM_TRUSTED_FIRMWARE_INSTALL_IMAGES_CMDS
> >         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
> >                 cp -dpf $(ARM_TRUSTED_FIRMWARE_IMG_DIR)/$(f) $(BINARIES_DIR)/
> >         )
> >
> > Build log:
> >
> > >>> arm-trusted-firmware v2.2 Installing to images directory
> > cp -dpf /mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin
> > /mnt/out/images/
> > cp: cannot stat
> > '/mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin':
> > No such file or directory
> > package/pkg-generic.mk:339: recipe for target
> > '/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed'
> > failed
> > make[1]: *** [/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed]
> > Error 1
> > Makefile:23: recipe for target '_all' failed
> > make: *** [_all] Error 2
>
> What is your image name then? You don't install any image?

As I said, the image that required for rk3399 is elf  it is bl31.elf.

We have enabled BR2_TARGET_UBOOT_NEEDS_ATF_BL31_ELF in defconfig on
this series. elf copy is already supported in buildroot on this commit
cab8bd3b46badb11e734e64747aee848c2b4282d

Jagan.
Suniel Mahesh Feb. 14, 2020, 10:57 a.m. UTC | #7 
On Fri, Feb 14, 2020 at 12:56 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Fri, 14 Feb 2020 12:25:37 +0530
> Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> > > Sergey is right here: I don't think we want to encode platform-specific
> > > stuff like this in Kconfig files.
> > >
> > > What is the problem if you leave this to the default of *.bin ?
> >
> > Is looking for *.bin in build/rk3399/release/*.bin but v2.2 is not
> > creating any bin since rk3399 (or rockchip) doesn't need bins.
> >
> > .mk file change:
> >
> > define ARM_TRUSTED_FIRMWARE_INSTALL_IMAGES_CMDS
> >         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
> >                 cp -dpf $(ARM_TRUSTED_FIRMWARE_IMG_DIR)/$(f) $(BINARIES_DIR)/
> >         )
> >
> > Build log:
> >
> > >>> arm-trusted-firmware v2.2 Installing to images directory
> > cp -dpf /mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin
> > /mnt/out/images/
> > cp: cannot stat
> > '/mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin':
> > No such file or directory
> > package/pkg-generic.mk:339: recipe for target
> > '/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed'
> > failed
> > make[1]: *** [/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed]
> > Error 1
> > Makefile:23: recipe for target '_all' failed
> > make: *** [_all] Error 2
>
> What is your image name then? You don't install any image?
>
> One possibility is to change:
>
> -         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
> +         $(foreach f,$(wilard $(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES))), \
>

Yes wildcard function is working. tested with the above change in BR.
Suniel

> But still, it would be nice to understand what is your image name. I
> guess I could find that out by doing a build, though :)
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Suniel Mahesh Feb. 14, 2020, 12:33 p.m. UTC | #8 
On Fri, Feb 14, 2020 at 4:27 PM Sunil Kumar Mahesh
<sunil@amarulasolutions.com> wrote:
>
> On Fri, Feb 14, 2020 at 12:56 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> >
> > On Fri, 14 Feb 2020 12:25:37 +0530
> > Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > > > Sergey is right here: I don't think we want to encode platform-specific
> > > > stuff like this in Kconfig files.
> > > >
> > > > What is the problem if you leave this to the default of *.bin ?
> > >
> > > Is looking for *.bin in build/rk3399/release/*.bin but v2.2 is not
> > > creating any bin since rk3399 (or rockchip) doesn't need bins.
> > >
> > > .mk file change:
> > >
> > > define ARM_TRUSTED_FIRMWARE_INSTALL_IMAGES_CMDS
> > >         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
> > >                 cp -dpf $(ARM_TRUSTED_FIRMWARE_IMG_DIR)/$(f) $(BINARIES_DIR)/
> > >         )
> > >
> > > Build log:
> > >
> > > >>> arm-trusted-firmware v2.2 Installing to images directory
> > > cp -dpf /mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin
> > > /mnt/out/images/
> > > cp: cannot stat
> > > '/mnt/out/build/arm-trusted-firmware-v2.2/build/rk3399/release/*.bin':
> > > No such file or directory
> > > package/pkg-generic.mk:339: recipe for target
> > > '/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed'
> > > failed
> > > make[1]: *** [/mnt/out/build/arm-trusted-firmware-v2.2/.stamp_images_installed]
> > > Error 1
> > > Makefile:23: recipe for target '_all' failed
> > > make: *** [_all] Error 2
> >
> > What is your image name then? You don't install any image?
> >
> > One possibility is to change:
> >
> > -         $(foreach f,$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES)), \
> > +         $(foreach f,$(wilard $(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES))), \
> >
>
> Yes wildcard function is working. tested with the above change in BR.
> Suniel

Sorry, my bad, need to test other targets which might depend on this
changeset. will update
Suniel
>
> > But still, it would be nice to understand what is your image name. I
> > guess I could find that out by doing a build, though :)
> >
> > Thomas
> > --
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
diff mbox series

Patch

diff --git a/boot/arm-trusted-firmware/Config.in b/boot/arm-trusted-firmware/Config.in
index 373591d..0006931 100644
--- a/boot/arm-trusted-firmware/Config.in
+++ b/boot/arm-trusted-firmware/Config.in
@@ -155,7 +155,7 @@  config BR2_TARGET_ARM_TRUSTED_FIRMWARE_DEBUG
 
 config BR2_TARGET_ARM_TRUSTED_FIRMWARE_IMAGES
 	string "Binary boot images"
-	default "*.bin"
+	default "*.bin" if BR2_TARGET_ARM_TRUSTED_FIRMWARE_PLATFORM!=rk3399
 	help
 	  Names of generated image files that are installed in the
 	  output images/ directory.