[PATCH v2 1/9] ram: k3-ddrss: Use DDR address instead of system address for ecc_regions

[Neha Malcom Francis]

Hi Udit,

On 07/08/25 09:51, Kumar, Udit wrote:
> Hello Neha
> 
> On 7/30/2025 7:07 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.
> 
> comments are not inline with patch contents ,
> 
> LTM, you are doing clean up how ecc_regions[0].start is being used.

Yes that is what we are doing.

> 
> Originally ddrss->ecc_regions[0].start = ddrss->ddr_bank_base[0];
> 
> and while using it  we did (ecc_region_start - ddrss->ddr_bank_base[0])
> 
> other then that I don't see we are using ecc_region_start anywhere.

We will use it later on which is why the message says we are changing
what this variable means so that we can use it in later for multi-DDR
systems. Sorry I didn't understand what exactly needs to change in the
commit message here?

Maybe s/Let/Change will make it a little more easier to understand?

"Change ecc_regions[x].start to reflect the start of the ECC region in
terms of DDR addressing rather than system addressing..."

> 
> With update in commit message , Please use
> 
> 
> Reviewed-by: Udit Kumar <u-kumar1 at ti.com>
> 
> 
> 
>> Signed-off-by: Neha Malcom Francis <n-francis at 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 6590d57ad84..7389fd8b82f 100644
>> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
>> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
>> @@ -752,7 +752,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 |
>> @@ -817,7 +817,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 = 0;
>>           ddrss->ecc_regions[0].range = ddrss->ddr_ram_size -
>> ddrss->ecc_reserved_space;
>>           k3_ddrss_lpddr4_ecc_init(ddrss);
>>       }

-- 
Thanking You
Neha Malcom Francis