Attention is currently required from: Elyes Haouas, Felix Singer, Jérémy Compostella, Shuo Liu, Vasiliy Khoruzhick.
yuchi.chen@intel.com 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 21:
(20 comments)
File src/soc/intel/snowridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/83321/comment/1fb1a946_d134e082?usp... : PS15, Line 123: default 0x3ff00 if FSP_CAR
Are FSP_CAR and !FSP_CAR both validated?
Only FSP_CAR is valid, I will remove the untested one.
https://review.coreboot.org/c/coreboot/+/83321/comment/9b3d841b_c74d1ebe?usp... : PS15, Line 244: 0xfe00000. It will be used until coreboot resource allocation overwrite it
0xfe000000?
Done
File src/soc/intel/snowridge/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83321/comment/d98d4dbb_0d8fdeac?usp... : PS15, Line 60: endif ## CONFIG_SOC_INTEL_FSP_SNOWRIDGE
CONFIG_SOC_INTEL_SNOWRIDGE
Done
File src/soc/intel/snowridge/acpi.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/198d78c2_5c0d20b8?usp... : PS15, Line 243: if (read32p(HPET_BASE_ADDRESS + 0x100) & 0x00008000) {
are there macros for these constants?
The same code also exists in src/soc/intel/xeon_sp/uncore_acpi.c. I will add a separate patch that adds macros and update that file.
File src/soc/intel/snowridge/acpi/ith.asl:
https://review.coreboot.org/c/coreboot/+/83321/comment/8368517d_1a1c209d?usp... : PS17, Line 2:
if this is not referred, clean it? maybe to clean to a minimal asl set?
It's referred by the last line of southcluster.asl.
File src/soc/intel/snowridge/bootblock/early_uart_init.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/848f1c28_876faf51?usp... : PS15, Line 11: static void pci_early_hsuart_device_probe(uint8_t bus, uint8_t dev, uint8_t func,
probe -> enable?
Done
https://review.coreboot.org/c/coreboot/+/83321/comment/31a08eb7_a8c7e117?usp... : PS15, Line 43: write32p(reg32 + UCMR_OFFSET, read32p(reg32 + UCMR_OFFSET) * HIGH_SPEED_CLK_MULT);
reg32 -> iobase?
Done
File src/soc/intel/snowridge/chip.h:
https://review.coreboot.org/c/coreboot/+/83321/comment/406c01f9_6eee1984?usp... : PS15, Line 12: * additional root bus in stack 2 and 7 (UBox1).
So what is for stack 2?
It's Intel Dynamic Load Balancer, I will update the comments.
File src/soc/intel/snowridge/chip.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/3dc3ed6a_58885d73?usp... : PS15, Line 75: if (sr[i].Personality >= BL_TYPE_DISABLED)
is there any reason not using !=?
For SNR, each type of stack has its own disabled enumeration value, see BL_STACK_TYPE in src/vendorcode/intel/fsp/fsp2_0/snowridge/FspmUpd.h. I will also update the comment.
https://review.coreboot.org/c/coreboot/+/83321/comment/c04c22cc_69a20b15?usp... : PS15, Line 180: int i;
for (int i = 0; ... […]
Done
File src/soc/intel/snowridge/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/83321/comment/707fbecb_e6af8552?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 o […]
These sets of values are used to define struct pad_community, see src/soc/intel/snowridge/common/gpio.c.
File src/soc/intel/snowridge/lockdown.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/65eae10a_96235654?usp... : PS15, Line 22: if (chipset_lockdown == CHIPSET_LOCKDOWN_COREBOOT)
is CHIPSET_LOCKDOWN_FSP be tested? If no, we could just assert only support coreboot lockdown?
It's not tested, replacing it with an assert is more reasonable.
File src/soc/intel/snowridge/romstage/gpio_snr.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/becd718b_18340ea5?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 […]
It still works. Since configuration lock registers for pads in the same group are in the same 32-bit registers, the inner for-j loop will collect and unlock them in one PCR sideband message. I will remove the inner loop if there is no significant impacts on the performance.
https://review.coreboot.org/c/coreboot/+/83321/comment/649b97fc_907b0d91?usp... : PS15, Line 160: */
so the assumption is: only pad_config_mask covered bits are with explicit setting, while the not cov […]
yes.
https://review.coreboot.org/c/coreboot/+/83321/comment/ca8e436c_d53ec631?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. […]
I think we can just remove the inner for-j loop just for simplicity.
File src/soc/intel/snowridge/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/4c600f62_ce527c30?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)?
From FSP outputs, it seems ChannelCount is equal to array size of ChannelInfo. I think I can add an assert to simplify the for condition.
https://review.coreboot.org/c/coreboot/+/83321/comment/620659ae_2e990335?usp... : PS15, Line 139: dimm < ARRAY_SIZE(channel_info->DimmInfo) &&
assume dimm < channel_info->DimmCoun == dimm < ARRAY_SIZE(channel_info->DimmInfo)?
I will add an assert for DimmCount and DimmInfo.
File src/soc/intel/snowridge/systemagent.c:
https://review.coreboot.org/c/coreboot/+/83321/comment/87abb042_ab5447df?usp... : PS15, Line 32: void soc_add_fixed_mmio_resources(struct device *dev, int *resource_cnt)
resource_cnt -> resource_index?
The parameter in the declaration is resource_cnt, but all the SoC implementations renamed it to index. Maybe we need another patch to keep them consistent.
https://review.coreboot.org/c/coreboot/+/83321/comment/d3d18c4c_8b6b5d13?usp... : PS15, Line 33: {
The resource_cnt is a bit confusing, is there cases where resource_cnt != 1?
This pointer is passed from common system agent code, representing current assigned resources in system agent, then passed to sa_add_fixed_mmio_resources. When it returns, it will be increased.
https://review.coreboot.org/c/coreboot/+/83321/comment/2d0c9ca1_42d352d9?usp... : PS15, Line 56: I
PCIe MMCFG BAR?
Done