diff mbox series

[meta-ti,master/kirkstone,8/8] recipes-bsp: Do not use MACHINE_ARCH when package is not machine specific

Message ID 20231025165630.2274889-8-afd@ti.com
State Rejected
Delegated to: Ryan Eatmon
Headers show
Series [meta-ti,master/kirkstone,1/8] conf: machine: Move IMAGE_BOOT_FILES to the SoC inc for J721s2 and J784s4 | expand

Commit Message

Andrew Davis Oct. 25, 2023, 4:56 p.m. UTC
Signed-off-by: Andrew Davis <afd@ti.com>
---
 meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
 meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
 meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
 meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
 meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
 meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
 meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
 meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
 meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
 meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
 meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
 11 files changed, 19 deletions(-)

Comments

Denys Dmytriyenko Oct. 26, 2023, 3:27 a.m. UTC | #1
On Wed, Oct 25, 2023 at 11:56:30AM -0500, Andrew Davis via lists.yoctoproject.org wrote:
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
>  meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
>  meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
>  meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
>  meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
>  meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
>  meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
>  meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
>  meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
>  meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
>  meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
>  11 files changed, 19 deletions(-)

Overall I agree and fully support the first 7 patches in this series.

But for this last one I wanted to open a discussion.

On one hand I understand the desire to make components as generic as possible 
and reduce the number of machine-specific components to a bare minimum.

But on another hand, marking the resulting package as machine-specific when it 
has a short list of compatible machines is a standard practice. The reason is 
that the list of compatible machines controls only compile time filtering, but 
doesn't have any effect on run time. And marking packages as machine specific 
helps with that. That closes the loophole of installing incompatible packages.

For example, first recipe below specifies that Cadence MHDP firmware is 
compatible with 3 J7 platforms only (or their SoC families, to be exact).
But w/o marking resulting binary package as machine-specific (therefore 
producing separate packages for those platforms), there will be a single 
generic Aarch64 package made. And there's no protection from installing 
this generic package on non-compatible platforms, like J7200 or AM65xx, 
either manullay or by pulling it into a rootfs for those incompatible 
platforms.

And you normally want to prevent this for regular components. But I guess 
this doesn't fully apply to FW images that are loaded by corresponding 
drivers anyway. Moreover, there's no compilation involved, just packaging 
the binary blob.

In that case, should we also remove COMPATIBLE_MACHINE from these firmware 
recipes?



> diff --git a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
> index d88bca6e..ed1c7817 100644
> --- a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
> @@ -10,8 +10,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "j721e|j721s2|j784s4"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET = "mhdp8546.bin"
>  
>  do_install() {
> diff --git a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
> index 5b1d8be1..ef7bc2ad 100644
> --- a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
> @@ -12,8 +12,6 @@ PR = "${INC_PR}.1"
>  
>  COMPATIBLE_MACHINE = "j721s2|j784s4|am62axx"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET_WAVE521C = "wave521c_codec_fw.bin"
>  
>  SOURCE_WAVE521C = "wave521c_k3_codec_fw.bin"
> diff --git a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
> index 6e2996ce..e333d212 100755
> --- a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
> @@ -4,7 +4,6 @@ LICENSE = "TI-TFL"
>  LIC_FILES_CHKSUM = "file://LICENSE.ti;md5=04ad0a015d4bb63c2b9e7b112debf3db"
>  
>  PV = "6.2+git${SRCPV}"
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>  
>  inherit update-alternatives
>  
> diff --git a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
> index e58f2d58..ee3a94dc 100644
> --- a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
> @@ -10,8 +10,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "dra7xx"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  ORIGIN = "DRA71x-RevA-GT9271_SpecDig_Config.bin"
>  TARGET = "goodix_9271_cfg.bin"
>  
> diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
> index 2c0736ed..4b6ef75d 100644
> --- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET = " \
>      am65x-sr2-pru0-prueth-fw.elf \
>      am65x-sr2-pru1-prueth-fw.elf \
> diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
> index 8b15ab7f..20b2bfb9 100644
> --- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "am65xx"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET = " \
>      am65x-pru0-prueth-fw.elf \
>      am65x-pru1-prueth-fw.elf \
> diff --git a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
> index ea39d73d..bc731094 100644
> --- a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET = " \
>      am65x-sr2-pru0-pruhsr-fw.elf \
>      am65x-sr2-pru1-pruhsr-fw.elf \
> diff --git a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
> index 63c2d311..6e296e7c 100644
> --- a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET = " \
>      am65x-sr2-pru0-prusw-fw.elf \
>      am65x-sr2-pru1-prusw-fw.elf \
> diff --git a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
> index d295a1c1..74729c16 100644
> --- a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
> +++ b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
> @@ -9,8 +9,6 @@ PR = "${INC_PR}.0"
>  
>  COMPATIBLE_MACHINE = "j721e"
>  
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
> -
>  TARGET = "pvdec_full_bin.fw"
>  
>  do_install() {
> diff --git a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
> index 7d16ae39..4ec09a70 100644
> --- a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
> +++ b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
> @@ -3,7 +3,6 @@ LICENSE = "TI-TSPA"
>  LIC_FILES_CHKSUM = "file://${S}/J6_VIS_DEMO_LINUX_BINARY_01.50.07.15-Manifest.html;md5=a59aa54b9470f555cf086b91dca0afa3"
>  
>  COMPATIBLE_MACHINE = "dra7xx"
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>  
>  PR = "r1"
>  
> diff --git a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
> index 2452d111..8af49577 100644
> --- a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
> +++ b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
> @@ -4,7 +4,6 @@ LICENSE = "TI-TSPA"
>  LIC_FILES_CHKSUM = "file://COPYING;md5=fd463c9500441ed91d07a0331baa635c"
>  
>  COMPATIBLE_MACHINE = "dra7xx"
> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>  
>  SRC_URI = "http://downloads.ti.com/dsps/dsps_public_sw/glsdk/vpdma-fw/03-2012/exports/vpdma-fw_03-2012.tar.gz;protocol=http;name=dra7xx-evm"
>  SRC_URI[dra7xx-evm.md5sum] = "80176df1350c21d9efa90171789c546e"
> -- 
> 2.39.2
Andrew Davis Oct. 26, 2023, 2 p.m. UTC | #2
On 10/25/23 10:27 PM, Denys Dmytriyenko wrote:
> On Wed, Oct 25, 2023 at 11:56:30AM -0500, Andrew Davis via lists.yoctoproject.org wrote:
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
>>   meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
>>   meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
>>   meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
>>   meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
>>   meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
>>   meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
>>   meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
>>   meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
>>   meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
>>   meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
>>   11 files changed, 19 deletions(-)
> 
> Overall I agree and fully support the first 7 patches in this series.
> 
> But for this last one I wanted to open a discussion.
> 

I try to sort my series in least-to-most likely to be controversial, I was
just wondering how far we would get down the list, glad we got to 8 :)

> On one hand I understand the desire to make components as generic as possible
> and reduce the number of machine-specific components to a bare minimum.
> 
> But on another hand, marking the resulting package as machine-specific when it
> has a short list of compatible machines is a standard practice. The reason is
> that the list of compatible machines controls only compile time filtering, but
> doesn't have any effect on run time. And marking packages as machine specific
> helps with that. That closes the loophole of installing incompatible packages.
> 
> For example, first recipe below specifies that Cadence MHDP firmware is
> compatible with 3 J7 platforms only (or their SoC families, to be exact).
> But w/o marking resulting binary package as machine-specific (therefore
> producing separate packages for those platforms), there will be a single
> generic Aarch64 package made. And there's no protection from installing
> this generic package on non-compatible platforms, like J7200 or AM65xx,
> either manullay or by pulling it into a rootfs for those incompatible
> platforms.
> 
> And you normally want to prevent this for regular components. But I guess
> this doesn't fully apply to FW images that are loaded by corresponding
> drivers anyway. Moreover, there's no compilation involved, just packaging
> the binary blob.
> 
> In that case, should we also remove COMPATIBLE_MACHINE from these firmware
> recipes?
> 

That is exactly where I was going to go next. These firmware packages are
not technically incompatible with the other machines.

We just use COMPATIBLE_MACHINE checks here to keep us from accidentily bundling
them with images where they wouldn't add any value (which you did with prusw-fw
which is what stated me thinking on all this). But since they don't break
anything either, forcing them to be machine specific seemed like overkill also.

Only place where we still need this is firmware recipes that only ship some of
the firmware based on machine (see prueth-fw for instance). I'd like to get
that cleaned up next. If we only want some of the firmware then we should split
it into different packages (prueth-fw-am57xx, etc.) and only install the one
we want for that platform.

Andrew

> 
> 
>> diff --git a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
>> index d88bca6e..ed1c7817 100644
>> --- a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
>> @@ -10,8 +10,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "j721e|j721s2|j784s4"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET = "mhdp8546.bin"
>>   
>>   do_install() {
>> diff --git a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
>> index 5b1d8be1..ef7bc2ad 100644
>> --- a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
>> @@ -12,8 +12,6 @@ PR = "${INC_PR}.1"
>>   
>>   COMPATIBLE_MACHINE = "j721s2|j784s4|am62axx"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET_WAVE521C = "wave521c_codec_fw.bin"
>>   
>>   SOURCE_WAVE521C = "wave521c_k3_codec_fw.bin"
>> diff --git a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
>> index 6e2996ce..e333d212 100755
>> --- a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
>> @@ -4,7 +4,6 @@ LICENSE = "TI-TFL"
>>   LIC_FILES_CHKSUM = "file://LICENSE.ti;md5=04ad0a015d4bb63c2b9e7b112debf3db"
>>   
>>   PV = "6.2+git${SRCPV}"
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>   
>>   inherit update-alternatives
>>   
>> diff --git a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
>> index e58f2d58..ee3a94dc 100644
>> --- a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
>> @@ -10,8 +10,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "dra7xx"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   ORIGIN = "DRA71x-RevA-GT9271_SpecDig_Config.bin"
>>   TARGET = "goodix_9271_cfg.bin"
>>   
>> diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
>> index 2c0736ed..4b6ef75d 100644
>> --- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET = " \
>>       am65x-sr2-pru0-prueth-fw.elf \
>>       am65x-sr2-pru1-prueth-fw.elf \
>> diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
>> index 8b15ab7f..20b2bfb9 100644
>> --- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "am65xx"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET = " \
>>       am65x-pru0-prueth-fw.elf \
>>       am65x-pru1-prueth-fw.elf \
>> diff --git a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
>> index ea39d73d..bc731094 100644
>> --- a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET = " \
>>       am65x-sr2-pru0-pruhsr-fw.elf \
>>       am65x-sr2-pru1-pruhsr-fw.elf \
>> diff --git a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
>> index 63c2d311..6e296e7c 100644
>> --- a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET = " \
>>       am65x-sr2-pru0-prusw-fw.elf \
>>       am65x-sr2-pru1-prusw-fw.elf \
>> diff --git a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
>> index d295a1c1..74729c16 100644
>> --- a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
>> +++ b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
>> @@ -9,8 +9,6 @@ PR = "${INC_PR}.0"
>>   
>>   COMPATIBLE_MACHINE = "j721e"
>>   
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>> -
>>   TARGET = "pvdec_full_bin.fw"
>>   
>>   do_install() {
>> diff --git a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
>> index 7d16ae39..4ec09a70 100644
>> --- a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
>> +++ b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
>> @@ -3,7 +3,6 @@ LICENSE = "TI-TSPA"
>>   LIC_FILES_CHKSUM = "file://${S}/J6_VIS_DEMO_LINUX_BINARY_01.50.07.15-Manifest.html;md5=a59aa54b9470f555cf086b91dca0afa3"
>>   
>>   COMPATIBLE_MACHINE = "dra7xx"
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>   
>>   PR = "r1"
>>   
>> diff --git a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
>> index 2452d111..8af49577 100644
>> --- a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
>> +++ b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
>> @@ -4,7 +4,6 @@ LICENSE = "TI-TSPA"
>>   LIC_FILES_CHKSUM = "file://COPYING;md5=fd463c9500441ed91d07a0331baa635c"
>>   
>>   COMPATIBLE_MACHINE = "dra7xx"
>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>   
>>   SRC_URI = "http://downloads.ti.com/dsps/dsps_public_sw/glsdk/vpdma-fw/03-2012/exports/vpdma-fw_03-2012.tar.gz;protocol=http;name=dra7xx-evm"
>>   SRC_URI[dra7xx-evm.md5sum] = "80176df1350c21d9efa90171789c546e"
>> -- 
>> 2.39.2
Ryan Eatmon Nov. 2, 2023, 2:57 p.m. UTC | #3
On 10/26/2023 9:00 AM, Andrew Davis wrote:
> On 10/25/23 10:27 PM, Denys Dmytriyenko wrote:
>> On Wed, Oct 25, 2023 at 11:56:30AM -0500, Andrew Davis via 
>> lists.yoctoproject.org wrote:
>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>> ---
>>>   meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
>>>   meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
>>>   meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
>>>   meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
>>>   meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
>>>   meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
>>>   meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
>>>   meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
>>>   meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
>>>   meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
>>>   meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
>>>   11 files changed, 19 deletions(-)
>>
>> Overall I agree and fully support the first 7 patches in this series.
>>
>> But for this last one I wanted to open a discussion.
>>
> 
> I try to sort my series in least-to-most likely to be controversial, I was
> just wondering how far we would get down the list, glad we got to 8 :)
> 
>> On one hand I understand the desire to make components as generic as 
>> possible
>> and reduce the number of machine-specific components to a bare minimum.
>>
>> But on another hand, marking the resulting package as machine-specific 
>> when it
>> has a short list of compatible machines is a standard practice. The 
>> reason is
>> that the list of compatible machines controls only compile time 
>> filtering, but
>> doesn't have any effect on run time. And marking packages as machine 
>> specific
>> helps with that. That closes the loophole of installing incompatible 
>> packages.
>>
>> For example, first recipe below specifies that Cadence MHDP firmware is
>> compatible with 3 J7 platforms only (or their SoC families, to be exact).
>> But w/o marking resulting binary package as machine-specific (therefore
>> producing separate packages for those platforms), there will be a single
>> generic Aarch64 package made. And there's no protection from installing
>> this generic package on non-compatible platforms, like J7200 or AM65xx,
>> either manullay or by pulling it into a rootfs for those incompatible
>> platforms.
>>
>> And you normally want to prevent this for regular components. But I guess
>> this doesn't fully apply to FW images that are loaded by corresponding
>> drivers anyway. Moreover, there's no compilation involved, just packaging
>> the binary blob.
>>
>> In that case, should we also remove COMPATIBLE_MACHINE from these 
>> firmware
>> recipes?
>>
> 
> That is exactly where I was going to go next. These firmware packages are
> not technically incompatible with the other machines.
> 
> We just use COMPATIBLE_MACHINE checks here to keep us from accidentily 
> bundling
> them with images where they wouldn't add any value (which you did with 
> prusw-fw
> which is what stated me thinking on all this). But since they don't break
> anything either, forcing them to be machine specific seemed like 
> overkill also.
> 
> Only place where we still need this is firmware recipes that only ship 
> some of
> the firmware based on machine (see prueth-fw for instance). I'd like to get
> that cleaned up next. If we only want some of the firmware then we 
> should split
> it into different packages (prueth-fw-am57xx, etc.) and only install the 
> one
> we want for that platform.
> 
> Andrew


Denys, does Andrew's response address your concerns?



>>
>>
>>> diff --git 
>>> a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
>>> index d88bca6e..ed1c7817 100644
>>> --- a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
>>> @@ -10,8 +10,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "j721e|j721s2|j784s4"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET = "mhdp8546.bin"
>>>   do_install() {
>>> diff --git a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
>>> index 5b1d8be1..ef7bc2ad 100644
>>> --- a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
>>> @@ -12,8 +12,6 @@ PR = "${INC_PR}.1"
>>>   COMPATIBLE_MACHINE = "j721s2|j784s4|am62axx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET_WAVE521C = "wave521c_codec_fw.bin"
>>>   SOURCE_WAVE521C = "wave521c_k3_codec_fw.bin"
>>> diff --git 
>>> a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
>>> index 6e2996ce..e333d212 100755
>>> --- a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
>>> @@ -4,7 +4,6 @@ LICENSE = "TI-TFL"
>>>   LIC_FILES_CHKSUM = 
>>> "file://LICENSE.ti;md5=04ad0a015d4bb63c2b9e7b112debf3db"
>>>   PV = "6.2+git${SRCPV}"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>>   inherit update-alternatives
>>> diff --git a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
>>> index e58f2d58..ee3a94dc 100644
>>> --- a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
>>> @@ -10,8 +10,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "dra7xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   ORIGIN = "DRA71x-RevA-GT9271_SpecDig_Config.bin"
>>>   TARGET = "goodix_9271_cfg.bin"
>>> diff --git 
>>> a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
>>> index 2c0736ed..4b6ef75d 100644
>>> --- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
>>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET = " \
>>>       am65x-sr2-pru0-prueth-fw.elf \
>>>       am65x-sr2-pru1-prueth-fw.elf \
>>> diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
>>> index 8b15ab7f..20b2bfb9 100644
>>> --- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
>>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "am65xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET = " \
>>>       am65x-pru0-prueth-fw.elf \
>>>       am65x-pru1-prueth-fw.elf \
>>> diff --git 
>>> a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
>>> index ea39d73d..bc731094 100644
>>> --- a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
>>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET = " \
>>>       am65x-sr2-pru0-pruhsr-fw.elf \
>>>       am65x-sr2-pru1-pruhsr-fw.elf \
>>> diff --git 
>>> a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
>>> index 63c2d311..6e296e7c 100644
>>> --- a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
>>> @@ -7,8 +7,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET = " \
>>>       am65x-sr2-pru0-prusw-fw.elf \
>>>       am65x-sr2-pru1-prusw-fw.elf \
>>> diff --git 
>>> a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb 
>>> b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
>>> index d295a1c1..74729c16 100644
>>> --- a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
>>> @@ -9,8 +9,6 @@ PR = "${INC_PR}.0"
>>>   COMPATIBLE_MACHINE = "j721e"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>> -
>>>   TARGET = "pvdec_full_bin.fw"
>>>   do_install() {
>>> diff --git a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb 
>>> b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
>>> index 7d16ae39..4ec09a70 100644
>>> --- a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
>>> @@ -3,7 +3,6 @@ LICENSE = "TI-TSPA"
>>>   LIC_FILES_CHKSUM = 
>>> "file://${S}/J6_VIS_DEMO_LINUX_BINARY_01.50.07.15-Manifest.html;md5=a59aa54b9470f555cf086b91dca0afa3"
>>>   COMPATIBLE_MACHINE = "dra7xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>>   PR = "r1"
>>> diff --git a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb 
>>> b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
>>> index 2452d111..8af49577 100644
>>> --- a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
>>> +++ b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
>>> @@ -4,7 +4,6 @@ LICENSE = "TI-TSPA"
>>>   LIC_FILES_CHKSUM = 
>>> "file://COPYING;md5=fd463c9500441ed91d07a0331baa635c"
>>>   COMPATIBLE_MACHINE = "dra7xx"
>>> -PACKAGE_ARCH = "${MACHINE_ARCH}"
>>>   SRC_URI = 
>>> "http://downloads.ti.com/dsps/dsps_public_sw/glsdk/vpdma-fw/03-2012/exports/vpdma-fw_03-2012.tar.gz;protocol=http;name=dra7xx-evm"
>>>   SRC_URI[dra7xx-evm.md5sum] = "80176df1350c21d9efa90171789c546e"
>>> -- 
>>> 2.39.2
Denys Dmytriyenko Nov. 3, 2023, 6:15 a.m. UTC | #4
On Thu, Nov 02, 2023 at 09:57:11AM -0500, Ryan Eatmon wrote:
> 
> 
> On 10/26/2023 9:00 AM, Andrew Davis wrote:
> >On 10/25/23 10:27 PM, Denys Dmytriyenko wrote:
> >>On Wed, Oct 25, 2023 at 11:56:30AM -0500, Andrew Davis via
> >>lists.yoctoproject.org wrote:
> >>>Signed-off-by: Andrew Davis <afd@ti.com>
> >>>---
> >>>  meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
> >>>  meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
> >>>  meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
> >>>  meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
> >>>  meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
> >>>  meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
> >>>  meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
> >>>  meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
> >>>  meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
> >>>  meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
> >>>  meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
> >>>  11 files changed, 19 deletions(-)
> >>
> >>Overall I agree and fully support the first 7 patches in this series.
> >>
> >>But for this last one I wanted to open a discussion.
> >>
> >
> >I try to sort my series in least-to-most likely to be controversial, I was
> >just wondering how far we would get down the list, glad we got to 8 :)
> >
> >>On one hand I understand the desire to make components as
> >>generic as possible
> >>and reduce the number of machine-specific components to a bare minimum.
> >>
> >>But on another hand, marking the resulting package as
> >>machine-specific when it
> >>has a short list of compatible machines is a standard practice.
> >>The reason is
> >>that the list of compatible machines controls only compile time
> >>filtering, but
> >>doesn't have any effect on run time. And marking packages as
> >>machine specific
> >>helps with that. That closes the loophole of installing
> >>incompatible packages.
> >>
> >>For example, first recipe below specifies that Cadence MHDP firmware is
> >>compatible with 3 J7 platforms only (or their SoC families, to be exact).
> >>But w/o marking resulting binary package as machine-specific (therefore
> >>producing separate packages for those platforms), there will be a single
> >>generic Aarch64 package made. And there's no protection from installing
> >>this generic package on non-compatible platforms, like J7200 or AM65xx,
> >>either manullay or by pulling it into a rootfs for those incompatible
> >>platforms.
> >>
> >>And you normally want to prevent this for regular components. But I guess
> >>this doesn't fully apply to FW images that are loaded by corresponding
> >>drivers anyway. Moreover, there's no compilation involved, just packaging
> >>the binary blob.
> >>
> >>In that case, should we also remove COMPATIBLE_MACHINE from
> >>these firmware
> >>recipes?
> >>
> >
> >That is exactly where I was going to go next. These firmware packages are
> >not technically incompatible with the other machines.
> >
> >We just use COMPATIBLE_MACHINE checks here to keep us from
> >accidentily bundling
> >them with images where they wouldn't add any value (which you did
> >with prusw-fw
> >which is what stated me thinking on all this). But since they don't break
> >anything either, forcing them to be machine specific seemed like
> >overkill also.
> >
> >Only place where we still need this is firmware recipes that only
> >ship some of
> >the firmware based on machine (see prueth-fw for instance). I'd like to get
> >that cleaned up next. If we only want some of the firmware then we
> >should split
> >it into different packages (prueth-fw-am57xx, etc.) and only
> >install the one
> >we want for that platform.
> 
> Denys, does Andrew's response address your concerns?

Well, I do believe we are generally in agreement here. But I'm not sure if 
that means we should merge patch #8 as is and address COMPATIBLE_MACHINE 
changes later, or rework the patch to address that with MACHINE_ARCH together?
Ryan Eatmon Nov. 3, 2023, 4:20 p.m. UTC | #5
On 11/3/2023 1:15 AM, Denys Dmytriyenko wrote:
> On Thu, Nov 02, 2023 at 09:57:11AM -0500, Ryan Eatmon wrote:
>>
>>
>> On 10/26/2023 9:00 AM, Andrew Davis wrote:
>>> On 10/25/23 10:27 PM, Denys Dmytriyenko wrote:
>>>> On Wed, Oct 25, 2023 at 11:56:30AM -0500, Andrew Davis via
>>>> lists.yoctoproject.org wrote:
>>>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>>>> ---
>>>>>    meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb         | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb     | 1 -
>>>>>    meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb             | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb   | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb       | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb   | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb     | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb | 2 --
>>>>>    meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb              | 1 -
>>>>>    meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb           | 1 -
>>>>>    11 files changed, 19 deletions(-)
>>>>
>>>> Overall I agree and fully support the first 7 patches in this series.
>>>>
>>>> But for this last one I wanted to open a discussion.
>>>>
>>>
>>> I try to sort my series in least-to-most likely to be controversial, I was
>>> just wondering how far we would get down the list, glad we got to 8 :)
>>>
>>>> On one hand I understand the desire to make components as
>>>> generic as possible
>>>> and reduce the number of machine-specific components to a bare minimum.
>>>>
>>>> But on another hand, marking the resulting package as
>>>> machine-specific when it
>>>> has a short list of compatible machines is a standard practice.
>>>> The reason is
>>>> that the list of compatible machines controls only compile time
>>>> filtering, but
>>>> doesn't have any effect on run time. And marking packages as
>>>> machine specific
>>>> helps with that. That closes the loophole of installing
>>>> incompatible packages.
>>>>
>>>> For example, first recipe below specifies that Cadence MHDP firmware is
>>>> compatible with 3 J7 platforms only (or their SoC families, to be exact).
>>>> But w/o marking resulting binary package as machine-specific (therefore
>>>> producing separate packages for those platforms), there will be a single
>>>> generic Aarch64 package made. And there's no protection from installing
>>>> this generic package on non-compatible platforms, like J7200 or AM65xx,
>>>> either manullay or by pulling it into a rootfs for those incompatible
>>>> platforms.
>>>>
>>>> And you normally want to prevent this for regular components. But I guess
>>>> this doesn't fully apply to FW images that are loaded by corresponding
>>>> drivers anyway. Moreover, there's no compilation involved, just packaging
>>>> the binary blob.
>>>>
>>>> In that case, should we also remove COMPATIBLE_MACHINE from
>>>> these firmware
>>>> recipes?
>>>>
>>>
>>> That is exactly where I was going to go next. These firmware packages are
>>> not technically incompatible with the other machines.
>>>
>>> We just use COMPATIBLE_MACHINE checks here to keep us from
>>> accidentily bundling
>>> them with images where they wouldn't add any value (which you did
>>> with prusw-fw
>>> which is what stated me thinking on all this). But since they don't break
>>> anything either, forcing them to be machine specific seemed like
>>> overkill also.
>>>
>>> Only place where we still need this is firmware recipes that only
>>> ship some of
>>> the firmware based on machine (see prueth-fw for instance). I'd like to get
>>> that cleaned up next. If we only want some of the firmware then we
>>> should split
>>> it into different packages (prueth-fw-am57xx, etc.) and only
>>> install the one
>>> we want for that platform.
>>
>> Denys, does Andrew's response address your concerns?
> 
> Well, I do believe we are generally in agreement here. But I'm not sure if
> that means we should merge patch #8 as is and address COMPATIBLE_MACHINE
> changes later, or rework the patch to address that with MACHINE_ARCH together?

Ok.  I'll accept 1-7 and reject 8 for now.  8 can be resubmitted once 
those additional changes are made and can be taken all at once.
diff mbox series

Patch

diff --git a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
index d88bca6e..ed1c7817 100644
--- a/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
+++ b/meta-ti-bsp/recipes-bsp/cadence-mhdp-fw/cadence-mhdp-fw_git.bb
@@ -10,8 +10,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "j721e|j721s2|j784s4"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET = "mhdp8546.bin"
 
 do_install() {
diff --git a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
index 5b1d8be1..ef7bc2ad 100644
--- a/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
+++ b/meta-ti-bsp/recipes-bsp/cnm-wave-fw/cnm-wave-fw_git.bb
@@ -12,8 +12,6 @@  PR = "${INC_PR}.1"
 
 COMPATIBLE_MACHINE = "j721s2|j784s4|am62axx"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET_WAVE521C = "wave521c_codec_fw.bin"
 
 SOURCE_WAVE521C = "wave521c_k3_codec_fw.bin"
diff --git a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
index 6e2996ce..e333d212 100755
--- a/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
+++ b/meta-ti-bsp/recipes-bsp/cpsw9g-eth-fw/cpsw9g-eth-fw_git.bb
@@ -4,7 +4,6 @@  LICENSE = "TI-TFL"
 LIC_FILES_CHKSUM = "file://LICENSE.ti;md5=04ad0a015d4bb63c2b9e7b112debf3db"
 
 PV = "6.2+git${SRCPV}"
-PACKAGE_ARCH = "${MACHINE_ARCH}"
 
 inherit update-alternatives
 
diff --git a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
index e58f2d58..ee3a94dc 100644
--- a/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
+++ b/meta-ti-bsp/recipes-bsp/goodix-fw/goodix-fw_git.bb
@@ -10,8 +10,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "dra7xx"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 ORIGIN = "DRA71x-RevA-GT9271_SpecDig_Config.bin"
 TARGET = "goodix_9271_cfg.bin"
 
diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
index 2c0736ed..4b6ef75d 100644
--- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
+++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x-sr2_git.bb
@@ -7,8 +7,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET = " \
     am65x-sr2-pru0-prueth-fw.elf \
     am65x-sr2-pru1-prueth-fw.elf \
diff --git a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
index 8b15ab7f..20b2bfb9 100644
--- a/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
+++ b/meta-ti-bsp/recipes-bsp/prueth-fw/prueth-fw-am65x_git.bb
@@ -7,8 +7,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "am65xx"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET = " \
     am65x-pru0-prueth-fw.elf \
     am65x-pru1-prueth-fw.elf \
diff --git a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
index ea39d73d..bc731094 100644
--- a/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
+++ b/meta-ti-bsp/recipes-bsp/pruhsr-fw/pruhsr-fw-am65x-sr2_git.bb
@@ -7,8 +7,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET = " \
     am65x-sr2-pru0-pruhsr-fw.elf \
     am65x-sr2-pru1-pruhsr-fw.elf \
diff --git a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
index 63c2d311..6e296e7c 100644
--- a/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
+++ b/meta-ti-bsp/recipes-bsp/prusw-fw/prusw-fw-am65x-sr2_git.bb
@@ -7,8 +7,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "am65xx-evm|am64xx"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET = " \
     am65x-sr2-pru0-prusw-fw.elf \
     am65x-sr2-pru1-prusw-fw.elf \
diff --git a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
index d295a1c1..74729c16 100644
--- a/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
+++ b/meta-ti-bsp/recipes-bsp/ti-img-encode-decode/vxd-dec-fw_git.bb
@@ -9,8 +9,6 @@  PR = "${INC_PR}.0"
 
 COMPATIBLE_MACHINE = "j721e"
 
-PACKAGE_ARCH = "${MACHINE_ARCH}"
-
 TARGET = "pvdec_full_bin.fw"
 
 do_install() {
diff --git a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
index 7d16ae39..4ec09a70 100644
--- a/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
+++ b/meta-ti-bsp/recipes-bsp/vis-fw/vis_01.50.07.15.bb
@@ -3,7 +3,6 @@  LICENSE = "TI-TSPA"
 LIC_FILES_CHKSUM = "file://${S}/J6_VIS_DEMO_LINUX_BINARY_01.50.07.15-Manifest.html;md5=a59aa54b9470f555cf086b91dca0afa3"
 
 COMPATIBLE_MACHINE = "dra7xx"
-PACKAGE_ARCH = "${MACHINE_ARCH}"
 
 PR = "r1"
 
diff --git a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
index 2452d111..8af49577 100644
--- a/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
+++ b/meta-ti-bsp/recipes-bsp/vpdma-fw/vpdma-fw_03-2012.bb
@@ -4,7 +4,6 @@  LICENSE = "TI-TSPA"
 LIC_FILES_CHKSUM = "file://COPYING;md5=fd463c9500441ed91d07a0331baa635c"
 
 COMPATIBLE_MACHINE = "dra7xx"
-PACKAGE_ARCH = "${MACHINE_ARCH}"
 
 SRC_URI = "http://downloads.ti.com/dsps/dsps_public_sw/glsdk/vpdma-fw/03-2012/exports/vpdma-fw_03-2012.tar.gz;protocol=http;name=dra7xx-evm"
 SRC_URI[dra7xx-evm.md5sum] = "80176df1350c21d9efa90171789c546e"