[RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Moteen Shah]

In the U-Boot pre-relocation stage, if the parent node lacks bootph*
property and the driver lacks a pre-reloc flag, all of its subsequent
subnodes gets skipped over from driver binding—even if they have a
bootph* property.

This series addresses the issue by scanning through all the subnodes
of the current node for the bootph* property and propagate it to all
of its supernodes, ensuring that all of the applicable drivers are
bound and probed prior to relocation. This series implements one of
the solutions mentioned in [0].

Since, all the nodes which are not having any bootph* property will
also be traversed, we will have to incur some overheads in boot time,
hence protecting the feature under a config.

Boot time overheads:
    Baseline: Upstream u-boot

    Patch test: Baseline + remove all bootph-all properties from
    *-u-boot.dtsi except the ones which are supposed to be probed
    but have no bootph in any of its subnode.

    J7200 delta from baseline  : ~100ms
    J784S4 delta from baseline : ~350ms

Boot logs with the patch:
https://gist.github.com/Jamm02/d6e76233437525b266ec96f8a7982096

References: 
[0] https://lore.kernel.org/u-boot/CAFLszTiEZYoo+=+MJg7sgN93k144zEi18A5_Hk+J3sce00g+=w@mail.gmail.com/

Moteen Shah (2):
  arch: arm: Kconfig: Add config to use subnode's bootph property for
    binding drivers
  drivers: core: lists.c: Bind drivers using bootph* property in
    subnodes

 arch/arm/Kconfig     | 11 ++++++
 drivers/core/lists.c | 88 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 97 insertions(+), 2 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Moteen Shah]

Add a new config when set will traverse through all the subnodes of
a given node scanning for bootph-all property and propagate it to
all of its parent node up the hierarchy.

Signed-off-by: Moteen Shah <m-shah@ti.com>
---
 arch/arm/Kconfig | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 314916527c9..51fc952b0db 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -805,6 +805,7 @@ config ARCH_K3
 	select FIT
 	select REGEX
 	select FIT_SIGNATURE if ARM64
+	imply BIND_FROM_CHILD_BOOTPH
 	imply TI_SECURE_DEVICE
 
 config ARCH_OMAP2PLUS
@@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
 	  Path within the source directory to the kwbimage.cfg file to use
 	  when packaging the U-Boot image for use.
 
+
+config BIND_FROM_CHILD_BOOTPH
+	bool "Bind drivers from bootph* in subnode"
+	depends on ARCH_K3
+	help
+	 This config must be set to bind drivers in pre reloc stage whose
+	 compatible parent nodes are implicitly declared to be bound to
+	 their respective drivers by having bootph* property in one of
+	 their subnodes.
+
 source "arch/arm/mach-apple/Kconfig"
 
 source "arch/arm/mach-aspeed/Kconfig"
-- 
2.34.1

Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Kumar, Udit]

On 2/12/2025 2:48 PM, Moteen Shah wrote:
> Add a new config when set will traverse through all the subnodes of
> a given node scanning for bootph-all property and propagate it to
> all of its parent node up the hierarchy.
>
> Signed-off-by: Moteen Shah <m-shah@ti.com>
> ---
>   arch/arm/Kconfig | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 314916527c9..51fc952b0db 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -805,6 +805,7 @@ config ARCH_K3
>   	select FIT
>   	select REGEX
>   	select FIT_SIGNATURE if ARM64
> +	imply BIND_FROM_CHILD_BOOTPH

Why config, this fix looks to be valid for all configs ?


>   	imply TI_SECURE_DEVICE
>   
>   config ARCH_OMAP2PLUS
> @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
>   	  Path within the source directory to the kwbimage.cfg file to use
>   	  when packaging the U-Boot image for use.
>   
> +
> +config BIND_FROM_CHILD_BOOTPH
> +	bool "Bind drivers from bootph* in subnode"
> +	depends on ARCH_K3
> +	help
> +	 This config must be set to bind drivers in pre reloc stage whose
> +	 compatible parent nodes are implicitly declared to be bound to
> +	 their respective drivers by having bootph* property in one of
> +	 their subnodes.
> +
>   source "arch/arm/mach-apple/Kconfig"
>   
>   source "arch/arm/mach-aspeed/Kconfig"

Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Moteen Shah]

Hey Udit,

On 12/02/25 20:19, Kumar, Udit wrote:
>
> On 2/12/2025 2:48 PM, Moteen Shah wrote:
>> Add a new config when set will traverse through all the subnodes of
>> a given node scanning for bootph-all property and propagate it to
>> all of its parent node up the hierarchy.
>>
>> Signed-off-by: Moteen Shah <m-shah@ti.com>
>> ---
>>   arch/arm/Kconfig | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 314916527c9..51fc952b0db 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -805,6 +805,7 @@ config ARCH_K3
>>       select FIT
>>       select REGEX
>>       select FIT_SIGNATURE if ARM64
>> +    imply BIND_FROM_CHILD_BOOTPH
>
> Why config, this fix looks to be valid for all configs ?
True, but for vendors having a fixed up DT and have constraints for faster
boot times, this fix will add up overheads of traversing all of the nodes.

Regards,
Moteen
>
>
>>       imply TI_SECURE_DEVICE
>>     config ARCH_OMAP2PLUS
>> @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
>>         Path within the source directory to the kwbimage.cfg file to use
>>         when packaging the U-Boot image for use.
>>   +
>> +config BIND_FROM_CHILD_BOOTPH
>> +    bool "Bind drivers from bootph* in subnode"
>> +    depends on ARCH_K3
>> +    help
>> +     This config must be set to bind drivers in pre reloc stage whose
>> +     compatible parent nodes are implicitly declared to be bound to
>> +     their respective drivers by having bootph* property in one of
>> +     their subnodes.
>> +
>>   source "arch/arm/mach-apple/Kconfig"
>>     source "arch/arm/mach-aspeed/Kconfig"

Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Simon Glass]

Hi Moteen,

On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah@ti.com> wrote:
>
> Add a new config when set will traverse through all the subnodes of
> a given node scanning for bootph-all property and propagate it to
> all of its parent node up the hierarchy.
>
> Signed-off-by: Moteen Shah <m-shah@ti.com>
> ---
>  arch/arm/Kconfig | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 314916527c9..51fc952b0db 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -805,6 +805,7 @@ config ARCH_K3
>         select FIT
>         select REGEX
>         select FIT_SIGNATURE if ARM64
> +       imply BIND_FROM_CHILD_BOOTPH
>         imply TI_SECURE_DEVICE
>
>  config ARCH_OMAP2PLUS
> @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
>           Path within the source directory to the kwbimage.cfg file to use
>           when packaging the U-Boot image for use.
>
> +
> +config BIND_FROM_CHILD_BOOTPH

How about DM_F_STRICT_BOOTPH ?  or DM_F_CHILD_BOOTPH ?

It indicates that it relates to driver model before relocation.

This behaviour is actually required by the schema. I agree it should
be optional, but only due to its performance issues.

A Binman solution would not have any performance issues.

> +       bool "Bind drivers from bootph* in subnode"
> +       depends on ARCH_K3
> +       help
> +        This config must be set to bind drivers in pre reloc stage whose
> +        compatible parent nodes are implicitly declared to be bound to
> +        their respective drivers by having bootph* property in one of
> +        their subnodes.
> +
>  source "arch/arm/mach-apple/Kconfig"
>
>  source "arch/arm/mach-aspeed/Kconfig"
> --
> 2.34.1
>

Regards,
Simon

Re: [EXTERNAL] Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Moteen Shah]

On 13/02/25 19:31, Simon Glass wrote:
> Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah 
> <m-shah@ ti. com> wrote: > > Add a new config when set will traverse 
> through all the subnodes of > a given node scanning for bootph-all 
> property and propagate it to > all of
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoUgIvDspkOY_N58UTgTbK8UGS8zfYNsuu6kFHonhYC9p6QDyxO6fEM-jwDoJbz7g9IYZuow6CShwWbGJRkE39jUS3OLaz9G1-q9eGLvRc6M$> 
>
> ZjQcmQRYFpfptBannerEnd
> Hi Moteen,
>
> On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah@ti.com> wrote:
> >
> > Add a new config when set will traverse through all the subnodes of
> > a given node scanning for bootph-all property and propagate it to
> > all of its parent node up the hierarchy.
> >
> > Signed-off-by: Moteen Shah <m-shah@ti.com>
> > ---
> >  arch/arm/Kconfig | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 314916527c9..51fc952b0db 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -805,6 +805,7 @@ config ARCH_K3
> >         select FIT
> >         select REGEX
> >         select FIT_SIGNATURE if ARM64
> > +       imply BIND_FROM_CHILD_BOOTPH
> >         imply TI_SECURE_DEVICE
> >
> >  config ARCH_OMAP2PLUS
> > @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
> >           Path within the source directory to the kwbimage.cfg file to use
> >           when packaging the U-Boot image for use.
> >
> > +
> > +config BIND_FROM_CHILD_BOOTPH
>
> How about DM_F_STRICT_BOOTPH ?  or DM_F_CHILD_BOOTPH ?

Yes, this should be more descriptive, will include this in v2.

>
> It indicates that it relates to driver model before relocation.
>
> This behaviour is actually required by the schema. I agree it should
> be optional, but only due to its performance issues.
>
> A Binman solution would not have any performance issues.

Will explore on this more, if you have some more pointers on this,
then do let me know.


Regards,
Moteen

>
> > +       bool "Bind drivers from bootph* in subnode"
> > +       depends on ARCH_K3
> > +       help
> > +        This config must be set to bind drivers in pre reloc stage whose
> > +        compatible parent nodes are implicitly declared to be bound to
> > +        their respective drivers by having bootph* property in one of
> > +        their subnodes.
> > +
> >  source "arch/arm/mach-apple/Kconfig"
> >
> >  source "arch/arm/mach-aspeed/Kconfig"
> > --
> > 2.34.1
> >
>
> Regards,
> Simon

Re: [EXTERNAL] Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Simon Glass]

Hi Moteen,

On Thu, 13 Feb 2025 at 22:05, Moteen Shah <m-shah@ti.com> wrote:
>
>
> On 13/02/25 19:31, Simon Glass wrote:
> > Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah
> > <m-shah@ ti. com> wrote: > > Add a new config when set will traverse
> > through all the subnodes of > a given node scanning for bootph-all
> > property and propagate it to > all of
> > ZjQcmQRYFpfptBannerStart
> > This message was sent from outside of Texas Instruments.
> > Do not click links or open attachments unless you recognize the source
> > of this email and know the content is safe.
> > Report Suspicious
> > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoUgIvDspkOY_N58UTgTbK8UGS8zfYNsuu6kFHonhYC9p6QDyxO6fEM-jwDoJbz7g9IYZuow6CShwWbGJRkE39jUS3OLaz9G1-q9eGLvRc6M$>
> >
> > ZjQcmQRYFpfptBannerEnd
> > Hi Moteen,
> >
> > On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah@ti.com> wrote:
> > >
> > > Add a new config when set will traverse through all the subnodes of
> > > a given node scanning for bootph-all property and propagate it to
> > > all of its parent node up the hierarchy.
> > >
> > > Signed-off-by: Moteen Shah <m-shah@ti.com>
> > > ---
> > >  arch/arm/Kconfig | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 314916527c9..51fc952b0db 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -805,6 +805,7 @@ config ARCH_K3
> > >         select FIT
> > >         select REGEX
> > >         select FIT_SIGNATURE if ARM64
> > > +       imply BIND_FROM_CHILD_BOOTPH
> > >         imply TI_SECURE_DEVICE
> > >
> > >  config ARCH_OMAP2PLUS
> > > @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
> > >           Path within the source directory to the kwbimage.cfg file to use
> > >           when packaging the U-Boot image for use.
> > >
> > > +
> > > +config BIND_FROM_CHILD_BOOTPH
> >
> > How about DM_F_STRICT_BOOTPH ?  or DM_F_CHILD_BOOTPH ?
>
> Yes, this should be more descriptive, will include this in v2.
>
> >
> > It indicates that it relates to driver model before relocation.
> >
> > This behaviour is actually required by the schema. I agree it should
> > be optional, but only due to its performance issues.
> >
> > A Binman solution would not have any performance issues.
>
> Will explore on this more, if you have some more pointers on this,
> then do let me know.

As you requested on the call today:

PrepareImagesAndDtbs() fiddles with the dtb so you can add some code there.

My suggestion is to insert something after this chunk:

    # Get the device tree ready by compiling it and copying the compiled
    # output into a file in our output directly. Then scan it for use
    # in binman.
    dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
    fname = tools.get_output_filename('u-boot.dtb.out')

here you can read in the file and modify it, e.g.:

dtb = fdt.FdtScan(dtb_fname)
add_bootph_nodes(dtb)
dtb.Sync(True)
tools.write_file(fname, dtb.GetContents())

    tools.write_file(fname, tools.read_file(dtb_fname))   # delete this line
    dtb = fdt.FdtScan(fname)

It might need some tweaking. It will need some sort of test, see
ftest.py for that. As an example, testCompressSectionSize() reads back
the dtb to check it., so you can follow that You need to create a .dts
file in tools/binman/test, containing nodes other than 'binman'.

>
>
> Regards,
> Moteen
>
> >
> > > +       bool "Bind drivers from bootph* in subnode"
> > > +       depends on ARCH_K3
> > > +       help
> > > +        This config must be set to bind drivers in pre reloc stage whose
> > > +        compatible parent nodes are implicitly declared to be bound to
> > > +        their respective drivers by having bootph* property in one of
> > > +        their subnodes.
> > > +
> > >  source "arch/arm/mach-apple/Kconfig"
> > >
> > >  source "arch/arm/mach-aspeed/Kconfig"
> > > --
> > > 2.34.1
> > >
> >
> > Regards,
> > Simon

Regards,
Simon

Re: [EXTERNAL] Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Moteen Shah]

On 26/02/25 02:57, Simon Glass wrote:
> Hi Moteen, On Thu, 13 Feb 2025 at 22: 05, Moteen Shah 
> <m-shah@ ti. com> wrote: > > > On 13/02/25 19: 31, Simon Glass wrote: 
> > > Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah > > <m-shah@ 
> ti. com> wrote: >
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoUoEULhq8lPl3ziRhLj9IlA3ZXDoziTij0dPBpn6-DgWRD6rC1BVPnBWST9bilaw10uU17sjumdmw40nkGdzZUT48cV7qkEDIlAys_tcmpU$> 
>
> ZjQcmQRYFpfptBannerEnd
> Hi Moteen,
>
> On Thu, 13 Feb 2025 at 22:05, Moteen Shah <m-shah@ti.com> wrote:
> >
> >
> > On 13/02/25 19:31, Simon Glass wrote:
> > > Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah
> > > <m-shah@ ti. com> wrote: > > Add a new config when set will traverse
> > > through all the subnodes of > a given node scanning for bootph-all
> > > property and propagate it to > all of
> > > ZjQcmQRYFpfptBannerStart
> > > This message was sent from outside of Texas Instruments.
> > > Do not click links or open attachments unless you recognize the source
> > > of this email and know the content is safe.
> > > Report Suspicious
> > > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoUgIvDspkOY_N58UTgTbK8UGS8zfYNsuu6kFHonhYC9p6QDyxO6fEM-jwDoJbz7g9IYZuow6CShwWbGJRkE39jUS3OLaz9G1-q9eGLvRc6M$>
> > >
> > > ZjQcmQRYFpfptBannerEnd
> > > Hi Moteen,
> > >
> > > On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah@ti.com> wrote:
> > > >
> > > > Add a new config when set will traverse through all the subnodes of
> > > > a given node scanning for bootph-all property and propagate it to
> > > > all of its parent node up the hierarchy.
> > > >
> > > > Signed-off-by: Moteen Shah <m-shah@ti.com>
> > > > ---
> > > >  arch/arm/Kconfig | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > index 314916527c9..51fc952b0db 100644
> > > > --- a/arch/arm/Kconfig
> > > > +++ b/arch/arm/Kconfig
> > > > @@ -805,6 +805,7 @@ config ARCH_K3
> > > >         select FIT
> > > >         select REGEX
> > > >         select FIT_SIGNATURE if ARM64
> > > > +       imply BIND_FROM_CHILD_BOOTPH
> > > >         imply TI_SECURE_DEVICE
> > > >
> > > >  config ARCH_OMAP2PLUS
> > > > @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
> > > >           Path within the source directory to the kwbimage.cfg file to use
> > > >           when packaging the U-Boot image for use.
> > > >
> > > > +
> > > > +config BIND_FROM_CHILD_BOOTPH
> > >
> > > How about DM_F_STRICT_BOOTPH ?  or DM_F_CHILD_BOOTPH ?
> >
> > Yes, this should be more descriptive, will include this in v2.
> >
> > >
> > > It indicates that it relates to driver model before relocation.
> > >
> > > This behaviour is actually required by the schema. I agree it should
> > > be optional, but only due to its performance issues.
> > >
> > > A Binman solution would not have any performance issues.
> >
> > Will explore on this more, if you have some more pointers on this,
> > then do let me know.
>
> As you requested on the call today:
>
> PrepareImagesAndDtbs() fiddles with the dtb so you can add some code there.
>
> My suggestion is to insert something after this chunk:
>
>      # Get the device tree ready by compiling it and copying the compiled
>      # output into a file in our output directly. Then scan it for use
>      # in binman.
>      dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
>      fname = tools.get_output_filename('u-boot.dtb.out')
>
> here you can read in the file and modify it, e.g.:
>
> dtb = fdt.FdtScan(dtb_fname)
> add_bootph_nodes(dtb)
> dtb.Sync(True)
> tools.write_file(fname, dtb.GetContents())
>
>      tools.write_file(fname, tools.read_file(dtb_fname))   # delete this line
>      dtb = fdt.FdtScan(fname)
>
> It might need some tweaking. It will need some sort of test, see
> ftest.py for that. As an example, testCompressSectionSize() reads back
> the dtb to check it., so you can follow that You need to create a .dts
> file in tools/binman/test, containing nodes other than 'binman'.


Thanks for the pointers Simon. I'll start with the implementation soon 
after checking the time delta
we get with this patch but with direct binding rather than propagating 
bootph-* properties.


> >
> >
> > Regards,
> > Moteen
> >
> > >
> > > > +       bool "Bind drivers from bootph* in subnode"
> > > > +       depends on ARCH_K3
> > > > +       help
> > > > +        This config must be set to bind drivers in pre reloc stage whose
> > > > +        compatible parent nodes are implicitly declared to be bound to
> > > > +        their respective drivers by having bootph* property in one of
> > > > +        their subnodes.
> > > > +
> > > >  source "arch/arm/mach-apple/Kconfig"
> > > >
> > > >  source "arch/arm/mach-aspeed/Kconfig"
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Regards,
> > > Simon
>
> Regards,
> Simon


Regards,
Moteen

Re: [EXTERNAL] Re: [RFC PATCH 1/2 v1] arch: arm: Kconfig: Add config to use subnode's bootph property for binding drivers

[Moteen Shah]

Hello Simon,

On 07/03/25 10:25, Moteen Shah wrote:
>
> On 26/02/25 02:57, Simon Glass wrote:
>> Hi Moteen, On Thu, 13 Feb 2025 at 22: 05, Moteen Shah 
>> <m-shah@ ti. com> wrote: > > > On 13/02/25 19: 31, Simon Glass wrote: 
>> > > Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah > > 
>> <m-shah@ ti. com> wrote: >
>> ZjQcmQRYFpfptBannerStart
>> This message was sent from outside of Texas Instruments.
>> Do not click links or open attachments unless you recognize the 
>> source of this email and know the content is safe.
>> Report Suspicious
>> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoUoEULhq8lPl3ziRhLj9IlA3ZXDoziTij0dPBpn6-DgWRD6rC1BVPnBWST9bilaw10uU17sjumdmw40nkGdzZUT48cV7qkEDIlAys_tcmpU$> 
>>
>> ZjQcmQRYFpfptBannerEnd
>> Hi Moteen,
>>
>> On Thu, 13 Feb 2025 at 22:05, Moteen Shah <m-shah@ti.com> wrote:
>> >
>> >
>> > On 13/02/25 19:31, Simon Glass wrote:
>> > > Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah
>> > > <m-shah@ ti. com> wrote: > > Add a new config when set will traverse
>> > > through all the subnodes of > a given node scanning for bootph-all
>> > > property and propagate it to > all of
>> > > ZjQcmQRYFpfptBannerStart
>> > > This message was sent from outside of Texas Instruments.
>> > > Do not click links or open attachments unless you recognize the 
>> source
>> > > of this email and know the content is safe.
>> > > Report Suspicious
>> > > 
>> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoUgIvDspkOY_N58UTgTbK8UGS8zfYNsuu6kFHonhYC9p6QDyxO6fEM-jwDoJbz7g9IYZuow6CShwWbGJRkE39jUS3OLaz9G1-q9eGLvRc6M$>
>> > >
>> > > ZjQcmQRYFpfptBannerEnd
>> > > Hi Moteen,
>> > >
>> > > On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah@ti.com> wrote:
>> > > >
>> > > > Add a new config when set will traverse through all the 
>> subnodes of
>> > > > a given node scanning for bootph-all property and propagate it to
>> > > > all of its parent node up the hierarchy.
>> > > >
>> > > > Signed-off-by: Moteen Shah <m-shah@ti.com>
>> > > > ---
>> > > >  arch/arm/Kconfig | 11 +++++++++++
>> > > >  1 file changed, 11 insertions(+)
>> > > >
>> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> > > > index 314916527c9..51fc952b0db 100644
>> > > > --- a/arch/arm/Kconfig
>> > > > +++ b/arch/arm/Kconfig
>> > > > @@ -805,6 +805,7 @@ config ARCH_K3
>> > > >         select FIT
>> > > >         select REGEX
>> > > >         select FIT_SIGNATURE if ARM64
>> > > > +       imply BIND_FROM_CHILD_BOOTPH
>> > > >         imply TI_SECURE_DEVICE
>> > > >
>> > > >  config ARCH_OMAP2PLUS
>> > > > @@ -2232,6 +2233,16 @@ config SYS_KWD_CONFIG
>> > > >           Path within the source directory to the kwbimage.cfg 
>> file to use
>> > > >           when packaging the U-Boot image for use.
>> > > >
>> > > > +
>> > > > +config BIND_FROM_CHILD_BOOTPH
>> > >
>> > > How about DM_F_STRICT_BOOTPH ?  or DM_F_CHILD_BOOTPH ?
>> >
>> > Yes, this should be more descriptive, will include this in v2.
>> >
>> > >
>> > > It indicates that it relates to driver model before relocation.
>> > >
>> > > This behaviour is actually required by the schema. I agree it should
>> > > be optional, but only due to its performance issues.
>> > >
>> > > A Binman solution would not have any performance issues.
>> >
>> > Will explore on this more, if you have some more pointers on this,
>> > then do let me know.
>>
>> As you requested on the call today:
>>
>> PrepareImagesAndDtbs() fiddles with the dtb so you can add some code 
>> there.
>>
>> My suggestion is to insert something after this chunk:
>>
>>      # Get the device tree ready by compiling it and copying the 
>> compiled
>>      # output into a file in our output directly. Then scan it for use
>>      # in binman.
>>      dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
>>      fname = tools.get_output_filename('u-boot.dtb.out')
>>
>> here you can read in the file and modify it, e.g.:
>>
>> dtb = fdt.FdtScan(dtb_fname)
>> add_bootph_nodes(dtb)
>> dtb.Sync(True)
>> tools.write_file(fname, dtb.GetContents())
>>
>>      tools.write_file(fname, tools.read_file(dtb_fname))   # delete 
>> this line
>>      dtb = fdt.FdtScan(fname)
>>
>> It might need some tweaking. It will need some sort of test, see
>> ftest.py for that. As an example, testCompressSectionSize() reads back
>> the dtb to check it., so you can follow that You need to create a .dts
>> file in tools/binman/test, containing nodes other than 'binman'.
>
>
> Thanks for the pointers Simon. I'll start with the implementation soon 
> after checking the time delta
> we get with this patch but with direct binding rather than propagating 
> bootph-* properties.
>

I checked the time delta with the above mentioned approach, here are the 
results:

Board         Baseline   Previous delay  New delay
J7200-evm       2.2s       ~100 ms         ~60ms
J784s4-evm     2.7s        ~350 ms     300ms-350ms

The time delta is still significant, a binman solution should be the 
right approach to this, a binman based approach should be the right step 
I think. Will send a v2 based on it.


>
>> >
>> >
>> > Regards,
>> > Moteen
>> >
>> > >
>> > > > +       bool "Bind drivers from bootph* in subnode"
>> > > > +       depends on ARCH_K3
>> > > > +       help
>> > > > +        This config must be set to bind drivers in pre reloc 
>> stage whose
>> > > > +        compatible parent nodes are implicitly declared to be 
>> bound to
>> > > > +        their respective drivers by having bootph* property in 
>> one of
>> > > > +        their subnodes.
>> > > > +
>> > > >  source "arch/arm/mach-apple/Kconfig"
>> > > >
>> > > >  source "arch/arm/mach-aspeed/Kconfig"
>> > > > --
>> > > > 2.34.1
>> > > >
>> > >
>> > > Regards,
>> > > Simon
>>
>> Regards,
>> Simon
>
>
> Regards,
> Moteen


Regards,
Moteen

[RFC PATCH 2/2 v1] drivers: core: lists.c: Bind drivers using bootph* property in subnodes

[Moteen Shah]

Add a function to scan through all the subnodes of a given node
recusively for bootph* property. If found, propagate it to all
of its parent node up the hierarchy so as to bind the respective
drivers of the nodes in the pre-relocation stage. 

The current model skips the node if there is no bootph* found in
it, even if the property is present in one of the subnodes. The 
issue is tracked in [0].

Signed-off-by: Moteen Shah <m-shah@ti.com>

References: 
[0] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21

---
 drivers/core/lists.c | 88 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index c7be504b6fc..5be556ea951 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -17,6 +17,7 @@
 #include <dm/platdata.h>
 #include <dm/uclass.h>
 #include <dm/util.h>
+#include <fdt_support.h>
 #include <fdtdec.h>
 #include <linux/compiler.h>
 
@@ -196,6 +197,80 @@ static int driver_check_compatible(const struct udevice_id *of_match,
 	return -ENOENT;
 }
 
+/**
+ * scan_and_prop_bootph() - Check if a driver matches a compatible string
+ *
+ * @param of_node:	Parent node for child traversal
+ * @param fdt:	Blob to use
+ */
+static int scan_and_prop_bootph(ofnode node, void *fdt)
+{
+	static int bootph_index = -1;
+	static const char * const bootph_props[] = {
+		"bootph-all",
+		"bootph-some-ram",
+		"bootph-pre-ram",
+		"bootph-pre-sram",
+	};
+	int offset;
+	int ret = -ENOENT;
+	ofnode subnode;
+
+	/* Note the index where bootph-* was found and return success */
+	for (int i = 0; i < ARRAY_SIZE(bootph_props); i++) {
+		if (ofnode_read_bool(node, bootph_props[i])) {
+			bootph_index = i;
+			return 0;
+		}
+	}
+
+	ofnode_for_each_subnode(subnode, node) {
+		if (!ofnode_valid(subnode))
+			continue;
+
+		ret = scan_and_prop_bootph(subnode, fdt);
+		if (ret != -ENOENT && ret)
+			return ret;
+
+		/*
+		 * Break the search if bootph-* is found in any of the subnodes.
+		 * Breaking the loop early helps in avoiding unnecessary traversal
+		 * of the sibling node given that bootph-* has been found in previous
+		 * sibling node.
+		 */
+		if (!ret)
+			break;
+	}
+
+	/* If no bootph-* found then return no such entry */
+	if (ret)
+		return -ENOENT;
+
+	if (bootph_index != -1) {
+		offset = ofnode_to_offset(node);
+		if (offset < 0) {
+			log_err("Failed to get offset %d\n", offset);
+			return -EINVAL;
+		}
+
+		ret = fdt_increase_size(fdt, 32);
+		if (ret) {
+			log_err("Cannot increase FDT size!\n");
+			return ret;
+		}
+
+		ret = fdt_setprop_empty(fdt, offset, bootph_props[bootph_index]);
+		if (ret) {
+			log_err("Failed to add bootph prop to node:%s ret=%d\n",
+				ofnode_get_name(node), ret);
+			return ret;
+		}
+		bootph_index = -1;
+	}
+
+	return ret;
+}
+
 int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 		   struct driver *drv, bool pre_reloc_only)
 {
@@ -205,6 +280,7 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 	struct driver *entry;
 	struct udevice *dev;
 	bool found = false;
+	void *fdt = (void *)gd->fdt_blob;
 	const char *name, *compat_list, *compat;
 	int compat_length, i;
 	int result = 0;
@@ -256,8 +332,16 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 		if (pre_reloc_only) {
 			if (!ofnode_pre_reloc(node) &&
 			    !(entry->flags & DM_FLAG_PRE_RELOC)) {
-				log_debug("Skipping device pre-relocation\n");
-				return 0;
+				if (IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH)) {
+					ret = scan_and_prop_bootph(node, fdt);
+					if (ret) {
+						log_debug("Skipping device pre-relocation\n");
+						return 0;
+					}
+				} else {
+					log_debug("Skipping device pre-relocation\n");
+					return 0;
+				}
 			}
 		}
 
-- 
2.34.1

Re: [RFC PATCH 2/2 v1] drivers: core: lists.c: Bind drivers using bootph* property in subnodes

[Simon Glass]

Hi Moteen,

On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah@ti.com> wrote:
>
> Add a function to scan through all the subnodes of a given node
> recusively for bootph* property. If found, propagate it to all
> of its parent node up the hierarchy so as to bind the respective
> drivers of the nodes in the pre-relocation stage.
>
> The current model skips the node if there is no bootph* found in
> it, even if the property is present in one of the subnodes. The
> issue is tracked in [0].
>
> Signed-off-by: Moteen Shah <m-shah@ti.com>
>
> References:
> [0] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21
>
> ---
>  drivers/core/lists.c | 88 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index c7be504b6fc..5be556ea951 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -17,6 +17,7 @@
>  #include <dm/platdata.h>
>  #include <dm/uclass.h>
>  #include <dm/util.h>
> +#include <fdt_support.h>
>  #include <fdtdec.h>
>  #include <linux/compiler.h>
>
> @@ -196,6 +197,80 @@ static int driver_check_compatible(const struct udevice_id *of_match,
>         return -ENOENT;
>  }
>
> +/**
> + * scan_and_prop_bootph() - Check if a driver matches a compatible string
> + *
> + * @param of_node:     Parent node for child traversal
> + * @param fdt: Blob to use

We stopped using @param, so just @of_node now

Document return

> + */
> +static int scan_and_prop_bootph(ofnode node, void *fdt)
> +{
> +       static int bootph_index = -1;
> +       static const char * const bootph_props[] = {
> +               "bootph-all",
> +               "bootph-some-ram",
> +               "bootph-pre-ram",
> +               "bootph-pre-sram",

Since this is pre-relocation, you should only need the first two. But
there is ofnode_pre_reloc() which you can call.

BTW ofnode_pre_reloc() has the checks you have here. I'm not sure if
this code can be dropped?:

/*
* In regular builds individual spl and tpl handling both
* count as handled pre-relocation for later second init.
*/
if (ofnode_read_bool(node, "bootph-pre-ram") ||
    ofnode_read_bool(node, "bootph-pre-sram"))
return gd->flags & GD_FLG_RELOC;


> +       };
> +       int offset;
> +       int ret = -ENOENT;
> +       ofnode subnode;
> +
> +       /* Note the index where bootph-* was found and return success */
> +       for (int i = 0; i < ARRAY_SIZE(bootph_props); i++) {
> +               if (ofnode_read_bool(node, bootph_props[i])) {
> +                       bootph_index = i;
> +                       return 0;
> +               }
> +       }
> +
> +       ofnode_for_each_subnode(subnode, node) {
> +               if (!ofnode_valid(subnode))
> +                       continue;
> +
> +               ret = scan_and_prop_bootph(subnode, fdt);
> +               if (ret != -ENOENT && ret)
> +                       return ret;
> +
> +               /*
> +                * Break the search if bootph-* is found in any of the subnodes.
> +                * Breaking the loop early helps in avoiding unnecessary traversal
> +                * of the sibling node given that bootph-* has been found in previous
> +                * sibling node.
> +                */
> +               if (!ret)
> +                       break;
> +       }
> +
> +       /* If no bootph-* found then return no such entry */
> +       if (ret)
> +               return -ENOENT;
> +
> +       if (bootph_index != -1) {
> +               offset = ofnode_to_offset(node);
> +               if (offset < 0) {
> +                       log_err("Failed to get offset %d\n", offset);
> +                       return -EINVAL;
> +               }
> +
> +               ret = fdt_increase_size(fdt, 32);
> +               if (ret) {
> +                       log_err("Cannot increase FDT size!\n");
> +                       return ret;
> +               }
> +
> +               ret = fdt_setprop_empty(fdt, offset, bootph_props[bootph_index]);

I don't believe you need to actually add the property. It costs time
and I don't believe anything else will read it. You should just need
to return a value indicating whether the tag was found.

> +               if (ret) {
> +                       log_err("Failed to add bootph prop to node:%s ret=%d\n",
> +                               ofnode_get_name(node), ret);
> +                       return ret;
> +               }
> +               bootph_index = -1;
> +       }
> +
> +       return ret;
> +}
> +
>  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                    struct driver *drv, bool pre_reloc_only)
>  {
> @@ -205,6 +280,7 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>         struct driver *entry;
>         struct udevice *dev;
>         bool found = false;
> +       void *fdt = (void *)gd->fdt_blob;

Could you drop this var? I am hoping you don't need to change the devicetree.

>         const char *name, *compat_list, *compat;
>         int compat_length, i;
>         int result = 0;
> @@ -256,8 +332,16 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                 if (pre_reloc_only) {
>                         if (!ofnode_pre_reloc(node) &&
>                             !(entry->flags & DM_FLAG_PRE_RELOC)) {
> -                               log_debug("Skipping device pre-relocation\n");
> -                               return 0;
> +                               if (IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH)) {
> +                                       ret = scan_and_prop_bootph(node, fdt);
> +                                       if (ret) {
> +                                               log_debug("Skipping device pre-relocation\n");
> +                                               return 0;
> +                                       }
> +                               } else {
> +                                       log_debug("Skipping device pre-relocation\n");
> +                                       return 0;
> +                               }

Can you either use a bool or combine the statements so there is only
one log_debug() ?

if (!IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH) ||
   scan_and_prop_bootph(node, fdt)) {
     log_debug...

>                         }
>                 }
>
> --
> 2.34.1
>

Please also add a test for this, probably to test/dm/test-fdt.c

Did you look at doing this in Binman?

Regards,
Simon

Re: [EXTERNAL] Re: [RFC PATCH 2/2 v1] drivers: core: lists.c: Bind drivers using bootph* property in subnodes

[Moteen Shah]

Hey Simon

On 12/02/25 19:07, Simon Glass wrote:
> Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah 
> <m-shah@ ti. com> wrote: > > Add a function to scan through all the 
> subnodes of a given node > recusively for bootph* property. If found, 
> propagate it to all > of its parent
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoS0MsZKL_DT2tZpXfsvUu_exS1E1jUk7Sm4iKSS-jq4dD2ZmFBvi3F-y_3fESFLg5xMd4B_mX8lyj-MIXHAbpyrcoh9f55Q5L2iNtE7kn3s$> 
>
> ZjQcmQRYFpfptBannerEnd
> Hi Moteen,
>
> On Wed, 12 Feb 2025 at 02:18, Moteen Shah<m-shah@ti.com>  wrote:
> >
> > Add a function to scan through all the subnodes of a given node
> > recusively for bootph* property. If found, propagate it to all
> > of its parent node up the hierarchy so as to bind the respective
> > drivers of the nodes in the pre-relocation stage.
> >
> > The current model skips the node if there is no bootph* found in
> > it, even if the property is present in one of the subnodes. The
> > issue is tracked in [0].
> >
> > Signed-off-by: Moteen Shah <m-shah@ti.com>
> >
> > References:
> > [0] https://urldefense.com/v3/__https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21__;!!G3vK!W_iIPAjX_8KQ0KYkaLmMpffBZbsxQLF5MvKHbZeivm2bxn7gih_UYCufMU0pfun_xo1lK17Y$
> >
> > ---
> >  drivers/core/lists.c | 88 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 86 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> > index c7be504b6fc..5be556ea951 100644
> > --- a/drivers/core/lists.c
> > +++ b/drivers/core/lists.c
> > @@ -17,6 +17,7 @@
> >  #include <dm/platdata.h>
> >  #include <dm/uclass.h>
> >  #include <dm/util.h>
> > +#include <fdt_support.h>
> >  #include <fdtdec.h>
> >  #include <linux/compiler.h>
> >
> > @@ -196,6 +197,80 @@ static int driver_check_compatible(const struct udevice_id *of_match,
> >         return -ENOENT;
> >  }
> >
> > +/**
> > + * scan_and_prop_bootph() - Check if a driver matches a compatible string
> > + *
> > + * @param of_node:     Parent node for child traversal
> > + * @param fdt: Blob to use
>
> We stopped using @param, so just @of_node now
>
> Document return
Noted, will fix this up in v2.
> > + */
> > +static int scan_and_prop_bootph(ofnode node, void *fdt)
> > +{
> > +       static int bootph_index = -1;
> > +       static const char * const bootph_props[] = {
> > +               "bootph-all",
> > +               "bootph-some-ram",
> > +               "bootph-pre-ram",
> > +               "bootph-pre-sram",
>
> Since this is pre-relocation, you should only need the first two. But
> there is ofnode_pre_reloc() which you can call.
>
> BTW ofnode_pre_reloc() has the checks you have here. I'm not sure if
> this code can be dropped?:
I started with using ofnode_pre_reloc(), but the function won't return
which bootph-* property was found hence the manual checks here.
> /*
> * In regular builds individual spl and tpl handling both
> * count as handled pre-relocation for later second init.
> */
> if (ofnode_read_bool(node, "bootph-pre-ram") ||
>      ofnode_read_bool(node, "bootph-pre-sram"))
> return gd->flags & GD_FLG_RELOC;
>
>
> > +       };
> > +       int offset;
> > +       int ret = -ENOENT;
> > +       ofnode subnode;
> > +
> > +       /* Note the index where bootph-* was found and return success */
> > +       for (int i = 0; i < ARRAY_SIZE(bootph_props); i++) {
> > +               if (ofnode_read_bool(node, bootph_props[i])) {
> > +                       bootph_index = i;
> > +                       return 0;
> > +               }
> > +       }
> > +
> > +       ofnode_for_each_subnode(subnode, node) {
> > +               if (!ofnode_valid(subnode))
> > +                       continue;
> > +
> > +               ret = scan_and_prop_bootph(subnode, fdt);
> > +               if (ret != -ENOENT && ret)
> > +                       return ret;
> > +
> > +               /*
> > +                * Break the search if bootph-* is found in any of the subnodes.
> > +                * Breaking the loop early helps in avoiding unnecessary traversal
> > +                * of the sibling node given that bootph-* has been found in previous
> > +                * sibling node.
> > +                */
> > +               if (!ret)
> > +                       break;
> > +       }
> > +
> > +       /* If no bootph-* found then return no such entry */
> > +       if (ret)
> > +               return -ENOENT;
> > +
> > +       if (bootph_index != -1) {
> > +               offset = ofnode_to_offset(node);
> > +               if (offset < 0) {
> > +                       log_err("Failed to get offset %d\n", offset);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               ret = fdt_increase_size(fdt, 32);
> > +               if (ret) {
> > +                       log_err("Cannot increase FDT size!\n");
> > +                       return ret;
> > +               }
> > +
> > +               ret = fdt_setprop_empty(fdt, offset, bootph_props[bootph_index]);
>
> I don't believe you need to actually add the property. It costs time
> and I don't believe anything else will read it. You should just need
> to return a value indicating whether the tag was found.
Considering a case that we are deep inside the recursion and bootph-*
is found, with the return value approach we will be able to bind the
super parent node but for nodes under this super parent, we will again
have to traverse the DT right?

One more approach I thought of was to bind the nodes in this function
itself if bootph-* is found in subnode, but for that we will require the
driver/device info of that node which we don't have at this point in the
code flow. (Again considering a node deep in the DT).

> > +               if (ret) {
> > +                       log_err("Failed to add bootph prop to node:%s ret=%d\n",
> > +                               ofnode_get_name(node), ret);
> > +                       return ret;
> > +               }
> > +               bootph_index = -1;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
> >                    struct driver *drv, bool pre_reloc_only)
> >  {
> > @@ -205,6 +280,7 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
> >         struct driver *entry;
> >         struct udevice *dev;
> >         bool found = false;
> > +       void *fdt = (void *)gd->fdt_blob;
>
> Could you drop this var? I am hoping you don't need to change the devicetree.
Sure, if the return value approach works.
> >         const char *name, *compat_list, *compat;
> >         int compat_length, i;
> >         int result = 0;
> > @@ -256,8 +332,16 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
> >                 if (pre_reloc_only) {
> >                         if (!ofnode_pre_reloc(node) &&
> >                             !(entry->flags & DM_FLAG_PRE_RELOC)) {
> > -                               log_debug("Skipping device pre-relocation\n");
> > -                               return 0;
> > +                               if (IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH)) {
> > +                                       ret = scan_and_prop_bootph(node, fdt);
> > +                                       if (ret) {
> > +                                               log_debug("Skipping device pre-relocation\n");
> > +                                               return 0;
> > +                                       }
> > +                               } else {
> > +                                       log_debug("Skipping device pre-relocation\n");
> > +                                       return 0;
> > +                               }
>
> Can you either use a bool or combine the statements so there is only
> one log_debug() ?
Noted, will fix this in v2.
> if (!IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH) ||
>     scan_and_prop_bootph(node, fdt)) {
>       log_debug...
>
> >                         }
> >                 }
> >
> > --
> > 2.34.1
> >
>
> Please also add a test for this, probably to test/dm/test-fdt.c
Noted.
> Did you look at doing this in Binman?
Haven't explored much on the binman implementation, if my understanding
is correct we might have to decompile the blob and do the fixup in
blob_dtb.py likely?
If you have some more pointers on this do let me know.
> Regards,
> Simon


Thank you for the review.

Regards,
Moteen

Re: [RFC PATCH 2/2 v1] drivers: core: lists.c: Bind drivers using bootph* property in subnodes

[Kumar, Udit]

Hello Moteen

On 2/12/2025 2:48 PM, Moteen Shah wrote:
> Add a function to scan through all the subnodes of a given node
> recusively for bootph* property. If found, propagate it to all
> of its parent node up the hierarchy so as to bind the respective
> drivers of the nodes in the pre-relocation stage.
>
> The current model skips the node if there is no bootph* found in
> it, even if the property is present in one of the subnodes. The
> issue is tracked in [0].
>
> Signed-off-by: Moteen Shah <m-shah@ti.com>
>
> References:
> [0] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21
>
> ---
>   drivers/core/lists.c | 88 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index c7be504b6fc..5be556ea951 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -17,6 +17,7 @@
>   #include <dm/platdata.h>
>   #include <dm/uclass.h>
>   #include <dm/util.h>
> +#include <fdt_support.h>
>   #include <fdtdec.h>
>   #include <linux/compiler.h>
>   
> @@ -196,6 +197,80 @@ static int driver_check_compatible(const struct udevice_id *of_match,
>   	return -ENOENT;
>   }
>   
> +/**
> + * scan_and_prop_bootph() - Check if a driver matches a compatible string
> + *
> + * @param of_node:	Parent node for child traversal
> + * @param fdt:	Blob to use
> + */
> +static int scan_and_prop_bootph(ofnode node, void *fdt)
> +{
> +	static int bootph_index = -1;
> +	static const char * const bootph_props[] = {
> +		"bootph-all",
> +		"bootph-some-ram",
> +		"bootph-pre-ram",
> +		"bootph-pre-sram",
> +	};
> +	int offset;
> +	int ret = -ENOENT;
> +	ofnode subnode;
> +
> +	/* Note the index where bootph-* was found and return success */
> +	for (int i = 0; i < ARRAY_SIZE(bootph_props); i++) {
> +		if (ofnode_read_bool(node, bootph_props[i])) {
> +			bootph_index = i;
> +			return 0;
> +		}
> +	}
> +
> +	ofnode_for_each_subnode(subnode, node) {
> +		if (!ofnode_valid(subnode))
> +			continue;
> +
> +		ret = scan_and_prop_bootph(subnode, fdt);
> +		if (ret != -ENOENT && ret)
> +			return ret;
> +
> +		/*
> +		 * Break the search if bootph-* is found in any of the subnodes.
> +		 * Breaking the loop early helps in avoiding unnecessary traversal
> +		 * of the sibling node given that bootph-* has been found in previous
> +		 * sibling node.
> +		 */
> +		if (!ret)
> +			break;
> +	}
> +
> +	/* If no bootph-* found then return no such entry */
> +	if (ret)
> +		return -ENOENT;
> +
> +	if (bootph_index != -1) {
> +		offset = ofnode_to_offset(node);
> +		if (offset < 0) {
> +			log_err("Failed to get offset %d\n", offset);
> +			return -EINVAL;
> +		}
> +
> +		ret = fdt_increase_size(fdt, 32);
> +		if (ret) {
> +			log_err("Cannot increase FDT size!\n");
> +			return ret;
> +		}

I understand you are trying to carry bootph to parent,so that parent can 
be probed.

In case, you are failing here then you need to tune memory map or in 
worse case you end up

not probing needed driver and hang.

then why not to override , we have u-boot specific DTs.



> +
> +		ret = fdt_setprop_empty(fdt, offset, bootph_props[bootph_index]);
> +		if (ret) {
> +			log_err("Failed to add bootph prop to node:%s ret=%d\n",
> +				ofnode_get_name(node), ret);
> +			return ret;
> +		}
> +		bootph_index = -1;
> +	}
> +
> +	return ret;
> +}
> +
>   int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>   		   struct driver *drv, bool pre_reloc_only)
>   {
> @@ -205,6 +280,7 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>   	struct driver *entry;
>   	struct udevice *dev;
>   	bool found = false;
> +	void *fdt = (void *)gd->fdt_blob;
>   	const char *name, *compat_list, *compat;
>   	int compat_length, i;
>   	int result = 0;
> @@ -256,8 +332,16 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>   		if (pre_reloc_only) {
>   			if (!ofnode_pre_reloc(node) &&
>   			    !(entry->flags & DM_FLAG_PRE_RELOC)) {
> -				log_debug("Skipping device pre-relocation\n");
> -				return 0;
> +				if (IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH)) {
> +					ret = scan_and_prop_bootph(node, fdt);
> +					if (ret) {
> +						log_debug("Skipping device pre-relocation\n");
> +						return 0;
> +					}
> +				} else {
> +					log_debug("Skipping device pre-relocation\n");
> +					return 0;
> +				}
>   			}
>   		}
>   

Re: [RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Quentin Schulz]

Hi Moteen,

On 2/12/25 10:18 AM, Moteen Shah wrote:
> [You don't often get email from m-shah@ti.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> In the U-Boot pre-relocation stage, if the parent node lacks bootph*
> property and the driver lacks a pre-reloc flag, all of its subsequent
> subnodes gets skipped over from driver binding—even if they have a
> bootph* property.
> 
> This series addresses the issue by scanning through all the subnodes
> of the current node for the bootph* property and propagate it to all
> of its supernodes, ensuring that all of the applicable drivers are
> bound and probed prior to relocation. This series implements one of
> the solutions mentioned in [0].
> 
> Since, all the nodes which are not having any bootph* property will
> also be traversed, we will have to incur some overheads in boot time,
> hence protecting the feature under a config.
> 
> Boot time overheads:
>      Baseline: Upstream u-boot
> 
>      Patch test: Baseline + remove all bootph-all properties from
>      *-u-boot.dtsi except the ones which are supposed to be probed
>      but have no bootph in any of its subnode.
> 
>      J7200 delta from baseline  : ~100ms
>      J784S4 delta from baseline : ~350ms
> 

Pfew, that's a lot of time. Can you tell us what's the delta in 
percentage from baseline? Because if your system is usually booting in 
one minute, 100ms isn't too bad :)

FYI, I believe we've been hit by this issue on Rockchip but cannot find 
the thread or patch right now.

For TPL and SPL, the Device Tree is parsed and looked for appropriate 
bootph properties. Any node which doesn't have a bootph property and 
doesn't have any children with a bootph property is removed from the 
tree. However, the bootph property (if only present in a children node) 
isn't propagated (meaning the node doesn't get the property). This is 
done by fdtgrep.

The issue is that for U-Boot proper pre-relocation, the whole DT is 
taken and only nodes with the appropriate bootph property is probed and 
children nodes do NOT count as opposed to the TPL/SPL case.

My idea was that maybe we should rather propagate the property, at the 
very least in U-Boot proper pre-relocation. This does mean we will 
increase (by which amount?) the size of the DT in U-Boot proper because 
we would add this property recursively up the tree from a node that has 
the bootph property for U-Boot proper pre-relocation. This **could** be 
an issue as the DT could be passed between stages and we would then hit 
the size limit. Sadly, I didn't take the time to look into adding 
support for that in fdtgrep nor will I have the time to do it, so take 
this as me sharing my wish list with you.

I would really like to avoid having it guarded by a knob though because 
I really think this is an issue that should simply be fixed 
unconditionally for every platform with DT support.

Cheers,
Quentin

Re: [RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Moteen Shah]

On 17/02/25 20:32, Quentin Schulz wrote:
> Hi Moteen,
>
> On 2/12/25 10:18 AM, Moteen Shah wrote:
>> [You don't often get email from m-shah@ti.com. Learn why this is 
>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> In the U-Boot pre-relocation stage, if the parent node lacks bootph*
>> property and the driver lacks a pre-reloc flag, all of its subsequent
>> subnodes gets skipped over from driver binding—even if they have a
>> bootph* property.
>>
>> This series addresses the issue by scanning through all the subnodes
>> of the current node for the bootph* property and propagate it to all
>> of its supernodes, ensuring that all of the applicable drivers are
>> bound and probed prior to relocation. This series implements one of
>> the solutions mentioned in [0].
>>
>> Since, all the nodes which are not having any bootph* property will
>> also be traversed, we will have to incur some overheads in boot time,
>> hence protecting the feature under a config.
>>
>> Boot time overheads:
>>      Baseline: Upstream u-boot
>>
>>      Patch test: Baseline + remove all bootph-all properties from
>>      *-u-boot.dtsi except the ones which are supposed to be probed
>>      but have no bootph in any of its subnode.
>>
>>      J7200 delta from baseline  : ~100ms
>>      J784S4 delta from baseline : ~350ms
>>
>
> Pfew, that's a lot of time. Can you tell us what's the delta in 
> percentage from baseline? Because if your system is usually booting in 
> one minute, 100ms isn't too bad :)
>

Our system's boot time is about 2.2s (J7200) and that of J784s4 is 2.7s 
(owing to a larger DT).


> FYI, I believe we've been hit by this issue on Rockchip but cannot 
> find the thread or patch right now.
>
> For TPL and SPL, the Device Tree is parsed and looked for appropriate 
> bootph properties. Any node which doesn't have a bootph property and 
> doesn't have any children with a bootph property is removed from the 
> tree. However, the bootph property (if only present in a children 
> node) isn't propagated (meaning the node doesn't get the property). 
> This is done by fdtgrep.
>
> The issue is that for U-Boot proper pre-relocation, the whole DT is 
> taken and only nodes with the appropriate bootph property is probed 
> and children nodes do NOT count as opposed to the TPL/SPL case.
>
> My idea was that maybe we should rather propagate the property, at the 
> very least in U-Boot proper pre-relocation. This does mean we will 
> increase (by which amount?) the size of the DT in U-Boot proper 
> because we would add this property recursively up the tree from a node 
> that has the bootph property for U-Boot proper pre-relocation. This 
> **could** be an issue as the DT could be passed between stages and we 
> would then hit the size limit. Sadly, I didn't take the time to look 
> into adding support for that in fdtgrep nor will I have the time to do 
> it, so take this as me sharing my wish list with you.
>

Thanks for sharing this, the size increase this patch introduces for 48 
such bootph-* tags is about 1.5KB's, we can go ahead and bind the super 
parent to bypass the part of adding the tag, but for the next parent we 
will have to traverse again down the DT adding in unnecessary 
traversals(considering a case that we are 4-5 levels deep in the DT).

> I would really like to avoid having it guarded by a knob though 
> because I really think this is an issue that should simply be fixed 
> unconditionally for every platform with DT support.
>

Agreed. This fix should be for all platforms, the knob is for preventing 
boot time performance issues.


> Cheers,
> Quentin


Regards,
Moteen

Re: [RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Quentin Schulz]

Hi Moteen,

On 2/26/25 6:57 AM, Moteen Shah wrote:
> 
> On 17/02/25 20:32, Quentin Schulz wrote:
>> Hi Moteen,
>>
>> On 2/12/25 10:18 AM, Moteen Shah wrote:
>>> [You don't often get email from m-shah@ti.com. Learn why this is 
>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> In the U-Boot pre-relocation stage, if the parent node lacks bootph*
>>> property and the driver lacks a pre-reloc flag, all of its subsequent
>>> subnodes gets skipped over from driver binding—even if they have a
>>> bootph* property.
>>>
>>> This series addresses the issue by scanning through all the subnodes
>>> of the current node for the bootph* property and propagate it to all
>>> of its supernodes, ensuring that all of the applicable drivers are
>>> bound and probed prior to relocation. This series implements one of
>>> the solutions mentioned in [0].
>>>
>>> Since, all the nodes which are not having any bootph* property will
>>> also be traversed, we will have to incur some overheads in boot time,
>>> hence protecting the feature under a config.
>>>
>>> Boot time overheads:
>>>      Baseline: Upstream u-boot
>>>
>>>      Patch test: Baseline + remove all bootph-all properties from
>>>      *-u-boot.dtsi except the ones which are supposed to be probed
>>>      but have no bootph in any of its subnode.
>>>
>>>      J7200 delta from baseline  : ~100ms
>>>      J784S4 delta from baseline : ~350ms
>>>
>>
>> Pfew, that's a lot of time. Can you tell us what's the delta in 
>> percentage from baseline? Because if your system is usually booting in 
>> one minute, 100ms isn't too bad :)
>>
> 
> Our system's boot time is about 2.2s (J7200) and that of J784s4 is 2.7s 
> (owing to a larger DT).
> 

OK so respectively 4.5 and 12.9% boot time increase if I'm doing maths 
the right way, that's really a lot :/

> 
>> FYI, I believe we've been hit by this issue on Rockchip but cannot 
>> find the thread or patch right now.
>>
>> For TPL and SPL, the Device Tree is parsed and looked for appropriate 
>> bootph properties. Any node which doesn't have a bootph property and 
>> doesn't have any children with a bootph property is removed from the 
>> tree. However, the bootph property (if only present in a children 
>> node) isn't propagated (meaning the node doesn't get the property). 
>> This is done by fdtgrep.
>>
>> The issue is that for U-Boot proper pre-relocation, the whole DT is 
>> taken and only nodes with the appropriate bootph property is probed 
>> and children nodes do NOT count as opposed to the TPL/SPL case.
>>
>> My idea was that maybe we should rather propagate the property, at the 
>> very least in U-Boot proper pre-relocation. This does mean we will 
>> increase (by which amount?) the size of the DT in U-Boot proper 
>> because we would add this property recursively up the tree from a node 
>> that has the bootph property for U-Boot proper pre-relocation. This 
>> **could** be an issue as the DT could be passed between stages and we 
>> would then hit the size limit. Sadly, I didn't take the time to look 
>> into adding support for that in fdtgrep nor will I have the time to do 
>> it, so take this as me sharing my wish list with you.
>>
> 
> Thanks for sharing this, the size increase this patch introduces for 48 
> such bootph-* tags is about 1.5KB's, we can go ahead and bind the super 
> parent to bypass the part of adding the tag, but for the next parent we 
> will have to traverse again down the DT adding in unnecessary 
> traversals(considering a case that we are 4-5 levels deep in the DT).
> 

j784s4-evm/u-boot.dtb is 131616B
j7200-evm/u-boot.dtb is 88368B

so 1.5KiB would mean respectively, 1.1 and 1.7% in **DTB** **size** 
increase, not sure how that translate in terms of boot time though.

Reading Tom's notes from the meeting a few days ago where this was 
seemingly discussed, I believe the issue is that the kernel wants the 
bootph- property only in the child node. But I assume this applies only 
to the DTS, which would be fine with this build time propagation of the 
bootph- properties to node parents recursively. Would the kernel also 
want the same limitation for the DTB?

Cheers,
Quentin

Re: [RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Simon Glass]

Hi Quentin,

On Wed, 26 Feb 2025 at 03:53, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Moteen,
>
> On 2/26/25 6:57 AM, Moteen Shah wrote:
> >
> > On 17/02/25 20:32, Quentin Schulz wrote:
> >> Hi Moteen,
> >>
> >> On 2/12/25 10:18 AM, Moteen Shah wrote:
> >>> [You don't often get email from m-shah@ti.com. Learn why this is
> >>> important at https://aka.ms/LearnAboutSenderIdentification ]
> >>>
> >>> In the U-Boot pre-relocation stage, if the parent node lacks bootph*
> >>> property and the driver lacks a pre-reloc flag, all of its subsequent
> >>> subnodes gets skipped over from driver binding—even if they have a
> >>> bootph* property.
> >>>
> >>> This series addresses the issue by scanning through all the subnodes
> >>> of the current node for the bootph* property and propagate it to all
> >>> of its supernodes, ensuring that all of the applicable drivers are
> >>> bound and probed prior to relocation. This series implements one of
> >>> the solutions mentioned in [0].
> >>>
> >>> Since, all the nodes which are not having any bootph* property will
> >>> also be traversed, we will have to incur some overheads in boot time,
> >>> hence protecting the feature under a config.
> >>>
> >>> Boot time overheads:
> >>>      Baseline: Upstream u-boot
> >>>
> >>>      Patch test: Baseline + remove all bootph-all properties from
> >>>      *-u-boot.dtsi except the ones which are supposed to be probed
> >>>      but have no bootph in any of its subnode.
> >>>
> >>>      J7200 delta from baseline  : ~100ms
> >>>      J784S4 delta from baseline : ~350ms
> >>>
> >>
> >> Pfew, that's a lot of time. Can you tell us what's the delta in
> >> percentage from baseline? Because if your system is usually booting in
> >> one minute, 100ms isn't too bad :)
> >>
> >
> > Our system's boot time is about 2.2s (J7200) and that of J784s4 is 2.7s
> > (owing to a larger DT).
> >
>
> OK so respectively 4.5 and 12.9% boot time increase if I'm doing maths
> the right way, that's really a lot :/
>
> >
> >> FYI, I believe we've been hit by this issue on Rockchip but cannot
> >> find the thread or patch right now.
> >>
> >> For TPL and SPL, the Device Tree is parsed and looked for appropriate
> >> bootph properties. Any node which doesn't have a bootph property and
> >> doesn't have any children with a bootph property is removed from the
> >> tree. However, the bootph property (if only present in a children
> >> node) isn't propagated (meaning the node doesn't get the property).
> >> This is done by fdtgrep.
> >>
> >> The issue is that for U-Boot proper pre-relocation, the whole DT is
> >> taken and only nodes with the appropriate bootph property is probed
> >> and children nodes do NOT count as opposed to the TPL/SPL case.
> >>
> >> My idea was that maybe we should rather propagate the property, at the
> >> very least in U-Boot proper pre-relocation. This does mean we will
> >> increase (by which amount?) the size of the DT in U-Boot proper
> >> because we would add this property recursively up the tree from a node
> >> that has the bootph property for U-Boot proper pre-relocation. This
> >> **could** be an issue as the DT could be passed between stages and we
> >> would then hit the size limit. Sadly, I didn't take the time to look
> >> into adding support for that in fdtgrep nor will I have the time to do
> >> it, so take this as me sharing my wish list with you.
> >>
> >
> > Thanks for sharing this, the size increase this patch introduces for 48
> > such bootph-* tags is about 1.5KB's, we can go ahead and bind the super
> > parent to bypass the part of adding the tag, but for the next parent we
> > will have to traverse again down the DT adding in unnecessary
> > traversals(considering a case that we are 4-5 levels deep in the DT).
> >
>
> j784s4-evm/u-boot.dtb is 131616B
> j7200-evm/u-boot.dtb is 88368B
>
> so 1.5KiB would mean respectively, 1.1 and 1.7% in **DTB** **size**
> increase, not sure how that translate in terms of boot time though.
>
> Reading Tom's notes from the meeting a few days ago where this was
> seemingly discussed, I believe the issue is that the kernel wants the
> bootph- property only in the child node. But I assume this applies only
> to the DTS, which would be fine with this build time propagation of the
> bootph- properties to node parents recursively. Would the kernel also
> want the same limitation for the DTB?

It doesn't matter to the kernel, since there is no restriction as to
which nodes such a property can be added.

I like the binman route as it lends itself to further optimisations in
the future. For example, we might provide an ordered list of node
offsets to process before relocation.

But we could also have an algorithm which maintains a small list of
node-offsets which have been visited and have the required tag, so
avoid constant re-traversal.

Ideally, pre-relocation, we should not need a lot of drivers, since
SPL has done much of the early-init work already. Perhaps a UART and
not even any pinctrl / clocks / power?

Regards,
Simon

Re: [RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Quentin Schulz]

Hi Simon,

On 2/27/25 5:24 PM, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 26 Feb 2025 at 03:53, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Moteen,
>>
>> On 2/26/25 6:57 AM, Moteen Shah wrote:
>>>
>>> On 17/02/25 20:32, Quentin Schulz wrote:
>>>> Hi Moteen,
>>>>
>>>> On 2/12/25 10:18 AM, Moteen Shah wrote:
>>>>> [You don't often get email from m-shah@ti.com. Learn why this is
>>>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> In the U-Boot pre-relocation stage, if the parent node lacks bootph*
>>>>> property and the driver lacks a pre-reloc flag, all of its subsequent
>>>>> subnodes gets skipped over from driver binding—even if they have a
>>>>> bootph* property.
>>>>>
>>>>> This series addresses the issue by scanning through all the subnodes
>>>>> of the current node for the bootph* property and propagate it to all
>>>>> of its supernodes, ensuring that all of the applicable drivers are
>>>>> bound and probed prior to relocation. This series implements one of
>>>>> the solutions mentioned in [0].
>>>>>
>>>>> Since, all the nodes which are not having any bootph* property will
>>>>> also be traversed, we will have to incur some overheads in boot time,
>>>>> hence protecting the feature under a config.
>>>>>
>>>>> Boot time overheads:
>>>>>       Baseline: Upstream u-boot
>>>>>
>>>>>       Patch test: Baseline + remove all bootph-all properties from
>>>>>       *-u-boot.dtsi except the ones which are supposed to be probed
>>>>>       but have no bootph in any of its subnode.
>>>>>
>>>>>       J7200 delta from baseline  : ~100ms
>>>>>       J784S4 delta from baseline : ~350ms
>>>>>
>>>>
>>>> Pfew, that's a lot of time. Can you tell us what's the delta in
>>>> percentage from baseline? Because if your system is usually booting in
>>>> one minute, 100ms isn't too bad :)
>>>>
>>>
>>> Our system's boot time is about 2.2s (J7200) and that of J784s4 is 2.7s
>>> (owing to a larger DT).
>>>
>>
>> OK so respectively 4.5 and 12.9% boot time increase if I'm doing maths
>> the right way, that's really a lot :/
>>
>>>
>>>> FYI, I believe we've been hit by this issue on Rockchip but cannot
>>>> find the thread or patch right now.
>>>>
>>>> For TPL and SPL, the Device Tree is parsed and looked for appropriate
>>>> bootph properties. Any node which doesn't have a bootph property and
>>>> doesn't have any children with a bootph property is removed from the
>>>> tree. However, the bootph property (if only present in a children
>>>> node) isn't propagated (meaning the node doesn't get the property).
>>>> This is done by fdtgrep.
>>>>
>>>> The issue is that for U-Boot proper pre-relocation, the whole DT is
>>>> taken and only nodes with the appropriate bootph property is probed
>>>> and children nodes do NOT count as opposed to the TPL/SPL case.
>>>>
>>>> My idea was that maybe we should rather propagate the property, at the
>>>> very least in U-Boot proper pre-relocation. This does mean we will
>>>> increase (by which amount?) the size of the DT in U-Boot proper
>>>> because we would add this property recursively up the tree from a node
>>>> that has the bootph property for U-Boot proper pre-relocation. This
>>>> **could** be an issue as the DT could be passed between stages and we
>>>> would then hit the size limit. Sadly, I didn't take the time to look
>>>> into adding support for that in fdtgrep nor will I have the time to do
>>>> it, so take this as me sharing my wish list with you.
>>>>
>>>
>>> Thanks for sharing this, the size increase this patch introduces for 48
>>> such bootph-* tags is about 1.5KB's, we can go ahead and bind the super
>>> parent to bypass the part of adding the tag, but for the next parent we
>>> will have to traverse again down the DT adding in unnecessary
>>> traversals(considering a case that we are 4-5 levels deep in the DT).
>>>
>>
>> j784s4-evm/u-boot.dtb is 131616B
>> j7200-evm/u-boot.dtb is 88368B
>>
>> so 1.5KiB would mean respectively, 1.1 and 1.7% in **DTB** **size**
>> increase, not sure how that translate in terms of boot time though.
>>
>> Reading Tom's notes from the meeting a few days ago where this was
>> seemingly discussed, I believe the issue is that the kernel wants the
>> bootph- property only in the child node. But I assume this applies only
>> to the DTS, which would be fine with this build time propagation of the
>> bootph- properties to node parents recursively. Would the kernel also
>> want the same limitation for the DTB?
> 
> It doesn't matter to the kernel, since there is no restriction as to
> which nodes such a property can be added.
> 
> I like the binman route as it lends itself to further optimisations in
> the future. For example, we might provide an ordered list of node
> offsets to process before relocation.
> 

We already have bootph- properties for pre-reloc, why would we need an 
ordered list of node offsets to process before relocation? What's the 
idea here?

> But we could also have an algorithm which maintains a small list of
> node-offsets which have been visited and have the required tag, so
> avoid constant re-traversal.
> 

Why can't we use the same behavior we have for bootph- properties for 
TPL/SPL in pre-reloc, why do we have to have a completely different 
implementation there?

In the worst case, we could have a separate DTB for pre-reloc too, 
stripped down the same way it's done for TPL and SPL for example.

> Ideally, pre-relocation, we should not need a lot of drivers, since
> SPL has done much of the early-init work already. Perhaps a UART and
> not even any pinctrl / clocks / power?
> 

I can tell you I need the storage medium used by SPL to load proper to 
be in the pre-reloc phase of proper for everything to work properly, so 
"just UART" is not good enough for me.

So, SPI flash, SPI controller, eMMC controller, SD controller, pinconf, 
pinmux, clocks, ... c.f. 100f489f58a6 ("rockchip: rk3399: Fix loading 
FIT from SD-card when booting from eMMC") (yes the commit is about 
fixing FIT loading, but see the end of the commit log).

I don't think it makes sense to have an automagic solution that decides 
which nodes should appear or not. We already have bootph- properties for 
that.

I feel like I'm missing something from the big picture, can someone tell 
me what it is :) ?

Cheers,
Quentin

Re: [RFC PATCH 0/2 v1] Propagate bootph* property to all parent nodes

[Simon Glass]

Hi Quentin,

On Fri, 28 Feb 2025 at 04:04, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Simon,
>
> On 2/27/25 5:24 PM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 26 Feb 2025 at 03:53, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Moteen,
> >>
> >> On 2/26/25 6:57 AM, Moteen Shah wrote:
> >>>
> >>> On 17/02/25 20:32, Quentin Schulz wrote:
> >>>> Hi Moteen,
> >>>>
> >>>> On 2/12/25 10:18 AM, Moteen Shah wrote:
> >>>>> [You don't often get email from m-shah@ti.com. Learn why this is
> >>>>> important at https://aka.ms/LearnAboutSenderIdentification ]
> >>>>>
> >>>>> In the U-Boot pre-relocation stage, if the parent node lacks bootph*
> >>>>> property and the driver lacks a pre-reloc flag, all of its subsequent
> >>>>> subnodes gets skipped over from driver binding—even if they have a
> >>>>> bootph* property.
> >>>>>
> >>>>> This series addresses the issue by scanning through all the subnodes
> >>>>> of the current node for the bootph* property and propagate it to all
> >>>>> of its supernodes, ensuring that all of the applicable drivers are
> >>>>> bound and probed prior to relocation. This series implements one of
> >>>>> the solutions mentioned in [0].
> >>>>>
> >>>>> Since, all the nodes which are not having any bootph* property will
> >>>>> also be traversed, we will have to incur some overheads in boot time,
> >>>>> hence protecting the feature under a config.
> >>>>>
> >>>>> Boot time overheads:
> >>>>>       Baseline: Upstream u-boot
> >>>>>
> >>>>>       Patch test: Baseline + remove all bootph-all properties from
> >>>>>       *-u-boot.dtsi except the ones which are supposed to be probed
> >>>>>       but have no bootph in any of its subnode.
> >>>>>
> >>>>>       J7200 delta from baseline  : ~100ms
> >>>>>       J784S4 delta from baseline : ~350ms
> >>>>>
> >>>>
> >>>> Pfew, that's a lot of time. Can you tell us what's the delta in
> >>>> percentage from baseline? Because if your system is usually booting in
> >>>> one minute, 100ms isn't too bad :)
> >>>>
> >>>
> >>> Our system's boot time is about 2.2s (J7200) and that of J784s4 is 2.7s
> >>> (owing to a larger DT).
> >>>
> >>
> >> OK so respectively 4.5 and 12.9% boot time increase if I'm doing maths
> >> the right way, that's really a lot :/
> >>
> >>>
> >>>> FYI, I believe we've been hit by this issue on Rockchip but cannot
> >>>> find the thread or patch right now.
> >>>>
> >>>> For TPL and SPL, the Device Tree is parsed and looked for appropriate
> >>>> bootph properties. Any node which doesn't have a bootph property and
> >>>> doesn't have any children with a bootph property is removed from the
> >>>> tree. However, the bootph property (if only present in a children
> >>>> node) isn't propagated (meaning the node doesn't get the property).
> >>>> This is done by fdtgrep.
> >>>>
> >>>> The issue is that for U-Boot proper pre-relocation, the whole DT is
> >>>> taken and only nodes with the appropriate bootph property is probed
> >>>> and children nodes do NOT count as opposed to the TPL/SPL case.
> >>>>
> >>>> My idea was that maybe we should rather propagate the property, at the
> >>>> very least in U-Boot proper pre-relocation. This does mean we will
> >>>> increase (by which amount?) the size of the DT in U-Boot proper
> >>>> because we would add this property recursively up the tree from a node
> >>>> that has the bootph property for U-Boot proper pre-relocation. This
> >>>> **could** be an issue as the DT could be passed between stages and we
> >>>> would then hit the size limit. Sadly, I didn't take the time to look
> >>>> into adding support for that in fdtgrep nor will I have the time to do
> >>>> it, so take this as me sharing my wish list with you.
> >>>>
> >>>
> >>> Thanks for sharing this, the size increase this patch introduces for 48
> >>> such bootph-* tags is about 1.5KB's, we can go ahead and bind the super
> >>> parent to bypass the part of adding the tag, but for the next parent we
> >>> will have to traverse again down the DT adding in unnecessary
> >>> traversals(considering a case that we are 4-5 levels deep in the DT).
> >>>
> >>
> >> j784s4-evm/u-boot.dtb is 131616B
> >> j7200-evm/u-boot.dtb is 88368B
> >>
> >> so 1.5KiB would mean respectively, 1.1 and 1.7% in **DTB** **size**
> >> increase, not sure how that translate in terms of boot time though.
> >>
> >> Reading Tom's notes from the meeting a few days ago where this was
> >> seemingly discussed, I believe the issue is that the kernel wants the
> >> bootph- property only in the child node. But I assume this applies only
> >> to the DTS, which would be fine with this build time propagation of the
> >> bootph- properties to node parents recursively. Would the kernel also
> >> want the same limitation for the DTB?
> >
> > It doesn't matter to the kernel, since there is no restriction as to
> > which nodes such a property can be added.
> >
> > I like the binman route as it lends itself to further optimisations in
> > the future. For example, we might provide an ordered list of node
> > offsets to process before relocation.
> >
>
> We already have bootph- properties for pre-reloc, why would we need an
> ordered list of node offsets to process before relocation? What's the
> idea here?

Just to save execution time.

>
> > But we could also have an algorithm which maintains a small list of
> > node-offsets which have been visited and have the required tag, so
> > avoid constant re-traversal.
> >
>
> Why can't we use the same behavior we have for bootph- properties for
> TPL/SPL in pre-reloc, why do we have to have a completely different
> implementation there?
>
> In the worst case, we could have a separate DTB for pre-reloc too,
> stripped down the same way it's done for TPL and SPL for example.

The reason we (already) have a completely separate implementation is
that the unwanted nodes simply aren't present in xPL. So there is no
need to check for tags.

>
> > Ideally, pre-relocation, we should not need a lot of drivers, since
> > SPL has done much of the early-init work already. Perhaps a UART and
> > not even any pinctrl / clocks / power?
> >
>
> I can tell you I need the storage medium used by SPL to load proper to
> be in the pre-reloc phase of proper for everything to work properly, so
> "just UART" is not good enough for me.
>
> So, SPI flash, SPI controller, eMMC controller, SD controller, pinconf,
> pinmux, clocks, ... c.f. 100f489f58a6 ("rockchip: rk3399: Fix loading
> FIT from SD-card when booting from eMMC") (yes the commit is about
> fixing FIT loading, but see the end of the commit log).

In principle I don't see why. If there is a full-featured SPL, then
U-Boot really doesn't need to do much in its pre-relocation phase.

One of the challenges is that we can't remove clock/power/etc. because
drivers expect them. For example the UART will use the clock to set
the baud rate. But we can check for -ENOSYS and assume that we don't
need to do it.

>
> I don't think it makes sense to have an automagic solution that decides
> which nodes should appear or not. We already have bootph- properties for
> that.

Are you referring to my 'ordered list of nodes'? It's just that we can
do a little pre-processing and save time. It would need to be part of
the DT though, so it's a bit tricky....

>
> I feel like I'm missing something from the big picture, can someone tell
> me what it is :) ?

We're just trying to reduce the time taken by board_init_f(), which
seems too high on some platforms.

Regards,
Simon