Re: [PATCH v3 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled

[Santhosh Kumar K]

Hi, Wadim,

Thanks for the review.

On 30/05/24 17:48, Wadim Egorov wrote:

Hi Santhosh,

thanks for this series!

Am 23.05.24 um 07:04 schrieb Santhosh Kumar K:

As there are few redundant functions in board/ti/*/evm.c files, pull
them to a common location of access to reuse and include the common file
to access the functions.

Call k3-ddrss driver through fixup_ddr_driver_for_ecc() to fixup the
device tree and resize the available amount of DDR, if ECC is enabled.
Otherwise, fixup the device tree using the regular
fdt_fixup_memory_banks().

Also call dram_init_banksize() after every call to
fixup_ddr_driver_for_ecc() is made so that gd->bd is populated
correctly.

Ensure that fixup_ddr_driver_for_ecc() is agnostic to the number of DDR
controllers present.

Signed-off-by: Santhosh Kumar K <s...@ti.com>
Signed-off-by: Neha Malcom Francis <n-fran...@ti.com>
---

[...]

+++ b/board/ti/common/k3-ddr-init.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*

+ * Copyright (C) 2023, Texas Instruments Incorporated - https://www.ti.com/

+ */
+
+#include <fdt_support.h>
+#include <dm/uclass.h>
+#include <k3-ddrss.h>
+#include <spl.h>
+
+#include "k3-ddr-init.h"
+
+int dram_init(void)
+{
+    s32 ret;
+
+    ret = fdtdec_setup_mem_size_base_lowest();
+    if (ret)
+        printf("Error setting up mem size and base. %d\n", ret);
+
+    return ret;
+}
+
+int dram_init_banksize(void)
+{
+    s32 ret;
+
+    ret = fdtdec_setup_memory_banksize();
+    if (ret)
+        printf("Error setting up memory banksize. %d\n", ret);
+
+    return ret;
+}

I'm wondering if we can generalize more.

What do you say if we keep dram_init() & dram_init_banksize() in the board code and move fixup_ddr_driver_for_ecc() & fixup_memory_node() to mach-k3 and make them available for all K3 based boards?

It looks like I will reuse the code for our boards and I assume other vendors too.

Regards,
Wadim

Yeah, makes sense, will work on it and post v4.

Regards,
Santhosh.

+
+#if defined(CONFIG_SPL_BUILD)
+
+void fixup_ddr_driver_for_ecc(struct spl_image_info *spl_image)
+{
+    struct udevice *dev;
+    int ret, ctr = 1;
+
+    dram_init_banksize();
+
+    ret = uclass_get_device(UCLASS_RAM, 0, &dev);
+    if (ret)
+        panic("Cannnot get RAM device for ddr size fixup: %d\n", ret);
+
+    ret = k3_ddrss_ddr_fdt_fixup(dev, spl_image->fdt_addr, gd->bd);
+    if (ret)
+        printf("Error fixing up ddr node for ECC use! %d\n", ret);
+
+    dram_init_banksize();
+
+    ret = uclass_next_device_err(&dev);
+
+    while (!ret) {
+        ret = k3_ddrss_ddr_fdt_fixup(dev, spl_image->fdt_addr, gd->bd);
+        if (ret)

+            printf("Error fixing up ddr node %d for ECC use! %d\n", ctr, ret);

+
+        dram_init_banksize();
+        ret = uclass_next_device_err(&dev);
+        ctr++;
+    }
+}
+
+void fixup_memory_node(struct spl_image_info *spl_image)
+{
+    u64 start[CONFIG_NR_DRAM_BANKS];
+    u64 size[CONFIG_NR_DRAM_BANKS];
+    int bank;
+    int ret;
+
+    dram_init();
+    dram_init_banksize();
+
+    for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
+        start[bank] = gd->bd->bi_dram[bank].start;
+        size[bank] = gd->bd->bi_dram[bank].size;
+    }
+
+    ret = fdt_fixup_memory_banks(spl_image->fdt_addr, start, size,
+                     CONFIG_NR_DRAM_BANKS);
+
+    if (ret)
+        printf("Error fixing up memory node! %d\n", ret);
+}
+
+#endif

diff --git a/board/ti/common/k3-ddr-init.h b/board/ti/common/k3-ddr-init.h

new file mode 100644
index 000000000000..9d1826815dfd
--- /dev/null
+++ b/board/ti/common/k3-ddr-init.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*

+ * Copyright (C) 2023, Texas Instruments Incorporated - https://www.ti.com/

+ */
+
+#ifndef __K3_DDR_INIT_H
+#define __K3_DDR_INIT_H
+
+int dram_init(void);
+int dram_init_banksize(void);
+
+void fixup_ddr_driver_for_ecc(struct spl_image_info *spl_image);
+void fixup_memory_node(struct spl_image_info *spl_image);
+
+#endif /* __K3_DDR_INIT_H */
diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
index 539eaf47186a..e0cd8529bc2b 100644
--- a/board/ti/j721e/evm.c
+++ b/board/ti/j721e/evm.c
@@ -17,6 +17,7 @@
  #include "../common/board_detect.h"
  #include "../common/fdt_ops.h"
+#include "../common/k3-ddr-init.h"
  #define board_is_j721e_som()    (board_ti_k3_is("J721EX-PM1-SOM") || \
                   board_ti_k3_is("J721EX-PM2-SOM"))
@@ -37,17 +38,6 @@ int board_init(void)
      return 0;
  }
-int dram_init(void)
-{
-#ifdef CONFIG_PHYS_64BIT
-    gd->ram_size = 0x100000000;
-#else
-    gd->ram_size = 0x80000000;
-#endif
-
-    return 0;
-}
-
  phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
  {
  #ifdef CONFIG_PHYS_64BIT

@@ -59,23 +49,6 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size)

      return gd->ram_top;
  }
-int dram_init_banksize(void)
-{
-    /* Bank 0 declares the memory available in the DDR low region */
-    gd->bd->bi_dram[0].start = 0x80000000;
-    gd->bd->bi_dram[0].size = 0x80000000;
-    gd->ram_size = 0x80000000;
-
-#ifdef CONFIG_PHYS_64BIT
-    /* Bank 1 declares the memory available in the DDR high region */
-    gd->bd->bi_dram[1].start = 0x880000000;
-    gd->bd->bi_dram[1].size = 0x80000000;
-    gd->ram_size = 0x100000000;
-#endif
-
-    return 0;
-}
-
  #ifdef CONFIG_SPL_LOAD_FIT
  int board_fit_config_name_match(const char *name)
  {
diff --git a/board/ti/j721s2/evm.c b/board/ti/j721s2/evm.c
index 5a0281d6b483..fca30107cddb 100644
--- a/board/ti/j721s2/evm.c
+++ b/board/ti/j721s2/evm.c
@@ -24,6 +24,7 @@
  #include "../common/board_detect.h"
  #include "../common/fdt_ops.h"
+#include "../common/k3-ddr-init.h"
  DECLARE_GLOBAL_DATA_PTR;
@@ -32,17 +33,6 @@ int board_init(void)
      return 0;
  }
-int dram_init(void)
-{
-#ifdef CONFIG_PHYS_64BIT
-    gd->ram_size = 0x100000000;
-#else
-    gd->ram_size = 0x80000000;
-#endif
-
-    return 0;
-}
-
  phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
  {
  #ifdef CONFIG_PHYS_64BIT

@@ -54,22 +44,17 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size)

      return gd->ram_top;
  }
-int dram_init_banksize(void)
+#if defined(CONFIG_SPL_BUILD)
+void spl_perform_fixups(struct spl_image_info *spl_image)
  {
-    /* Bank 0 declares the memory available in the DDR low region */
-    gd->bd->bi_dram[0].start = 0x80000000;
-    gd->bd->bi_dram[0].size = 0x7fffffff;
-    gd->ram_size = 0x80000000;
-
-#ifdef CONFIG_PHYS_64BIT
-    /* Bank 1 declares the memory available in the DDR high region */
-    gd->bd->bi_dram[1].start = 0x880000000;
-    gd->bd->bi_dram[1].size = 0x37fffffff;
-    gd->ram_size = 0x400000000;
-#endif
-
-    return 0;
+    if (IS_ENABLED(CONFIG_K3_DDRSS)) {
+        if (IS_ENABLED(CONFIG_K3_INLINE_ECC))
+            fixup_ddr_driver_for_ecc(spl_image);
+    } else {
+        fixup_memory_node(spl_image);
+    }
  }
+#endif
  #ifdef CONFIG_TI_I2C_BOARD_DETECT
  /*
diff --git a/board/ti/j784s4/evm.c b/board/ti/j784s4/evm.c
index aed0ea5b9495..73056a16dd5f 100644
--- a/board/ti/j784s4/evm.c
+++ b/board/ti/j784s4/evm.c
@@ -10,6 +10,7 @@
  #include <init.h>
  #include <spl.h>
  #include "../common/fdt_ops.h"
+#include "../common/k3-ddr-init.h"
  DECLARE_GLOBAL_DATA_PTR;
@@ -18,15 +19,17 @@ int board_init(void)
      return 0;
  }
-int dram_init(void)
+#if defined(CONFIG_SPL_BUILD)
+void spl_perform_fixups(struct spl_image_info *spl_image)
  {
-    return fdtdec_setup_mem_size_base();
-}
-
-int dram_init_banksize(void)
-{
-    return fdtdec_setup_memory_banksize();
+    if (IS_ENABLED(CONFIG_K3_DDRSS)) {
+        if (IS_ENABLED(CONFIG_K3_INLINE_ECC))
+            fixup_ddr_driver_for_ecc(spl_image);
+    } else {
+        fixup_memory_node(spl_image);
+    }
  }
+#endif
  #ifdef CONFIG_BOARD_LATE_INIT
  int board_late_init(void)