Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_param.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 34: static void set_ddr_size_type(struct dramc_param *param) : { : u32 ramcode = get_rom_code(); : u16 ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : : switch (ramcode) { : case 1: : case 2: : case 3: : case 4: : case 5: : case 6: : case 7: : case 8: : ddr_size_type = DDR_TYPE_2CH_2RK_4GB_2_2; : break; : default: : break; : } : : param->header.ddr_size_type = ddr_size_type; : }
the sdram_config are uesless now,
But your change just makes it important again.
My point is, RAMCODE -> DRAM CONFIG mapping should be a board-specific thing, and definitely not for code inside SOC.
Meanwhile, we're planning to change the mapping table to be model-specific, e.g., the same RAM code may be 4GB for model A and 6 GB for model B in future; so this information must come from some where, and sdram_config should be the right place.
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 64: set_ddr_size_type(param);
please more details.
I'd prefer this being change to
int initialize_dramc_param(void *blob, u32 ddr_geometry, u16 config) { ... hdr->ddr_geometry = ddr_geometry; }
Like said, all the ram code to geometry(config) translation should happen in board specific functions, not in the SOC side functions.