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

[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>
> ---
> v2: Add Tested-by from Daniel
> ---
>  board/phytec/phycore_am64x/Kconfig         |  25 +++++
>  board/phytec/phycore_am64x/phycore-am64x.c | 105 ++++++++++++++++++++-
>  2 files changed, 128 insertions(+), 2 deletions(-)
> 
> diff --git a/board/phytec/phycore_am64x/Kconfig b/board/phytec/phycore_am64x/Kconfig
> index 829526c3295..a709b71ba4d 100644
> --- a/board/phytec/phycore_am64x/Kconfig
> +++ b/board/phytec/phycore_am64x/Kconfig
> @@ -35,3 +35,28 @@ config SYS_CONFIG_NAME
>  source "board/phytec/common/Kconfig"
>  
>  endif
> +
> +config PHYCORE_AM64X_RAM_SIZE_FIX
> +        bool "Set phyCORE-AM64x RAM size fix instead of detecting"
> +        default false
> +        help
> +          RAM size is automatic being detected with the help of

s/automatic/automatically

> +          the EEPROM introspection data. Set RAM size to a fix value
> +          instead.
> +
> +choice
> +        prompt "phyCORE-AM64x RAM size"
> +        depends on PHYCORE_AM64X_RAM_SIZE_FIX
> +        default PHYCORE_AM64X_RAM_SIZE_2GB
> +
> +config PHYCORE_AM64X_RAM_SIZE_1GB
> +        bool "1GB RAM"
> +        help
> +          Set RAM size fix to 1GB for phyCORE-AM64x.
> +
> +config PHYCORE_AM64X_RAM_SIZE_2GB
> +        bool "2GB RAM"
> +        help
> +          Set RAM size fix to 2GB for phyCORE-AM64x.
> +
> +endchoice
> diff --git a/board/phytec/phycore_am64x/phycore-am64x.c b/board/phytec/phycore_am64x/phycore-am64x.c
> index 8f3b22657c7..f14c87f5c72 100644
> --- a/board/phytec/phycore_am64x/phycore-am64x.c
> +++ b/board/phytec/phycore_am64x/phycore-am64x.c
> @@ -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.

>  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 ?

> +
> +	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 */

Follow the convention everywhere else as it really improves code
readability.

> +
> +#if IS_ENABLED(CONFIG_XPL_BUILD)
> +void spl_perform_fixups(struct spl_image_info *spl_image)
> +{
> +	if (IS_ENABLED(CONFIG_K3_DDRSS) && IS_ENABLED(CONFIG_K3_INLINE_ECC))
> +		fixup_ddr_driver_for_ecc(spl_image);
> +	else
> +		fixup_memory_node(spl_image);
>  }
> +#endif
>  
>  #define CTRLMMR_USB0_PHY_CTRL	0x43004008
>  #define CORE_VOLTAGE		0x80000000
> -- 
> 2.34.1
> 

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated