[meta-ti,dunfell,v2,1/2] conf: machine: am62xx-evm: Add Support for AM62xx HS-SE

- Add machine conf for AM62xx HS-SE.
- The wic images will boot on AM62x GP devices by default. To boot on AM62x HS-SE, simply switch out the SYSFW image:

$ cd /mnt/sd-card/boot
$ mv tiboot3-am62x-hs-evm.bin tiboot3.bin

Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
---
 ...-evm-k3r5.conf => am62xx-evm-k3r5-gp.conf} |  3 +++
 conf/machine/am62xx-evm-k3r5-hs-se.conf       | 21 +++++++++++++++++++
 conf/machine/am62xx-evm.conf                  |  4 ++++
 conf/machine/include/am62xx.inc               | 11 ++++++++--
 4 files changed, 37 insertions(+), 2 deletions(-)
 rename conf/machine/{am62xx-evm-k3r5.conf => am62xx-evm-k3r5-gp.conf} (67%)
 create mode 100644 conf/machine/am62xx-evm-k3r5-hs-se.conf

On 1/23/23 11:14 PM, Chirag Shilwant via lists.yoctoproject.org wrote:
> - Add machine conf for AM62xx HS-SE.
> - The wic images will boot on AM62x GP devices by default. To boot on AM62x HS-SE, simply switch out the SYSFW image:
> 
> $ cd /mnt/sd-card/boot
> $ mv tiboot3-am62x-hs-evm.bin tiboot3.bin
> 
> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>

You should collect the Reviewed-by/Acked-by from previous
versions of this patch here.

> ---

And put what changed between versions right here.

Andrew

>   ...-evm-k3r5.conf => am62xx-evm-k3r5-gp.conf} |  3 +++
>   conf/machine/am62xx-evm-k3r5-hs-se.conf       | 21 +++++++++++++++++++
>   conf/machine/am62xx-evm.conf                  |  4 ++++
>   conf/machine/include/am62xx.inc               | 11 ++++++++--
>   4 files changed, 37 insertions(+), 2 deletions(-)
>   rename conf/machine/{am62xx-evm-k3r5.conf => am62xx-evm-k3r5-gp.conf} (67%)
>   create mode 100644 conf/machine/am62xx-evm-k3r5-hs-se.conf
> 
> diff --git a/conf/machine/am62xx-evm-k3r5.conf b/conf/machine/am62xx-evm-k3r5-gp.conf
> similarity index 67%
> rename from conf/machine/am62xx-evm-k3r5.conf
> rename to conf/machine/am62xx-evm-k3r5-gp.conf
> index 724a8d4a..d9867bc7 100644
> --- a/conf/machine/am62xx-evm-k3r5.conf
> +++ b/conf/machine/am62xx-evm-k3r5-gp.conf
> @@ -9,3 +9,6 @@ SYSFW_CONFIG = "evm"
>   SYSFW_SUFFIX = "gp"
>   
>   UBOOT_MACHINE = "am62x_evm_r5_defconfig"
> +
> +UBOOT_BINARY = "u-boot-spl.${UBOOT_SUFFIX}"
> +UBOOT_IMAGE = "u-boot-r5spl-gp-${MAINMACHINE}-${PV}-${PR}.${UBOOT_SUFFIX}"
> diff --git a/conf/machine/am62xx-evm-k3r5-hs-se.conf b/conf/machine/am62xx-evm-k3r5-hs-se.conf
> new file mode 100644
> index 00000000..913e69fa
> --- /dev/null
> +++ b/conf/machine/am62xx-evm-k3r5-hs-se.conf
> @@ -0,0 +1,21 @@
> +#@TYPE: Machine
> +#@NAME: AM62xx HS-SE EVM (R5F)
> +#@DESCRIPTION: Machine configuration for the TI AM62xx HS-SE EVM (R5F core)
> +
> +# Booting HS-SE requires different SYSFW, the rest is handled at runtime
> +
> +require conf/machine/include/k3r5.inc
> +SOC_FAMILY_append = ":k3r5-hs-se"
> +
> +SYSFW_SOC = "am62x"
> +SYSFW_CONFIG = "evm"
> +SYSFW_SUFFIX = "hs"
> +
> +SYSFW_TIBOOT3_SYMLINK = ""
> +
> +UBOOT_MACHINE = "am62x_evm_r5_defconfig"
> +
> +SPL_BINARY = ""
> +UBOOT_BINARY = "u-boot-spl.${UBOOT_SUFFIX}"
> +UBOOT_IMAGE = "u-boot-r5spl-hs-se-${MAINMACHINE}-${PV}-${PR}.${UBOOT_SUFFIX}"
> +UBOOT_SYMLINK = "u-boot-r5spl-hs-se.${UBOOT_SUFFIX}"
> diff --git a/conf/machine/am62xx-evm.conf b/conf/machine/am62xx-evm.conf
> index 3d9e3b90..b4904eca 100644
> --- a/conf/machine/am62xx-evm.conf
> +++ b/conf/machine/am62xx-evm.conf
> @@ -19,3 +19,7 @@ KERNEL_DEVICETREE = " \
>   "
>   
>   UBOOT_MACHINE = "am62x_evm_a53_defconfig"
> +
> +SPL_BINARY = "tispl.bin_HS"
> +UBOOT_BINARY = "u-boot.img_HS"
> +UBOOT_SYMLINK = "u-boot.img"
> diff --git a/conf/machine/include/am62xx.inc b/conf/machine/include/am62xx.inc
> index 0231235f..912318e9 100644
> --- a/conf/machine/include/am62xx.inc
> +++ b/conf/machine/include/am62xx.inc
> @@ -8,8 +8,15 @@ SERIAL_CONSOLES_CHECK = "${SERIAL_CONSOLES}"
>   
>   TFA_K3_SYSTEM_SUSPEND = "1"
>   
> -do_image_wic[mcdepends] = "mc::k3r5:ti-sci-fw:do_deploy"
> -do_image_tar[mcdepends] = "mc::k3r5:ti-sci-fw:do_deploy"
> +BBMULTICONFIG = "k3r5-gp"
> +do_image_wic[mcdepends] = "mc::k3r5-gp:ti-sci-fw:do_deploy"
> +do_image_tar[mcdepends] = "mc::k3r5-gp:ti-sci-fw:do_deploy"
> +
> +# Since default tiboot3.bin on AM62x is for GP, add a version for HS-SE
> +BBMULTICONFIG += "k3r5-hs-se"
> +IMAGE_BOOT_FILES += " tiboot3-am62x-hs-evm.bin"
> +do_image_wic[mcdepends] += "mc::k3r5-hs-se:ti-sci-fw:do_deploy"
> +do_image_tar[mcdepends] += "mc::k3r5-hs-se:ti-sci-fw:do_deploy"
>   
>   TFA_BOARD = "lite"
>   OPTEEMACHINE = "k3-am62x"
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#15638): https://lists.yoctoproject.org/g/meta-ti/message/15638
> Mute This Topic: https://lists.yoctoproject.org/mt/96491882/3619733
> Group Owner: meta-ti+owner@lists.yoctoproject.org
> Unsubscribe: https://lists.yoctoproject.org/g/meta-ti/unsub [afd@ti.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

I would prefer to see this as a single patch.  Adding in support for a 
new platform should roll-up all of the needed changes at the same time 
and not sequence them out in multiple commits.  The only time we do this 
is when not all of the pieces of firmware are in place but we want to go 
ahead and add the platform into the builds.

Also, please address Andrew's question about the DM firmware, and about 
pulling in past Reviewed-by/Acked-by on the patches.


On 1/23/2023 23:14, Chirag Shilwant wrote:
> - Add machine conf for AM62xx HS-SE.
> - The wic images will boot on AM62x GP devices by default. To boot on AM62x HS-SE, simply switch out the SYSFW image:
> 
> $ cd /mnt/sd-card/boot
> $ mv tiboot3-am62x-hs-evm.bin tiboot3.bin
> 
> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> ---
>   ...-evm-k3r5.conf => am62xx-evm-k3r5-gp.conf} |  3 +++
>   conf/machine/am62xx-evm-k3r5-hs-se.conf       | 21 +++++++++++++++++++
>   conf/machine/am62xx-evm.conf                  |  4 ++++
>   conf/machine/include/am62xx.inc               | 11 ++++++++--
>   4 files changed, 37 insertions(+), 2 deletions(-)
>   rename conf/machine/{am62xx-evm-k3r5.conf => am62xx-evm-k3r5-gp.conf} (67%)
>   create mode 100644 conf/machine/am62xx-evm-k3r5-hs-se.conf
> 
> diff --git a/conf/machine/am62xx-evm-k3r5.conf b/conf/machine/am62xx-evm-k3r5-gp.conf
> similarity index 67%
> rename from conf/machine/am62xx-evm-k3r5.conf
> rename to conf/machine/am62xx-evm-k3r5-gp.conf
> index 724a8d4a..d9867bc7 100644
> --- a/conf/machine/am62xx-evm-k3r5.conf
> +++ b/conf/machine/am62xx-evm-k3r5-gp.conf
> @@ -9,3 +9,6 @@ SYSFW_CONFIG = "evm"
>   SYSFW_SUFFIX = "gp"
>   
>   UBOOT_MACHINE = "am62x_evm_r5_defconfig"
> +
> +UBOOT_BINARY = "u-boot-spl.${UBOOT_SUFFIX}"
> +UBOOT_IMAGE = "u-boot-r5spl-gp-${MAINMACHINE}-${PV}-${PR}.${UBOOT_SUFFIX}"
> diff --git a/conf/machine/am62xx-evm-k3r5-hs-se.conf b/conf/machine/am62xx-evm-k3r5-hs-se.conf
> new file mode 100644
> index 00000000..913e69fa
> --- /dev/null
> +++ b/conf/machine/am62xx-evm-k3r5-hs-se.conf
> @@ -0,0 +1,21 @@
> +#@TYPE: Machine
> +#@NAME: AM62xx HS-SE EVM (R5F)
> +#@DESCRIPTION: Machine configuration for the TI AM62xx HS-SE EVM (R5F core)
> +
> +# Booting HS-SE requires different SYSFW, the rest is handled at runtime
> +
> +require conf/machine/include/k3r5.inc
> +SOC_FAMILY_append = ":k3r5-hs-se"
> +
> +SYSFW_SOC = "am62x"
> +SYSFW_CONFIG = "evm"
> +SYSFW_SUFFIX = "hs"
> +
> +SYSFW_TIBOOT3_SYMLINK = ""
> +
> +UBOOT_MACHINE = "am62x_evm_r5_defconfig"
> +
> +SPL_BINARY = ""
> +UBOOT_BINARY = "u-boot-spl.${UBOOT_SUFFIX}"
> +UBOOT_IMAGE = "u-boot-r5spl-hs-se-${MAINMACHINE}-${PV}-${PR}.${UBOOT_SUFFIX}"
> +UBOOT_SYMLINK = "u-boot-r5spl-hs-se.${UBOOT_SUFFIX}"
> diff --git a/conf/machine/am62xx-evm.conf b/conf/machine/am62xx-evm.conf
> index 3d9e3b90..b4904eca 100644
> --- a/conf/machine/am62xx-evm.conf
> +++ b/conf/machine/am62xx-evm.conf
> @@ -19,3 +19,7 @@ KERNEL_DEVICETREE = " \
>   "
>   
>   UBOOT_MACHINE = "am62x_evm_a53_defconfig"
> +
> +SPL_BINARY = "tispl.bin_HS"
> +UBOOT_BINARY = "u-boot.img_HS"
> +UBOOT_SYMLINK = "u-boot.img"
> diff --git a/conf/machine/include/am62xx.inc b/conf/machine/include/am62xx.inc
> index 0231235f..912318e9 100644
> --- a/conf/machine/include/am62xx.inc
> +++ b/conf/machine/include/am62xx.inc
> @@ -8,8 +8,15 @@ SERIAL_CONSOLES_CHECK = "${SERIAL_CONSOLES}"
>   
>   TFA_K3_SYSTEM_SUSPEND = "1"
>   
> -do_image_wic[mcdepends] = "mc::k3r5:ti-sci-fw:do_deploy"
> -do_image_tar[mcdepends] = "mc::k3r5:ti-sci-fw:do_deploy"
> +BBMULTICONFIG = "k3r5-gp"
> +do_image_wic[mcdepends] = "mc::k3r5-gp:ti-sci-fw:do_deploy"
> +do_image_tar[mcdepends] = "mc::k3r5-gp:ti-sci-fw:do_deploy"
> +
> +# Since default tiboot3.bin on AM62x is for GP, add a version for HS-SE
> +BBMULTICONFIG += "k3r5-hs-se"
> +IMAGE_BOOT_FILES += " tiboot3-am62x-hs-evm.bin"
> +do_image_wic[mcdepends] += "mc::k3r5-hs-se:ti-sci-fw:do_deploy"
> +do_image_tar[mcdepends] += "mc::k3r5-hs-se:ti-sci-fw:do_deploy"
>   
>   TFA_BOARD = "lite"
>   OPTEEMACHINE = "k3-am62x"

I would prefer to see this as a single patch.

> Well after we added AM62Q HS-SE and AM62A support in single commit, Praneeth has suggested us to split the patches if possible as reviewing becomes difficult if we have everything in single patch. Also, Denys mentioned that single patch or patch series will also do. Hence, to satisfy both conditions kept it as 2 separate patches in one series with all conf in a patch and signing stuff in other 

Yes.  But that single patch was trying to do two different platforms. 
So splitting them was needed.  In this case you are submitting support 
for a new platform but breaking the implementation into multiple commits.

Multiple commits is okay for times when you submitting multiple changes 
where each commit is stand alone, but taken together they add support 
for a feature/change.

In this case, the single change is adding the new platform and the 
multiple commits are not stand alone.  The first just defines the 
platform but it would still be incorrect.  And the second patch is 
pointless without the first.  So merging them into a single commit make 
sense.


On 1/24/2023 23:18, Shilwant, Chirag wrote:
> I would prefer to see this as a single patch.
> 
>> Well after we added AM62Q HS-SE and AM62A support in single commit, Praneeth has suggested us to split the patchesif possible as reviewing becomes difficult if we have everything in 
> single patch. Also, Denys mentioned that single patch or patch series 
> will also do. Hence, to satisfy both conditions kept it as 2 separate 
> patches in one series with all conf in a patch and signing stuff in other 

And one of the key goals is to keep the tree bisectable, so when adding 
multiple changes as separate patches, the order is crucial.


On Wed, Jan 25, 2023 at 02:44:44PM -0600, Ryan Eatmon wrote:
> 
> Yes.  But that single patch was trying to do two different
> platforms. So splitting them was needed.  In this case you are
> submitting support for a new platform but breaking the
> implementation into multiple commits.
> 
> Multiple commits is okay for times when you submitting multiple
> changes where each commit is stand alone, but taken together they
> add support for a feature/change.
> 
> In this case, the single change is adding the new platform and the
> multiple commits are not stand alone.  The first just defines the
> platform but it would still be incorrect.  And the second patch is
> pointless without the first.  So merging them into a single commit
> make sense.
> 
> 
> On 1/24/2023 23:18, Shilwant, Chirag wrote:
> >I would prefer to see this as a single patch.
> >
> >>Well after we added AM62Q HS-SE and AM62A support in single
> >>commit, Praneeth has suggested us to split the patchesif
> >>possible as reviewing becomes difficult if we have everything in
> >single patch. Also, Denys mentioned that single patch or patch
> >series will also do. Hence, to satisfy both conditions kept it as
> >2 separate patches in one series with all conf in a patch and
> >signing stuff in other