Change in coreboot[master]: util/ifdtool: Use -p platform name to detect IFDv2 platform and chipset

[Subrata Banik (Code Review)]

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:
> X11SSH-LN4F is C236 chipset. It passes "-p sklkbl" to ifdtool.
>
> Prior to this change get_ifd_version_from_fcba() detected this as IFDv2, in https://review.coreboot.org/c/coreboot/+/54305/10/util/ifdtool/ifdtool.c#b29... get_ifd_version_from_fcba() got called if it's not in the list.

I guess this was doing the trick for you https://review.coreboot.org/c/coreboot/+/54305/10/util/ifdtool/ifdtool.c#b28...
> So it did not matter whether it was in that list, because it got detected as IFD2 anyway.

I would say, it was wrong that SKL was considered as IFDv2.
> By removing get_ifd_version_from_fcba() and not adding it to the list it now gets treated as IFDv1.
>
> Because it worked with the IFDv2 code and breaks with the IFDv1 code, it must be an IFDv2 chipset.

If i remember the SKL Flash layout correctly, it was IFWI layout 1.1

looking at the code here, i could only detect IFD_VERSION_2 check is there for lock_descriptor() to map Flash Master access and for SKL, it was Flash Master 1 to 3 and rest of the code doesn't really bother about chipset version as it use platform name directly.

Same as unlock_descriptor() function where it program those required flash master range as per SPI programming guide.

At high-level the only delta between SKL and other IFDv2 chipsets are bits[7:0] for all Flash master range. I don't really see anything wrong in the code as we shuldn't let the IFD to program a range which is marked "reserved"

For SKL, all Flash Master register bit 7:0 are marked reserved and for other IFDv2 chipsets, bit [7:0] are split into two parts 7:4 Extended Region Write Access and 3:0 Extended Region Read Access

if (ifd_version >= IFD_VERSION_2) {
    /* Access bits for each region are read: 19:8 write: 31:20 */
    fmba->flmstr1 = 0xffffff00 | (fmba->flmstr1 & 0xff);
    fmba->flmstr2 = 0xffffff00 | (fmba->flmstr2 & 0xff);
    fmba->flmstr3 = 0xffffff00 | (fmba->flmstr3 & 0xff);
    fmba->flmstr5 = 0xffffff00 | (fmba->flmstr5 & 0xff);
} else {
    fmba->flmstr1 = 0xffff0000;
    fmba->flmstr2 = 0xffff0000;
    /* Keep chipset specific Requester ID */
    fmba->flmstr3 = 0x08080000 | (fmba->flmstr3 & 0xffff);
}

Earlier if your platform was entering into earlier if loop and touching bit 7:0 then it was doing wrong programming.

Apart from that flmstr5 is for EC region access that IFDv2 chipsets are locking and earlier won't. Do you have concern regarding that?