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

[Carl-Daniel Hailfinger]

Am 20.08.2011 12:39 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 basic compiler support in the
> makefile and for correct layout before taking any action on runtime.

Checking for presence of bitfield support in the compiler is the way to
madness. Next we have to test for C99 capability and other fun stuff.
Just assume that bitfields are supported, otherwise compilation will
blow up or the runtime checks will catch a miscompilation. That
compile-time check (including the #if BITFIELDS) is nacked.
> Signed-off-by: Stefan Tauner
>
> Makefile          |   25 +++++-
>  ich_descriptors.c |  288 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ich_descriptors.h |  261 ++++++++++++++++++++++++++++++++++++++++++++++++
>  ichspi.c          |   60 +++++++++--
>  4 files changed, 622 insertions(+), 12 deletions(-)
>  create mode 100644 ich_descriptors.c
>  create mode 100644 ich_descriptors.h
>
> diff --git a/Makefile b/Makefile
> index 26f978f..247a60c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -340,9 +340,11 @@ endif
>
> ifeq ($(CONFIG_INTERNAL), yes)
>  FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
> +FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "BITFIELDS := yes" .features && printf "%s" "-D'BITFIELDS=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
> @@ -635,6 +637,19 @@ int main(int argc, char **argv)
>  endef
>  export UTSNAME_TEST
>
> +define BITFIELDS_TEST
> +#include "ich_descriptors.h"
> +int main(int argc, char **argv)
> +{
> struct ich_descriptors desc;
> (void) argc;
> (void) argv;
> desc.region.FLREGs[4] = 0x5aa5a55a;
> return desc.region.reg4_base;
>
> +}
> +endef
> +export BITFIELDS_TEST
>
> features: compiler
>    @echo "FEATURES := yes" > .features.tmp
>  ifeq ($(CONFIG_FT2232_SPI), yes)
> @@ -649,6 +664,14 @@ endif
>    @$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) >/dev/null 2>&1 &&	\
>    	( echo "found."; echo "UTSNAME := yes" >> .features.tmp ) ||	\
>    	( echo "not found."; echo "UTSNAME := no" >> .features.tmp )
> +# ugly hack to get a logical AND *sigh*
> +ifeq ($(CONFIG_INTERNAL)$(ARCH), yes"x86")
> @printf "Checking if bit-fields support is sufficient... "
> @echo "$$BITFIELDS_TEST" > .featuretest.c
> @$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -D BITFIELDS .featuretest.c -o .featuretest$(EXEC_SUFFIX) >/dev/null 2>&1 &&	\
> ( echo "yes."; echo "BITFIELDS := yes" >> .features.tmp ) ||	\
> ( echo "no."; echo "BITFIELDS := no" >> .features.tmp )
>
> +endif
>    @$(DIFF) -q .features.tmp .features >/dev/null 2>&1 && rm .features.tmp || mv .features.tmp .features
>    @rm -f .featuretest.c .featuretest$(EXEC_SUFFIX)

That Makefile change needs to be dropped except for the PROGRAMMER_OBJS
addition.
> diff --git a/ich_descriptors.c b/ich_descriptors.c
> new file mode 100644
> index 0000000..ce40d9f
> --- /dev/null
> +++ b/ich_descriptors.c
> @@ -0,0 +1,288 @@
> [...]
> +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;

The snippet below needs to be in its own function named
selfcheck_bitfields(), and that function should live in flashrom.c (or
any other reasonably central place) and be called in the initial
selfcheck() function. We want to know where this explodes, even if the
functionality would not be necessary.
> /* 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;
> }

Snippet ends here.
If you want, I can send my own bitfield checker code to the list so you
can compare both implementations.
> diff --git a/ich_descriptors.h b/ich_descriptors.h
> new file mode 100644
> index 0000000..b1da914
> --- /dev/null
> +++ b/ich_descriptors.h
> @@ -0,0 +1,261 @@
> +/*
> 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

Too generic names.
> +#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)
>
> +/* 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);
>
> +#if BITFIELDS == 1
> +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;
> };
> };
>
> +};
>
> +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 /* BITFIELDS == 1 */
> +#endif /* __ICH_DESCRIPTORS_H__ */
> +#endif /* defined(__i386__) || defined(__x86_64__) */
> diff --git a/ichspi.c b/ichspi.c
> index 8b4210e..09af2b3 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -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;

How does FDOVR (or whatever the correct acronym for Flash Descriptor
Override is) relate to this?
> tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
> msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
>
> @@ -1308,12 +1318,40 @@ 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);
> }
>
> msg_pdbg("\n");
>
> +#if BITFIELDS == 1
> 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);
> }
>
> +#endif /* BITFIELDS == 1 */
>    	ich_init_opcodes();
>    	break;
>    default:

Regards,
Carl-Daniel

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

[Stefan Tauner]

On Fri, 02 Sep 2011 21:33:33 +0200
Carl-Daniel Hailfinger  wrote:
> Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > because this patch relies on (compiler) implementation-specific
> > layouting of bit-fields, it checks for basic compiler support in the
> > makefile and for correct layout before taking any action on runtime.
>
> Checking for presence of bitfield support in the compiler is the way to
> madness. Next we have to test for C99 capability and other fun stuff.
> Just assume that bitfields are supported, otherwise compilation will
> blow up or the runtime checks will catch a miscompilation. That
> compile-time check (including the #if BITFIELDS) is nacked.

well, for now bit-fields are only needed in one place that is seldom
needed. why break compilation unnecessarily for all other cases too?
basically i agree, but since everyone thinks bit-fields break all the
time i thought it is a good idea to integrate it in the least
interfering way possible... :)
> […]
> > +#define RET_OK		0
> > +#define RET_ERR		-1
> > +#define RET_WARN	-2
> > +#define RET_PARAM	-3
> > +#define RET_OOB		-4
>
> Too generic names.

not at all, but they should be moved to wherever we want the generic
return macros to be eventually ;)
can we do that and/or introduce the universal values before/with merge
of this patch?
some additional FL_ prefix or so would be ok with me.
i could of course rename them now, and then again, but we should add
the universal return values soon-ish anyway.
> […]
> > diff --git a/ichspi.c b/ichspi.c
> > index 8b4210e..09af2b3 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -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;
>
> How does FDOVR (or whatever the correct acronym for Flash Descriptor
> Override is) relate to this?

Flash Descriptor Override Pin-Strap Status (FDOPSS).
it is later called flash descriptor *security* override pin, and this
hints at its true purpose: overriding the region access rights declared
in the descriptor.

quote ich8 datasheet description of the respective pin (GPIO33):
"If sampled low, the Flash Descriptor Security will be overridden. If
high, the security measures defined in the Flash Descriptor will be in effect."

even then the whole descriptor is of interest imo... and this will most
likely never occur in practice anyway.

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

[Carl-Daniel Hailfinger]

Am 03.09.2011 12:33 schrieb Stefan Tauner:
> On Fri, 02 Sep 2011 21:33:33 +0200
> Carl-Daniel Hailfinger  wrote
> > Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > > because this patch relies on (compiler) implementation-specific
> > > layouting of bit-fields, it checks for basic compiler support in the
> > > makefile and for correct layout before taking any action on runtime.
> >
> > Checking for presence of bitfield support in the compiler is the way to
> > madness. Next we have to test for C99 capability and other fun stuff.
> > Just assume that bitfields are supported, otherwise compilation will
> > blow up or the runtime checks will catch a miscompilation. That
> > compile-time check (including the #if BITFIELDS) is nacked.
>
> well, for now bit-fields are only needed in one place that is seldom
> needed. why break compilation unnecessarily for all other cases too?

Would it really break compilation? AFAICS not.
Bitfield support is available at least in all recent gcc and clang, and
probably other compilers as well. No need to test for that at compile time.
> basically i agree, but since everyone thinks bit-fields break all the
> time i thought it is a good idea to integrate it in the least
> interfering way possible... :)

Bitfields have various implementation-defined aspects which may explode
in your face unless you test for them at runtime, but it is technically
impossible to test those at compile time.
> > […]
> > > +#define RET_OK		0
> > > +#define RET_ERR		-1
> > > +#define RET_WARN	-2
> > > +#define RET_PARAM	-3
> > > +#define RET_OOB		-4
> >
> > Too generic names.
>
> not at all, but they should be moved to wherever we want the generic
> return macros to be eventually ;)
> can we do that and/or introduce the universal values before/with merge
> of this patch?
> some additional FL_ prefix or so would be ok with me.
> i could of course rename them now, and then again, but we should add
> the universal return values soon-ish anyway.

You mean like the various (and partially unmerged) patches which added
definitions like ERROR_PTR and TIMEOUT_ERROR? Much more important than
the name is deciding on which errors we want to handle as separate
return codes and whether a warning should indeed be a return code
different from OK.
> > […]
> > > diff --git a/ichspi.c b/ichspi.c
> > > index 8b4210e..09af2b3 100644
> > > --- a/ichspi.c
> > > +++ b/ichspi.c
> > > @@ -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;
> >
> > How does FDOVR (or whatever the correct acronym for Flash Descriptor
> > Override is) relate to this?
>
> Flash Descriptor Override Pin-Strap Status (FDOPSS).
> it is later called flash descriptor *security* override pin, and this
> hints at its true purpose: overriding the region access rights declared
> in the descriptor.
>
> quote ich8 datasheet description of the respective pin (GPIO33):
> "If sampled low, the Flash Descriptor Security will be overridden. If
> high, the security measures defined in the Flash Descriptor will be in effect."

Excellent, this clears up one FIXME in the ICH code.
> even then the whole descriptor is of interest imo... and this will most
> likely never occur in practice anyway.

A possible use case is allowing reflashing of a dev board with an
additional jumper.

Regards,
Carl-Daniel

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

[Stefan Tauner]

On Sat, 03 Sep 2011 14:46:00 +0200
Carl-Daniel Hailfinger  wrote:
> Am 03.09.2011 12:33 schrieb Stefan Tauner:
> > On Fri, 02 Sep 2011 21:33:33 +0200
> > Carl-Daniel Hailfinger  wrote
> > > Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > > > because this patch relies on (compiler) implementation-specific
> > > > layouting of bit-fields, it checks for basic compiler support in the
> > > > makefile and for correct layout before taking any action on runtime.
> > >
> > > Checking for presence of bitfield support in the compiler is the way to
> > > madness. Next we have to test for C99 capability and other fun stuff.
> > > Just assume that bitfields are supported, otherwise compilation will
> > > blow up or the runtime checks will catch a miscompilation. That
> > > compile-time check (including the #if BITFIELDS) is nacked.
> >
> > well, for now bit-fields are only needed in one place that is seldom
> > needed. why break compilation unnecessarily for all other cases too?
>
> Would it really break compilation? AFAICS not.

it could due to:
"A bit-field shall have a type that is a qualified or unqualified
version of _Bool, signed int, unsigned int, or some other
implementation-defined type."

maybe i don't understand the "qualified" part of this definition
(i can not remember what this means in the specs right now), but i
would think that uint32_t is not guaranteed to work as a bit-field type?
> Bitfield support is available at least in all recent gcc and clang, and
> probably other compilers as well.

under the premise that all used compilers can deal with bit-fields the
way we want, even the run time test is unnecessary ;)
> No need to test for that at compile time.

well it could be tested when host=target arch, but sure this is not the
way to go (and i did not go there).
> > basically i agree, but since everyone thinks bit-fields break all the
> > time i thought it is a good idea to integrate it in the least
> > interfering way possible... :)
>
> Bitfields have various implementation-defined aspects which may explode
> in your face unless you test for them at runtime, but it is technically
> impossible to test those at compile time.

if it would not possibly break compilation, i would totally agree with
you. i am not insisting on keeping the test + ifdefs, but you have been
warned now ;)
please confirm what i should do.
> > > […]
> > > > +#define RET_OK		0
> > > > +#define RET_ERR		-1
> > > > +#define RET_WARN	-2
> > > > +#define RET_PARAM	-3
> > > > +#define RET_OOB		-4
> > >
> > > Too generic names.
> >
> > not at all, but they should be moved to wherever we want the generic
> > return macros to be eventually ;)
> > can we do that and/or introduce the universal values before/with merge
> > of this patch?
> > some additional FL_ prefix or so would be ok with me.
> > i could of course rename them now, and then again, but we should add
> > the universal return values soon-ish anyway.
>
> You mean like the various (and partially unmerged) patches which added
> definitions like ERROR_PTR and TIMEOUT_ERROR? Much more important than
> the name is deciding on which errors we want to handle as separate
> return codes and whether a warning should indeed be a return code
> different from OK.

error_ptr was a macro to indicate errors when NULL is a valid response
iirc... but timeout_error and the fatal thingy tadas/uwe introduced
today are such things yes.
of course the naming is not the most important point to
discuss, but also discussing the issues you named needs to be started
at some point... :)

if not you will continue to see randomly named error macro definitions
in patches with funny values... forever!
> […]
> > > How does FDOVR (or whatever the correct acronym for Flash Descriptor
> > > Override is) relate to this?
> >
> > Flash Descriptor Override Pin-Strap Status (FDOPSS).
> > it is later called flash descriptor *security* override pin, and this
> > hints at its true purpose: overriding the region access rights declared
> > in the descriptor.
> >
> > quote ich8 datasheet description of the respective pin (GPIO33):
> > "If sampled low, the Flash Descriptor Security will be overridden. If
> > high, the security measures defined in the Flash Descriptor will be in effect."
>
> Excellent, this clears up one FIXME in the ICH code.

mhm nomnomnom, i like FIXMEs!</coreboot-status>
> > even then the whole descriptor is of interest imo... and this will most
> > likely never occur in practice anyway.
>
> A possible use case is allowing reflashing of a dev board with an
> additional jumper.

sure, but even then (or maybe then even more) the user would like to
know the contents of the descriptor. the flag does indicate that the
descriptor is valid only, nothing else. i don't see a problem.

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

[Carl-Daniel Hailfinger]

Am 03.09.2011 23:43 schrieb Stefan Tauner:
> On Sat, 03 Sep 2011 14:46:00 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 03.09.2011 12:33 schrieb Stefan Tauner:
> > > On Fri, 02 Sep 2011 21:33:33 +0200
> > > Carl-Daniel Hailfinger  wrote
> > > > Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > > > > because this patch relies on (compiler) implementation-specific
> > > > > layouting of bit-fields, it checks for basic compiler support in the
> > > > > makefile and for correct layout before taking any action on runtime.
> > > >
> > > > Checking for presence of bitfield support in the compiler is the way to
> > > > madness. Next we have to test for C99 capability and other fun stuff.
> > > > Just assume that bitfields are supported, otherwise compilation will
> > > > blow up or the runtime checks will catch a miscompilation. That
> > > > compile-time check (including the #if BITFIELDS) is nacked.
> > >
> > > well, for now bit-fields are only needed in one place that is seldom
> > > needed. why break compilation unnecessarily for all other cases too?
> >
> > Would it really break compilation? AFAICS not.
>
> it could due to:
> "A bit-field shall have a type that is a qualified or unqualified
> version of _Bool, signed int, unsigned int, or some other
> implementation-defined type."
>
> maybe i don't understand the "qualified" part of this definition
> (i can not remember what this means in the specs right now), but i
> would think that uint32_t is not guaranteed to work as a bit-field type?

I thought that refers to the bitfield members, not the whole bitfield.
We have to ask a C99 expert about that.
> > Bitfield support is available at least in all recent gcc and clang, and
> > probably other compilers as well.
>
> under the premise that all used compilers can deal with bit-fields the
> way we want, even the run time test is unnecessary ;)

You have to differentiate between the functionality mandated by C99 (I
assume that as a given) and the functionality labeled as implementation
defined by C99 (needs to be tested at runtime).
> > No need to test for that at compile time.
>
> well it could be tested when host=target arch, but sure this is not the
> way to go (and i did not go there).

Indeed, this would screw up cross compilation.
> > > basically i agree, but since everyone thinks bit-fields break all the
> > > time i thought it is a good idea to integrate it in the least
> > > interfering way possible... :)
> >
> > Bitfields have various implementation-defined aspects which may explode
> > in your face unless you test for them at runtime, but it is technically
> > impossible to test those at compile time.
>
> if it would not possibly break compilation, i would totally agree with
> you. i am not insisting on keeping the test + ifdefs, but you have been
> warned now ;)
> please confirm what i should do.

If bitfields ever break compilation, we can handle the failure with some
compiler dependent #ifdefs. Until then I'd rather avoid conditional code.
Could you comment on my "bitfield checker" mail? I know that endianness
should not be complained about and rather be automatically fixed, but at
least the code in there hopefully detects all possible bitfield layout
variants allowed by C99.

Semi-related question: Can we compile flashrom with gcc 2.95 (may break
due to anonymous unions)? What about gcc 3.0/3.3?
> > > > […]
> > > > > +#define RET_OK		0
> > > > > +#define RET_ERR		-1
> > > > > +#define RET_WARN	-2
> > > > > +#define RET_PARAM	-3
> > > > > +#define RET_OOB		-4
> > > >
> > > > Too generic names.
> > >
> > > not at all, but they should be moved to wherever we want the generic
> > > return macros to be eventually ;)
> > > can we do that and/or introduce the universal values before/with merge
> > > of this patch?
> > > some additional FL_ prefix or so would be ok with me.
> > > i could of course rename them now, and then again, but we should add
> > > the universal return values soon-ish anyway.
> >
> > You mean like the various (and partially unmerged) patches which added
> > definitions like ERROR_PTR and TIMEOUT_ERROR? Much more important than
> > the name is deciding on which errors we want to handle as separate
> > return codes and whether a warning should indeed be a return code
> > different from OK.
>
> error_ptr was a macro to indicate errors when NULL is a valid response
> iirc... but timeout_error and the fatal thingy tadas/uwe introduced
> today are such things yes.
> of course the naming is not the most important point to
> discuss, but also discussing the issues you named needs to be started
> at some point... :)
>
> if not you will continue to see randomly named error macro definitions
> in patches with funny values... forever!

Can you start a separate discussion thread about error numbers? I see
that Uwe has started assigning unique return codes to various ft2232_spi
errors, and I was not really happy with those.
> > > > How does FDOVR (or whatever the correct acronym for Flash Descriptor
> > > > Override is) relate to this?
> > >
> > > Flash Descriptor Override Pin-Strap Status (FDOPSS).
> > > it is later called flash descriptor *security* override pin, and this
> > > hints at its true purpose: overriding the region access rights declared
> > > in the descriptor.
> > >
> > > quote ich8 datasheet description of the respective pin (GPIO33):
> > > "If sampled low, the Flash Descriptor Security will be overridden. If
> > > high, the security measures defined in the Flash Descriptor will be in effect."
> >
> > Excellent, this clears up one FIXME in the ICH code.
>
> mhm nomnomnom, i like FIXMEs!</coreboot-status>
> > > even then the whole descriptor is of interest imo... and this will most
> > > likely never occur in practice anyway.
> >
> > A possible use case is allowing reflashing of a dev board with an
> > additional jumper.
>
> sure, but even then (or maybe then even more) the user would like to
> know the contents of the descriptor. the flag does indicate that the
> descriptor is valid only, nothing else. i don't see a problem.

Agreed. My comment was about the "likely never occur" aspect of your
statement. We might want to handle FDOPSS by disregarding region
protection registers, but I don't know if the documentation matches the
hardware.

Regards,
Carl-Daniel

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

[Stefan Tauner]

On Sun, 04 Sep 2011 02:25:59 +0200
Carl-Daniel Hailfinger  wrote:
> Am 03.09.2011 23:43 schrieb Stefan Tauner:
> > On Sat, 03 Sep 2011 14:46:00 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 03.09.2011 12:33 schrieb Stefan Tauner:
> > > > On Fri, 02 Sep 2011 21:33:33 +0200
> > > > Carl-Daniel Hailfinger  wrote
> > > > > Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > > > > > because this patch relies on (compiler) implementation-specific
> > > > > > layouting of bit-fields, it checks for basic compiler support in the
> > > > > > makefile and for correct layout before taking any action on runtime.
> > > > >
> > > > > Checking for presence of bitfield support in the compiler is the way to
> > > > > madness. Next we have to test for C99 capability and other fun stuff.
> > > > > Just assume that bitfields are supported, otherwise compilation will
> > > > > blow up or the runtime checks will catch a miscompilation. That
> > > > > compile-time check (including the #if BITFIELDS) is nacked.
> > > >
> > > > well, for now bit-fields are only needed in one place that is seldom
> > > > needed. why break compilation unnecessarily for all other cases too?
> > >
> > > Would it really break compilation? AFAICS not.
> >
> > it could due to:
> > "A bit-field shall have a type that is a qualified or unqualified
> > version of _Bool, signed int, unsigned int, or some other
> > implementation-defined type."
> >
> > maybe i don't understand the "qualified" part of this definition
> > (i can not remember what this means in the specs right now), but i
> > would think that uint32_t is not guaranteed to work as a bit-field type?
>
> I thought that refers to the bitfield members, not the whole bitfield.
> We have to ask a C99 expert about that.

*shrug* can you please persuade michael to comment on this? :)
> > > Bitfield support is available at least in all recent gcc and clang, and
> > > probably other compilers as well.
> >
> > under the premise that all used compilers can deal with bit-fields the
> > way we want, even the run time test is unnecessary ;)
>
> You have to differentiate between the functionality mandated by C99 (I
> assume that as a given) and the functionality labeled as implementation
> defined by C99 (needs to be tested at runtime).

the question is if the implementation-defined functionality is two-fold
and does not only create implementation-defined run time behavior
but also have an impact on successful compiler termination. i was under
the presumption that both can be a problem for us and handled them
accordingly. there is no reason to discuss this approach further until
we know if that premise is correct or false. :)
> > > > basically i agree, but since everyone thinks bit-fields break all the
> > > > time i thought it is a good idea to integrate it in the least
> > > > interfering way possible... :)
> > >
> > > Bitfields have various implementation-defined aspects which may explode
> > > in your face unless you test for them at runtime, but it is technically
> > > impossible to test those at compile time.
> >
> > if it would not possibly break compilation, i would totally agree with
> > you. i am not insisting on keeping the test + ifdefs, but you have been
> > warned now ;)
> > please confirm what i should do.
>
> If bitfields ever break compilation, we can handle the failure with some
> compiler dependent #ifdefs. Until then I'd rather avoid conditional code.

me too.
> Could you comment on my "bitfield checker" mail? I know that endianness
> should not be complained about and rather be automatically fixed, but at
> least the code in there hopefully detects all possible bitfield layout
> variants allowed by C99.

i have skimmed through it before. what is the exact purpose? just
checking if the implemented layout corresponds to our expectations
(true/false)?
the main problem i see with your test is that you are using the union
only for read access and not write access, but write the data with
memcpy only. imo writing via type pruning/unions should also be
tested (i use that, but no memcpy iirc).
> Semi-related question: Can we compile flashrom with gcc 2.95 (may break
> due to anonymous unions)? What about gcc 3.0/3.3?

not tested.
> Can you start a separate discussion thread about error numbers? I see
> that Uwe has started assigning unique return codes to various ft2232_spi
> errors, and I was not really happy with those.

will do.
> > > > even then the whole descriptor is of interest imo... and this will most
> > > > likely never occur in practice anyway.
> > >
> > > A possible use case is allowing reflashing of a dev board with an
> > > additional jumper.
> >
> > sure, but even then (or maybe then even more) the user would like to
> > know the contents of the descriptor. the flag does indicate that the
> > descriptor is valid only, nothing else. i don't see a problem.
>
> Agreed. My comment was about the "likely never occur" aspect of your
> statement. We might want to handle FDOPSS by disregarding region
> protection registers, but I don't know if the documentation matches the
> hardware.

jup. also, the whole protection scheme handling in ichspi needs to be
more refined (see the discussion in [PATCH 2/8] ichspi: disable writes
when locked or read-only regions are detected). FDOPSS needs to be
incorporated too...
but this is unrelated to the hunk discussed here. the descriptor should
always be printed if it seems to be valid (and the verbose level is
sufficient of course).