[PATCH 0/8] ram: k3-ddrss: Support partial inline ECC

[PATCH 0/8] ram: k3-ddrss: Support partial inline ECC

[Neha Malcom Francis]

Currently, the inline ECC implementation enables inline ECC across the
entire DDR space. However this is not always required and a more common
ask is to have only a portion of the DDR protected as enabling ECC
impacts read/write performance metrics.

This series aims to modify the logic to firstly support partial inline
ECC in its' most basic form which works for single controllers. Then it
introduces an algorithm to support multi DDR controllers where
interleaving plays a role. Since interleaving is handled by the MSMC, it
only makes sense to have the MSMC decide the inline ECC ranges for each
DDR.

WIP: A commandline test case patch for verifying the correct behaviour
of inline ECC including partial case. Will add the patch in v2, for now
repeated boot tests on the affected platforms as well as internal
memtester runs stand as testing.

Boot logs (J721S2): https://gist.github.com/nehamalcom/e5a76bece133c1ec716e2ed94d60ce74

Neha Malcom Francis (8):
  k3-ddr.c: Remove unwanted header files
  ram: k3-ddrss: Use DDR address instead of system address for
    ecc_regions
  ram: k3-ddrss: Add comment about ecc_reserved_space
  ram: k3-ddrss: Add support for a partial inline ECC region
  ram: k3-ddrss: Add CONFIG_K3_MULTI_DDR
  ram: k3-ddrss: Add support for number of controllers under MSMC
  ram: k3-ddrss: Add support for MSMC calculation of DDR inline ECC
    regions
  ram: k3-ddrss: Add support for partial inline ECC in multi-DDR systems

 arch/arm/mach-k3/k3-ddr.c       |   1 -
 board/ti/common/k3-ddr.c        |   1 -
 drivers/ram/Kconfig             |  10 ++
 drivers/ram/k3-ddrss/k3-ddrss.c | 227 ++++++++++++++++++++++++++++++--
 4 files changed, 226 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH 1/8] k3-ddr.c: Remove unwanted header files

[Neha Malcom Francis]

This header file is not in use in these arch/board specific files,
remove them.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 arch/arm/mach-k3/k3-ddr.c | 1 -
 board/ti/common/k3-ddr.c  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/arch/arm/mach-k3/k3-ddr.c b/arch/arm/mach-k3/k3-ddr.c
index 6e3e60cdc86..026381490da 100644
--- a/arch/arm/mach-k3/k3-ddr.c
+++ b/arch/arm/mach-k3/k3-ddr.c
@@ -5,7 +5,6 @@
 
 #include <fdt_support.h>
 #include <dm/uclass.h>
-#include <k3-ddrss.h>
 #include <spl.h>
 
 #include <asm/arch/k3-ddr.h>
diff --git a/board/ti/common/k3-ddr.c b/board/ti/common/k3-ddr.c
index a8425da8de5..926f6ea0e7d 100644
--- a/board/ti/common/k3-ddr.c
+++ b/board/ti/common/k3-ddr.c
@@ -5,7 +5,6 @@
 
 #include <fdt_support.h>
 #include <dm/uclass.h>
-#include <k3-ddrss.h>
 #include <spl.h>
 
 #include "k3-ddr.h"
-- 
2.34.1

[PATCH 2/8] ram: k3-ddrss: Use DDR address instead of system address for ecc_regions

[Neha Malcom Francis]

Let ecc_regions[x].start reflect the start of the ECC region in terms of
DDR addressing rather than system addressing. This will make it easier
to extend the usage of the same ecc_regions structure for multi-DDR
systems as well.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/ram/k3-ddrss/k3-ddrss.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index 05ea61bbfe9..24932cd837c 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -730,7 +730,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
 	u32 val;
 
 	/* Only Program region 0 which covers full ddr space */
-	k3_ddrss_set_ecc_range_r0(base, ecc_region_start - ddrss->ddr_bank_base[0], ecc_range);
+	k3_ddrss_set_ecc_range_r0(base, ecc_region_start, ecc_range);
 
 	/* Enable ECC, RMW, WR_ALLOC */
 	writel(DDRSS_ECC_CTRL_REG_ECC_EN | DDRSS_ECC_CTRL_REG_RMW_EN |
@@ -799,7 +799,7 @@ 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 = ddrss->ddr_bank_base[0];
+		ddrss->ecc_regions[0].start = ddrss->ddr_bank_base[0] - 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

[PATCH 3/8] ram: k3-ddrss: Add comment about ecc_reserved_space

[Neha Malcom Francis]

The reserved space needed for storing the parity remains the same no
matter the size of the region that is being protected. Add this as a
comment for better code understanding.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/ram/k3-ddrss/k3-ddrss.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index 24932cd837c..ab46098adbf 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -715,6 +715,10 @@ static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
 {
 	fdtdec_setup_mem_size_base_lowest();
 
+	/*
+	 * Reserved region remains 1/9th of the total DDR available no matter the
+	 * size of the region under protection
+	 */
 	ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
 	do_div(ddrss->ecc_reserved_space, 9);
 
-- 
2.34.1

[PATCH 4/8] ram: k3-ddrss: Add support for a partial inline ECC region

[Neha Malcom Francis]

Instead of defaulting to choosing the entire DDR region when enabling
inline ECC, allow picking of a range within the DDR space using DT to
enable.

It expects such a node within the memory node, in the absence of which
we resort to enabling inline ECC for the entire DDR region:

inline_ecc: protected@9e780000 {
        device_type = "ecc";
        reg = <0x9e780000 0x0080000>;
        bootph-all;
};

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/ram/k3-ddrss/k3-ddrss.c | 52 +++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index ab46098adbf..ea2578cda58 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -149,6 +149,7 @@ struct k3_ddrss_desc {
 	lpddr4_obj *driverdt;
 	lpddr4_config config;
 	lpddr4_privatedata pd;
+	struct k3_ddrss_ecc_region ecc_range;
 	struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
 	u64 ecc_reserved_space;
 	u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
@@ -711,6 +712,30 @@ static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
 		ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
 }
 
+static void k3_ddrss_ddr_inline_ecc_base_size_calc(struct k3_ddrss_ecc_region *range)
+{
+	fdt_addr_t base;
+	fdt_size_t size;
+	ofnode node1;
+
+	node1 = ofnode_null();
+
+	do {
+		node1 = ofnode_by_prop_value(node1, "device_type", "ecc", 4);
+	} while (!ofnode_is_enabled(node1));
+
+	base = ofnode_get_addr_size(node1, "reg", &size);
+
+	if (base == FDT_ADDR_T_NONE) {
+		debug("%s: Failed to get ECC node reg and size\n", __func__);
+		range->start = 0;
+		range->range = 0;
+	} else {
+		range->start = base;
+		range->range = size;
+	}
+}
+
 static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
 {
 	fdtdec_setup_mem_size_base_lowest();
@@ -759,8 +784,11 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
 
 static int k3_ddrss_probe(struct udevice *dev)
 {
+	u64 end;
 	int ret;
 	struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
+	__maybe_unused struct k3_ddrss_data *ddrss_data = (struct k3_ddrss_data *)dev_get_driver_data(dev);
+	__maybe_unused struct k3_ddrss_ecc_region *range = &ddrss->ecc_range;
 
 	debug("%s(dev=%p)\n", __func__, dev);
 
@@ -802,9 +830,27 @@ 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 = ddrss->ddr_bank_base[0] - ddrss->ddr_bank_base[0];
-		ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
+		k3_ddrss_ddr_inline_ecc_base_size_calc(range);
+		if (!range->range) {
+			/* Configure entire DDR space by default */
+			debug("%s: Defaulting to protecting entire DDR space using inline ECC\n",
+			      __func__);
+			ddrss->ecc_range.start = ddrss->ddr_bank_base[0];
+			ddrss->ecc_range.range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
+		} else {
+			ddrss->ecc_range.start = range->start;
+			ddrss->ecc_range.range = range->range;
+		}
+
+		end = ddrss->ecc_range.start + ddrss->ecc_range.range;
+
+		if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
+			ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
+		else
+			ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
+
+		ddrss->ecc_regions[0].start = ddrss->ecc_range.start - ddrss->ddr_bank_base[0];
+
 		k3_ddrss_lpddr4_ecc_init(ddrss);
 	}
 
-- 
2.34.1

[PATCH 5/8] ram: k3-ddrss: Add CONFIG_K3_MULTI_DDR

[Neha Malcom Francis]

As we increase the functionalities that the K3 DDRSS sub-system support,
it is becoming more evident that the same logic cannot apply to both
single as well as multiple DDR controller devices. Add
CONFIG_K3_MULTI_DDR to be used to differentiate between the two.

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 899d7585489..0d19021d8e3 100644
--- a/drivers/ram/Kconfig
+++ b/drivers/ram/Kconfig
@@ -126,6 +126,16 @@ config K3_INLINE_ECC
 	  need to be primed with a predefined value prior to enabling ECC
 	  check.
 
+config K3_MULTI_DDR
+	bool "Enable support for multiple K3 DDRSS controllers"
+	depends on K3_DDRSS
+	help
+	  Enabling this option adds support for configuring multiple DDR memory
+	  controllers for K3 devices. The external memory interleave layer
+	  present in the MSMC (Multicore Shared Memory Controller) is
+	  responsible for interleaving between the controllers.
+	default y if SOC_K3_J721S2 || SOC_K3_J784S4
+
 source "drivers/ram/aspeed/Kconfig"
 source "drivers/ram/cadence/Kconfig"
 source "drivers/ram/octeon/Kconfig"
-- 
2.34.1

[PATCH 6/8] ram: k3-ddrss: Add support for number of controllers under MSMC

[Neha Malcom Francis]

In K3 multi-DDR systems, the MSMC is responsible for the interleave
mechanism across all the DDR controllers. Add support for MSMC to obtain
the number of controllers it's responsible for using the DT.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/ram/k3-ddrss/k3-ddrss.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index ea2578cda58..51b4c1bf45a 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -121,6 +121,7 @@ struct k3_msmc {
 	enum ecc_enable enable;
 	enum emif_config config;
 	enum emif_active active;
+	u32 num_ddr;
 };
 
 #define K3_DDRSS_MAX_ECC_REGIONS		3
@@ -985,6 +986,13 @@ static int k3_msmc_probe(struct udevice *dev)
 		return -EINVAL;
 	}
 
+	ret = device_get_child_count(dev);
+	if (ret <= 0) {
+		dev_err(dev, "no child ddr nodes present");
+		return -EINVAL;
+	}
+	msmc->num_ddr = ret;
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH 7/8] ram: k3-ddrss: Add support for MSMC calculation of DDR inline ECC regions

[Neha Malcom Francis]

Add support for calculation of the protected regions for each DDR in
multi-DDR systems. Since MSMC is the parent node of the individual DDRs
as well as responsible for their interleaving, it only makes sense for
MSMC to contain the logic for dividing the regions.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/ram/k3-ddrss/k3-ddrss.c | 122 ++++++++++++++++++++++++++++++--
 1 file changed, 115 insertions(+), 7 deletions(-)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index 51b4c1bf45a..205dce51799 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -53,6 +53,7 @@
 
 #define SINGLE_DDR_SUBSYSTEM	0x1
 #define MULTI_DDR_SUBSYSTEM	0x2
+#define MAX_MULTI_DDR 4
 
 #define MULTI_DDR_CFG0  0x00114100
 #define MULTI_DDR_CFG1  0x00114104
@@ -76,6 +77,24 @@ enum intrlv_gran {
 	GRAN_16GB
 };
 
+u64 gran_bytes[] = {
+	0x80,
+	0x200,
+	0x800,
+	0x1000,
+	0x4000,
+	0x8000,
+	0x80000,
+	0x40000000,
+	0x60000000,
+	0x80000000,
+	0xC0000000,
+	0x100000000,
+	0x180000000,
+	0x200000000,
+	0x400000000
+};
+
 enum intrlv_size {
 	SIZE_0,
 	SIZE_128MB,
@@ -115,6 +134,13 @@ enum emif_active {
 	EMIF_ALL
 };
 
+#define K3_DDRSS_MAX_ECC_REGIONS		3
+
+struct k3_ddrss_ecc_region {
+	u64 start;
+	u64 range;
+};
+
 struct k3_msmc {
 	enum intrlv_gran gran;
 	enum intrlv_size size;
@@ -122,13 +148,7 @@ struct k3_msmc {
 	enum emif_config config;
 	enum emif_active active;
 	u32 num_ddr;
-};
-
-#define K3_DDRSS_MAX_ECC_REGIONS		3
-
-struct k3_ddrss_ecc_region {
-	u64 start;
-	u64 range;
+	struct k3_ddrss_ecc_region R0[MAX_MULTI_DDR];
 };
 
 struct k3_ddrss_desc {
@@ -914,6 +934,83 @@ U_BOOT_DRIVER(k3_ddrss) = {
 	.priv_auto		= sizeof(struct k3_ddrss_desc),
 };
 
+#if IS_ENABLED(CONFIG_K3_MULTI_DDR)
+static int k3_msmc_calculate_r0_regions(struct k3_msmc *msmc)
+{
+	u32 n1;
+	u32 size, ret = 0;
+	u32 gran = gran_bytes[msmc->gran];
+	u32 num_ddr = msmc->num_ddr;
+	struct k3_ddrss_ecc_region *range = NULL;
+	struct k3_ddrss_ecc_region R[num_ddr];
+
+	range = kzalloc(sizeof(range), GFP_KERNEL);
+	if (!range) {
+		debug("%s: failed to allocate range\n", __func__);
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	k3_ddrss_ddr_inline_ecc_base_size_calc(range);
+
+	if (!range->range) {
+		ret = -EINVAL;
+		goto range_err;
+	}
+
+	memset(R, 0, num_ddr);
+
+	/* Find the first controller in the range */
+	n1 = ((range->start / gran) % num_ddr);
+	size = range->range;
+
+	if (size < gran) {
+		R[n1].start = range->start - 0x80000000;
+		R[n1].range = range->start + range->range - 0x80000000;
+	} else {
+		u32 chunk_start_addr = range->start;
+		u32 chunk_size = range->range;
+
+		while (chunk_size > 0) {
+			u32 edge;
+			u32 end = range->start + range->range;
+
+			if ((chunk_start_addr % gran) == 0)
+				edge = chunk_start_addr + gran;
+			else
+				edge = ((chunk_start_addr + (gran - 1)) & (-gran));
+
+			if (edge > end)
+				break;
+
+			if (R[n1].start == 0)
+				R[n1].start = chunk_start_addr;
+
+			R[n1].range = edge - R[n1].start;
+			chunk_size = end - edge;
+			chunk_start_addr = edge;
+
+			if (n1 == (num_ddr - 1))
+				n1 = 0;
+			else
+				n1++;
+		}
+
+		for (int i = 0; i < num_ddr; i++)
+			R[i].start = (R[i].start - 0x80000000 - (gran * i)) / num_ddr;
+	}
+
+	for (int i = 0; i < num_ddr; i++) {
+		msmc->R0[i].start = R[i].start;
+		msmc->R0[i].range = R[i].range;
+	}
+
+range_err:
+	free(range);
+	return ret;
+}
+#endif
+
 static int k3_msmc_set_config(struct k3_msmc *msmc)
 {
 	u32 ddr_cfg0 = 0;
@@ -993,6 +1090,17 @@ static int k3_msmc_probe(struct udevice *dev)
 	}
 	msmc->num_ddr = ret;
 
+#if IS_ENABLED(CONFIG_K3_MULTI_DDR) && IS_ENABLED(CONFIG_K3_INLINE_ECC)
+	ret = k3_msmc_calculate_r0_regions(msmc);
+	if (ret) {
+		/* Default to enabling inline ECC for entire DDR region */
+		debug("%s: calculation of inline ECC regions failed, defaulting to entire region\n",
+		      __func__);
+
+		/* Use first R0 entry as a flag to denote MSMC calculation failure */
+		msmc->R0[0].start = -1;
+	}
+#endif
 	return 0;
 }
 
-- 
2.34.1

[PATCH 8/8] ram: k3-ddrss: Add support for partial inline ECC in multi-DDR systems

[Neha Malcom Francis]

The existing approach does not account for interleaving in the DDRs when
setting up regions. There is support for MSMC to calculate the regions
for each DDR, so modify k3_ddrss_probe to set the regions accordingly
for multi-DDR systems.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/ram/k3-ddrss/k3-ddrss.c | 43 +++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index 205dce51799..8429dddeb58 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -808,8 +808,10 @@ static int k3_ddrss_probe(struct udevice *dev)
 	u64 end;
 	int ret;
 	struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
+	__maybe_unused u32 inst, ddr_ram_size, ecc_res;
 	__maybe_unused struct k3_ddrss_data *ddrss_data = (struct k3_ddrss_data *)dev_get_driver_data(dev);
 	__maybe_unused struct k3_ddrss_ecc_region *range = &ddrss->ecc_range;
+	__maybe_unused struct k3_msmc *msmc_parent = NULL;
 
 	debug("%s(dev=%p)\n", __func__, dev);
 
@@ -863,14 +865,51 @@ static int k3_ddrss_probe(struct udevice *dev)
 			ddrss->ecc_range.range = range->range;
 		}
 
+#if !CONFIG_IS_ENABLED(K3_MULTI_DDR)
 		end = ddrss->ecc_range.start + ddrss->ecc_range.range;
+		inst = ddrss->instance;
+		ddr_ram_size = ddrss->ddr_ram_size;
+		ecc_res = ddrss->ecc_reserved_space;
 
-		if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
-			ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
+		if (end > (ddr_ram_size - ecc_res))
+			ddrss->ecc_regions[0].range = ddr_ram_size - ecc_res;
 		else
 			ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
 
 		ddrss->ecc_regions[0].start = ddrss->ecc_range.start - ddrss->ddr_bank_base[0];
+#else
+
+		/* In case multi-DDR, we rely on MSMC's calculation of regions for each DDR */
+		msmc_parent = kzalloc(sizeof(msmc_parent), GFP_KERNEL);
+		if (!msmc_parent) {
+			debug("%s: failed to allocate msmc_parent\n", __func__);
+			return -ENOMEM;
+		}
+		msmc_parent = dev_get_priv(dev->parent);
+		if (!msmc_parent) {
+			printf("%s: could not get MSMC parent to set up inline ECC regions\n",
+			       __func__);
+			kfree(msmc_parent);
+			return -EINVAL;
+		}
+
+		if (msmc_parent->R0[0].start < 0) {
+			/* Configure entire DDR space by default */
+			ddrss->ecc_regions[0].start = ddrss->ddr_bank_base[0];
+			ddrss->ecc_regions[0].range = ddr_ram_size - ecc_res;
+		} else {
+			end = msmc_parent->R0[inst].start + msmc_parent->R0[inst].range;
+
+			if (end > (ddr_ram_size - ecc_res))
+				ddrss->ecc_regions[0].range = ddr_ram_size - ecc_res;
+			else
+				ddrss->ecc_regions[0].range = msmc_parent->R0[inst].range;
+
+			ddrss->ecc_regions[0].start =  msmc_parent->R0[inst].start;
+		}
+
+		kfree(msmc_parent);
+#endif
 
 		k3_ddrss_lpddr4_ecc_init(ddrss);
 	}
-- 
2.34.1

Re: [PATCH 1/8] k3-ddr.c: Remove unwanted header files

[Wadim Egorov]

Hi Neha,

Am 27.01.25 um 21:22 schrieb Neha Malcom Francis:
> This header file is not in use in these arch/board specific files,
> remove them.
> 
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>   arch/arm/mach-k3/k3-ddr.c | 1 -
>   board/ti/common/k3-ddr.c  | 1 -
>   2 files changed, 2 deletions(-)
> 
> diff --git a/arch/arm/mach-k3/k3-ddr.c b/arch/arm/mach-k3/k3-ddr.c
> index 6e3e60cdc86..026381490da 100644
> --- a/arch/arm/mach-k3/k3-ddr.c
> +++ b/arch/arm/mach-k3/k3-ddr.c
> @@ -5,7 +5,6 @@
>   
>   #include <fdt_support.h>
>   #include <dm/uclass.h>
> -#include <k3-ddrss.h>

k3_ddrss_ddr_fdt_fixup() is still used here and the compiler gives us a 
implicit declaration warning.

Regards,
Wadim

Re: [PATCH 4/8] ram: k3-ddrss: Add support for a partial inline ECC region

[Wadim Egorov]

Am 27.01.25 um 21:22 schrieb Neha Malcom Francis:
> Instead of defaulting to choosing the entire DDR region when enabling
> inline ECC, allow picking of a range within the DDR space using DT to
> enable.
> 
> It expects such a node within the memory node, in the absence of which
> we resort to enabling inline ECC for the entire DDR region:
> 
> inline_ecc: protected@9e780000 {
>          device_type = "ecc";
>          reg = <0x9e780000 0x0080000>;
>          bootph-all;
> };
> 
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>

I'm not sure what went wrong, but I had difficulties applying this 
patch. I had to use git am --reject and manually resolve a few blocks.


> ---
>   drivers/ram/k3-ddrss/k3-ddrss.c | 52 +++++++++++++++++++++++++++++++--
>   1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> index ab46098adbf..ea2578cda58 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -149,6 +149,7 @@ struct k3_ddrss_desc {
>   	lpddr4_obj *driverdt;
>   	lpddr4_config config;
>   	lpddr4_privatedata pd;
> +	struct k3_ddrss_ecc_region ecc_range;
>   	struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>   	u64 ecc_reserved_space;
>   	u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> @@ -711,6 +712,30 @@ static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>   		ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>   }
>   
> +static void k3_ddrss_ddr_inline_ecc_base_size_calc(struct k3_ddrss_ecc_region *range)
> +{
> +	fdt_addr_t base;
> +	fdt_size_t size;
> +	ofnode node1;
> +
> +	node1 = ofnode_null();
> +
> +	do {
> +		node1 = ofnode_by_prop_value(node1, "device_type", "ecc", 4);
> +	} while (!ofnode_is_enabled(node1));
> +
> +	base = ofnode_get_addr_size(node1, "reg", &size);
> +
> +	if (base == FDT_ADDR_T_NONE) {
> +		debug("%s: Failed to get ECC node reg and size\n", __func__);
> +		range->start = 0;
> +		range->range = 0;
> +	} else {
> +		range->start = base;
> +		range->range = size;
> +	}
> +}
> +
>   static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>   {
>   	fdtdec_setup_mem_size_base_lowest();
> @@ -759,8 +784,11 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>   
>   static int k3_ddrss_probe(struct udevice *dev)
>   {
> +	u64 end;
>   	int ret;
>   	struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> +	__maybe_unused struct k3_ddrss_data *ddrss_data = (struct k3_ddrss_data *)dev_get_driver_data(dev);
> +	__maybe_unused struct k3_ddrss_ecc_region *range = &ddrss->ecc_range;
>   
>   	debug("%s(dev=%p)\n", __func__, dev);
>   
> @@ -802,9 +830,27 @@ 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 = ddrss->ddr_bank_base[0] - ddrss->ddr_bank_base[0];
> -		ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
> +		k3_ddrss_ddr_inline_ecc_base_size_calc(range);
> +		if (!range->range) {
> +			/* Configure entire DDR space by default */
> +			debug("%s: Defaulting to protecting entire DDR space using inline ECC\n",
> +			      __func__);
> +			ddrss->ecc_range.start = ddrss->ddr_bank_base[0];
> +			ddrss->ecc_range.range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
> +		} else {
> +			ddrss->ecc_range.start = range->start;
> +			ddrss->ecc_range.range = range->range;
> +		}
> +
> +		end = ddrss->ecc_range.start + ddrss->ecc_range.range;
> +
> +		if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
> +			ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
> +		else
> +			ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
> +
> +		ddrss->ecc_regions[0].start = ddrss->ecc_range.start - ddrss->ddr_bank_base[0];
> +
>   		k3_ddrss_lpddr4_ecc_init(ddrss);
>   	}
>   

Re: [PATCH 1/8] k3-ddr.c: Remove unwanted header files

[Neha Malcom Francis]

Hi Wadim

On 28-Jan-25 10:32 AM, Wadim Egorov wrote:
> Hi Neha,
> 
> Am 27.01.25 um 21:22 schrieb Neha Malcom Francis:
>> This header file is not in use in these arch/board specific files,
>> remove them.
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>>   arch/arm/mach-k3/k3-ddr.c | 1 -
>>   board/ti/common/k3-ddr.c  | 1 -
>>   2 files changed, 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-k3/k3-ddr.c b/arch/arm/mach-k3/k3-ddr.c
>> index 6e3e60cdc86..026381490da 100644
>> --- a/arch/arm/mach-k3/k3-ddr.c
>> +++ b/arch/arm/mach-k3/k3-ddr.c
>> @@ -5,7 +5,6 @@
>>   #include <fdt_support.h>
>>   #include <dm/uclass.h>
>> -#include <k3-ddrss.h>
> 
> k3_ddrss_ddr_fdt_fixup() is still used here and the compiler gives us a 
> implicit declaration warning.
> 

Let me take a look at that, must've missed it out; this cleanup was 
intended for the test patch that I will send out later.

> Regards,
> Wadim
> 

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 4/8] ram: k3-ddrss: Add support for a partial inline ECC region

[Neha Malcom Francis]

Hi Wadim

On 28-Jan-25 10:43 AM, Wadim Egorov wrote:
> Am 27.01.25 um 21:22 schrieb Neha Malcom Francis:
>> Instead of defaulting to choosing the entire DDR region when enabling
>> inline ECC, allow picking of a range within the DDR space using DT to
>> enable.
>>
>> It expects such a node within the memory node, in the absence of which
>> we resort to enabling inline ECC for the entire DDR region:
>>
>> inline_ecc: protected@9e780000 {
>>          device_type = "ecc";
>>          reg = <0x9e780000 0x0080000>;
>>          bootph-all;
>> };
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> 
> I'm not sure what went wrong, but I had difficulties applying this 
> patch. I had to use git am --reject and manually resolve a few blocks.
> 

This shouldn't have been the case, sorry about that; probably I didn't 
base it on the latest tip; will rebase and send again.
> 
>> ---
>>   drivers/ram/k3-ddrss/k3-ddrss.c | 52 +++++++++++++++++++++++++++++++--
>>   1 file changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c 
>> b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index ab46098adbf..ea2578cda58 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -149,6 +149,7 @@ struct k3_ddrss_desc {
>>       lpddr4_obj *driverdt;
>>       lpddr4_config config;
>>       lpddr4_privatedata pd;
>> +    struct k3_ddrss_ecc_region ecc_range;
>>       struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>>       u64 ecc_reserved_space;
>>       u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> @@ -711,6 +712,30 @@ static void 
>> k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>>           ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>>   }
>> +static void k3_ddrss_ddr_inline_ecc_base_size_calc(struct 
>> k3_ddrss_ecc_region *range)
>> +{
>> +    fdt_addr_t base;
>> +    fdt_size_t size;
>> +    ofnode node1;
>> +
>> +    node1 = ofnode_null();
>> +
>> +    do {
>> +        node1 = ofnode_by_prop_value(node1, "device_type", "ecc", 4);
>> +    } while (!ofnode_is_enabled(node1));
>> +
>> +    base = ofnode_get_addr_size(node1, "reg", &size);
>> +
>> +    if (base == FDT_ADDR_T_NONE) {
>> +        debug("%s: Failed to get ECC node reg and size\n", __func__);
>> +        range->start = 0;
>> +        range->range = 0;
>> +    } else {
>> +        range->start = base;
>> +        range->range = size;
>> +    }
>> +}
>> +
>>   static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct 
>> k3_ddrss_desc *ddrss)
>>   {
>>       fdtdec_setup_mem_size_base_lowest();
>> @@ -759,8 +784,11 @@ static void k3_ddrss_lpddr4_ecc_init(struct 
>> k3_ddrss_desc *ddrss)
>>   static int k3_ddrss_probe(struct udevice *dev)
>>   {
>> +    u64 end;
>>       int ret;
>>       struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>> +    __maybe_unused struct k3_ddrss_data *ddrss_data = (struct 
>> k3_ddrss_data *)dev_get_driver_data(dev);
>> +    __maybe_unused struct k3_ddrss_ecc_region *range = 
>> &ddrss->ecc_range;
>>       debug("%s(dev=%p)\n", __func__, dev);
>> @@ -802,9 +830,27 @@ 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 = ddrss->ddr_bank_base[0] - 
>> ddrss->ddr_bank_base[0];
>> -        ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        k3_ddrss_ddr_inline_ecc_base_size_calc(range);
>> +        if (!range->range) {
>> +            /* Configure entire DDR space by default */
>> +            debug("%s: Defaulting to protecting entire DDR space 
>> using inline ECC\n",
>> +                  __func__);
>> +            ddrss->ecc_range.start = ddrss->ddr_bank_base[0];
>> +            ddrss->ecc_range.range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        } else {
>> +            ddrss->ecc_range.start = range->start;
>> +            ddrss->ecc_range.range = range->range;
>> +        }
>> +
>> +        end = ddrss->ecc_range.start + ddrss->ecc_range.range;
>> +
>> +        if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
>> +            ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        else
>> +            ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
>> +
>> +        ddrss->ecc_regions[0].start = ddrss->ecc_range.start - 
>> ddrss->ddr_bank_base[0];
>> +
>>           k3_ddrss_lpddr4_ecc_init(ddrss);
>>       }
> 

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 2/8] ram: k3-ddrss: Use DDR address instead of system address for ecc_regions

[Kumar, Udit]

On 1/27/2025 7:52 PM, Neha Malcom Francis wrote:
> Let ecc_regions[x].start reflect the start of the ECC region in terms of
> DDR addressing rather than system addressing. This will make it easier
> to extend the usage of the same ecc_regions structure for multi-DDR
> systems as well.
>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>   drivers/ram/k3-ddrss/k3-ddrss.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> index 05ea61bbfe9..24932cd837c 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -730,7 +730,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>   	u32 val;
>   
>   	/* Only Program region 0 which covers full ddr space */
> -	k3_ddrss_set_ecc_range_r0(base, ecc_region_start - ddrss->ddr_bank_base[0], ecc_range);
> +	k3_ddrss_set_ecc_range_r0(base, ecc_region_start, ecc_range);
>   
>   	/* Enable ECC, RMW, WR_ALLOC */
>   	writel(DDRSS_ECC_CTRL_REG_ECC_EN | DDRSS_ECC_CTRL_REG_RMW_EN |
> @@ -799,7 +799,7 @@ 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 = ddrss->ddr_bank_base[0];
> +		ddrss->ecc_regions[0].start = ddrss->ddr_bank_base[0] - ddrss->ddr_bank_base[0];

you will get start as zero, here , if this is what you need just set to 
zero


>   		ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
>   		k3_ddrss_lpddr4_ecc_init(ddrss);
>   	}

Re: [PATCH 3/8] ram: k3-ddrss: Add comment about ecc_reserved_space

[Kumar, Udit]

On 1/27/2025 7:52 PM, Neha Malcom Francis wrote:
> The reserved space needed for storing the parity remains the same no
> matter the size of the region that is being protected. Add this as a
> comment for better code understanding.
>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>   drivers/ram/k3-ddrss/k3-ddrss.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> index 24932cd837c..ab46098adbf 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -715,6 +715,10 @@ static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>   {
>   	fdtdec_setup_mem_size_base_lowest();
>   
> +	/*
> +	 * Reserved region remains 1/9th of the total DDR available no matter the
> +	 * size of the region under protection
> +	 */

Please add why , you need 1/9th of DDR


>   	ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
>   	do_div(ddrss->ecc_reserved_space, 9);
>   

Re: [PATCH 4/8] ram: k3-ddrss: Add support for a partial inline ECC region

[Kumar, Udit]

On 1/27/2025 7:52 PM, Neha Malcom Francis wrote:
> Instead of defaulting to choosing the entire DDR region when enabling
> inline ECC, allow picking of a range within the DDR space using DT to
> enable.
>
> It expects such a node within the memory node, in the absence of which
> we resort to enabling inline ECC for the entire DDR region:
>
> inline_ecc: protected@9e780000 {
>          device_type = "ecc";
>          reg = <0x9e780000 0x0080000>;
>          bootph-all;
> };

is this possible to have ECC on different part of memory ?

i mean, protect 0x9e780000 + 0x0080000

and then 0xa000_0000 + 0x0080000

>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>   drivers/ram/k3-ddrss/k3-ddrss.c | 52 +++++++++++++++++++++++++++++++--
>   1 file changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
> index ab46098adbf..ea2578cda58 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -149,6 +149,7 @@ struct k3_ddrss_desc {
>   	lpddr4_obj *driverdt;
>   	lpddr4_config config;
>   	lpddr4_privatedata pd;
> +	struct k3_ddrss_ecc_region ecc_range;
>   	struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>   	u64 ecc_reserved_space;
>   	u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> @@ -711,6 +712,30 @@ static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>   		ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>   }
>   
> +static void k3_ddrss_ddr_inline_ecc_base_size_calc(struct k3_ddrss_ecc_region *range)
> +{
> +	fdt_addr_t base;
> +	fdt_size_t size;
> +	ofnode node1;
> +
> +	node1 = ofnode_null();
> +
> +	do {
> +		node1 = ofnode_by_prop_value(node1, "device_type", "ecc", 4);
> +	} while (!ofnode_is_enabled(node1));
> +
> +	base = ofnode_get_addr_size(node1, "reg", &size);
> +
> +	if (base == FDT_ADDR_T_NONE) {
> +		debug("%s: Failed to get ECC node reg and size\n", __func__);
> +		range->start = 0;
> +		range->range = 0;
> +	} else {
> +		range->start = base;
> +		range->range = size;
> +	}
> +}
> +
>   static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc *ddrss)
>   {
>   	fdtdec_setup_mem_size_base_lowest();
> @@ -759,8 +784,11 @@ static void k3_ddrss_lpddr4_ecc_init(struct k3_ddrss_desc *ddrss)
>   
>   static int k3_ddrss_probe(struct udevice *dev)
>   {
> +	u64 end;
>   	int ret;
>   	struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> +	__maybe_unused struct k3_ddrss_data *ddrss_data = (struct k3_ddrss_data *)dev_get_driver_data(dev);
> +	__maybe_unused struct k3_ddrss_ecc_region *range = &ddrss->ecc_range;
>   
>   	debug("%s(dev=%p)\n", __func__, dev);
>   
> @@ -802,9 +830,27 @@ 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 = ddrss->ddr_bank_base[0] - ddrss->ddr_bank_base[0];
> -		ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
> +		k3_ddrss_ddr_inline_ecc_base_size_calc(range);
> +		if (!range->range) {
> +			/* Configure entire DDR space by default */
> +			debug("%s: Defaulting to protecting entire DDR space using inline ECC\n",
> +			      __func__);
> +			ddrss->ecc_range.start = ddrss->ddr_bank_base[0];
> +			ddrss->ecc_range.range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
> +		} else {
> +			ddrss->ecc_range.start = range->start;
> +			ddrss->ecc_range.range = range->range;
> +		}
Do you really need ecc_range variable ? if not please thing of using 
ddrss->ecc_regions[0]
> +
> +		end = ddrss->ecc_range.start + ddrss->ecc_range.range;
> +
> +		if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
> +			ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - ddrss->ecc_reserved_space;
> +		else
> +			ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
> +
> +		ddrss->ecc_regions[0].start = ddrss->ecc_range.start - ddrss->ddr_bank_base[0];
> +
>   		k3_ddrss_lpddr4_ecc_init(ddrss);
>   	}
>   

Re: [PATCH 4/8] ram: k3-ddrss: Add support for a partial inline ECC region

[Neha Malcom Francis]

Hi Udit

On 28-Jan-25 11:23 AM, Kumar, Udit wrote:
> 
> On 1/27/2025 7:52 PM, Neha Malcom Francis wrote:
>> Instead of defaulting to choosing the entire DDR region when enabling
>> inline ECC, allow picking of a range within the DDR space using DT to
>> enable.
>>
>> It expects such a node within the memory node, in the absence of which
>> we resort to enabling inline ECC for the entire DDR region:
>>
>> inline_ecc: protected@9e780000 {
>>          device_type = "ecc";
>>          reg = <0x9e780000 0x0080000>;
>>          bootph-all;
>> };
> 
> is this possible to have ECC on different part of memory ?
> 
> i mean, protect 0x9e780000 + 0x0080000
> 
> and then 0xa000_0000 + 0x0080000

This hasn't been tested, and it won't work with this series presently. I 
can take it up later once the base functionality is taken in.

> 
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>>   drivers/ram/k3-ddrss/k3-ddrss.c | 52 +++++++++++++++++++++++++++++++--
>>   1 file changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c 
>> b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index ab46098adbf..ea2578cda58 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -149,6 +149,7 @@ struct k3_ddrss_desc {
>>       lpddr4_obj *driverdt;
>>       lpddr4_config config;
>>       lpddr4_privatedata pd;
>> +    struct k3_ddrss_ecc_region ecc_range;
>>       struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>>       u64 ecc_reserved_space;
>>       u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> @@ -711,6 +712,30 @@ static void 
>> k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>>           ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>>   }
>> +static void k3_ddrss_ddr_inline_ecc_base_size_calc(struct 
>> k3_ddrss_ecc_region *range)
>> +{
>> +    fdt_addr_t base;
>> +    fdt_size_t size;
>> +    ofnode node1;
>> +
>> +    node1 = ofnode_null();
>> +
>> +    do {
>> +        node1 = ofnode_by_prop_value(node1, "device_type", "ecc", 4);
>> +    } while (!ofnode_is_enabled(node1));
>> +
>> +    base = ofnode_get_addr_size(node1, "reg", &size);
>> +
>> +    if (base == FDT_ADDR_T_NONE) {
>> +        debug("%s: Failed to get ECC node reg and size\n", __func__);
>> +        range->start = 0;
>> +        range->range = 0;
>> +    } else {
>> +        range->start = base;
>> +        range->range = size;
>> +    }
>> +}
>> +
>>   static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct 
>> k3_ddrss_desc *ddrss)
>>   {
>>       fdtdec_setup_mem_size_base_lowest();
>> @@ -759,8 +784,11 @@ static void k3_ddrss_lpddr4_ecc_init(struct 
>> k3_ddrss_desc *ddrss)
>>   static int k3_ddrss_probe(struct udevice *dev)
>>   {
>> +    u64 end;
>>       int ret;
>>       struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>> +    __maybe_unused struct k3_ddrss_data *ddrss_data = (struct 
>> k3_ddrss_data *)dev_get_driver_data(dev);
>> +    __maybe_unused struct k3_ddrss_ecc_region *range = 
>> &ddrss->ecc_range;
>>       debug("%s(dev=%p)\n", __func__, dev);
>> @@ -802,9 +830,27 @@ 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 = ddrss->ddr_bank_base[0] - 
>> ddrss->ddr_bank_base[0];
>> -        ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        k3_ddrss_ddr_inline_ecc_base_size_calc(range);
>> +        if (!range->range) {
>> +            /* Configure entire DDR space by default */
>> +            debug("%s: Defaulting to protecting entire DDR space 
>> using inline ECC\n",
>> +                  __func__);
>> +            ddrss->ecc_range.start = ddrss->ddr_bank_base[0];
>> +            ddrss->ecc_range.range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        } else {
>> +            ddrss->ecc_range.start = range->start;
>> +            ddrss->ecc_range.range = range->range;
>> +        }
> Do you really need ecc_range variable ? if not please thing of using 
> ddrss->ecc_regions[0]
>> +
>> +        end = ddrss->ecc_range.start + ddrss->ecc_range.range;
>> +
>> +        if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
>> +            ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        else
>> +            ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
>> +
>> +        ddrss->ecc_regions[0].start = ddrss->ecc_range.start - 
>> ddrss->ddr_bank_base[0];
>> +
>>           k3_ddrss_lpddr4_ecc_init(ddrss);
>>       }

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 4/8] ram: k3-ddrss: Add support for a partial inline ECC region

[Neha Malcom Francis]

On 28-Jan-25 11:23 AM, Kumar, Udit wrote:
> 
> On 1/27/2025 7:52 PM, Neha Malcom Francis wrote:
>> Instead of defaulting to choosing the entire DDR region when enabling
>> inline ECC, allow picking of a range within the DDR space using DT to
>> enable.
>>
>> It expects such a node within the memory node, in the absence of which
>> we resort to enabling inline ECC for the entire DDR region:
>>
>> inline_ecc: protected@9e780000 {
>>          device_type = "ecc";
>>          reg = <0x9e780000 0x0080000>;
>>          bootph-all;
>> };
> 
> is this possible to have ECC on different part of memory ?
> 
> i mean, protect 0x9e780000 + 0x0080000
> 
> and then 0xa000_0000 + 0x0080000
> 
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>>   drivers/ram/k3-ddrss/k3-ddrss.c | 52 +++++++++++++++++++++++++++++++--
>>   1 file changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c 
>> b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index ab46098adbf..ea2578cda58 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -149,6 +149,7 @@ struct k3_ddrss_desc {
>>       lpddr4_obj *driverdt;
>>       lpddr4_config config;
>>       lpddr4_privatedata pd;
>> +    struct k3_ddrss_ecc_region ecc_range;
>>       struct k3_ddrss_ecc_region ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
>>       u64 ecc_reserved_space;
>>       u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> @@ -711,6 +712,30 @@ static void 
>> k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
>>           ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
>>   }
>> +static void k3_ddrss_ddr_inline_ecc_base_size_calc(struct 
>> k3_ddrss_ecc_region *range)
>> +{
>> +    fdt_addr_t base;
>> +    fdt_size_t size;
>> +    ofnode node1;
>> +
>> +    node1 = ofnode_null();
>> +
>> +    do {
>> +        node1 = ofnode_by_prop_value(node1, "device_type", "ecc", 4);
>> +    } while (!ofnode_is_enabled(node1));
>> +
>> +    base = ofnode_get_addr_size(node1, "reg", &size);
>> +
>> +    if (base == FDT_ADDR_T_NONE) {
>> +        debug("%s: Failed to get ECC node reg and size\n", __func__);
>> +        range->start = 0;
>> +        range->range = 0;
>> +    } else {
>> +        range->start = base;
>> +        range->range = size;
>> +    }
>> +}
>> +
>>   static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct 
>> k3_ddrss_desc *ddrss)
>>   {
>>       fdtdec_setup_mem_size_base_lowest();
>> @@ -759,8 +784,11 @@ static void k3_ddrss_lpddr4_ecc_init(struct 
>> k3_ddrss_desc *ddrss)
>>   static int k3_ddrss_probe(struct udevice *dev)
>>   {
>> +    u64 end;
>>       int ret;
>>       struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
>> +    __maybe_unused struct k3_ddrss_data *ddrss_data = (struct 
>> k3_ddrss_data *)dev_get_driver_data(dev);
>> +    __maybe_unused struct k3_ddrss_ecc_region *range = 
>> &ddrss->ecc_range;
>>       debug("%s(dev=%p)\n", __func__, dev);
>> @@ -802,9 +830,27 @@ 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 = ddrss->ddr_bank_base[0] - 
>> ddrss->ddr_bank_base[0];
>> -        ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        k3_ddrss_ddr_inline_ecc_base_size_calc(range);
>> +        if (!range->range) {
>> +            /* Configure entire DDR space by default */
>> +            debug("%s: Defaulting to protecting entire DDR space 
>> using inline ECC\n",
>> +                  __func__);
>> +            ddrss->ecc_range.start = ddrss->ddr_bank_base[0];
>> +            ddrss->ecc_range.range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        } else {
>> +            ddrss->ecc_range.start = range->start;
>> +            ddrss->ecc_range.range = range->range;
>> +        }
> Do you really need ecc_range variable ? if not please thing of using 
> ddrss->ecc_regions[0]

Using it as ddrss->ecc_regions[0] led to excess character count in the line.

>> +
>> +        end = ddrss->ecc_range.start + ddrss->ecc_range.range;
>> +
>> +        if (end > (ddrss->ddr_ram_size - ddrss->ecc_reserved_space))
>> +            ddrss->ecc_regions[0].range = ddrss->ddr_ram_size - 
>> ddrss->ecc_reserved_space;
>> +        else
>> +            ddrss->ecc_regions[0].range = ddrss->ecc_range.range;
>> +
>> +        ddrss->ecc_regions[0].start = ddrss->ecc_range.start - 
>> ddrss->ddr_bank_base[0];
>> +
>>           k3_ddrss_lpddr4_ecc_init(ddrss);
>>       }

-- 
Thanking You
Neha Malcom Francis