Re: [meta-ti][master/kirkstone][PATCH 1/8] ti-rtos-firmware: Use ti-k3-secdev if TI_SECURE_DEV_PKG_K3 not defined

[Denys Dmytriyenko <denis@denix.org>]

On Fri, Feb 10, 2023 at 03:13:15PM -0600, Andrew Davis wrote:
> On 2/10/23 3:02 PM, Denys Dmytriyenko wrote:
> >On Tue, Feb 07, 2023 at 05:42:32PM -0600, Andrew Davis via lists.yoctoproject.org wrote:
> >>Use ti-k3-secdev native package to provide secure-binary-image.sh script
> >>when it is not passed on as an environment variable. This fixes and issue
> >
> >Fixes *an* issue
> >
> 
> ACK
> 
> >
> >>with AM64xx which is always HS but should not require TI_SECURE_DEV_PKG
> >>when building for HS-FS platforms.
> >
> >And GP?
> >
> 
> AM64 is HS-FS only :)

Don't we also have GP variant of AM64 covered in the configs?


> But yes, this fixes GP building too. Will add to the message.
> 
> >
> >>
> >>Signed-off-by: Andrew Davis <afd@ti.com>
> >>---
> >>  meta-ti-bsp/recipes-ti/ti-rtos-bin/ti-rtos-firmware.bb | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/meta-ti-bsp/recipes-ti/ti-rtos-bin/ti-rtos-firmware.bb b/meta-ti-bsp/recipes-ti/ti-rtos-bin/ti-rtos-firmware.bb
> >>index 9a6da088..6c59b8ab 100644
> >>--- a/meta-ti-bsp/recipes-ti/ti-rtos-bin/ti-rtos-firmware.bb
> >>+++ b/meta-ti-bsp/recipes-ti/ti-rtos-bin/ti-rtos-firmware.bb
> >>@@ -31,9 +31,11 @@ CLEANBROKEN = "1"
> >>  PR = "${INC_PR}.0"
> >>  # Secure Build
> >>+include recipes-ti/includes/ti-paths.inc
> >
> >Set TI_K3_SECDEV_INSTALL_DIR here instead of including ti-paths.inc?
> >
> 
> Sure, and if we really do not like this include I can drop it everywhere
> in the next spin.

Would be nice, thank you!


> >>  DEPENDS += "openssl-native"
> >>-
> >>-TI_SECURE_DEV_PKG ?= ""
> >>+DEPENDS:append = "${@ '' if d.getVar('TI_SECURE_DEV_PKG_K3') else ' ti-k3-secdev-native' }"
> >
> >Should probably use DEPENDS += here instead of a generic :append override.
> >As :append and :remove are almost impossible to fight/override downstream,
> >it is recommended to use += whenever possible, like in this case...
> >
> 
> I'm never really sure which is right, the docs all seem to say to use
> :append/:remove when possible, but I can see it being a pain to override later..

Hmm, can you point to such instances? That needs to get fixed...

In general, using :append/:remove is justifiable when used with another 
override, like VAR:append:k3 and such. Because you shouldn't mix += and 
an override.

The biggest obvious and documented difference between += and :append is that 
the first adds a whitespace and the other does not. BTW, there's .= that 
behaves like += but does not add a whitespace:
https://docs.yoctoproject.org/bitbake/2.2/bitbake-user-manual/bitbake-user-manual-metadata.html#appending-and-prepending-with-spaces
https://docs.yoctoproject.org/bitbake/2.2/bitbake-user-manual/bitbake-user-manual-metadata.html#appending-and-prepending-without-spaces

But the issue comes from the fact that :append and :remove are just special 
cases of regular overrides and get collected and processed at the very end of 
the parsing process (unlike += and .= which are processed on the spot). 
There's no implied order of the overrides, unfortunately...


> >>+TI_SECURE_DEV_PKG = "${@ d.getVar('TI_SECURE_DEV_PKG_K3') or d.getVar('TI_K3_SECDEV_INSTALL_DIR') }"
> >>+export TI_SECURE_DEV_PKG
> >>  RTOS_ETH_FW_DIR = "${S}/ti-eth/${PLAT_SFX}"
> >>  RTOS_DM_FW_DIR = "${S}/ti-dm/${PLAT_SFX}"
> >>@@ -49,7 +51,6 @@ DM_FIRMWARE = "ipc_echo_testb_mcu1_0_release_strip.xer5f"
> >>  # J721e HS support
> >>  do_install:prepend:j721e-hs-evm() {
> >>-        export TI_SECURE_DEV_PKG=${TI_SECURE_DEV_PKG}
> >>          ( cd ${RTOS_DM_FW_DIR}; \
> >>                  mv ${DM_FIRMWARE} ${DM_FIRMWARE}.unsigned; \
> >>                  ${TI_SECURE_DEV_PKG}/scripts/secure-binary-image.sh ${DM_FIRMWARE}.unsigned ${DM_FIRMWARE}; \
> >>@@ -80,7 +81,6 @@ do_install:prepend:j721e-hs-evm() {
> >>  # J7200 HS support
> >>  do_install:prepend:j7200-hs-evm() {
> >>-        export TI_SECURE_DEV_PKG=${TI_SECURE_DEV_PKG}
> >>          ( cd ${RTOS_DM_FW_DIR}; \
> >>                  mv ${DM_FIRMWARE} ${DM_FIRMWARE}.unsigned; \
> >>                  ${TI_SECURE_DEV_PKG}/scripts/secure-binary-image.sh ${DM_FIRMWARE}.unsigned ${DM_FIRMWARE}; \
> >>@@ -101,7 +101,6 @@ do_install:prepend:j7200-hs-evm() {
> >>  # J721s2 HS support
> >>  do_install:prepend:j721s2-hs-evm() {
> >>-        export TI_SECURE_DEV_PKG=${TI_SECURE_DEV_PKG}
> >>          ( cd ${RTOS_DM_FW_DIR}; \
> >>                  mv ${DM_FIRMWARE} ${DM_FIRMWARE}.unsigned; \
> >>                  ${TI_SECURE_DEV_PKG}/scripts/secure-binary-image.sh ${DM_FIRMWARE}.unsigned ${DM_FIRMWARE}; \
> >>-- 
> >>2.39.1