Re: [PATCH v4 07/11] arm: dts: k3-am62lx: Add binman support

On August  6, 2026 thus sayeth Simon Glass:
> Hi Vignesh,
> 
> On 2026-08-04T21:55:02, Bryan Brattlof <[email protected]> wrote:
> > arm: dts: k3-am62lx: Add binman support
> >
> > Add the initial configuration to build the boot binaries for the 
> > AM62L
> > family of devices
> >
> > Tested-by: Anshul Dalal <[email protected]>
> > Reviewed-by: Anshul Dalal <[email protected]>
> > Signed-off-by: Vignesh Raghavendra <[email protected]>
> > Signed-off-by: Bryan Brattlof <[email protected]>
> >
> > arch/arm/dts/k3-am62l3-evm-binman.dtsi | 218 +++++++++++++++++++++++++++++++++
> >  arch/arm/dts/k3-am62l3-evm-u-boot.dtsi |  12 ++
> >  arch/arm/mach-k3/Kconfig               |  21 ++++
> >  3 files changed, 251 insertions(+)
> 
> >     arm: dts: k3-am62lx: Add binman support
> >
> >     Add the initial configuration to build the boot binaries for the AM62L
> >     family of devices
> 
> The body needs more context - how this differs from the existing
> k3-binman.dtsi (why AM62L cannot reuse it), and why the new Kconfig
> options are being introduced now.

Sure. I thought this is what documentation was for but I can copy and 
paste it here in the commit body as well.

> 
> > diff --git a/arch/arm/dts/k3-am62l3-evm-binman.dtsi b/arch/arm/dts/k3-am62l3-evm-binman.dtsi
> > @@ -0,0 +1,218 @@
> > +// SPDX-License-Identifier: GPL-2.0-only or MIT
> 
> Per the SPDX spec the operator is upper-case OR, i.e. GPL-2.0-only OR
> MIT. Every other K3 binman dtsi in the tree uses plain GPL-2.0 - is
> there a reason to dual-license this one?
> 

Sure I'll uppercase the operator. Thanks. 

As for the dual license, it's to align the license with what we have on 
the Linux side.

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am62l3-evm.dts

> > diff --git a/arch/arm/dts/k3-am62l3-evm-binman.dtsi b/arch/arm/dts/k3-am62l3-evm-binman.dtsi
> > @@ -0,0 +1,218 @@
> > +             fit {
> > +                     description = "U-Boot fitImage for AM62Lx SK";
> > +
> > +                     images {
> > +                             uboot {
> > +                                     description = "U-Boot for AM62Lx SK";
> 
> This board is the AM62L-EVM, not an SK, isn't it? Please update both
> description strings (the two k3-am62lx-evm ones look correct already).

Ah yeah the board name was changed half way through. I guess I forgot 
some places when updating things.

> 
> > diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> > @@ -129,6 +129,26 @@ config K3_EARLY_CONS_IDX
> > +config K3_TIFS_LOAD_ADDR
> > +     hex "Load address of TI's Foundational Security (TIFS) firmware"
> > +     default 0x40000
> > +     help
> > +       The load address for TI's Foundation Security firmware. This value is
> > +       used to build the x509 certificate to tell boot ROM where to place the
> > +       TIFS firmware in memory.
> > +
> > +       Unless you know what you're doing. Do not change this value.
> 
> The last line reads as two sentence fragments - how about 'Do not
> change this value unless you know what you are doing'.

Sure

> 
> Second, these options are unconditional so they appear for every K3
> SoC, but only AM62L3 consumes them - every other K3 binman dtsi still
> hard-codes 0x40000/0x67000. Either gate the prompt on SOC_K3_AM62L3
> (with per-SoC defaults) or convert the other dtsi files to use these
> too. Introducing a global knob that only one SoC honours is confusing.
> 
> > diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> > @@ -129,6 +129,26 @@ config K3_EARLY_CONS_IDX
> > +config K3_TIFS_DATA_LOAD_ADDR
> > +     hex "Load address of TIFS configuration data"
> > +     default 0x6c000
> > +     help
> > +       The load address for TI's Foundation Security configuration data. This
> > +       value is used to build the x509 certificate to tell ROM where to place
> > +       the TIFS firmware.
> 
> The help text says 'place the TIFS firmware' but this option is the
> load address of the TIFS *data*, not the firmware itself.

Sure I can reword this

~Bryan