Re: [flashrom] [PATCH 2/4] add ICH/PCH flash descriptor decoding via FDOC/FDOD

[Carl-Daniel Hailfinger]

Am 21.07.2011 01:24 schrieb Stefan Tauner:
> based on the work of Matthias 'mazzoo' Wenzel this patch adds pretty
> printing of those ICH/PCH flash descriptors that are cached/mapped
> by the chipset (and which are therefore reachable via FDOC/FDOD
> registers).
>
> this includes the following:
> content descriptor:
>   describes the image and some generic properties (number of regions,
>   offset of regions,

offset of which regions, or something else? How does this relate to
region descriptor(s)?
> PCH/ICH and MCH/PROC strap offsets and lengths)
> component descriptor:
>   identify the different SPI Flash Components and their capabilities.

components==chips? If yes, please say so in the changelog (and maybe
code comments).
> region descriptor
>   similarly to a partition table this describes the different regions

One common descriptor for all regions or one descriptor per region?
> master descriptor
>   defines access rights of the individual regions
>
> this is only a part of the data included in the descriptor. other
> information can be retreived

retrieved
> from a complete binary dump of the descriptor image only.

What's a descriptor image?
> this patch also adds macros and pretty printing for "Vendor Specific
> Component Capabilities" registers:
> there are two of them: lower and upper. they describe the properties
> of the address space divided by FPBA

We really need some place (maybe as comment in ich_decriptor.h?) where
all Intel acronyms are expanded and explained.
> (which allows to use multiple
> flash chips or partitions with different properties). the properties
> of flash chips supported by the ME are stored in the same format in
> a descriptor table.

"a descriptor table" implies there is more than one. Which one is it?
> a later patch will uses them outside of ichspi.c
> which is also the reason why the prettyprinting function and the
> register bit macros are not defined in ichspi.c but ich_descriptors.h.
>
> TODO:
> file naming and partitioning was not discussed. we may want a ichspi.h.
> and/or rename ichspi.c to ich_spi.c or similar...

I'd say naming of the files is the least of our worries. We'll
reorganize all flashrom code in a directory structure anyway, and that's
when we can rename files if needed.
> straps of ich8 (only)

Why not ICH7 (no descriptors?) or ICH9 and later (no datasheets?)
> could be printed at runtime via FDOC/FDOD.
>   worth it? no, imo.

Agreed.
> but reading out the descriptor region manually and
>   parsing it with the following patch may be useful?

following patch == 3/4?
Once we support true partial reads with the help of layout files, this
should be possible.
> Signed-off-by: Stefan Tauner

Please note that I didn't check the code for correctness against the
datasheets nor did I check the code flow. The comments below are what I
noticed from reading the patch alone.

The message levels of most messages really need to be discussed... a
flood of msg_dbg is not necessarily a good idea, downgrading a lot of
them to dbg2 may improve said flood. That said, msg_[cpg]dbg2 is now in
the tree, so the dbg1 hack can go away. Looking again at your
messagelevels, maybe dbg1->dbg2 and dbg2->spew would be in order.

All ich specific structs should have an ich_ prefix in the struct name
to avoid polluting the global namespace.
> Makefile          |    3 +-
>  ich_descriptors.c |  247 +++++++++++++++++++++++++++++++++++++++++++++++++
>  ich_descriptors.h |  264 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ichspi.c          |   39 ++++++--
>  4 files changed, 544 insertions(+), 9 deletions(-)
>  create mode 100644 ich_descriptors.c
>  create mode 100644 ich_descriptors.h
>
> diff --git a/Makefile b/Makefile
> index a35df06..7f73452 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -308,7 +308,8 @@ ifeq ($(CONFIG_INTERNAL), yes)
>  FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
>  PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o cbtable.o dmi.o internal.o
>  ifeq ($(ARCH),"x86")
> -PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o sb600spi.o wbsio_spi.o mcp6x_spi.o
> +PROGRAMMER_OBJS += it87spi.o it85spi.o sb600spi.o wbsio_spi.o mcp6x_spi.o
> +PROGRAMMER_OBJS += ichspi.o ich_descriptors.o
>  else
>  endif
>  NEED_PCI := yes
> diff --git a/ich_descriptors.c b/ich_descriptors.c
> new file mode 100644
> index 0000000..cf77601
> --- /dev/null
> +++ b/ich_descriptors.c
> @@ -0,0 +1,247 @@
> +/*
> This file is part of the flashrom project.
> Copyright (c) 2010  Matthias Wenzel <bios at mazzoo dot de>
> Copyright (c) 2011  Stefan Tauner
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#if defined(__i386__) || defined(__x86_64__)
>
> +#include "ich_descriptors.h"
> +#include "flash.h" /* for msg_* */
>
> +#define getFCBA(cont)	(((cont)->FLMAP0 <<  4) & 0x00000ff0)
> +#define getFRBA(cont)	(((cont)->FLMAP0 >> 12) & 0x00000ff0)
> +#define getFMBA(cont)	(((cont)->FLMAP1 <<  4) & 0x00000ff0)
> +#define getFISBA(cont)	(((cont)->FLMAP1 >> 12) & 0x00000ff0)
> +#define getFMSBA(cont)	(((cont)->FLMAP2 <<  4) & 0x00000ff0)
>
> +void prettyprint_ich_descriptors(enum chipset cs, const struct flash_descriptors *desc)

(enum ich_chipset cs, const struct ich_flash_descriptors *desc)
> +{

Please try to handle cs at least somewhat. Check if it has the expected
value, and skip descriptor decoding (with a dbg/dbg2 message telling the
user about it) if the result would be wrong (i.e. for the wrong ICH family).
> prettyprint_ich_descriptor_content(&desc->content);
> prettyprint_ich_descriptor_component(desc);
> prettyprint_ich_descriptor_region(&desc->region);
> prettyprint_ich_descriptor_master(&desc->master);
>
> +}
>
> +void prettyprint_ich_descriptor_content(const struct flash_content *cont)

ich_flash_content

Does this work on all chipsets?
> +{
> msg_pdbg1("=== Content Section ===\n");
> msg_pdbg1("FLVALSIG 0x%08x\n", cont->FLVALSIG);
> msg_pdbg1("FLMAP0   0x%08x\n", cont->FLMAP0);
> msg_pdbg1("FLMAP1   0x%08x\n", cont->FLMAP1);
> msg_pdbg1("FLMAP2   0x%08x\n", cont->FLMAP2);
> msg_pdbg1("\n");

dbg1->dbg, or maybe ->dbg2 to reduce output. The message level
discussion should not hold back this patch.
> msg_pdbg2("--- Details ---\n");
> msg_pdbg2("NR          (Number of Regions):                 %5d\n",
>   cont->NR + 1);
> msg_pdbg2("FRBA        (Flash Region Base Address):         0x%03x\n",
>   getFRBA(cont));
> msg_pdbg2("NC          (Number of Components):              %5d\n",
>   cont->NC + 1);
> msg_pdbg2("FCBA        (Flash Component Base Address):      0x%03x\n",
>   getFCBA(cont));
> msg_pdbg2("ISL         (ICH/PCH Strap Length):              %5d\n",
>   cont->ISL);
> msg_pdbg2("FISBA/FPSBA (Flash ICH/PCH Strap Base Address):  0x%03x\n",
>   getFISBA(cont));
> msg_pdbg2("NM          (Number of Masters):                 %5d\n",
>   cont->NM + 1);
> msg_pdbg2("FMBA        (Flash Master Base Address):         0x%03x\n",
>   getFMBA(cont));
> msg_pdbg2("MSL/PSL     (MCH/PROC Strap Length):             %5d\n",
>   cont->MSL);
> msg_pdbg2("FMSBA       (Flash MCH/PROC Strap Base Address): 0x%03x\n",
>   getFMSBA(cont));
> msg_pdbg2("\n");
>
> +}
>
> +void prettyprint_ich_descriptor_component(const struct flash_descriptors *desc)

ich_flash_descriptors

Which chipset is handled by this function?
> +{
> const char * const str_freq[8] = {

static? Or is that useless here?
> "20 MHz",	/* 000 */
> "33 MHz",	/* 001 */
> "reserved",	/* 010 */
> "reserved",	/* 011 */
> "50 MHz",	/* 100 */

Mh. Is 50 MHz available on all descriptor capable chipsets?
> "reserved",	/* 101 */
> "reserved",	/* 110 */
> "reserved"	/* 111 */
> };
> const char * const str_mem[8] = {

static?
> "512 kB",	/* 000 */
> "  1 MB",	/* 001 */
> "  2 MB",	/* 010 */
> "  4 MB",	/* 011 */
> "  8 MB",	/* 100 */
> " 16 MB",	/* 101 */
> "reserved",	/* 110 */
> "reserved",	/* 111 */
> };
> msg_pdbg1("=== Component Section ===\n");
> msg_pdbg1("FLCOMP   0x%08x\n", desc->component.FLCOMP);
> msg_pdbg1("FLILL    0x%08x\n", desc->component.FLILL );
> msg_pdbg1("\n");
> msg_pdbg2("--- Details ---\n");
> msg_pdbg2("Component 1 density:           %s\n",
>   str_mem[desc->component.comp1_density]);
> if (desc->content.NC)
> msg_pdbg2("Component 2 density:           %s\n",
> 	  str_mem[desc->component.comp2_density]);
> else
> msg_pdbg2("Component 2 is not used.\n");
> msg_pdbg2("Read Clock Frequency:           %s\n",
>   str_freq[desc->component.freq_read]);
> msg_pdbg2("Read ID and Status Clock Freq.: %s\n",
>   str_freq[desc->component.freq_read_id]);
> msg_pdbg2("Write and Erase Clock Freq.:    %s\n",
>   str_freq[desc->component.freq_write]);
> msg_pdbg2("Fast Read is %ssupported.\n",
>   desc->component.fastread ? "" : "not ");
> if (desc->component.fastread)
> msg_pdbg2("Fast Read Clock Frequency:      %s\n",
> 	  str_freq[desc->component.freq_fastread]);
> if (desc->component.FLILL == 0)
> msg_pdbg2("No forbidden opcodes.\n");
> else {
> msg_pdbg2("Invalid instruction 0:          0x%02x\n",
> 	  desc->component.invalid_instr0);
> msg_pdbg2("Invalid instruction 1:          0x%02x\n",
> 	  desc->component.invalid_instr1);
> msg_pdbg2("Invalid instruction 2:          0x%02x\n",
> 	  desc->component.invalid_instr2);
> msg_pdbg2("Invalid instruction 3:          0x%02x\n",
> 	  desc->component.invalid_instr3);
> }
> msg_pdbg2("\n");
>
> +}
>
> +static void pprint_freg(const struct flash_region *reg, uint32_t i)

ich_flash_region

Which chipset? Or is the code generic?
> +{
> const char *const region_names[5] = {
> "Descr.", "BIOS", "ME", "GbE", "Platf."
> };
> uint32_t base = ICH_FREG_BASE(reg->FLREGs[i]);
> uint32_t limit = ICH_FREG_LIMIT(reg->FLREGs[i]);
> msg_pdbg2("Region %d (%-6s) ", i, region_names[i]);
> if (base > limit)
> msg_pdbg2("is unused.\n");
> else
> msg_pdbg2("0x%08x - 0x%08x\n", base, limit | 0x0fff);
>
> +}
>
> +void prettyprint_ich_descriptor_region(const struct flash_region *reg)

ich_flash_region

Chipset specific or generic?
> +{
> uint8_t i;
> msg_pdbg1("=== Region Section ===\n");
> for (i = 0; i < 5; i++)
> msg_pdbg1("FLREG%d   0x%08x\n", i, reg->FLREGs[i]);
> msg_pdbg1("\n");
> msg_pdbg2("--- Details ---\n");
> for (i = 0; i < 5; i++)
> pprint_freg(reg, i);
> msg_pdbg2("\n");
>
> +}
>
> +void prettyprint_ich_descriptor_master(const struct flash_master *mstr)

ich_flash_master
> +{
> msg_pdbg1("=== Master Section ===\n");
> msg_pdbg1("FLMSTR1  0x%08x\n", mstr->FLMSTR1);
> msg_pdbg1("FLMSTR2  0x%08x\n", mstr->FLMSTR2);
> msg_pdbg1("FLMSTR3  0x%08x\n", mstr->FLMSTR3);
> msg_pdbg1("\n");
> msg_pdbg2("--- Details ---\n");
> msg_pdbg2("      Descr. BIOS ME GbE Platf.\n");
> msg_pdbg2("BIOS    %c%c    %c%c  %c%c  %c%c   %c%c\n",
> (mstr->BIOS_descr_r)	?'r':' ', (mstr->BIOS_descr_w)	?'w':' ',
> (mstr->BIOS_BIOS_r)	?'r':' ', (mstr->BIOS_BIOS_w)	?'w':' ',
> (mstr->BIOS_ME_r)	?'r':' ', (mstr->BIOS_ME_w)	?'w':' ',
> (mstr->BIOS_GbE_r)	?'r':' ', (mstr->BIOS_GbE_w)	?'w':' ',
> (mstr->BIOS_plat_r)	?'r':' ', (mstr->BIOS_plat_w)	?'w':' ');
> msg_pdbg2("ME      %c%c    %c%c  %c%c  %c%c   %c%c\n",
> (mstr->ME_descr_r)	?'r':' ', (mstr->ME_descr_w)	?'w':' ',
> (mstr->ME_BIOS_r)	?'r':' ', (mstr->ME_BIOS_w)	?'w':' ',
> (mstr->ME_ME_r)		?'r':' ', (mstr->ME_ME_w)	?'w':' ',
> (mstr->ME_GbE_r)	?'r':' ', (mstr->ME_GbE_w)	?'w':' ',
> (mstr->ME_plat_r)	?'r':' ', (mstr->ME_plat_w)	?'w':' ');
> msg_pdbg2("GbE     %c%c    %c%c  %c%c  %c%c   %c%c\n",
> (mstr->GbE_descr_r)	?'r':' ', (mstr->GbE_descr_w)	?'w':' ',
> (mstr->GbE_BIOS_r)	?'r':' ', (mstr->GbE_BIOS_w)	?'w':' ',
> (mstr->GbE_ME_r)	?'r':' ', (mstr->GbE_ME_w)	?'w':' ',
> (mstr->GbE_GbE_r)	?'r':' ', (mstr->GbE_GbE_w)	?'w':' ',
> (mstr->GbE_plat_r)	?'r':' ', (mstr->GbE_plat_w)	?'w':' ');
> msg_pdbg2("\n");
>
> +}
>
> +void prettyprint_ich9_reg_vscc(uint32_t reg_val)
> +{
> pprint_reg_hex(VSCC, BES, reg_val, ", ");
> pprint_reg(VSCC, WG, reg_val, ", ");
> pprint_reg(VSCC, WSR, reg_val, ", ");
> pprint_reg(VSCC, WEWS, reg_val, ", ");
> pprint_reg_hex(VSCC, EO, reg_val, ", ");
> pprint_reg(VSCC, VCL, reg_val, "\n");
>
> +}
>
> +static uint32_t read_descriptor_reg(uint8_t section, uint16_t offset, void *spibar)
> +{
> uint32_t control = 0;
> control |= (section << FDOC_FDSS_OFF) & FDOC_FDSS;
> control |= (offset << FDOC_FDSI_OFF) & FDOC_FDSI;
> *(volatile uint32_t *) (spibar + ICH9_REG_FDOC) = control;

volatile is absolutely no-go outside hwaccess.c. Use mmio_write[bwl] or
the littleendian variant of mmio_write[bwl] instead.
> return *(volatile uint32_t *)(spibar + ICH9_REG_FDOD);

Same here. Use mmio_read[bwl] instead.
> +}
>
> +void read_ich_descriptors_via_fdo(void *spibar, struct flash_descriptors *desc)
> +{
> msg_pdbg1("Reading flash descriptors "
>  "mapped by the chipset via FDOC/FDOD...");
> /* content section */
> desc->content.FLVALSIG	= read_descriptor_reg(0, 0, spibar);
> desc->content.FLMAP0	= read_descriptor_reg(0, 1, spibar);
> desc->content.FLMAP1	= read_descriptor_reg(0, 2, spibar);
> desc->content.FLMAP2	= read_descriptor_reg(0, 3, spibar);
> /* component section */
> desc->component.FLCOMP	= read_descriptor_reg(1, 0, spibar);
> desc->component.FLILL	= read_descriptor_reg(1, 1, spibar);
> desc->component.FLPB	= read_descriptor_reg(1, 2, spibar);
> /* region section */
> desc->region.FLREGs[0] = read_descriptor_reg(2, 0, spibar);
> desc->region.FLREGs[1] = read_descriptor_reg(2, 1, spibar);
> desc->region.FLREGs[2] = read_descriptor_reg(2, 2, spibar);
> desc->region.FLREGs[3] = read_descriptor_reg(2, 3, spibar);
> desc->region.FLREGs[4] = read_descriptor_reg(2, 4, spibar);
> /* master section */
> desc->master.FLMSTR1 = read_descriptor_reg(3, 0, spibar);
> desc->master.FLMSTR2 = read_descriptor_reg(3, 1, spibar);
> desc->master.FLMSTR3 = read_descriptor_reg(3, 2, spibar);
> /* Accessing the strap section via FDOC/D is only possible on ICH8 and
> * reading the upper map is impossible on all chipsets, so don't bother.
> */
> msg_pdbg1(" done\n");

This seems to be a true flood of output. dbg2 for all callees maybe?
> +}
> +#endif // defined(__i386__) || defined(__x86_64__)
> diff --git a/ich_descriptors.h b/ich_descriptors.h
> new file mode 100644
> index 0000000..f248394
> --- /dev/null
> +++ b/ich_descriptors.h
> @@ -0,0 +1,264 @@
> +/*
> This file is part of the flashrom project.
> Copyright (c) 2010  Matthias Wenzel <bios at mazzoo dot de>
> Copyright (c) 2011  Stefan Tauner
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#if defined(__i386__) || defined(__x86_64__)
> +#ifndef __ICH_DESCRIPTORS_H__
> +#define __ICH_DESCRIPTORS_H__ 1
>
> +#include <stdint.h>
>
> +/* most of the following should probably be in a new file named ichspi.h */
> +#define msg_pdbg1 msg_pdbg
> +#define msg_pdbg2 msg_pspew
>
> +#define ICH9_REG_FDOC		0xB0	/* 32 Bits Flash Descriptor Observability Control */
> 			/* 0-1: reserved */
>
> +#define FDOC_FDSI_OFF		2	/* 2-11: Flash Descriptor Section Index */
> +#define FDOC_FDSI		(0x3f << FDOC_FDSI_OFF)
> +#define FDOC_FDSS_OFF		12	/* 12-14: Flash Descriptor Section Select */
> +#define FDOC_FDSS		(0x3 << FDOC_FDSS_OFF)
> 			/* 15-31: reserved */
>
> +#define ICH9_REG_FDOD		0xB4	/* 32 Bits Flash Descriptor Observability Data */
>
> +/* Field locations and semantics for LVSCC, UVSCC and related words in the flash
> descriptor are equal therefore they all share the same macros below. */
>
> +#define VSCC_BES_OFF		0	/* 0-1: Block/Sector Erase Size */
> +#define VSCC_BES			(0x3 << VSCC_BES_OFF)
> +#define VSCC_WG_OFF		2	/* 2: Write Granularity */
> +#define VSCC_WG				(0x1 << VSCC_WG_OFF)

Whitespace damage or is this caused by the leading + in the patch?
> +#define VSCC_WSR_OFF		3	/* 3: Write Status Required */
> +#define VSCC_WSR			(0x1 << VSCC_WSR_OFF)
> +#define VSCC_WEWS_OFF		4	/* 4: Write Enable on Write Status */
> +#define VSCC_WEWS			(0x1 << VSCC_WEWS_OFF)
> 			/* 5-7: reserved */
>
> +#define VSCC_EO_OFF		8	/* 8-15: Erase Opcode */
> +#define VSCC_EO				(0xff << VSCC_EO_OFF)
> 			/* 16-22: reserved */

Whitespace damage?
> +#define VSCC_VCL_OFF		23	/* 23: Vendor Component Lock */
> +#define VSCC_VCL			(0x1 << VSCC_VCL_OFF)
> 			/* 24-31: reserved */
>
> +#define pprint_reg(reg, bit, val, sep)     msg_pdbg("%s=%d"   sep, #bit, (val & reg##_##bit)>>reg##_##bit##_OFF)
> +#define pprint_reg_hex(reg, bit, val, sep) msg_pdbg("%s=0x%x" sep, #bit, (val & reg##_##bit)>>reg##_##bit##_OFF)

Please add () around val at the right hand side.
> +void prettyprint_ich9_reg_vscc(uint32_t reg_val);
>
> +#define ICH_FREG_BASE(flreg)  ((flreg << 12) & 0x01fff000)
> +#define ICH_FREG_LIMIT(flreg) ((flreg >>  4) & 0x01fff000)

Please add () around flreg at the right hand side.
> +/* Used to select the right descriptor printing function.
> Currently only ICH8 and Ibex Peak are supported.
> */
>
> +enum chipset {

ich_chipset
> CHIPSET_UNKNOWN,

CHIPSET_ICH_UNKNOWN
We may need a CHIPSET_UNKNOWN elsewhere...
> CHIPSET_ICH7 = 7,
> CHIPSET_ICH8,
> CHIPSET_ICH9,
> CHIPSET_ICH10,
> CHIPSET_5_SERIES_IBEX_PEAK,
> CHIPSET_6_SERIES_COUGAR_POINT,
> CHIPSET_7_SERIES_PANTHER_POINT
>
> +};

All structs below probably need __attribute__((packed)).
> +struct flash_content {

ich_flash_content
> uint32_t FLVALSIG;	/* 0x00 */
> union {			/* 0x04 */
> uint32_t FLMAP0;
> struct {
> 	uint32_t FCBA	:8;
> 	uint32_t NC	:2;
> 	uint32_t	:6;
> 	uint32_t FRBA	:8;
> 	uint32_t NR	:3;
> 	uint32_t	:5;
> };
> };
> union {			/* 0x08 */
> uint32_t FLMAP1;
> struct {
> 	uint32_t FMBA	:8;
> 	uint32_t NM	:3;
> 	uint32_t	:5;
> 	uint32_t FISBA	:8;
> 	uint32_t ISL	:8;
> };
> };
> union {			/* 0x0c */
> uint32_t FLMAP2;
> struct {
> 	uint32_t FMSBA	:8;
> 	uint32_t MSL	:8;
> 	uint32_t	:16;
> };
> };
>
> +};
>
> +struct flash_component {

ich_flash_component
> union {			/* 0x00 */
> uint32_t FLCOMP;
> struct {
> 	uint32_t comp1_density	:3;
> 	uint32_t comp2_density	:3;
> 	uint32_t		:11;
> 	uint32_t freq_read	:3;
> 	uint32_t fastread	:1;
> 	uint32_t freq_fastread	:3;
> 	uint32_t freq_write	:3;
> 	uint32_t freq_read_id	:3;
> 	uint32_t		:2;
> };
> };
> union {			/* 0x04 */
> uint32_t FLILL;
> struct {
> 	uint32_t invalid_instr0;

Really uint32_t? If FLILL is 32 bits, but all invalid_instr are 32 bits
as well, the union members have inconsistent sizes.
uint8_t invalid_instr0 maybe?
> 	uint32_t invalid_instr1;
> 	uint32_t invalid_instr2;
> 	uint32_t invalid_instr3;

Dito for the other invalid_instr above.
> };
> };
> union {			/* 0x08 */
> uint32_t FLPB;
> struct {
> 	uint32_t FPBA	:13;
> 	uint32_t	:19;
> };
> };

All code accessing FLPB/FPBA was broken due to the wrong FLILL union
size, so if it worked anyway, you have another bug hidden somewhere.
> +};
>
> +struct flash_region {

ich_flash_region
> union {
> uint32_t FLREGs[5];
> struct {
> 	/* FLREG0 Flash Descriptor */
> 	struct {
> 		uint32_t reg0_base	:13;

Are uint32_t bitfields allowed? I thought bitfields were either signed
int or unsigned int, but never uint32_t.
> 		uint32_t		:3;

Are anonymous bitfields allowed?
> 		uint32_t reg0_limit	:13;
> 		uint32_t		:3;
> 	};
> 	/* FLREG1 BIOS */
> 	struct {
> 		uint32_t reg1_base	:13;
> 		uint32_t		:3;
> 		uint32_t reg1_limit	:13;
> 		uint32_t		:3;
> 	};
> 	/* FLREG2 ME */
> 	struct {
> 		uint32_t reg2_base	:13;
> 		uint32_t		:3;
> 		uint32_t reg2_limit	:13;
> 		uint32_t		:3;
> 	};
> 	/* FLREG3 GbE */
> 	struct {
> 		uint32_t reg3_base	:13;
> 		uint32_t		:3;
> 		uint32_t reg3_limit	:13;
> 		uint32_t		:3;
> 	};
> 	/* FLREG4 Platform */
> 	struct {
> 		uint32_t reg4_base	:13;
> 		uint32_t		:3;
> 		uint32_t reg4_limit	:13;
> 		uint32_t		:3;
> 	};
> };
> };
>
> +};
>
> +struct flash_master {

ich_flash_master
> union {
> uint32_t FLMSTR1;
> struct {
> 	uint32_t BIOS_req_ID	:16;
> 	uint32_t BIOS_descr_r	:1;
> 	uint32_t BIOS_BIOS_r	:1;
> 	uint32_t BIOS_ME_r	:1;
> 	uint32_t BIOS_GbE_r	:1;
> 	uint32_t BIOS_plat_r	:1;
> 	uint32_t		:3;
> 	uint32_t BIOS_descr_w	:1;
> 	uint32_t BIOS_BIOS_w	:1;
> 	uint32_t BIOS_ME_w	:1;
> 	uint32_t BIOS_GbE_w	:1;
> 	uint32_t BIOS_plat_w	:1;
> 	uint32_t		:3;
> };
> };
> union {
> uint32_t FLMSTR2;
> struct {
> 	uint32_t ME_req_ID	:16;
> 	uint32_t ME_descr_r	:1;
> 	uint32_t ME_BIOS_r	:1;
> 	uint32_t ME_ME_r	:1;
> 	uint32_t ME_GbE_r	:1;
> 	uint32_t ME_plat_r	:1;
> 	uint32_t		:3;
> 	uint32_t ME_descr_w	:1;
> 	uint32_t ME_BIOS_w	:1;
> 	uint32_t ME_ME_w	:1;
> 	uint32_t ME_GbE_w	:1;
> 	uint32_t ME_plat_w	:1;
> 	uint32_t		:3;
> };
> };
> union {
> uint32_t FLMSTR3;
> struct {
> 	uint32_t GbE_req_ID	:16;
> 	uint32_t GbE_descr_r	:1;
> 	uint32_t GbE_BIOS_r	:1;
> 	uint32_t GbE_ME_r	:1;
> 	uint32_t GbE_GbE_r	:1;
> 	uint32_t GbE_plat_r	:1;
> 	uint32_t		:3;
> 	uint32_t GbE_descr_w	:1;
> 	uint32_t GbE_BIOS_w	:1;
> 	uint32_t GbE_ME_w	:1;
> 	uint32_t GbE_GbE_w	:1;
> 	uint32_t GbE_plat_w	:1;
> 	uint32_t		:3;
> };
> };
>
> +};
>
> +struct flash_descriptors {

ich_flash_descriptors
> struct flash_content content;

ich_flash_content
> struct flash_component component;

ich_flash_component
> struct flash_region region;

ich_flash_region
> struct flash_master master;

ich_flash_master
> +};
>
> +void prettyprint_ich_descriptors(enum chipset, const struct flash_descriptors *desc);

Missing parameter name for first parameter, and of course
ich_chipset ... ich_flash_descriptors.
> +void prettyprint_ich_descriptor_content(const struct flash_content *content);
> +void prettyprint_ich_descriptor_component(const struct flash_descriptors *desc);
> +void prettyprint_ich_descriptor_region(const struct flash_region *region);
> +void prettyprint_ich_descriptor_master(const struct flash_master *master);

ich_ for all struct names (not for the variable names)
> +void read_ich_descriptors_via_fdo(void *spibar, struct flash_descriptors *desc);

ich_flash_descriptors
> +#endif // __ICH_DESCRIPTORS_H__
> +#endif // defined(__i386__) || defined(__x86_64__)
> diff --git a/ichspi.c b/ichspi.c
> index 2480fa5..b5ee23a 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -30,6 +30,7 @@
>  #include "chipdrivers.h"
>  #include "programmer.h"
>  #include "spi.h"
> +#include "ich_descriptors.h"
>
> /* ICH9 controller register definition */
>  #define ICH9_REG_HSFS		0x04	/* 16 Bits Hardware Sequencing Flash Status */
> @@ -120,6 +121,14 @@
>  #define ICH9_REG_BBAR		0xA0	/* 32 Bits BIOS Base Address Configuration */
>  #define BBAR_MASK	0x00ffff00		/* 8-23: Bottom of System Flash */
>
> +#define ICH9_REG_LVSCC		0xC4	/* 32 Bits Host Lower Vendor Specific Component Capabilities */
> +#define ICH9_REG_UVSCC		0xC8	/* 32 Bits Host Upper Vendor Specific Component Capabilities */
> +/* The individual fields of the VSCC registers are defined in the file
> ich_descriptors.h. The reason is that the same fields are also used in the
> flash descriptors to define the properties of the different flash chips
> supported by the BIOS image. These descriptors are also the source for the
> registers above. */

This comment is a bit unclear to me.
> #define ICH9_REG_FPB		0xD0	/* 32 Bits Flash Partition Boundary */
>  #define FPB_FPBA_OFF		0	/* 0-12: Block/Sector Erase Size */
>  #define FPB_FPBA			(0x1FFF << FPB_FPBA_OFF)
> @@ -302,8 +311,6 @@ static void prettyprint_opcodes(OPCODES *ops)
>    }
>  }
>
> -#define pprint_reg(reg, bit, val, sep) msg_pdbg("%s=%d" sep, #bit, (val & reg##_##bit)>>reg##_##bit##_OFF)
>
> static void prettyprint_ich9_reg_hsfs(uint16_t reg_val)
>  {
>    msg_pdbg("HSFS: ");
> @@ -1130,9 +1137,6 @@ static int ich_spi_send_multicommand(struct spi_command *cmds)
>  #define ICH_BRWA(x)  ((x >>  8) & 0xff)
>  #define ICH_BRRA(x)  ((x >>  0) & 0xff)
>
> -#define ICH_FREG_BASE(x)  ((x >>  0) & 0x1fff)
> -#define ICH_FREG_LIMIT(x) ((x >> 16) & 0x1fff)
>
> static void do_ich9_spi_frap(uint32_t frap, int i)
>  {
>    static const char *const access_names[4] = {
> @@ -1159,9 +1163,8 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
>    	return;
>    }
> msg_pdbg("0x%08x-0x%08x is %s\n",
>     (base << 12), (limit << 12) | 0x0fff,
>     access_names[rwperms]);
> msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
>  access_names[rwperms]);
>
> }
>
> static const struct spi_programmer spi_programmer_ich7 = {
> @@ -1191,6 +1194,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    uint8_t old, new;
>    uint16_t spibar_offset, tmp2;
>    uint32_t tmp;
>
> int ichspi_desc = 0;
>
> switch (ich_generation) {
> case 7:
>
> @@ -1262,6 +1266,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    		msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
>    		ichspi_lock = 1;
>    	}
> if (tmp2 & HSFS_FDV)
> 	ichspi_desc = 1;
>
> tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
> msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
>
> @@ -1312,9 +1318,26 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    	ichspi_bbar = mmio_readl(ich_spibar + ICH9_REG_BBAR);
>    	msg_pdbg("0xA0: 0x%08x (BBAR)\n",
>    		     ichspi_bbar);
> tmp = mmio_readl(ich_spibar + ICH9_REG_LVSCC);
> msg_pdbg("0xC4: 0x%08x (LVSCC)\n", tmp);
> msg_pdbg("LVSCC: ");
> prettyprint_ich9_reg_vscc(tmp);

Does the block above work correctly for all ICH8 and later chipsets?
> tmp = mmio_readl(ich_spibar + ICH9_REG_UVSCC);
> msg_pdbg("0xC8: 0x%08x (UVSCC)\n", tmp);
> msg_pdbg("UVSCC: ");
> prettyprint_ich9_reg_vscc(tmp);

Ditto.
> tmp = mmio_readl(ich_spibar + ICH9_REG_FPB);
>   msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp);
> msg_pdbg("\n");
> if (ichspi_desc) {
> 	struct flash_descriptors desc = {{ 0 }};
> 	read_ich_descriptors_via_fdo(ich_spibar, &desc);

Odd code structure, but I see what you're trying to do. Makes sense.
> 	prettyprint_ich_descriptors(CHIPSET_UNKNOWN, &desc);

Hmmm... I really would like to see this fixed, but it's OK if you want
to do that in a followup patch.
> }
> ich_init_opcodes();
> break;
> default:

Regards,
Carl-Daniel