* [PATCH v4 1/8] ram: k3-ddrss: Use the DDR controller BIST engine for ECC priming
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-21 4:40 ` [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue Santhosh Kumar K
` (7 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
From: Georgi Vlaev <g-vlaev@ti.com>
The 1-bit inline ECC support in TI's DDRSS bridge requires
the configured memory regions to be preloaded with a pattern
before use. This is done by the k3-ddrss driver from the
R5 SPL in a 'for' loop. It takes around 10 seconds to fill
2GB of memory, for example. Memset can cut the time in half
and using DMA currently yields a similar result.
The BIST engine of DDR controller provides support for
initializing any memory region with a pattern. This
bypasses the DDRSS bridge, so the required inline ECC
data is not computed and populated in the memory. For
some values like zero, the computed ECC syndrome is also
zero and we can use these values to preload the memory
from the DDR controller, without the assistance of the
bridge.
The registers involved in the process are described in the
'DDR controller registers' topic in [1] AM62 and [2] J721E
reference manuals.
The patch replaces the 'for' loop memory fill function with
the BIST memory initialization procedure. This cuts the time
to preload the 2GB memory from 10 seconds down to 1 second.
The bist preload function uses the lpddr4 APIs in the k3-ddrss,
so this is compatible with devices with both 16-bit LPDDR4 and
32-bit LPDDR4 interfaces (e.g J721E).
[1] AM62x: https://www.ti.com/lit/pdf/spruiv7
[2] DRA829/TDA4VM: https://www.ti.com/lit/zip/spruil1
Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
| 122 +++++++++++++++++++++++++++++---
1 file changed, 114 insertions(+), 8 deletions(-)
--git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index 525b6d5b79fc..f2ab8cf2b49b 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -6,6 +6,7 @@
*/
#include <config.h>
+#include <time.h>
#include <clk.h>
#include <div64.h>
#include <dm.h>
@@ -553,14 +554,118 @@ static void k3_ddrss_set_ecc_range_r0(u32 base, u32 start_address, u32 size)
writel((start_address + size - 1) >> 16, base + DDRSS_ECC_R0_END_ADDR_REG);
}
-static void k3_ddrss_preload_ecc_mem_region(u32 *addr, u32 size, u32 word)
+#define BIST_MODE_MEM_INIT 4
+#define BIST_MEM_INIT_TIMEOUT 10000 /* 1msec loops per block = 10s */
+static void k3_lpddr4_bist_init_mem_region(struct k3_ddrss_desc *ddrss,
+ u64 addr, u64 size,
+ u32 pattern)
{
- int i;
+ lpddr4_obj *driverdt = ddrss->driverdt;
+ lpddr4_privatedata *pd = &ddrss->pd;
+ u32 status, offset, regval;
+ bool int_status;
+ int i = 0;
+
+ /* Set BIST_START_ADDR_0 [31:0] */
+ regval = (u32)(addr & TH_FLD_MASK(LPDDR4__BIST_START_ADDRESS_0__FLD));
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_START_ADDRESS_0__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+
+ /* Set BIST_START_ADDR_1 [32 or 34:32] */
+ regval = (u32)(addr >> TH_FLD_WIDTH(LPDDR4__BIST_START_ADDRESS_0__FLD));
+ regval &= TH_FLD_MASK(LPDDR4__BIST_START_ADDRESS_1__FLD);
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_START_ADDRESS_1__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+
+ /* Set ADDR_SPACE = log2(size) */
+ regval = (u32)(ilog2(size) << TH_FLD_SHIFT(LPDDR4__ADDR_SPACE__FLD));
+ TH_OFFSET_FROM_REG(LPDDR4__ADDR_SPACE__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+
+ /* Enable the BIST data check. On 32bit lpddr4 (e.g J7) this shares a
+ * register with ADDR_SPACE and BIST_GO.
+ */
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_DATA_CHECK__REG, CTL_SHIFT, offset);
+ driverdt->readreg(pd, LPDDR4_CTL_REGS, offset, ®val);
+ regval |= TH_FLD_MASK(LPDDR4__BIST_DATA_CHECK__FLD);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+ /* Clear the address check bit */
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_ADDR_CHECK__REG, CTL_SHIFT, offset);
+ driverdt->readreg(pd, LPDDR4_CTL_REGS, offset, ®val);
+ regval &= ~TH_FLD_MASK(LPDDR4__BIST_ADDR_CHECK__FLD);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+
+ /* Set BIST_TEST_MODE[2:0] to memory initialize (4) */
+ regval = BIST_MODE_MEM_INIT;
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_TEST_MODE__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+
+ /* Set BIST_DATA_PATTERN[31:0] */
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_DATA_PATTERN_0__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, pattern);
+
+ /* Set BIST_DATA_PATTERN[63:32] */
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_DATA_PATTERN_1__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, pattern);
+
+ udelay(1000);
+
+ /* Enable the programmed BIST operation - BIST_GO = 1 */
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_GO__REG, CTL_SHIFT, offset);
+ driverdt->readreg(pd, LPDDR4_CTL_REGS, offset, ®val);
+ regval |= TH_FLD_MASK(LPDDR4__BIST_GO__FLD);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, regval);
+
+ /* Wait for the BIST_DONE interrupt */
+ while (i < BIST_MEM_INIT_TIMEOUT) {
+ status = driverdt->checkctlinterrupt(pd, LPDDR4_INTR_BIST_DONE,
+ &int_status);
+ if (!status & int_status) {
+ /* Clear LPDDR4_INTR_BIST_DONE */
+ driverdt->ackctlinterrupt(pd, LPDDR4_INTR_BIST_DONE);
+ break;
+ }
+ udelay(1000);
+ i++;
+ }
- printf("ECC is enabled, priming DDR which will take several seconds.\n");
+ /* Before continuing we have to stop BIST - BIST_GO = 0 */
+ TH_OFFSET_FROM_REG(LPDDR4__BIST_GO__REG, CTL_SHIFT, offset);
+ driverdt->writereg(pd, LPDDR4_CTL_REGS, offset, 0);
+
+ /* Timeout hit while priming the memory. We can't continue,
+ * since the memory is not fully initialized and we most
+ * likely get an uncorrectable error exception while booting.
+ */
+ if (i == BIST_MEM_INIT_TIMEOUT) {
+ printf("ERROR: Timeout while priming the memory.\n");
+ hang();
+ }
+}
- for (i = 0; i < (size / 4); i++)
- addr[i] = word;
+static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
+ u64 total_size, u32 pattern)
+{
+ u32 done, max_size2;
+
+ /* Get the max size (log2) supported in this config (16/32 lpddr4)
+ * from the start_addess width - 16bit: 8G, 32bit: 32G
+ */
+ max_size2 = TH_FLD_WIDTH(LPDDR4__BIST_START_ADDRESS_0__FLD) +
+ TH_FLD_WIDTH(LPDDR4__BIST_START_ADDRESS_1__FLD) + 1;
+
+ /* ECC is enabled in dt but we can't preload the memory if
+ * the memory configuration is recognized and supported.
+ */
+ if (!total_size || total_size > (1ull << max_size2) ||
+ total_size & (total_size - 1)) {
+ printf("ECC: the memory configuration is not supported\n");
+ hang();
+ }
+ printf("ECC is enabled, priming DDR which will take several seconds.\n");
+ done = get_timer(0);
+ k3_lpddr4_bist_init_mem_region(ddrss, 0, total_size, pattern);
+ printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
}
static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
@@ -588,9 +693,10 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
writel(DDRSS_ECC_CTRL_REG_ECC_EN | DDRSS_ECC_CTRL_REG_RMW_EN |
DDRSS_ECC_CTRL_REG_WR_ALLOC, base + DDRSS_ECC_CTRL_REG);
- /* Preload ECC Mem region with 0's */
- k3_ddrss_preload_ecc_mem_region((u32 *)ecc_region_start, ecc_range,
- 0x00000000);
+ /* Preload the full memory with 0's using the BIST engine of
+ * the LPDDR4 controller.
+ */
+ k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
/* Clear Error Count Register */
writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
2024-10-21 4:40 ` [PATCH v4 1/8] ram: k3-ddrss: Use the DDR controller BIST engine for ECC priming Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-23 4:44 ` Manorit Chawdhry
2024-10-23 14:39 ` Bryan Brattlof
2024-10-21 4:40 ` [PATCH v4 3/8] ram: k3-ddrss: Setup ECC region start and range Santhosh Kumar K
` (6 subsequent siblings)
8 siblings, 2 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
As R5 is a 32 bit processor, the RAM banks' base and size calculation
is restricted to 32 bits, which results in wrong values if bank's base
is greater than 32 bits or bank's size is greater than or equal to 4GB.
So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
size of RAM's banks from the device tree memory node, and store in a
64 bit device private data which can be used for ECC reserved memory
calculation, Setting ECC range and Fixing up bank size in device tree
when ECC is enabled.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
| 72 ++++++++++++++++++++++++++-------
1 file changed, 57 insertions(+), 15 deletions(-)
--git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index f2ab8cf2b49b..bd6783ebc2cd 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -147,6 +147,9 @@ struct k3_ddrss_desc {
struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
u64 ecc_reserved_space;
bool ti_ecc_enabled;
+ unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
+ unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
+ unsigned long long ddr_ram_size;
};
struct reginitdata {
@@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
}
+static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
+{
+ int bank, na, ns, len, parent;
+ const fdt32_t *ptr, *end;
+
+ for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
+ ddrss->ddr_bank_base[bank] = 0;
+ ddrss->ddr_bank_size[bank] = 0;
+ }
+
+ ofnode mem = ofnode_null();
+
+ do {
+ mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
+ } while (!ofnode_is_enabled(mem));
+
+ const void *fdt = ofnode_to_fdt(mem);
+ int node = ofnode_to_offset(mem);
+ const char *property = "reg";
+
+ parent = fdt_parent_offset(fdt, node);
+ na = fdt_address_cells(fdt, parent);
+ ns = fdt_size_cells(fdt, parent);
+ ptr = fdt_getprop(fdt, node, property, &len);
+ end = ptr + len / sizeof(*ptr);
+
+ for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
+ if (ptr + na + ns <= end) {
+ if (CONFIG_IS_ENABLED(OF_TRANSLATE))
+ ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
+ else
+ ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
+
+ ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
+ }
+
+ ptr += na + ns;
+ }
+
+ for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
+ ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
+}
+
static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
{
fdtdec_setup_mem_size_base_lowest();
- ddrss->ecc_reserved_space = gd->ram_size;
+ ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
do_div(ddrss->ecc_reserved_space, 9);
/* Round to clean number */
@@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
/* Preload the full memory with 0's using the BIST engine of
* the LPDDR4 controller.
*/
- k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
+ k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
/* Clear Error Count Register */
writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
@@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
k3_lpddr4_start(ddrss);
+ k3_ddrss_ddr_bank_base_size_calc(ddrss);
+
if (ddrss->ti_ecc_enabled) {
if (!ddrss->ddrss_ss_cfg) {
printf("%s: ss_cfg is required if ecc is enabled but not provided.",
@@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
{
- struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
- u64 start[CONFIG_NR_DRAM_BANKS];
- u64 size[CONFIG_NR_DRAM_BANKS];
int bank;
+ struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
if (ddrss->ecc_reserved_space == 0)
return 0;
for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
- if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
- ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
- bd->bi_dram[bank].size = 0;
+ if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
+ ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
+ ddrss->ddr_bank_size[bank] = 0;
} else {
- bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
+ ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
break;
}
}
- for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
- start[bank] = bd->bi_dram[bank].start;
- size[bank] = bd->bi_dram[bank].size;
- }
-
- return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
+ return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
+ ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
}
static int k3_ddrss_get_info(struct udevice *dev, struct ram_info *info)
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-21 4:40 ` [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue Santhosh Kumar K
@ 2024-10-23 4:44 ` Manorit Chawdhry
2024-12-12 14:38 ` Santhosh Kumar K
2024-10-23 14:39 ` Bryan Brattlof
1 sibling, 1 reply; 27+ messages in thread
From: Manorit Chawdhry @ 2024-10-23 4:44 UTC (permalink / raw)
To: Santhosh Kumar K
Cc: trini, nm, u-kumar1, vigneshr, bb, u-boot, w.egorov, n-francis
Hi Santhosh,
On 10:10-20241021, Santhosh Kumar K wrote:
> As R5 is a 32 bit processor, the RAM banks' base and size calculation
> is restricted to 32 bits, which results in wrong values if bank's base
> is greater than 32 bits or bank's size is greater than or equal to 4GB.
>
> So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
> size of RAM's banks from the device tree memory node, and store in a
> 64 bit device private data which can be used for ECC reserved memory
> calculation, Setting ECC range and Fixing up bank size in device tree
> when ECC is enabled.
>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> ---
> drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
> 1 file changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> index f2ab8cf2b49b..bd6783ebc2cd 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
> struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
> u64 ecc_reserved_space;
> bool ti_ecc_enabled;
> + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
> + unsigned long long ddr_ram_size;
Would suggest using u64 ig if you are sending another revision, would be
a bit more readable IMHO.
Regards,
Manorit
> };
>
> struct reginitdata {
> @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
> printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
> }
>
> +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
> +{
> + int bank, na, ns, len, parent;
> + const fdt32_t *ptr, *end;
> +
> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> + ddrss->ddr_bank_base[bank] = 0;
> + ddrss->ddr_bank_size[bank] = 0;
> + }
> +
> + ofnode mem = ofnode_null();
> +
> + do {
> + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
> + } while (!ofnode_is_enabled(mem));
> +
> + const void *fdt = ofnode_to_fdt(mem);
> + int node = ofnode_to_offset(mem);
> + const char *property = "reg";
> +
> + parent = fdt_parent_offset(fdt, node);
> + na = fdt_address_cells(fdt, parent);
> + ns = fdt_size_cells(fdt, parent);
> + ptr = fdt_getprop(fdt, node, property, &len);
> + end = ptr + len / sizeof(*ptr);
> +
> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> + if (ptr + na + ns <= end) {
> + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
> + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
> + else
> + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
> +
> + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
> + }
> +
> + ptr += na + ns;
> + }
> +
> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
> + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
> +}
> +
> static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
> {
> fdtdec_setup_mem_size_base_lowest();
>
> - ddrss->ecc_reserved_space = gd->ram_size;
> + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
> do_div(ddrss->ecc_reserved_space, 9);
>
> /* Round to clean number */
> @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
> /* Preload the full memory with 0's using the BIST engine of
> * the LPDDR4 controller.
> */
> - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
> + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
>
> /* Clear Error Count Register */
> writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
> @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
>
> k3_lpddr4_start(ddrss);
>
> + k3_ddrss_ddr_bank_base_size_calc(ddrss);
> +
> if (ddrss->ti_ecc_enabled) {
> if (!ddrss->ddrss_ss_cfg) {
> printf("%s: ss_cfg is required if ecc is enabled but not provided.",
> @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
>
> int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
> {
> - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> - u64 start[CONFIG_NR_DRAM_BANKS];
> - u64 size[CONFIG_NR_DRAM_BANKS];
> int bank;
> + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>
> if (ddrss->ecc_reserved_space == 0)
> return 0;
>
> for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
> - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
> - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
> - bd->bi_dram[bank].size = 0;
> + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
> + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
> + ddrss->ddr_bank_size[bank] = 0;
> } else {
> - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
> + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
> break;
> }
> }
>
> - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> - start[bank] = bd->bi_dram[bank].start;
> - size[bank] = bd->bi_dram[bank].size;
> - }
> -
> - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
> + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
> + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
> }
>
> static int k3_ddrss_get_info(struct udevice *dev, struct ram_info *info)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-23 4:44 ` Manorit Chawdhry
@ 2024-12-12 14:38 ` Santhosh Kumar K
0 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:38 UTC (permalink / raw)
To: Manorit Chawdhry
Cc: trini, nm, u-kumar1, vigneshr, bb, u-boot, w.egorov, n-francis,
s-k6
Hi, Manorit,
On 23/10/24 10:14, Manorit Chawdhry wrote:
> Hi Santhosh,
>
> On 10:10-20241021, Santhosh Kumar K wrote:
>> As R5 is a 32 bit processor, the RAM banks' base and size calculation
>> is restricted to 32 bits, which results in wrong values if bank's base
>> is greater than 32 bits or bank's size is greater than or equal to 4GB.
>>
>> So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
>> size of RAM's banks from the device tree memory node, and store in a
>> 64 bit device private data which can be used for ECC reserved memory
>> calculation, Setting ECC range and Fixing up bank size in device tree
>> when ECC is enabled.
>>
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>> ---
>> drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
>> 1 file changed, 57 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index f2ab8cf2b49b..bd6783ebc2cd 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
>> struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>> u64 ecc_reserved_space;
>> bool ti_ecc_enabled;
>> + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
>> + unsigned long long ddr_ram_size;
>
> Would suggest using u64 ig if you are sending another revision, would be
> a bit more readable IMHO.
Yeah, sure!
Regards,
Santhosh.
>
> Regards,
> Manorit
>
>> };
>>
>> struct reginitdata {
>> @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
>> printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
>> }
>>
>> +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>> +{
>> + int bank, na, ns, len, parent;
>> + const fdt32_t *ptr, *end;
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + ddrss->ddr_bank_base[bank] = 0;
>> + ddrss->ddr_bank_size[bank] = 0;
>> + }
>> +
>> + ofnode mem = ofnode_null();
>> +
>> + do {
>> + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
>> + } while (!ofnode_is_enabled(mem));
>> +
>> + const void *fdt = ofnode_to_fdt(mem);
>> + int node = ofnode_to_offset(mem);
>> + const char *property = "reg";
>> +
>> + parent = fdt_parent_offset(fdt, node);
>> + na = fdt_address_cells(fdt, parent);
>> + ns = fdt_size_cells(fdt, parent);
>> + ptr = fdt_getprop(fdt, node, property, &len);
>> + end = ptr + len / sizeof(*ptr);
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + if (ptr + na + ns <= end) {
>> + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
>> + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
>> + else
>> + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
>> +
>> + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
>> + }
>> +
>> + ptr += na + ns;
>> + }
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
>> + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>> +}
>> +
>> static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>> {
>> fdtdec_setup_mem_size_base_lowest();
>>
>> - ddrss->ecc_reserved_space = gd->ram_size;
>> + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
>> do_div(ddrss->ecc_reserved_space, 9);
>>
>> /* Round to clean number */
>> @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>> /* Preload the full memory with 0's using the BIST engine of
>> * the LPDDR4 controller.
>> */
>> - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
>> + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
>>
>> /* Clear Error Count Register */
>> writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
>> @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
>>
>> k3_lpddr4_start(ddrss);
>>
>> + k3_ddrss_ddr_bank_base_size_calc(ddrss);
>> +
>> if (ddrss->ti_ecc_enabled) {
>> if (!ddrss->ddrss_ss_cfg) {
>> printf("%s: ss_cfg is required if ecc is enabled but not provided.",
>> @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
>>
>> int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
>> {
>> - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>> - u64 start[CONFIG_NR_DRAM_BANKS];
>> - u64 size[CONFIG_NR_DRAM_BANKS];
>> int bank;
>> + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>>
>> if (ddrss->ecc_reserved_space == 0)
>> return 0;
>>
>> for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
>> - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
>> - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
>> - bd->bi_dram[bank].size = 0;
>> + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
>> + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
>> + ddrss->ddr_bank_size[bank] = 0;
>> } else {
>> - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
>> + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
>> break;
>> }
>> }
>>
>> - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> - start[bank] = bd->bi_dram[bank].start;
>> - size[bank] = bd->bi_dram[bank].size;
>> - }
>> -
>> - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
>> + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
>> + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
>> }
>>
>> static int k3_ddrss_get_info(struct udevice *dev, struct ram_info *info)
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-21 4:40 ` [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue Santhosh Kumar K
2024-10-23 4:44 ` Manorit Chawdhry
@ 2024-10-23 14:39 ` Bryan Brattlof
2024-10-24 4:01 ` Neha Malcom Francis
2024-12-12 14:39 ` Santhosh Kumar K
1 sibling, 2 replies; 27+ messages in thread
From: Bryan Brattlof @ 2024-10-23 14:39 UTC (permalink / raw)
To: Santhosh Kumar K
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov,
n-francis
On October 21, 2024 thus sayeth Santhosh Kumar K:
> As R5 is a 32 bit processor, the RAM banks' base and size calculation
> is restricted to 32 bits, which results in wrong values if bank's base
> is greater than 32 bits or bank's size is greater than or equal to 4GB.
>
> So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
> size of RAM's banks from the device tree memory node, and store in a
> 64 bit device private data which can be used for ECC reserved memory
> calculation, Setting ECC range and Fixing up bank size in device tree
> when ECC is enabled.
>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> ---
> drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
> 1 file changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> index f2ab8cf2b49b..bd6783ebc2cd 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
> struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
> u64 ecc_reserved_space;
> bool ti_ecc_enabled;
> + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
> + unsigned long long ddr_ram_size;
Should we use the u64 typedef here?
> };
>
> struct reginitdata {
> @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
> printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
> }
>
> +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
> +{
> + int bank, na, ns, len, parent;
> + const fdt32_t *ptr, *end;
> +
> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> + ddrss->ddr_bank_base[bank] = 0;
> + ddrss->ddr_bank_size[bank] = 0;
> + }
> +
> + ofnode mem = ofnode_null();
> +
> + do {
> + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
> + } while (!ofnode_is_enabled(mem));
> +
> + const void *fdt = ofnode_to_fdt(mem);
> + int node = ofnode_to_offset(mem);
> + const char *property = "reg";
> +
> + parent = fdt_parent_offset(fdt, node);
> + na = fdt_address_cells(fdt, parent);
> + ns = fdt_size_cells(fdt, parent);
> + ptr = fdt_getprop(fdt, node, property, &len);
> + end = ptr + len / sizeof(*ptr);
> +
> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> + if (ptr + na + ns <= end) {
> + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
> + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
> + else
> + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
> +
> + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
> + }
> +
> + ptr += na + ns;
> + }
> +
> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
> + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
> +}
> +
> static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
> {
> fdtdec_setup_mem_size_base_lowest();
>
> - ddrss->ecc_reserved_space = gd->ram_size;
> + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
> do_div(ddrss->ecc_reserved_space, 9);
>
> /* Round to clean number */
> @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
> /* Preload the full memory with 0's using the BIST engine of
> * the LPDDR4 controller.
> */
> - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
> + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
>
> /* Clear Error Count Register */
> writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
> @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
>
> k3_lpddr4_start(ddrss);
>
> + k3_ddrss_ddr_bank_base_size_calc(ddrss);
> +
> if (ddrss->ti_ecc_enabled) {
> if (!ddrss->ddrss_ss_cfg) {
> printf("%s: ss_cfg is required if ecc is enabled but not provided.",
> @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
>
> int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
> {
> - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> - u64 start[CONFIG_NR_DRAM_BANKS];
> - u64 size[CONFIG_NR_DRAM_BANKS];
> int bank;
> + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>
> if (ddrss->ecc_reserved_space == 0)
> return 0;
>
> for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
> - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
> - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
> - bd->bi_dram[bank].size = 0;
> + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
> + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
> + ddrss->ddr_bank_size[bank] = 0;
> } else {
> - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
> + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
> break;
> }
> }
>
> - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> - start[bank] = bd->bi_dram[bank].start;
> - size[bank] = bd->bi_dram[bank].size;
> - }
> -
> - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
> + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
> + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
> }
Have we looked into passing this information via the global data pointer
rather than fixing up the device tree on each boot phase?
https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-sjg@chromium.org/
I don't see a clear path to pass GD from SPL to SPL but there is a TPL
to SPL which I think we should be able to copy.
~Bryan
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-23 14:39 ` Bryan Brattlof
@ 2024-10-24 4:01 ` Neha Malcom Francis
2024-10-24 16:19 ` Bryan Brattlof
2024-12-12 14:39 ` Santhosh Kumar K
1 sibling, 1 reply; 27+ messages in thread
From: Neha Malcom Francis @ 2024-10-24 4:01 UTC (permalink / raw)
To: Bryan Brattlof, Santhosh Kumar K
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov
Hi Bryan
On 23/10/24 20:09, Bryan Brattlof wrote:
> On October 21, 2024 thus sayeth Santhosh Kumar K:
>> As R5 is a 32 bit processor, the RAM banks' base and size calculation
>> is restricted to 32 bits, which results in wrong values if bank's base
>> is greater than 32 bits or bank's size is greater than or equal to 4GB.
>>
>> So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
>> size of RAM's banks from the device tree memory node, and store in a
>> 64 bit device private data which can be used for ECC reserved memory
>> calculation, Setting ECC range and Fixing up bank size in device tree
>> when ECC is enabled.
>>
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>> ---
>> drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
>> 1 file changed, 57 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index f2ab8cf2b49b..bd6783ebc2cd 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
>> struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>> u64 ecc_reserved_space;
>> bool ti_ecc_enabled;
>> + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
>> + unsigned long long ddr_ram_size;
>
> Should we use the u64 typedef here?
>
>> };
>>
>> struct reginitdata {
>> @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
>> printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
>> }
>>
>> +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>> +{
>> + int bank, na, ns, len, parent;
>> + const fdt32_t *ptr, *end;
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + ddrss->ddr_bank_base[bank] = 0;
>> + ddrss->ddr_bank_size[bank] = 0;
>> + }
>> +
>> + ofnode mem = ofnode_null();
>> +
>> + do {
>> + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
>> + } while (!ofnode_is_enabled(mem));
>> +
>> + const void *fdt = ofnode_to_fdt(mem);
>> + int node = ofnode_to_offset(mem);
>> + const char *property = "reg";
>> +
>> + parent = fdt_parent_offset(fdt, node);
>> + na = fdt_address_cells(fdt, parent);
>> + ns = fdt_size_cells(fdt, parent);
>> + ptr = fdt_getprop(fdt, node, property, &len);
>> + end = ptr + len / sizeof(*ptr);
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + if (ptr + na + ns <= end) {
>> + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
>> + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
>> + else
>> + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
>> +
>> + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
>> + }
>> +
>> + ptr += na + ns;
>> + }
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
>> + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>> +}
>> +
>> static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>> {
>> fdtdec_setup_mem_size_base_lowest();
>>
>> - ddrss->ecc_reserved_space = gd->ram_size;
>> + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
>> do_div(ddrss->ecc_reserved_space, 9);
>>
>> /* Round to clean number */
>> @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>> /* Preload the full memory with 0's using the BIST engine of
>> * the LPDDR4 controller.
>> */
>> - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
>> + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
>>
>> /* Clear Error Count Register */
>> writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
>> @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
>>
>> k3_lpddr4_start(ddrss);
>>
>> + k3_ddrss_ddr_bank_base_size_calc(ddrss);
>> +
>> if (ddrss->ti_ecc_enabled) {
>> if (!ddrss->ddrss_ss_cfg) {
>> printf("%s: ss_cfg is required if ecc is enabled but not provided.",
>> @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
>>
>> int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
>> {
>> - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>> - u64 start[CONFIG_NR_DRAM_BANKS];
>> - u64 size[CONFIG_NR_DRAM_BANKS];
>> int bank;
>> + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>>
>> if (ddrss->ecc_reserved_space == 0)
>> return 0;
>>
>> for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
>> - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
>> - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
>> - bd->bi_dram[bank].size = 0;
>> + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
>> + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
>> + ddrss->ddr_bank_size[bank] = 0;
>> } else {
>> - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
>> + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
>> break;
>> }
>> }
>>
>> - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> - start[bank] = bd->bi_dram[bank].start;
>> - size[bank] = bd->bi_dram[bank].size;
>> - }
>> -
>> - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
>> + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
>> + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
>> }
>
> Have we looked into passing this information via the global data pointer
> rather than fixing up the device tree on each boot phase?
>
> https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-sjg@chromium.org/
>
> I don't see a clear path to pass GD from SPL to SPL but there is a TPL
> to SPL which I think we should be able to copy.
But we also make use of this to fix up the device tree that passes onto kernel
as well, I need to look into this patch though. But from a first glance looks
like we will be able to pass on the information from SPL->SPL (with modification
as you said) and SPL->U-Boot but U-Boot->Kernel would still require this
function to be present.
>
> ~Bryan
--
Thanking You
Neha Malcom Francis
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-24 4:01 ` Neha Malcom Francis
@ 2024-10-24 16:19 ` Bryan Brattlof
2024-12-06 12:53 ` Wadim Egorov
0 siblings, 1 reply; 27+ messages in thread
From: Bryan Brattlof @ 2024-10-24 16:19 UTC (permalink / raw)
To: Neha Malcom Francis
Cc: Santhosh Kumar K, trini, m-chawdhry, nm, u-kumar1, vigneshr,
u-boot, w.egorov
On October 24, 2024 thus sayeth Neha Malcom Francis:
> Hi Bryan
>
> On 23/10/24 20:09, Bryan Brattlof wrote:
> > On October 21, 2024 thus sayeth Santhosh Kumar K:
> > > As R5 is a 32 bit processor, the RAM banks' base and size calculation
> > > is restricted to 32 bits, which results in wrong values if bank's base
> > > is greater than 32 bits or bank's size is greater than or equal to 4GB.
> > >
> > > So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
> > > size of RAM's banks from the device tree memory node, and store in a
> > > 64 bit device private data which can be used for ECC reserved memory
> > > calculation, Setting ECC range and Fixing up bank size in device tree
> > > when ECC is enabled.
> > >
> > > Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> > > ---
> > > drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
> > > 1 file changed, 57 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> > > index f2ab8cf2b49b..bd6783ebc2cd 100644
> > > --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> > > +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> > > @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
> > > struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
> > > u64 ecc_reserved_space;
> > > bool ti_ecc_enabled;
> > > + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> > > + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
> > > + unsigned long long ddr_ram_size;
> >
> > Should we use the u64 typedef here?
> >
> > > };
> > > struct reginitdata {
> > > @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
> > > printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
> > > }
> > > +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
> > > +{
> > > + int bank, na, ns, len, parent;
> > > + const fdt32_t *ptr, *end;
> > > +
> > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > + ddrss->ddr_bank_base[bank] = 0;
> > > + ddrss->ddr_bank_size[bank] = 0;
> > > + }
> > > +
> > > + ofnode mem = ofnode_null();
> > > +
> > > + do {
> > > + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
> > > + } while (!ofnode_is_enabled(mem));
> > > +
> > > + const void *fdt = ofnode_to_fdt(mem);
> > > + int node = ofnode_to_offset(mem);
> > > + const char *property = "reg";
> > > +
> > > + parent = fdt_parent_offset(fdt, node);
> > > + na = fdt_address_cells(fdt, parent);
> > > + ns = fdt_size_cells(fdt, parent);
> > > + ptr = fdt_getprop(fdt, node, property, &len);
> > > + end = ptr + len / sizeof(*ptr);
> > > +
> > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > + if (ptr + na + ns <= end) {
> > > + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
> > > + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
> > > + else
> > > + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
> > > +
> > > + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
> > > + }
> > > +
> > > + ptr += na + ns;
> > > + }
> > > +
> > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
> > > + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
> > > +}
> > > +
> > > static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
> > > {
> > > fdtdec_setup_mem_size_base_lowest();
> > > - ddrss->ecc_reserved_space = gd->ram_size;
> > > + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
> > > do_div(ddrss->ecc_reserved_space, 9);
> > > /* Round to clean number */
> > > @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
> > > /* Preload the full memory with 0's using the BIST engine of
> > > * the LPDDR4 controller.
> > > */
> > > - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
> > > + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
> > > /* Clear Error Count Register */
> > > writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
> > > @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
> > > k3_lpddr4_start(ddrss);
> > > + k3_ddrss_ddr_bank_base_size_calc(ddrss);
> > > +
> > > if (ddrss->ti_ecc_enabled) {
> > > if (!ddrss->ddrss_ss_cfg) {
> > > printf("%s: ss_cfg is required if ecc is enabled but not provided.",
> > > @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
> > > int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
> > > {
> > > - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> > > - u64 start[CONFIG_NR_DRAM_BANKS];
> > > - u64 size[CONFIG_NR_DRAM_BANKS];
> > > int bank;
> > > + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> > > if (ddrss->ecc_reserved_space == 0)
> > > return 0;
> > > for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
> > > - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
> > > - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
> > > - bd->bi_dram[bank].size = 0;
> > > + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
> > > + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
> > > + ddrss->ddr_bank_size[bank] = 0;
> > > } else {
> > > - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
> > > + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
> > > break;
> > > }
> > > }
> > > - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > - start[bank] = bd->bi_dram[bank].start;
> > > - size[bank] = bd->bi_dram[bank].size;
> > > - }
> > > -
> > > - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
> > > + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
> > > + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
> > > }
> >
> > Have we looked into passing this information via the global data pointer
> > rather than fixing up the device tree on each boot phase?
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-sjg@chromium.org/
> >
> > I don't see a clear path to pass GD from SPL to SPL but there is a TPL
> > to SPL which I think we should be able to copy.
>
> But we also make use of this to fix up the device tree that passes onto
> kernel as well, I need to look into this patch though. But from a first
> glance looks like we will be able to pass on the information from SPL->SPL
> (with modification as you said) and SPL->U-Boot but U-Boot->Kernel would
> still require this function to be present.
>
I agree. If this scheme works out we can place this kernel fixup
function with the other OPN fixups we do though later on in the boot.
https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-k3/common_fdt.c
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-24 16:19 ` Bryan Brattlof
@ 2024-12-06 12:53 ` Wadim Egorov
2024-12-06 17:40 ` Bryan Brattlof
0 siblings, 1 reply; 27+ messages in thread
From: Wadim Egorov @ 2024-12-06 12:53 UTC (permalink / raw)
To: Bryan Brattlof, Neha Malcom Francis
Cc: Santhosh Kumar K, trini, m-chawdhry, nm, u-kumar1, vigneshr,
u-boot
Am 24.10.24 um 18:19 schrieb Bryan Brattlof:
> On October 24, 2024 thus sayeth Neha Malcom Francis:
>> Hi Bryan
>>
>> On 23/10/24 20:09, Bryan Brattlof wrote:
>>> On October 21, 2024 thus sayeth Santhosh Kumar K:
>>>> As R5 is a 32 bit processor, the RAM banks' base and size calculation
>>>> is restricted to 32 bits, which results in wrong values if bank's base
>>>> is greater than 32 bits or bank's size is greater than or equal to 4GB.
>>>>
>>>> So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
>>>> size of RAM's banks from the device tree memory node, and store in a
>>>> 64 bit device private data which can be used for ECC reserved memory
>>>> calculation, Setting ECC range and Fixing up bank size in device tree
>>>> when ECC is enabled.
>>>>
>>>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>>>> ---
>>>> drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
>>>> 1 file changed, 57 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
>>>> index f2ab8cf2b49b..bd6783ebc2cd 100644
>>>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>>>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>>>> @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
>>>> struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>>>> u64 ecc_reserved_space;
>>>> bool ti_ecc_enabled;
>>>> + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>>>> + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
>>>> + unsigned long long ddr_ram_size;
>>>
>>> Should we use the u64 typedef here?
>>>
>>>> };
>>>> struct reginitdata {
>>>> @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
>>>> printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
>>>> }
>>>> +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>>>> +{
>>>> + int bank, na, ns, len, parent;
>>>> + const fdt32_t *ptr, *end;
>>>> +
>>>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>>>> + ddrss->ddr_bank_base[bank] = 0;
>>>> + ddrss->ddr_bank_size[bank] = 0;
>>>> + }
>>>> +
>>>> + ofnode mem = ofnode_null();
>>>> +
>>>> + do {
>>>> + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
>>>> + } while (!ofnode_is_enabled(mem));
>>>> +
>>>> + const void *fdt = ofnode_to_fdt(mem);
>>>> + int node = ofnode_to_offset(mem);
>>>> + const char *property = "reg";
>>>> +
>>>> + parent = fdt_parent_offset(fdt, node);
>>>> + na = fdt_address_cells(fdt, parent);
>>>> + ns = fdt_size_cells(fdt, parent);
>>>> + ptr = fdt_getprop(fdt, node, property, &len);
>>>> + end = ptr + len / sizeof(*ptr);
>>>> +
>>>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>>>> + if (ptr + na + ns <= end) {
>>>> + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
>>>> + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
>>>> + else
>>>> + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
>>>> +
>>>> + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
>>>> + }
>>>> +
>>>> + ptr += na + ns;
>>>> + }
>>>> +
>>>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
>>>> + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>>>> +}
>>>> +
>>>> static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>>>> {
>>>> fdtdec_setup_mem_size_base_lowest();
>>>> - ddrss->ecc_reserved_space = gd->ram_size;
>>>> + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
>>>> do_div(ddrss->ecc_reserved_space, 9);
>>>> /* Round to clean number */
>>>> @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>>>> /* Preload the full memory with 0's using the BIST engine of
>>>> * the LPDDR4 controller.
>>>> */
>>>> - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
>>>> + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
>>>> /* Clear Error Count Register */
>>>> writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
>>>> @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
>>>> k3_lpddr4_start(ddrss);
>>>> + k3_ddrss_ddr_bank_base_size_calc(ddrss);
>>>> +
>>>> if (ddrss->ti_ecc_enabled) {
>>>> if (!ddrss->ddrss_ss_cfg) {
>>>> printf("%s: ss_cfg is required if ecc is enabled but not provided.",
>>>> @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
>>>> int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
>>>> {
>>>> - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>>>> - u64 start[CONFIG_NR_DRAM_BANKS];
>>>> - u64 size[CONFIG_NR_DRAM_BANKS];
>>>> int bank;
>>>> + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>>>> if (ddrss->ecc_reserved_space == 0)
>>>> return 0;
>>>> for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
>>>> - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
>>>> - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
>>>> - bd->bi_dram[bank].size = 0;
>>>> + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
>>>> + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
>>>> + ddrss->ddr_bank_size[bank] = 0;
>>>> } else {
>>>> - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
>>>> + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
>>>> break;
>>>> }
>>>> }
>>>> - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>>>> - start[bank] = bd->bi_dram[bank].start;
>>>> - size[bank] = bd->bi_dram[bank].size;
>>>> - }
>>>> -
>>>> - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
>>>> + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
>>>> + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
>>>> }
>>>
>>> Have we looked into passing this information via the global data pointer
>>> rather than fixing up the device tree on each boot phase?
>>>
>>> https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-sjg@chromium.org/
>>>
>>> I don't see a clear path to pass GD from SPL to SPL but there is a TPL
>>> to SPL which I think we should be able to copy.
>>
>> But we also make use of this to fix up the device tree that passes onto
>> kernel as well, I need to look into this patch though. But from a first
>> glance looks like we will be able to pass on the information from SPL->SPL
>> (with modification as you said) and SPL->U-Boot but U-Boot->Kernel would
>> still require this function to be present.
Just came across this series again and noticed that this will be a bit
tricky with boards detecting RAM at runtime.
Updating the memory node with detected RAM size prio to DDR driver
probing should be no problem. But the next stages will need to update
u-boot's and Linux memory nodes too. Unfortunately, we do not have the
data at this stage which tells us how much space was reserved for ECC.
It would be best to do the memory setup for GD or memory node once and
have other stages reuse it.
Right now I have no idea how my board should be aware of the reserved
ecc memory in u-boot. Any suggestions?
>>
>
> I agree. If this scheme works out we can place this kernel fixup
> function with the other OPN fixups we do though later on in the boot.
>
> https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-k3/common_fdt.c
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-12-06 12:53 ` Wadim Egorov
@ 2024-12-06 17:40 ` Bryan Brattlof
0 siblings, 0 replies; 27+ messages in thread
From: Bryan Brattlof @ 2024-12-06 17:40 UTC (permalink / raw)
To: Wadim Egorov
Cc: Neha Malcom Francis, Santhosh Kumar K, trini, m-chawdhry, nm,
u-kumar1, vigneshr, u-boot
On December 6, 2024 thus sayeth Wadim Egorov:
> Am 24.10.24 um 18:19 schrieb Bryan Brattlof:
> > On October 24, 2024 thus sayeth Neha Malcom Francis:
> > > Hi Bryan
> > >
> > > On 23/10/24 20:09, Bryan Brattlof wrote:
> > > > On October 21, 2024 thus sayeth Santhosh Kumar K:
> > > > > As R5 is a 32 bit processor, the RAM banks' base and size calculation
> > > > > is restricted to 32 bits, which results in wrong values if bank's base
> > > > > is greater than 32 bits or bank's size is greater than or equal to 4GB.
> > > > >
> > > > > So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
> > > > > size of RAM's banks from the device tree memory node, and store in a
> > > > > 64 bit device private data which can be used for ECC reserved memory
> > > > > calculation, Setting ECC range and Fixing up bank size in device tree
> > > > > when ECC is enabled.
> > > > >
> > > > > Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> > > > > ---
> > > > > drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
> > > > > 1 file changed, 57 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> > > > > index f2ab8cf2b49b..bd6783ebc2cd 100644
> > > > > --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> > > > > +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> > > > > @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
> > > > > struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
> > > > > u64 ecc_reserved_space;
> > > > > bool ti_ecc_enabled;
> > > > > + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> > > > > + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
> > > > > + unsigned long long ddr_ram_size;
> > > >
> > > > Should we use the u64 typedef here?
> > > >
> > > > > };
> > > > > struct reginitdata {
> > > > > @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
> > > > > printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
> > > > > }
> > > > > +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
> > > > > +{
> > > > > + int bank, na, ns, len, parent;
> > > > > + const fdt32_t *ptr, *end;
> > > > > +
> > > > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > > > + ddrss->ddr_bank_base[bank] = 0;
> > > > > + ddrss->ddr_bank_size[bank] = 0;
> > > > > + }
> > > > > +
> > > > > + ofnode mem = ofnode_null();
> > > > > +
> > > > > + do {
> > > > > + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
> > > > > + } while (!ofnode_is_enabled(mem));
> > > > > +
> > > > > + const void *fdt = ofnode_to_fdt(mem);
> > > > > + int node = ofnode_to_offset(mem);
> > > > > + const char *property = "reg";
> > > > > +
> > > > > + parent = fdt_parent_offset(fdt, node);
> > > > > + na = fdt_address_cells(fdt, parent);
> > > > > + ns = fdt_size_cells(fdt, parent);
> > > > > + ptr = fdt_getprop(fdt, node, property, &len);
> > > > > + end = ptr + len / sizeof(*ptr);
> > > > > +
> > > > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > > > + if (ptr + na + ns <= end) {
> > > > > + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
> > > > > + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
> > > > > + else
> > > > > + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
> > > > > +
> > > > > + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
> > > > > + }
> > > > > +
> > > > > + ptr += na + ns;
> > > > > + }
> > > > > +
> > > > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
> > > > > + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
> > > > > +}
> > > > > +
> > > > > static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
> > > > > {
> > > > > fdtdec_setup_mem_size_base_lowest();
> > > > > - ddrss->ecc_reserved_space = gd->ram_size;
> > > > > + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
> > > > > do_div(ddrss->ecc_reserved_space, 9);
> > > > > /* Round to clean number */
> > > > > @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
> > > > > /* Preload the full memory with 0's using the BIST engine of
> > > > > * the LPDDR4 controller.
> > > > > */
> > > > > - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
> > > > > + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
> > > > > /* Clear Error Count Register */
> > > > > writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
> > > > > @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
> > > > > k3_lpddr4_start(ddrss);
> > > > > + k3_ddrss_ddr_bank_base_size_calc(ddrss);
> > > > > +
> > > > > if (ddrss->ti_ecc_enabled) {
> > > > > if (!ddrss->ddrss_ss_cfg) {
> > > > > printf("%s: ss_cfg is required if ecc is enabled but not provided.",
> > > > > @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
> > > > > int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
> > > > > {
> > > > > - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> > > > > - u64 start[CONFIG_NR_DRAM_BANKS];
> > > > > - u64 size[CONFIG_NR_DRAM_BANKS];
> > > > > int bank;
> > > > > + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> > > > > if (ddrss->ecc_reserved_space == 0)
> > > > > return 0;
> > > > > for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
> > > > > - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
> > > > > - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
> > > > > - bd->bi_dram[bank].size = 0;
> > > > > + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
> > > > > + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
> > > > > + ddrss->ddr_bank_size[bank] = 0;
> > > > > } else {
> > > > > - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
> > > > > + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
> > > > > break;
> > > > > }
> > > > > }
> > > > > - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > > > - start[bank] = bd->bi_dram[bank].start;
> > > > > - size[bank] = bd->bi_dram[bank].size;
> > > > > - }
> > > > > -
> > > > > - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
> > > > > + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
> > > > > + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
> > > > > }
> > > >
> > > > Have we looked into passing this information via the global data pointer
> > > > rather than fixing up the device tree on each boot phase?
> > > >
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-sjg@chromium.org/
> > > >
> > > > I don't see a clear path to pass GD from SPL to SPL but there is a TPL
> > > > to SPL which I think we should be able to copy.
> > >
> > > But we also make use of this to fix up the device tree that passes onto
> > > kernel as well, I need to look into this patch though. But from a first
> > > glance looks like we will be able to pass on the information from SPL->SPL
> > > (with modification as you said) and SPL->U-Boot but U-Boot->Kernel would
> > > still require this function to be present.
>
> Just came across this series again and noticed that this will be a bit
> tricky with boards detecting RAM at runtime.
>
> Updating the memory node with detected RAM size prio to DDR driver probing
> should be no problem. But the next stages will need to update u-boot's and
> Linux memory nodes too. Unfortunately, we do not have the data at this stage
> which tells us how much space was reserved for ECC.
>
I agree. We'll need to prototype this out real quick but my goal is to
set the gd->ram_size in the R5 SPL that we can then pass GD to each boot
phase. What I haven't looked into is what other drivers will break :)
> It would be best to do the memory setup for GD or memory node once and have
> other stages reuse it.
>
> Right now I have no idea how my board should be aware of the reserved ecc
> memory in u-boot. Any suggestions?
>
I don't have a solution to this either. My only thought would be to look
at the controller to see if it has ECC enabled and take 8/9s of the
total capacity.
I'm not a huge fan of that as well as it's kinda opaque. It's much more
clean to let the R5 SPL do this and pass the info along all the way to
the kernel.
~Bryan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue
2024-10-23 14:39 ` Bryan Brattlof
2024-10-24 4:01 ` Neha Malcom Francis
@ 2024-12-12 14:39 ` Santhosh Kumar K
1 sibling, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:39 UTC (permalink / raw)
To: Bryan Brattlof
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov,
n-francis, s-k6
Hi, Bryan,
On 23/10/24 20:09, Bryan Brattlof wrote:
> On October 21, 2024 thus sayeth Santhosh Kumar K:
>> As R5 is a 32 bit processor, the RAM banks' base and size calculation
>> is restricted to 32 bits, which results in wrong values if bank's base
>> is greater than 32 bits or bank's size is greater than or equal to 4GB.
>>
>> So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
>> size of RAM's banks from the device tree memory node, and store in a
>> 64 bit device private data which can be used for ECC reserved memory
>> calculation, Setting ECC range and Fixing up bank size in device tree
>> when ECC is enabled.
>>
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>> ---
>> drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
>> 1 file changed, 57 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index f2ab8cf2b49b..bd6783ebc2cd 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
>> struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>> u64 ecc_reserved_space;
>> bool ti_ecc_enabled;
>> + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
>> + unsigned long long ddr_ram_size;
>
> Should we use the u64 typedef here?
Yes, I'll replace.
>
>> };
>>
>> struct reginitdata {
>> @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct k3_ddrss_desc *ddrss,
>> printf("ECC: priming DDR completed in %lu msec\n", get_timer(done));
>> }
>>
>> +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>> +{
>> + int bank, na, ns, len, parent;
>> + const fdt32_t *ptr, *end;
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + ddrss->ddr_bank_base[bank] = 0;
>> + ddrss->ddr_bank_size[bank] = 0;
>> + }
>> +
>> + ofnode mem = ofnode_null();
>> +
>> + do {
>> + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
>> + } while (!ofnode_is_enabled(mem));
>> +
>> + const void *fdt = ofnode_to_fdt(mem);
>> + int node = ofnode_to_offset(mem);
>> + const char *property = "reg";
>> +
>> + parent = fdt_parent_offset(fdt, node);
>> + na = fdt_address_cells(fdt, parent);
>> + ns = fdt_size_cells(fdt, parent);
>> + ptr = fdt_getprop(fdt, node, property, &len);
>> + end = ptr + len / sizeof(*ptr);
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> + if (ptr + na + ns <= end) {
>> + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
>> + ddrss->ddr_bank_base[bank] = fdt_translate_address(fdt, node, ptr);
>> + else
>> + ddrss->ddr_bank_base[bank] = fdtdec_get_number(ptr, na);
>> +
>> + ddrss->ddr_bank_size[bank] = fdtdec_get_number(&ptr[na], ns);
>> + }
>> +
>> + ptr += na + ns;
>> + }
>> +
>> + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
>> + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>> +}
>> +
>> static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>> {
>> fdtdec_setup_mem_size_base_lowest();
>>
>> - ddrss->ecc_reserved_space = gd->ram_size;
>> + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
>> do_div(ddrss->ecc_reserved_space, 9);
>>
>> /* Round to clean number */
>> @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>> /* Preload the full memory with 0's using the BIST engine of
>> * the LPDDR4 controller.
>> */
>> - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
>> + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
>>
>> /* Clear Error Count Register */
>> writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
>> @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
>>
>> k3_lpddr4_start(ddrss);
>>
>> + k3_ddrss_ddr_bank_base_size_calc(ddrss);
>> +
>> if (ddrss->ti_ecc_enabled) {
>> if (!ddrss->ddrss_ss_cfg) {
>> printf("%s: ss_cfg is required if ecc is enabled but not provided.",
>> @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
>>
>> int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct bd_info *bd)
>> {
>> - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>> - u64 start[CONFIG_NR_DRAM_BANKS];
>> - u64 size[CONFIG_NR_DRAM_BANKS];
>> int bank;
>> + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>>
>> if (ddrss->ecc_reserved_space == 0)
>> return 0;
>>
>> for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
>> - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
>> - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
>> - bd->bi_dram[bank].size = 0;
>> + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
>> + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
>> + ddrss->ddr_bank_size[bank] = 0;
>> } else {
>> - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
>> + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
>> break;
>> }
>> }
>>
>> - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> - start[bank] = bd->bi_dram[bank].start;
>> - size[bank] = bd->bi_dram[bank].size;
>> - }
>> -
>> - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
>> + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
>> + ddrss->ddr_bank_size, CONFIG_NR_DRAM_BANKS);
>> }
>
> Have we looked into passing this information via the global data pointer
> rather than fixing up the device tree on each boot phase?
>
> https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-sjg@chromium.org/
>
> I don't see a clear path to pass GD from SPL to SPL but there is a TPL
> to SPL which I think we should be able to copy.
At R5, the phys_addr_t is 32 bits - so, can't store more than 4GB in
global data pointer.
Even in the above series you shared, handoff_save_dram() saves the RAM
and banks' size from global data only.
Hence, the fix-up is necessary to pass information from R5 SPL to A53 SPL.
Regards,
Santhosh.
>
> ~Bryan
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 3/8] ram: k3-ddrss: Setup ECC region start and range
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
2024-10-21 4:40 ` [PATCH v4 1/8] ram: k3-ddrss: Use the DDR controller BIST engine for ECC priming Santhosh Kumar K
2024-10-21 4:40 ` [PATCH v4 2/8] ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve 'calculations restricted to 32 bits' issue Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-21 4:40 ` [PATCH v4 4/8] ram: k3-ddrss: Enable ECC interrupts Santhosh Kumar K
` (5 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
Setup the ECC region's start and range using the device private data,
ddrss->ddr_bank_base[0] and ddrss->ddr_ram_size. Also, move start and
range of ECC regions from 32 bits to 64 bits to accommodate for
DDR greater than or equal to 4GB.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
| 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
--git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index bd6783ebc2cd..66b7f31f4f64 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -121,8 +121,8 @@ struct k3_msmc {
#define K3_DDRSS_MAX_ECC_REGIONS 3
struct k3_ddrss_ecc_region {
- u32 start;
- u32 range;
+ u64 start;
+ u64 range;
};
struct k3_ddrss_desc {
@@ -551,7 +551,7 @@ void k3_lpddr4_start(struct k3_ddrss_desc *ddrss)
}
}
-static void k3_ddrss_set_ecc_range_r0(u32 base, u32 start_address, u32 size)
+static void k3_ddrss_set_ecc_range_r0(u32 base, u64 start_address, u64 size)
{
writel((start_address) >> 16, base + DDRSS_ECC_R0_STR_ADDR_REG);
writel((start_address + size - 1) >> 16, base + DDRSS_ECC_R0_END_ADDR_REG);
@@ -727,13 +727,13 @@ static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
{
- u32 ecc_region_start = ddrss->ecc_regions[0].start;
- u32 ecc_range = ddrss->ecc_regions[0].range;
+ u64 ecc_region_start = ddrss->ecc_regions[0].start;
+ u64 ecc_range = ddrss->ecc_regions[0].range;
u32 base = (u32)ddrss->ddrss_ss_cfg;
u32 val;
/* Only Program region 0 which covers full ddr space */
- k3_ddrss_set_ecc_range_r0(base, ecc_region_start - gd->ram_base, ecc_range);
+ k3_ddrss_set_ecc_range_r0(base, ecc_region_start - ddrss->ddr_bank_base[0], ecc_range);
/* Enable ECC, RMW, WR_ALLOC */
writel(DDRSS_ECC_CTRL_REG_ECC_EN | DDRSS_ECC_CTRL_REG_RMW_EN |
@@ -799,8 +799,8 @@ static int k3_ddrss_probe(struct udevice *dev)
k3_ddrss_lpddr4_ecc_calc_reserved_mem(ddrss);
/* Always configure one region that covers full DDR space */
- ddrss->ecc_regions[0].start = gd->ram_base;
- ddrss->ecc_regions[0].range = gd->ram_size - ddrss->ecc_reserved_space;
+ ddrss->ecc_regions[0].start = ddrss->ddr_bank_base[0];
+ ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
k3_ddrss_lpddr4_ecc_init(ddrss);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v4 4/8] ram: k3-ddrss: Enable ECC interrupts
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
` (2 preceding siblings ...)
2024-10-21 4:40 ` [PATCH v4 3/8] ram: k3-ddrss: Setup ECC region start and range Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-21 4:40 ` [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC Santhosh Kumar K
` (4 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
Enable ECC 1-bit error, 2-bit error, multiple 1-bit error interrupts
by setting the respective bits in the DDRSS_V2A_INT_SET_REG register.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
| 8 ++++++++
1 file changed, 8 insertions(+)
--git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index 66b7f31f4f64..9f1a3530c704 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -45,6 +45,11 @@
#define DDRSS_ECC_R2_STR_ADDR_REG 0x0140
#define DDRSS_ECC_R2_END_ADDR_REG 0x0144
#define DDRSS_ECC_1B_ERR_CNT_REG 0x0150
+#define DDRSS_V2A_INT_SET_REG 0x00a8
+
+#define DDRSS_V2A_INT_SET_REG_ECC1BERR_EN BIT(3)
+#define DDRSS_V2A_INT_SET_REG_ECC2BERR_EN BIT(4)
+#define DDRSS_V2A_INT_SET_REG_ECCM1BERR_EN BIT(5)
#define SINGLE_DDR_SUBSYSTEM 0x1
#define MULTI_DDR_SUBSYSTEM 0x2
@@ -747,6 +752,9 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
/* Clear Error Count Register */
writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
+ writel(DDRSS_V2A_INT_SET_REG_ECC1BERR_EN | DDRSS_V2A_INT_SET_REG_ECC2BERR_EN |
+ DDRSS_V2A_INT_SET_REG_ECCM1BERR_EN, base + DDRSS_V2A_INT_SET_REG);
+
/* Enable ECC Check */
val = readl(base + DDRSS_ECC_CTRL_REG);
val |= DDRSS_ECC_CTRL_REG_ECC_CK;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
` (3 preceding siblings ...)
2024-10-21 4:40 ` [PATCH v4 4/8] ram: k3-ddrss: Enable ECC interrupts Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-23 14:45 ` Bryan Brattlof
2024-10-21 4:40 ` [PATCH v4 6/8] configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2 Santhosh Kumar K
` (3 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
From: Neha Malcom Francis <n-francis@ti.com>
Add CONFIG_K3_INLINE_ECC so that ECC functions can be compiled into R5 SPL
only when the config has been enabled.
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
drivers/ram/Kconfig | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
index f7e357f24da7..533d0c629a64 100644
--- a/drivers/ram/Kconfig
+++ b/drivers/ram/Kconfig
@@ -116,6 +116,16 @@ config IMXRT_SDRAM
to support external memories like sdram, psram & nand.
This driver is for the sdram memory interface with the SEMC.
+config K3_INLINE_ECC
+ bool "Enable TI Inline ECC support"
+ depends on K3_DDRSS
+ help
+ Enable Inline ECC support on K3 platforms. 1/9th of the SDRAM space
+ is used for ECC storage and the rest 8/9th is available for system
+ use. Enabling ECC increases boot time as the ECC protected regions
+ need to be primed with a predefined value prior to enabling ECC
+ check.
+
source "drivers/ram/aspeed/Kconfig"
source "drivers/ram/cadence/Kconfig"
source "drivers/ram/octeon/Kconfig"
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC
2024-10-21 4:40 ` [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC Santhosh Kumar K
@ 2024-10-23 14:45 ` Bryan Brattlof
2024-10-24 4:03 ` Neha Malcom Francis
0 siblings, 1 reply; 27+ messages in thread
From: Bryan Brattlof @ 2024-10-23 14:45 UTC (permalink / raw)
To: Santhosh Kumar K
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov,
n-francis
On October 21, 2024 thus sayeth Santhosh Kumar K:
> From: Neha Malcom Francis <n-francis@ti.com>
>
> Add CONFIG_K3_INLINE_ECC so that ECC functions can be compiled into R5 SPL
> only when the config has been enabled.
>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
> drivers/ram/Kconfig | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
> index f7e357f24da7..533d0c629a64 100644
> --- a/drivers/ram/Kconfig
> +++ b/drivers/ram/Kconfig
> @@ -116,6 +116,16 @@ config IMXRT_SDRAM
> to support external memories like sdram, psram & nand.
> This driver is for the sdram memory interface with the SEMC.
>
> +config K3_INLINE_ECC
> + bool "Enable TI Inline ECC support"
> + depends on K3_DDRSS
> + help
> + Enable Inline ECC support on K3 platforms. 1/9th of the SDRAM space
> + is used for ECC storage and the rest 8/9th is available for system
> + use. Enabling ECC increases boot time as the ECC protected regions
> + need to be primed with a predefined value prior to enabling ECC
> + check.
> +
I agree size will always be a complaint at such an early stage of boot
but we're already adding the ECC configuration in the memory{} node.
We should do one or the other but not both. It would be hard to debug
problems if someone added the protected{} node but not enable this
config
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC
2024-10-23 14:45 ` Bryan Brattlof
@ 2024-10-24 4:03 ` Neha Malcom Francis
2024-12-12 14:39 ` Santhosh Kumar K
0 siblings, 1 reply; 27+ messages in thread
From: Neha Malcom Francis @ 2024-10-24 4:03 UTC (permalink / raw)
To: Bryan Brattlof, Santhosh Kumar K
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov
Hi Bryan
On 23/10/24 20:15, Bryan Brattlof wrote:
> On October 21, 2024 thus sayeth Santhosh Kumar K:
>> From: Neha Malcom Francis <n-francis@ti.com>
>>
>> Add CONFIG_K3_INLINE_ECC so that ECC functions can be compiled into R5 SPL
>> only when the config has been enabled.
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>> drivers/ram/Kconfig | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
>> index f7e357f24da7..533d0c629a64 100644
>> --- a/drivers/ram/Kconfig
>> +++ b/drivers/ram/Kconfig
>> @@ -116,6 +116,16 @@ config IMXRT_SDRAM
>> to support external memories like sdram, psram & nand.
>> This driver is for the sdram memory interface with the SEMC.
>>
>> +config K3_INLINE_ECC
>> + bool "Enable TI Inline ECC support"
>> + depends on K3_DDRSS
>> + help
>> + Enable Inline ECC support on K3 platforms. 1/9th of the SDRAM space
>> + is used for ECC storage and the rest 8/9th is available for system
>> + use. Enabling ECC increases boot time as the ECC protected regions
>> + need to be primed with a predefined value prior to enabling ECC
>> + check.
>> +
>
> I agree size will always be a complaint at such an early stage of boot
> but we're already adding the ECC configuration in the memory{} node.
>
> We should do one or the other but not both. It would be hard to debug
> problems if someone added the protected{} node but not enable this
> config
Actually I do agree with this, maybe we can get rid of the flag ti,ecc-enable
which is redundant. In multi-DDR systems, it was still a possibility that we
could use it to select the DDR we want ECC enabled on, but that hasn't been
tested nor does it make any sense since they'll be interleaved anyways.
--
Thanking You
Neha Malcom Francis
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC
2024-10-24 4:03 ` Neha Malcom Francis
@ 2024-12-12 14:39 ` Santhosh Kumar K
0 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:39 UTC (permalink / raw)
To: Neha Malcom Francis, Bryan Brattlof
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov, s-k6
Hi, Bryan and Neha,
On 24/10/24 09:33, Neha Malcom Francis wrote:
> Hi Bryan
>
> On 23/10/24 20:15, Bryan Brattlof wrote:
>> On October 21, 2024 thus sayeth Santhosh Kumar K:
>>> From: Neha Malcom Francis <n-francis@ti.com>
>>>
>>> Add CONFIG_K3_INLINE_ECC so that ECC functions can be compiled into
>>> R5 SPL
>>> only when the config has been enabled.
>>>
>>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>>> ---
>>> drivers/ram/Kconfig | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
>>> index f7e357f24da7..533d0c629a64 100644
>>> --- a/drivers/ram/Kconfig
>>> +++ b/drivers/ram/Kconfig
>>> @@ -116,6 +116,16 @@ config IMXRT_SDRAM
>>> to support external memories like sdram, psram & nand.
>>> This driver is for the sdram memory interface with the SEMC.
>>> +config K3_INLINE_ECC
>>> + bool "Enable TI Inline ECC support"
>>> + depends on K3_DDRSS
>>> + help
>>> + Enable Inline ECC support on K3 platforms. 1/9th of the SDRAM
>>> space
>>> + is used for ECC storage and the rest 8/9th is available for
>>> system
>>> + use. Enabling ECC increases boot time as the ECC protected
>>> regions
>>> + need to be primed with a predefined value prior to enabling ECC
>>> + check.
>>> +
>>
>> I agree size will always be a complaint at such an early stage of boot
>> but we're already adding the ECC configuration in the memory{} node.
>>
>> We should do one or the other but not both. It would be hard to debug
>> problems if someone added the protected{} node but not enable this
>> config
>
> Actually I do agree with this, maybe we can get rid of the flag
> ti,ecc-enable which is redundant. In multi-DDR systems, it was still a
> possibility that we could use it to select the DDR we want ECC enabled
> on, but that hasn't been tested nor does it make any sense since they'll
> be interleaved anyways.
Yes, we can get rid of 'ti,ecc-enable' flag and use only K3_INLINE_ECC
config for enabling ECC.
Regards,
Santhosh.
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 6/8] configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
` (4 preceding siblings ...)
2024-10-21 4:40 ` [PATCH v4 5/8] drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-23 14:48 ` Bryan Brattlof
2024-10-21 4:40 ` [PATCH v4 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled Santhosh Kumar K
` (2 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
From: Neha Malcom Francis <n-francis@ti.com>
Set CONFIG_NR_DRAM_BANKS to 2 as we have two banks described in the
memory/ node for lower and higher addressible DDR regions.
This allows use of FDT functions from fdt_support.c to set up and fixup
the memory/ node correctly.
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
configs/j7200_evm_r5_defconfig | 1 +
configs/j721e_evm_r5_defconfig | 1 +
configs/j721s2_evm_r5_defconfig | 1 +
configs/j784s4_evm_r5_defconfig | 1 +
4 files changed, 4 insertions(+)
diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
index 774a9eff439d..78dbd44f845f 100644
--- a/configs/j7200_evm_r5_defconfig
+++ b/configs/j7200_evm_r5_defconfig
@@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x70000
CONFIG_SPL_GPIO=y
CONFIG_SPL_LIBCOMMON_SUPPORT=y
CONFIG_SPL_LIBGENERIC_SUPPORT=y
+CONFIG_NR_DRAM_BANKS=2
CONFIG_SOC_K3_J721E=y
CONFIG_K3_EARLY_CONS=y
CONFIG_TARGET_J7200_R5_EVM=y
diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
index 3b4a7c3c0eeb..dc391f14b193 100644
--- a/configs/j721e_evm_r5_defconfig
+++ b/configs/j721e_evm_r5_defconfig
@@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x70000
CONFIG_SPL_GPIO=y
CONFIG_SPL_LIBCOMMON_SUPPORT=y
CONFIG_SPL_LIBGENERIC_SUPPORT=y
+CONFIG_NR_DRAM_BANKS=2
CONFIG_SOC_K3_J721E=y
CONFIG_K3_EARLY_CONS=y
CONFIG_K3_QOS=y
diff --git a/configs/j721s2_evm_r5_defconfig b/configs/j721s2_evm_r5_defconfig
index b6adb6a77d7e..374bc283be2d 100644
--- a/configs/j721s2_evm_r5_defconfig
+++ b/configs/j721s2_evm_r5_defconfig
@@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x10000
CONFIG_SPL_GPIO=y
CONFIG_SPL_LIBCOMMON_SUPPORT=y
CONFIG_SPL_LIBGENERIC_SUPPORT=y
+CONFIG_NR_DRAM_BANKS=2
CONFIG_SOC_K3_J721S2=y
CONFIG_K3_EARLY_CONS=y
CONFIG_K3_QOS=y
diff --git a/configs/j784s4_evm_r5_defconfig b/configs/j784s4_evm_r5_defconfig
index a11688194977..6fe36ab38f02 100644
--- a/configs/j784s4_evm_r5_defconfig
+++ b/configs/j784s4_evm_r5_defconfig
@@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x10000
CONFIG_SPL_GPIO=y
CONFIG_SPL_LIBCOMMON_SUPPORT=y
CONFIG_SPL_LIBGENERIC_SUPPORT=y
+CONFIG_NR_DRAM_BANKS=2
CONFIG_SOC_K3_J784S4=y
CONFIG_K3_EARLY_CONS=y
CONFIG_K3_QOS=y
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v4 6/8] configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2
2024-10-21 4:40 ` [PATCH v4 6/8] configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2 Santhosh Kumar K
@ 2024-10-23 14:48 ` Bryan Brattlof
2024-12-12 14:40 ` Santhosh Kumar K
0 siblings, 1 reply; 27+ messages in thread
From: Bryan Brattlof @ 2024-10-23 14:48 UTC (permalink / raw)
To: Santhosh Kumar K
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov,
n-francis
On October 21, 2024 thus sayeth Santhosh Kumar K:
> From: Neha Malcom Francis <n-francis@ti.com>
>
> Set CONFIG_NR_DRAM_BANKS to 2 as we have two banks described in the
> memory/ node for lower and higher addressible DDR regions.
>
> This allows use of FDT functions from fdt_support.c to set up and fixup
> the memory/ node correctly.
>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
> configs/j7200_evm_r5_defconfig | 1 +
> configs/j721e_evm_r5_defconfig | 1 +
> configs/j721s2_evm_r5_defconfig | 1 +
> configs/j784s4_evm_r5_defconfig | 1 +
> 4 files changed, 4 insertions(+)
>
> diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
> index 774a9eff439d..78dbd44f845f 100644
> --- a/configs/j7200_evm_r5_defconfig
> +++ b/configs/j7200_evm_r5_defconfig
> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x70000
> CONFIG_SPL_GPIO=y
> CONFIG_SPL_LIBCOMMON_SUPPORT=y
> CONFIG_SPL_LIBGENERIC_SUPPORT=y
> +CONFIG_NR_DRAM_BANKS=2
Should these go into Kconfig somewhere? These boards will always have 2
DRAAM banks unless someone is artificially limiting DDR sizes.
> CONFIG_SOC_K3_J721E=y
> CONFIG_K3_EARLY_CONS=y
> CONFIG_TARGET_J7200_R5_EVM=y
> diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
> index 3b4a7c3c0eeb..dc391f14b193 100644
> --- a/configs/j721e_evm_r5_defconfig
> +++ b/configs/j721e_evm_r5_defconfig
> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x70000
> CONFIG_SPL_GPIO=y
> CONFIG_SPL_LIBCOMMON_SUPPORT=y
> CONFIG_SPL_LIBGENERIC_SUPPORT=y
> +CONFIG_NR_DRAM_BANKS=2
> CONFIG_SOC_K3_J721E=y
> CONFIG_K3_EARLY_CONS=y
> CONFIG_K3_QOS=y
> diff --git a/configs/j721s2_evm_r5_defconfig b/configs/j721s2_evm_r5_defconfig
> index b6adb6a77d7e..374bc283be2d 100644
> --- a/configs/j721s2_evm_r5_defconfig
> +++ b/configs/j721s2_evm_r5_defconfig
> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x10000
> CONFIG_SPL_GPIO=y
> CONFIG_SPL_LIBCOMMON_SUPPORT=y
> CONFIG_SPL_LIBGENERIC_SUPPORT=y
> +CONFIG_NR_DRAM_BANKS=2
> CONFIG_SOC_K3_J721S2=y
> CONFIG_K3_EARLY_CONS=y
> CONFIG_K3_QOS=y
> diff --git a/configs/j784s4_evm_r5_defconfig b/configs/j784s4_evm_r5_defconfig
> index a11688194977..6fe36ab38f02 100644
> --- a/configs/j784s4_evm_r5_defconfig
> +++ b/configs/j784s4_evm_r5_defconfig
> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x10000
> CONFIG_SPL_GPIO=y
> CONFIG_SPL_LIBCOMMON_SUPPORT=y
> CONFIG_SPL_LIBGENERIC_SUPPORT=y
> +CONFIG_NR_DRAM_BANKS=2
> CONFIG_SOC_K3_J784S4=y
> CONFIG_K3_EARLY_CONS=y
> CONFIG_K3_QOS=y
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 6/8] configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2
2024-10-23 14:48 ` Bryan Brattlof
@ 2024-12-12 14:40 ` Santhosh Kumar K
0 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:40 UTC (permalink / raw)
To: Bryan Brattlof
Cc: trini, m-chawdhry, nm, u-kumar1, vigneshr, u-boot, w.egorov,
n-francis, s-k6
Hi, Bryan,
On 23/10/24 20:18, Bryan Brattlof wrote:
> On October 21, 2024 thus sayeth Santhosh Kumar K:
>> From: Neha Malcom Francis <n-francis@ti.com>
>>
>> Set CONFIG_NR_DRAM_BANKS to 2 as we have two banks described in the
>> memory/ node for lower and higher addressible DDR regions.
>>
>> This allows use of FDT functions from fdt_support.c to set up and fixup
>> the memory/ node correctly.
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>> configs/j7200_evm_r5_defconfig | 1 +
>> configs/j721e_evm_r5_defconfig | 1 +
>> configs/j721s2_evm_r5_defconfig | 1 +
>> configs/j784s4_evm_r5_defconfig | 1 +
>> 4 files changed, 4 insertions(+)
>>
>> diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
>> index 774a9eff439d..78dbd44f845f 100644
>> --- a/configs/j7200_evm_r5_defconfig
>> +++ b/configs/j7200_evm_r5_defconfig
>> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x70000
>> CONFIG_SPL_GPIO=y
>> CONFIG_SPL_LIBCOMMON_SUPPORT=y
>> CONFIG_SPL_LIBGENERIC_SUPPORT=y
>> +CONFIG_NR_DRAM_BANKS=2
>
> Should these go into Kconfig somewhere? These boards will always have 2
> DRAAM banks unless someone is artificially limiting DDR sizes.
Thanks for the suggestion. We can make it default at Kconfig.
Regards,
Santhosh.
>
>
>> CONFIG_SOC_K3_J721E=y
>> CONFIG_K3_EARLY_CONS=y
>> CONFIG_TARGET_J7200_R5_EVM=y
>> diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
>> index 3b4a7c3c0eeb..dc391f14b193 100644
>> --- a/configs/j721e_evm_r5_defconfig
>> +++ b/configs/j721e_evm_r5_defconfig
>> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x70000
>> CONFIG_SPL_GPIO=y
>> CONFIG_SPL_LIBCOMMON_SUPPORT=y
>> CONFIG_SPL_LIBGENERIC_SUPPORT=y
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_SOC_K3_J721E=y
>> CONFIG_K3_EARLY_CONS=y
>> CONFIG_K3_QOS=y
>> diff --git a/configs/j721s2_evm_r5_defconfig b/configs/j721s2_evm_r5_defconfig
>> index b6adb6a77d7e..374bc283be2d 100644
>> --- a/configs/j721s2_evm_r5_defconfig
>> +++ b/configs/j721s2_evm_r5_defconfig
>> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x10000
>> CONFIG_SPL_GPIO=y
>> CONFIG_SPL_LIBCOMMON_SUPPORT=y
>> CONFIG_SPL_LIBGENERIC_SUPPORT=y
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_SOC_K3_J721S2=y
>> CONFIG_K3_EARLY_CONS=y
>> CONFIG_K3_QOS=y
>> diff --git a/configs/j784s4_evm_r5_defconfig b/configs/j784s4_evm_r5_defconfig
>> index a11688194977..6fe36ab38f02 100644
>> --- a/configs/j784s4_evm_r5_defconfig
>> +++ b/configs/j784s4_evm_r5_defconfig
>> @@ -5,6 +5,7 @@ CONFIG_SYS_MALLOC_F_LEN=0x10000
>> CONFIG_SPL_GPIO=y
>> CONFIG_SPL_LIBCOMMON_SUPPORT=y
>> CONFIG_SPL_LIBGENERIC_SUPPORT=y
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_SOC_K3_J784S4=y
>> CONFIG_K3_EARLY_CONS=y
>> CONFIG_K3_QOS=y
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
` (5 preceding siblings ...)
2024-10-21 4:40 ` [PATCH v4 6/8] configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2 Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-22 10:04 ` Wadim Egorov
2024-10-21 4:40 ` [PATCH v4 8/8] arm: dts: k3-*-ddr: Add ss_cfg reg entry Santhosh Kumar K
2024-12-12 14:38 ` [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
8 siblings, 1 reply; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
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-k6@ti.com>
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
arch/arm/mach-k3/Makefile | 2 +-
arch/arm/mach-k3/include/mach/k3-ddr.h | 15 +++++
arch/arm/mach-k3/k3-ddr.c | 88 ++++++++++++++++++++++++++
board/ti/am62ax/evm.c | 17 +++--
board/ti/am62px/evm.c | 17 +++--
board/ti/am62x/evm.c | 62 ++----------------
board/ti/am64x/evm.c | 73 ++-------------------
board/ti/am65x/evm.c | 29 +--------
board/ti/j721e/evm.c | 29 +--------
board/ti/j721s2/evm.c | 35 +++-------
board/ti/j784s4/evm.c | 17 +++--
11 files changed, 160 insertions(+), 224 deletions(-)
create mode 100644 arch/arm/mach-k3/include/mach/k3-ddr.h
create mode 100644 arch/arm/mach-k3/k3-ddr.c
diff --git a/arch/arm/mach-k3/Makefile b/arch/arm/mach-k3/Makefile
index 8c4f6786a5b5..5ce7fc62d80c 100644
--- a/arch/arm/mach-k3/Makefile
+++ b/arch/arm/mach-k3/Makefile
@@ -6,7 +6,7 @@
obj-$(CONFIG_ARM64) += arm64/
obj-$(CONFIG_CPU_V7R) += r5/
obj-$(CONFIG_OF_LIBFDT) += common_fdt.o
-obj-y += common.o security.o
+obj-y += common.o security.o k3-ddr.o
obj-$(CONFIG_SOC_K3_AM62A7) += am62ax/
obj-$(CONFIG_SOC_K3_AM62P5) += am62px/
obj-$(CONFIG_SOC_K3_AM625) += am62x/
diff --git a/arch/arm/mach-k3/include/mach/k3-ddr.h b/arch/arm/mach-k3/include/mach/k3-ddr.h
new file mode 100644
index 000000000000..754e9ba29dac
--- /dev/null
+++ b/arch/arm/mach-k3/include/mach/k3-ddr.h
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024, Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#ifndef _K3_DDR_H_
+#define _K3_DDR_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_H_ */
diff --git a/arch/arm/mach-k3/k3-ddr.c b/arch/arm/mach-k3/k3-ddr.c
new file mode 100644
index 000000000000..2383d061e93e
--- /dev/null
+++ b/arch/arm/mach-k3/k3-ddr.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024, Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#include <fdt_support.h>
+#include <dm/uclass.h>
+#include <k3-ddrss.h>
+#include <spl.h>
+
+#include <asm/arch/k3-ddr.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;
+}
+
+#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);
+
+ ret = uclass_next_device_err(&dev);
+
+ while (ret && ret != -ENODEV)
+ {
+ 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);
+
+ 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/am62ax/evm.c b/board/ti/am62ax/evm.c
index 62d3664936e7..ce359be6eaa2 100644
--- a/board/ti/am62ax/evm.c
+++ b/board/ti/am62ax/evm.c
@@ -12,6 +12,7 @@
#include <env.h>
#include <fdt_support.h>
#include <spl.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/fdt_ops.h"
@@ -20,15 +21,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)
diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c
index 7362fa4520a3..e85bac40a910 100644
--- a/board/ti/am62px/evm.c
+++ b/board/ti/am62px/evm.c
@@ -13,6 +13,7 @@
#include <env.h>
#include <fdt_support.h>
#include <spl.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/fdt_ops.h"
struct efi_fw_image fw_images[] = {
@@ -53,15 +54,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
#if IS_ENABLED(CONFIG_BOARD_LATE_INIT)
int board_late_init(void)
diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
index 9bdd0223cdb6..3dd8d1722a1a 100644
--- a/board/ti/am62x/evm.c
+++ b/board/ti/am62x/evm.c
@@ -19,6 +19,7 @@
#include <asm/io.h>
#include <asm/arch/hardware.h>
#include <dm/uclass.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/fdt_ops.h"
@@ -85,11 +86,6 @@ int board_init(void)
return 0;
}
-int dram_init(void)
-{
- return fdtdec_setup_mem_size_base();
-}
-
#ifdef CONFIG_BOARD_LATE_INIT
int board_late_init(void)
{
@@ -98,11 +94,6 @@ int board_late_init(void)
}
#endif
-int dram_init_banksize(void)
-{
- return fdtdec_setup_memory_banksize();
-}
-
#if defined(CONFIG_SPL_BUILD)
void spl_board_init(void)
@@ -113,52 +104,13 @@ void spl_board_init(void)
}
-#if defined(CONFIG_K3_AM64_DDRSS)
-static void fixup_ddr_driver_for_ecc(struct spl_image_info *spl_image)
-{
- struct udevice *dev;
- int ret;
-
- dram_init_banksize();
-
- ret = uclass_get_device(UCLASS_RAM, 0, &dev);
- if (ret)
- panic("Cannot 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);
-}
-#else
-static 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;
- }
-
- /* dram_init functions use SPL fdt, and we must fixup u-boot fdt */
- 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
-
void spl_perform_fixups(struct spl_image_info *spl_image)
{
-#if defined(CONFIG_K3_AM64_DDRSS)
- fixup_ddr_driver_for_ecc(spl_image);
-#else
- fixup_memory_node(spl_image);
-#endif
+ 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
diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
index 609e5cf6d51f..61ab1714e5dc 100644
--- a/board/ti/am64x/evm.c
+++ b/board/ti/am64x/evm.c
@@ -15,6 +15,7 @@
#include <fdt_support.h>
#include <asm/arch/hardware.h>
#include <env.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/board_detect.h"
#include "../common/fdt_ops.h"
@@ -66,28 +67,6 @@ int board_init(void)
return 0;
}
-int dram_init(void)
-{
- s32 ret;
-
- ret = fdtdec_setup_mem_size_base();
- 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;
-}
-
#if defined(CONFIG_SPL_LOAD_FIT)
int board_fit_config_name_match(const char *name)
{
@@ -132,52 +111,14 @@ static int fixup_usb_boot(const void *fdt_blob)
}
#endif
-#if defined(CONFIG_K3_AM64_DDRSS)
-static void fixup_ddr_driver_for_ecc(struct spl_image_info *spl_image)
-{
- struct udevice *dev;
- int ret;
-
- dram_init_banksize();
-
- ret = uclass_get_device(UCLASS_RAM, 0, &dev);
- if (ret)
- panic("Cannot 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);
-}
-#else
-static 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;
- }
-
- /* dram_init functions use SPL fdt, and we must fixup u-boot fdt */
- 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
-
void spl_perform_fixups(struct spl_image_info *spl_image)
{
-#if defined(CONFIG_K3_AM64_DDRSS)
- fixup_ddr_driver_for_ecc(spl_image);
-#else
- fixup_memory_node(spl_image);
-#endif
+ 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);
+ }
#if CONFIG_IS_ENABLED(USB_STORAGE)
fixup_usb_boot(spl_image->fdt_addr);
diff --git a/board/ti/am65x/evm.c b/board/ti/am65x/evm.c
index 07073a5940b1..6658794a1375 100644
--- a/board/ti/am65x/evm.c
+++ b/board/ti/am65x/evm.c
@@ -20,6 +20,7 @@
#include <env.h>
#include <spl.h>
#include <linux/printk.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/board_detect.h"
#include "../common/fdt_ops.h"
@@ -49,17 +50,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
@@ -71,23 +61,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/j721e/evm.c b/board/ti/j721e/evm.c
index f3452ff0a8fb..d0385aa25973 100644
--- a/board/ti/j721e/evm.c
+++ b/board/ti/j721e/evm.c
@@ -15,6 +15,7 @@
#include <asm/gpio.h>
#include <spl.h>
#include <dm.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/board_detect.h"
#include "../common/fdt_ops.h"
@@ -77,17 +78,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
@@ -99,23 +89,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..be21b0d95da5 100644
--- a/board/ti/j721s2/evm.c
+++ b/board/ti/j721s2/evm.c
@@ -21,6 +21,7 @@
#include <dm.h>
#include <dm/uclass-internal.h>
#include <dm/root.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/board_detect.h"
#include "../common/fdt_ops.h"
@@ -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 548dbd5925df..c843c9501588 100644
--- a/board/ti/j784s4/evm.c
+++ b/board/ti/j784s4/evm.c
@@ -10,6 +10,7 @@
#include <efi_loader.h>
#include <init.h>
#include <spl.h>
+#include <asm/arch/k3-ddr.h>
#include "../common/fdt_ops.h"
DECLARE_GLOBAL_DATA_PTR;
@@ -52,15 +53,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)
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v4 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled
2024-10-21 4:40 ` [PATCH v4 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled Santhosh Kumar K
@ 2024-10-22 10:04 ` Wadim Egorov
2024-12-12 14:40 ` Santhosh Kumar K
0 siblings, 1 reply; 27+ messages in thread
From: Wadim Egorov @ 2024-10-22 10:04 UTC (permalink / raw)
To: Santhosh Kumar K, trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, n-francis
Hi Santhosh,
Am 21.10.24 um 06:40 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-k6@ti.com>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
<snip>
> +obj-y += common.o security.o k3-ddr.o
> obj-$(CONFIG_SOC_K3_AM62A7) += am62ax/
> obj-$(CONFIG_SOC_K3_AM62P5) += am62px/
> obj-$(CONFIG_SOC_K3_AM625) += am62x/
> diff --git a/arch/arm/mach-k3/include/mach/k3-ddr.h b/arch/arm/mach-k3/include/mach/k3-ddr.h
> new file mode 100644
> index 000000000000..754e9ba29dac
> --- /dev/null
<snip>
> +#include <fdt_support.h>
> +#include <dm/uclass.h>
> +#include <k3-ddrss.h>
> +#include <spl.h>
> +
> +#include <asm/arch/k3-ddr.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;
> +}
> +Moving dram_init() and dram_init_banksize() into a generic part forces
every K3 based board to use your implementation.
This also breaks the builds for phycore_am62x, phycore_am64x and also
the verdin-am62,
phycore-am62x.c:49: multiple definition of `dram_init'
phycore-am62x.c:97: multiple definition of `dram_init_banksize
Regards,
Wadim
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled
2024-10-22 10:04 ` Wadim Egorov
@ 2024-12-12 14:40 ` Santhosh Kumar K
0 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:40 UTC (permalink / raw)
To: Wadim Egorov, trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, n-francis, s-k6
Hi, Wadim,
On 22/10/24 15:34, Wadim Egorov wrote:
> Hi Santhosh,
>
> Am 21.10.24 um 06:40 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-k6@ti.com>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
> <snip>
>
>> +obj-y += common.o security.o k3-ddr.o
>> obj-$(CONFIG_SOC_K3_AM62A7) += am62ax/
>> obj-$(CONFIG_SOC_K3_AM62P5) += am62px/
>> obj-$(CONFIG_SOC_K3_AM625) += am62x/
>> diff --git a/arch/arm/mach-k3/include/mach/k3-ddr.h
>> b/arch/arm/mach-k3/include/mach/k3-ddr.h
>> new file mode 100644
>> index 000000000000..754e9ba29dac
>> --- /dev/null
> <snip>
>
>> +#include <fdt_support.h>
>> +#include <dm/uclass.h>
>> +#include <k3-ddrss.h>
>> +#include <spl.h>
>> +
>> +#include <asm/arch/k3-ddr.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;
>> +}
>> +Moving dram_init() and dram_init_banksize() into a generic part forces
> every K3 based board to use your implementation.
>
> This also breaks the builds for phycore_am62x, phycore_am64x and also
> the verdin-am62,
>
> phycore-am62x.c:49: multiple definition of `dram_init'
> phycore-am62x.c:97: multiple definition of `dram_init_banksize
Oops, that's bad. I'll fix this in the next version.
Regards,
Santhosh.
>
> Regards,
> Wadim
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 8/8] arm: dts: k3-*-ddr: Add ss_cfg reg entry
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
` (6 preceding siblings ...)
2024-10-21 4:40 ` [PATCH v4 7/8] board: ti: Pull redundant DDR functions to a common location and Fixup DDR size when ECC is enabled Santhosh Kumar K
@ 2024-10-21 4:40 ` Santhosh Kumar K
2024-10-22 9:22 ` Neha Malcom Francis
2024-12-12 14:38 ` [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
8 siblings, 1 reply; 27+ messages in thread
From: Santhosh Kumar K @ 2024-10-21 4:40 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
Add ss_cfg memory region which maps the DDRSS configuration region
for the memory controller node.
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
arch/arm/dts/k3-am62a-ddr.dtsi | 7 ++++---
arch/arm/dts/k3-j721s2-ddr.dtsi | 12 ++++++++----
arch/arm/dts/k3-j784s4-ddr.dtsi | 24 ++++++++++++++++--------
3 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/arch/arm/dts/k3-am62a-ddr.dtsi b/arch/arm/dts/k3-am62a-ddr.dtsi
index 8629ea45b847..42e41f78505a 100644
--- a/arch/arm/dts/k3-am62a-ddr.dtsi
+++ b/arch/arm/dts/k3-am62a-ddr.dtsi
@@ -4,11 +4,12 @@
*/
/ {
- memorycontroller: memory-controller@f308000 {
+ memorycontroller: memory-controller@f300000 {
compatible = "ti,am62a-ddrss";
reg = <0x00 0x0f308000 0x00 0x4000>,
- <0x00 0x43014000 0x00 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x00 0x43014000 0x00 0x100>,
+ <0x00 0x0f300000 0x00 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
ti,ddr-freq1 = <DDRSS_PLL_FREQUENCY_1>;
ti,ddr-freq2 = <DDRSS_PLL_FREQUENCY_2>;
ti,ddr-fhs-cnt = <DDRSS_PLL_FHS_CNT>;
diff --git a/arch/arm/dts/k3-j721s2-ddr.dtsi b/arch/arm/dts/k3-j721s2-ddr.dtsi
index 345e2b84f9e8..9764085163c4 100644
--- a/arch/arm/dts/k3-j721s2-ddr.dtsi
+++ b/arch/arm/dts/k3-j721s2-ddr.dtsi
@@ -5,6 +5,8 @@
&main_navss {
ranges = <0x00 0x00114000 0x00 0x00114000 0x00 0x00000100>, // ctrl_mmr_lpr
+ <0x00 0x02980000 0x00 0x02980000 0x00 0x00000200>, // ss cfg 0
+ <0x00 0x029a0000 0x00 0x029a0000 0x00 0x00000200>, // ss cfg 1
<0x00 0x02990000 0x00 0x02990000 0x00 0x00004000>, // ddr0 cfg
<0x00 0x029b0000 0x00 0x029b0000 0x00 0x00004000>, // ddr1 cfg
<0x00 0x30000000 0x00 0x30000000 0x00 0x0c400000>;
@@ -24,8 +26,9 @@
memorycontroller0: memorycontroller@2990000 {
compatible = "ti,j721s2-ddrss";
reg = <0x0 0x02990000 0x0 0x4000>,
- <0x0 0x0114000 0x0 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x0 0x0114000 0x0 0x100>,
+ <0x0 0x02980000 0x0 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
power-domains = <&k3_pds 138 TI_SCI_PD_SHARED>,
<&k3_pds 96 TI_SCI_PD_SHARED>;
clocks = <&k3_clks 138 0>, <&k3_clks 43 2>;
@@ -2232,8 +2235,9 @@
memorycontroller1: memorycontroller@29b0000 {
compatible = "ti,j721s2-ddrss";
reg = <0x0 0x029b0000 0x0 0x4000>,
- <0x0 0x0114000 0x0 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x0 0x0114000 0x0 0x100>,
+ <0x0 0x029a0000 0x0 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
power-domains = <&k3_pds 139 TI_SCI_PD_SHARED>,
<&k3_pds 97 TI_SCI_PD_SHARED>;
clocks = <&k3_clks 139 0>, <&k3_clks 43 2>;
diff --git a/arch/arm/dts/k3-j784s4-ddr.dtsi b/arch/arm/dts/k3-j784s4-ddr.dtsi
index 1c3242b0870c..fc74c539331e 100644
--- a/arch/arm/dts/k3-j784s4-ddr.dtsi
+++ b/arch/arm/dts/k3-j784s4-ddr.dtsi
@@ -9,6 +9,10 @@
<0x00 0x029b0000 0x00 0x029b0000 0x00 0x00004000>, // ddr1 cfg
<0x00 0x029d0000 0x00 0x029d0000 0x00 0x00004000>, // ddr2 cfg
<0x00 0x029f0000 0x00 0x029f0000 0x00 0x00004000>, // ddr3 cfg
+ <0x00 0x02980000 0x00 0x02980000 0x00 0x00000200>, // ss cfg 0
+ <0x00 0x029a0000 0x00 0x029a0000 0x00 0x00000200>, // ss cfg 1
+ <0x00 0x029c0000 0x00 0x029c0000 0x00 0x00000200>, // ss cfg 2
+ <0x00 0x029e0000 0x00 0x029e0000 0x00 0x00000200>, // ss cfg 3
<0x00 0x30000000 0x00 0x30000000 0x00 0x0c400000>;
msmc0: msmc {
@@ -26,8 +30,9 @@
memorycontroller0: memorycontroller@2990000 {
compatible = "ti,j721s2-ddrss";
reg = <0x0 0x02990000 0x0 0x4000>,
- <0x0 0x0114000 0x0 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x0 0x0114000 0x0 0x100>,
+ <0x0 0x02980000 0x0 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
power-domains = <&k3_pds 191 TI_SCI_PD_SHARED>,
<&k3_pds 131 TI_SCI_PD_SHARED>;
clocks = <&k3_clks 191 1>, <&k3_clks 78 2>;
@@ -2234,8 +2239,9 @@
memorycontroller1: memorycontroller@29b0000 {
compatible = "ti,j721s2-ddrss";
reg = <0x0 0x029b0000 0x0 0x4000>,
- <0x0 0x0114000 0x0 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x0 0x0114000 0x0 0x100>,
+ <0x0 0x029a0000 0x0 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
power-domains = <&k3_pds 192 TI_SCI_PD_SHARED>,
<&k3_pds 132 TI_SCI_PD_SHARED>;
clocks = <&k3_clks 192 1>, <&k3_clks 78 2>;
@@ -4442,8 +4448,9 @@
memorycontroller2: memorycontroller@29d0000 {
compatible = "ti,j721s2-ddrss";
reg = <0x0 0x029d0000 0x0 0x4000>,
- <0x0 0x0114000 0x0 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x0 0x0114000 0x0 0x100>,
+ <0x0 0x029c0000 0x0 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
power-domains = <&k3_pds 193 TI_SCI_PD_SHARED>,
<&k3_pds 133 TI_SCI_PD_SHARED>;
clocks = <&k3_clks 193 1>, <&k3_clks 78 2>;
@@ -6650,8 +6657,9 @@
memorycontroller3: memorycontroller@29f0000 {
compatible = "ti,j721s2-ddrss";
reg = <0x0 0x029f0000 0x0 0x4000>,
- <0x0 0x0114000 0x0 0x100>;
- reg-names = "cfg", "ctrl_mmr_lp4";
+ <0x0 0x0114000 0x0 0x100>,
+ <0x0 0x29e0000 0x0 0x200>;
+ reg-names = "cfg", "ctrl_mmr_lp4", "ss_cfg";
power-domains = <&k3_pds 194 TI_SCI_PD_SHARED>,
<&k3_pds 139 TI_SCI_PD_SHARED>;
clocks = <&k3_clks 194 1>, <&k3_clks 78 2>;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v4 8/8] arm: dts: k3-*-ddr: Add ss_cfg reg entry
2024-10-21 4:40 ` [PATCH v4 8/8] arm: dts: k3-*-ddr: Add ss_cfg reg entry Santhosh Kumar K
@ 2024-10-22 9:22 ` Neha Malcom Francis
2024-12-12 14:40 ` Santhosh Kumar K
0 siblings, 1 reply; 27+ messages in thread
From: Neha Malcom Francis @ 2024-10-22 9:22 UTC (permalink / raw)
To: Santhosh Kumar K, trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov
Hi Santhosh
On 21/10/24 10:10, Santhosh Kumar K wrote:
> Add ss_cfg memory region which maps the DDRSS configuration region
> for the memory controller node.
>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
> arch/arm/dts/k3-am62a-ddr.dtsi | 7 ++++---
> arch/arm/dts/k3-j721s2-ddr.dtsi | 12 ++++++++----
> arch/arm/dts/k3-j784s4-ddr.dtsi | 24 ++++++++++++++++--------
> 3 files changed, 28 insertions(+), 15 deletions(-)
>
Thank you for this series. Maybe I missed something, but why is this DT patch at
the very end of the series? Doesn't it make sense to include it before the
patches triggering inline ECC i.e. patch 7/8?
Sanity tested inline ECC on j784s4 and j721s2 and all looks good [1].
Tested-by: Neha Malcom Francis <n-francis@ti.com>
[1] https://gist.github.com/nehamalcom/66f1a2ef5288a94eebabe97708b5a277
--
Thanking You
Neha Malcom Francis
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 8/8] arm: dts: k3-*-ddr: Add ss_cfg reg entry
2024-10-22 9:22 ` Neha Malcom Francis
@ 2024-12-12 14:40 ` Santhosh Kumar K
0 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:40 UTC (permalink / raw)
To: Neha Malcom Francis, trini, m-chawdhry, nm, u-kumar1, vigneshr,
bb
Cc: u-boot, w.egorov, s-k6
Hi, Neha,
On 22/10/24 14:52, Neha Malcom Francis wrote:
> Hi Santhosh
>
> On 21/10/24 10:10, Santhosh Kumar K wrote:
>> Add ss_cfg memory region which maps the DDRSS configuration region
>> for the memory controller node.
>>
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>> arch/arm/dts/k3-am62a-ddr.dtsi | 7 ++++---
>> arch/arm/dts/k3-j721s2-ddr.dtsi | 12 ++++++++----
>> arch/arm/dts/k3-j784s4-ddr.dtsi | 24 ++++++++++++++++--------
>> 3 files changed, 28 insertions(+), 15 deletions(-)
>>
>
> Thank you for this series. Maybe I missed something, but why is this DT
> patch at the very end of the series? Doesn't it make sense to include it
> before the patches triggering inline ECC i.e. patch 7/8?
True, will change the order in next version.
Regards,
Santhosh.
>
> Sanity tested inline ECC on j784s4 and j721s2 and all looks good [1].
>
> Tested-by: Neha Malcom Francis <n-francis@ti.com>
>
> [1] https://gist.github.com/nehamalcom/66f1a2ef5288a94eebabe97708b5a277
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 0/8] Inline ECC Series
2024-10-21 4:40 [PATCH v4 0/8] Inline ECC Series Santhosh Kumar K
` (7 preceding siblings ...)
2024-10-21 4:40 ` [PATCH v4 8/8] arm: dts: k3-*-ddr: Add ss_cfg reg entry Santhosh Kumar K
@ 2024-12-12 14:38 ` Santhosh Kumar K
8 siblings, 0 replies; 27+ messages in thread
From: Santhosh Kumar K @ 2024-12-12 14:38 UTC (permalink / raw)
To: trini, m-chawdhry, nm, u-kumar1, vigneshr, bb
Cc: u-boot, w.egorov, n-francis, s-k6
Hi, everyone,
On 21/10/24 10:10, Santhosh Kumar K wrote:
> Hello,
>
> This series is to:
>
> Add support for Inline ECC in DDR for AM64X, AM62X, AM62AX, AM62PX,
> J721S2 and J784S4 devices.
>
> (1/8) Enable ECC priming with BIST engine
> (2/8) Add a function to store base address and size of RAM's banks
> in a 64-bit device private data
> (3/8) Setup the ECC region start and range
> (4/8) Enable ECC 1-bit error, 2-bit error and multiple-bit
> error interrupts
> (5/8) Add CONFIG_K3_INLINE_ECC
> (6/8) Set NR_DRAM_BANKS to 2
> (7/8) Pull Redundant DDR functions to a common location and Fixup
> DDR size when ECC is enabled
> (8/8) Add ss_cfg reg entry
>
> Changes since v3:
> Pull redundant DDR functions to arch/arm/mach-k3/*
>
> v3: https://lore.kernel.org/all/20240523050430.455201-1-s-k6@ti.com/
>
> Changes since v2:
> Removed 'default n' in Kconfig as that is default setting
>
> v2: https://lore.kernel.org/u-boot/20240510084707.1903133-1-s-k6@ti.com/
>
> Changes since v1:
> Add support for J7* devices.
>
> v1: https://lore.kernel.org/u-boot/20240131060213.1128024-1-s-k6@ti.com/
>
> Test Results: https://gist.github.com/santhosh21/25937355aa2c134a64fa76480ac6a4a2
I apologize for the delayed response!
Regards,
Santhosh.
>
> Thanks and Regards,
> Santhosh.
>
> Georgi Vlaev (1):
> ram: k3-ddrss: Use the DDR controller BIST engine for ECC priming
>
> Neha Malcom Francis (2):
> drivers: ram: Kconfig: Add CONFIG_K3_INLINE_ECC
> configs: j7*_evm_r5_defconfig: Set NR_DRAM_BANKS to 2
>
> Santhosh Kumar K (5):
> ram: k3-ddrss: Add k3_ddrss_ddr_bank_base_size_calc() to solve
> 'calculations restricted to 32 bits' issue
> ram: k3-ddrss: Setup ECC region start and range
> ram: k3-ddrss: Enable ECC interrupts
> board: ti: Pull redundant DDR functions to a common location and Fixup
> DDR size when ECC is enabled
> arm: dts: k3-*-ddr: Add ss_cfg reg entry
>
> arch/arm/dts/k3-am62a-ddr.dtsi | 7 +-
> arch/arm/dts/k3-j721s2-ddr.dtsi | 12 +-
> arch/arm/dts/k3-j784s4-ddr.dtsi | 24 ++-
> arch/arm/mach-k3/Makefile | 2 +-
> arch/arm/mach-k3/include/mach/k3-ddr.h | 15 ++
> arch/arm/mach-k3/k3-ddr.c | 88 ++++++++++
> board/ti/am62ax/evm.c | 17 +-
> board/ti/am62px/evm.c | 17 +-
> board/ti/am62x/evm.c | 62 +------
> board/ti/am64x/evm.c | 73 +--------
> board/ti/am65x/evm.c | 29 +---
> board/ti/j721e/evm.c | 29 +---
> board/ti/j721s2/evm.c | 35 ++--
> board/ti/j784s4/evm.c | 17 +-
> configs/j7200_evm_r5_defconfig | 1 +
> configs/j721e_evm_r5_defconfig | 1 +
> configs/j721s2_evm_r5_defconfig | 1 +
> configs/j784s4_evm_r5_defconfig | 1 +
> drivers/ram/Kconfig | 10 ++
> drivers/ram/k3-ddrss/k3-ddrss.c | 214 +++++++++++++++++++++----
> 20 files changed, 387 insertions(+), 268 deletions(-)
> create mode 100644 arch/arm/mach-k3/include/mach/k3-ddr.h
> create mode 100644 arch/arm/mach-k3/k3-ddr.c
>
^ permalink raw reply [flat|nested] 27+ messages in thread