Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44163 )
Change subject: soc/intel/alderlake/bootblock: Do initial SoC commit till bootblock ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... File src/soc/intel/alderlake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 9: #include <console/console.h> : #include <console/post_codes.h>
unused?
Ack
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 59: reg16 = pci_read_config16(PCH_DEV_PMC, PCI_COMMAND); : reg16 &= ~(PCI_COMMAND_MEMORY); : pci_write_config16(PCH_DEV_PMC, PCI_COMMAND, reg16);
use pci_and_config16
Ack
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 69: /* Enable PWRM in PMC */ : reg32 = read32((void *)(PCH_PWRM_BASE_ADDRESS + ACTL)); : write32((void *)(PCH_PWRM_BASE_ADDRESS + ACTL), reg32 | PWRM_EN);
use setbits32
Done
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 116: dmi_control = pcr_read32(PID_DMI, PCR_DMI_DMICTL);
maybe do the asignment and declaration on the same line to save space?
Ack
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... File src/soc/intel/alderlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 172: index = 0x80000000; : cpuidr = cpuid(index); : if (cpuidr.eax >= 0x80000004) { : int j = 0; : : for (i = 2; i <= 4; i++) { : cpuidr = cpuid(index + i); : p[j++] = cpuidr.eax; : p[j++] = cpuidr.ebx; : p[j++] = cpuidr.ecx; : p[j++] = cpuidr.edx; : } : p[12] = 0; : cpu_name = (char *)p; : : /* Skip leading spaces in CPU name string */ : while (cpu_name[0] == ' ' && strlen(cpu_name) > 0) : cpu_name++; : }
there is a function for that: fill_processor_name(). […]
Ack
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 192: microcode_ver.lo = 0; : microcode_ver.hi = 0; : wrmsr(BIOS_SIGN_ID, microcode_ver); : cpu_id = cpu_get_cpuid(); : microcode_ver = rdmsr(BIOS_SIGN_ID)
maybe extend the scope of 'static inline u32 read_microcode_rev(void)' in cpu/intel/microcode/microc […]
valid point, shall i address that in separate CL for all possible intel SoC ?
https://review.coreboot.org/c/coreboot/+/44163/2/src/soc/intel/alderlake/boo... PS2, Line 222: pci_devfn_t dev = SA_DEV_ROOT; : uint16_t mchid = get_dev_id(dev); : uint8_t mch_revision = get_dev_revision(dev);
nit: just use uint16_t mchid = get_dev_id(SA_DEV_ROOT) ?
Ack