[meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Chirag Shilwant]

- Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config 
to be used.

- Include u-boot-mergeconfig.inc in u-boot-ti.inc

Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
---
 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
 meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
 2 files changed, 8 insertions(+)
 create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc

diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
new file mode 100644
index 00000000..69db6260
--- /dev/null
+++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
@@ -0,0 +1,7 @@
+do_compile:prepend () {
+   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
+   then
+       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
+       oe_runmake -C ${S} O=${B} olddefconfig
+   fi
+}
diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
index f3285c23..5292517b 100644
--- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
+++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
@@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
 
 require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
 require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
+require u-boot-mergeconfig.inc
 
 FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
 
-- 
2.34.1

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Denys Dmytriyenko]

Sorry for replying late.

I have some comments and few concerns about these patches - please give me a 
day to get back with more details. Thanks.

-- 
Denys


On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
> - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
> using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config 
> to be used.
> 
> - Include u-boot-mergeconfig.inc in u-boot-ti.inc
> 
> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> ---
>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
>  2 files changed, 8 insertions(+)
>  create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> 
> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> new file mode 100644
> index 00000000..69db6260
> --- /dev/null
> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> @@ -0,0 +1,7 @@
> +do_compile:prepend () {
> +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
> +   then
> +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
> +       oe_runmake -C ${S} O=${B} olddefconfig
> +   fi
> +}
> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> index f3285c23..5292517b 100644
> --- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> @@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
>  
>  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
>  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
> +require u-boot-mergeconfig.inc
>  
>  FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
>  
> -- 
> 2.34.1

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Denys Dmytriyenko]

On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
> - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
> using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config 
> to be used.

Would be nice to provide extra details here in the commit message about config 
fragment support in U-boot and its recipe. E.g.:

* U-boot recipe in OE-Core supports out-of-tree config fragments that are 
passed via SRC_URI and automatically merges all *.cfg files as fragments. 
This makes specifying config fragments in the machine configuration a bit 
difficult.

* U-boot itself supports in-tree config fragments and recently been adding 
fragments with *.config extension (first in configs/ dir, but will be moving 
to the corresponding board/ dir), so adding a way to specify and pass those.


> - Include u-boot-mergeconfig.inc in u-boot-ti.inc
> 
> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> ---
>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
>  2 files changed, 8 insertions(+)
>  create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> 
> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> new file mode 100644
> index 00000000..69db6260
> --- /dev/null
> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> @@ -0,0 +1,7 @@
> +do_compile:prepend () {

Should be done in do_configure instead. Tasks should be self-contained and 
granular. Plus, do_compile can be repeated w/o do_configure. If you are 
re-configuring every time and generate a new .config file in do_compile, 
that would probably trigger full re-compile due to make tracking file 
timestamps...


> +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]

Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS 
plural?


> +   then
> +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
> +       oe_runmake -C ${S} O=${B} olddefconfig
> +   fi

So, this basically repeats configuration done in u-boot-configure.inc in 
OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs. 
While I realize you just want to address a single use-case, making it a bit 
future-proof shouldn't be overlooked.

Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same 
time. Probably completely rewriting do_configure from u-boot-configure.inc 
would be needed...

BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config 
fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG 
takes a list of defconfigs and iterates through them building each separately.


> +}
> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> index f3285c23..5292517b 100644
> --- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> @@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
>  
>  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
>  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
> +require u-boot-mergeconfig.inc
>  
>  FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
>  
> -- 
> 2.34.1

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Denys Dmytriyenko]

On Tue, Sep 12, 2023 at 04:04:03PM -0400, Denys Dmytriyenko wrote:
> On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
> > - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
> > using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config 
> > to be used.
> 
> Would be nice to provide extra details here in the commit message about config 
> fragment support in U-boot and its recipe. E.g.:
> 
> * U-boot recipe in OE-Core supports out-of-tree config fragments that are 
> passed via SRC_URI and automatically merges all *.cfg files as fragments. 
> This makes specifying config fragments in the machine configuration a bit 
> difficult.
> 
> * U-boot itself supports in-tree config fragments and recently been adding 
> fragments with *.config extension (first in configs/ dir, but will be moving 
> to the corresponding board/ dir), so adding a way to specify and pass those.
> 
> 
> > - Include u-boot-mergeconfig.inc in u-boot-ti.inc
> > 
> > Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> > ---
> >  meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
> >  meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
> >  2 files changed, 8 insertions(+)
> >  create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> > 
> > diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> > new file mode 100644
> > index 00000000..69db6260
> > --- /dev/null
> > +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> > @@ -0,0 +1,7 @@
> > +do_compile:prepend () {
> 
> Should be done in do_configure instead. Tasks should be self-contained and 
> granular. Plus, do_compile can be repeated w/o do_configure. If you are 
> re-configuring every time and generate a new .config file in do_compile, 
> that would probably trigger full re-compile due to make tracking file 
> timestamps...
> 
> 
> > +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
> 
> Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS 
> plural?
> 
> 
> > +   then
> > +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
> > +       oe_runmake -C ${S} O=${B} olddefconfig
> > +   fi
> 
> So, this basically repeats configuration done in u-boot-configure.inc in 
> OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs. 
> While I realize you just want to address a single use-case, making it a bit 
> future-proof shouldn't be overlooked.
> 
> Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same 
> time. Probably completely rewriting do_configure from u-boot-configure.inc 
> would be needed...
> 
> BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config 
> fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG 
> takes a list of defconfigs and iterates through them building each separately.

Ah, forget another use-case. So, besides combining UBOOT_CONFIG_FRAGMENTS with 
UBOOT_CONFIG, what about mixing in-tree and out-of-tree fragments? :) I know 
the matrix of possible options is getting quite complex, that's why we have a 
separate setup-defconfig.inc for the kernel in meta-ti to cover all the bases.


> > +}
> > diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> > index f3285c23..5292517b 100644
> > --- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> > +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> > @@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
> >  
> >  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
> >  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
> > +require u-boot-mergeconfig.inc
> >  
> >  FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
> >  
> > -- 
> > 2.34.1

Re: [EXTERNAL] Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Chirag Shilwant]

On 13/09/23 01:34, Denys Dmytriyenko wrote:
> On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
>> - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
>> using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
>> to be used.
> Would be nice to provide extra details here in the commit message about config
> fragment support in U-boot and its recipe. E.g.:
>
> * U-boot recipe in OE-Core supports out-of-tree config fragments that are
> passed via SRC_URI and automatically merges all *.cfg files as fragments.
> This makes specifying config fragments in the machine configuration a bit
> difficult.
>
> * U-boot itself supports in-tree config fragments and recently been adding
> fragments with *.config extension (first in configs/ dir, but will be moving
> to the corresponding board/ dir), so adding a way to specify and pass those.

Sure, will update the commit message & provide extra details in my v2 
PATCH.

>
>> - Include u-boot-mergeconfig.inc in u-boot-ti.inc
>>
>> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
>> ---
>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
>>   2 files changed, 8 insertions(+)
>>   create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>
>> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>> new file mode 100644
>> index 00000000..69db6260
>> --- /dev/null
>> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>> @@ -0,0 +1,7 @@
>> +do_compile:prepend () {
> Should be done in do_configure instead. Tasks should be self-contained and
> granular. Plus, do_compile can be repeated w/o do_configure. If you are
> re-configuring every time and generate a new .config file in do_compile,
> that would probably trigger full re-compile due to make tracking file
> timestamps...
>
Yes, we can update this to happen in do_configure. I believe 
do_configure:append should be good & will work. Will handle this in v2.

>> +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
> Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
> plural?
>
>
>> +   then
>> +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
>> +       oe_runmake -C ${S} O=${B} olddefconfig
>> +   fi
> So, this basically repeats configuration done in u-boot-configure.inc in
> OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
> While I realize you just want to address a single use-case, making it a bit
> future-proof shouldn't be overlooked.
>
> Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
> time. Probably completely rewriting do_configure from u-boot-configure.inc
> would be needed...
>
> BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
> fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
> takes a list of defconfigs and iterates through them building each separately.
>
Thanks for your suggestions. I appreciate your idea to cover all use 
cases with u-boot-mergeconfig.inc, but I have few concerns due to the 
current release situation we are into.

As far as I know, meta-ti does not have a use case for UBOOT_CONFIG 
itself. We are using UBOOT_MACHINE for handling the base defconfig, 
which works well for our needs. The concept of having u-boot fragments 
was introduced recently in ti-u-boot after we got a feedback from 
upstream uboot as around 90% of configs were same between am62xx-evm & 
am62xxsip-evm. This was a valid input and hence we considered in 
ti-u-boot. In order to support that I had to find a quick way to handle 
this in Yocto it with UBOOT_MACHINE along with UBOOT_CONFIG_FRAGMENT 
logic, which I had also discussed with you on the syncup call.
Considering that the am62xxsip-evm release is only two weeks away, can 
we consider the fragments approach for now and avoid any potential risks 
or delays. I will work with you in coming weeks to make this better by 
handling all the cases of uboot-config and fragments.

Please let me know if I you agree with my views and if I can proceed 
with fixing the other issues you highlighted, they are trivial I can 
submit a patch tomorrow.

>> +}
>> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
>> index f3285c23..5292517b 100644
>> --- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
>> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
>> @@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
>>   
>>   require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
>>   require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
>> +require u-boot-mergeconfig.inc
>>   
>>   FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
>>   
>> -- 
>> 2.34.1

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Denys Dmytriyenko]

On Wed, Sep 13, 2023 at 10:12:57PM +0530, Chirag Shilwant via lists.yoctoproject.org wrote:
> On 13/09/23 01:34, Denys Dmytriyenko wrote:
> >On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
> >>- Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
> >>using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
> >>to be used.
> >Would be nice to provide extra details here in the commit message about config
> >fragment support in U-boot and its recipe. E.g.:
> >
> >* U-boot recipe in OE-Core supports out-of-tree config fragments that are
> >passed via SRC_URI and automatically merges all *.cfg files as fragments.
> >This makes specifying config fragments in the machine configuration a bit
> >difficult.
> >
> >* U-boot itself supports in-tree config fragments and recently been adding
> >fragments with *.config extension (first in configs/ dir, but will be moving
> >to the corresponding board/ dir), so adding a way to specify and pass those.
> 
> Sure, will update the commit message & provide extra details in my
> v2 PATCH.
> 
> >
> >>- Include u-boot-mergeconfig.inc in u-boot-ti.inc
> >>
> >>Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> >>---
> >>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
> >>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
> >>  2 files changed, 8 insertions(+)
> >>  create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>
> >>diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>new file mode 100644
> >>index 00000000..69db6260
> >>--- /dev/null
> >>+++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>@@ -0,0 +1,7 @@
> >>+do_compile:prepend () {
> >Should be done in do_configure instead. Tasks should be self-contained and
> >granular. Plus, do_compile can be repeated w/o do_configure. If you are
> >re-configuring every time and generate a new .config file in do_compile,
> >that would probably trigger full re-compile due to make tracking file
> >timestamps...
> >
> Yes, we can update this to happen in do_configure. I believe
> do_configure:append should be good & will work. Will handle this in
> v2.
> 
> >>+   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
> >Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
> >plural?
> >
> >
> >>+   then
> >>+       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
> >>+       oe_runmake -C ${S} O=${B} olddefconfig
> >>+   fi
> >So, this basically repeats configuration done in u-boot-configure.inc in
> >OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
> >While I realize you just want to address a single use-case, making it a bit
> >future-proof shouldn't be overlooked.
> >
> >Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
> >time. Probably completely rewriting do_configure from u-boot-configure.inc
> >would be needed...
> >
> >BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
> >fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
> >takes a list of defconfigs and iterates through them building each separately.
> >
> Thanks for your suggestions. I appreciate your idea to cover all use
> cases with u-boot-mergeconfig.inc, but I have few concerns due to
> the current release situation we are into.
> 
> As far as I know, meta-ti does not have a use case for UBOOT_CONFIG
> itself.

It does:
https://git.yoctoproject.org/meta-ti/tree/meta-ti-bsp/conf/machine/am335x-hs-evm.conf#n7

Which means, since your new code is added to all TI platforms, it could 
potentially break AM335x HS platform.


> We are using UBOOT_MACHINE for handling the base defconfig,
> which works well for our needs. The concept of having u-boot
> fragments was introduced recently in ti-u-boot after we got a
> feedback from upstream uboot as around 90% of configs were same
> between am62xx-evm & am62xxsip-evm. This was a valid input and hence
> we considered in ti-u-boot. In order to support that I had to find a
> quick way to handle this in Yocto it with UBOOT_MACHINE along with
> UBOOT_CONFIG_FRAGMENT logic, which I had also discussed with you on
> the syncup call.

Actually, the support was pretty much always there in U-boot, as kconfig 
framework along with support for fragments by calling merge_config.sh was 
borrowed from the kernel long ago and had been periodically synced with 
latest. Though U-boot didn't have own in-tree config fragments until very 
recently. First few got into the main config/ directory, but going forward 
those will reside in the corresponding board/ directories...


> Considering that the am62xxsip-evm release is only two weeks away,
> can we consider the fragments approach for now and avoid any
> potential risks or delays. I will work with you in coming weeks to
> make this better by handling all the cases of uboot-config and
> fragments.

Sure, this can be handled in stages. A bit more checks would be needed then 
to avoid breaking at least UBOOT_CONFIG use case.


> Please let me know if I you agree with my views and if I can proceed
> with fixing the other issues you highlighted, they are trivial I can
> submit a patch tomorrow.


> >>+}
> >>diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> >>index f3285c23..5292517b 100644
> >>--- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> >>+++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
> >>@@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
> >>  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
> >>  require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
> >>+require u-boot-mergeconfig.inc
> >>  FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
> >>-- 
> >>2.34.1

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Ryan Eatmon]

On 9/14/2023 12:08 PM, Denys Dmytriyenko wrote:
> On Wed, Sep 13, 2023 at 10:12:57PM +0530, Chirag Shilwant via lists.yoctoproject.org wrote:
>> On 13/09/23 01:34, Denys Dmytriyenko wrote:
>>> On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
>>>> - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
>>>> using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
>>>> to be used.
>>> Would be nice to provide extra details here in the commit message about config
>>> fragment support in U-boot and its recipe. E.g.:
>>>
>>> * U-boot recipe in OE-Core supports out-of-tree config fragments that are
>>> passed via SRC_URI and automatically merges all *.cfg files as fragments.
>>> This makes specifying config fragments in the machine configuration a bit
>>> difficult.
>>>
>>> * U-boot itself supports in-tree config fragments and recently been adding
>>> fragments with *.config extension (first in configs/ dir, but will be moving
>>> to the corresponding board/ dir), so adding a way to specify and pass those.
>>
>> Sure, will update the commit message & provide extra details in my
>> v2 PATCH.
>>
>>>
>>>> - Include u-boot-mergeconfig.inc in u-boot-ti.inc
>>>>
>>>> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
>>>> ---
>>>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
>>>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
>>>>   2 files changed, 8 insertions(+)
>>>>   create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>
>>>> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>> new file mode 100644
>>>> index 00000000..69db6260
>>>> --- /dev/null
>>>> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>> @@ -0,0 +1,7 @@
>>>> +do_compile:prepend () {
>>> Should be done in do_configure instead. Tasks should be self-contained and
>>> granular. Plus, do_compile can be repeated w/o do_configure. If you are
>>> re-configuring every time and generate a new .config file in do_compile,
>>> that would probably trigger full re-compile due to make tracking file
>>> timestamps...
>>>
>> Yes, we can update this to happen in do_configure. I believe
>> do_configure:append should be good & will work. Will handle this in
>> v2.
>>
>>>> +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
>>> Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
>>> plural?
>>>
>>>
>>>> +   then
>>>> +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
>>>> +       oe_runmake -C ${S} O=${B} olddefconfig
>>>> +   fi
>>> So, this basically repeats configuration done in u-boot-configure.inc in
>>> OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
>>> While I realize you just want to address a single use-case, making it a bit
>>> future-proof shouldn't be overlooked.
>>>
>>> Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
>>> time. Probably completely rewriting do_configure from u-boot-configure.inc
>>> would be needed...
>>>
>>> BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
>>> fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
>>> takes a list of defconfigs and iterates through them building each separately.
>>>
>> Thanks for your suggestions. I appreciate your idea to cover all use
>> cases with u-boot-mergeconfig.inc, but I have few concerns due to
>> the current release situation we are into.
>>
>> As far as I know, meta-ti does not have a use case for UBOOT_CONFIG
>> itself.
> 
> It does:
> https://git.yoctoproject.org/meta-ti/tree/meta-ti-bsp/conf/machine/am335x-hs-evm.conf#n7
> 
> Which means, since your new code is added to all TI platforms, it could
> potentially break AM335x HS platform.

The code is wrapped behind the setting of UBOOT_CONFIG_FRAGMENT, so 
unless we set both UBOOT_CONFIG_FRAGMENT and UBOOT_CONFIG in am335 it 
should be ok.


> 
>> We are using UBOOT_MACHINE for handling the base defconfig,
>> which works well for our needs. The concept of having u-boot
>> fragments was introduced recently in ti-u-boot after we got a
>> feedback from upstream uboot as around 90% of configs were same
>> between am62xx-evm & am62xxsip-evm. This was a valid input and hence
>> we considered in ti-u-boot. In order to support that I had to find a
>> quick way to handle this in Yocto it with UBOOT_MACHINE along with
>> UBOOT_CONFIG_FRAGMENT logic, which I had also discussed with you on
>> the syncup call.
> 
> Actually, the support was pretty much always there in U-boot, as kconfig
> framework along with support for fragments by calling merge_config.sh was
> borrowed from the kernel long ago and had been periodically synced with
> latest. Though U-boot didn't have own in-tree config fragments until very
> recently. First few got into the main config/ directory, but going forward
> those will reside in the corresponding board/ directories...
> 
> 
>> Considering that the am62xxsip-evm release is only two weeks away,
>> can we consider the fragments approach for now and avoid any
>> potential risks or delays. I will work with you in coming weeks to
>> make this better by handling all the cases of uboot-config and
>> fragments.
> 
> Sure, this can be handled in stages. A bit more checks would be needed then
> to avoid breaking at least UBOOT_CONFIG use case.
> 
> 
>> Please let me know if I you agree with my views and if I can proceed
>> with fixing the other issues you highlighted, they are trivial I can
>> submit a patch tomorrow.
> 
> 
>>>> +}
>>>> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
>>>> index f3285c23..5292517b 100644
>>>> --- a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
>>>> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc
>>>> @@ -7,6 +7,7 @@ SPL_BINARY ?= "MLO"
>>>>   require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot-common.inc
>>>>   require ${COREBASE}/meta/recipes-bsp/u-boot/u-boot.inc
>>>> +require u-boot-mergeconfig.inc
>>>>   FILESEXTRAPATHS:prepend := "${THISDIR}/u-boot:"
>>>> -- 
>>>> 2.34.1

-- 
Ryan Eatmon                reatmon@ti.com
-----------------------------------------
Texas Instruments, Inc.  -  LCPD  -  MGTS

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Denys Dmytriyenko]

On Thu, Sep 14, 2023 at 01:23:31PM -0500, Ryan Eatmon wrote:
> 
> 
> On 9/14/2023 12:08 PM, Denys Dmytriyenko wrote:
> >On Wed, Sep 13, 2023 at 10:12:57PM +0530, Chirag Shilwant via lists.yoctoproject.org wrote:
> >>On 13/09/23 01:34, Denys Dmytriyenko wrote:
> >>>On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
> >>>>- Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
> >>>>using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
> >>>>to be used.
> >>>Would be nice to provide extra details here in the commit message about config
> >>>fragment support in U-boot and its recipe. E.g.:
> >>>
> >>>* U-boot recipe in OE-Core supports out-of-tree config fragments that are
> >>>passed via SRC_URI and automatically merges all *.cfg files as fragments.
> >>>This makes specifying config fragments in the machine configuration a bit
> >>>difficult.
> >>>
> >>>* U-boot itself supports in-tree config fragments and recently been adding
> >>>fragments with *.config extension (first in configs/ dir, but will be moving
> >>>to the corresponding board/ dir), so adding a way to specify and pass those.
> >>
> >>Sure, will update the commit message & provide extra details in my
> >>v2 PATCH.
> >>
> >>>
> >>>>- Include u-boot-mergeconfig.inc in u-boot-ti.inc
> >>>>
> >>>>Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> >>>>---
> >>>>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
> >>>>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
> >>>>  2 files changed, 8 insertions(+)
> >>>>  create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>>>
> >>>>diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>>>new file mode 100644
> >>>>index 00000000..69db6260
> >>>>--- /dev/null
> >>>>+++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>>>@@ -0,0 +1,7 @@
> >>>>+do_compile:prepend () {
> >>>Should be done in do_configure instead. Tasks should be self-contained and
> >>>granular. Plus, do_compile can be repeated w/o do_configure. If you are
> >>>re-configuring every time and generate a new .config file in do_compile,
> >>>that would probably trigger full re-compile due to make tracking file
> >>>timestamps...
> >>>
> >>Yes, we can update this to happen in do_configure. I believe
> >>do_configure:append should be good & will work. Will handle this in
> >>v2.
> >>
> >>>>+   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
> >>>Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
> >>>plural?
> >>>
> >>>
> >>>>+   then
> >>>>+       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
> >>>>+       oe_runmake -C ${S} O=${B} olddefconfig
> >>>>+   fi
> >>>So, this basically repeats configuration done in u-boot-configure.inc in
> >>>OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
> >>>While I realize you just want to address a single use-case, making it a bit
> >>>future-proof shouldn't be overlooked.
> >>>
> >>>Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
> >>>time. Probably completely rewriting do_configure from u-boot-configure.inc
> >>>would be needed...
> >>>
> >>>BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
> >>>fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
> >>>takes a list of defconfigs and iterates through them building each separately.
> >>>
> >>Thanks for your suggestions. I appreciate your idea to cover all use
> >>cases with u-boot-mergeconfig.inc, but I have few concerns due to
> >>the current release situation we are into.
> >>
> >>As far as I know, meta-ti does not have a use case for UBOOT_CONFIG
> >>itself.
> >
> >It does:
> >https://git.yoctoproject.org/meta-ti/tree/meta-ti-bsp/conf/machine/am335x-hs-evm.conf#n7
> >
> >Which means, since your new code is added to all TI platforms, it could
> >potentially break AM335x HS platform.
> 
> The code is wrapped behind the setting of UBOOT_CONFIG_FRAGMENT, so
> unless we set both UBOOT_CONFIG_FRAGMENT and UBOOT_CONFIG in am335
> it should be ok.

Correct, that's why I phrased it as "could potentially break". Maybe not even 
specific to TI's AM335x HS, but to some downstream customer platform using 
meta-ti. As meta-ti is not the end product.


> >>We are using UBOOT_MACHINE for handling the base defconfig,
> >>which works well for our needs. The concept of having u-boot
> >>fragments was introduced recently in ti-u-boot after we got a
> >>feedback from upstream uboot as around 90% of configs were same
> >>between am62xx-evm & am62xxsip-evm. This was a valid input and hence
> >>we considered in ti-u-boot. In order to support that I had to find a
> >>quick way to handle this in Yocto it with UBOOT_MACHINE along with
> >>UBOOT_CONFIG_FRAGMENT logic, which I had also discussed with you on
> >>the syncup call.
> >
> >Actually, the support was pretty much always there in U-boot, as kconfig
> >framework along with support for fragments by calling merge_config.sh was
> >borrowed from the kernel long ago and had been periodically synced with
> >latest. Though U-boot didn't have own in-tree config fragments until very
> >recently. First few got into the main config/ directory, but going forward
> >those will reside in the corresponding board/ directories...
> >
> >
> >>Considering that the am62xxsip-evm release is only two weeks away,
> >>can we consider the fragments approach for now and avoid any
> >>potential risks or delays. I will work with you in coming weeks to
> >>make this better by handling all the cases of uboot-config and
> >>fragments.
> >
> >Sure, this can be handled in stages. A bit more checks would be needed then
> >to avoid breaking at least UBOOT_CONFIG use case.
> >
> >
> >>Please let me know if I you agree with my views and if I can proceed
> >>with fixing the other issues you highlighted, they are trivial I can
> >>submit a patch tomorrow.

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Ryan Eatmon]

On 9/14/2023 2:42 PM, Denys Dmytriyenko wrote:
> On Thu, Sep 14, 2023 at 01:23:31PM -0500, Ryan Eatmon wrote:
>>
>>
>> On 9/14/2023 12:08 PM, Denys Dmytriyenko wrote:
>>> On Wed, Sep 13, 2023 at 10:12:57PM +0530, Chirag Shilwant via lists.yoctoproject.org wrote:
>>>> On 13/09/23 01:34, Denys Dmytriyenko wrote:
>>>>> On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
>>>>>> - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
>>>>>> using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
>>>>>> to be used.
>>>>> Would be nice to provide extra details here in the commit message about config
>>>>> fragment support in U-boot and its recipe. E.g.:
>>>>>
>>>>> * U-boot recipe in OE-Core supports out-of-tree config fragments that are
>>>>> passed via SRC_URI and automatically merges all *.cfg files as fragments.
>>>>> This makes specifying config fragments in the machine configuration a bit
>>>>> difficult.
>>>>>
>>>>> * U-boot itself supports in-tree config fragments and recently been adding
>>>>> fragments with *.config extension (first in configs/ dir, but will be moving
>>>>> to the corresponding board/ dir), so adding a way to specify and pass those.
>>>>
>>>> Sure, will update the commit message & provide extra details in my
>>>> v2 PATCH.
>>>>
>>>>>
>>>>>> - Include u-boot-mergeconfig.inc in u-boot-ti.inc
>>>>>>
>>>>>> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
>>>>>> ---
>>>>>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
>>>>>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
>>>>>>   2 files changed, 8 insertions(+)
>>>>>>   create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>>>
>>>>>> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>>> new file mode 100644
>>>>>> index 00000000..69db6260
>>>>>> --- /dev/null
>>>>>> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>>> @@ -0,0 +1,7 @@
>>>>>> +do_compile:prepend () {
>>>>> Should be done in do_configure instead. Tasks should be self-contained and
>>>>> granular. Plus, do_compile can be repeated w/o do_configure. If you are
>>>>> re-configuring every time and generate a new .config file in do_compile,
>>>>> that would probably trigger full re-compile due to make tracking file
>>>>> timestamps...
>>>>>
>>>> Yes, we can update this to happen in do_configure. I believe
>>>> do_configure:append should be good & will work. Will handle this in
>>>> v2.
>>>>
>>>>>> +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
>>>>> Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
>>>>> plural?
>>>>>
>>>>>
>>>>>> +   then
>>>>>> +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
>>>>>> +       oe_runmake -C ${S} O=${B} olddefconfig
>>>>>> +   fi
>>>>> So, this basically repeats configuration done in u-boot-configure.inc in
>>>>> OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
>>>>> While I realize you just want to address a single use-case, making it a bit
>>>>> future-proof shouldn't be overlooked.
>>>>>
>>>>> Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
>>>>> time. Probably completely rewriting do_configure from u-boot-configure.inc
>>>>> would be needed...
>>>>>
>>>>> BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
>>>>> fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
>>>>> takes a list of defconfigs and iterates through them building each separately.
>>>>>
>>>> Thanks for your suggestions. I appreciate your idea to cover all use
>>>> cases with u-boot-mergeconfig.inc, but I have few concerns due to
>>>> the current release situation we are into.
>>>>
>>>> As far as I know, meta-ti does not have a use case for UBOOT_CONFIG
>>>> itself.
>>>
>>> It does:
>>> https://git.yoctoproject.org/meta-ti/tree/meta-ti-bsp/conf/machine/am335x-hs-evm.conf#n7
>>>
>>> Which means, since your new code is added to all TI platforms, it could
>>> potentially break AM335x HS platform.
>>
>> The code is wrapped behind the setting of UBOOT_CONFIG_FRAGMENT, so
>> unless we set both UBOOT_CONFIG_FRAGMENT and UBOOT_CONFIG in am335
>> it should be ok.
> 
> Correct, that's why I phrased it as "could potentially break". Maybe not even
> specific to TI's AM335x HS, but to some downstream customer platform using
> meta-ti. As meta-ti is not the end product.

Excellent point.  We really do need to do this the right way and not 
punt this too far off into the future to do the correct implementation.


> 
>>>> We are using UBOOT_MACHINE for handling the base defconfig,
>>>> which works well for our needs. The concept of having u-boot
>>>> fragments was introduced recently in ti-u-boot after we got a
>>>> feedback from upstream uboot as around 90% of configs were same
>>>> between am62xx-evm & am62xxsip-evm. This was a valid input and hence
>>>> we considered in ti-u-boot. In order to support that I had to find a
>>>> quick way to handle this in Yocto it with UBOOT_MACHINE along with
>>>> UBOOT_CONFIG_FRAGMENT logic, which I had also discussed with you on
>>>> the syncup call.
>>>
>>> Actually, the support was pretty much always there in U-boot, as kconfig
>>> framework along with support for fragments by calling merge_config.sh was
>>> borrowed from the kernel long ago and had been periodically synced with
>>> latest. Though U-boot didn't have own in-tree config fragments until very
>>> recently. First few got into the main config/ directory, but going forward
>>> those will reside in the corresponding board/ directories...
>>>
>>>
>>>> Considering that the am62xxsip-evm release is only two weeks away,
>>>> can we consider the fragments approach for now and avoid any
>>>> potential risks or delays. I will work with you in coming weeks to
>>>> make this better by handling all the cases of uboot-config and
>>>> fragments.
>>>
>>> Sure, this can be handled in stages. A bit more checks would be needed then
>>> to avoid breaking at least UBOOT_CONFIG use case.
>>>
>>>
>>>> Please let me know if I you agree with my views and if I can proceed
>>>> with fixing the other issues you highlighted, they are trivial I can
>>>> submit a patch tomorrow.

-- 
Ryan Eatmon                reatmon@ti.com
-----------------------------------------
Texas Instruments, Inc.  -  LCPD  -  MGTS

Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Denys Dmytriyenko]

On Thu, Sep 14, 2023 at 03:46:57PM -0500, Ryan Eatmon wrote:
> 
> 
> On 9/14/2023 2:42 PM, Denys Dmytriyenko wrote:
> >On Thu, Sep 14, 2023 at 01:23:31PM -0500, Ryan Eatmon wrote:
> >>
> >>
> >>On 9/14/2023 12:08 PM, Denys Dmytriyenko wrote:
> >>>On Wed, Sep 13, 2023 at 10:12:57PM +0530, Chirag Shilwant via lists.yoctoproject.org wrote:
> >>>>On 13/09/23 01:34, Denys Dmytriyenko wrote:
> >>>>>On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
> >>>>>>- Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
> >>>>>>using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
> >>>>>>to be used.
> >>>>>Would be nice to provide extra details here in the commit message about config
> >>>>>fragment support in U-boot and its recipe. E.g.:
> >>>>>
> >>>>>* U-boot recipe in OE-Core supports out-of-tree config fragments that are
> >>>>>passed via SRC_URI and automatically merges all *.cfg files as fragments.
> >>>>>This makes specifying config fragments in the machine configuration a bit
> >>>>>difficult.
> >>>>>
> >>>>>* U-boot itself supports in-tree config fragments and recently been adding
> >>>>>fragments with *.config extension (first in configs/ dir, but will be moving
> >>>>>to the corresponding board/ dir), so adding a way to specify and pass those.
> >>>>
> >>>>Sure, will update the commit message & provide extra details in my
> >>>>v2 PATCH.
> >>>>
> >>>>>
> >>>>>>- Include u-boot-mergeconfig.inc in u-boot-ti.inc
> >>>>>>
> >>>>>>Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
> >>>>>>---
> >>>>>>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
> >>>>>>  meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
> >>>>>>  2 files changed, 8 insertions(+)
> >>>>>>  create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>>>>>
> >>>>>>diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>>>>>new file mode 100644
> >>>>>>index 00000000..69db6260
> >>>>>>--- /dev/null
> >>>>>>+++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
> >>>>>>@@ -0,0 +1,7 @@
> >>>>>>+do_compile:prepend () {
> >>>>>Should be done in do_configure instead. Tasks should be self-contained and
> >>>>>granular. Plus, do_compile can be repeated w/o do_configure. If you are
> >>>>>re-configuring every time and generate a new .config file in do_compile,
> >>>>>that would probably trigger full re-compile due to make tracking file
> >>>>>timestamps...
> >>>>>
> >>>>Yes, we can update this to happen in do_configure. I believe
> >>>>do_configure:append should be good & will work. Will handle this in
> >>>>v2.
> >>>>
> >>>>>>+   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
> >>>>>Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
> >>>>>plural?
> >>>>>
> >>>>>
> >>>>>>+   then
> >>>>>>+       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
> >>>>>>+       oe_runmake -C ${S} O=${B} olddefconfig
> >>>>>>+   fi
> >>>>>So, this basically repeats configuration done in u-boot-configure.inc in
> >>>>>OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
> >>>>>While I realize you just want to address a single use-case, making it a bit
> >>>>>future-proof shouldn't be overlooked.
> >>>>>
> >>>>>Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
> >>>>>time. Probably completely rewriting do_configure from u-boot-configure.inc
> >>>>>would be needed...
> >>>>>
> >>>>>BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
> >>>>>fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
> >>>>>takes a list of defconfigs and iterates through them building each separately.
> >>>>>
> >>>>Thanks for your suggestions. I appreciate your idea to cover all use
> >>>>cases with u-boot-mergeconfig.inc, but I have few concerns due to
> >>>>the current release situation we are into.
> >>>>
> >>>>As far as I know, meta-ti does not have a use case for UBOOT_CONFIG
> >>>>itself.
> >>>
> >>>It does:
> >>>https://git.yoctoproject.org/meta-ti/tree/meta-ti-bsp/conf/machine/am335x-hs-evm.conf#n7
> >>>
> >>>Which means, since your new code is added to all TI platforms, it could
> >>>potentially break AM335x HS platform.
> >>
> >>The code is wrapped behind the setting of UBOOT_CONFIG_FRAGMENT, so
> >>unless we set both UBOOT_CONFIG_FRAGMENT and UBOOT_CONFIG in am335
> >>it should be ok.
> >
> >Correct, that's why I phrased it as "could potentially break". Maybe not even
> >specific to TI's AM335x HS, but to some downstream customer platform using
> >meta-ti. As meta-ti is not the end product.
> 
> Excellent point.  We really do need to do this the right way and not
> punt this too far off into the future to do the correct
> implementation.

At least doing an extra check for UBOOT_MACHINE being set, as I mentioned in 
another thread, would be a good first step.


> >>>>We are using UBOOT_MACHINE for handling the base defconfig,
> >>>>which works well for our needs. The concept of having u-boot
> >>>>fragments was introduced recently in ti-u-boot after we got a
> >>>>feedback from upstream uboot as around 90% of configs were same
> >>>>between am62xx-evm & am62xxsip-evm. This was a valid input and hence
> >>>>we considered in ti-u-boot. In order to support that I had to find a
> >>>>quick way to handle this in Yocto it with UBOOT_MACHINE along with
> >>>>UBOOT_CONFIG_FRAGMENT logic, which I had also discussed with you on
> >>>>the syncup call.
> >>>
> >>>Actually, the support was pretty much always there in U-boot, as kconfig
> >>>framework along with support for fragments by calling merge_config.sh was
> >>>borrowed from the kernel long ago and had been periodically synced with
> >>>latest. Though U-boot didn't have own in-tree config fragments until very
> >>>recently. First few got into the main config/ directory, but going forward
> >>>those will reside in the corresponding board/ directories...
> >>>
> >>>
> >>>>Considering that the am62xxsip-evm release is only two weeks away,
> >>>>can we consider the fragments approach for now and avoid any
> >>>>potential risks or delays. I will work with you in coming weeks to
> >>>>make this better by handling all the cases of uboot-config and
> >>>>fragments.
> >>>
> >>>Sure, this can be handled in stages. A bit more checks would be needed then
> >>>to avoid breaking at least UBOOT_CONFIG use case.
> >>>
> >>>
> >>>>Please let me know if I you agree with my views and if I can proceed
> >>>>with fixing the other issues you highlighted, they are trivial I can
> >>>>submit a patch tomorrow.

Re: [EXTERNAL] Re: [meta-ti][master/kirkstone][PATCH 1/2] recipes-bsp: u-boot: Add u-boot-mergeconfig.inc to handle fragment u-boot config

[Chirag Shilwant]

On 15/09/23 03:54, Denys Dmytriyenko wrote:
> On Thu, Sep 14, 2023 at 03:46:57PM -0500, Ryan Eatmon wrote:
>>
>> On 9/14/2023 2:42 PM, Denys Dmytriyenko wrote:
>>> On Thu, Sep 14, 2023 at 01:23:31PM -0500, Ryan Eatmon wrote:
>>>>
>>>> On 9/14/2023 12:08 PM, Denys Dmytriyenko wrote:
>>>>> On Wed, Sep 13, 2023 at 10:12:57PM +0530, Chirag Shilwant via lists.yoctoproject.org wrote:
>>>>>> On 13/09/23 01:34, Denys Dmytriyenko wrote:
>>>>>>> On Sat, Sep 09, 2023 at 02:00:04AM +0530, Chirag Shilwant wrote:
>>>>>>>> - Add a new file u-boot-mergeconfig.inc which will ensure we handle fragment u-boot configs
>>>>>>>> using a new variable UBOOT_CONFIG_FRAGMENT which stores the name of fragment u-boot config
>>>>>>>> to be used.
>>>>>>> Would be nice to provide extra details here in the commit message about config
>>>>>>> fragment support in U-boot and its recipe. E.g.:
>>>>>>>
>>>>>>> * U-boot recipe in OE-Core supports out-of-tree config fragments that are
>>>>>>> passed via SRC_URI and automatically merges all *.cfg files as fragments.
>>>>>>> This makes specifying config fragments in the machine configuration a bit
>>>>>>> difficult.
>>>>>>>
>>>>>>> * U-boot itself supports in-tree config fragments and recently been adding
>>>>>>> fragments with *.config extension (first in configs/ dir, but will be moving
>>>>>>> to the corresponding board/ dir), so adding a way to specify and pass those.
>>>>>> Sure, will update the commit message & provide extra details in my
>>>>>> v2 PATCH.
>>>>>>
>>>>>>>> - Include u-boot-mergeconfig.inc in u-boot-ti.inc
>>>>>>>>
>>>>>>>> Signed-off-by: Chirag Shilwant <c-shilwant@ti.com>
>>>>>>>> ---
>>>>>>>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc | 7 +++++++
>>>>>>>>   meta-ti-bsp/recipes-bsp/u-boot/u-boot-ti.inc          | 1 +
>>>>>>>>   2 files changed, 8 insertions(+)
>>>>>>>>   create mode 100644 meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>>>>>
>>>>>>>> diff --git a/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>>>>> new file mode 100644
>>>>>>>> index 00000000..69db6260
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/meta-ti-bsp/recipes-bsp/u-boot/u-boot-mergeconfig.inc
>>>>>>>> @@ -0,0 +1,7 @@
>>>>>>>> +do_compile:prepend () {
>>>>>>> Should be done in do_configure instead. Tasks should be self-contained and
>>>>>>> granular. Plus, do_compile can be repeated w/o do_configure. If you are
>>>>>>> re-configuring every time and generate a new .config file in do_compile,
>>>>>>> that would probably trigger full re-compile due to make tracking file
>>>>>>> timestamps...
>>>>>>>
>>>>>> Yes, we can update this to happen in do_configure. I believe
>>>>>> do_configure:append should be good & will work. Will handle this in
>>>>>> v2.
>>>>>>
>>>>>>>> +   if [ -n "${UBOOT_CONFIG_FRAGMENT}" ]
>>>>>>> Multiple fragments are supported, so maybe call it UBOOT_CONFIG_FRAGMENTS
>>>>>>> plural?
>>>>>>>
>>>>>>>
>>>>>>>> +   then
>>>>>>>> +       oe_runmake -C ${S} O=${B} ${UBOOT_MACHINE} ${UBOOT_CONFIG_FRAGMENT}
>>>>>>>> +       oe_runmake -C ${S} O=${B} olddefconfig
>>>>>>>> +   fi
>>>>>>> So, this basically repeats configuration done in u-boot-configure.inc in
>>>>>>> OE-Core. But it also ignores UBOOT_CONFIG support for multiple (def-)configs.
>>>>>>> While I realize you just want to address a single use-case, making it a bit
>>>>>>> future-proof shouldn't be overlooked.
>>>>>>>
>>>>>>> Think of supporting both UBOOT_CONFIG and UBOOT_CONFIG_FRAGMENTS at the same
>>>>>>> time. Probably completely rewriting do_configure from u-boot-configure.inc
>>>>>>> would be needed...
>>>>>>>
>>>>>>> BTW, UBOOT_CONFIG naming is unfortunate - it has nothing to do with config
>>>>>>> fragments. While UBOOT_MACHINE specifies a single defconfig, UBOOT_CONFIG
>>>>>>> takes a list of defconfigs and iterates through them building each separately.
>>>>>>>
>>>>>> Thanks for your suggestions. I appreciate your idea to cover all use
>>>>>> cases with u-boot-mergeconfig.inc, but I have few concerns due to
>>>>>> the current release situation we are into.
>>>>>>
>>>>>> As far as I know, meta-ti does not have a use case for UBOOT_CONFIG
>>>>>> itself.
>>>>> It does:
>>>>> https://git.yoctoproject.org/meta-ti/tree/meta-ti-bsp/conf/machine/am335x-hs-evm.conf#n7
>>>>>
>>>>> Which means, since your new code is added to all TI platforms, it could
>>>>> potentially break AM335x HS platform.
>>>> The code is wrapped behind the setting of UBOOT_CONFIG_FRAGMENT, so
>>>> unless we set both UBOOT_CONFIG_FRAGMENT and UBOOT_CONFIG in am335
>>>> it should be ok.
>>> Correct, that's why I phrased it as "could potentially break". Maybe not even
>>> specific to TI's AM335x HS, but to some downstream customer platform using
>>> meta-ti. As meta-ti is not the end product.
>> Excellent point.  We really do need to do this the right way and not
>> punt this too far off into the future to do the correct
>> implementation.
> At least doing an extra check for UBOOT_MACHINE being set, as I mentioned in
> another thread, would be a good first step.
>

Great catch! It's good to have a check for UBOOT_MACHINE being set so 
that it
doesn't enter do_configure:append when users try to use UBOOT_CONFIG 
with UBOOT_CONFIG_FRAGMENT.
Will incorporate this with all other feedbacks you have mentioned in 
another thread and send a v4 patch.


>>>>>> We are using UBOOT_MACHINE for handling the base defconfig,
>>>>>> which works well for our needs. The concept of having u-boot
>>>>>> fragments was introduced recently in ti-u-boot after we got a
>>>>>> feedback from upstream uboot as around 90% of configs were same
>>>>>> between am62xx-evm & am62xxsip-evm. This was a valid input and hence
>>>>>> we considered in ti-u-boot. In order to support that I had to find a
>>>>>> quick way to handle this in Yocto it with UBOOT_MACHINE along with
>>>>>> UBOOT_CONFIG_FRAGMENT logic, which I had also discussed with you on
>>>>>> the syncup call.
>>>>> Actually, the support was pretty much always there in U-boot, as kconfig
>>>>> framework along with support for fragments by calling merge_config.sh was
>>>>> borrowed from the kernel long ago and had been periodically synced with
>>>>> latest. Though U-boot didn't have own in-tree config fragments until very
>>>>> recently. First few got into the main config/ directory, but going forward
>>>>> those will reside in the corresponding board/ directories...
>>>>>
>>>>>
>>>>>> Considering that the am62xxsip-evm release is only two weeks away,
>>>>>> can we consider the fragments approach for now and avoid any
>>>>>> potential risks or delays. I will work with you in coming weeks to
>>>>>> make this better by handling all the cases of uboot-config and
>>>>>> fragments.
>>>>> Sure, this can be handled in stages. A bit more checks would be needed then
>>>>> to avoid breaking at least UBOOT_CONFIG use case.
>>>>>
>>>>>
>>>>>> Please let me know if I you agree with my views and if I can proceed
>>>>>> with fixing the other issues you highlighted, they are trivial I can
>>>>>> submit a patch tomorrow.