[PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Siddharth Vadapalli]

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.

This series is based on commit:
f28a77589e Merge tag 'dm-next-7jan23' of https://source.denx.de/u-boot/custodians/u-boot-dm into next
of the next branch of u-boot.

Regards,
Siddharth.

Andreas Dannenberg (1):
  arm: mach-k3: am62: Update SoC autogenerated data to enable Ethernet
    Boot

Kishon Vijay Abraham I (7):
  board: ti: am62x: Init DRAM size in R5/A53 SPL
  firmware: ti_sci: Add No-OP for "RX_FL_CFG"
  soc: ti: k3-navss-ringacc: Initialize base address of ring cfg
    registers
  dma: ti: k3-udma: Add support for native configuration of chan/flow
  arm: mach-k3: am625_init: Probe AM65 CPSW NUSS
  configs: am62: Add configs for enabling ETHBOOT in R5SPL
  configs: am62x_evm_a53_defconfig: Enable configs required for Ethboot

Siddharth Vadapalli (1):
  arm: dts: k3-am625-r5-sk: Enable DM services for main_pktdma

Vignesh Raghavendra (1):
  soc: ti: k3-navss-ringacc: Fix reset ring API

 arch/arm/dts/k3-am625-r5-sk.dts          |   5 ++
 arch/arm/mach-k3/am625_init.c            |  10 +++
 arch/arm/mach-k3/r5/am62x/clk-data.c     |  79 ++++++++--------
 board/ti/am62x/evm.c                     |   3 +
 configs/am62x_evm_a53_defconfig          |   8 ++
 configs/am62x_evm_r5_ethboot_defconfig   | 110 +++++++++++++++++++++++
 drivers/dma/ti/k3-udma.c                 |   6 ++
 drivers/firmware/ti_sci.c                |   8 +-
 drivers/soc/ti/k3-navss-ringacc-u-boot.c |   9 +-
 drivers/soc/ti/k3-navss-ringacc.c        |   7 +-
 10 files changed, 202 insertions(+), 43 deletions(-)
 create mode 100644 configs/am62x_evm_r5_ethboot_defconfig

-- 
2.34.1

[PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

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 <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 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

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Nishanth Menon]

On 12:17-20240112, Siddharth Vadapalli wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
> 
> 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 <kishon@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  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

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Siddharth Vadapalli]

Hello Nishanth,

On 12/01/24 17:56, Nishanth Menon wrote:
> On 12:17-20240112, Siddharth Vadapalli wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> 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 <kishon@ti.com>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  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

[Nishanth Menon]

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 <kishon@ti.com>
> >>
> >> 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 <kishon@ti.com>
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >> ---
> >>  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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On Fri, Jan 12, 2024 at 12:17:50PM +0530, Siddharth Vadapalli wrote:

> From: Kishon Vijay Abraham I <kishon@ti.com>
> 
> 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 <kishon@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Siddharth Vadapalli]

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 <kishon@ti.com>
>>
>> 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 <kishon@ti.com>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

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 <kishon@ti.com>
> >>
> >> 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 <kishon@ti.com>
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >> ---
> >>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Siddharth Vadapalli]

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

[Chintan Vankar]

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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Chintan Vankar]

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 ?

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4035 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Chintan Vankar]

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()".

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Sughosh Ganu]

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

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5588 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Chintan Vankar]

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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Sughosh Ganu]

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.

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
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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Chintan Vankar]

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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Sughosh Ganu]

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,
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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Chintan Vankar]

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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Sughosh Ganu]

On Fri, 19 Apr 2024 at 17:23, Chintan Vankar <c-vankar@ti.com> wrote:
>
>
>
> 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. ?

I am not familiar with how your platform boots. But based on the
SPL_TEXT_BASE, it looks like some other entity is initialising the
dram and then loading the SPL to SPL_TEXT_BASE. If that is true, can
you not load the SPL image at an address higher than your SPL stack
address? You just need to link the SPL image for that corresponding
address, and then load the SPL image to that address in memory. If
that is not possible for whatever reasons, I feel that the U-Boot
image that you are loading in SPL should be loaded at a lower address
-- trying to load it at 0x82000000 would overwrite your SPL stack
region.

-sughosh

>
> > 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.

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7519 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Chintan Vankar]

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 ?

Re: [PATCH 01/10] board: ti: am62x: Init DRAM size in R5/A53 SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7167 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 02/10] firmware: ti_sci: Add No-OP for "RX_FL_CFG"

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

RX_FL_CFG message should not be forwarded to TIFS
and should be handled within R5 SPL (when DM services
are not available). Add a no-op function to not handle
RX_FL_CFG messages.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/firmware/ti_sci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 6e9f93e9a3..a13a2ead08 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c
@@ -2445,6 +2445,12 @@ fail:
 	return ret;
 }
 
+static int ti_sci_cmd_rm_udmap_rx_flow_cfg_noop(const struct ti_sci_handle *handle,
+						const struct ti_sci_msg_rm_udmap_flow_cfg *params)
+{
+	return 0;
+}
+
 /**
  * ti_sci_cmd_set_fwl_region() - Request for configuring a firewall region
  * @handle:    pointer to TI SCI handle
@@ -2884,7 +2890,7 @@ static __maybe_unused int ti_sci_dm_probe(struct udevice *dev)
 	udmap_ops = &ops->rm_udmap_ops;
 	udmap_ops->tx_ch_cfg = ti_sci_cmd_rm_udmap_tx_ch_cfg;
 	udmap_ops->rx_ch_cfg = ti_sci_cmd_rm_udmap_rx_ch_cfg;
-	udmap_ops->rx_flow_cfg = ti_sci_cmd_rm_udmap_rx_flow_cfg;
+	udmap_ops->rx_flow_cfg = ti_sci_cmd_rm_udmap_rx_flow_cfg_noop;
 
 	return ret;
 }
-- 
2.34.1

[PATCH 03/10] soc: ti: k3-navss-ringacc: Initialize base address of ring cfg registers

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

Initialize base address of ring config registers required to natively
setup ring cfg registers in the absence of Device Manager (DM) services
at R5 SPL stage.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/soc/ti/k3-navss-ringacc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/ti/k3-navss-ringacc.c b/drivers/soc/ti/k3-navss-ringacc.c
index 7a2fbb0db6..31e9b372ee 100644
--- a/drivers/soc/ti/k3-navss-ringacc.c
+++ b/drivers/soc/ti/k3-navss-ringacc.c
@@ -1030,8 +1030,8 @@ static int k3_nav_ringacc_init(struct udevice *dev, struct k3_nav_ringacc *ringa
 struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
 						struct k3_ringacc_init_data *data)
 {
+	void __iomem *base_rt, *base_cfg;
 	struct k3_nav_ringacc *ringacc;
-	void __iomem *base_rt;
 	int i;
 
 	ringacc = devm_kzalloc(dev, sizeof(*ringacc), GFP_KERNEL);
@@ -1049,6 +1049,10 @@ struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
 	if (!base_rt)
 		return ERR_PTR(-EINVAL);
 
+	base_cfg = dev_read_addr_name_ptr(dev, "cfg");
+	if (!base_cfg)
+		return ERR_PTR(-EINVAL);
+
 	ringacc->rings = devm_kzalloc(dev,
 				      sizeof(*ringacc->rings) *
 				      ringacc->num_rings * 2,
@@ -1063,6 +1067,7 @@ struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
 	for (i = 0; i < ringacc->num_rings; i++) {
 		struct k3_nav_ring *ring = &ringacc->rings[i];
 
+		ring->cfg = base_cfg + KNAV_RINGACC_CFG_REGS_STEP * i;
 		ring->rt = base_rt + K3_DMARING_RING_RT_REGS_STEP * i;
 		ring->parent = ringacc;
 		ring->ring_id = i;
-- 
2.34.1

Re: [PATCH 03/10] soc: ti: k3-navss-ringacc: Initialize base address of ring cfg registers

[Roger Quadros]

Hi,

On 12/01/2024 08:47, Siddharth Vadapalli wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
> 
> Initialize base address of ring config registers required to natively
> setup ring cfg registers in the absence of Device Manager (DM) services
> at R5 SPL stage.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/soc/ti/k3-navss-ringacc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/ti/k3-navss-ringacc.c b/drivers/soc/ti/k3-navss-ringacc.c
> index 7a2fbb0db6..31e9b372ee 100644
> --- a/drivers/soc/ti/k3-navss-ringacc.c
> +++ b/drivers/soc/ti/k3-navss-ringacc.c
> @@ -1030,8 +1030,8 @@ static int k3_nav_ringacc_init(struct udevice *dev, struct k3_nav_ringacc *ringa
>  struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
>  						struct k3_ringacc_init_data *data)
>  {
> +	void __iomem *base_rt, *base_cfg;
>  	struct k3_nav_ringacc *ringacc;
> -	void __iomem *base_rt;
>  	int i;
>  
>  	ringacc = devm_kzalloc(dev, sizeof(*ringacc), GFP_KERNEL);
> @@ -1049,6 +1049,10 @@ struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
>  	if (!base_rt)
>  		return ERR_PTR(-EINVAL);
>  
> +	base_cfg = dev_read_addr_name_ptr(dev, "cfg");
> +	if (!base_cfg)
> +		return ERR_PTR(-EINVAL);
> +

Should this be restricted only for R5 SPL case? else we conflict with
Device Manager services?

>  	ringacc->rings = devm_kzalloc(dev,
>  				      sizeof(*ringacc->rings) *
>  				      ringacc->num_rings * 2,
> @@ -1063,6 +1067,7 @@ struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
>  	for (i = 0; i < ringacc->num_rings; i++) {
>  		struct k3_nav_ring *ring = &ringacc->rings[i];
>  
> +		ring->cfg = base_cfg + KNAV_RINGACC_CFG_REGS_STEP * i;
>  		ring->rt = base_rt + K3_DMARING_RING_RT_REGS_STEP * i;
>  		ring->parent = ringacc;
>  		ring->ring_id = i;

-- 
cheers,
-roger

Re: [PATCH 03/10] soc: ti: k3-navss-ringacc: Initialize base address of ring cfg registers

[Chintan Vankar]

On 16/01/24 17:13, Roger Quadros wrote:
> Hi,
> 
> On 12/01/2024 08:47, Siddharth Vadapalli wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> Initialize base address of ring config registers required to natively
>> setup ring cfg registers in the absence of Device Manager (DM) services
>> at R5 SPL stage.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>   drivers/soc/ti/k3-navss-ringacc.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/ti/k3-navss-ringacc.c b/drivers/soc/ti/k3-navss-ringacc.c
>> index 7a2fbb0db6..31e9b372ee 100644
>> --- a/drivers/soc/ti/k3-navss-ringacc.c
>> +++ b/drivers/soc/ti/k3-navss-ringacc.c
>> @@ -1030,8 +1030,8 @@ static int k3_nav_ringacc_init(struct udevice *dev, struct k3_nav_ringacc *ringa
>>   struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
>>   						struct k3_ringacc_init_data *data)
>>   {
>> +	void __iomem *base_rt, *base_cfg;
>>   	struct k3_nav_ringacc *ringacc;
>> -	void __iomem *base_rt;
>>   	int i;
>>   
>>   	ringacc = devm_kzalloc(dev, sizeof(*ringacc), GFP_KERNEL);
>> @@ -1049,6 +1049,10 @@ struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
>>   	if (!base_rt)
>>   		return ERR_PTR(-EINVAL);
>>   
>> +	base_cfg = dev_read_addr_name_ptr(dev, "cfg");
>> +	if (!base_cfg)
>> +		return ERR_PTR(-EINVAL);
>> +
> 
> Should this be restricted only for R5 SPL case? else we conflict with
> Device Manager services?
> 

I don't see any conflict in it, you can go through this:
https://github.com/u-boot/u-boot/commit/86e58800fd7cdba4fa9229aeee3a54a2ccece406

>>   	ringacc->rings = devm_kzalloc(dev,
>>   				      sizeof(*ringacc->rings) *
>>   				      ringacc->num_rings * 2,
>> @@ -1063,6 +1067,7 @@ struct k3_nav_ringacc *k3_ringacc_dmarings_init(struct udevice *dev,
>>   	for (i = 0; i < ringacc->num_rings; i++) {
>>   		struct k3_nav_ring *ring = &ringacc->rings[i];
>>   
>> +		ring->cfg = base_cfg + KNAV_RINGACC_CFG_REGS_STEP * i;
>>   		ring->rt = base_rt + K3_DMARING_RING_RT_REGS_STEP * i;
>>   		ring->parent = ringacc;
>>   		ring->ring_id = i;
> 

[PATCH 04/10] soc: ti: k3-navss-ringacc: Fix reset ring API

[Siddharth Vadapalli]

From: Vignesh Raghavendra <vigneshr@ti.com>

Expectation of k3_ringacc_ring_reset_raw() is to reset the ring to
requested size and not to 0. Fix this.

Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/soc/ti/k3-navss-ringacc-u-boot.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/ti/k3-navss-ringacc-u-boot.c b/drivers/soc/ti/k3-navss-ringacc-u-boot.c
index f958239c2a..5d650b9de7 100644
--- a/drivers/soc/ti/k3-navss-ringacc-u-boot.c
+++ b/drivers/soc/ti/k3-navss-ringacc-u-boot.c
@@ -25,9 +25,16 @@ struct k3_nav_ring_cfg_regs {
 #define KNAV_RINGACC_CFG_RING_SIZE_ELSIZE_MASK		GENMASK(26, 24)
 #define KNAV_RINGACC_CFG_RING_SIZE_ELSIZE_SHIFT		(24)
 
+#define KNAV_RINGACC_CFG_RING_SIZE_MASK			GENMASK(15, 0)
+
 static void k3_ringacc_ring_reset_raw(struct k3_nav_ring *ring)
 {
-	writel(0, &ring->cfg->size);
+	u32 reg;
+
+	reg = readl(&ring->cfg->size);
+	reg &= KNAV_RINGACC_CFG_RING_SIZE_MASK;
+	reg |= ring->size;
+	writel(reg, &ring->cfg->size);
 }
 
 static void k3_ringacc_ring_reconfig_qmode_raw(struct k3_nav_ring *ring, enum k3_nav_ring_mode mode)
-- 
2.34.1

[PATCH 05/10] dma: ti: k3-udma: Add support for native configuration of chan/flow

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

In absence of Device Manager (DM) services such as at R5 SPL stage,
driver will have to natively setup TCHAN/RCHAN/RFLOW cfg registers.
Existing UDMA driver performed the above mentioned configuration
for UDMA. Add similar configuration for PKTDMA here.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/dma/ti/k3-udma.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 8a62d63dfe..3bd1e7668b 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -2110,6 +2110,9 @@ static int bcdma_tisci_tx_channel_config(struct udma_chan *uc)
 	if (ret)
 		dev_err(ud->dev, "tchan%d cfg failed %d\n", tchan->id, ret);
 
+	if (IS_ENABLED(CONFIG_K3_DM_FW))
+		udma_alloc_tchan_raw(uc);
+
 	return ret;
 }
 
@@ -2158,6 +2161,9 @@ static int pktdma_tisci_rx_channel_config(struct udma_chan *uc)
 		dev_err(ud->dev, "flow%d config failed: %d\n", uc->rflow->id,
 			ret);
 
+	if (IS_ENABLED(CONFIG_K3_DM_FW))
+		udma_alloc_rchan_raw(uc);
+
 	return ret;
 }
 
-- 
2.34.1

[PATCH 06/10] arm: mach-k3: am62: Update SoC autogenerated data to enable Ethernet Boot

[Siddharth Vadapalli]

From: Andreas Dannenberg <dannenberg@ti.com>

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

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(-)

diff --git a/arch/arm/mach-k3/r5/am62x/clk-data.c b/arch/arm/mach-k3/r5/am62x/clk-data.c
index d7bfed0e03..880f05c40b 100644
--- a/arch/arm/mach-k3/r5/am62x/clk-data.c
+++ b/arch/arm/mach-k3/r5/am62x/clk-data.c
@@ -3,9 +3,9 @@
  * AM62X specific clock platform data
  *
  * This file is auto generated. Please do not hand edit and report any issues
- * to Dave Gerlach <d-gerlach@ti.com>.
+ * to Bryan Brattlof <bb@ti.com>.
  *
- * Copyright (C) 2020-2022 Texas Instruments Incorporated - https://www.ti.com/
+ * Copyright (C) 2020-2024 Texas Instruments Incorporated - https://www.ti.com/
  */
 
 #include <linux/clk-provider.h>
@@ -57,7 +57,7 @@ static const char * const sam62_pll_ctrl_wrap_mcu_0_sysclkout_clk_parents[] = {
 
 static const char * const clkout0_ctrl_out0_parents[] = {
 	"hsdiv4_16fft_main_2_hsdivout1_clk",
-	"hsdiv4_16fft_main_2_hsdivout1_clk10",
+	"hsdiv4_16fft_main_2_hsdivout1_clk",
 };
 
 static const char * const clk_32k_rc_sel_out0_parents[] = {
@@ -158,46 +158,45 @@ static const struct clk_data clk_list[] = {
 	CLK_FIXED_RATE("fss_ul_main_0_ospi_0_ospi_oclk_clk", 0, 0),
 	CLK_DIV("hsdiv0_16fft_mcu_32khz_gen_0_hsdivout0_clk", "gluelogic_hfosc0_clkout", 0x4508030, 0, 7, 0, 0),
 	CLK_FIXED_RATE("mshsi2c_main_0_porscl", 0, 0),
-	CLK_PLL("pllfracf_ssmod_16fft_main_0_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x680000, 0),
-	CLK_DIV("pllfracf_ssmod_16fft_main_0_foutpostdiv_clk_subdiv", "pllfracf_ssmod_16fft_main_0_foutvcop_clk", 0x680038, 16, 3, 0, CLK_DIVIDER_ONE_BASED),
-	CLK_DIV("pllfracf_ssmod_16fft_main_0_foutpostdiv_clk", "pllfracf_ssmod_16fft_main_0_foutpostdiv_clk_subdiv", 0x680038, 24, 3, 0, CLK_DIVIDER_ONE_BASED),
-	CLK_PLL("pllfracf_ssmod_16fft_main_1_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x681000, 0),
-	CLK_DIV("pllfracf_ssmod_16fft_main_1_foutpostdiv_clk_subdiv", "pllfracf_ssmod_16fft_main_1_foutvcop_clk", 0x681038, 16, 3, 0, CLK_DIVIDER_ONE_BASED),
-	CLK_DIV("pllfracf_ssmod_16fft_main_1_foutpostdiv_clk", "pllfracf_ssmod_16fft_main_1_foutpostdiv_clk_subdiv", 0x681038, 24, 3, 0, CLK_DIVIDER_ONE_BASED),
-	CLK_PLL("pllfracf_ssmod_16fft_main_12_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x68c000, 0),
-	CLK_PLL("pllfracf_ssmod_16fft_main_15_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x68f000, 0),
-	CLK_PLL("pllfracf_ssmod_16fft_main_2_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x682000, 0),
-	CLK_DIV("pllfracf_ssmod_16fft_main_2_foutpostdiv_clk_subdiv", "pllfracf_ssmod_16fft_main_2_foutvcop_clk", 0x682038, 16, 3, 0, CLK_DIVIDER_ONE_BASED),
-	CLK_DIV("pllfracf_ssmod_16fft_main_2_foutpostdiv_clk", "pllfracf_ssmod_16fft_main_2_foutpostdiv_clk_subdiv", 0x682038, 24, 3, 0, CLK_DIVIDER_ONE_BASED),
-	CLK_PLL("pllfracf_ssmod_16fft_main_8_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x688000, 0),
-	CLK_PLL("pllfracf_ssmod_16fft_mcu_0_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x4040000, 0),
-	CLK_DIV("postdiv1_16fft_main_1_hsdivout5_clk", "pllfracf_ssmod_16fft_main_1_foutpostdiv_clk", 0x681094, 0, 7, 0, 0),
-	CLK_DIV("postdiv4_16ff_main_0_hsdivout5_clk", "pllfracf_ssmod_16fft_main_0_foutpostdiv_clk", 0x680094, 0, 7, 0, 0),
-	CLK_DIV("postdiv4_16ff_main_0_hsdivout6_clk", "pllfracf_ssmod_16fft_main_0_foutpostdiv_clk", 0x680098, 0, 7, 0, 0),
-	CLK_DIV("postdiv4_16ff_main_0_hsdivout8_clk", "pllfracf_ssmod_16fft_main_0_foutpostdiv_clk", 0x6800a0, 0, 7, 0, 0),
-	CLK_DIV("postdiv4_16ff_main_2_hsdivout5_clk", "pllfracf_ssmod_16fft_main_2_foutpostdiv_clk", 0x682094, 0, 7, 0, 0),
-	CLK_DIV("postdiv4_16ff_main_2_hsdivout8_clk", "pllfracf_ssmod_16fft_main_2_foutpostdiv_clk", 0x6820a0, 0, 7, 0, 0),
-	CLK_DIV("postdiv4_16ff_main_2_hsdivout9_clk", "pllfracf_ssmod_16fft_main_2_foutpostdiv_clk", 0x6820a4, 0, 7, 0, 0),
+	CLK_PLL("pllfracf2_ssmod_16fft_main_0_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x680000, 0),
+	CLK_DIV("pllfracf2_ssmod_16fft_main_0_foutpostdiv_clk_subdiv", "pllfracf2_ssmod_16fft_main_0_foutvcop_clk", 0x680038, 16, 3, 0, CLK_DIVIDER_ONE_BASED),
+	CLK_DIV("pllfracf2_ssmod_16fft_main_0_foutpostdiv_clk", "pllfracf2_ssmod_16fft_main_0_foutpostdiv_clk_subdiv", 0x680038, 24, 3, 0, CLK_DIVIDER_ONE_BASED),
+	CLK_PLL_DEFFREQ("pllfracf2_ssmod_16fft_main_1_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x681000, 0, 1920000000),
+	CLK_DIV("pllfracf2_ssmod_16fft_main_1_foutpostdiv_clk_subdiv", "pllfracf2_ssmod_16fft_main_1_foutvcop_clk", 0x681038, 16, 3, 0, CLK_DIVIDER_ONE_BASED),
+	CLK_DIV("pllfracf2_ssmod_16fft_main_1_foutpostdiv_clk", "pllfracf2_ssmod_16fft_main_1_foutpostdiv_clk_subdiv", 0x681038, 24, 3, 0, CLK_DIVIDER_ONE_BASED),
+	CLK_PLL("pllfracf2_ssmod_16fft_main_12_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x68c000, 0),
+	CLK_PLL("pllfracf2_ssmod_16fft_main_15_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x68f000, 0),
+	CLK_PLL("pllfracf2_ssmod_16fft_main_2_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x682000, 0),
+	CLK_DIV("pllfracf2_ssmod_16fft_main_2_foutpostdiv_clk_subdiv", "pllfracf2_ssmod_16fft_main_2_foutvcop_clk", 0x682038, 16, 3, 0, CLK_DIVIDER_ONE_BASED),
+	CLK_DIV("pllfracf2_ssmod_16fft_main_2_foutpostdiv_clk", "pllfracf2_ssmod_16fft_main_2_foutpostdiv_clk_subdiv", 0x682038, 24, 3, 0, CLK_DIVIDER_ONE_BASED),
+	CLK_PLL("pllfracf2_ssmod_16fft_main_8_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x688000, 0),
+	CLK_PLL("pllfracf2_ssmod_16fft_mcu_0_foutvcop_clk", "gluelogic_hfosc0_clkout", 0x4040000, 0),
+	CLK_DIV("postdiv1_16fft_main_1_hsdivout5_clk", "pllfracf2_ssmod_16fft_main_1_foutpostdiv_clk", 0x681094, 0, 7, 0, 0),
+	CLK_DIV("postdiv4_16ff_main_0_hsdivout5_clk", "pllfracf2_ssmod_16fft_main_0_foutpostdiv_clk", 0x680094, 0, 7, 0, 0),
+	CLK_DIV("postdiv4_16ff_main_0_hsdivout6_clk", "pllfracf2_ssmod_16fft_main_0_foutpostdiv_clk", 0x680098, 0, 7, 0, 0),
+	CLK_DIV("postdiv4_16ff_main_0_hsdivout8_clk", "pllfracf2_ssmod_16fft_main_0_foutpostdiv_clk", 0x6800a0, 0, 7, 0, 0),
+	CLK_DIV("postdiv4_16ff_main_2_hsdivout5_clk", "pllfracf2_ssmod_16fft_main_2_foutpostdiv_clk", 0x682094, 0, 7, 0, 0),
+	CLK_DIV("postdiv4_16ff_main_2_hsdivout8_clk", "pllfracf2_ssmod_16fft_main_2_foutpostdiv_clk", 0x6820a0, 0, 7, 0, 0),
+	CLK_DIV("postdiv4_16ff_main_2_hsdivout9_clk", "pllfracf2_ssmod_16fft_main_2_foutpostdiv_clk", 0x6820a4, 0, 7, 0, 0),
 	CLK_MUX("main_emmcsd0_io_clklb_sel_out0", main_emmcsd0_io_clklb_sel_out0_parents, 2, 0x108160, 16, 1, 0),
 	CLK_MUX("main_emmcsd1_io_clklb_sel_out0", main_emmcsd1_io_clklb_sel_out0_parents, 2, 0x108168, 16, 1, 0),
 	CLK_MUX("main_ospi_loopback_clk_sel_out0", main_ospi_loopback_clk_sel_out0_parents, 2, 0x108500, 4, 1, 0),
 	CLK_MUX("main_usb0_refclk_sel_out0", main_usb0_refclk_sel_out0_parents, 2, 0x43008190, 0, 1, 0),
 	CLK_MUX("main_usb1_refclk_sel_out0", main_usb1_refclk_sel_out0_parents, 2, 0x43008194, 0, 1, 0),
-	CLK_DIV("hsdiv0_16fft_main_12_hsdivout0_clk", "pllfracf_ssmod_16fft_main_12_foutvcop_clk", 0x68c080, 0, 7, 0, 0),
-	CLK_DIV("hsdiv0_16fft_main_8_hsdivout0_clk", "pllfracf_ssmod_16fft_main_8_foutvcop_clk", 0x688080, 0, 7, 0, 0),
-	CLK_DIV("hsdiv1_16fft_main_15_hsdivout0_clk", "pllfracf_ssmod_16fft_main_15_foutvcop_clk", 0x68f080, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_0_hsdivout0_clk", "pllfracf_ssmod_16fft_main_0_foutvcop_clk", 0x680080, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_0_hsdivout1_clk", "pllfracf_ssmod_16fft_main_0_foutvcop_clk", 0x680084, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_0_hsdivout2_clk", "pllfracf_ssmod_16fft_main_0_foutvcop_clk", 0x680088, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_0_hsdivout3_clk", "pllfracf_ssmod_16fft_main_0_foutvcop_clk", 0x68008c, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_0_hsdivout4_clk", "pllfracf_ssmod_16fft_main_0_foutvcop_clk", 0x680090, 0, 7, 0, 0),
-	CLK_DIV_DEFFREQ("hsdiv4_16fft_main_1_hsdivout0_clk", "pllfracf_ssmod_16fft_main_1_foutvcop_clk", 0x681080, 0, 7, 0, 0, 192000000),
-	CLK_DIV("hsdiv4_16fft_main_1_hsdivout1_clk", "pllfracf_ssmod_16fft_main_1_foutvcop_clk", 0x681084, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_1_hsdivout2_clk", "pllfracf_ssmod_16fft_main_1_foutvcop_clk", 0x681088, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_2_hsdivout1_clk", "pllfracf_ssmod_16fft_main_2_foutvcop_clk", 0x682084, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_2_hsdivout1_clk10", "pllfracf_ssmod_16fft_main_2_foutvcop_clk", 0x682084, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_main_2_hsdivout2_clk", "pllfracf_ssmod_16fft_main_2_foutvcop_clk", 0x682088, 0, 7, 0, 0),
-	CLK_DIV("hsdiv4_16fft_mcu_0_hsdivout0_clk", "pllfracf_ssmod_16fft_mcu_0_foutvcop_clk", 0x4040080, 0, 7, 0, 0),
+	CLK_DIV("hsdiv0_16fft_main_12_hsdivout0_clk", "pllfracf2_ssmod_16fft_main_12_foutvcop_clk", 0x68c080, 0, 7, 0, 0),
+	CLK_DIV("hsdiv0_16fft_main_8_hsdivout0_clk", "pllfracf2_ssmod_16fft_main_8_foutvcop_clk", 0x688080, 0, 7, 0, 0),
+	CLK_DIV("hsdiv1_16fft_main_15_hsdivout0_clk", "pllfracf2_ssmod_16fft_main_15_foutvcop_clk", 0x68f080, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_0_hsdivout0_clk", "pllfracf2_ssmod_16fft_main_0_foutvcop_clk", 0x680080, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_0_hsdivout1_clk", "pllfracf2_ssmod_16fft_main_0_foutvcop_clk", 0x680084, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_0_hsdivout2_clk", "pllfracf2_ssmod_16fft_main_0_foutvcop_clk", 0x680088, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_0_hsdivout3_clk", "pllfracf2_ssmod_16fft_main_0_foutvcop_clk", 0x68008c, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_0_hsdivout4_clk", "pllfracf2_ssmod_16fft_main_0_foutvcop_clk", 0x680090, 0, 7, 0, 0),
+	CLK_DIV_DEFFREQ("hsdiv4_16fft_main_1_hsdivout0_clk", "pllfracf2_ssmod_16fft_main_1_foutvcop_clk", 0x681080, 0, 7, 0, 0, 192000000),
+	CLK_DIV("hsdiv4_16fft_main_1_hsdivout1_clk", "pllfracf2_ssmod_16fft_main_1_foutvcop_clk", 0x681084, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_1_hsdivout2_clk", "pllfracf2_ssmod_16fft_main_1_foutvcop_clk", 0x681088, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_2_hsdivout1_clk", "pllfracf2_ssmod_16fft_main_2_foutvcop_clk", 0x682084, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_main_2_hsdivout2_clk", "pllfracf2_ssmod_16fft_main_2_foutvcop_clk", 0x682088, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_mcu_0_hsdivout0_clk", "pllfracf2_ssmod_16fft_mcu_0_foutvcop_clk", 0x4040080, 0, 7, 0, 0),
 	CLK_MUX_PLLCTRL("sam62_pll_ctrl_wrap_main_0_sysclkout_clk", sam62_pll_ctrl_wrap_main_0_sysclkout_clk_parents, 2, 0x410000, 0),
 	CLK_DIV("sam62_pll_ctrl_wrap_main_0_chip_div1_clk_clk", "sam62_pll_ctrl_wrap_main_0_sysclkout_clk", 0x410118, 0, 5, 0, 0),
 	CLK_MUX_PLLCTRL("sam62_pll_ctrl_wrap_mcu_0_sysclkout_clk", sam62_pll_ctrl_wrap_mcu_0_sysclkout_clk_parents, 2, 0x4020000, 0),
@@ -213,7 +212,7 @@ static const struct clk_data clk_list[] = {
 	CLK_MUX("wkup_clkout_sel_out0", wkup_clkout_sel_out0_parents, 8, 0x43008020, 0, 3, 0),
 	CLK_MUX("wkup_clksel_out0", wkup_clksel_out0_parents, 2, 0x43008010, 0, 1, 0),
 	CLK_MUX("main_usart0_fclk_sel_out0", main_usart0_fclk_sel_out0_parents, 2, 0x108280, 0, 1, 0),
-	CLK_DIV("hsdiv4_16fft_mcu_0_hsdivout1_clk", "pllfracf_ssmod_16fft_mcu_0_foutvcop_clk", 0x4040084, 0, 7, 0, 0),
+	CLK_DIV("hsdiv4_16fft_mcu_0_hsdivout1_clk", "pllfracf2_ssmod_16fft_mcu_0_foutvcop_clk", 0x4040084, 0, 7, 0, 0),
 	CLK_FIXED_RATE("mshsi2c_wkup_0_porscl", 0, 0),
 	CLK_DIV("sam62_pll_ctrl_wrap_main_0_chip_div24_clk_clk", "sam62_pll_ctrl_wrap_main_0_sysclkout_clk", 0x41011c, 0, 5, 0, 0),
 	CLK_DIV("sam62_pll_ctrl_wrap_mcu_0_chip_div24_clk_clk", "sam62_pll_ctrl_wrap_mcu_0_sysclkout_clk", 0x402011c, 0, 5, 0, 0),
@@ -314,7 +313,7 @@ static const struct dev_clk soc_dev_clk_data[] = {
 	DEV_CLK(146, 5, "sam62_pll_ctrl_wrap_main_0_chip_div1_clk_clk"),
 	DEV_CLK(157, 20, "clkout0_ctrl_out0"),
 	DEV_CLK(157, 21, "hsdiv4_16fft_main_2_hsdivout1_clk"),
-	DEV_CLK(157, 22, "hsdiv4_16fft_main_2_hsdivout1_clk10"),
+	DEV_CLK(157, 22, "hsdiv4_16fft_main_2_hsdivout1_clk"),
 	DEV_CLK(157, 24, "sam62_pll_ctrl_wrap_main_0_chip_div1_clk_clk"),
 	DEV_CLK(157, 25, "board_0_ddr0_ck0_out"),
 	DEV_CLK(157, 40, "mshsi2c_main_0_porscl"),
-- 
2.34.1

Re: [PATCH 06/10] arm: mach-k3: am62: Update SoC autogenerated data to enable Ethernet Boot

[Nishanth Menon]

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(-)
> 
[...]
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

[PATCH 07/10] arm: mach-k3: am625_init: Probe AM65 CPSW NUSS

[Siddharth Vadapalli]

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().

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 arch/arm/mach-k3/am625_init.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mach-k3/am625_init.c b/arch/arm/mach-k3/am625_init.c
index 6c96e88114..b89dd206e5 100644
--- a/arch/arm/mach-k3/am625_init.c
+++ b/arch/arm/mach-k3/am625_init.c
@@ -209,6 +209,16 @@ void board_init_f(ulong dummy)
 		if (ret)
 			panic("DRAM init failed: %d\n", ret);
 	}
+
+	if (IS_ENABLED(CONFIG_SPL_ETH) && IS_ENABLED(CONFIG_TI_AM65_CPSW_NUSS) &&
+	    spl_boot_device() == BOOT_DEVICE_ETHERNET) {
+		struct udevice *cpswdev;
+
+		if (uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(am65_cpsw_nuss),
+						&cpswdev))
+			printf("Failed to probe am65_cpsw_nuss driver\n");
+	}
+
 	spl_enable_cache();
 }
 
-- 
2.34.1

Re: [PATCH 07/10] arm: mach-k3: am625_init: Probe AM65 CPSW NUSS

[Nishanth Menon]

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?

> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  arch/arm/mach-k3/am625_init.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/mach-k3/am625_init.c b/arch/arm/mach-k3/am625_init.c
> index 6c96e88114..b89dd206e5 100644
> --- a/arch/arm/mach-k3/am625_init.c
> +++ b/arch/arm/mach-k3/am625_init.c
> @@ -209,6 +209,16 @@ void board_init_f(ulong dummy)
>  		if (ret)
>  			panic("DRAM init failed: %d\n", ret);
>  	}
> +
> +	if (IS_ENABLED(CONFIG_SPL_ETH) && IS_ENABLED(CONFIG_TI_AM65_CPSW_NUSS) &&
> +	    spl_boot_device() == BOOT_DEVICE_ETHERNET) {
> +		struct udevice *cpswdev;
> +
> +		if (uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(am65_cpsw_nuss),
> +						&cpswdev))
> +			printf("Failed to probe am65_cpsw_nuss driver\n");
> +	}
> +


-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 07/10] arm: mach-k3: am625_init: Probe AM65 CPSW NUSS

[Chintan Vankar]

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 ?
>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>   arch/arm/mach-k3/am625_init.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/mach-k3/am625_init.c b/arch/arm/mach-k3/am625_init.c
>> index 6c96e88114..b89dd206e5 100644
>> --- a/arch/arm/mach-k3/am625_init.c
>> +++ b/arch/arm/mach-k3/am625_init.c
>> @@ -209,6 +209,16 @@ void board_init_f(ulong dummy)
>>   		if (ret)
>>   			panic("DRAM init failed: %d\n", ret);
>>   	}
>> +
>> +	if (IS_ENABLED(CONFIG_SPL_ETH) && IS_ENABLED(CONFIG_TI_AM65_CPSW_NUSS) &&
>> +	    spl_boot_device() == BOOT_DEVICE_ETHERNET) {
>> +		struct udevice *cpswdev;
>> +
>> +		if (uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(am65_cpsw_nuss),
>> +						&cpswdev))
>> +			printf("Failed to probe am65_cpsw_nuss driver\n");
>> +	}
>> +
>

Re: [PATCH 07/10] arm: mach-k3: am625_init: Probe AM65 CPSW NUSS

[Nishanth Menon]

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/
?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 07/10] arm: mach-k3: am625_init: Probe AM65 CPSW NUSS

[Chintan Vankar]

On 24/01/24 02:27, Nishanth Menon wrote:
> 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?
Hello,
In board_init_f(), the ESM driver which is modeled as UClASS_MISC driver
seems to be probed explicitly. So it appears to me that the DM framework 
is probably not capable of handling probe of UCLASS_MISC. The am65-cpsw 
driver is also a UCLASS_MISC driver because of which it has to be probed 
similar to the ESM driver. Can someone suggest me if there is a way to 
trigger probe of all UCLASS_MISC drivers using the DM framework or 
am65-cpsw should be probed in this way only ?
> 
>> 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/
> ?
> 

[PATCH 08/10] configs: am62: Add configs for enabling ETHBOOT in R5SPL

[Siddharth Vadapalli]

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 +++++++++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 configs/am62x_evm_r5_ethboot_defconfig

diff --git a/configs/am62x_evm_r5_ethboot_defconfig b/configs/am62x_evm_r5_ethboot_defconfig
new file mode 100644
index 0000000000..4912f97d7c
--- /dev/null
+++ b/configs/am62x_evm_r5_ethboot_defconfig
@@ -0,0 +1,110 @@
+CONFIG_ARM=y
+CONFIG_ARCH_K3=y
+CONFIG_SYS_MALLOC_F_LEN=0x9000
+CONFIG_SPL_GPIO=y
+CONFIG_SPL_LIBCOMMON_SUPPORT=y
+CONFIG_SPL_LIBGENERIC_SUPPORT=y
+CONFIG_NR_DRAM_BANKS=2
+CONFIG_SOC_K3_AM625=y
+CONFIG_TARGET_AM625_R5_EVM=y
+CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
+CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x43c3a7f0
+CONFIG_ENV_SIZE=0x20000
+CONFIG_DM_GPIO=y
+CONFIG_DEFAULT_DEVICE_TREE="k3-am625-r5-sk"
+CONFIG_SPL_TEXT_BASE=0x43c00000
+CONFIG_DM_RESET=y
+CONFIG_SPL_MMC=y
+CONFIG_SPL_SERIAL=y
+CONFIG_SPL_DRIVERS_MISC=y
+CONFIG_SPL_STACK_R_ADDR=0x82000000
+CONFIG_SPL_SYS_MALLOC_F_LEN=0x7000
+CONFIG_SPL_SIZE_LIMIT=0x3A7F0
+CONFIG_SPL_SIZE_LIMIT_PROVIDE_STACK=0x3500
+CONFIG_SPL_LIBDISK_SUPPORT=y
+CONFIG_SPL_LOAD_FIT=y
+CONFIG_SPL_LOAD_FIT_ADDRESS=0x80080000
+# CONFIG_DISPLAY_CPUINFO is not set
+CONFIG_SPL_SIZE_LIMIT_SUBTRACT_GD=y
+CONFIG_SPL_SIZE_LIMIT_SUBTRACT_MALLOC=y
+CONFIG_SPL_MAX_SIZE=0x3B000
+CONFIG_SPL_PAD_TO=0x0
+CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
+CONFIG_SPL_BSS_START_ADDR=0x43c3b000
+CONFIG_SPL_BSS_MAX_SIZE=0x3000
+CONFIG_SPL_SYS_REPORT_STACK_F_USAGE=y
+CONFIG_SPL_BOARD_INIT=y
+CONFIG_SPL_SYS_MALLOC_SIMPLE=y
+CONFIG_SPL_STACK_R=y
+CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x200000
+CONFIG_SPL_SEPARATE_BSS=y
+CONFIG_SPL_EARLY_BSS=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x400
+CONFIG_SPL_DMA=y
+CONFIG_SPL_ENV_SUPPORT=y
+CONFIG_SPL_ETH=y
+CONFIG_SPL_I2C=y
+CONFIG_SPL_DM_MAILBOX=y
+CONFIG_SPL_NET=y
+CONFIG_SPL_NET_VCI_STRING="AM62X U-Boot R5 SPL"
+CONFIG_SPL_DM_RESET=y
+CONFIG_SPL_POWER_DOMAIN=y
+CONFIG_SPL_RAM_SUPPORT=y
+CONFIG_SPL_RAM_DEVICE=y
+CONFIG_SPL_REMOTEPROC=y
+CONFIG_SPL_YMODEM_SUPPORT=y
+CONFIG_HUSH_PARSER=y
+CONFIG_CMD_ASKENV=y
+CONFIG_CMD_GPT=y
+CONFIG_CMD_MMC=y
+CONFIG_CMD_REMOTEPROC=y
+# CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_TIME=y
+CONFIG_CMD_FAT=y
+CONFIG_OF_CONTROL=y
+CONFIG_SPL_OF_CONTROL=y
+CONFIG_SPL_MULTI_DTB_FIT=y
+CONFIG_SPL_MULTI_DTB_FIT_NO_COMPRESSION=y
+CONFIG_SYS_RELOC_GD_ENV_ADDR=y
+CONFIG_SPL_DM=y
+CONFIG_SPL_DM_SEQ_ALIAS=y
+CONFIG_REGMAP=y
+CONFIG_SPL_REGMAP=y
+CONFIG_SPL_SYSCON=y
+CONFIG_SPL_OF_TRANSLATE=y
+CONFIG_CLK=y
+CONFIG_SPL_CLK=y
+CONFIG_SPL_CLK_CCF=y
+CONFIG_SPL_CLK_K3_PLL=y
+CONFIG_SPL_CLK_K3=y
+CONFIG_DMA_CHANNELS=y
+CONFIG_TI_K3_NAVSS_UDMA=y
+CONFIG_TI_SCI_PROTOCOL=y
+CONFIG_DA8XX_GPIO=y
+CONFIG_DM_I2C=y
+CONFIG_DM_MAILBOX=y
+CONFIG_K3_SEC_PROXY=y
+CONFIG_PHY_TI_DP83867=y
+CONFIG_TI_AM65_CPSW_NUSS=y
+CONFIG_PINCTRL=y
+# CONFIG_PINCTRL_GENERIC is not set
+CONFIG_SPL_PINCTRL=y
+# CONFIG_SPL_PINCTRL_GENERIC is not set
+CONFIG_PINCTRL_SINGLE=y
+CONFIG_POWER_DOMAIN=y
+CONFIG_TI_POWER_DOMAIN=y
+CONFIG_K3_SYSTEM_CONTROLLER=y
+CONFIG_REMOTEPROC_TI_K3_ARM64=y
+CONFIG_RESET_TI_SCI=y
+CONFIG_SPECIFY_CONSOLE_INDEX=y
+CONFIG_DM_SERIAL=y
+CONFIG_SOC_DEVICE=y
+CONFIG_SOC_DEVICE_TI_K3=y
+CONFIG_SOC_TI=y
+CONFIG_TIMER=y
+CONFIG_SPL_TIMER=y
+CONFIG_OMAP_TIMER=y
+CONFIG_LIB_RATIONAL=y
+CONFIG_SPL_LIB_RATIONAL=y
-- 
2.34.1

Re: [PATCH 08/10] configs: am62: Add configs for enabling ETHBOOT in R5SPL

[Nishanth Menon]

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.
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 08/10] configs: am62: Add configs for enabling ETHBOOT in R5SPL

[Siddharth Vadapalli]

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.

-- 
Regards,
Siddharth.

[PATCH 09/10] configs: am62x_evm_a53_defconfig: Enable configs required for Ethboot

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

Enable config options needed to support Ethernet boot on AM62x SK.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 configs/am62x_evm_a53_defconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig
index aa96c1b312..5c56b17a3e 100644
--- a/configs/am62x_evm_a53_defconfig
+++ b/configs/am62x_evm_a53_defconfig
@@ -17,6 +17,7 @@ CONFIG_OF_LIBFDT_OVERLAY=y
 CONFIG_DM_RESET=y
 CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
+CONFIG_SPL_DRIVERS_MISC=y
 CONFIG_SPL_STACK_R_ADDR=0x82000000
 CONFIG_SPL_SIZE_LIMIT=0x40000
 CONFIG_SPL_SIZE_LIMIT_PROVIDE_STACK=0x800
@@ -37,13 +38,19 @@ CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
 CONFIG_SPL_BSS_START_ADDR=0x80c80000
 CONFIG_SPL_BSS_MAX_SIZE=0x80000
 CONFIG_SPL_SYS_REPORT_STACK_F_USAGE=y
+CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x1400
+CONFIG_SPL_DMA=y
+CONFIG_SPL_ENV_SUPPORT=y
+CONFIG_SPL_ETH=y
 CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="u-boot.img"
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_SPI_FLASH=y
+CONFIG_SPL_NET=y
+CONFIG_SPL_NET_VCI_STRING="AM62X U-Boot A53 SPL"
 CONFIG_SPL_POWER_DOMAIN=y
 # CONFIG_SPL_SPI_FLASH_TINY is not set
 CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT=y
@@ -61,6 +68,7 @@ CONFIG_SPL_DM=y
 CONFIG_SPL_DM_SEQ_ALIAS=y
 CONFIG_REGMAP=y
 CONFIG_SPL_REGMAP=y
+CONFIG_SPL_SYSCON=y
 CONFIG_SPL_OF_TRANSLATE=y
 CONFIG_CLK=y
 CONFIG_SPL_CLK=y
-- 
2.34.1

Re: [PATCH 09/10] configs: am62x_evm_a53_defconfig: Enable configs required for Ethboot

[Nishanth Menon]

On 12:17-20240112, Siddharth Vadapalli wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
> 
> Enable config options needed to support Ethernet boot on AM62x SK.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  configs/am62x_evm_a53_defconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig
> index aa96c1b312..5c56b17a3e 100644
> --- a/configs/am62x_evm_a53_defconfig
> +++ b/configs/am62x_evm_a53_defconfig
> @@ -17,6 +17,7 @@ CONFIG_OF_LIBFDT_OVERLAY=y
>  CONFIG_DM_RESET=y
>  CONFIG_SPL_MMC=y
>  CONFIG_SPL_SERIAL=y
> +CONFIG_SPL_DRIVERS_MISC=y
>  CONFIG_SPL_STACK_R_ADDR=0x82000000
>  CONFIG_SPL_SIZE_LIMIT=0x40000
>  CONFIG_SPL_SIZE_LIMIT_PROVIDE_STACK=0x800
> @@ -37,13 +38,19 @@ CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
>  CONFIG_SPL_BSS_START_ADDR=0x80c80000
>  CONFIG_SPL_BSS_MAX_SIZE=0x80000
>  CONFIG_SPL_SYS_REPORT_STACK_F_USAGE=y
> +CONFIG_SPL_BOARD_INIT=y
>  CONFIG_SPL_SYS_MALLOC_SIMPLE=y
>  CONFIG_SPL_STACK_R=y
>  CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
>  CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x1400
> +CONFIG_SPL_DMA=y
> +CONFIG_SPL_ENV_SUPPORT=y
> +CONFIG_SPL_ETH=y
>  CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="u-boot.img"
>  CONFIG_SPL_DM_MAILBOX=y
>  CONFIG_SPL_DM_SPI_FLASH=y
> +CONFIG_SPL_NET=y
> +CONFIG_SPL_NET_VCI_STRING="AM62X U-Boot A53 SPL"
>  CONFIG_SPL_POWER_DOMAIN=y
>  # CONFIG_SPL_SPI_FLASH_TINY is not set
>  CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT=y
> @@ -61,6 +68,7 @@ CONFIG_SPL_DM=y
>  CONFIG_SPL_DM_SEQ_ALIAS=y
>  CONFIG_REGMAP=y
>  CONFIG_SPL_REGMAP=y
> +CONFIG_SPL_SYSCON=y
>  CONFIG_SPL_OF_TRANSLATE=y
>  CONFIG_CLK=y
>  CONFIG_SPL_CLK=y
> -- 
> 2.34.1
> 

NAK again - why cant we use config fragments?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 09/10] configs: am62x_evm_a53_defconfig: Enable configs required for Ethboot

[Siddharth Vadapalli]

On 12/01/24 18:03, Nishanth Menon wrote:
> On 12:17-20240112, Siddharth Vadapalli wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> Enable config options needed to support Ethernet boot on AM62x SK.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  configs/am62x_evm_a53_defconfig | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig
>> index aa96c1b312..5c56b17a3e 100644
>> --- a/configs/am62x_evm_a53_defconfig
>> +++ b/configs/am62x_evm_a53_defconfig
>> @@ -17,6 +17,7 @@ CONFIG_OF_LIBFDT_OVERLAY=y
>>  CONFIG_DM_RESET=y
>>  CONFIG_SPL_MMC=y
>>  CONFIG_SPL_SERIAL=y
>> +CONFIG_SPL_DRIVERS_MISC=y
>>  CONFIG_SPL_STACK_R_ADDR=0x82000000
>>  CONFIG_SPL_SIZE_LIMIT=0x40000
>>  CONFIG_SPL_SIZE_LIMIT_PROVIDE_STACK=0x800
>> @@ -37,13 +38,19 @@ CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
>>  CONFIG_SPL_BSS_START_ADDR=0x80c80000
>>  CONFIG_SPL_BSS_MAX_SIZE=0x80000
>>  CONFIG_SPL_SYS_REPORT_STACK_F_USAGE=y
>> +CONFIG_SPL_BOARD_INIT=y
>>  CONFIG_SPL_SYS_MALLOC_SIMPLE=y
>>  CONFIG_SPL_STACK_R=y
>>  CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
>>  CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x1400
>> +CONFIG_SPL_DMA=y
>> +CONFIG_SPL_ENV_SUPPORT=y
>> +CONFIG_SPL_ETH=y
>>  CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="u-boot.img"
>>  CONFIG_SPL_DM_MAILBOX=y
>>  CONFIG_SPL_DM_SPI_FLASH=y
>> +CONFIG_SPL_NET=y
>> +CONFIG_SPL_NET_VCI_STRING="AM62X U-Boot A53 SPL"
>>  CONFIG_SPL_POWER_DOMAIN=y
>>  # CONFIG_SPL_SPI_FLASH_TINY is not set
>>  CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT=y
>> @@ -61,6 +68,7 @@ CONFIG_SPL_DM=y
>>  CONFIG_SPL_DM_SEQ_ALIAS=y
>>  CONFIG_REGMAP=y
>>  CONFIG_SPL_REGMAP=y
>> +CONFIG_SPL_SYSCON=y
>>  CONFIG_SPL_OF_TRANSLATE=y
>>  CONFIG_CLK=y
>>  CONFIG_SPL_CLK=y
>> -- 
>> 2.34.1
>>
> 
> NAK again - why cant we use config fragments?

Ok. I will use config fragments.

> 

-- 
Regards,
Siddharth.

[PATCH 10/10] arm: dts: k3-am625-r5-sk: Enable DM services for main_pktdma

[Siddharth Vadapalli]

Enable DM services for main_pktdma during R5 SPL stage.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 arch/arm/dts/k3-am625-r5-sk.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/dts/k3-am625-r5-sk.dts b/arch/arm/dts/k3-am625-r5-sk.dts
index 6b9f40e555..0912b953db 100644
--- a/arch/arm/dts/k3-am625-r5-sk.dts
+++ b/arch/arm/dts/k3-am625-r5-sk.dts
@@ -83,3 +83,8 @@
 	reg = <0x00 0x0fc40000 0x00 0x100>,
 	      <0x00 0x60000000 0x00 0x08000000>;
 };
+
+&main_pktdma {
+	ti,sci = <&dm_tifs>;
+	bootph-all;
+};
-- 
2.34.1

Re: [PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Nishanth Menon]

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?

> 
> This series is based on commit:
> f28a77589e Merge tag 'dm-next-7jan23' of https://source.denx.de/u-boot/custodians/u-boot-dm into next
> of the next branch of u-boot.
> 
> Regards,
> Siddharth.
> 
> Andreas Dannenberg (1):
>   arm: mach-k3: am62: Update SoC autogenerated data to enable Ethernet
>     Boot
> 
> Kishon Vijay Abraham I (7):
>   board: ti: am62x: Init DRAM size in R5/A53 SPL
>   firmware: ti_sci: Add No-OP for "RX_FL_CFG"
>   soc: ti: k3-navss-ringacc: Initialize base address of ring cfg
>     registers
>   dma: ti: k3-udma: Add support for native configuration of chan/flow
>   arm: mach-k3: am625_init: Probe AM65 CPSW NUSS
>   configs: am62: Add configs for enabling ETHBOOT in R5SPL
>   configs: am62x_evm_a53_defconfig: Enable configs required for Ethboot
> 
> Siddharth Vadapalli (1):
>   arm: dts: k3-am625-r5-sk: Enable DM services for main_pktdma
> 
> Vignesh Raghavendra (1):
>   soc: ti: k3-navss-ringacc: Fix reset ring API
> 
>  arch/arm/dts/k3-am625-r5-sk.dts          |   5 ++
>  arch/arm/mach-k3/am625_init.c            |  10 +++
>  arch/arm/mach-k3/r5/am62x/clk-data.c     |  79 ++++++++--------
>  board/ti/am62x/evm.c                     |   3 +
>  configs/am62x_evm_a53_defconfig          |   8 ++
>  configs/am62x_evm_r5_ethboot_defconfig   | 110 +++++++++++++++++++++++
>  drivers/dma/ti/k3-udma.c                 |   6 ++
>  drivers/firmware/ti_sci.c                |   8 +-
>  drivers/soc/ti/k3-navss-ringacc-u-boot.c |   9 +-
>  drivers/soc/ti/k3-navss-ringacc.c        |   7 +-
>  10 files changed, 202 insertions(+), 43 deletions(-)
>  create mode 100644 configs/am62x_evm_r5_ethboot_defconfig
> 
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Siddharth Vadapalli]

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?

...

-- 
Regards,
Siddharth.

Re: [PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Nishanth Menon]

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.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Siddharth Vadapalli]

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.

> 

-- 
Regards,
Siddharth.

Re: [PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Nishanth Menon]

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.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 00/10] Add support for Ethernet Boot on SK-AM62

[Siddharth Vadapalli]

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.

> 

-- 
Regards,
Siddharth.