Attention is currently required from: Elyes Haouas, Felix Singer, Jérémy Compostella, Vasiliy Khoruzhick, yuchi.chen@intel.com.
Shuo Liu has posted comments on this change by yuchi.chen@intel.com. ( https://review.coreboot.org/c/coreboot/+/83321?usp=email )
Change subject: soc/intel/snowridge: Add support for Intel Atom Snow Ridge SoC ......................................................................
Patch Set 15:
(7 comments)
File src/soc/intel/snowridge/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/83321/comment/cb5be18a_8ec8ef32?usp... : PS15, Line 22: #define GPIO_WEST2_PADCFGLOCKTX 0x00c4 Not sure if these cfg registers are the same across community/groups? If yes, if it is possible to only define one set of register offsets and applied for all community/groups by rebase? (what is did by gpio common codes)
File src/soc/intel/snowridge/romstage/gpio_snr.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/3eb8034a_a45537bd?usp... : PS15, Line 145: for (j = i + 1; j < num; j++) { this assumes the bits in the same group are placed together, if removing this assumption, will this code still work?
https://review.coreboot.org/c/coreboot/+/83321/comment/e5d2be89_98fe497a?usp... : PS15, Line 160: */ so the assumption is: only pad_config_mask covered bits are with explicit setting, while the not covered parts are left default, right?
https://review.coreboot.org/c/coreboot/+/83321/comment/c3c71421_335fa683?usp... : PS15, Line 207: if (comm_index != SNR_GPIO_COMMUNITY(gpio[j].cfg.pad) || if remove this assumption, there will be duplicated settings, but the result will be still correct. P.S. this assumption cannot remove all duplication cases (e.g. pads in the same community are scattered in the input list), maybe we can directly remove it?
File src/soc/intel/snowridge/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/536c7ed1_dd329243?usp... : PS15, Line 108: const FSP_SMBIOS_MEMORY_INFO *fsp_smbios_memory_info; to declare variable along with using will help the readability of the codes.
https://review.coreboot.org/c/coreboot/+/83321/comment/dc156a3e_d5fa7bda?usp... : PS15, Line 133: channel < ARRAY_SIZE(fsp_smbios_memory_info->ChannelInfo) && assume fsp_smbios_memory_info->ChannelCount == ARRAY_SIZE(fsp_smbios_memory_info->ChannelInfo)?
https://review.coreboot.org/c/coreboot/+/83321/comment/89ac4f7e_acd9f3e2?usp... : PS15, Line 139: dimm < ARRAY_SIZE(channel_info->DimmInfo) && assume dimm < channel_info->DimmCoun == dimm < ARRAY_SIZE(channel_info->DimmInfo)?