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

[Carl-Daniel Hailfinger]

Am 15.09.2011 00:53 schrieb Stefan Tauner:
> based on the work of Matthias 'mazzoo' Wenzel this patch adds pretty
> printing of those ICH/PCH flash descriptor sections that are
> cached/mapped by the chipset (and which are therefore reachable via
> FDOC/FDOD registers).
>
> this includes the following:
> - content section:
>     describes the image and some generic properties (number of
>     sections, offset of sections, PCH/ICH and MCH/PROC strap
>     offsets and lengths)
> - component section:
>     identify the different SPI flash chips and their capabilities.
> - region section
>     similarly to a partition table this describes the different regions.
>     the content of FLREG* is derived from this section.
> - master section
>     defines SPI master (host, ME, GbE) access rights of the
>     individual regions. the content of PR* is derived from this section.
>
> this is only a part of the data included in the descriptor. 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 (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).
>
> because this patch relies on (compiler) implementation-specific
> layouting of bit-fields, it checks for correct layout before taking
> any action on runtime.
>
> Signed-off-by: Stefan Tauner <[e-mail]>

Get it in!
Acked-by: Carl-Daniel Hailfinger <[e-mail]>
with small comments.
> this is the version without the compile-time checks as requested by carldani.
>
> Makefile          |    3 +-
>  ich_descriptors.c |  286 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ich_descriptors.h |  259 ++++++++++++++++++++++++++++++++++++++++++++++++
>  ichspi.c          |   58 +++++++++--
>  4 files changed, 594 insertions(+), 12 deletions(-)
>  create mode 100644 ich_descriptors.c
>  create mode 100644 ich_descriptors.h
>
> diff --git a/Makefile b/Makefile
> index 6407e6b..4a8dc2c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -349,7 +349,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..4ab2602
> --- /dev/null
> +++ b/ich_descriptors.c
> @@ -0,0 +1,286 @@
> +/*
> + * 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_* */
> +#include "programmer.h"
> +
> +void prettyprint_ich_reg_vscc(uint32_t reg_val, int verbosity)
> +{
> +	print(verbosity, "BES=0x%x, ",	(reg_val & VSCC_BES)  >> VSCC_BES_OFF);
> +	print(verbosity, "WG=%d, ",	(reg_val & VSCC_WG)   >> VSCC_WG_OFF);
> +	print(verbosity, "WSR=%d, ",	(reg_val & VSCC_WSR)  >> VSCC_WSR_OFF);
> +	print(verbosity, "WEWS=%d, ",	(reg_val & VSCC_WEWS) >> VSCC_WEWS_OFF);
> +	print(verbosity, "EO=0x%x, ",	(reg_val & VSCC_EO)   >> VSCC_EO_OFF);
> +	print(verbosity, "VCL=%d\n",	(reg_val & VSCC_VCL)  >> VSCC_VCL_OFF);
> +}
> +
> +#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 ich_chipset cs, const struct ich_descriptors *desc)
> +{
> +	prettyprint_ich_descriptor_content(&desc->content);
> +	prettyprint_ich_descriptor_component(desc);
> +	prettyprint_ich_descriptor_region(desc);
> +	prettyprint_ich_descriptor_master(&desc->master);
> +}
> +
> +void prettyprint_ich_descriptor_content(const struct ich_desc_content *cont)
> +{
> +	msg_pdbg2("=== Content Section ===\n");
> +	msg_pdbg2("FLVALSIG 0x%08x\n", cont->FLVALSIG);
> +	msg_pdbg2("FLMAP0   0x%08x\n", cont->FLMAP0);
> +	msg_pdbg2("FLMAP1   0x%08x\n", cont->FLMAP1);
> +	msg_pdbg2("FLMAP2   0x%08x\n", cont->FLMAP2);
> +	msg_pdbg2("\n");
> +
> +	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 ich_descriptors *desc)
> +{
> +	static const char * const str_freq[8] = {

maybe freq_name or freq_str?
> +		"20 MHz",	/* 000 */
> +		"33 MHz",	/* 001 */
> +		"reserved",	/* 010 */
> +		"reserved",	/* 011 */
> +		"50 MHz",	/* 100 */
> +		"reserved",	/* 101 */
> +		"reserved",	/* 110 */
> +		"reserved"	/* 111 */
> +	};
> +	static const char * const str_mem[8] = {

Maybe size_name or size_str?
> +		"512 kB",	/* 000 */
> +		"  1 MB",	/* 001 */
> +		"  2 MB",	/* 010 */
> +		"  4 MB",	/* 011 */
> +		"  8 MB",	/* 100 */
> +		" 16 MB",	/* 101 */
> +		"reserved",	/* 110 */
> +		"reserved",	/* 111 */
> +	};
> +
> +	msg_pdbg2("=== Component Section ===\n");
> +	msg_pdbg2("FLCOMP   0x%08x\n", desc->component.FLCOMP);
> +	msg_pdbg2("FLILL    0x%08x\n", desc->component.FLILL );
> +	msg_pdbg2("\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 ");

Not something you have to change, but maybe we should store that
information about fast read capability somewhere.
> +	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 ich_desc_region *reg, uint32_t i)
> +{
> +	static const char *const region_names[5] = {
> +		"Descr.", "BIOS", "ME", "GbE", "Platf."
> +	};
> +	if (i >= 5) {
> +		msg_pdbg2("%s: region index too high.\n", __func__);
> +		return;
> +	}
> +	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 ich_descriptors *desc)
> +{
> +	uint8_t i;
> +	uint8_t nr = desc->content.NR + 1;
> +	msg_pdbg2("=== Region Section ===\n");
> +	if (nr >= 5) {
> +		msg_pdbg2("%s: number of regions too high (%d).\n", __func__,
> +			 nr);
> +		return;
> +	}
> +	for (i = 0; i <= nr; i++)
> +		msg_pdbg2("FLREG%d   0x%08x\n", i, desc->region.FLREGs[i]);
> +	msg_pdbg2("\n");
> +
> +	msg_pdbg2("--- Details ---\n");
> +	for (i = 0; i <= nr; i++)
> +		pprint_freg(&desc->region, i);
> +	msg_pdbg2("\n");
> +}
> +
> +void prettyprint_ich_descriptor_master(const struct ich_desc_master *mstr)
> +{
> +	msg_pdbg2("=== Master Section ===\n");
> +	msg_pdbg2("FLMSTR1  0x%08x\n", mstr->FLMSTR1);
> +	msg_pdbg2("FLMSTR2  0x%08x\n", mstr->FLMSTR2);
> +	msg_pdbg2("FLMSTR3  0x%08x\n", mstr->FLMSTR3);
> +	msg_pdbg2("\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");
> +}
> +
> +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;
> +	mmio_le_writel(control, spibar + ICH9_REG_FDOC);
> +	return mmio_le_readl(spibar + ICH9_REG_FDOD);
> +}
> +
> +int read_ich_descriptors_via_fdo(void *spibar, struct ich_descriptors *desc)
> +{
> +	uint8_t i;
> +	uint8_t nr;
> +	struct ich_desc_region *r = &desc->region;
> +
> +	/* Test if bit-fields are working as expected */
> +	for (i = 0; i < 4; i++)
> +		desc->region.FLREGs[i] = 0x5A << (i * 8);
> +	if (r->reg0_base != 0x005A || r->reg0_limit != 0x0000 ||
> +	    r->reg1_base != 0x1A00 || r->reg1_limit != 0x0000 ||
> +	    r->reg2_base != 0x0000 || r->reg2_limit != 0x005A ||
> +	    r->reg3_base != 0x0000 || r->reg3_limit != 0x1A00) {
> +		msg_pdbg("The combination of compiler and CPU architecture used"
> +			 "does not lay out bit-fields as expected, sorry.\n");
> +		msg_pspew("r->reg0_base  = 0x%04X (0x005A)\n", r->reg0_base);
> +		msg_pspew("r->reg0_limit = 0x%04X (0x0000)\n", r->reg0_limit);
> +		msg_pspew("r->reg1_base  = 0x%04X (0x1A00)\n", r->reg1_base);
> +		msg_pspew("r->reg1_limit = 0x%04X (0x0000)\n", r->reg1_limit);
> +		msg_pspew("r->reg2_base  = 0x%04X (0x0000)\n", r->reg2_base);
> +		msg_pspew("r->reg2_limit = 0x%04X (0x005A)\n", r->reg2_limit);
> +		msg_pspew("r->reg3_base  = 0x%04X (0x0000)\n", r->reg3_base);
> +		msg_pspew("r->reg3_limit = 0x%04X (0x1A00)\n", r->reg3_limit);
> +		return RET_ERR;
> +	}

Hm. Having the block above in a separate function would be nice, but if
you add a short
/* FIXME: Replace this with dynamic bitfield fixup */
comment, that would be totally OK for me (and the pending patch queue
would be a lot shorter).
> +
> +	msg_pdbg2("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 */
> +	nr = desc->content.NR + 1;
> +	if (nr >= 5) {
> +		msg_pdbg2("%s: number of regions too high (%d) - failed\n",
> +			  __func__, nr);
> +		return RET_ERR;
> +	}
> +	for (i = 0; i <= nr; i++)
> +		desc->region.FLREGs[i] = read_descriptor_reg(2, i, 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.

Interesting. So Intel removed that capability on ICH9 and later?
> +	 */
> +
> +	msg_pdbg2(" done.\n");
> +	return RET_OK;
> +}
> +#endif /* defined(__i386__) || defined(__x86_64__) */
> diff --git a/ich_descriptors.h b/ich_descriptors.h
> new file mode 100644
> index 0000000..098d3cd
> --- /dev/null
> +++ b/ich_descriptors.h
> @@ -0,0 +1,259 @@
> +/*
> + * 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>
> +
> +#define RET_OK		0
> +#define RET_ERR		-1
> +#define RET_WARN	-2
> +#define RET_PARAM	-3
> +#define RET_OOB		-4

Argl... If you add a comment
/* FIXME: Replace with generic return codes */
that's OK for me now.
> +
> +#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)
> +#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 */
> +#define VSCC_VCL_OFF		23	/* 23: Vendor Component Lock */
> +#define VSCC_VCL			(0x1 << VSCC_VCL_OFF)
> +					/* 24-31: reserved */
> +
> +#define ICH_FREG_BASE(flreg)  (((flreg) << 12) & 0x01fff000)
> +#define ICH_FREG_LIMIT(flreg) (((flreg) >>  4) & 0x01fff000)

Are you sure all users of ICH_FREG_{BASE,LIMIT} have been adjusted to
the new definition, especially in ichspi.c and elsewhere?
> +
> +/* Used to select the right descriptor printing function.
> + * Currently only ICH8 and Ibex Peak are supported.
> + */
> +enum ich_chipset {
> +	CHIPSET_ICH_UNKNOWN,
> +	CHIPSET_ICH7 = 7,
> +	CHIPSET_ICH8,
> +	CHIPSET_ICH9,
> +	CHIPSET_ICH10,
> +	CHIPSET_5_SERIES_IBEX_PEAK,
> +	CHIPSET_6_SERIES_COUGAR_POINT,
> +	CHIPSET_7_SERIES_PANTHER_POINT
> +};
> +
> +void prettyprint_ich_reg_vscc(uint32_t reg_val, int verbosity);
> +
> +struct ich_desc_content {
> +	uint32_t FLVALSIG;	/* 0x00 */
> +	union {			/* 0x04 */
> +		uint32_t FLMAP0;
> +		struct {
> +			uint32_t FCBA	:8, /* Flash Component Base Address */
> +				 NC	:2, /* Number Of Components */
> +					:6,
> +				 FRBA	:8, /* Flash Region Base Address */
> +				 NR	:3, /* Number Of Regions */
> +					:5;
> +		};
> +	};
> +	union {			/* 0x08 */
> +		uint32_t FLMAP1;
> +		struct {
> +			uint32_t FMBA	:8, /* Flash Master Base Address */
> +				 NM	:3, /* Number Of Masters */
> +					:5,
> +				 FISBA	:8, /* Flash ICH Strap Base Address */
> +				 ISL	:8; /* ICH Strap Length */
> +		};
> +	};
> +	union {			/* 0x0c */
> +		uint32_t FLMAP2;
> +		struct {
> +			uint32_t FMSBA	:8, /* Flash (G)MCH Strap Base Addr. */
> +				 MSL	:8, /* MCH Strap Length */
> +					:16;
> +		};
> +	};
> +};
> +
> +struct ich_desc_component {
> +	union {			/* 0x00 */
> +		uint32_t FLCOMP; /* Flash Components Register */
> +		struct {
> +			uint32_t comp1_density	:3,
> +				 comp2_density	:3,
> +						:11,
> +				 freq_read	:3,
> +				 fastread	:1,
> +				 freq_fastread	:3,
> +				 freq_write	:3,
> +				 freq_read_id	:3,
> +						:2;
> +		};
> +	};
> +	union {			/* 0x04 */
> +		uint32_t FLILL; /* Flash Invalid Instructions Register */
> +		struct {
> +			uint32_t invalid_instr0	:8,
> +				 invalid_instr1	:8,
> +				 invalid_instr2	:8,
> +				 invalid_instr3	:8;
> +		};
> +	};
> +	union {			/* 0x08 */
> +		uint32_t FLPB; /* Flash Partition Boundary Register */
> +		struct {
> +			uint32_t FPBA	:13, /* Flash Partition Boundary Addr */
> +					:19;
> +		};
> +	};
> +};
> +
> +struct ich_desc_region {
> +	union {
> +		uint32_t FLREGs[5];
> +		struct {
> +			struct { /* FLREG0 Flash Descriptor */
> +				uint32_t reg0_base	:13,
> +							:3,
> +					 reg0_limit	:13,
> +							:3;
> +			};
> +			struct { /* FLREG1 BIOS */
> +				uint32_t reg1_base	:13,
> +							:3,
> +					 reg1_limit	:13,
> +							:3;
> +			};
> +			struct { /* FLREG2 ME */
> +				uint32_t reg2_base	:13,
> +							:3,
> +					 reg2_limit	:13,
> +							:3;
> +			};
> +			struct { /* FLREG3 GbE */
> +				uint32_t reg3_base	:13,
> +							:3,
> +					 reg3_limit	:13,
> +							:3;
> +			};
> +			struct { /* FLREG4 Platform */
> +				uint32_t reg4_base	:13,
> +							:3,
> +					 reg4_limit	:13,
> +							:3;
> +			};
> +		};
> +	};
> +};
> +
> +struct ich_desc_master {
> +	union {
> +		uint32_t FLMSTR1;
> +		struct {
> +			uint32_t BIOS_req_ID	:16,
> +				 BIOS_descr_r	:1,
> +				 BIOS_BIOS_r	:1,
> +				 BIOS_ME_r	:1,
> +				 BIOS_GbE_r	:1,
> +				 BIOS_plat_r	:1,
> +						:3,
> +				 BIOS_descr_w	:1,
> +				 BIOS_BIOS_w	:1,
> +				 BIOS_ME_w	:1,
> +				 BIOS_GbE_w	:1,
> +				 BIOS_plat_w	:1,
> +						:3;
> +		};
> +	};
> +	union {
> +		uint32_t FLMSTR2;
> +		struct {
> +			uint32_t ME_req_ID	:16,
> +				 ME_descr_r	:1,
> +				 ME_BIOS_r	:1,
> +				 ME_ME_r	:1,
> +				 ME_GbE_r	:1,
> +				 ME_plat_r	:1,
> +						:3,
> +				 ME_descr_w	:1,
> +				 ME_BIOS_w	:1,
> +				 ME_ME_w	:1,
> +				 ME_GbE_w	:1,
> +				 ME_plat_w	:1,
> +						:3;
> +		};
> +	};
> +	union {
> +		uint32_t FLMSTR3;
> +		struct {
> +			uint32_t GbE_req_ID	:16,
> +				 GbE_descr_r	:1,
> +				 GbE_BIOS_r	:1,
> +				 GbE_ME_r	:1,
> +				 GbE_GbE_r	:1,
> +				 GbE_plat_r	:1,
> +						:3,
> +				 GbE_descr_w	:1,
> +				 GbE_BIOS_w	:1,
> +				 GbE_ME_w	:1,
> +				 GbE_GbE_w	:1,
> +				 GbE_plat_w	:1,
> +						:3;
> +		};
> +	};
> +};

I once complained about the size of one of the bitfields in this file,
but I don't remember if you fixed that. Can you verify?
> +
> +struct ich_descriptors {
> +	struct ich_desc_content content;
> +	struct ich_desc_component component;
> +	struct ich_desc_region region;
> +	struct ich_desc_master master;
> +};
> +
> +void prettyprint_ich_descriptors(enum ich_chipset, const struct ich_descriptors *desc);
> +
> +void prettyprint_ich_descriptor_content(const struct ich_desc_content *content);
> +void prettyprint_ich_descriptor_component(const struct ich_descriptors *desc);
> +void prettyprint_ich_descriptor_region(const struct ich_descriptors *desc);
> +void prettyprint_ich_descriptor_master(const struct ich_desc_master *master);
> +
> +int read_ich_descriptors_via_fdo(void *spibar, struct ich_descriptors *desc);
> +
> +#endif /* __ICH_DESCRIPTORS_H__ */
> +#endif /* defined(__i386__) || defined(__x86_64__) */
> diff --git a/ichspi.c b/ichspi.c
> index 8b4210e..ad0b536 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -29,6 +29,7 @@
>  #include "flash.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 */
> @@ -119,6 +120,16 @@
>  #define ICH9_REG_BBAR		0xA0	/* 32 Bits BIOS Base Address Configuration */
>  #define BBAR_MASK	0x00ffff00		/* 8-23: Bottom of System Flash */
>
> +#define ICH8_REG_VSCC		0xC1	/* 32 Bits Vendor Specific Component Capabilities */
> +#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 layout is also used in the
> + * flash descriptor to define the properties of the different flash chips
> + * supported. The BIOS (or the ME?) is responsible to populate the ICH registers
> + * with the information from the descriptor on startup depending on the actual
> + * chip(s) detected. */
> +
>  #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)
> @@ -1129,9 +1140,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] = {
> @@ -1158,9 +1166,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]);

Can you recheck this change? Looks odd.
> }
>
> static const struct spi_programmer spi_programmer_ich7 = {
> @@ -1190,6 +1197,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:
> @@ -1261,6 +1269,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);
> @@ -1308,12 +1318,38 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>       	     mmio_readl(ich_spibar + ICH9_REG_OPMENU));
>       msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n",
>       	     mmio_readl(ich_spibar + ICH9_REG_OPMENU + 4));
> -		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_FPB);
> -		msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp);
> +		if (ich_generation == 8) {
> +			tmp = mmio_readl(ich_spibar + ICH8_REG_VSCC);
> +			msg_pdbg("0xC1: 0x%08x (VSCC)\n", tmp);
> +			msg_pdbg("VSCC: ");
> +			prettyprint_ich_reg_vscc(tmp, MSG_DEBUG);
> +		} else {
> +			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_ich_reg_vscc(tmp, MSG_DEBUG);
> +
> +			tmp = mmio_readl(ich_spibar + ICH9_REG_UVSCC);
> +			msg_pdbg("0xC8: 0x%08x (UVSCC)\n", tmp);
> +			msg_pdbg("UVSCC: ");
> +			prettyprint_ich_reg_vscc(tmp, MSG_DEBUG);
> +
> +			tmp = mmio_readl(ich_spibar + ICH9_REG_FPB);
> +			msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp);
> +		}

Didn't see that flash descriptor override is printed anywhere... you
once described its effect in detail and that description would be really
nice to have in ichspi.c, maybe even with a flag which tells flashrom
about the effect (access control disabled or whatever it was).
> +		msg_pdbg("\n");
> +		if (ichspi_desc) {
> +			struct ich_descriptors desc = {{ 0 }};
> +			if (read_ich_descriptors_via_fdo(ich_spibar, &desc) ==
> +			    RET_OK)
> +				prettyprint_ich_descriptors(CHIPSET_ICH_UNKNOWN,
> +							    &desc);
> +		}
>       ich_init_opcodes();
>       break;
>   default:

Thanks a lot for reworking this patch so often!

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] ichspi: add ICH/PCH flash descriptor decoding via FDOC/FDOD

[Stefan Tauner]

On Thu, 15 Sep 2011 09:07:20 +0200
Carl-Daniel Hailfinger <[e-mail]> wrote:
> Am 15.09.2011 00:53 schrieb Stefan Tauner:
> > based on the work of Matthias 'mazzoo' Wenzel this patch adds pretty
> > printing of those ICH/PCH flash descriptor sections that are
> > cached/mapped by the chipset (and which are therefore reachable via
> > FDOC/FDOD registers).
> >
> > this includes the following:
> > - content section:
> >     describes the image and some generic properties (number of
> >     sections, offset of sections, PCH/ICH and MCH/PROC strap
> >     offsets and lengths)
> > - component section:
> >     identify the different SPI flash chips and their capabilities.
> > - region section
> >     similarly to a partition table this describes the different regions.
> >     the content of FLREG* is derived from this section.
> > - master section
> >     defines SPI master (host, ME, GbE) access rights of the
> >     individual regions. the content of PR* is derived from this section.
> >
> > this is only a part of the data included in the descriptor. 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 (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).
> >
> > because this patch relies on (compiler) implementation-specific
> > layouting of bit-fields, it checks for correct layout before taking
> > any action on runtime.
> >
> > Signed-off-by: Stefan Tauner <[e-mail]>
>
> Get it in!
> Acked-by: Carl-Daniel Hailfinger <[e-mail]>
> with small comments.

thanks!
> > this is the version without the compile-time checks as requested by carldani.
> >
> > Makefile          |    3 +-
> >  ich_descriptors.c |  286 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  ich_descriptors.h |  259 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  ichspi.c          |   58 +++++++++--
> >  4 files changed, 594 insertions(+), 12 deletions(-)
> >  create mode 100644 ich_descriptors.c
> >  create mode 100644 ich_descriptors.h
> >
> > diff --git a/Makefile b/Makefile
> > index 6407e6b..4a8dc2c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -349,7 +349,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..4ab2602
> > --- /dev/null
> > +++ b/ich_descriptors.c
> > @@ -0,0 +1,286 @@
> > +/*
> > + * 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_* */
> > +#include "programmer.h"
> > +
> > +void prettyprint_ich_reg_vscc(uint32_t reg_val, int verbosity)
> > +{
> > +	print(verbosity, "BES=0x%x, ",	(reg_val & VSCC_BES)  >> VSCC_BES_OFF);
> > +	print(verbosity, "WG=%d, ",	(reg_val & VSCC_WG)   >> VSCC_WG_OFF);
> > +	print(verbosity, "WSR=%d, ",	(reg_val & VSCC_WSR)  >> VSCC_WSR_OFF);
> > +	print(verbosity, "WEWS=%d, ",	(reg_val & VSCC_WEWS) >> VSCC_WEWS_OFF);
> > +	print(verbosity, "EO=0x%x, ",	(reg_val & VSCC_EO)   >> VSCC_EO_OFF);
> > +	print(verbosity, "VCL=%d\n",	(reg_val & VSCC_VCL)  >> VSCC_VCL_OFF);
> > +}
> > +
> > +#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 ich_chipset cs, const struct ich_descriptors *desc)
> > +{
> > +	prettyprint_ich_descriptor_content(&desc->content);
> > +	prettyprint_ich_descriptor_component(desc);
> > +	prettyprint_ich_descriptor_region(desc);
> > +	prettyprint_ich_descriptor_master(&desc->master);
> > +}
> > +
> > +void prettyprint_ich_descriptor_content(const struct ich_desc_content *cont)
> > +{
> > +	msg_pdbg2("=== Content Section ===\n");
> > +	msg_pdbg2("FLVALSIG 0x%08x\n", cont->FLVALSIG);
> > +	msg_pdbg2("FLMAP0   0x%08x\n", cont->FLMAP0);
> > +	msg_pdbg2("FLMAP1   0x%08x\n", cont->FLMAP1);
> > +	msg_pdbg2("FLMAP2   0x%08x\n", cont->FLMAP2);
> > +	msg_pdbg2("\n");
> > +
> > +	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 ich_descriptors *desc)
> > +{
> > +	static const char * const str_freq[8] = {
>
> maybe freq_name or freq_str?
> > +		"20 MHz",	/* 000 */
> > +		"33 MHz",	/* 001 */
> > +		"reserved",	/* 010 */
> > +		"reserved",	/* 011 */
> > +		"50 MHz",	/* 100 */
> > +		"reserved",	/* 101 */
> > +		"reserved",	/* 110 */
> > +		"reserved"	/* 111 */
> > +	};
> > +	static const char * const str_mem[8] = {
>
> Maybe size_name or size_str?

freq_str and size_str
> > +		"512 kB",	/* 000 */
> > +		"  1 MB",	/* 001 */
> > +		"  2 MB",	/* 010 */
> > +		"  4 MB",	/* 011 */
> > +		"  8 MB",	/* 100 */
> > +		" 16 MB",	/* 101 */
> > +		"reserved",	/* 110 */
> > +		"reserved",	/* 111 */
> > +	};
> > +
> > +	msg_pdbg2("=== Component Section ===\n");
> > +	msg_pdbg2("FLCOMP   0x%08x\n", desc->component.FLCOMP);
> > +	msg_pdbg2("FLILL    0x%08x\n", desc->component.FLILL );
> > +	msg_pdbg2("\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 ");
>
> Not something you have to change, but maybe we should store that
> information about fast read capability somewhere.

well it is... in the descriptor struct ;)
filling any programmer-related properties field should be done in
ichspi.c directly i guess. this certainly needs to be thought in a
broader scope. ps: i know that you know the latter and just
mentioned it here :)
> > +	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 ich_desc_region *reg, uint32_t i)
> > +{
> > +	static const char *const region_names[5] = {
> > +		"Descr.", "BIOS", "ME", "GbE", "Platf."
> > +	};
> > +	if (i >= 5) {
> > +		msg_pdbg2("%s: region index too high.\n", __func__);
> > +		return;
> > +	}
> > +	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 ich_descriptors *desc)
> > +{
> > +	uint8_t i;
> > +	uint8_t nr = desc->content.NR + 1;
> > +	msg_pdbg2("=== Region Section ===\n");
> > +	if (nr >= 5) {
> > +		msg_pdbg2("%s: number of regions too high (%d).\n", __func__,
> > +			 nr);
> > +		return;
> > +	}
> > +	for (i = 0; i <= nr; i++)
> > +		msg_pdbg2("FLREG%d   0x%08x\n", i, desc->region.FLREGs[i]);
> > +	msg_pdbg2("\n");
> > +
> > +	msg_pdbg2("--- Details ---\n");
> > +	for (i = 0; i <= nr; i++)
> > +		pprint_freg(&desc->region, i);
> > +	msg_pdbg2("\n");
> > +}
> > +
> > +void prettyprint_ich_descriptor_master(const struct ich_desc_master *mstr)
> > +{
> > +	msg_pdbg2("=== Master Section ===\n");
> > +	msg_pdbg2("FLMSTR1  0x%08x\n", mstr->FLMSTR1);
> > +	msg_pdbg2("FLMSTR2  0x%08x\n", mstr->FLMSTR2);
> > +	msg_pdbg2("FLMSTR3  0x%08x\n", mstr->FLMSTR3);
> > +	msg_pdbg2("\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");
> > +}
> > +
> > +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;
> > +	mmio_le_writel(control, spibar + ICH9_REG_FDOC);
> > +	return mmio_le_readl(spibar + ICH9_REG_FDOD);
> > +}
> > +
> > +int read_ich_descriptors_via_fdo(void *spibar, struct ich_descriptors *desc)
> > +{
> > +	uint8_t i;
> > +	uint8_t nr;
> > +	struct ich_desc_region *r = &desc->region;
> > +
> > +	/* Test if bit-fields are working as expected */
> > +	for (i = 0; i < 4; i++)
> > +		desc->region.FLREGs[i] = 0x5A << (i * 8);
> > +	if (r->reg0_base != 0x005A || r->reg0_limit != 0x0000 ||
> > +	    r->reg1_base != 0x1A00 || r->reg1_limit != 0x0000 ||
> > +	    r->reg2_base != 0x0000 || r->reg2_limit != 0x005A ||
> > +	    r->reg3_base != 0x0000 || r->reg3_limit != 0x1A00) {
> > +		msg_pdbg("The combination of compiler and CPU architecture used"
> > +			 "does not lay out bit-fields as expected, sorry.\n");
> > +		msg_pspew("r->reg0_base  = 0x%04X (0x005A)\n", r->reg0_base);
> > +		msg_pspew("r->reg0_limit = 0x%04X (0x0000)\n", r->reg0_limit);
> > +		msg_pspew("r->reg1_base  = 0x%04X (0x1A00)\n", r->reg1_base);
> > +		msg_pspew("r->reg1_limit = 0x%04X (0x0000)\n", r->reg1_limit);
> > +		msg_pspew("r->reg2_base  = 0x%04X (0x0000)\n", r->reg2_base);
> > +		msg_pspew("r->reg2_limit = 0x%04X (0x005A)\n", r->reg2_limit);
> > +		msg_pspew("r->reg3_base  = 0x%04X (0x0000)\n", r->reg3_base);
> > +		msg_pspew("r->reg3_limit = 0x%04X (0x1A00)\n", r->reg3_limit);
> > +		return RET_ERR;
> > +	}
>
> Hm. Having the block above in a separate function would be nice, but if
> you add a short
> /* FIXME: Replace this with dynamic bitfield fixup */
> comment, that would be totally OK for me (and the pending patch queue
> would be a lot shorter).

comment added for now. i'd like to have a look at that automagical
fixing stuff. i'd guess it is not totally trivial to integrate it?
> > +
> > +	msg_pdbg2("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 */
> > +	nr = desc->content.NR + 1;
> > +	if (nr >= 5) {
> > +		msg_pdbg2("%s: number of regions too high (%d) - failed\n",
> > +			  __func__, nr);
> > +		return RET_ERR;
> > +	}
> > +	for (i = 0; i <= nr; i++)
> > +		desc->region.FLREGs[i] = read_descriptor_reg(2, i, 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.
>
> Interesting. So Intel removed that capability on ICH9 and later?

i tried to pass this information to you about a zillion times in commit
messages etc :)
yes, this was removed. just like the description of the straps were
removed from later data sheets. accessing the information from the
raw descriptor dump works of course, but that requires active access to
the flash chip.
> > +	 */
> > +
> > +	msg_pdbg2(" done.\n");
> > +	return RET_OK;
> > +}
> > +#endif /* defined(__i386__) || defined(__x86_64__) */
> > diff --git a/ich_descriptors.h b/ich_descriptors.h
> > new file mode 100644
> > index 0000000..098d3cd
> > --- /dev/null
> > +++ b/ich_descriptors.h
> > @@ -0,0 +1,259 @@
> > +/*
> > + * 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>
> > +
> > +#define RET_OK		0
> > +#define RET_ERR		-1
> > +#define RET_WARN	-2
> > +#define RET_PARAM	-3
> > +#define RET_OOB		-4
>
> Argl... If you add a comment
> /* FIXME: Replace with generic return codes */
> that's OK for me now.

i have also added a ICH_ prefix for now because i have the feeling it
will take a while till we have generic return code macros in the repo ;)
> > +
> > +#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)
> > +#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 */
> > +#define VSCC_VCL_OFF		23	/* 23: Vendor Component Lock */
> > +#define VSCC_VCL			(0x1 << VSCC_VCL_OFF)
> > +					/* 24-31: reserved */
> > +
> > +#define ICH_FREG_BASE(flreg)  (((flreg) << 12) & 0x01fff000)
> > +#define ICH_FREG_LIMIT(flreg) (((flreg) >>  4) & 0x01fff000)
>
> Are you sure all users of ICH_FREG_{BASE,LIMIT} have been adjusted to
> the new definition, especially in ichspi.c and elsewhere?

i have missed this in one iteration of the patch set (due to some
patch reordering), but i hope i have not repeated that. will check
before committing.
> > +
> > +/* Used to select the right descriptor printing function.
> > + * Currently only ICH8 and Ibex Peak are supported.
> > + */
> > +enum ich_chipset {
> > +	CHIPSET_ICH_UNKNOWN,
> > +	CHIPSET_ICH7 = 7,
> > +	CHIPSET_ICH8,
> > +	CHIPSET_ICH9,
> > +	CHIPSET_ICH10,
> > +	CHIPSET_5_SERIES_IBEX_PEAK,
> > +	CHIPSET_6_SERIES_COUGAR_POINT,
> > +	CHIPSET_7_SERIES_PANTHER_POINT
> > +};
> > +
> > +void prettyprint_ich_reg_vscc(uint32_t reg_val, int verbosity);
> > +
> > +struct ich_desc_content {
> > +	uint32_t FLVALSIG;	/* 0x00 */
> > +	union {			/* 0x04 */
> > +		uint32_t FLMAP0;
> > +		struct {
> > +			uint32_t FCBA	:8, /* Flash Component Base Address */
> > +				 NC	:2, /* Number Of Components */
> > +					:6,
> > +				 FRBA	:8, /* Flash Region Base Address */
> > +				 NR	:3, /* Number Of Regions */
> > +					:5;
> > +		};
> > +	};
> > +	union {			/* 0x08 */
> > +		uint32_t FLMAP1;
> > +		struct {
> > +			uint32_t FMBA	:8, /* Flash Master Base Address */
> > +				 NM	:3, /* Number Of Masters */
> > +					:5,
> > +				 FISBA	:8, /* Flash ICH Strap Base Address */
> > +				 ISL	:8; /* ICH Strap Length */
> > +		};
> > +	};
> > +	union {			/* 0x0c */
> > +		uint32_t FLMAP2;
> > +		struct {
> > +			uint32_t FMSBA	:8, /* Flash (G)MCH Strap Base Addr. */
> > +				 MSL	:8, /* MCH Strap Length */
> > +					:16;
> > +		};
> > +	};
> > +};
> > +
> > +struct ich_desc_component {
> > +	union {			/* 0x00 */
> > +		uint32_t FLCOMP; /* Flash Components Register */
> > +		struct {
> > +			uint32_t comp1_density	:3,
> > +				 comp2_density	:3,
> > +						:11,
> > +				 freq_read	:3,
> > +				 fastread	:1,
> > +				 freq_fastread	:3,
> > +				 freq_write	:3,
> > +				 freq_read_id	:3,
> > +						:2;
> > +		};
> > +	};
> > +	union {			/* 0x04 */
> > +		uint32_t FLILL; /* Flash Invalid Instructions Register */
> > +		struct {
> > +			uint32_t invalid_instr0	:8,
> > +				 invalid_instr1	:8,
> > +				 invalid_instr2	:8,
> > +				 invalid_instr3	:8;
> > +		};
> > +	};
> > […]
>
> I once complained about the size of one of the bitfields in this file,
> but I don't remember if you fixed that. Can you verify?

the invalid instructions (mapping to FLILL) iirc.
> > +
> > +struct ich_descriptors {
> > +	struct ich_desc_content content;
> > +	struct ich_desc_component component;
> > +	struct ich_desc_region region;
> > +	struct ich_desc_master master;
> > +};
> > +
> > +void prettyprint_ich_descriptors(enum ich_chipset, const struct ich_descriptors *desc);
> > +
> > +void prettyprint_ich_descriptor_content(const struct ich_desc_content *content);
> > +void prettyprint_ich_descriptor_component(const struct ich_descriptors *desc);
> > +void prettyprint_ich_descriptor_region(const struct ich_descriptors *desc);
> > +void prettyprint_ich_descriptor_master(const struct ich_desc_master *master);
> > +
> > +int read_ich_descriptors_via_fdo(void *spibar, struct ich_descriptors *desc);
> > +
> > +#endif /* __ICH_DESCRIPTORS_H__ */
> > +#endif /* defined(__i386__) || defined(__x86_64__) */
> > diff --git a/ichspi.c b/ichspi.c
> > index 8b4210e..ad0b536 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -29,6 +29,7 @@
> >  #include "flash.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 */
> > @@ -119,6 +120,16 @@
> >  #define ICH9_REG_BBAR		0xA0	/* 32 Bits BIOS Base Address Configuration */
> >  #define BBAR_MASK	0x00ffff00		/* 8-23: Bottom of System Flash */
> >
> > +#define ICH8_REG_VSCC		0xC1	/* 32 Bits Vendor Specific Component Capabilities */
> > +#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 layout is also used in the
> > + * flash descriptor to define the properties of the different flash chips
> > + * supported. The BIOS (or the ME?) is responsible to populate the ICH registers
> > + * with the information from the descriptor on startup depending on the actual
> > + * chip(s) detected. */
> > +
> >  #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)
> > @@ -1129,9 +1140,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] = {
> > @@ -1158,9 +1166,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]);
>
> Can you recheck this change? Looks odd.

looks odd because i have changed the ICH_FREG macros for the better:
> > +#define ICH_FREG_BASE(flreg)  (((flreg) << 12) & 0x01fff000)
> > +#define ICH_FREG_LIMIT(flreg) (((flreg) >>  4) & 0x01fff000)

they now return the complete address and not only the (shifted) part
that is saved in the register.
> > }
> >
> > static const struct spi_programmer spi_programmer_ich7 = {
> > @@ -1190,6 +1197,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:
> > @@ -1261,6 +1269,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);
> > @@ -1308,12 +1318,38 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >       	     mmio_readl(ich_spibar + ICH9_REG_OPMENU));
> >       msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n",
> >       	     mmio_readl(ich_spibar + ICH9_REG_OPMENU + 4));
> > -		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_FPB);
> > -		msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp);
> > +		if (ich_generation == 8) {
> > +			tmp = mmio_readl(ich_spibar + ICH8_REG_VSCC);
> > +			msg_pdbg("0xC1: 0x%08x (VSCC)\n", tmp);
> > +			msg_pdbg("VSCC: ");
> > +			prettyprint_ich_reg_vscc(tmp, MSG_DEBUG);
> > +		} else {
> > +			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_ich_reg_vscc(tmp, MSG_DEBUG);
> > +
> > +			tmp = mmio_readl(ich_spibar + ICH9_REG_UVSCC);
> > +			msg_pdbg("0xC8: 0x%08x (UVSCC)\n", tmp);
> > +			msg_pdbg("UVSCC: ");
> > +			prettyprint_ich_reg_vscc(tmp, MSG_DEBUG);
> > +
> > +			tmp = mmio_readl(ich_spibar + ICH9_REG_FPB);
> > +			msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp);
> > +		}
>
> Didn't see that flash descriptor override is printed anywhere... you
> once described its effect in detail and that description would be really
> nice to have in ichspi.c, maybe even with a flag which tells flashrom
> about the effect (access control disabled or whatever it was).

yes, will put that on my todo list for later.
> > +		msg_pdbg("\n");
> > +		if (ichspi_desc) {
> > +			struct ich_descriptors desc = {{ 0 }};
> > +			if (read_ich_descriptors_via_fdo(ich_spibar, &desc) ==
> > +			    RET_OK)
> > +				prettyprint_ich_descriptors(CHIPSET_ICH_UNKNOWN,
> > +							    &desc);
> > +		}
> >       ich_init_opcodes();
> >       break;
> >   default:
>
> Thanks a lot for reworking this patch so often!

constant dripping wears away the stone. ;)

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner

[PATCH] ichspi: add ICH/PCH flash descriptor decoding via FDOC/FDOD

[Stefan Tauner]

finally committed in r1443 \o/

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner