Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81349?usp=email )
Change subject: soc/intel/xeon_sp: Remove PAM unlock operations ......................................................................
Patch Set 15:
(2 comments)
Patchset:
PS15:
It's moved from romstage to ramstage on platforms that we think need it. […]
Sure, unlock_pam_region() will route the access to PAM-F segment to DRAM, coreboot places ACPI RSDP lower pointer here, without RSDP pointer, LinuxBoot cannot boot.
In SPR, FSP covered such unlock operations (confirmed from source code), and after removed from coreboot, the coreboot ACPI table can still be accessed by LinuxBoot, hence it proves the removal is okay for SPR. (for GNR, the same).
For the calling site of the unlock in CPX and SKX, it is moved from late romstage and early ramstage, which is close to each other. From the ACPI table write point of view, they are similar. However, only compilation test are executed for SKX and CPX.
File src/soc/intel/xeon_sp/cpx/chip.c:
https://review.coreboot.org/c/coreboot/+/81349/comment/95563f6e_6ac2b76e : PS15, Line 164: /* Only call this code from socket0! */ : static void unlock_pam_regions(void) : { : uint32_t pam0123_unlock_dram = 0x33333330; : uint32_t pam456_unlock_dram = 0x00333333; : /* Get UBOX(1) for socket0 */ : uint32_t bus1 = socket0_get_ubox_busno(PCU_IIO_STACK); : : /* Assume socket0 owns PCI segment 0 */ : pci_io_write_config32(PCI_DEV(bus1, SAD_ALL_DEV, SAD_ALL_FUNC), : SAD_ALL_PAM0123_CSR, pam0123_unlock_dram); : pci_io_write_config32(PCI_DEV(bus1, SAD_ALL_DEV, SAD_ALL_FUNC), : SAD_ALL_PAM456_CSR, pam456_unlock_dram); : : uint32_t reg1 = pci_io_read_config32(PCI_DEV(bus1, SAD_ALL_DEV, : SAD_ALL_FUNC), SAD_ALL_PAM0123_CSR); : uint32_t reg2 = pci_io_read_config32(PCI_DEV(bus1, SAD_ALL_DEV, : SAD_ALL_FUNC), SAD_ALL_PAM456_CSR); : printk(BIOS_DEBUG, "%s:%s pam0123_csr: 0x%x, pam456_csr: 0x%x\n", : __FILE__, __func__, reg1, reg2); : }
Why is this code duplicated in cpx and skx. It was also valid for SPR-SP but not used. […]
This is because this code cannot pass compilation with the GNR target. If we would like to further share the codes, I can make another patch to put this into chip_gen1.c. Your opinion?