diff mbox series

[meta-ti,master/scarthgap,v3] conf: machine: am62*-evm-k3r5.conf: apply fragment for enabling USB DFU

Message ID 20240708123653.2200919-1-s-vadapalli@ti.com
State Superseded
Delegated to: Ryan Eatmon
Headers show
Series [meta-ti,master/scarthgap,v3] conf: machine: am62*-evm-k3r5.conf: apply fragment for enabling USB DFU | expand

Commit Message

Siddharth Vadapalli July 8, 2024, 12:36 p.m. UTC
The config fragment "am62x_r5_usbdfu.config" is applicable to AM62x,
AM62x-SIP, AM62x-LP-EVM, AM62Ax and AM62Px devices and enables USB DFU boot
support at the R5 stage of boot.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

Hello, 

This patch is based on commit
ff9bf7b9 CI/CD Auto-Merger: cicd.scarthgap.202407071800
of scarthgap-next branch of meta-ti tree.

v2:
https://lore.kernel.org/r/20240708060054.1985156-1-s-vadapalli@ti.com/
Changes since v2:
- Based on feedback from Chirag Shilwant <c-shilwant@ti.com> at:
  https://lore.kernel.org/r/32db66e5-730c-42ec-9145-72abdf35b6d1@ti.com/
  the subject-prefix has been updated to master/scarthgap and the config
  fragment for USB DFU is being applied to AM62x SIP as well.

v1:
https://lore.kernel.org/r/20240708052457.1974547-1-s-vadapalli@ti.com/
Changes since v1:
- As pointed out by Vignesh Raghavendra <vigneshr@ti.com> at:
  https://lore.kernel.org/r/829dc5b5-e87c-4679-9e2d-7417c744a9c9@ti.com/
  the A53 defconfigs already include the "am62x_a53_usbdfu.config" config
  fragment. Hence, they are not required in the am62*-evm.conf files and
  have been dropped in this patch.
- Commit message has been updated accordingly.

Regards,
Siddharth.

 meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf   | 1 +
 meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf   | 1 +
 meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf    | 1 +
 meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf | 1 +
 meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf | 1 +
 5 files changed, 5 insertions(+)

Comments

Ryan Eatmon July 8, 2024, 2:04 p.m. UTC | #1
On 7/8/2024 7:36 AM, Siddharth Vadapalli wrote:
> The config fragment "am62x_r5_usbdfu.config" is applicable to AM62x,
> AM62x-SIP, AM62x-LP-EVM, AM62Ax and AM62Px devices and enables USB DFU boot
> support at the R5 stage of boot.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on commit
> ff9bf7b9 CI/CD Auto-Merger: cicd.scarthgap.202407071800
> of scarthgap-next branch of meta-ti tree.
> 
> v2:
> https://lore.kernel.org/r/20240708060054.1985156-1-s-vadapalli@ti.com/
> Changes since v2:
> - Based on feedback from Chirag Shilwant <c-shilwant@ti.com> at:
>    https://lore.kernel.org/r/32db66e5-730c-42ec-9145-72abdf35b6d1@ti.com/
>    the subject-prefix has been updated to master/scarthgap and the config
>    fragment for USB DFU is being applied to AM62x SIP as well.
> 
> v1:
> https://lore.kernel.org/r/20240708052457.1974547-1-s-vadapalli@ti.com/
> Changes since v1:
> - As pointed out by Vignesh Raghavendra <vigneshr@ti.com> at:
>    https://lore.kernel.org/r/829dc5b5-e87c-4679-9e2d-7417c744a9c9@ti.com/
>    the A53 defconfigs already include the "am62x_a53_usbdfu.config" config
>    fragment. Hence, they are not required in the am62*-evm.conf files and
>    have been dropped in this patch.
> - Commit message has been updated accordingly.
> 
> Regards,
> Siddharth.
> 
>   meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf   | 1 +
>   meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf   | 1 +
>   meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf    | 1 +
>   meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf | 1 +
>   meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf | 1 +
>   5 files changed, 5 insertions(+)
> 
> diff --git a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
> index 2af3317e..5ba8c27e 100644
> --- a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
> +++ b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
> @@ -9,3 +9,4 @@ SYSFW_CONFIG = "evm"
>   SYSFW_SUFFIX = "hs-fs"
>   
>   UBOOT_MACHINE = "am62ax_evm_r5_defconfig"
> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"

This will not work with the new BSP system we have in meta-ti.  Since we 
are trying to have support for multiple kernel/uboot/graphics 
combinations we need to be aware of which of those other combinations 
that are present in the BSP file will or wont work with the setting we 
are trying to do.

In this case, I see am62x_r5_usbdfu.config in ti-u-boot-2024.04 and in 
upstream, but I do not see it in ti-u-boot-2023.04.  So the correct 
setting here would be:

UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = ""


> diff --git a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> index 36915381..9b68e626 100644
> --- a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> +++ b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>   SYSFW_SUFFIX = "hs-fs"
>   
>   UBOOT_MACHINE = "am62px_evm_r5_defconfig"
> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
For this one, there are already UBOOT_CONFIG_FRAGMENTS later in the file 
that you are not taking into account by just setting the variable.  This 
file is going to be a little more difficult to do correctly.

Since we already have a setting with an override, I think the best plan 
would be to rework the existing logic while adding your changes.  Add 
some intermediate variables to construct the final value.  Something 
like this:

BASE_FRAGMENTS = "am62x_r5_usbdfu.config"
BASE_FRAGMENTS:ti-bsp-6_1 = ""

# UBOOT_CONFIG_FRAGMENTS holds the list of u-boot config fragments which 
has to be build
# along with the base defconfig mentioned in UBOOT_MACHINE. Refer 
u-boot-mergeconfig.inc
# under meta-ti-bsp/recipes-bsp/u-boot/ for more details.
# For AM62P tisdk-display-cluster image, splash screen is handled by SBL.
# Hence, disable the A53 based splash screen using the 
am62x_evm_prune_splashscreen.config fragment present in ti-u-boot tree
PRUNE_SPLASHSCREEN_FRAGMENT = 
"${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1', 
'am62x_evm_prune_splashscreen.config', '', d)}"
PRUNE_SPLASHSCREEN_FRAGMENT :bsp-ti-6_1 = 
"${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1', 
'am62px_evm_prune_splashscreen.config', '', d)}"

UBOOT_CONFIG_FRAGMENTS = " $(BASE_FRAGMENTS) $(PRUNE_SPLASHSCREEN_FRAGMENT)"


> diff --git a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
> index 548369ca..85c48334 100644
> --- a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
> +++ b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>   SYSFW_SUFFIX = "hs-fs"
>   
>   UBOOT_MACHINE = "am62x_evm_r5_defconfig"
> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"

Same as the first comment.

> diff --git a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
> index 52b69a72..24f96858 100644
> --- a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
> +++ b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>   SYSFW_SUFFIX = "hs-fs"
>   
>   UBOOT_MACHINE = "am62x_lpsk_r5_defconfig"
> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"

Same as the first comment.

> diff --git a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
> index 55bc530b..b6045d89 100644
> --- a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
> +++ b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
> @@ -13,3 +13,4 @@ UBOOT_MACHINE = "am62xsip_evm_r5_defconfig"
>   UBOOT_MACHINE:bsp-ti-6_1 = "am62x_evm_r5_defconfig"
>   
>   UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"
> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"

For this one there is already a setting for bsp-ti-6_1 so this is 
technically ok.  But in general we should try and have the variables in 
"correct" order.  The setting of the default, followed by the overrides 
of the variable.  So:

UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"
Ryan Eatmon July 8, 2024, 2:08 p.m. UTC | #2
On 7/8/2024 9:04 AM, Ryan Eatmon via lists.yoctoproject.org wrote:
> 
> 

Also, is this something that beagleplay should also have in it's config, 
or will that board not support this?  The logic for supporting this will 
be a little more complicated because of the beagle uboot settings if so.
Siddharth Vadapalli July 9, 2024, 4:06 a.m. UTC | #3
On Mon, Jul 08, 2024 at 09:04:43AM -0500, Ryan Eatmon wrote:
> 
> 
> On 7/8/2024 7:36 AM, Siddharth Vadapalli wrote:

[...]

> > diff --git a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
> > index 2af3317e..5ba8c27e 100644
> > --- a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
> > +++ b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
> > @@ -9,3 +9,4 @@ SYSFW_CONFIG = "evm"
> >   SYSFW_SUFFIX = "hs-fs"
> >   UBOOT_MACHINE = "am62ax_evm_r5_defconfig"
> > +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> 
> This will not work with the new BSP system we have in meta-ti.  Since we are
> trying to have support for multiple kernel/uboot/graphics combinations we
> need to be aware of which of those other combinations that are present in
> the BSP file will or wont work with the setting we are trying to do.
> 
> In this case, I see am62x_r5_usbdfu.config in ti-u-boot-2024.04 and in
> upstream, but I do not see it in ti-u-boot-2023.04.  So the correct setting
> here would be:
> 
> UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = ""

So I have to set "bsp-ti-6_1" to the empty string to indicate that the
CONFIG_FRAGMENT is not applicable to it. I wasn't aware of this. Thank you
for reviewing the patch and pointing this out.

> 
> 
> > diff --git a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> > index 36915381..9b68e626 100644
> > --- a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> > +++ b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> > @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
> >   SYSFW_SUFFIX = "hs-fs"
> >   UBOOT_MACHINE = "am62px_evm_r5_defconfig"
> > +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> For this one, there are already UBOOT_CONFIG_FRAGMENTS later in the file
> that you are not taking into account by just setting the variable.  This
> file is going to be a little more difficult to do correctly.
> 
> Since we already have a setting with an override, I think the best plan
> would be to rework the existing logic while adding your changes.  Add some
> intermediate variables to construct the final value.  Something like this:
> 
> BASE_FRAGMENTS = "am62x_r5_usbdfu.config"
> BASE_FRAGMENTS:ti-bsp-6_1 = ""
> 
> # UBOOT_CONFIG_FRAGMENTS holds the list of u-boot config fragments which has
> to be build
> # along with the base defconfig mentioned in UBOOT_MACHINE. Refer
> u-boot-mergeconfig.inc
> # under meta-ti-bsp/recipes-bsp/u-boot/ for more details.
> # For AM62P tisdk-display-cluster image, splash screen is handled by SBL.
> # Hence, disable the A53 based splash screen using the
> am62x_evm_prune_splashscreen.config fragment present in ti-u-boot tree
> PRUNE_SPLASHSCREEN_FRAGMENT =
> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
> 'am62x_evm_prune_splashscreen.config', '', d)}"
> PRUNE_SPLASHSCREEN_FRAGMENT :bsp-ti-6_1 =
> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
> 'am62px_evm_prune_splashscreen.config', '', d)}"
> 
> UBOOT_CONFIG_FRAGMENTS = " $(BASE_FRAGMENTS) $(PRUNE_SPLASHSCREEN_FRAGMENT)"

Since USB DFU is independent of Splash Screen, I agree that the above
approach you have suggested is the right way to implement this. I will
implement your suggestion in the v4 patch.

> 
> 
> > diff --git a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
> > index 548369ca..85c48334 100644
> > --- a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
> > +++ b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
> > @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
> >   SYSFW_SUFFIX = "hs-fs"
> >   UBOOT_MACHINE = "am62x_evm_r5_defconfig"
> > +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> 
> Same as the first comment.

I will fix this.

> 
> > diff --git a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
> > index 52b69a72..24f96858 100644
> > --- a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
> > +++ b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
> > @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
> >   SYSFW_SUFFIX = "hs-fs"
> >   UBOOT_MACHINE = "am62x_lpsk_r5_defconfig"
> > +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> 
> Same as the first comment.

Will fix this too.

> 
> > diff --git a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
> > index 55bc530b..b6045d89 100644
> > --- a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
> > +++ b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
> > @@ -13,3 +13,4 @@ UBOOT_MACHINE = "am62xsip_evm_r5_defconfig"
> >   UBOOT_MACHINE:bsp-ti-6_1 = "am62x_evm_r5_defconfig"
> >   UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"
> > +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> 
> For this one there is already a setting for bsp-ti-6_1 so this is
> technically ok.  But in general we should try and have the variables in
> "correct" order.  The setting of the default, followed by the overrides of
> the variable.  So:
> 
> UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"

I understand. I will fix the ordering. Thank you for reviewing the patch
and providing your valuable feedback.

Regards,
Siddharth.
Chirag Shilwant July 9, 2024, 4:25 a.m. UTC | #4
Hi Siddharth,
On 09/07/24 09:36, Siddharth Vadapalli wrote:
> On Mon, Jul 08, 2024 at 09:04:43AM -0500, Ryan Eatmon wrote:
>>
>> On 7/8/2024 7:36 AM, Siddharth Vadapalli wrote:
> [...]
>
>>> diff --git a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
>>> index 2af3317e..5ba8c27e 100644
>>> --- a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
>>> +++ b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
>>> @@ -9,3 +9,4 @@ SYSFW_CONFIG = "evm"
>>>    SYSFW_SUFFIX = "hs-fs"
>>>    UBOOT_MACHINE = "am62ax_evm_r5_defconfig"
>>> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> This will not work with the new BSP system we have in meta-ti.  Since we are
>> trying to have support for multiple kernel/uboot/graphics combinations we
>> need to be aware of which of those other combinations that are present in
>> the BSP file will or wont work with the setting we are trying to do.
>>
>> In this case, I see am62x_r5_usbdfu.config in ti-u-boot-2024.04 and in
>> upstream, but I do not see it in ti-u-boot-2023.04.  So the correct setting
>> here would be:
>>
>> UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = ""
> So I have to set "bsp-ti-6_1" to the empty string to indicate that the
> CONFIG_FRAGMENT is not applicable to it. I wasn't aware of this. Thank you
> for reviewing the patch and pointing this out.
>
>>
>>> diff --git a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
>>> index 36915381..9b68e626 100644
>>> --- a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
>>> +++ b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
>>> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>>>    SYSFW_SUFFIX = "hs-fs"
>>>    UBOOT_MACHINE = "am62px_evm_r5_defconfig"
>>> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> For this one, there are already UBOOT_CONFIG_FRAGMENTS later in the file
>> that you are not taking into account by just setting the variable.  This
>> file is going to be a little more difficult to do correctly.
>>
>> Since we already have a setting with an override, I think the best plan
>> would be to rework the existing logic while adding your changes.  Add some
>> intermediate variables to construct the final value.  Something like this:
>>
>> BASE_FRAGMENTS = "am62x_r5_usbdfu.config"
>> BASE_FRAGMENTS:ti-bsp-6_1 = ""
>>
>> # UBOOT_CONFIG_FRAGMENTS holds the list of u-boot config fragments which has
>> to be build
>> # along with the base defconfig mentioned in UBOOT_MACHINE. Refer
>> u-boot-mergeconfig.inc
>> # under meta-ti-bsp/recipes-bsp/u-boot/ for more details.
>> # For AM62P tisdk-display-cluster image, splash screen is handled by SBL.
>> # Hence, disable the A53 based splash screen using the
>> am62x_evm_prune_splashscreen.config fragment present in ti-u-boot tree
>> PRUNE_SPLASHSCREEN_FRAGMENT =
>> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
>> 'am62x_evm_prune_splashscreen.config', '', d)}"
>> PRUNE_SPLASHSCREEN_FRAGMENT :bsp-ti-6_1 =
>> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
>> 'am62px_evm_prune_splashscreen.config', '', d)}"
>>
>> UBOOT_CONFIG_FRAGMENTS = " $(BASE_FRAGMENTS) $(PRUNE_SPLASHSCREEN_FRAGMENT)"
> Since USB DFU is independent of Splash Screen, I agree that the above
> approach you have suggested is the right way to implement this. I will
> implement your suggestion in the v4 patch.




Agreed with Ryan. This approach looks much cleaner. I would encourage you
to build test the case where both am62x_evm_prune_splashscreen.config & 
am62x_r5_usbdfu.config
are passed to u-boot just to avoid last moment hiccups during final code 
freeze stage :)

Thanks,
Chirag




>
>>
>>> diff --git a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
>>> index 548369ca..85c48334 100644
>>> --- a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
>>> +++ b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
>>> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>>>    SYSFW_SUFFIX = "hs-fs"
>>>    UBOOT_MACHINE = "am62x_evm_r5_defconfig"
>>> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> Same as the first comment.
> I will fix this.
>
>>> diff --git a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
>>> index 52b69a72..24f96858 100644
>>> --- a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
>>> +++ b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
>>> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>>>    SYSFW_SUFFIX = "hs-fs"
>>>    UBOOT_MACHINE = "am62x_lpsk_r5_defconfig"
>>> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> Same as the first comment.
> Will fix this too.
>
>>> diff --git a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
>>> index 55bc530b..b6045d89 100644
>>> --- a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
>>> +++ b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
>>> @@ -13,3 +13,4 @@ UBOOT_MACHINE = "am62xsip_evm_r5_defconfig"
>>>    UBOOT_MACHINE:bsp-ti-6_1 = "am62x_evm_r5_defconfig"
>>>    UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"
>>> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> For this one there is already a setting for bsp-ti-6_1 so this is
>> technically ok.  But in general we should try and have the variables in
>> "correct" order.  The setting of the default, followed by the overrides of
>> the variable.  So:
>>
>> UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"
> I understand. I will fix the ordering. Thank you for reviewing the patch
> and providing your valuable feedback.
>
> Regards,
> Siddharth.
Siddharth Vadapalli July 9, 2024, 5:31 a.m. UTC | #5
On Tue, Jul 09, 2024 at 09:55:41AM +0530, Chirag Shilwant wrote:
> Hi Siddharth,
> On 09/07/24 09:36, Siddharth Vadapalli wrote:

[...]

> > Since USB DFU is independent of Splash Screen, I agree that the above
> > approach you have suggested is the right way to implement this. I will
> > implement your suggestion in the v4 patch.
> 
> 
> 
> 
> Agreed with Ryan. This approach looks much cleaner. I would encourage you
> to build test the case where both am62x_evm_prune_splashscreen.config &
> am62x_r5_usbdfu.config
> are passed to u-boot just to avoid last moment hiccups during final code
> freeze stage :)
> 

I have built and tested the combination:
am62px_evm_r5_defconfig + am62x_r5_usbdfu.config +
am62x_evm_prune_splashscreen.config

I did not face any build errors and have also validated USB DFU
functionatlity. Logs:
https://gist.github.com/Siddharth-Vadapalli-at-TI/38331822c5f8ed0887225ecab51f3395

Regards,
Siddharth.
Chirag Shilwant July 9, 2024, 7:18 a.m. UTC | #6
Hi Siddharth,
On 09/07/24 11:01, Siddharth Vadapalli wrote:
> On Tue, Jul 09, 2024 at 09:55:41AM +0530, Chirag Shilwant wrote:
>> Hi Siddharth,
>> On 09/07/24 09:36, Siddharth Vadapalli wrote:
> [...]
>
>>> Since USB DFU is independent of Splash Screen, I agree that the above
>>> approach you have suggested is the right way to implement this. I will
>>> implement your suggestion in the v4 patch.
>>
>>
>>
>> Agreed with Ryan. This approach looks much cleaner. I would encourage you
>> to build test the case where both am62x_evm_prune_splashscreen.config &
>> am62x_r5_usbdfu.config
>> are passed to u-boot just to avoid last moment hiccups during final code
>> freeze stage :)
>>
> I have built and tested the combination:
> am62px_evm_r5_defconfig + am62x_r5_usbdfu.config +
> am62x_evm_prune_splashscreen.config
>
> I did not face any build errors and have also validated USB DFU
> functionatlity. Logs:
> https://gist.github.com/Siddharth-Vadapalli-at-TI/38331822c5f8ed0887225ecab51f3395

Thanks Siddharth for confirmation.
With Ryan's comments getting addressed in v4, you have my

Acked-by: Chirag Shilwant <c-shilwant@ti.com>

>
> Regards,
> Siddharth.
Siddharth Vadapalli July 9, 2024, 7:58 a.m. UTC | #7
On Mon, Jul 08, 2024 at 09:04:43AM -0500, Ryan Eatmon wrote:

[...]

> > diff --git a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> > index 36915381..9b68e626 100644
> > --- a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> > +++ b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> > @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
> >   SYSFW_SUFFIX = "hs-fs"
> >   UBOOT_MACHINE = "am62px_evm_r5_defconfig"
> > +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> For this one, there are already UBOOT_CONFIG_FRAGMENTS later in the file
> that you are not taking into account by just setting the variable.  This
> file is going to be a little more difficult to do correctly.
> 
> Since we already have a setting with an override, I think the best plan
> would be to rework the existing logic while adding your changes.  Add some
> intermediate variables to construct the final value.  Something like this:
> 
> BASE_FRAGMENTS = "am62x_r5_usbdfu.config"
> BASE_FRAGMENTS:ti-bsp-6_1 = ""
> 
> # UBOOT_CONFIG_FRAGMENTS holds the list of u-boot config fragments which has
> to be build
> # along with the base defconfig mentioned in UBOOT_MACHINE. Refer
> u-boot-mergeconfig.inc
> # under meta-ti-bsp/recipes-bsp/u-boot/ for more details.
> # For AM62P tisdk-display-cluster image, splash screen is handled by SBL.
> # Hence, disable the A53 based splash screen using the
> am62x_evm_prune_splashscreen.config fragment present in ti-u-boot tree
> PRUNE_SPLASHSCREEN_FRAGMENT =
> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
> 'am62x_evm_prune_splashscreen.config', '', d)}"
> PRUNE_SPLASHSCREEN_FRAGMENT :bsp-ti-6_1 =
> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
> 'am62px_evm_prune_splashscreen.config', '', d)}"
> 
> UBOOT_CONFIG_FRAGMENTS = " $(BASE_FRAGMENTS) $(PRUNE_SPLASHSCREEN_FRAGMENT)"

I failed to notice it in my earlier response. The above file that you
are referring to is different from the one that this patch modifies.
This patch is updating:
meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
while the file that you are referring to above is:
meta-ti-bsp/conf/machine/am62pxx-evm.conf

So
UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = ""
is applicable to this as well, similar to other files.
I will implement the above change uniformly across all files in the v4
patch.

[...]

Regards,
Siddharth.
Ryan Eatmon July 9, 2024, 1:30 p.m. UTC | #8
On 7/9/2024 2:58 AM, Siddharth Vadapalli wrote:
> On Mon, Jul 08, 2024 at 09:04:43AM -0500, Ryan Eatmon wrote:
> 
> [...]
> 
>>> diff --git a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
>>> index 36915381..9b68e626 100644
>>> --- a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
>>> +++ b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
>>> @@ -10,3 +10,4 @@ SYSFW_CONFIG = "evm"
>>>    SYSFW_SUFFIX = "hs-fs"
>>>    UBOOT_MACHINE = "am62px_evm_r5_defconfig"
>>> +UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
>> For this one, there are already UBOOT_CONFIG_FRAGMENTS later in the file
>> that you are not taking into account by just setting the variable.  This
>> file is going to be a little more difficult to do correctly.
>>
>> Since we already have a setting with an override, I think the best plan
>> would be to rework the existing logic while adding your changes.  Add some
>> intermediate variables to construct the final value.  Something like this:
>>
>> BASE_FRAGMENTS = "am62x_r5_usbdfu.config"
>> BASE_FRAGMENTS:ti-bsp-6_1 = ""
>>
>> # UBOOT_CONFIG_FRAGMENTS holds the list of u-boot config fragments which has
>> to be build
>> # along with the base defconfig mentioned in UBOOT_MACHINE. Refer
>> u-boot-mergeconfig.inc
>> # under meta-ti-bsp/recipes-bsp/u-boot/ for more details.
>> # For AM62P tisdk-display-cluster image, splash screen is handled by SBL.
>> # Hence, disable the A53 based splash screen using the
>> am62x_evm_prune_splashscreen.config fragment present in ti-u-boot tree
>> PRUNE_SPLASHSCREEN_FRAGMENT =
>> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
>> 'am62x_evm_prune_splashscreen.config', '', d)}"
>> PRUNE_SPLASHSCREEN_FRAGMENT :bsp-ti-6_1 =
>> "${@oe.utils.conditional('DISPLAY_CLUSTER_ENABLE', '1',
>> 'am62px_evm_prune_splashscreen.config', '', d)}"
>>
>> UBOOT_CONFIG_FRAGMENTS = " $(BASE_FRAGMENTS) $(PRUNE_SPLASHSCREEN_FRAGMENT)"
> 
> I failed to notice it in my earlier response. The above file that you
> are referring to is different from the one that this patch modifies.
> This patch is updating:
> meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
> while the file that you are referring to above is:
> meta-ti-bsp/conf/machine/am62pxx-evm.conf
> 
> So
> UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
> UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = ""
> is applicable to this as well, similar to other files.
> I will implement the above change uniformly across all files in the v4
> patch.

Good point.  I looked in the wrong file when I was looking in the files.


> [...]
> 
> Regards,
> Siddharth.
diff mbox series

Patch

diff --git a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
index 2af3317e..5ba8c27e 100644
--- a/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
+++ b/meta-ti-bsp/conf/machine/am62axx-evm-k3r5.conf
@@ -9,3 +9,4 @@  SYSFW_CONFIG = "evm"
 SYSFW_SUFFIX = "hs-fs"
 
 UBOOT_MACHINE = "am62ax_evm_r5_defconfig"
+UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
diff --git a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
index 36915381..9b68e626 100644
--- a/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
+++ b/meta-ti-bsp/conf/machine/am62pxx-evm-k3r5.conf
@@ -10,3 +10,4 @@  SYSFW_CONFIG = "evm"
 SYSFW_SUFFIX = "hs-fs"
 
 UBOOT_MACHINE = "am62px_evm_r5_defconfig"
+UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
diff --git a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
index 548369ca..85c48334 100644
--- a/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
+++ b/meta-ti-bsp/conf/machine/am62xx-evm-k3r5.conf
@@ -10,3 +10,4 @@  SYSFW_CONFIG = "evm"
 SYSFW_SUFFIX = "hs-fs"
 
 UBOOT_MACHINE = "am62x_evm_r5_defconfig"
+UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
diff --git a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
index 52b69a72..24f96858 100644
--- a/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
+++ b/meta-ti-bsp/conf/machine/am62xx-lp-evm-k3r5.conf
@@ -10,3 +10,4 @@  SYSFW_CONFIG = "evm"
 SYSFW_SUFFIX = "hs-fs"
 
 UBOOT_MACHINE = "am62x_lpsk_r5_defconfig"
+UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"
diff --git a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
index 55bc530b..b6045d89 100644
--- a/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
+++ b/meta-ti-bsp/conf/machine/am62xxsip-evm-k3r5.conf
@@ -13,3 +13,4 @@  UBOOT_MACHINE = "am62xsip_evm_r5_defconfig"
 UBOOT_MACHINE:bsp-ti-6_1 = "am62x_evm_r5_defconfig"
 
 UBOOT_CONFIG_FRAGMENTS:bsp-ti-6_1 = "am62xsip_sk_r5.config"
+UBOOT_CONFIG_FRAGMENTS = "am62x_r5_usbdfu.config"