On 20/01/24 22:11, Tom Rini wrote:

On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote:

...

Hello Tom,

On 12/01/24 18:56, Tom Rini wrote:

...

...

...

The list of conditionals in common/spl/spl.c::board_init_r() should be updated and probably use SPL_NET as the option to check for.

Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I assume that you are referring to the following change:

    if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) ||

•       IS_ENABLED(CONFIG_SPL_ATF))
  

•       IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET))
            dram_init_banksize();
  
  

I shall replace the current patch with the above change in the v2 series. Since this is in the common section, is there a generic reason I could provide in the commit message rather than the existing commit message which seems to be board specific? Also, I hope that the above change will not cause regressions for other non-TI devices. Please let me know.

Yes, that's the area, and just note that networking also requires the DDR to be initialized.

Thank you for confirming and providing your suggestion for the contents of the commit message.

On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote:

On 22/01/24 10:11, Siddharth Vadapalli wrote:

...

On 20/01/24 22:11, Tom Rini wrote:

...

On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote:

...

Hello Tom,

On 12/01/24 18:56, Tom Rini wrote:

...

...

...

...

The list of conditionals in common/spl/spl.c::board_init_r() should be updated and probably use SPL_NET as the option to check for.

Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I assume that you are referring to the following change:

     if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) ||

•       IS_ENABLED(CONFIG_SPL_ATF))
  

•       IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET))
             dram_init_banksize();
  
  

I shall replace the current patch with the above change in the v2 series. Since this is in the common section, is there a generic reason I could provide in the commit message rather than the existing commit message which seems to be board specific? Also, I hope that the above change will not cause regressions for other non-TI devices. Please let me know.

Yes, that's the area, and just note that networking also requires the DDR to be initialized.

Thank you for confirming and providing your suggestion for the contents of the commit message.

Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c "dram_init_banksize()", the issue of fetching a file at SPL stage seemed to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top for the very first time in "spl_enable_cache()" results in "arch_lmb_reserve()" function reserving memory region from Stack pointer at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" function call that invokes "arch_lmb_reserve()" function, which reserves entire memory starting from Stack Pointer to gd->ram_top leaving no space to load U-Boot image via TFTP since TFTP loads files at pre configured memory address at "0x82000000".

As a workaround for this issue, one solution we can propose is to disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For that we can define a new config option for LMB reserve checks as "SPL_LMB". This config will be enable by default for the backword compatibility and disable for our use case at SPL and U-Boot stage.

The problem here is that we need LMB for booting an OS, which is something we'll want in SPL in non-cortex-R cases too, which means this platform, so that's a no-go. I think you need to dig harder and see if you can correct the logic somewhere so that we don't over reserve?

On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote:

On 12/04/24 03:37, Tom Rini wrote:

...

On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote:

...

On 22/01/24 10:11, Siddharth Vadapalli wrote:

...

On 20/01/24 22:11, Tom Rini wrote:

...

On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote:

...

Hello Tom,

On 12/01/24 18:56, Tom Rini wrote:

...

...

...

> The list of conditionals in common/spl/spl.c::board_init_r() should be > updated and probably use SPL_NET as the option to check for.

Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I assume that you are referring to the following change:

      if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) ||

•       IS_ENABLED(CONFIG_SPL_ATF))
  

•       IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET))
              dram_init_banksize();
  
  

I shall replace the current patch with the above change in the v2 series. Since this is in the common section, is there a generic reason I could provide in the commit message rather than the existing commit message which seems to be board specific? Also, I hope that the above change will not cause regressions for other non-TI devices. Please let me know.

Yes, that's the area, and just note that networking also requires the DDR to be initialized.

Thank you for confirming and providing your suggestion for the contents of the commit message.

Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c "dram_init_banksize()", the issue of fetching a file at SPL stage seemed to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top for the very first time in "spl_enable_cache()" results in "arch_lmb_reserve()" function reserving memory region from Stack pointer at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" function call that invokes "arch_lmb_reserve()" function, which reserves entire memory starting from Stack Pointer to gd->ram_top leaving no space to load U-Boot image via TFTP since TFTP loads files at pre configured memory address at "0x82000000".

As a workaround for this issue, one solution we can propose is to disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For that we can define a new config option for LMB reserve checks as "SPL_LMB". This config will be enable by default for the backword compatibility and disable for our use case at SPL and U-Boot stage.

The problem here is that we need LMB for booting an OS, which is something we'll want in SPL in non-cortex-R cases too, which means this platform, so that's a no-go. I think you need to dig harder and see if you can correct the logic somewhere so that we don't over reserve?

Since this issue is due to function call "lmb_init_and_reserve()" function invoked from "tftp_init_load_addr()" function. This function is defined by Simon in commit "a156c47e39ad", which fixes "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you explain why do we need to call "lmb_init_and_reserve()" function here ?

This is indeed a tricky area which is why Sughosh is looking in to trying to re-work the LMB mechanic and we've had a few long threads about it as well.

I've honestly forgotten the use case you have here, can you please remind us?

hi Chintan,

On Wed, 17 Apr 2024 at 13:21, Chintan Vankar c-vankar@ti.com wrote:

On 16/04/24 22:30, Tom Rini wrote:

...

On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote:

...

On 12/04/24 03:37, Tom Rini wrote:

...

On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote:

...

On 22/01/24 10:11, Siddharth Vadapalli wrote:

...

On 20/01/24 22:11, Tom Rini wrote: > On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: >> Hello Tom, >> >> On 12/01/24 18:56, Tom Rini wrote:

...

>>> The list of conditionals in common/spl/spl.c::board_init_r() should be >>> updated and probably use SPL_NET as the option to check for. >> >> Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I >> assume that you are referring to the following change: >> >> if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) || >> - IS_ENABLED(CONFIG_SPL_ATF)) >> + IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET)) >> dram_init_banksize(); >> >> I shall replace the current patch with the above change in the v2 series. Since >> this is in the common section, is there a generic reason I could provide in the >> commit message rather than the existing commit message which seems to be board >> specific? Also, I hope that the above change will not cause regressions for >> other non-TI devices. Please let me know. > > Yes, that's the area, and just note that networking also requires the > DDR to be initialized. >

Thank you for confirming and providing your suggestion for the contents of the commit message.

Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c "dram_init_banksize()", the issue of fetching a file at SPL stage seemed to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top for the very first time in "spl_enable_cache()" results in "arch_lmb_reserve()" function reserving memory region from Stack pointer at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" function call that invokes "arch_lmb_reserve()" function, which reserves entire memory starting from Stack Pointer to gd->ram_top leaving no space to load U-Boot image via TFTP since TFTP loads files at pre configured memory address at "0x82000000".

As a workaround for this issue, one solution we can propose is to disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For that we can define a new config option for LMB reserve checks as "SPL_LMB". This config will be enable by default for the backword compatibility and disable for our use case at SPL and U-Boot stage.

The problem here is that we need LMB for booting an OS, which is something we'll want in SPL in non-cortex-R cases too, which means this platform, so that's a no-go. I think you need to dig harder and see if you can correct the logic somewhere so that we don't over reserve?

Since this issue is due to function call "lmb_init_and_reserve()" function invoked from "tftp_init_load_addr()" function. This function is defined by Simon in commit "a156c47e39ad", which fixes "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you explain why do we need to call "lmb_init_and_reserve()" function here ?

This is indeed a tricky area which is why Sughosh is looking in to trying to re-work the LMB mechanic and we've had a few long threads about it as well.

I've honestly forgotten the use case you have here, can you please remind us?

We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP. To store the file we need a free memory in RAM, specifically we are storing these files at 0x82000000. But we are facing an issue while loading the file since the memory area having an address 0x82000000 is reserved due to "lmb_init_and_reserve()" function call. This function is called in "tftp_init_load_addr()" function which is getting called exactly before we are trying to get the free memory area by calling "lmb_get_free_size()".

I have no idea about your platform but I was wondering if there is any particular importance of the load address of 0x82000000? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is getting reserved for the U-Boot image. Do you not have the option of loading the image at a lower address instead?

-sughosh

On 17/04/24 21:34, Tom Rini wrote:

On Wed, Apr 17, 2024 at 05:48:31PM +0530, Sughosh Ganu wrote:

...

hi Chintan,

On Wed, 17 Apr 2024 at 13:21, Chintan Vankar c-vankar@ti.com wrote:

...

On 16/04/24 22:30, Tom Rini wrote:

...

On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote:

...

On 12/04/24 03:37, Tom Rini wrote:

...

On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote: > > > On 22/01/24 10:11, Siddharth Vadapalli wrote: >> >> >> On 20/01/24 22:11, Tom Rini wrote: >>> On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: >>>> Hello Tom, >>>> >>>> On 12/01/24 18:56, Tom Rini wrote: >> >> ... >> >>>>> The list of conditionals in common/spl/spl.c::board_init_r() should be >>>>> updated and probably use SPL_NET as the option to check for. >>>> >>>> Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I >>>> assume that you are referring to the following change: >>>> >>>> if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) || >>>> - IS_ENABLED(CONFIG_SPL_ATF)) >>>> + IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET)) >>>> dram_init_banksize(); >>>> >>>> I shall replace the current patch with the above change in the v2 series. Since >>>> this is in the common section, is there a generic reason I could provide in the >>>> commit message rather than the existing commit message which seems to be board >>>> specific? Also, I hope that the above change will not cause regressions for >>>> other non-TI devices. Please let me know. >>> >>> Yes, that's the area, and just note that networking also requires the >>> DDR to be initialized. >>> >> >> Thank you for confirming and providing your suggestion for the contents of the >> commit message. >> > Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c > "dram_init_banksize()", the issue of fetching a file at SPL stage seemed > to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top > for the very first time in "spl_enable_cache()" results in > "arch_lmb_reserve()" function reserving memory region from Stack pointer > at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously > when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP > to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" > function call that invokes "arch_lmb_reserve()" function, which reserves > entire memory starting from Stack Pointer to gd->ram_top leaving no > space to load U-Boot image via TFTP since TFTP loads files at pre > configured memory address at "0x82000000". > > As a workaround for this issue, one solution we can propose is to > disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For > that we can define a new config option for LMB reserve checks as > "SPL_LMB". This config will be enable by default for the backword > compatibility and disable for our use case at SPL and U-Boot stage.

The problem here is that we need LMB for booting an OS, which is something we'll want in SPL in non-cortex-R cases too, which means this platform, so that's a no-go. I think you need to dig harder and see if you can correct the logic somewhere so that we don't over reserve?

Since this issue is due to function call "lmb_init_and_reserve()" function invoked from "tftp_init_load_addr()" function. This function is defined by Simon in commit "a156c47e39ad", which fixes "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you explain why do we need to call "lmb_init_and_reserve()" function here ?

This is indeed a tricky area which is why Sughosh is looking in to trying to re-work the LMB mechanic and we've had a few long threads about it as well.

I've honestly forgotten the use case you have here, can you please remind us?

We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP. To store the file we need a free memory in RAM, specifically we are storing these files at 0x82000000. But we are facing an issue while loading the file since the memory area having an address 0x82000000 is reserved due to "lmb_init_and_reserve()" function call. This function is called in "tftp_init_load_addr()" function which is getting called exactly before we are trying to get the free memory area by calling "lmb_get_free_size()".

I have no idea about your platform but I was wondering if there is any particular importance of the load address of 0x82000000? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is getting reserved for the U-Boot image. Do you not have the option of loading the image at a lower address instead?

Sughosh,

I think my explanation was not clear at: "We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP." - In Ethernet Booting we are fetching U-Boot image at SPL stage via TFTP at specified address 0x82000000. While loading U-Boot image we are getting TFTP error, since address from stack pointer till gd->ram_top is reserved due to "lmb_init_and_reserve()" function call. I want to know for which purpose this address range is reserved.

Or using a higher address for SPL stack? You might be able to solve this just by re-examining which addresses (and RAM size limitations) need to be considered here.

Tom,

We tried this approach of assigning a higher address for SPL stack, but it is not working as expected.

On 18/04/24 17:30, Sughosh Ganu wrote:

On Thu, 18 Apr 2024 at 16:08, Chintan Vankar c-vankar@ti.com wrote:

...

On 17/04/24 21:34, Tom Rini wrote:

...

On Wed, Apr 17, 2024 at 05:48:31PM +0530, Sughosh Ganu wrote:

...

hi Chintan,

On Wed, 17 Apr 2024 at 13:21, Chintan Vankar c-vankar@ti.com wrote:

...

On 16/04/24 22:30, Tom Rini wrote:

...

On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote: > > > On 12/04/24 03:37, Tom Rini wrote: >> On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote: >>> >>> >>> On 22/01/24 10:11, Siddharth Vadapalli wrote: >>>> >>>> >>>> On 20/01/24 22:11, Tom Rini wrote: >>>>> On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: >>>>>> Hello Tom, >>>>>> >>>>>> On 12/01/24 18:56, Tom Rini wrote: >>>> >>>> ... >>>> >>>>>>> The list of conditionals in common/spl/spl.c::board_init_r() should be >>>>>>> updated and probably use SPL_NET as the option to check for. >>>>>> >>>>>> Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I >>>>>> assume that you are referring to the following change: >>>>>> >>>>>> if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) || >>>>>> - IS_ENABLED(CONFIG_SPL_ATF)) >>>>>> + IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET)) >>>>>> dram_init_banksize(); >>>>>> >>>>>> I shall replace the current patch with the above change in the v2 series. Since >>>>>> this is in the common section, is there a generic reason I could provide in the >>>>>> commit message rather than the existing commit message which seems to be board >>>>>> specific? Also, I hope that the above change will not cause regressions for >>>>>> other non-TI devices. Please let me know. >>>>> >>>>> Yes, that's the area, and just note that networking also requires the >>>>> DDR to be initialized. >>>>> >>>> >>>> Thank you for confirming and providing your suggestion for the contents of the >>>> commit message. >>>> >>> Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c >>> "dram_init_banksize()", the issue of fetching a file at SPL stage seemed >>> to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top >>> for the very first time in "spl_enable_cache()" results in >>> "arch_lmb_reserve()" function reserving memory region from Stack pointer >>> at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously >>> when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP >>> to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" >>> function call that invokes "arch_lmb_reserve()" function, which reserves >>> entire memory starting from Stack Pointer to gd->ram_top leaving no >>> space to load U-Boot image via TFTP since TFTP loads files at pre >>> configured memory address at "0x82000000". >>> >>> As a workaround for this issue, one solution we can propose is to >>> disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For >>> that we can define a new config option for LMB reserve checks as >>> "SPL_LMB". This config will be enable by default for the backword >>> compatibility and disable for our use case at SPL and U-Boot stage. >> >> The problem here is that we need LMB for booting an OS, which is >> something we'll want in SPL in non-cortex-R cases too, which means this >> platform, so that's a no-go. I think you need to dig harder and see if >> you can correct the logic somewhere so that we don't over reserve? >> > Since this issue is due to function call "lmb_init_and_reserve()" > function invoked from "tftp_init_load_addr()" function. This function > is defined by Simon in commit "a156c47e39ad", which fixes > "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you > explain why do we need to call "lmb_init_and_reserve()" function here ?

This is indeed a tricky area which is why Sughosh is looking in to trying to re-work the LMB mechanic and we've had a few long threads about it as well.

I've honestly forgotten the use case you have here, can you please remind us?

We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP. To store the file we need a free memory in RAM, specifically we are storing these files at 0x82000000. But we are facing an issue while loading the file since the memory area having an address 0x82000000 is reserved due to "lmb_init_and_reserve()" function call. This function is called in "tftp_init_load_addr()" function which is getting called exactly before we are trying to get the free memory area by calling "lmb_get_free_size()".

I have no idea about your platform but I was wondering if there is any particular importance of the load address of 0x82000000? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is getting reserved for the U-Boot image. Do you not have the option of loading the image at a lower address instead?

Sughosh,

I think my explanation was not clear at: "We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP."

• In Ethernet Booting we are fetching U-Boot image at SPL stage via

TFTP at specified address 0x82000000. While loading U-Boot image we are getting TFTP error, since address from stack pointer till gd->ram_top is reserved due to "lmb_init_and_reserve()" function call. I want to know for which purpose this address range is reserved.

On relocation, the U-Boot image is located typically at the top of the DRAM memory used by U-Boot(ram_top). That region of memory is reserved to ensure that the memory occupied by the U-Boot image does not get overwritten by a LMB reservation.

Yes, you are correct about U-Boot relocation but we are facing an issue at the time of fetching U-Boot proper at SPL stage.

Btw, are you facing this issue in SPL, or U-Boot proper? I built the images for the am62x_evm_a53 config, and I don't see the

We are getting "TFTP error" at runtime while fetching U-Boot proper at SPL stage while booting via "Ethernet", and we are using "am62x_evm_a53_ethboot_defconfig" instead of "am62x_evm_a53_defconfig".

These are the extra configs we are using on top of "am62x_evm_a53_defconfig":

CONFIG_SPL_DRIVERS_MISC=y CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_DMA=y CONFIG_SPL_ENV_SUPPORT=y CONFIG_SPL_ETH=y CONFIG_SPL_NET=y CONFIG_SPL_NET_VCI_STRING="AM62X U-Boot A53 SPL" CONFIG_SPL_SYSCON=y

arch_lmb_reserve() function getting included in the SPL image -- both the .text.arch_lmb_reserve and .text.arch_lmb_reserve_generic are part of discarded sections. So I am wondering how you are observing this behaviour in SPL.

-sughosh

...

...

Or using a higher address for SPL stack? You might be able to solve this just by re-examining which addresses (and RAM size limitations) need to be considered here.

Tom,

We tried this approach of assigning a higher address for SPL stack, but it is not working as expected.

On 19/04/24 17:04, Sughosh Ganu wrote:

On Fri, 19 Apr 2024 at 16:04, Chintan Vankar c-vankar@ti.com wrote:

...

On 18/04/24 17:30, Sughosh Ganu wrote:

...

On Thu, 18 Apr 2024 at 16:08, Chintan Vankar c-vankar@ti.com wrote:

...

On 17/04/24 21:34, Tom Rini wrote:

...

On Wed, Apr 17, 2024 at 05:48:31PM +0530, Sughosh Ganu wrote:

...

hi Chintan,

On Wed, 17 Apr 2024 at 13:21, Chintan Vankar c-vankar@ti.com wrote: > > > > On 16/04/24 22:30, Tom Rini wrote: >> On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote: >>> >>> >>> On 12/04/24 03:37, Tom Rini wrote: >>>> On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote: >>>>> >>>>> >>>>> On 22/01/24 10:11, Siddharth Vadapalli wrote: >>>>>> >>>>>> >>>>>> On 20/01/24 22:11, Tom Rini wrote: >>>>>>> On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: >>>>>>>> Hello Tom, >>>>>>>> >>>>>>>> On 12/01/24 18:56, Tom Rini wrote: >>>>>> >>>>>> ... >>>>>> >>>>>>>>> The list of conditionals in common/spl/spl.c::board_init_r() should be >>>>>>>>> updated and probably use SPL_NET as the option to check for. >>>>>>>> >>>>>>>> Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I >>>>>>>> assume that you are referring to the following change: >>>>>>>> >>>>>>>> if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) || >>>>>>>> - IS_ENABLED(CONFIG_SPL_ATF)) >>>>>>>> + IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET)) >>>>>>>> dram_init_banksize(); >>>>>>>> >>>>>>>> I shall replace the current patch with the above change in the v2 series. Since >>>>>>>> this is in the common section, is there a generic reason I could provide in the >>>>>>>> commit message rather than the existing commit message which seems to be board >>>>>>>> specific? Also, I hope that the above change will not cause regressions for >>>>>>>> other non-TI devices. Please let me know. >>>>>>> >>>>>>> Yes, that's the area, and just note that networking also requires the >>>>>>> DDR to be initialized. >>>>>>> >>>>>> >>>>>> Thank you for confirming and providing your suggestion for the contents of the >>>>>> commit message. >>>>>> >>>>> Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c >>>>> "dram_init_banksize()", the issue of fetching a file at SPL stage seemed >>>>> to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top >>>>> for the very first time in "spl_enable_cache()" results in >>>>> "arch_lmb_reserve()" function reserving memory region from Stack pointer >>>>> at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously >>>>> when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP >>>>> to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" >>>>> function call that invokes "arch_lmb_reserve()" function, which reserves >>>>> entire memory starting from Stack Pointer to gd->ram_top leaving no >>>>> space to load U-Boot image via TFTP since TFTP loads files at pre >>>>> configured memory address at "0x82000000". >>>>> >>>>> As a workaround for this issue, one solution we can propose is to >>>>> disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For >>>>> that we can define a new config option for LMB reserve checks as >>>>> "SPL_LMB". This config will be enable by default for the backword >>>>> compatibility and disable for our use case at SPL and U-Boot stage. >>>> >>>> The problem here is that we need LMB for booting an OS, which is >>>> something we'll want in SPL in non-cortex-R cases too, which means this >>>> platform, so that's a no-go. I think you need to dig harder and see if >>>> you can correct the logic somewhere so that we don't over reserve? >>>> >>> Since this issue is due to function call "lmb_init_and_reserve()" >>> function invoked from "tftp_init_load_addr()" function. This function >>> is defined by Simon in commit "a156c47e39ad", which fixes >>> "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you >>> explain why do we need to call "lmb_init_and_reserve()" function here ? >> >> This is indeed a tricky area which is why Sughosh is looking in to >> trying to re-work the LMB mechanic and we've had a few long threads >> about it as well. >> >> I've honestly forgotten the use case you have here, can you please >> remind us? >> > We are trying to boot AM62x using Ethernet for which we need to load > binary files at SPL and U-Boot stage using TFTP. To store the file we > need a free memory in RAM, specifically we are storing these files at > 0x82000000. But we are facing an issue while loading the file since > the memory area having an address 0x82000000 is reserved due to > "lmb_init_and_reserve()" function call. This function is called in > "tftp_init_load_addr()" function which is getting called exactly before > we are trying to get the free memory area by calling > "lmb_get_free_size()".

I have no idea about your platform but I was wondering if there is any particular importance of the load address of 0x82000000? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is getting reserved for the U-Boot image. Do you not have the option of loading the image at a lower address instead?

Sughosh,

I think my explanation was not clear at: "We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP."

• In Ethernet Booting we are fetching U-Boot image at SPL stage via

TFTP at specified address 0x82000000. While loading U-Boot image we are getting TFTP error, since address from stack pointer till gd->ram_top is reserved due to "lmb_init_and_reserve()" function call. I want to know for which purpose this address range is reserved.

On relocation, the U-Boot image is located typically at the top of the DRAM memory used by U-Boot(ram_top). That region of memory is reserved to ensure that the memory occupied by the U-Boot image does not get overwritten by a LMB reservation.

Yes, you are correct about U-Boot relocation but we are facing an issue at the time of fetching U-Boot proper at SPL stage.

The arch_lmb_reserve_generic() function would reserve the memory region from the ram_top to the current SP. Btw, you mentioned in an earlier reply that you are trying to load the U-Boot image at 0x82000000. From the config file it looks like that is the address of your SPL stack in RAM. So you might be overwriting your SPL stack. I think you can try a couple of things. One, move the SPL image above the SPL stack, like it is with U-Boot -- I think the way things stand,

Are you suggesting to relocate SPL image similar to U-Boot relocation. ?

the SPL image is at a lower address than the SP. And then use a lower address to load the U-Boot image with tftp.

-sughosh

...

...

Btw, are you facing this issue in SPL, or U-Boot proper? I built the images for the am62x_evm_a53 config, and I don't see the

We are getting "TFTP error" at runtime while fetching U-Boot proper at SPL stage while booting via "Ethernet", and we are using "am62x_evm_a53_ethboot_defconfig" instead of "am62x_evm_a53_defconfig".

These are the extra configs we are using on top of "am62x_evm_a53_defconfig":

CONFIG_SPL_DRIVERS_MISC=y CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_DMA=y CONFIG_SPL_ENV_SUPPORT=y CONFIG_SPL_ETH=y CONFIG_SPL_NET=y CONFIG_SPL_NET_VCI_STRING="AM62X U-Boot A53 SPL" CONFIG_SPL_SYSCON=y

...

arch_lmb_reserve() function getting included in the SPL image -- both the .text.arch_lmb_reserve and .text.arch_lmb_reserve_generic are part of discarded sections. So I am wondering how you are observing this behaviour in SPL.

-sughosh

...

...

Or using a higher address for SPL stack? You might be able to solve this just by re-examining which addresses (and RAM size limitations) need to be considered here.

Tom,

We tried this approach of assigning a higher address for SPL stack, but it is not working as expected.

On Thu, Apr 18, 2024 at 04:08:46PM +0530, Chintan Vankar wrote:

On 17/04/24 21:34, Tom Rini wrote:

...

On Wed, Apr 17, 2024 at 05:48:31PM +0530, Sughosh Ganu wrote:

...

hi Chintan,

On Wed, 17 Apr 2024 at 13:21, Chintan Vankar c-vankar@ti.com wrote:

...

On 16/04/24 22:30, Tom Rini wrote:

...

On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote:

...

On 12/04/24 03:37, Tom Rini wrote: > On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote: > > > > > > On 22/01/24 10:11, Siddharth Vadapalli wrote: > > > > > > > > > On 20/01/24 22:11, Tom Rini wrote: > > > > On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: > > > > > Hello Tom, > > > > > > > > > > On 12/01/24 18:56, Tom Rini wrote: > > > > > > ... > > > > > > > > > The list of conditionals in common/spl/spl.c::board_init_r() should be > > > > > > updated and probably use SPL_NET as the option to check for. > > > > > > > > > > Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I > > > > > assume that you are referring to the following change: > > > > > > > > > > if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) || > > > > > - IS_ENABLED(CONFIG_SPL_ATF)) > > > > > + IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET)) > > > > > dram_init_banksize(); > > > > > > > > > > I shall replace the current patch with the above change in the v2 series. Since > > > > > this is in the common section, is there a generic reason I could provide in the > > > > > commit message rather than the existing commit message which seems to be board > > > > > specific? Also, I hope that the above change will not cause regressions for > > > > > other non-TI devices. Please let me know. > > > > > > > > Yes, that's the area, and just note that networking also requires the > > > > DDR to be initialized. > > > > > > > > > > Thank you for confirming and providing your suggestion for the contents of the > > > commit message. > > > > > Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c > > "dram_init_banksize()", the issue of fetching a file at SPL stage seemed > > to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top > > for the very first time in "spl_enable_cache()" results in > > "arch_lmb_reserve()" function reserving memory region from Stack pointer > > at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously > > when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP > > to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" > > function call that invokes "arch_lmb_reserve()" function, which reserves > > entire memory starting from Stack Pointer to gd->ram_top leaving no > > space to load U-Boot image via TFTP since TFTP loads files at pre > > configured memory address at "0x82000000". > > > > As a workaround for this issue, one solution we can propose is to > > disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For > > that we can define a new config option for LMB reserve checks as > > "SPL_LMB". This config will be enable by default for the backword > > compatibility and disable for our use case at SPL and U-Boot stage. > > The problem here is that we need LMB for booting an OS, which is > something we'll want in SPL in non-cortex-R cases too, which means this > platform, so that's a no-go. I think you need to dig harder and see if > you can correct the logic somewhere so that we don't over reserve? > Since this issue is due to function call "lmb_init_and_reserve()" function invoked from "tftp_init_load_addr()" function. This function is defined by Simon in commit "a156c47e39ad", which fixes "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you explain why do we need to call "lmb_init_and_reserve()" function here ?

This is indeed a tricky area which is why Sughosh is looking in to trying to re-work the LMB mechanic and we've had a few long threads about it as well.

I've honestly forgotten the use case you have here, can you please remind us?

We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP. To store the file we need a free memory in RAM, specifically we are storing these files at 0x82000000. But we are facing an issue while loading the file since the memory area having an address 0x82000000 is reserved due to "lmb_init_and_reserve()" function call. This function is called in "tftp_init_load_addr()" function which is getting called exactly before we are trying to get the free memory area by calling "lmb_get_free_size()".

I have no idea about your platform but I was wondering if there is any particular importance of the load address of 0x82000000? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is getting reserved for the U-Boot image. Do you not have the option of loading the image at a lower address instead?

Sughosh,

I think my explanation was not clear at: "We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP."

• In Ethernet Booting we are fetching U-Boot image at SPL stage via

TFTP at specified address 0x82000000. While loading U-Boot image we are getting TFTP error, since address from stack pointer till gd->ram_top is reserved due to "lmb_init_and_reserve()" function call. I want to know for which purpose this address range is reserved.

...

Or using a higher address for SPL stack? You might be able to solve this just by re-examining which addresses (and RAM size limitations) need to be considered here.

Tom,

We tried this approach of assigning a higher address for SPL stack, but it is not working as expected.

Looking at the context here again, I think you need to re-evaluate what addresses are used and for where / what. I'm not happy with the combination of "enable LMB in SPL and then also remove the functionality of LMB", which is what you're proposing in essence now. There's either a different set of memory locations that should work, or some underlying bugs elsewhere that this exposes that need to be fixed. Thanks.

On Mon, Apr 22, 2024 at 04:46:04PM +0530, Chintan Vankar wrote:

On 17/04/24 21:34, Tom Rini wrote:

...

On Wed, Apr 17, 2024 at 05:48:31PM +0530, Sughosh Ganu wrote:

...

hi Chintan,

On Wed, 17 Apr 2024 at 13:21, Chintan Vankar c-vankar@ti.com wrote:

...

On 16/04/24 22:30, Tom Rini wrote:

...

On Tue, Apr 16, 2024 at 05:52:58PM +0530, Chintan Vankar wrote:

...

On 12/04/24 03:37, Tom Rini wrote: > On Wed, Apr 03, 2024 at 06:18:01PM +0530, Chintan Vankar wrote: > > > > > > On 22/01/24 10:11, Siddharth Vadapalli wrote: > > > > > > > > > On 20/01/24 22:11, Tom Rini wrote: > > > > On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: > > > > > Hello Tom, > > > > > > > > > > On 12/01/24 18:56, Tom Rini wrote: > > > > > > ... > > > > > > > > > The list of conditionals in common/spl/spl.c::board_init_r() should be > > > > > > updated and probably use SPL_NET as the option to check for. > > > > > > > > > > Thank you for reviewing the patch and pointing this out. I wasn't aware of it. I > > > > > assume that you are referring to the following change: > > > > > > > > > > if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) || > > > > > - IS_ENABLED(CONFIG_SPL_ATF)) > > > > > + IS_ENABLED(CONFIG_SPL_ATF) || IS_ENABLED(CONFIG_SPL_NET)) > > > > > dram_init_banksize(); > > > > > > > > > > I shall replace the current patch with the above change in the v2 series. Since > > > > > this is in the common section, is there a generic reason I could provide in the > > > > > commit message rather than the existing commit message which seems to be board > > > > > specific? Also, I hope that the above change will not cause regressions for > > > > > other non-TI devices. Please let me know. > > > > > > > > Yes, that's the area, and just note that networking also requires the > > > > DDR to be initialized. > > > > > > > > > > Thank you for confirming and providing your suggestion for the contents of the > > > commit message. > > > > > Following Tom's Suggestion of adding CONFIG_SPL_NET in common/spl/spl.c > > "dram_init_banksize()", the issue of fetching a file at SPL stage seemed > > to be fixed. However the commit "ba20b2443c29", which sets gd->ram_top > > for the very first time in "spl_enable_cache()" results in > > "arch_lmb_reserve()" function reserving memory region from Stack pointer > > at "0x81FFB820" to gd->ram_top pointing to "0x100000000". Previously > > when gd->ram_top was zero "arch_lmb_reserve()" was noop. Now using TFTP > > to fetch U-Boot image at SPL stage results in "tftp_init_load_addr()" > > function call that invokes "arch_lmb_reserve()" function, which reserves > > entire memory starting from Stack Pointer to gd->ram_top leaving no > > space to load U-Boot image via TFTP since TFTP loads files at pre > > configured memory address at "0x82000000". > > > > As a workaround for this issue, one solution we can propose is to > > disable the checks "lmb_get_free_size()" at SPL and U-Boot stage. For > > that we can define a new config option for LMB reserve checks as > > "SPL_LMB". This config will be enable by default for the backword > > compatibility and disable for our use case at SPL and U-Boot stage. > > The problem here is that we need LMB for booting an OS, which is > something we'll want in SPL in non-cortex-R cases too, which means this > platform, so that's a no-go. I think you need to dig harder and see if > you can correct the logic somewhere so that we don't over reserve? > Since this issue is due to function call "lmb_init_and_reserve()" function invoked from "tftp_init_load_addr()" function. This function is defined by Simon in commit "a156c47e39ad", which fixes "CVE-2018-18439" to prevent overwriting reserved memory. Simon, can you explain why do we need to call "lmb_init_and_reserve()" function here ?

This is indeed a tricky area which is why Sughosh is looking in to trying to re-work the LMB mechanic and we've had a few long threads about it as well.

I've honestly forgotten the use case you have here, can you please remind us?

We are trying to boot AM62x using Ethernet for which we need to load binary files at SPL and U-Boot stage using TFTP. To store the file we need a free memory in RAM, specifically we are storing these files at 0x82000000. But we are facing an issue while loading the file since the memory area having an address 0x82000000 is reserved due to "lmb_init_and_reserve()" function call. This function is called in "tftp_init_load_addr()" function which is getting called exactly before we are trying to get the free memory area by calling "lmb_get_free_size()".

I have no idea about your platform but I was wondering if there is any particular importance of the load address of 0x82000000? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is getting reserved for the U-Boot image. Do you not have the option of loading the image at a lower address instead?

Or using a higher address for SPL stack? You might be able to solve this just by re-examining which addresses (and RAM size limitations) need to be considered here.

Tom,

We changed SPL_STACK_R_ADDR to higher address as you suggested here and observe that the memory area which was getting reserved by "lmb_init_and_reserve()" function, when SPL_STACK_R_ADDR was 0x82000000, is from 0x81FFB820 to gd->ram_top, but when SPL_STACK_R_ADDR is changed to 0x83000000, stack pointer is pointing to 0x82FFB810 and reserving a memory area till gd->ram_top. Since memory address 0x82000000 is not there in reserved memory area region U-Boot proper is successfully getting fetched and we are able to boot.

Can it be considered of changing "SPL_STACK_R_ADDR" independently for Ethernet Boot feature ?

Yes, it's a configuration option so that it can be changed as needed.

On 12:17-20240112, Siddharth Vadapalli wrote:

From: Andreas Dannenberg dannenberg@ti.com

In order to enable Ethernet Boot using CPSW, update the clock data.

There is too many changes in the patch - including ownership change etc.. and the actual data addition itself is not clear - what is added..

could please elaborate in the commit message.

Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Siddharth Vadapalli s-vadapalli@ti.com

---

arch/arm/mach-k3/r5/am62x/clk-data.c | 79 ++++++++++++++-------------- 1 file changed, 39 insertions(+), 40 deletions(-)

[...]

On 15:49-20240122, Chintan Vankar wrote:

On 12/01/24 18:00, Nishanth Menon wrote:

...

On 12:17-20240112, Siddharth Vadapalli wrote:

...

From: Kishon Vijay Abraham I kishon@ti.com

In order to support Ethernet boot on AM62x, probe AM65 CPSW NUSS driver in board_init_f().

Why? doesn't the DM framework handle this?

Can you suggest how can we do this ?

Did'nt you guys just discuss this in https://lore.kernel.org/all/48c63fc4-9f06-4066-b206-a0a548936dcd@ti.com/ ?

On 12/01/24 18:01, Nishanth Menon wrote:

On 12:17-20240112, Siddharth Vadapalli wrote:

...

From: Kishon Vijay Abraham I kishon@ti.com

Add configs for enabling ETHBOOT in R5SPL. Adding a separate config minimizes the risk of going past the R5-SPL size limit for any future config additions.

Signed-off-by: Kishon Vijay Abraham I kishon@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com Signed-off-by: Siddharth Vadapalli s-vadapalli@ti.com

---

configs/am62x_evm_r5_ethboot_defconfig | 110 +++++++++++++++++++++++++

NAK. use config fragments please.

Ok.

On 12/01/24 18:02, Nishanth Menon wrote:

On 12:17-20240112, Siddharth Vadapalli wrote:

...

Hello,

This series enables Ethernet Boot on SK-AM62 device. Product Link: https://www.ti.com/tool/SK-AM62 User Guide: https://www.ti.com/lit/pdf/spruj40

Ethernet Boot flow is as follows:

1. The BOOT MODE pins are configured for Ethernet Boot.
2. On powering on the device, ROM uses the Ethernet Port corresponding

to CPSW3G's MAC Port 1 to transmit "TI K3 Bootp Boot". 3. The TFTP server and DHCP server on the receiver device need to be configured such that VCI string "TI K3 Bootp Boot" maps to the file "tiboot3.bin" and the TFTP server should be capable of transferring it to the device. 4. ROM loads and executes "tiboot3.bin" provided by the TFTP server. 5. The "tiboot3.bin" file is expected to be built using the config: am62x_evm_r5_ethboot_defconfig introduced in this series, which shall enable "tispl.bin" to be fetched over TFTP using "tiboot3.bin". 6. "tiboot3.bin" is configured to transmit "AM62X U-Boot R5 SPL" as its NET_VCI_STRING, thereby implying that the DHCP server and TFTP server need to be configured to transfer "tispl.bin" to the device. 7. "tiboot3.bin" loads and executes "tispl.bin" provided by the TFTP server. 8. The "tispl.bin" file is expected to be built using the config: am62x_evm_a53_defconfig which has been updated in this series to enable Ethernet Boot specific configs, allowing "u-boot.img" to be fetched over TFTP using "tispl.bin". 9. "tispl.bin" is configured to transmit "AM62X U-Boot A53 SPL" as its NET_VCI_STRING. The DHCP server and TFTP server need to be configured to transfer "u-boot.img" to the device for the aforementioned NET_VCI_STRING. 10. "tispl.bin" then fetches "u-boot.img" using TFTP and loads and executes it on the device, completing the process of Ethernet Boot on the device.

NOTE: ROM configures CPSW3G's MAC Port 1 for 100 Mbps full-duplex mode of operation due to which it is expected that the Link Partner also supports the same mode of operation. Additionally, enabling "phy_gmii_sel" node at SPL stage will be necessary and is not added as a part of this series with the aim of adding the "bootph-all" property to its counterpart in Linux device-tree first.

NAK - instead of writing all this up in the commit message, why would you not spend that time updating the excellent documentation we have?

I plan to document it after the feature is in. The reason for mentioning the content above is for explaining the flow in case anyone wishes to test and verify it. Wouldn't documenting it make it appear that the feature is present when it isn't?

...

On 12/01/24 18:12, Nishanth Menon wrote:

On 18:06-20240112, Siddharth Vadapalli wrote:

...

On 12/01/24 18:02, Nishanth Menon wrote:

...

On 12:17-20240112, Siddharth Vadapalli wrote:

...

Hello,

This series enables Ethernet Boot on SK-AM62 device. Product Link: https://www.ti.com/tool/SK-AM62 User Guide: https://www.ti.com/lit/pdf/spruj40

Ethernet Boot flow is as follows:

1. The BOOT MODE pins are configured for Ethernet Boot.
2. On powering on the device, ROM uses the Ethernet Port corresponding

to CPSW3G's MAC Port 1 to transmit "TI K3 Bootp Boot". 3. The TFTP server and DHCP server on the receiver device need to be configured such that VCI string "TI K3 Bootp Boot" maps to the file "tiboot3.bin" and the TFTP server should be capable of transferring it to the device. 4. ROM loads and executes "tiboot3.bin" provided by the TFTP server. 5. The "tiboot3.bin" file is expected to be built using the config: am62x_evm_r5_ethboot_defconfig introduced in this series, which shall enable "tispl.bin" to be fetched over TFTP using "tiboot3.bin". 6. "tiboot3.bin" is configured to transmit "AM62X U-Boot R5 SPL" as its NET_VCI_STRING, thereby implying that the DHCP server and TFTP server need to be configured to transfer "tispl.bin" to the device. 7. "tiboot3.bin" loads and executes "tispl.bin" provided by the TFTP server. 8. The "tispl.bin" file is expected to be built using the config: am62x_evm_a53_defconfig which has been updated in this series to enable Ethernet Boot specific configs, allowing "u-boot.img" to be fetched over TFTP using "tispl.bin". 9. "tispl.bin" is configured to transmit "AM62X U-Boot A53 SPL" as its NET_VCI_STRING. The DHCP server and TFTP server need to be configured to transfer "u-boot.img" to the device for the aforementioned NET_VCI_STRING. 10. "tispl.bin" then fetches "u-boot.img" using TFTP and loads and executes it on the device, completing the process of Ethernet Boot on the device.

NOTE: ROM configures CPSW3G's MAC Port 1 for 100 Mbps full-duplex mode of operation due to which it is expected that the Link Partner also supports the same mode of operation. Additionally, enabling "phy_gmii_sel" node at SPL stage will be necessary and is not added as a part of this series with the aim of adding the "bootph-all" property to its counterpart in Linux device-tree first.

NAK - instead of writing all this up in the commit message, why would you not spend that time updating the excellent documentation we have?

I plan to document it after the feature is in. The reason for mentioning the content above is for explaining the flow in case anyone wishes to test and verify it. Wouldn't documenting it make it appear that the feature is present when it isn't?

So you are saying this series does NOT work! why are you sending patches then? If you are introducing a feature and enabling it, ensure you send documentation along with it allowing people to be able to actually use the feature.

I have mentioned in the "NOTE" above that enabling "phy_gmii_sel" node at SPL stage by adding the "bootph-all" property is necessary to verify this series. I cannot post that with this series since Linux device-tree needs to have the property added first and the merge window is closed now. Once it is in the Linux device-tree, syncing U-Boot device-tree with Linux device-tree will enable Ethernet Boot which is when the feature will work. That is when I was planning to document it. However, based on your feedback, in the next version for this series I will add the documentation as well along with the note that "phy_gmii_sel" needs to be enabled at SPL stage for the feature to work.

Please let me know if that is acceptable.

On 12/01/24 18:31, Nishanth Menon wrote:

On 18:17-20240112, Siddharth Vadapalli wrote:

...

On 12/01/24 18:12, Nishanth Menon wrote:

...

On 18:06-20240112, Siddharth Vadapalli wrote:

...

On 12/01/24 18:02, Nishanth Menon wrote:

...

On 12:17-20240112, Siddharth Vadapalli wrote:

...

Hello,

This series enables Ethernet Boot on SK-AM62 device. Product Link: https://www.ti.com/tool/SK-AM62 User Guide: https://www.ti.com/lit/pdf/spruj40

Ethernet Boot flow is as follows:

1. The BOOT MODE pins are configured for Ethernet Boot.
2. On powering on the device, ROM uses the Ethernet Port corresponding

to CPSW3G's MAC Port 1 to transmit "TI K3 Bootp Boot". 3. The TFTP server and DHCP server on the receiver device need to be configured such that VCI string "TI K3 Bootp Boot" maps to the file "tiboot3.bin" and the TFTP server should be capable of transferring it to the device. 4. ROM loads and executes "tiboot3.bin" provided by the TFTP server. 5. The "tiboot3.bin" file is expected to be built using the config: am62x_evm_r5_ethboot_defconfig introduced in this series, which shall enable "tispl.bin" to be fetched over TFTP using "tiboot3.bin". 6. "tiboot3.bin" is configured to transmit "AM62X U-Boot R5 SPL" as its NET_VCI_STRING, thereby implying that the DHCP server and TFTP server need to be configured to transfer "tispl.bin" to the device. 7. "tiboot3.bin" loads and executes "tispl.bin" provided by the TFTP server. 8. The "tispl.bin" file is expected to be built using the config: am62x_evm_a53_defconfig which has been updated in this series to enable Ethernet Boot specific configs, allowing "u-boot.img" to be fetched over TFTP using "tispl.bin". 9. "tispl.bin" is configured to transmit "AM62X U-Boot A53 SPL" as its NET_VCI_STRING. The DHCP server and TFTP server need to be configured to transfer "u-boot.img" to the device for the aforementioned NET_VCI_STRING. 10. "tispl.bin" then fetches "u-boot.img" using TFTP and loads and executes it on the device, completing the process of Ethernet Boot on the device.

NOTE: ROM configures CPSW3G's MAC Port 1 for 100 Mbps full-duplex mode of operation due to which it is expected that the Link Partner also supports the same mode of operation. Additionally, enabling "phy_gmii_sel" node at SPL stage will be necessary and is not added as a part of this series with the aim of adding the "bootph-all" property to its counterpart in Linux device-tree first.

NAK - instead of writing all this up in the commit message, why would you not spend that time updating the excellent documentation we have?

I plan to document it after the feature is in. The reason for mentioning the content above is for explaining the flow in case anyone wishes to test and verify it. Wouldn't documenting it make it appear that the feature is present when it isn't?

So you are saying this series does NOT work! why are you sending patches then? If you are introducing a feature and enabling it, ensure you send documentation along with it allowing people to be able to actually use the feature.

I have mentioned in the "NOTE" above that enabling "phy_gmii_sel" node at SPL stage by adding the "bootph-all" property is necessary to verify this series. I cannot post that with this series since Linux device-tree needs to have the property added first and the merge window is closed now. Once it is in the Linux device-tree, syncing U-Boot device-tree with Linux device-tree will enable Ethernet Boot which is when the feature will work. That is when I was planning to document it. However, based on your feedback, in the next version for this series I will add the documentation as well along with the note that "phy_gmii_sel" needs to be enabled at SPL stage for the feature to work.

considered first posting a patch to kernel.org (merge window has nothing to do with posting and having patches reviewed) and in the interim, doing that in u-boot.dtsi so that the next sync will drop it from u-boot.dtsi?

OR hold the series back till it is merged into kernel.org master?

Either way, please do not send patches to the list that does not work.

Since it is now hard to trust your patches without detailed cover letter analysis, next time you are updating the series post test logs as well with just the patches applied and no additional changes.

Thank you for clarifying regarding the approach to be taken. I shall include the logs when I post the v2 series, in addition to the Documentation update.