<p>Furquan Shaikh has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/26023">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">ifdtool: Add a list of known platforms that support IFD_VERSION_2<br><br>ifdtool has relied on one of the fields within FCBA(read_freq) to<br>determine whether a platform supports IFD_VERSION_1 or<br>IFD_VERSION_2. However, newer platforms like GLK and CNL do not have<br>read_freq field in FCBA and so the value of these bits cannot be used<br>as an indicator to distinguish IFD versions. In the long run, we need<br>to re-write ifdtool to have a better mapping of SoC to IFD fields. But<br>until that is done, this change adds a list of platforms that we know<br>do not support read_freq field but still use IFD_VERSION_2. This<br>change also updates GLK and CNL to pass in platform parameter to<br>ifdtool.<br><br>BUG=b:79109029, b:69270831<br><br>Change-Id: I36c49f4dcb480ad53b0538ad12292fb94b0e3934<br>Signed-off-by: Furquan Shaikh <furquan@google.com><br>---<br>M src/soc/intel/apollolake/Kconfig<br>M src/soc/intel/cannonlake/Kconfig<br>M util/ifdtool/ifdtool.c<br>M util/ifdtool/ifdtool.h<br>4 files changed, 58 insertions(+), 11 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/26023/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig</span><br><span>index 8cac151..0f1f121 100644</span><br><span>--- a/src/soc/intel/apollolake/Kconfig</span><br><span>+++ b/src/soc/intel/apollolake/Kconfig</span><br><span>@@ -350,6 +350,7 @@</span><br><span> </span><br><span> config IFD_CHIPSET</span><br><span> string</span><br><span style="color: hsl(120, 100%, 40%);">+ default "glk" if SOC_INTEL_GLK</span><br><span> default "aplk"</span><br><span> </span><br><span> config CPU_BCLK_MHZ</span><br><span>diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig</span><br><span>index 541e516..898b444 100644</span><br><span>--- a/src/soc/intel/cannonlake/Kconfig</span><br><span>+++ b/src/soc/intel/cannonlake/Kconfig</span><br><span>@@ -114,6 +114,10 @@</span><br><span> The amount of anticipated stack usage in CAR by bootblock and</span><br><span> other stages.</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+config IFD_CHIPSET</span><br><span style="color: hsl(120, 100%, 40%);">+ string</span><br><span style="color: hsl(120, 100%, 40%);">+ default "cnl"</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> config IED_REGION_SIZE</span><br><span> hex</span><br><span> default 0x400000</span><br><span>diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c</span><br><span>index 12dfdd1..73fa0ff 100644</span><br><span>--- a/util/ifdtool/ifdtool.c</span><br><span>+++ b/util/ifdtool/ifdtool.c</span><br><span>@@ -126,14 +126,35 @@</span><br><span> }</span><br><span> </span><br><span> /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * Some newer platforms have re-defined the FCBA field that was used to</span><br><span style="color: hsl(120, 100%, 40%);">+ * distinguish IFD v1 v/s v2. Define a list of platforms that we know do not</span><br><span style="color: hsl(120, 100%, 40%);">+ * have the required FCBA field, but are IFD v2 and return true if current</span><br><span style="color: hsl(120, 100%, 40%);">+ * platform is one of them.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+static int is_platform_ifd_2(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ static const int ifd_2_platforms[] = {</span><br><span style="color: hsl(120, 100%, 40%);">+ PLATFORM_GLK,</span><br><span style="color: hsl(120, 100%, 40%);">+ PLATFORM_CNL,</span><br><span style="color: hsl(120, 100%, 40%);">+ };</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int i;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ for (i = 0; i < ARRAY_SIZE(ifd_2_platforms); i++) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (platform == ifd_2_platforms[i])</span><br><span style="color: hsl(120, 100%, 40%);">+ return 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*</span><br><span> * There is no version field in the descriptor so to determine</span><br><span> * if this is a new descriptor format we check the hardcoded SPI</span><br><span> * read frequency to see if it is fixed at 20MHz or 17MHz.</span><br><span> */</span><br><span style="color: hsl(0, 100%, 40%);">-static void check_ifd_version(char *image, int size)</span><br><span style="color: hsl(120, 100%, 40%);">+static int get_ifd_version_from_fcba(char *image, int size)</span><br><span> {</span><br><span> int read_freq;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> const fcba_t *fcba = find_fcba(image, size);</span><br><span> if (!fcba)</span><br><span> exit(EXIT_FAILURE);</span><br><span>@@ -142,14 +163,10 @@</span><br><span> </span><br><span> switch (read_freq) {</span><br><span> case SPI_FREQUENCY_20MHZ:</span><br><span style="color: hsl(0, 100%, 40%);">- ifd_version = IFD_VERSION_1;</span><br><span style="color: hsl(0, 100%, 40%);">- max_regions = MAX_REGIONS_OLD;</span><br><span style="color: hsl(0, 100%, 40%);">- break;</span><br><span style="color: hsl(120, 100%, 40%);">+ return IFD_VERSION_1;</span><br><span> case SPI_FREQUENCY_17MHZ:</span><br><span> case SPI_FREQUENCY_50MHZ_30MHZ:</span><br><span style="color: hsl(0, 100%, 40%);">- ifd_version = IFD_VERSION_2;</span><br><span style="color: hsl(0, 100%, 40%);">- max_regions = MAX_REGIONS;</span><br><span style="color: hsl(0, 100%, 40%);">- break;</span><br><span style="color: hsl(120, 100%, 40%);">+ return IFD_VERSION_2;</span><br><span> default:</span><br><span> fprintf(stderr, "Unknown descriptor version: %d\n",</span><br><span> read_freq);</span><br><span>@@ -157,6 +174,20 @@</span><br><span> }</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static void check_ifd_version(char *image, int size)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ if (is_platform_ifd_2())</span><br><span style="color: hsl(120, 100%, 40%);">+ ifd_version = IFD_VERSION_2;</span><br><span style="color: hsl(120, 100%, 40%);">+ else</span><br><span style="color: hsl(120, 100%, 40%);">+ ifd_version = get_ifd_version_from_fcba(image, size);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ifd_version == IFD_VERSION_1)</span><br><span style="color: hsl(120, 100%, 40%);">+ max_regions = MAX_REGIONS_OLD;</span><br><span style="color: hsl(120, 100%, 40%);">+ else</span><br><span style="color: hsl(120, 100%, 40%);">+ max_regions = MAX_REGIONS;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static region_t get_region(const frba_t *frba, unsigned int region_type)</span><br><span> {</span><br><span> int base_mask;</span><br><span>@@ -816,7 +847,8 @@</span><br><span> }</span><br><span> </span><br><span> switch (platform) {</span><br><span style="color: hsl(0, 100%, 40%);">- case PLATFORM_APOLLOLAKE:</span><br><span style="color: hsl(120, 100%, 40%);">+ case PLATFORM_APL:</span><br><span style="color: hsl(120, 100%, 40%);">+ case PLATFORM_GLK:</span><br><span> /* CPU/BIOS can read descriptor and BIOS */</span><br><span> fmba->flmstr1 |= 0x3 << rd_shift;</span><br><span> /* CPU/BIOS can write BIOS */</span><br><span>@@ -1153,6 +1185,9 @@</span><br><span> " -u | --unlock Unlock firmware descriptor and ME region\n"</span><br><span> " -p | --platform Add platform-specific quirks\n"</span><br><span> " aplk - Apollo Lake\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ " cnl - Cannon Lake\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ " glk - Gemini Lake\n"</span><br><span style="color: hsl(120, 100%, 40%);">+ " sklkbl - Skylake/Kaby Lake\n"</span><br><span> " -v | --version: print the version\n"</span><br><span> " -h | --help: print this help\n\n"</span><br><span> "<region> is one of Descriptor, BIOS, ME, GbE, Platform\n"</span><br><span>@@ -1342,13 +1377,18 @@</span><br><span> break;</span><br><span> case 'p':</span><br><span> if (!strcmp(optarg, "aplk")) {</span><br><span style="color: hsl(0, 100%, 40%);">- platform = PLATFORM_APOLLOLAKE;</span><br><span style="color: hsl(120, 100%, 40%);">+ platform = PLATFORM_APL;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (!strcmp(optarg, "cnl")) {</span><br><span style="color: hsl(120, 100%, 40%);">+ platform = PLATFORM_CNL;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (!strcmp(optarg, "glk")) {</span><br><span style="color: hsl(120, 100%, 40%);">+ platform = PLATFORM_GLK;</span><br><span> } else if (!strcmp(optarg, "sklkbl")) {</span><br><span> platform = PLATFORM_SKLKBL;</span><br><span> } else {</span><br><span> fprintf(stderr, "Unknown platform: %s\n", optarg);</span><br><span> exit(EXIT_FAILURE);</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+ fprintf(stderr, "Platform is: %s\n", optarg);</span><br><span> break;</span><br><span> case 'v':</span><br><span> print_version();</span><br><span>diff --git a/util/ifdtool/ifdtool.h b/util/ifdtool/ifdtool.h</span><br><span>index ad299a9..ef85555 100644</span><br><span>--- a/util/ifdtool/ifdtool.h</span><br><span>+++ b/util/ifdtool/ifdtool.h</span><br><span>@@ -22,7 +22,9 @@</span><br><span> };</span><br><span> </span><br><span> enum platform {</span><br><span style="color: hsl(0, 100%, 40%);">- PLATFORM_APOLLOLAKE,</span><br><span style="color: hsl(120, 100%, 40%);">+ PLATFORM_APL,</span><br><span style="color: hsl(120, 100%, 40%);">+ PLATFORM_CNL,</span><br><span style="color: hsl(120, 100%, 40%);">+ PLATFORM_GLK,</span><br><span> PLATFORM_SKLKBL,</span><br><span> };</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/26023">change 26023</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/26023"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I36c49f4dcb480ad53b0538ad12292fb94b0e3934 </div>
<div style="display:none"> Gerrit-Change-Number: 26023 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Furquan Shaikh <furquan@google.com> </div>