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:
(3 comments)
I think we want to separate this into two patches.
1. Add DDR size in dramc_config (and sdram_config) 2. Support 4/6/8 GB in MT8183 EMI
In the meantime, I think the right execution path should be:
1. ddr_size_type (or just ddr_size, as 4/6/8 ? will you need channel info or just total size would be good enough?) should be one information in sdram_configs 2. we should load the right sdram_configs using ramcode. 3. kukui board romstage to pass the information when creating dramc_param 4. mt8183/emi takes the param and pass to dramk
BTW, @jwerner did you have DDR size info as input for Trogdor?
https://review.coreboot.org/c/coreboot/+/41949/3/src/mainboard/google/kukui/... File src/mainboard/google/kukui/sdram_configs.c:
https://review.coreboot.org/c/coreboot/+/41949/3/src/mainboard/google/kukui/... PS3, Line 34: u32 get_rom_code(void) : { : return ram_code(); : } I don't see the point of duplicating function here...?
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; : } no, we should not do this. The ddr_size_type should be a data directly from sdram_config.
https://review.coreboot.org/c/coreboot/+/41949/3/src/soc/mediatek/mt8183/dra... PS3, Line 64: set_ddr_size_type(param); Please move this to be a param of initialize_dramc_param.