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 15:28:00 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Luca,
> 
> >
> > 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  
> 
> I will separate the patches to the individual Config.in files, but to
> simplify the usage of BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS, I plan to
> use it right away in the first patch by doing the following in boot/Config.in.
> 
> +if BR2_TARGET_XILINX_FIRMWARE_ARCH_SUPPORTS
>  source "boot/xilinx-embeddedsw/Config.in"
>  source "boot/xilinx-prebuilt/Config.in"
> +endif

Aah, yes, if it is that simple I guess a single patch will be fine.

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