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

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

[Stefan Tauner]

On Mon, 08 Aug 2011 03:41:28 +0200
Carl-Daniel Hailfinger  wrote:
> 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)?

"regions" here are not the flash regions we are talking about normally
(descriptor, bios, GbE, ME, platform), but the different descriptor
table regions (component, region, master etc). using regions here is
very misleading, sorry.

the whole terminology in the datasheets is not very clear. i'll try
to establish an uniform one that is also understandable for the average
flashrom hacker. in the output i was using section for this kind of
region. that might be a good choice, because it is not used anywhere
else in flashrom (apart from legal code and man page :)
the ibex peak SPI programming guide talks about "descriptor portions"
when talking about those descriptor table regions inside the
description of the FLMAP* registers and uses "xxx section" as headline
for the descriptions of most of them (e.g Flash descriptor component
section).

one could just name them "descriptors" but i am not sure it is a good
decision, because descriptor is used to refer to the whole descriptor
(flash region) i.e. the first 4kB too.

my suggestion is descriptor (table) for the whole thing, sections for
the different portions and regions for the already known flash
"partitions".
> > 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).

yes, chips. seemed to me like a recognizable synonym for us and that it
makes sense to stay in sync with the datasheets in this regard in
general (not in this change log line though).
> > region descriptor
> >   similarly to a partition table this describes the different regions
>
> One common descriptor for all regions or one descriptor per region?

what is a "descriptor" :)

this reads now:
- region section
    similarly to a partition table this describes the different regions.
    the content of FLREG* is derived from this section.

basically the flreg entries are a 1:1 mapping of this section.
> > from a complete binary dump of the descriptor image only.
>
> What's a descriptor image?

changed to:
other information can be retrieved from a complete binary dump of the
descriptor region only.
> > 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.

i would say datasheet.pdf is the right place :)
most of it makes sense when you know the phrase the abbreviation stands
for and this is noted in the header file already.
in the case of register names this is already noted next to the macros,
but i did not comment the abbreviations in the structs... will look
into it and probably do that. atm the only other documentation is the
output of the prettyprinting functions...
> > (which allows to use multiple
> > flash chips or partitions with different properties).

or inline where it is needed like i have done above.
> > 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?

table = section

the registers just represent the currently used/attached flash
chips/properties. the "Intel ME Vendor Specific Component Capabilities
Table" is a section of the flash descriptor where an array with
multiple such entries exist (to support different flash chips with a
single flash image).

i am not sure who populates the registers. could be the ME itself,
could be the vendor bios, could be the hardware (the latter is rather
implausible).

the rephrased paragraph now reads:
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 (which allows to use multiple flash chips or partitions with
different properties). the properties of all supported flash chips
(together with their RDIDs) are stored in the same format in table
in a descriptor section (which is used by the ME apparently). a
later patch will use the macros outside of ichspi.c which is the
reason why the prettyprinting function and the register bit macros
are not defined in ichspi.c but ich_descriptors.h (else they would
be moved in the follow-up patch).
> > 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.

ok, but moving all those (register and bit) macros from ichspi.c to a
header might be a good idea, if only to make ichspi.c smaller because
it is used only there... might be not a good idea also, just wanted to
mention that i am open to discussing this and that it was not discussed
yet.
> > straps of ich8 (only)
>
> Why not ICH7 (no descriptors?) or ICH9 and later (no datasheets?)

soft straps where introduced with ich8. in later versions they are still
in the image, but are not addressable via fdoc (.fdss)
ich8:
000 = Flash Signature and Descriptor Map
001 = Component
010 = Region
011 = Master
100 = ICH8 Soft Straps
111 = Reserved

ich9 (and later):
000 = Flash Signature and Descriptor Map
001 = Component
010 = Region
011 = Master
111 = Reserved

i tried anyway and it seems that they reduced the internal register to
2 bits only, i.e. it wraps from 011 to 000.
> > 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.

yes 3/4. 3/4 adds decoding of the binary dump of the descriptor
region. we need the first 4 kB to do this (the descriptor region can be
bigger, but only because it is aligned to the erase block size, if that
is > 4kB).
> > Signed-off-by: Stefan Tauner
>
> 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.

agreed, i postponed those decisions until... well now i guess :)
it is easy to change this, it is just a matter of deciding how much we
want to print.
> That said, msg_[cpg]dbg2 is now in
> the tree, so the dbg1 hack can go away.

the dbg2 hack is gone, the dbg1 hack is still there, but should
probably be removed because there wont be *dbg1 (soon)?
> Looking again at your
> messagelevels, maybe dbg1->dbg2 and dbg2->spew would be in order.

agreed on 1->2. i am not sure about 2 -> spew though. 
currently dbg1 is the "raw" output of the descriptor sections (the 32b
words of the structs) and dbg2 is the human-readable stuff.

for me spew now is output you want only to see when debugging flashrom
itself, not necessarily the whole system. dbg2 OTOH is information that
could be interesting for board enables and similar stuff. under this
premises i would say that all of them should be dbg2.
> All ich specific structs should have an ich_ prefix in the struct name
> to avoid polluting the global namespace.

ack. as discussed yesterday there is no need to point me to all
occasions of those structs (listing all current variable/struct/type
names that should be changed would help in case of ambiguities though).
> > 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).

that's the nice part about the descriptor sections, that are readable
via fdoc/fdod: they are equal for all versions and versions <ICH8 will
never call this anyway.
the reason why the parameter is there is because it is needed for the
follow-up patch. but that one does not change ichspi.c (or the code
path of flashrom) at all. i could add a wrapper without the parameter
if you like.
> > 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?

see above. the only differences are some of the abbreviations because
of the move from dual-chip chipsets to single-chip chipsets (i.e. ICH
-> PCH). at least i cant remember other differences now and i am quite
sure that i would have made provisions for this in the code already.

(of course intel was not able to do the renaming in a consistent way
either so i just used the old/ich names for the variables in the
structs and added both variants to the prettyprint functions).

the only things that differ are values that were reserved in earlier
chipsets and became useful/defined later (e.g. higher SPI read
frequencies or flash chip sizes). i have just ignored that i think.
the important descriptors for flashrom (i.e. for hwseq) are equal, so
it is only important for the pretty printing afaics.
i dont think it is worth it to complicate the decoding. what do you
think?
> > 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?

see above
> > +{
> > const char * const str_freq[8] = {
>
> static? Or is that useless here?

i have learned in the meantime that it is useful actually.
> > "20 MHz",	/* 000 */
> > "33 MHz",	/* 001 */
> > "reserved",	/* 010 */
> > "reserved",	/* 011 */
> > "50 MHz",	/* 100 */
>
> Mh. Is 50 MHz available on all descriptor capable chipsets?

no, see above.
> > "reserved",	/* 101 */
> > "reserved",	/* 110 */
> > "reserved"	/* 111 */
> > };
> > const char * const str_mem[8] = {
>
> static?

yes
> > "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?

see above (and below :)
> > +{
> > const char *const region_names[5] = {
> > "Descr.", "BIOS", "ME", "GbE", "Platf."
> > };

static ;)
> > 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?

hm hm that's another thing that changed with chipset versions.
the platform region was introduced in ich9 - not present in ich8.
it is not worth it to handle it with the chipset enum, but the right
number of regions could be derived from FLMAP0 (.NR).
will fix.
> > +{
> > 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.

ok. is there any documentation for this kind of stuff? hw access is
pretty much a mystery for me which i would really like to investigate.
> > +}
> >
> > +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?

hm? where is a flood in read_ich_descriptors_via_fdo (and its callees)?
the "Reading flash descriptors " "..." message should remain dbg1
imho btw - especially if we keep the bitfields approach.
> > +}
> > +#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?

looks fine in my editor
> > +#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?

ditto
that's something editors should account for when dealing with patch
files *sigh*
> > +#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.

ack
> > +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.

ack
> > +/* 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)).

it is a bit more complicate then that :/
> > +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?

they were uint8_t before, but should have been uint32_t :8 bitfields.
this is clearly wrong, thanks.
> > 	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.

jup, need to investigate.
> > +};
> >
> > +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.

it is complicated. will mail details later regarding the whole struct
approach.
> > 		uint32_t		:3;
>
> Are anonymous bitfields allowed?

yes. that's at least something that is pretty clear :)
> > 		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,

hm dunno why it was missing, it is already in my version (which i
thought is the version i have posted. maybe i did a compile check only
after mailing... dunno, sorry)
> 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.

will rephrase, until then the explanation/rephrasing of the commit log
above might help. :)
> > #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?

oh. actually no. on the ich8 there is only a single register for this.
need to investigate. on >=ich9 it should work.
> > 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.

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.

i wanted to get rid of calling malloc inside the ich_descriptors.c.
open for suggestions...
> > 	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.

the best "fix" for this is most probably a wrapper function without the
first argument. do you really want that?

follow-up mail regarding structs/bitfields will be sent later (maybe
tomorrow) and will have a more generic subject to lure the c gurus
into discussion ;)

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

[Stefan Tauner]

On Mon, 08 Aug 2011 03:41:28 +0200
Carl-Daniel Hailfinger  wrote:
> > 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?

(quoted for reference only)
> > 	uint32_t invalid_instr1;
> > 	uint32_t invalid_instr2;
> > 	uint32_t invalid_instr3;
>
> Dito for the other invalid_instr above.

(quoted for reference only)
> > };
> > };
> > 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.

actually no. in this patch accesses to FLPB and FPBA are fine because
they all dependent on the (common) starting point of the elements
*inside* the union only.

i set FPBA by
    desc->component.FLPB	= read_descriptor_reg(1, 2, spibar);
and then read out via the bit-fields.
it would be different if i would just memset the whole top-level struct.

in patch 3/4 i add a method that fills said structs using a uint32_t
array as input, but i do so by accessing the inner members directly
like above:
    desc->component.FLPB	= dump[(getFCBA(&desc->content) >> 2) + 2];

on my test machine the invalid instructions are all 0. the whole struct
is initialized to 0, so this was not detected in my tests.