Hi Udit,
On 07/08/25 10:16, Kumar, Udit wrote:
>
> On 7/30/2025 7:07 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;
>> };
>
> Please mention , where you expect this node to available in all
> controller nodes or just one controller (in case of multiple controllers)
>
This is not part of the memorycontroller node; it is a subnode of the
memory/ node.
>
>>
>> Signed-off-by: Neha Malcom Francis <n-fran...@ti.com>
>> ---
>> drivers/ram/k3-ddrss/k3-ddrss.c | 55 +++++++++++++++++++++++++++++++--
>> 1 file changed, 52 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c
>> b/drivers/ram/k3-ddrss/k3-ddrss.c
>> index a94755c3542..cd117d7eba7 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -155,6 +155,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_REG];
>> u64 ecc_reserved_space;
>> u64 ddr_bank_base[CONFIG_NR_DRAM_BANKS];
>> @@ -733,6 +734,30 @@ static void k3_ddrss_ddr_reg_init(struct
>> k3_ddrss_desc *ddrss)
>> writel(DDRSS_ECC_CTRL_REG_DEFAULT, ddrss->ddrss_ss_cfg +
>> DDRSS_ECC_CTRL_REG);
>> }
>> +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));
>> +
>
> Depending where you expect node to available, parsing logic may change a
> little
As mentioned above, it will always be in a single place i.e. memory/
node. The distribution to the different memory controllers is handled by
the driver.
>
>
>> + 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();
>> @@ -782,8 +807,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 u32 ddr_ram_size, ecc_res;
>
> ddr_ram_size may go above u32
>
>> + __maybe_unused struct k3_ddrss_ecc_region *range =
>> &ddrss->ecc_range;
>> debug("%s(dev=%p)\n", __func__, dev);
>> @@ -821,9 +849,30 @@ 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 = 0;
>> - ddrss->ecc_regions[0].range = ddrss->ddr_ram_size -
>> ddrss->ecc_reserved_space;
>
> patch 1/8 is remove here, please check if you really need 1/8 ?
>
1/8 still changes what ddrss->ecc_regions[0].start implies. So the call
to k3_ddrss_set_ecc_range_r0 is also changed there.
>
>> + k3_ddrss_ddr_inline_ecc_base_size_calc(range);
>> +
>> + end = ddrss->ecc_range.start + ddrss->ecc_range.range;
>> + ddr_ram_size = ddrss->ddr_ram_size;
>> + ecc_res = ddrss->ecc_reserved_space;
>> +
>> + 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 = ddr_ram_size - ecc_res;
>> + } else {
>> + ddrss->ecc_range.start = range->start;
>> + ddrss->ecc_range.range = range->range;
>> + }
>> +
>
> Please add comment here for below code .
>
> I think you are trying to fix, if end goes above one ddr
Yes will add comments here, thanks for the review!
>
>> + 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];
>> +
>> k3_ddrss_lpddr4_ecc_init(ddrss);
>> }
>>
--
Thanking You
Neha Malcom Francis
