Attention is currently required from: Angel Pons, Bill XIE, Jérémy Compostella, Keith Hui, Paul Menzel.
Nicholas Chin has posted comments on this change by Keith Hui. ( https://review.coreboot.org/c/coreboot/+/85413?usp=email )
Change subject: mb/asus/p8z77-v: Add support to reconfigure PCIe lanes ......................................................................
Patch Set 20:
(7 comments)
File src/mainboard/asus/p8x7x-series/ifd.c:
PS20: It probably would be good to factor this out as a separate driver outside the mainboard code, since this should be possible to adapt for multiple platforms. This could be done later in a follow up.
File src/mainboard/asus/p8x7x-series/mainboard.c:
https://review.coreboot.org/c/coreboot/+/85413/comment/3bd47a88_88c586ec?usp... : PS20, Line 57: /* Enable BIOS write with no prefetch or cache */ : pci_s_write_config8(PCH_LPC_DEV, BIOS_CNTL, 0x05); I'm not sure this is actually necessary. From what I understand, the BIOS_CNTL register only affects writes to the BIOS region. I seem to be able to write the IFD just fine with flashrom with unlocked permissions in the IFD, even when the the BLE bit is set. I'm not able to write the BIOS region in that case.
https://review.coreboot.org/c/coreboot/+/85413/comment/b62ab632_56fc99e7?usp... : PS20, Line 73: int32_t me_sig = pci_s_read_config32(PCH_ME_DEV, PCI_VENDOR_ID);
There is a signed comparison (see the -1 that follows?). […]
What it's really checking for is if the vendor ID is all ones. I would use uint32_t and compare against `0xffffffff` instead of using signed integer semantics where it doesn't really apply. That would also be consistent with other places in the tree where PCI_VENDOR_ID is being checked.
https://review.coreboot.org/c/coreboot/+/85413/comment/2860b8e0_453e3b3d?usp... : PS20, Line 75: if (me_sig != -1 && hfs.working_state != ME_HFS_CWS_DISABLED && : hfs.operation_mode != ME_HFS_MODE_DEBUG) { I don't think this is the best way to determine if the IFD can be written to. From what I understand, the writability of the IFD is controlled by the effective permissions of the IFD region, which is either whatever the FD region permissions are set to in the descriptor, or RW if the flash descriptor override is set. The state of the ME is not directly related to the writability of the IFD, though it may match by coincidence since things like the FDO also disable the ME.
The effective write permission of all the regions is accessible at SPIBAR + 0x50, bits [7:0], with each bit corresponding to a particular region (refer to section 22.1.7 FRAP—Flash Regions Access Permissions Register of the 7 Series PCH datasheet). The IFD is region 0, so its write permission is reflected in bit 0 of that register.
The state of the FDO can also be determined from SPIBAR + 0x4, bit 13 (refer to section 22.1.2 HSFS—Hardware Sequencing Flash Status Register of the 7 Series PCH datasheet)
Testing on my Latitude E6430, I can write the IFD perfectly fine with flashrom with a fully operational ME as long as the descriptor region is unlocked in the IFD. Setting the FDO would also make it writable (with the side effect of disabling the ME)
File src/mainboard/asus/p8x7x-series/variants/p8z77-v/early_init.c:
https://review.coreboot.org/c/coreboot/+/85413/comment/89fa571c_ed52d5fb?usp... : PS20, Line 47: ME_UNLOCK I know the boardview says this, and setting the FDO also disables the ME, but the main purpose of this in this case is to set the FDO so that the FD can be written to. That said, I don't really care too much either way if you just want to be consistent with the boardview.
File src/mainboard/asus/p8x7x-series/variants/p8z77-v/pcielane.c:
https://review.coreboot.org/c/coreboot/+/85413/comment/35c9a06e_d6dfc199?usp... : PS17, Line 18: static void pnp_enter_config(void) You should be able to use the code defined in `<device/pnp.h>` (since this is ramstage) for all these `pnp_*` functions. Those do need a `struct device` passed into them though. One way to get the device is by assigning a devicetree alias to a pnp device, and then using the `DEV_PTR(alias)` to access it. See `mb/purism/librem_l1um_v2/ramstage.c` and its devicetree for an example of this.
https://review.coreboot.org/c/coreboot/+/85413/comment/9918a0a1_38f76962?usp... : PS17, Line 46: mainboard_unlock_me I'd probably name this something like `mainboard_set_fdo` (don't forget to change the header) as that reflects that actual intent of the code. The fact that it also disables the ME is a side effect, but the main intent is to set the FDO so that the IFD can be written even if its permissions mark the FD as read only.