[PATCH 0/8] ram: k3-ddrss: Support partial inline ECC
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(-)
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"
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
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
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); }
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);}
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);
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);
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); }
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;elseddrss->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); }
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); }
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;elseddrss->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); }
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); }
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"
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; }
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; }
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); }
participants (3)
-
Kumar, Udit -
Neha Malcom Francis -
Wadim Egorov