The whole cannonlake_memcfg_init() seems to be not designed very well.
Please improve the api.

You could add an enum for each DIMM:
NOT_EXISTING,
READ_SMBUS,
READ_SPD_CBFS,
READ_SPD_MEMPTR

That way the board config would hold all possible combinations.

View Change

6 comments:

• File src/soc/intel/cannonlake/cnl_memcfg_init.c:

  • Patch Set #8, Line 52: static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg,

    only used once in this file.
    Please squash with meminit_spd_data().

  • Patch Set #8, Line 53: const struct cnl_mb_cfg *board_cfg,

    why is it board_cfg, but below it's cnl_cfg ?

  • Patch Set #8, Line 60: mem_cfg->MemorySpdPtr10 = 0;

    why do you assume channel0 is connected to DRAM ?
    That's not documented.

  • Patch Set #8, Line 103: const struct cnl_mb_cfg *cnl_cfg,

    why isn't struct spd_info part if struct cnl_mb_cfg ?

  • Patch Set #8, Line 112: /* Spd pointer will only be used if all smbus slave address of memory

    why ?

• File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:

  • Patch Set #8, Line 108: * Flag to indicate if the memory configuration is single

    it's only used for soldered DRAM where SPD resides in CBFS, but that's not documented

To view, visit change 31262. To unsubscribe, or for help writing mail filters, visit settings.