Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54305 )
Change subject: util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset ......................................................................
Patch Set 10:
(1 comment)
Patchset:
PS10:
Earlier if your platform was entering into earlier if loop and touching bit 7:0 then it was doing wrong programming.
I either don't understand that sentence correctly or it is the opposite, the else clause is what touches bit 7:0.
Its exactly opposite what i said, SKL SPI programming guide says, Bit 7:0 are reserved and other IFDv2 chipset has extended ranges hence it might need to protect.
if (ifd_version >= IFD_VERSION_2) { /* Clear non-reserved bits */ fmba->flmstr1 &= 0xff; fmba->flmstr2 &= 0xff; fmba->flmstr3 &= 0xff; fmba->flmstr5 &= 0xff; } else { wr_shift = FLMSTR_WR_SHIFT_V1; rd_shift = FLMSTR_RD_SHIFT_V1;
fmba->flmstr1 = 0; fmba->flmstr2 = 0; /* Requestor ID */ fmba->flmstr3 = 0x118; }
If ifd_version is set to IFD_VERSION_2 like it was before this change, then it will not touch bits 7:0 and set read and write permissions to true. In the else-clause of the condition, which it uses now, both 7:0 reserved bits are changed and (I think this is what breaks boot) read permission bits 15:8 are set to 0.
if you could download the SPI programming guide from this link https://www.intel.com/content/www/us/en/search.html?ws=text#q=550696&t=A... and go to page 33, Flash Descriptor Master Section, you will see 7:0 are reserved and what I could sense is that, existing code itself had issue
https://review.coreboot.org/c/coreboot/+/56070/1/util/ifdtool/ifdtool.c#1278 should be as below to protect bits 15:8 as well. fmba->flmstr1 = 0xffffff00; fmba->flmstr2 = 0xffffff00;
I would say, it was wrong that SKL was considered as IFDv2.
Even if IFDv2 is supposed to mean that 7:0 of the flash master are used for Extended Region Access Permissions somewhere else, in ifdtool it just decides whether FLMSTR uses 31:16 or 31:8 for permissions.
IMHO adding SKLKBL to the ifd_2_platforms list is the easiest solution to fix this issue. This is the change I applied to make it work for me: https://review.coreboot.org/c/coreboot/+/56070
Adding SKL into IFDv2 would be an easy W/A as SKL flash descriptor master register offsets are very close to IFDv2 (except the last byte).
Have you verified other fields as well with CB:56070 for skl platform?