Attention is currently required from: Angel Pons, Christian Walter, Dinesh Gehlot, Jayvik Desai, Jeremy Soller, Kapil Porwal, Lean Sheng Tan, Michał Kopeć, Michał Żygowski, Nick Vaccaro, Piotr Król, Sean Rhodes, Tim Crawford.
Subrata Banik has posted comments on this change by Kapil Porwal. ( https://review.coreboot.org/c/coreboot/+/85529?usp=email )
Change subject: soc/intel/alderlake: Add a function to force disable memory channels ......................................................................
Patch Set 1:
(5 comments)
File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/85529/comment/986ceef3_84eff778?usp... : PS1, Line 235: can you please write a help text for easy explanation of mem_init_override_channel_mask and ch_disable_mask (like BIT x = 0 aka enable and BIT x = 1 aka channel disable)
https://review.coreboot.org/c/coreboot/+/85529/comment/9a512d57_3fd64ab9?usp... : PS1, Line 236: static void mem_init_override_channel_mask(FSP_M_CONFIG *mem_cfg, : uint32_t ch_disable_mask) ``` static void mem_init_override_channel_mask(FSP_M_CONFIG *mem_cfg, uint32_t ch_disable_mask) { uint8_t *disable_channel_upds[MRC_CHANNELS] = { &mem_cfg->DisableMc0Ch0, &mem_cfg->DisableMc0Ch1, &mem_cfg->DisableMc0Ch2, &mem_cfg->DisableMc0Ch3, &mem_cfg->DisableMc1Ch0, &mem_cfg->DisableMc1Ch1, &mem_cfg->DisableMc1Ch2, &mem_cfg->DisableMc1Ch3, };
if (ch_disable_mask == 0) return;
for (size_t ch = 0; ch < MRC_CHANNELS; ch++) { if (ch_disable_mask & BIT(ch)) *disable_channel_upds[ch] = 1; } } ```
WDYT?
https://review.coreboot.org/c/coreboot/+/85529/comment/a589ea8f_9b8a0d09?usp... : PS1, Line 237: uint32_t should we take only `uint8` for mask disable ?
https://review.coreboot.org/c/coreboot/+/85529/comment/141d52b3_243b4230?usp... : PS1, Line 252: if (ch_disable_mask == 0) {
Do we have any invalid combination here? Like we must have Mc0Ch0 at least? Or you cann't disable DisableMc0Ch0 and DisableMc0Ch3 etc..
We always need to ensure that the bit mask cannot disable MC0Ch0. I agree that we need to check with the Intel team to determine whether additional conservation is required for other channels of MC0.
https://review.coreboot.org/c/coreboot/+/85529/comment/7f07c103_72f04a37?usp... : PS1, Line 264: void memcfg_init(FSPM_UPD *memupd, const struct mb_cfg *mb_cfg, : const struct mem_spd *spd_info, bool half_populated, : uint32_t ch_disable_mask) wondering if you really need to change the API to accommodate `ch_disable_mask`. this can be done using a KCONFIG and function override for platform that wish to subscribe to the `ch_disable_mask` feature ?
you could implement
inside .h file: ``` #if CONFIG(ENFORCE_MEM_CHANNEL_DISABLE) uint8 mb_fill_override_channel_mask(void); #else uint8 mb_fill_override_channel_mask(void) { return 0;} #endif ``` inside specific mainboard that wish to override the channel_mask should ``` 1. select ENFORCE_MEM_CHANNEL_DISABLE kconfig 2. Implement mb_fill_override_channel_mask() to return the `channel_mask` ```
inside meminit_c file
``` ch_disable_mask = mb_fill_override_channel_mask(); if (ch_disable_mask) mem_init_override_channel_mask(mem_cfg, ch_disable_mask); ```