Re: [Buildroot] [PATCH v4 18/20] boot/ti-k3: move TI_K3_{SECTYPE, SOC} out of the image-gen scope

[Andreas Dannenberg via buildroot <buildroot@buildroot.org>]

Dear Romain, Dario, Thomas:

On Tue, Feb 20, 2024 at 10:32:14AM +0100, Romain Naour wrote:
> Hi Dario,
> 
> Le 20/02/2024 à 10:05, Dario Binacchi a écrit :
> > Hi Andreas, Romain, All
> > 
> > On Tue, Feb 20, 2024 at 2:28 AM Andreas Dannenberg <dannenberg@ti.com> wrote:
> >>
> >> Romain, Dario,
> >>
> >> On Mon, Feb 19, 2024 at 11:28:59AM +0100, Romain Naour wrote:
> >>> Hello Dario,
> >>>
> >>> Le 17/02/2024 à 17:02, Dario Binacchi a écrit :
> >>>> Recent versions of U-Boot can use Binman to compile and sign the generated
> >>>> binaries, thus making the use of custom tools like ti-k3-image-gen no
> >>>> longer necessary. In ti-k3-image-gen, options such as SECTYPE and SOC were
> >>>> configured and used outside of it. This patch allows setting such options
> >>>> even if the package is not enabled.
> >>>
> >>> I'm agree that Buildroot doesn't provide any "Platform selection" as the Linux
> >>> kernel and I'm not sure where those TI_K3_{SECTYPE,SOC} should belong.
> >>
> >> There were discussions/suggestions around this before by Yann E Morin
> >> [1] and Thomas Petazzoni [2]. I think the gist of it was to move the
> >> selection to a new package like BR2_PACKAGE_TI_K3_PLATFORM containing
> >> all the K3 SoC specific selections. And then other packages and Kconfig
> >> menues can depend on it more cleanly. Like it's currently done with
> >> package/freescale-imx/Config.in.  This would also bring some consistency
> >> in how those NXP and TI device families are managed, which will make for
> >> a better end-user experience I think.
> > 
> > If I'm not mistaken, I think the patches:
> > [18/20] boot/ti-k3: move TI_K3_{SECTYPE,SOC} out of the image-gen scope
> > [17/20] boot: move ti-k3-* packages to 'ti-k3' directory
> > Are heading in the direction of what we find in
> > package/freescale-imx/, with the only difference being
> > that in our case, we're under boot.
> > So, can I consider these two patches correct?

Yes the general direction those are headed is a good...

> Yes, can you try to move ti-k3 to package directory in V5 ?
> It would also be used later by the GPU rogue firmwares.

...and yes, for alignment sake with IMX it would be even better to have
it in package/, while also keeping some alignment with the naming of the
CONFIG options (hence my comment with 'BR2_PACKAGE_TI_K3_PLATFORM').


Hi Thomas,
assuming we create a new package called 'package/ti-k3' I was trying to
figure out what would be the best way to name the associated CONFIG
options that control device security settings and can be used to
determine which device is being used (for later uses in context of GPU
drivers and other device capabilities)?

I see different naming conventions being used, like this:

1) BR2_PACKAGE_*
2) BR2_TARGET_*
3) BR2_BOOT_*

Seems like BR2_TARGET_* what we have now would be most appropriate, but
then the IMX stuff uses BR2_PACKAGE_*, which *seems* to be reserved for
"userspace" (non-boot) packages as far as I can tell from my poor-man's
research. While I'd like to align things, the use of the BR2_PACKAGE_*
prefix may or may not be appropriate for our new additions here?

Just was hoping to get some guidance here so we don't need to re-work
this too many times.


--
Andreas Dannenberg
Texas Instruments Inc



> 
> Please add a comment in the commit log that we make an exception to the rule "no
> subdirectory in package directory".
> 
> Thanks!
> 
> Best regards,
> Romain
> 
> 
> > 
> > Thanks and regards,
> > Dario
> > 
> >>
> >>
> >>
> >> --
> >> Andreas Dannenberg
> >> Texas Instruments Inc
> >>
> >> [1] http://lists.busybox.net/pipermail/buildroot/2023-June/669342.html
> >> [2] http://lists.busybox.net/pipermail/buildroot/2023-August/673292.html
> >>
> >>
> >>>
> >>> Best regards,
> >>> Romain
> >>>
> >>>
> >>>>
> >>>> ---
> >>>>
> >>>> Added in v4
> >>>>
> >>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >>>> ---
> >>>>  boot/ti-k3/Config.in                          | 46 ++++++++++++++
> >>>>  boot/ti-k3/ti-k3-image-gen/Config.in          | 63 ++-----------------
> >>>>  boot/ti-k3/ti-k3-image-gen/ti-k3-image-gen.mk |  6 +-
> >>>>  boot/uboot/Config.in                          |  7 +--
> >>>>  configs/ti_am62x_sk_defconfig                 |  2 +-
> >>>>  configs/ti_am64x_sk_defconfig                 |  2 +-
> >>>>  6 files changed, 59 insertions(+), 67 deletions(-)
> >>>>
> >>>> diff --git a/boot/ti-k3/Config.in b/boot/ti-k3/Config.in
> >>>> index 06dda2f156a9..9e508326c9c8 100644
> >>>> --- a/boot/ti-k3/Config.in
> >>>> +++ b/boot/ti-k3/Config.in
> >>>> @@ -5,6 +5,52 @@ config BR2_TARGET_TI_K3
> >>>>
> >>>>  if BR2_TARGET_TI_K3
> >>>>
> >>>> +choice
> >>>> +   prompt "SoC family"
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SOC_AM62AX
> >>>> +   bool "am62ax"
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SOC_AM62X
> >>>> +   bool "am62x"
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SOC_AM64X
> >>>> +   bool "am64x"
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SOC_AM65X
> >>>> +   bool "am65x"
> >>>> +
> >>>> +endchoice
> >>>> +
> >>>> +choice
> >>>> +   prompt "Security type"
> >>>> +   help
> >>>> +     The target SoC security type option.
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SECTYPE_GP
> >>>> +   bool "gp"
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SECTYPE_HS_FS
> >>>> +   bool "hs-fs"
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SECTYPE_HS
> >>>> +   bool "hs"
> >>>> +
> >>>> +endchoice
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SOC
> >>>> +   string
> >>>> +   default "am62ax" if BR2_TARGET_TI_K3_SOC_AM62AX
> >>>> +   default "am62x"  if BR2_TARGET_TI_K3_SOC_AM62X
> >>>> +   default "am64x"  if BR2_TARGET_TI_K3_SOC_AM64X
> >>>> +   default "am65x"  if BR2_TARGET_TI_K3_SOC_AM65X
> >>>> +
> >>>> +config BR2_TARGET_TI_K3_SECTYPE
> >>>> +   string
> >>>> +   default "gp"    if BR2_TARGET_TI_K3_SECTYPE_GP
> >>>> +   default "hs-fs" if BR2_TARGET_TI_K3_SECTYPE_HS_FS
> >>>> +   default "hs"    if BR2_TARGET_TI_K3_SECTYPE_HS
> >>>> +
> >>>>  source "boot/ti-k3/ti-k3-boot-firmware/Config.in"
> >>>>  source "boot/ti-k3/ti-k3-core-secdev/Config.in"
> >>>>  source "boot/ti-k3/ti-k3-image-gen/Config.in"
> >>>> diff --git a/boot/ti-k3/ti-k3-image-gen/Config.in b/boot/ti-k3/ti-k3-image-gen/Config.in
> >>>> index 38d1598d9fc0..2d53d62dd4ee 100644
> >>>> --- a/boot/ti-k3/ti-k3-image-gen/Config.in
> >>>> +++ b/boot/ti-k3/ti-k3-image-gen/Config.in
> >>>> @@ -2,7 +2,7 @@ config BR2_TARGET_TI_K3_IMAGE_GEN
> >>>>     bool "ti-k3-image-gen"
> >>>>     depends on BR2_TARGET_TI_K3_R5_LOADER
> >>>>     select BR2_TARGET_TI_K3_BOOT_FIRMWARE
> >>>> -   select BR2_TARGET_TI_K3_CORE_SECDEV if BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_HS_FS || BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_HS
> >>>> +   select BR2_TARGET_TI_K3_CORE_SECDEV if BR2_TARGET_TI_K3_SECTYPE_HS_FS || BR2_TARGET_TI_K3_SECTYPE_HS
> >>>>     # We need FIT support in uboot-tools, which is why we select a
> >>>>     # host package
> >>>>     select BR2_PACKAGE_HOST_UBOOT_TOOLS
> >>>> @@ -14,65 +14,12 @@ config BR2_TARGET_TI_K3_IMAGE_GEN
> >>>>       https://git.ti.com/cgit/k3-image-gen/k3-image-gen/
> >>>>
> >>>>  if BR2_TARGET_TI_K3_IMAGE_GEN
> >>>> -choice
> >>>> -   prompt "SoC family"
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62AX
> >>>> -   bool "am62ax"
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X
> >>>> -   bool "am62x"
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM64X
> >>>> -   bool "am64x"
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM65X
> >>>> -   bool "am65x"
> >>>> -
> >>>> -endchoice
> >>>> -
> >>>> -choice
> >>>> -   prompt "Security type"
> >>>> -   help
> >>>> -     The target SoC security type option for image gen.  Valid
> >>>> -     options are "gp" for General Purpose devices, "hs-fs" for
> >>>> -     High Security - Field Securable devices, or "hs" for High
> >>>> -     Security - Security Enforcing devices.  Note for all High
> >>>> -     Security device variants the TI_SECURE_DEV_PKG environmental
> >>>> -     variable must be defined at build time pointing to a valid
> >>>> -     core-secdev-k3 folder location, otherwise the build will
> >>>> -     fail, see
> >>>> -     https://git.ti.com/cgit/security-development-tools/core-secdev-k3
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_GP
> >>>> -   bool "gp"
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_HS_FS
> >>>> -   bool "hs-fs"
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_HS
> >>>> -   bool "hs"
> >>>> -
> >>>> -endchoice
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SOC
> >>>> -   string
> >>>> -   default "am62ax" if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62AX
> >>>> -   default "am62x"  if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X
> >>>> -   default "am64x"  if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM64X
> >>>> -   default "am65x"  if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM65X
> >>>>
> >>>>  config BR2_TARGET_TI_K3_IMAGE_GEN_FW_TYPE
> >>>>     string
> >>>> -   default "ti-fs"  if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62AX
> >>>> -   default "ti-fs"  if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X
> >>>> -   default "ti-sci" if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM64X
> >>>> -   default "ti-sci" if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM65X
> >>>> -
> >>>> -config BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE
> >>>> -   string
> >>>> -   default "gp"    if BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_GP
> >>>> -   default "hs-fs" if BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_HS_FS
> >>>> -   default "hs"    if BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_HS
> >>>> +   default "ti-fs"  if BR2_TARGET_TI_K3_SOC_AM62AX
> >>>> +   default "ti-fs"  if BR2_TARGET_TI_K3_SOC_AM62X
> >>>> +   default "ti-sci" if BR2_TARGET_TI_K3_SOC_AM64X
> >>>> +   default "ti-sci" if BR2_TARGET_TI_K3_SOC_AM65X
> >>>>
> >>>>  endif
> >>>> diff --git a/boot/ti-k3/ti-k3-image-gen/ti-k3-image-gen.mk b/boot/ti-k3/ti-k3-image-gen/ti-k3-image-gen.mk
> >>>> index 74b4fcb4bf89..834d8f7d5cd5 100644
> >>>> --- a/boot/ti-k3/ti-k3-image-gen/ti-k3-image-gen.mk
> >>>> +++ b/boot/ti-k3/ti-k3-image-gen/ti-k3-image-gen.mk
> >>>> @@ -26,10 +26,10 @@ TI_K3_IMAGE_GEN_DEPENDENCIES = \
> >>>>     $(BR2_MAKE_HOST_DEPENDENCY)
> >>>>
> >>>>  TI_K3_IMAGE_GEN_FW_TYPE = $(call qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_FW_TYPE))
> >>>> -TI_K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_SOC))
> >>>> -TI_K3_IMAGE_GEN_SECTYPE = $(call qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE))
> >>>> +TI_K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_TARGET_TI_K3_SOC))
> >>>> +TI_K3_IMAGE_GEN_SECTYPE = $(call qstrip,$(BR2_TARGET_TI_K3_SECTYPE))
> >>>>
> >>>> -ifeq ($(BR2_TARGET_TI_K3_IMAGE_GEN_SECTYPE_GP),y)
> >>>> +ifeq ($(BR2_TARGET_TI_K3_SECTYPE_GP),y)
> >>>>  TI_K3_IMAGE_GEN_SYSFW = \
> >>>>     $(TI_K3_IMAGE_GEN_FW_TYPE)-firmware-$(TI_K3_IMAGE_GEN_SOC)-$(TI_K3_IMAGE_GEN_SECTYPE).bin
> >>>>  else
> >>>> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> >>>> index beafb38930db..1a9d21e7c6d1 100644
> >>>> --- a/boot/uboot/Config.in
> >>>> +++ b/boot/uboot/Config.in
> >>>> @@ -244,8 +244,7 @@ config BR2_TARGET_UBOOT_NEEDS_TI_K3_DM
> >>>>     bool "U-Boot needs TI K3 Device Manager (DM)"
> >>>>     # We use the SoC selection defined for the ti-k3-image-gen
> >>>>     # package
> >>>> -   depends on BR2_TARGET_TI_K3_IMAGE_GEN
> >>>> -   depends on BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62AX || BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X
> >>>> +   depends on BR2_TARGET_TI_K3_SOC_AM62AX || BR2_TARGET_TI_K3_SOC_AM62X
> >>>>     help
> >>>>       Some TI K3 devices need the Device Manager (DM) firmware to
> >>>>       be available for the U-Boot build.
> >>>> @@ -254,8 +253,8 @@ if BR2_TARGET_UBOOT_NEEDS_TI_K3_DM
> >>>>
> >>>>  config BR2_TARGET_UBOOT_TI_K3_DM_SOCNAME
> >>>>     string
> >>>> -   default "am62axx" if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62AX
> >>>> -   default "am62xx"  if BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X
> >>>> +   default "am62axx" if BR2_TARGET_TI_K3_SOC_AM62AX
> >>>> +   default "am62xx"  if BR2_TARGET_TI_K3_SOC_AM62X
> >>>>
> >>>>  endif
> >>>>
> >>>> diff --git a/configs/ti_am62x_sk_defconfig b/configs/ti_am62x_sk_defconfig
> >>>> index cd26235342e4..cfefce0a77a4 100644
> >>>> --- a/configs/ti_am62x_sk_defconfig
> >>>> +++ b/configs/ti_am62x_sk_defconfig
> >>>> @@ -24,8 +24,8 @@ BR2_TARGET_OPTEE_OS=y
> >>>>  BR2_TARGET_OPTEE_OS_PLATFORM="k3-am62x"
> >>>>  BR2_TARGET_OPTEE_OS_PLATFORM_FLAVOR="am62x"
> >>>>  BR2_TARGET_TI_K3=y
> >>>> +BR2_TARGET_TI_K3_SOC_AM62X=y
> >>>>  BR2_TARGET_TI_K3_IMAGE_GEN=y
> >>>> -BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM62X=y
> >>>>  BR2_TARGET_TI_K3_R5_LOADER=y
> >>>>  BR2_TARGET_TI_K3_R5_LOADER_CUSTOM_VERSION=y
> >>>>  BR2_TARGET_TI_K3_R5_LOADER_CUSTOM_VERSION_VALUE="2024.01"
> >>>> diff --git a/configs/ti_am64x_sk_defconfig b/configs/ti_am64x_sk_defconfig
> >>>> index 1befeb4ab96c..daf78dbffff8 100644
> >>>> --- a/configs/ti_am64x_sk_defconfig
> >>>> +++ b/configs/ti_am64x_sk_defconfig
> >>>> @@ -24,8 +24,8 @@ BR2_TARGET_OPTEE_OS=y
> >>>>  BR2_TARGET_OPTEE_OS_PLATFORM="k3-am64x"
> >>>>  BR2_TARGET_OPTEE_OS_PLATFORM_FLAVOR="am64x"
> >>>>  BR2_TARGET_TI_K3=y
> >>>> +BR2_TARGET_TI_K3_SOC_AM64X=y
> >>>>  BR2_TARGET_TI_K3_IMAGE_GEN=y
> >>>> -BR2_TARGET_TI_K3_IMAGE_GEN_SOC_AM64X=y
> >>>>  BR2_TARGET_TI_K3_R5_LOADER=y
> >>>>  BR2_TARGET_TI_K3_R5_LOADER_CUSTOM_VERSION=y
> >>>>  BR2_TARGET_TI_K3_R5_LOADER_CUSTOM_VERSION_VALUE="2022.10"
> >>>
> > 
> > 
> > 
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot