diff mbox series

[meta-rockchip,5/6] bsp: rkbin: ddr: factor out do_deploy to be SoC-agnostic

Message ID 20250311-ddrbin-custom-v1-5-e5c994ac25e1@cherry.de
State New
Headers show
Series rkbin: factoring ddrbin do_deploy, customize ddrbin and bump rkbin | expand

Commit Message

Quentin Schulz March 11, 2025, 11:26 a.m. UTC 
From: Quentin Schulz <quentin.schulz@cherry.de>

The do_deploy task is essentially the same for all SoCs, install a file
from a specific path to another one.

No magic involved, so let's rather have one generic do_deploy task. For
this to work nicely, we check that all necessary variables are set and
notify the developer otherwise. This may be useful whenever a new SoC
will be supported by this recipe.

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb | 33 +++++++++++++++--------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Comments

Trevor Woerner April 15, 2025, 6:07 p.m. UTC | #1 
On Tue 2025-03-11 @ 12:26:34 PM, Quentin Schulz via lists.yoctoproject.org wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> The do_deploy task is essentially the same for all SoCs, install a file
> from a specific path to another one.
> 
> No magic involved, so let's rather have one generic do_deploy task. For
> this to work nicely, we check that all necessary variables are set and
> notify the developer otherwise. This may be useful whenever a new SoC
> will be supported by this recipe.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb | 33 +++++++++++++++--------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb b/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
> index 193d0f00a6868f050a0ff4531278b3296d3eae94..17131b6d5a6b443409eacb5806e0613b292852d7 100644
> --- a/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
> +++ b/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
> @@ -8,40 +8,41 @@ DDRBIN_FILE:rk3308 ?= "rk3308_ddr_589MHz_uart4_m0_${DDRBIN_VERS}.bin"
>  DDRBIN_DEPLOY_FILENAME:rk3308 ?= "ddr-rk3308.bin"
>  DDRBIN_TOOL_SOC:rk3308 ?= "rk3308"
>  
> -do_deploy:rk3308() {
> -	# Prebuilt U-Boot TPL (DDR init)
> -	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
> -}
> -
>  DDRBIN_DIR:rk3566 ?= "bin/rk35/"
>  DDRBIN_VERS:rk3566 ?= "v1.23"
>  DDRBIN_FILE:rk3566 ?= "rk3566_ddr_1056MHz_${DDRBIN_VERS}.bin"
>  DDRBIN_DEPLOY_FILENAME:rk3566 ?= "ddr-rk3566.bin"
>  DDRBIN_TOOL_SOC:rk3566 ?= "rk356x"
>  
> -do_deploy:rk3566() {
> -	# Prebuilt U-Boot TPL (DDR init)
> -	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
> -}
> -
>  DDRBIN_DIR:rk3568 ?= "bin/rk35/"
>  DDRBIN_VERS:rk3568 ?= "v1.23"
>  DDRBIN_FILE:rk3568 ?= "rk3568_ddr_1560MHz_${DDRBIN_VERS}.bin"
>  DDRBIN_DEPLOY_FILENAME:rk3568 ?= "ddr-rk3568.bin"
>  DDRBIN_TOOL_SOC:rk3568 ?= "rk356x"
>  
> -do_deploy:rk3568() {
> -	# Prebuilt U-Boot TPL (DDR init)
> -	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
> -}
> -
>  DDRBIN_DIR:rk3588s ?= "bin/rk35/"
>  DDRBIN_VERS:rk3588s ?= "v1.18"
>  DDRBIN_FILE:rk3588s ?= "rk3588_ddr_lp4_2112MHz_lp5_2400MHz_${DDRBIN_VERS}.bin"
>  DDRBIN_DEPLOY_FILENAME:rk3588s ?= "ddr-rk3588.bin"
>  DDRBIN_TOOL_SOC:rk3588s ?= "rk3588"
>  
> -do_deploy:rk3588s() {
> +do_deploy() {
> +	if [ -z "${DDRBIN_DIR}" ]; then
> +		bbfatal "Non-empty DDRBIN_DIR:<MACHINE> required!"
> +	fi
> +
> +	if [ -z "${DDRBIN_VERS}" ]; then
> +		bbfatal "Non-empty DDRBIN_VERS:<MACHINE> required!"
> +	fi
> +
> +	if [ -z "${DDRBIN_FILE}" ]; then
> +		bbfatal "Non-empty DDRBIN_FILE:<MACHINE> required!"
> +	fi
> +
> +	if [ -z "${DDRBIN_DEPLOY_FILENAME}" ]; then
> +		bbfatal "Non-empty DDRBIN_DEPLOY_FILENAME:<MACHINE> required!"
> +	fi
> +
>  	# Prebuilt U-Boot TPL (DDR init)
>  	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
>  }

This is nice, but the problem with this patch is that the rkbin handling
involves 4 files:
	- rockchip-rkbin.inc
	- rockchip-rkbin-ddr_git.bb
	- rockchip-rkbin-optee-os_git.bb
	- rockchip-rkbin-tf-a_git.bb

In the *inc file it says:

	 28 do_deploy() {
	 29         bbfatal "COMPATIBLE_MACHINE requires a corresponding do_deploy:<MACHINE>() override"
	 30 }

These machine-specific overrides are found in both:
	- rockchip-rkbin-optee-os_git.bb and -
	rockchip-rkbin-tf-a_git.bb
but, with this patch, not in rockchip-rkbin-ddr_git.bb.

I like what you've done, but I would prefer they all follow the same
"template". Either:
	1) modify rockchip-rkbin-optee-os_git.bb and
	rockchip-rkbin-tf-a_git.bb to match and update the bbfatal in
	rockchip-rkbin.inc so it makes sense, or

	2) skip this patch

I don't have a strong preference either way, but I do have a strong desire
that they all match.
Quentin Schulz April 16, 2025, 11:25 a.m. UTC | #2 
Hi Trevor,

On 4/15/25 8:07 PM, Trevor Woerner wrote:
> On Tue 2025-03-11 @ 12:26:34 PM, Quentin Schulz via lists.yoctoproject.org wrote:
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> The do_deploy task is essentially the same for all SoCs, install a file
>> from a specific path to another one.
>>
>> No magic involved, so let's rather have one generic do_deploy task. For
>> this to work nicely, we check that all necessary variables are set and
>> notify the developer otherwise. This may be useful whenever a new SoC
>> will be supported by this recipe.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
>> ---
>>   recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb | 33 +++++++++++++++--------------
>>   1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb b/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
>> index 193d0f00a6868f050a0ff4531278b3296d3eae94..17131b6d5a6b443409eacb5806e0613b292852d7 100644
>> --- a/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
>> +++ b/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
>> @@ -8,40 +8,41 @@ DDRBIN_FILE:rk3308 ?= "rk3308_ddr_589MHz_uart4_m0_${DDRBIN_VERS}.bin"
>>   DDRBIN_DEPLOY_FILENAME:rk3308 ?= "ddr-rk3308.bin"
>>   DDRBIN_TOOL_SOC:rk3308 ?= "rk3308"
>>   
>> -do_deploy:rk3308() {
>> -	# Prebuilt U-Boot TPL (DDR init)
>> -	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
>> -}
>> -
>>   DDRBIN_DIR:rk3566 ?= "bin/rk35/"
>>   DDRBIN_VERS:rk3566 ?= "v1.23"
>>   DDRBIN_FILE:rk3566 ?= "rk3566_ddr_1056MHz_${DDRBIN_VERS}.bin"
>>   DDRBIN_DEPLOY_FILENAME:rk3566 ?= "ddr-rk3566.bin"
>>   DDRBIN_TOOL_SOC:rk3566 ?= "rk356x"
>>   
>> -do_deploy:rk3566() {
>> -	# Prebuilt U-Boot TPL (DDR init)
>> -	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
>> -}
>> -
>>   DDRBIN_DIR:rk3568 ?= "bin/rk35/"
>>   DDRBIN_VERS:rk3568 ?= "v1.23"
>>   DDRBIN_FILE:rk3568 ?= "rk3568_ddr_1560MHz_${DDRBIN_VERS}.bin"
>>   DDRBIN_DEPLOY_FILENAME:rk3568 ?= "ddr-rk3568.bin"
>>   DDRBIN_TOOL_SOC:rk3568 ?= "rk356x"
>>   
>> -do_deploy:rk3568() {
>> -	# Prebuilt U-Boot TPL (DDR init)
>> -	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
>> -}
>> -
>>   DDRBIN_DIR:rk3588s ?= "bin/rk35/"
>>   DDRBIN_VERS:rk3588s ?= "v1.18"
>>   DDRBIN_FILE:rk3588s ?= "rk3588_ddr_lp4_2112MHz_lp5_2400MHz_${DDRBIN_VERS}.bin"
>>   DDRBIN_DEPLOY_FILENAME:rk3588s ?= "ddr-rk3588.bin"
>>   DDRBIN_TOOL_SOC:rk3588s ?= "rk3588"
>>   
>> -do_deploy:rk3588s() {
>> +do_deploy() {
>> +	if [ -z "${DDRBIN_DIR}" ]; then
>> +		bbfatal "Non-empty DDRBIN_DIR:<MACHINE> required!"
>> +	fi
>> +
>> +	if [ -z "${DDRBIN_VERS}" ]; then
>> +		bbfatal "Non-empty DDRBIN_VERS:<MACHINE> required!"
>> +	fi
>> +
>> +	if [ -z "${DDRBIN_FILE}" ]; then
>> +		bbfatal "Non-empty DDRBIN_FILE:<MACHINE> required!"
>> +	fi
>> +
>> +	if [ -z "${DDRBIN_DEPLOY_FILENAME}" ]; then
>> +		bbfatal "Non-empty DDRBIN_DEPLOY_FILENAME:<MACHINE> required!"
>> +	fi
>> +
>>   	# Prebuilt U-Boot TPL (DDR init)
>>   	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
>>   }
> 
> This is nice, but the problem with this patch is that the rkbin handling
> involves 4 files:
> 	- rockchip-rkbin.inc
> 	- rockchip-rkbin-ddr_git.bb
> 	- rockchip-rkbin-optee-os_git.bb
> 	- rockchip-rkbin-tf-a_git.bb
> 
> In the *inc file it says:
> 
> 	 28 do_deploy() {
> 	 29         bbfatal "COMPATIBLE_MACHINE requires a corresponding do_deploy:<MACHINE>() override"
> 	 30 }
> 
> These machine-specific overrides are found in both:
> 	- rockchip-rkbin-optee-os_git.bb and -
> 	rockchip-rkbin-tf-a_git.bb
> but, with this patch, not in rockchip-rkbin-ddr_git.bb.
> 

Which isn't an issue as do_deploy() is entirely overridden in 
rockchip-rkbin-ddr_git.bb so this message would never be shown for that 
recipe, but the others yes.

> I like what you've done, but I would prefer they all follow the same
> "template". Either:
> 	1) modify rockchip-rkbin-optee-os_git.bb and
> 	rockchip-rkbin-tf-a_git.bb to match and update the bbfatal in
> 	rockchip-rkbin.inc so it makes sense, or
> 

I can agree with consistency across similar recipes.

One of the issues is that we're using a shell glob at the moment to 
avoid specifying the exact version number for OP-TEE OS and TF-A 
binaries. If I remember correctly, I cannot have this pattern stored in 
a variable, but I could hardcode the version (which will require us to 
modify it for every rkbin bump we'll do) or use find with a glob stored 
in a variable instead.

If we want absolute consistency, we can simply hardcode the version 
number as well.

I could also move the whole do_deploy logic I added in 
recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb to rockchip-rkbin.inc and 
use recipe-agnostic variables. After all, we only ever install one 
binary in rockchip-rkbin-ddr, rockchip-rkbin-optee-os and 
rockchip-rkbin-tf-a so we would only need to specify the variables at 
the top of the file and the .inc will take care of deploying the proper 
file in the proper directory. What do you think? If we don't do that, 
then it doesn't make much sense to keep a default do_deploy() in the 
.inc, don't you think? And if we don't keep the default do_deploy() in 
the .inc, then we should probably not inherit deploy either?

I'll start working on a version that has hardcoded binary versions and 
moves the new do_deploy logic into the .inc, lemme know if you would 
rather have something different.

> 	2) skip this patch
> 
> I don't have a strong preference either way, but I do have a strong desire
> that they all match.

Fair enough :)

For what it's worth, you could skip the 5th patch and apply the others 
if you want, and I could work on a v2 for the 5th patch alone instead of 
making it a blocker for this series. I don't mind either way :)

Cheers,
Quentin
diff mbox series

Patch

diff --git a/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb b/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
index 193d0f00a6868f050a0ff4531278b3296d3eae94..17131b6d5a6b443409eacb5806e0613b292852d7 100644
--- a/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
+++ b/recipes-bsp/rkbin/rockchip-rkbin-ddr_git.bb
@@ -8,40 +8,41 @@  DDRBIN_FILE:rk3308 ?= "rk3308_ddr_589MHz_uart4_m0_${DDRBIN_VERS}.bin"
 DDRBIN_DEPLOY_FILENAME:rk3308 ?= "ddr-rk3308.bin"
 DDRBIN_TOOL_SOC:rk3308 ?= "rk3308"
 
-do_deploy:rk3308() {
-	# Prebuilt U-Boot TPL (DDR init)
-	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
-}
-
 DDRBIN_DIR:rk3566 ?= "bin/rk35/"
 DDRBIN_VERS:rk3566 ?= "v1.23"
 DDRBIN_FILE:rk3566 ?= "rk3566_ddr_1056MHz_${DDRBIN_VERS}.bin"
 DDRBIN_DEPLOY_FILENAME:rk3566 ?= "ddr-rk3566.bin"
 DDRBIN_TOOL_SOC:rk3566 ?= "rk356x"
 
-do_deploy:rk3566() {
-	# Prebuilt U-Boot TPL (DDR init)
-	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
-}
-
 DDRBIN_DIR:rk3568 ?= "bin/rk35/"
 DDRBIN_VERS:rk3568 ?= "v1.23"
 DDRBIN_FILE:rk3568 ?= "rk3568_ddr_1560MHz_${DDRBIN_VERS}.bin"
 DDRBIN_DEPLOY_FILENAME:rk3568 ?= "ddr-rk3568.bin"
 DDRBIN_TOOL_SOC:rk3568 ?= "rk356x"
 
-do_deploy:rk3568() {
-	# Prebuilt U-Boot TPL (DDR init)
-	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
-}
-
 DDRBIN_DIR:rk3588s ?= "bin/rk35/"
 DDRBIN_VERS:rk3588s ?= "v1.18"
 DDRBIN_FILE:rk3588s ?= "rk3588_ddr_lp4_2112MHz_lp5_2400MHz_${DDRBIN_VERS}.bin"
 DDRBIN_DEPLOY_FILENAME:rk3588s ?= "ddr-rk3588.bin"
 DDRBIN_TOOL_SOC:rk3588s ?= "rk3588"
 
-do_deploy:rk3588s() {
+do_deploy() {
+	if [ -z "${DDRBIN_DIR}" ]; then
+		bbfatal "Non-empty DDRBIN_DIR:<MACHINE> required!"
+	fi
+
+	if [ -z "${DDRBIN_VERS}" ]; then
+		bbfatal "Non-empty DDRBIN_VERS:<MACHINE> required!"
+	fi
+
+	if [ -z "${DDRBIN_FILE}" ]; then
+		bbfatal "Non-empty DDRBIN_FILE:<MACHINE> required!"
+	fi
+
+	if [ -z "${DDRBIN_DEPLOY_FILENAME}" ]; then
+		bbfatal "Non-empty DDRBIN_DEPLOY_FILENAME:<MACHINE> required!"
+	fi
+
 	# Prebuilt U-Boot TPL (DDR init)
 	install -m 644 ${S}/${DDRBIN_DIR}${DDRBIN_FILE} ${DEPLOYDIR}/${DDRBIN_DEPLOY_FILENAME}
 }