Re: [Buildroot] [PATCH v3 1/1] boot/xilinx-embeddedsw: only allow apps for device family

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

On Wed, 13 Aug 2025 14:22:39 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Luca,
> 
> >
> > Hi Yann,
> >
> > [--SNIP--]  
> > > > When the dependency is not tricvial, I think it is good to introduce an
> > > > intermediate symbol:
> > > >    config BR2_TARGET_XILINX_EMBEDDEDSW_ARCH_SUPPORTS  
> > [--SNIP--]  
> > > I see your point, but don't you think adding another symbol is still
> > > a bit overkill?  The dependencies are still only 3 lines and are rather
> > > straight forward.  
> >  
> > > This would not be unheard of, there already are a few packages with very
> > > few arch dependencies, but that still have a dedicated symbol, see for
> > > example (some are more interesting, as they do have comments for each
> > > dependency they carry):
> > >  - bayer2rgb-neon
> > >  - bitcoin
> > >  - bpftrace
> > >  - dpdk
> > >  - host-flutter-sdk-bin
> > >  - (host-)gdb
> > >  - gtkiostream
> > >  - ... and so on...  
> >  
> > > Note that we usually do that for libraries, so that it is easier to
> > > inherit their dependencies in selecting packages, but of the above,
> > > only two are libraries.  
> >  
> > > But maintainers will have to decide what they want. ;-)  
> >
> > In the end, I changed my mind on the issue since there are two places where
> > the arch dependency is used. The BR2_TARGET_XILINX_EMBEDDEDSW as well as the
> > comment about the dependency needs.
> >
> > To avoid having the two be out of sync, it makes sense to create a
> > symbol BR2_TARGET_XILINX_EMBEDDEDSW_ARCH_SUPPORTS that keeps all the arch
> > dependencies in one place. This way, when I add BR2_cortex_a78 in the near
> > future, I will only have one place to add it instead of two.  
> 
> > I agree this is a good idea.  
> 
> > However the same logic can apply to other Xilinx firmware and related
> > packages, such as xilinx-prebuilt. Thus I'd rather create a single
> > symbol that can be reused for both packages, and it should probably be
> > in boot/Config.in. It should be also named without "EMBEDDEDSW", e.g.
> > BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS.  
> 
> > Side note: I personally don't recommend sending new iterations so
> > quickly, without letting the discussion settle, it produces a lot of
> > noise. I'd wait at least a few days after the latest e-mail in the
> > discussion before sending a new version.  
> 
> Yes, you are right that we can create a global symbol in boot/Config.in called:
> BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS
> 
> This symbol could be used for both xilinx-embeddedsw and xilinx-prebuilt.
> 
> Not only would this enable us to only show the xilinx-prebuilt package as an
> option when one of the correct cpus is selected, but we could also use the
> BR2_cortex_a53 and BR2_cortex_a72 to limit the available choices in the
> family variant selector. This would prevent selecting BR2_cortex_a72 and
> having a zynqmp or kria xilinx-prebuilt family selection or vice versa.
> 
> It would not change the need for having the family and board information
> because the xilinx-prebuilt treats zynqmp and kria boards differently due to
> the directory names where the pmufw.elf files are located even though these
> are both based on BR2_cortex_a53.
> 
> But at least creating a BR2_TARGET_XILINX_ARCH_SUPPORTS would put the arch
> dependencies of Xilinx components in one place. We could even update the
> uboot package BR2_TARGET_UBOOT_ZYNQMP symbol to be dependent on
> BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS instead of BR2_aarch64 as well.

Sure.

> The only exception would be the zynq platform. The BR2_TARGET_UBOOT_ZYNQ
> symbol would stay dependent on BR2_arm because it is the only family that is
> not an aarch64. I suppose I should add a BR2_cortex_a9 dependency to
> BR2_TARGET_UBOOT_ZYNQ as well, so that it limits the option appearing even
> more.
> 
> https://patchwork.ozlabs.org/project/buildroot/patch/20250619153922.891360-1-neal.frager@amd.com/

That's true as well.

If you do all of there, please keep them as separate commits in the
series to be easier to review:

 - add BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS
 - use BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS in xilinx-embeddedsw
 - use BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS in xilinx-prebuilt
 - use BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS in uboot

And the zynq7 changes are already a separate series, so I guess thay
can stay separate.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot