Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 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 "0x1". > > > > > > > > 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 "0x8200". > > > > > > > > > > > > > > > > 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
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 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 "0x1". 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 "0x8200". 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 0x8200. But we are facing an issue while loading the file since the memory area having an address 0x8200 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 0x8200? 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 0x8200, is from 0x81FFB820 to gd->ram_top, but when
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On Fri, 19 Apr 2024 at 17:23, Chintan Vankar wrote: > > > > On 19/04/24 17:04, Sughosh Ganu wrote: > > On Fri, 19 Apr 2024 at 16:04, Chintan Vankar wrote: > >> > >> > >> > >> On 18/04/24 17:30, Sughosh Ganu wrote: > >>> On Thu, 18 Apr 2024 at 16:08, 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 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 "0x1". > >>> 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 "0x8200". > >>> > >>> 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
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On 19/04/24 17:04, Sughosh Ganu wrote: On Fri, 19 Apr 2024 at 16:04, Chintan Vankar wrote: On 18/04/24 17:30, Sughosh Ganu wrote: On Thu, 18 Apr 2024 at 16:08, 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 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 "0x1". 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 "0x8200". 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 0x8200. But we are facing an issue while loading the file since the memory area having an address 0x8200 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 0x8200? 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
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On Fri, 19 Apr 2024 at 16:04, Chintan Vankar wrote: > > > > On 18/04/24 17:30, Sughosh Ganu wrote: > > On Thu, 18 Apr 2024 at 16:08, 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 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 "0x1". 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 "0x8200". > > > > 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 > >>
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On 18/04/24 17:30, Sughosh Ganu wrote: On Thu, 18 Apr 2024 at 16:08, 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 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 "0x1". 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 "0x8200". 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 0x8200. But we are facing an issue while loading the file since the memory area having an address 0x8200 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 0x8200? 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 0x8200. While loading U-Boot image we are getting TFTP error,
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 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 "0x1". > > > > > > > > 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 "0x8200". > > > > > > > > > > > > > > > > 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
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On Thu, 18 Apr 2024 at 16:08, 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 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 "0x1". 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 "0x8200". > >>> > >>> 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 > >>> 0x8200. But we are facing an issue while loading the
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 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 "0x1". 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 "0x8200". 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 0x8200. But we are facing an issue while loading the file since the memory area having an address 0x8200 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 0x8200? 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 0x8200. 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
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 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 "0x1". 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 "0x8200". > > > > 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 > > 0x8200. But we are facing an issue while loading the file since > > the memory area having an address 0x8200 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
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
hi Chintan, On Wed, 17 Apr 2024 at 13:21, Chintan Vankar 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 "0x1". 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 "0x8200". > > 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 > 0x8200. But we are facing an issue while loading the file since > the memory area having an address 0x8200 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 0x8200? It looks as though the current location of the SP when arch_lmb_reserve() gets called means that the load address is
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 "0x1". 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 "0x8200". 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 0x8200. But we are facing an issue while loading the file since the memory area having an address 0x8200 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()".
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 "0x1". 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 "0x8200". > > > > > > 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? -- Tom signature.asc Description: PGP signature
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 "0x1". 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 "0x8200". 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 ?
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 "0x1". 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 "0x8200". > > 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? -- Tom signature.asc Description: PGP signature
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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 "0x1". 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 "0x8200". 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.
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
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. -- Regards, Siddharth.
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On Mon, Jan 15, 2024 at 01:42:51PM +0530, Siddharth Vadapalli wrote: > Hello Tom, > > On 12/01/24 18:56, Tom Rini wrote: > > On Fri, Jan 12, 2024 at 12:17:50PM +0530, Siddharth Vadapalli wrote: > > > >> From: Kishon Vijay Abraham I > >> > >> Call dram_init_banksize() from spl_board_init() otherwise TFTP download > >> fails with error "TFTP error: trying to overwrite reserved memory..." > >> due to lmb_get_free_size() not able to find unreserved region due > >> to lack of DRAM size info. Required to support Ethernet boot on AM62x. > >> > >> Signed-off-by: Kishon Vijay Abraham I > >> Signed-off-by: Siddharth Vadapalli > >> --- > >> board/ti/am62x/evm.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c > >> index ad93908840..35f291d83a 100644 > >> --- a/board/ti/am62x/evm.c > >> +++ b/board/ti/am62x/evm.c > >> @@ -85,6 +85,9 @@ void spl_board_init(void) > >>if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) > >>splash_display(); > >> > >> + if (IS_ENABLED(CONFIG_SPL_ETH)) > >> + /* Init DRAM size for R5/A53 SPL */ > >> + dram_init_banksize(); > > > > 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. -- Tom signature.asc Description: PGP signature
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
Hello Tom, On 12/01/24 18:56, Tom Rini wrote: > On Fri, Jan 12, 2024 at 12:17:50PM +0530, Siddharth Vadapalli wrote: > >> From: Kishon Vijay Abraham I >> >> Call dram_init_banksize() from spl_board_init() otherwise TFTP download >> fails with error "TFTP error: trying to overwrite reserved memory..." >> due to lmb_get_free_size() not able to find unreserved region due >> to lack of DRAM size info. Required to support Ethernet boot on AM62x. >> >> Signed-off-by: Kishon Vijay Abraham I >> Signed-off-by: Siddharth Vadapalli >> --- >> board/ti/am62x/evm.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c >> index ad93908840..35f291d83a 100644 >> --- a/board/ti/am62x/evm.c >> +++ b/board/ti/am62x/evm.c >> @@ -85,6 +85,9 @@ void spl_board_init(void) >> if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) >> splash_display(); >> >> +if (IS_ENABLED(CONFIG_SPL_ETH)) >> +/* Init DRAM size for R5/A53 SPL */ >> +dram_init_banksize(); > > 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. > -- Regards, Siddharth.
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On Fri, Jan 12, 2024 at 12:17:50PM +0530, Siddharth Vadapalli wrote: > From: Kishon Vijay Abraham I > > Call dram_init_banksize() from spl_board_init() otherwise TFTP download > fails with error "TFTP error: trying to overwrite reserved memory..." > due to lmb_get_free_size() not able to find unreserved region due > to lack of DRAM size info. Required to support Ethernet boot on AM62x. > > Signed-off-by: Kishon Vijay Abraham I > Signed-off-by: Siddharth Vadapalli > --- > board/ti/am62x/evm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c > index ad93908840..35f291d83a 100644 > --- a/board/ti/am62x/evm.c > +++ b/board/ti/am62x/evm.c > @@ -85,6 +85,9 @@ void spl_board_init(void) > if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) > splash_display(); > > + if (IS_ENABLED(CONFIG_SPL_ETH)) > + /* Init DRAM size for R5/A53 SPL */ > + dram_init_banksize(); 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. -- Tom signature.asc Description: PGP signature
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On 18:01-20240112, Siddharth Vadapalli wrote: > Hello Nishanth, > > On 12/01/24 17:56, Nishanth Menon wrote: > > On 12:17-20240112, Siddharth Vadapalli wrote: > >> From: Kishon Vijay Abraham I > >> > >> Call dram_init_banksize() from spl_board_init() otherwise TFTP download > >> fails with error "TFTP error: trying to overwrite reserved memory..." > >> due to lmb_get_free_size() not able to find unreserved region due > >> to lack of DRAM size info. Required to support Ethernet boot on AM62x. > >> > >> Signed-off-by: Kishon Vijay Abraham I > >> Signed-off-by: Siddharth Vadapalli > >> --- > >> board/ti/am62x/evm.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c > >> index ad93908840..35f291d83a 100644 > >> --- a/board/ti/am62x/evm.c > >> +++ b/board/ti/am62x/evm.c > >> @@ -85,6 +85,9 @@ void spl_board_init(void) > >>if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) > >>splash_display(); > >> > >> + if (IS_ENABLED(CONFIG_SPL_ETH)) > >> + /* Init DRAM size for R5/A53 SPL */ > >> + dram_init_banksize(); > >> } > >> > >> #if defined(CONFIG_K3_AM64_DDRSS) > >> -- > >> 2.34.1 > >> > > > > Are you sure? tftp seems to work without this patch. > > > > https://gist.github.com/nmenon/91e3282e00e38ae42e8cf640be2ab888 > > I noticed the error pointed out in the commit message at the A53 SPL stage > when > the U-Boot Image is being fetched over TFTP without this patch, so I have > verified that this patch is necessary. The logs you have shared verify TFTP at > U-Boot, but this patch is for enabling TFTP at A53 SPL for fetching U-Boot > image > via TFTP. Please fix the commit message. I still dont get why we have to explicitly have to call dram_init_banksize is that because some sort of configuration option was missed? -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
Hello Nishanth, On 12/01/24 17:56, Nishanth Menon wrote: > On 12:17-20240112, Siddharth Vadapalli wrote: >> From: Kishon Vijay Abraham I >> >> Call dram_init_banksize() from spl_board_init() otherwise TFTP download >> fails with error "TFTP error: trying to overwrite reserved memory..." >> due to lmb_get_free_size() not able to find unreserved region due >> to lack of DRAM size info. Required to support Ethernet boot on AM62x. >> >> Signed-off-by: Kishon Vijay Abraham I >> Signed-off-by: Siddharth Vadapalli >> --- >> board/ti/am62x/evm.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c >> index ad93908840..35f291d83a 100644 >> --- a/board/ti/am62x/evm.c >> +++ b/board/ti/am62x/evm.c >> @@ -85,6 +85,9 @@ void spl_board_init(void) >> if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) >> splash_display(); >> >> +if (IS_ENABLED(CONFIG_SPL_ETH)) >> +/* Init DRAM size for R5/A53 SPL */ >> +dram_init_banksize(); >> } >> >> #if defined(CONFIG_K3_AM64_DDRSS) >> -- >> 2.34.1 >> > > Are you sure? tftp seems to work without this patch. > > https://gist.github.com/nmenon/91e3282e00e38ae42e8cf640be2ab888 I noticed the error pointed out in the commit message at the A53 SPL stage when the U-Boot Image is being fetched over TFTP without this patch, so I have verified that this patch is necessary. The logs you have shared verify TFTP at U-Boot, but this patch is for enabling TFTP at A53 SPL for fetching U-Boot image via TFTP. -- Regards, Siddharth.
Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
On 12:17-20240112, Siddharth Vadapalli wrote: > From: Kishon Vijay Abraham I > > Call dram_init_banksize() from spl_board_init() otherwise TFTP download > fails with error "TFTP error: trying to overwrite reserved memory..." > due to lmb_get_free_size() not able to find unreserved region due > to lack of DRAM size info. Required to support Ethernet boot on AM62x. > > Signed-off-by: Kishon Vijay Abraham I > Signed-off-by: Siddharth Vadapalli > --- > board/ti/am62x/evm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c > index ad93908840..35f291d83a 100644 > --- a/board/ti/am62x/evm.c > +++ b/board/ti/am62x/evm.c > @@ -85,6 +85,9 @@ void spl_board_init(void) > if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) > splash_display(); > > + if (IS_ENABLED(CONFIG_SPL_ETH)) > + /* Init DRAM size for R5/A53 SPL */ > + dram_init_banksize(); > } > > #if defined(CONFIG_K3_AM64_DDRSS) > -- > 2.34.1 > Are you sure? tftp seems to work without this patch. https://gist.github.com/nmenon/91e3282e00e38ae42e8cf640be2ab888 -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
[PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL
From: Kishon Vijay Abraham I Call dram_init_banksize() from spl_board_init() otherwise TFTP download fails with error "TFTP error: trying to overwrite reserved memory..." due to lmb_get_free_size() not able to find unreserved region due to lack of DRAM size info. Required to support Ethernet boot on AM62x. Signed-off-by: Kishon Vijay Abraham I Signed-off-by: Siddharth Vadapalli --- board/ti/am62x/evm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c index ad93908840..35f291d83a 100644 --- a/board/ti/am62x/evm.c +++ b/board/ti/am62x/evm.c @@ -85,6 +85,9 @@ void spl_board_init(void) if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP)) splash_display(); + if (IS_ENABLED(CONFIG_SPL_ETH)) + /* Init DRAM size for R5/A53 SPL */ + dram_init_banksize(); } #if defined(CONFIG_K3_AM64_DDRSS) -- 2.34.1
