[PATCH v2 2/3] board: phytec: phycore_am64x: Add support for 1 GB RAM variant and ECC

[Wadim Egorov]

Hi Dhruva,

Am 05.02.25 um 12:55 schrieb Dhruva Gole:
> On Feb 05, 2025 at 05:30:12 +0100, Wadim Egorov wrote:
>> Detect RAM size via EEPROM and adjust DDR size and banks accordingly.
>> Include necessary fixups to handle ECC-enabled configurations.
>>
>> Signed-off-by: Wadim Egorov <w.egorov at phytec.de>
>> Tested-by: Daniel Schultz <d.schultz at phytec.de>
>> ---
<snip>
>> @@ -11,9 +11,12 @@
>>   #include <env.h>
>>   #include <env_internal.h>
>>   #include <spl.h>
>> +#include <asm/arch/k3-ddr.h>
> 
> If adding new header might as well put it below asm/arch/hardware.h to
> maintain some level of sort?
> 
>>   #include <fdt_support.h>
>>   #include <asm/arch/hardware.h>
>>   
>> +#include "../common/am6_som_detection.h"
>> +
> 
> I am already seeing 3 different way am6_som_detection.h is being
> included. Please can we consider using <> to abstract these relative
> paths?
> It looks much cleaner that way and easier to maintain IMHO.
> 

I'll keep this in mind for a future cleanup series.


>>   DECLARE_GLOBAL_DATA_PTR;
>>   
>>   int board_init(void)
>> @@ -21,15 +24,113 @@ int board_init(void)
>>   	return 0;
>>   }
>>   
>> +static u8 phytec_get_am64_ddr_size_default(void)
>> +{
>> +	int ret;
>> +	struct phytec_eeprom_data data;
>> +
>> +	if (IS_ENABLED(CONFIG_PHYCORE_AM64X_RAM_SIZE_FIX)) {
>> +		if (IS_ENABLED(CONFIG_PHYCORE_AM64X_RAM_SIZE_1GB))
>> +			return EEPROM_RAM_SIZE_1GB;
>> +		else if (IS_ENABLED(CONFIG_PHYCORE_AM64X_RAM_SIZE_2GB))
>> +			return EEPROM_RAM_SIZE_2GB;
>> +	}
>> +
>> +	ret = phytec_eeprom_data_setup(&data, 0, EEPROM_ADDR);
>> +	if (!ret && data.valid)
>> +		return phytec_get_am6_ddr_size(&data);
>> +
>> +	/* Default DDR size is 2GB */
>> +	return EEPROM_RAM_SIZE_2GB;
>> +}
>> +
>>   int dram_init(void)
>>   {
>> -	return fdtdec_setup_mem_size_base();
>> +	u8 ram_size;
>> +
>> +	if (!IS_ENABLED(CONFIG_CPU_V7R))
>> +		return fdtdec_setup_mem_size_base();
>> +
>> +	ram_size = phytec_get_am64_ddr_size_default();
>> +
>> +	switch (ram_size) {
>> +	case EEPROM_RAM_SIZE_1GB:
>> +		gd->ram_size = 0x40000000;
>> +		break;
>> +	case EEPROM_RAM_SIZE_2GB:
>> +		gd->ram_size = 0x80000000;
>> +		break;
>> +	default:
>> +		gd->ram_size = 0x80000000;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   int dram_init_banksize(void)
>>   {
>> -	return fdtdec_setup_memory_banksize();
>> +	u8 ram_size;
>> +
>> +	memset(gd->bd->bi_dram, 0, sizeof(gd->bd->bi_dram[0]) * CONFIG_NR_DRAM_BANKS);
>> +
>> +	if (!IS_ENABLED(CONFIG_CPU_V7R))
>> +		return fdtdec_setup_memory_banksize();
>> +
>> +	ram_size = phytec_get_am64_ddr_size_default();
>> +	switch (ram_size) {
>> +	case EEPROM_RAM_SIZE_1GB:
>> +		gd->bd->bi_dram[0].start = CFG_SYS_SDRAM_BASE;
>> +		gd->bd->bi_dram[0].size = 0x40000000;
>> +		gd->ram_size = 0x40000000;
>> +		break;
>> +
>> +	case EEPROM_RAM_SIZE_2GB:
>> +		gd->bd->bi_dram[0].start = CFG_SYS_SDRAM_BASE;
>> +		gd->bd->bi_dram[0].size = 0x80000000;
>> +		gd->ram_size = 0x80000000;
>> +		break;
>> +
>> +	default:
>> +		/* Continue with default 2GB setup */
>> +		gd->bd->bi_dram[0].start = CFG_SYS_SDRAM_BASE;
>> +		gd->bd->bi_dram[0].size = 0x80000000;
>> +		gd->ram_size = 0x80000000;
>> +		printf("DDR size %d is not supported\n", ram_size);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#if defined(CONFIG_K3_DDRSS)
>> +int do_board_detect(void)
>> +{
>> +	void *fdt = (void *)gd->fdt_blob;
>> +	int bank;
>> +
>> +	u64 start[CONFIG_NR_DRAM_BANKS];
>> +	u64 size[CONFIG_NR_DRAM_BANKS];
>> +
>> +	dram_init();
>> +	dram_init_banksize();
> 
> No check for any failures from fdtdec_setup_memory_banksize ?

No need to, because dram_init_banksize() always returns success in this 
case. fdtdec_setup_memory_banksize() gets never called for CONFIG_CPU_V7R.

> 
>> +
>> +	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;
>> +	}
>> +
>> +	return fdt_fixup_memory_banks(fdt, start, size, CONFIG_NR_DRAM_BANKS);
>> +}
>> +#endif
> 
> Nit:
> Try and end #endifs like this:
> #endif /* CONFIG_K3_DDRSS */

Same here, will keep this in mind for a future cleanup series.

Regards,
Wadim