Am 21.07.2011 01:24 schrieb Stefan Tauner:
> based on the work of Matthias 'mazzoo' Wenzel this patch adds pretty
> printing of those ICH/PCH flash descriptors that are cached/mapped
> by the chipset (and which are therefore reachable via FDOC/FDOD
> registers).
>
> this includes the following:
> - content descriptor:
> describes the image and some generic properties (number of regions,
> offset of regions,
offset of which regions, or something else? How does this relate to
region descriptor(s)?
> PCH/ICH and MCH/PROC strap offsets and lengths)
> - component descriptor:
> identify the different SPI Flash Components and their capabilities.
>
components==chips? If yes, please say so in the changelog (and maybe
code comments).
> - region descriptor
> similarly to a partition table this describes the different regions
>
One common descriptor for all regions or one descriptor per region?
> - master descriptor
> defines access rights of the individual regions
>
> this is only a part of the data included in the descriptor. other
> information can be retreived
retrieved
> from a complete binary dump of the descriptor image only.
>
What's a descriptor image?
> this patch also adds macros and pretty printing for "Vendor Specific
> Component Capabilities" registers:
> there are two of them: lower and upper. they describe the properties
> of the address space divided by FPBA
We really need some place (maybe as comment in ich_decriptor.h?) where
all Intel acronyms are expanded and explained.
> (which allows to use multiple
> flash chips or partitions with different properties). the properties
> of flash chips supported by the ME are stored in the same format in
> a descriptor table.
"a descriptor table" implies there is more than one. Which one is it?
> a later patch will uses them outside of ichspi.c
> which is also the reason why the prettyprinting function and the
> register bit macros are not defined in ichspi.c but ich_descriptors.h.
>
> TODO:
> - file naming and partitioning was not discussed. we may want a ichspi.h.
> and/or rename ichspi.c to ich_spi.c or similar...
>
I'd say naming of the files is the least of our worries. We'll
reorganize all flashrom code in a directory structure anyway, and that's
when we can rename files if needed.
> - straps of ich8 (only)
Why not ICH7 (no descriptors?) or ICH9 and later (no datasheets?)
> could be printed at runtime via FDOC/FDOD.
> worth it? no, imo.
Agreed.
> but reading out the descriptor region manually and
> parsing it with the following patch may be useful?
>
following patch == 3/4?
Once we support true partial reads with the help of layout files, this
should be possible.
> Signed-off-by: Stefan Tauner <stefan.tau...@student.tuwien.ac.at>
>
Please note that I didn't check the code for correctness against the
datasheets nor did I check the code flow. The comments below are what I
noticed from reading the patch alone.
The message levels of most messages really need to be discussed... a
flood of msg_dbg is not necessarily a good idea, downgrading a lot of
them to dbg2 may improve said flood. That said, msg_[cpg]dbg2 is now in
the tree, so the dbg1 hack can go away. Looking again at your
messagelevels, maybe dbg1->dbg2 and dbg2->spew would be in order.
All ich specific structs should have an ich_ prefix in the struct name
to avoid polluting the global namespace.
> ---
> Makefile | 3 +-
> ich_descriptors.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++
> ich_descriptors.h | 264
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> ichspi.c | 39 ++++++--
> 4 files changed, 544 insertions(+), 9 deletions(-)
> create mode 100644 ich_descriptors.c
> create mode 100644 ich_descriptors.h
>
> diff --git a/Makefile b/Makefile
> index a35df06..7f73452 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -308,7 +308,8 @@ ifeq ($(CONFIG_INTERNAL), yes)
> FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
> PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o
> cbtable.o dmi.o internal.o
> ifeq ($(ARCH),"x86")
> -PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o sb600spi.o wbsio_spi.o
> mcp6x_spi.o
> +PROGRAMMER_OBJS += it87spi.o it85spi.o sb600spi.o wbsio_spi.o mcp6x_spi.o
> +PROGRAMMER_OBJS += ichspi.o ich_descriptors.o
> else
> endif
> NEED_PCI := yes
> diff --git a/ich_descriptors.c b/ich_descriptors.c
> new file mode 100644
> index 0000000..cf77601
> --- /dev/null
> +++ b/ich_descriptors.c
> @@ -0,0 +1,247 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (c) 2010 Matthias Wenzel <bios at mazzoo dot de>
> + * Copyright (c) 2011 Stefan Tauner
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#if defined(__i386__) || defined(__x86_64__)
> +
> +#include "ich_descriptors.h"
> +#include "flash.h" /* for msg_* */
> +
> +#define getFCBA(cont) (((cont)->FLMAP0 << 4) & 0x00000ff0)
> +#define getFRBA(cont) (((cont)->FLMAP0 >> 12) & 0x00000ff0)
> +#define getFMBA(cont) (((cont)->FLMAP1 << 4) & 0x00000ff0)
> +#define getFISBA(cont) (((cont)->FLMAP1 >> 12) & 0x00000ff0)
> +#define getFMSBA(cont) (((cont)->FLMAP2 << 4) & 0x00000ff0)
> +
> +void prettyprint_ich_descriptors(enum chipset cs, const struct
> flash_descriptors *desc)
>
(enum ich_chipset cs, const struct ich_flash_descriptors *desc)
> +{
>
Please try to handle cs at least somewhat. Check if it has the expected
value, and skip descriptor decoding (with a dbg/dbg2 message telling the
user about it) if the result would be wrong (i.e. for the wrong ICH family).
> + prettyprint_ich_descriptor_content(&desc->content);
> + prettyprint_ich_descriptor_component(desc);
> + prettyprint_ich_descriptor_region(&desc->region);
> + prettyprint_ich_descriptor_master(&desc->master);
> +}
> +
> +void prettyprint_ich_descriptor_content(const struct flash_content *cont)
>
ich_flash_content
Does this work on all chipsets?
> +{
> + msg_pdbg1("=== Content Section ===\n");
> + msg_pdbg1("FLVALSIG 0x%08x\n", cont->FLVALSIG);
> + msg_pdbg1("FLMAP0 0x%08x\n", cont->FLMAP0);
> + msg_pdbg1("FLMAP1 0x%08x\n", cont->FLMAP1);
> + msg_pdbg1("FLMAP2 0x%08x\n", cont->FLMAP2);
> + msg_pdbg1("\n");
>
dbg1->dbg, or maybe ->dbg2 to reduce output. The message level
discussion should not hold back this patch.
> +
> + msg_pdbg2("--- Details ---\n");
> + msg_pdbg2("NR (Number of Regions): %5d\n",
> + cont->NR + 1);
> + msg_pdbg2("FRBA (Flash Region Base Address): 0x%03x\n",
> + getFRBA(cont));
> + msg_pdbg2("NC (Number of Components): %5d\n",
> + cont->NC + 1);
> + msg_pdbg2("FCBA (Flash Component Base Address): 0x%03x\n",
> + getFCBA(cont));
> + msg_pdbg2("ISL (ICH/PCH Strap Length): %5d\n",
> + cont->ISL);
> + msg_pdbg2("FISBA/FPSBA (Flash ICH/PCH Strap Base Address): 0x%03x\n",
> + getFISBA(cont));
> + msg_pdbg2("NM (Number of Masters): %5d\n",
> + cont->NM + 1);
> + msg_pdbg2("FMBA (Flash Master Base Address): 0x%03x\n",
> + getFMBA(cont));
> + msg_pdbg2("MSL/PSL (MCH/PROC Strap Length): %5d\n",
> + cont->MSL);
> + msg_pdbg2("FMSBA (Flash MCH/PROC Strap Base Address): 0x%03x\n",
> + getFMSBA(cont));
> + msg_pdbg2("\n");
> +}
> +
> +void prettyprint_ich_descriptor_component(const struct flash_descriptors
> *desc)
>
ich_flash_descriptors
Which chipset is handled by this function?
> +{
> + const char * const str_freq[8] = {
>
static? Or is that useless here?
> + "20 MHz", /* 000 */
> + "33 MHz", /* 001 */
> + "reserved", /* 010 */
> + "reserved", /* 011 */
> + "50 MHz", /* 100 */
>
Mh. Is 50 MHz available on all descriptor capable chipsets?
> + "reserved", /* 101 */
> + "reserved", /* 110 */
> + "reserved" /* 111 */
> + };
> + const char * const str_mem[8] = {
>
static?
> + "512 kB", /* 000 */
> + " 1 MB", /* 001 */
> + " 2 MB", /* 010 */
> + " 4 MB", /* 011 */
> + " 8 MB", /* 100 */
> + " 16 MB", /* 101 */
> + "reserved", /* 110 */
> + "reserved", /* 111 */
> + };
> +
> + msg_pdbg1("=== Component Section ===\n");
> + msg_pdbg1("FLCOMP 0x%08x\n", desc->component.FLCOMP);
> + msg_pdbg1("FLILL 0x%08x\n", desc->component.FLILL );
> + msg_pdbg1("\n");
> +
> + msg_pdbg2("--- Details ---\n");
> + msg_pdbg2("Component 1 density: %s\n",
> + str_mem[desc->component.comp1_density]);
> + if (desc->content.NC)
> + msg_pdbg2("Component 2 density: %s\n",
> + str_mem[desc->component.comp2_density]);
> + else
> + msg_pdbg2("Component 2 is not used.\n");
> + msg_pdbg2("Read Clock Frequency: %s\n",
> + str_freq[desc->component.freq_read]);
> + msg_pdbg2("Read ID and Status Clock Freq.: %s\n",
> + str_freq[desc->component.freq_read_id]);
> + msg_pdbg2("Write and Erase Clock Freq.: %s\n",
> + str_freq[desc->component.freq_write]);
> + msg_pdbg2("Fast Read is %ssupported.\n",
> + desc->component.fastread ? "" : "not ");
> + if (desc->component.fastread)
> + msg_pdbg2("Fast Read Clock Frequency: %s\n",
> + str_freq[desc->component.freq_fastread]);
> + if (desc->component.FLILL == 0)
> + msg_pdbg2("No forbidden opcodes.\n");
> + else {
> + msg_pdbg2("Invalid instruction 0: 0x%02x\n",
> + desc->component.invalid_instr0);
> + msg_pdbg2("Invalid instruction 1: 0x%02x\n",
> + desc->component.invalid_instr1);
> + msg_pdbg2("Invalid instruction 2: 0x%02x\n",
> + desc->component.invalid_instr2);
> + msg_pdbg2("Invalid instruction 3: 0x%02x\n",
> + desc->component.invalid_instr3);
> + }
> + msg_pdbg2("\n");
> +}
> +
> +static void pprint_freg(const struct flash_region *reg, uint32_t i)
>
ich_flash_region
Which chipset? Or is the code generic?
> +{
> + const char *const region_names[5] = {
> + "Descr.", "BIOS", "ME", "GbE", "Platf."
> + };
> + uint32_t base = ICH_FREG_BASE(reg->FLREGs[i]);
> + uint32_t limit = ICH_FREG_LIMIT(reg->FLREGs[i]);
> + msg_pdbg2("Region %d (%-6s) ", i, region_names[i]);
> + if (base > limit)
> + msg_pdbg2("is unused.\n");
> + else
> + msg_pdbg2("0x%08x - 0x%08x\n", base, limit | 0x0fff);
> +}
> +
> +void prettyprint_ich_descriptor_region(const struct flash_region *reg)
>
ich_flash_region
Chipset specific or generic?
> +{
> + uint8_t i;
> + msg_pdbg1("=== Region Section ===\n");
> + for (i = 0; i < 5; i++)
> + msg_pdbg1("FLREG%d 0x%08x\n", i, reg->FLREGs[i]);
> + msg_pdbg1("\n");
> +
> + msg_pdbg2("--- Details ---\n");
> + for (i = 0; i < 5; i++)
> + pprint_freg(reg, i);
> + msg_pdbg2("\n");
> +}
> +
> +void prettyprint_ich_descriptor_master(const struct flash_master *mstr)
>
ich_flash_master
> +{
> + msg_pdbg1("=== Master Section ===\n");
> + msg_pdbg1("FLMSTR1 0x%08x\n", mstr->FLMSTR1);
> + msg_pdbg1("FLMSTR2 0x%08x\n", mstr->FLMSTR2);
> + msg_pdbg1("FLMSTR3 0x%08x\n", mstr->FLMSTR3);
> + msg_pdbg1("\n");
> +
> + msg_pdbg2("--- Details ---\n");
> + msg_pdbg2(" Descr. BIOS ME GbE Platf.\n");
> + msg_pdbg2("BIOS %c%c %c%c %c%c %c%c %c%c\n",
> + (mstr->BIOS_descr_r) ?'r':' ', (mstr->BIOS_descr_w) ?'w':' ',
> + (mstr->BIOS_BIOS_r) ?'r':' ', (mstr->BIOS_BIOS_w) ?'w':' ',
> + (mstr->BIOS_ME_r) ?'r':' ', (mstr->BIOS_ME_w) ?'w':' ',
> + (mstr->BIOS_GbE_r) ?'r':' ', (mstr->BIOS_GbE_w) ?'w':' ',
> + (mstr->BIOS_plat_r) ?'r':' ', (mstr->BIOS_plat_w) ?'w':' ');
> + msg_pdbg2("ME %c%c %c%c %c%c %c%c %c%c\n",
> + (mstr->ME_descr_r) ?'r':' ', (mstr->ME_descr_w) ?'w':' ',
> + (mstr->ME_BIOS_r) ?'r':' ', (mstr->ME_BIOS_w) ?'w':' ',
> + (mstr->ME_ME_r) ?'r':' ', (mstr->ME_ME_w) ?'w':' ',
> + (mstr->ME_GbE_r) ?'r':' ', (mstr->ME_GbE_w) ?'w':' ',
> + (mstr->ME_plat_r) ?'r':' ', (mstr->ME_plat_w) ?'w':' ');
> + msg_pdbg2("GbE %c%c %c%c %c%c %c%c %c%c\n",
> + (mstr->GbE_descr_r) ?'r':' ', (mstr->GbE_descr_w) ?'w':' ',
> + (mstr->GbE_BIOS_r) ?'r':' ', (mstr->GbE_BIOS_w) ?'w':' ',
> + (mstr->GbE_ME_r) ?'r':' ', (mstr->GbE_ME_w) ?'w':' ',
> + (mstr->GbE_GbE_r) ?'r':' ', (mstr->GbE_GbE_w) ?'w':' ',
> + (mstr->GbE_plat_r) ?'r':' ', (mstr->GbE_plat_w) ?'w':' ');
> + msg_pdbg2("\n");
> +}
> +
> +void prettyprint_ich9_reg_vscc(uint32_t reg_val)
> +{
> + pprint_reg_hex(VSCC, BES, reg_val, ", ");
> + pprint_reg(VSCC, WG, reg_val, ", ");
> + pprint_reg(VSCC, WSR, reg_val, ", ");
> + pprint_reg(VSCC, WEWS, reg_val, ", ");
> + pprint_reg_hex(VSCC, EO, reg_val, ", ");
> + pprint_reg(VSCC, VCL, reg_val, "\n");
> +}
> +
> +static uint32_t read_descriptor_reg(uint8_t section, uint16_t offset, void
> *spibar)
> +{
> + uint32_t control = 0;
> + control |= (section << FDOC_FDSS_OFF) & FDOC_FDSS;
> + control |= (offset << FDOC_FDSI_OFF) & FDOC_FDSI;
> + *(volatile uint32_t *) (spibar + ICH9_REG_FDOC) = control;
>
volatile is absolutely no-go outside hwaccess.c. Use mmio_write[bwl] or
the littleendian variant of mmio_write[bwl] instead.
> + return *(volatile uint32_t *)(spibar + ICH9_REG_FDOD);
>
Same here. Use mmio_read[bwl] instead.
> +}
> +
> +void read_ich_descriptors_via_fdo(void *spibar, struct flash_descriptors
> *desc)
> +{
> + msg_pdbg1("Reading flash descriptors "
> + "mapped by the chipset via FDOC/FDOD...");
> + /* content section */
> + desc->content.FLVALSIG = read_descriptor_reg(0, 0, spibar);
> + desc->content.FLMAP0 = read_descriptor_reg(0, 1, spibar);
> + desc->content.FLMAP1 = read_descriptor_reg(0, 2, spibar);
> + desc->content.FLMAP2 = read_descriptor_reg(0, 3, spibar);
> +
> + /* component section */
> + desc->component.FLCOMP = read_descriptor_reg(1, 0, spibar);
> + desc->component.FLILL = read_descriptor_reg(1, 1, spibar);
> + desc->component.FLPB = read_descriptor_reg(1, 2, spibar);
> +
> + /* region section */
> + desc->region.FLREGs[0] = read_descriptor_reg(2, 0, spibar);
> + desc->region.FLREGs[1] = read_descriptor_reg(2, 1, spibar);
> + desc->region.FLREGs[2] = read_descriptor_reg(2, 2, spibar);
> + desc->region.FLREGs[3] = read_descriptor_reg(2, 3, spibar);
> + desc->region.FLREGs[4] = read_descriptor_reg(2, 4, spibar);
> +
> + /* master section */
> + desc->master.FLMSTR1 = read_descriptor_reg(3, 0, spibar);
> + desc->master.FLMSTR2 = read_descriptor_reg(3, 1, spibar);
> + desc->master.FLMSTR3 = read_descriptor_reg(3, 2, spibar);
> +
> + /* Accessing the strap section via FDOC/D is only possible on ICH8 and
> + * reading the upper map is impossible on all chipsets, so don't bother.
> + */
> +
> + msg_pdbg1(" done\n");
>
This seems to be a true flood of output. dbg2 for all callees maybe?
> +}
> +#endif // defined(__i386__) || defined(__x86_64__)
> diff --git a/ich_descriptors.h b/ich_descriptors.h
> new file mode 100644
> index 0000000..f248394
> --- /dev/null
> +++ b/ich_descriptors.h
> @@ -0,0 +1,264 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (c) 2010 Matthias Wenzel <bios at mazzoo dot de>
> + * Copyright (c) 2011 Stefan Tauner
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#if defined(__i386__) || defined(__x86_64__)
> +#ifndef __ICH_DESCRIPTORS_H__
> +#define __ICH_DESCRIPTORS_H__ 1
> +
> +#include <stdint.h>
> +
> +/* most of the following should probably be in a new file named ichspi.h */
> +#define msg_pdbg1 msg_pdbg
> +#define msg_pdbg2 msg_pspew
> +
> +#define ICH9_REG_FDOC 0xB0 /* 32 Bits Flash Descriptor
> Observability Control */
> + /* 0-1: reserved */
> +#define FDOC_FDSI_OFF 2 /* 2-11: Flash Descriptor
> Section Index */
> +#define FDOC_FDSI (0x3f << FDOC_FDSI_OFF)
> +#define FDOC_FDSS_OFF 12 /* 12-14: Flash Descriptor
> Section Select */
> +#define FDOC_FDSS (0x3 << FDOC_FDSS_OFF)
> + /* 15-31: reserved */
> +
> +#define ICH9_REG_FDOD 0xB4 /* 32 Bits Flash Descriptor
> Observability Data */
> +
> +/* Field locations and semantics for LVSCC, UVSCC and related words in the
> flash
> + * descriptor are equal therefore they all share the same macros below. */
> +#define VSCC_BES_OFF 0 /* 0-1: Block/Sector Erase Size */
> +#define VSCC_BES (0x3 << VSCC_BES_OFF)
> +#define VSCC_WG_OFF 2 /* 2: Write Granularity */
> +#define VSCC_WG (0x1 << VSCC_WG_OFF)
>
Whitespace damage or is this caused by the leading + in the patch?
> +#define VSCC_WSR_OFF 3 /* 3: Write Status Required */
> +#define VSCC_WSR (0x1 << VSCC_WSR_OFF)
> +#define VSCC_WEWS_OFF 4 /* 4: Write Enable on Write
> Status */
> +#define VSCC_WEWS (0x1 << VSCC_WEWS_OFF)
> + /* 5-7: reserved */
> +#define VSCC_EO_OFF 8 /* 8-15: Erase Opcode */
> +#define VSCC_EO (0xff << VSCC_EO_OFF)
> + /* 16-22: reserved */
>
Whitespace damage?
> +#define VSCC_VCL_OFF 23 /* 23: Vendor Component Lock */
> +#define VSCC_VCL (0x1 << VSCC_VCL_OFF)
> + /* 24-31: reserved */
> +
> +#define pprint_reg(reg, bit, val, sep) msg_pdbg("%s=%d" sep, #bit,
> (val & reg##_##bit)>>reg##_##bit##_OFF)
> +#define pprint_reg_hex(reg, bit, val, sep) msg_pdbg("%s=0x%x" sep, #bit,
> (val & reg##_##bit)>>reg##_##bit##_OFF)
>
Please add () around val at the right hand side.
> +void prettyprint_ich9_reg_vscc(uint32_t reg_val);
> +
> +#define ICH_FREG_BASE(flreg) ((flreg << 12) & 0x01fff000)
> +#define ICH_FREG_LIMIT(flreg) ((flreg >> 4) & 0x01fff000)
>
Please add () around flreg at the right hand side.
> +
> +/* Used to select the right descriptor printing function.
> + * Currently only ICH8 and Ibex Peak are supported.
> + */
> +enum chipset {
>
ich_chipset
> + CHIPSET_UNKNOWN,
>
CHIPSET_ICH_UNKNOWN
We may need a CHIPSET_UNKNOWN elsewhere...
> + CHIPSET_ICH7 = 7,
> + CHIPSET_ICH8,
> + CHIPSET_ICH9,
> + CHIPSET_ICH10,
> + CHIPSET_5_SERIES_IBEX_PEAK,
> + CHIPSET_6_SERIES_COUGAR_POINT,
> + CHIPSET_7_SERIES_PANTHER_POINT
> +};
> +
>
All structs below probably need __attribute__((packed)).
> +struct flash_content {
>
ich_flash_content
> + uint32_t FLVALSIG; /* 0x00 */
> + union { /* 0x04 */
> + uint32_t FLMAP0;
> + struct {
> + uint32_t FCBA :8;
> + uint32_t NC :2;
> + uint32_t :6;
> + uint32_t FRBA :8;
> + uint32_t NR :3;
> + uint32_t :5;
> + };
> + };
> + union { /* 0x08 */
> + uint32_t FLMAP1;
> + struct {
> + uint32_t FMBA :8;
> + uint32_t NM :3;
> + uint32_t :5;
> + uint32_t FISBA :8;
> + uint32_t ISL :8;
> + };
> + };
> + union { /* 0x0c */
> + uint32_t FLMAP2;
> + struct {
> + uint32_t FMSBA :8;
> + uint32_t MSL :8;
> + uint32_t :16;
> + };
> + };
> +};
> +
> +struct flash_component {
>
ich_flash_component
> + union { /* 0x00 */
> + uint32_t FLCOMP;
> + struct {
> + uint32_t comp1_density :3;
> + uint32_t comp2_density :3;
> + uint32_t :11;
> + uint32_t freq_read :3;
> + uint32_t fastread :1;
> + uint32_t freq_fastread :3;
> + uint32_t freq_write :3;
> + uint32_t freq_read_id :3;
> + uint32_t :2;
> + };
> + };
> + union { /* 0x04 */
> + uint32_t FLILL;
> + struct {
> + uint32_t invalid_instr0;
>
Really uint32_t? If FLILL is 32 bits, but all invalid_instr are 32 bits
as well, the union members have inconsistent sizes.
uint8_t invalid_instr0 maybe?
> + uint32_t invalid_instr1;
> + uint32_t invalid_instr2;
> + uint32_t invalid_instr3;
>
Dito for the other invalid_instr above.
> + };
> + };
> + union { /* 0x08 */
> + uint32_t FLPB;
> + struct {
> + uint32_t FPBA :13;
> + uint32_t :19;
> + };
> + };
>
All code accessing FLPB/FPBA was broken due to the wrong FLILL union
size, so if it worked anyway, you have another bug hidden somewhere.
> +};
> +
> +struct flash_region {
>
ich_flash_region
> + union {
> + uint32_t FLREGs[5];
> + struct {
> + /* FLREG0 Flash Descriptor */
> + struct {
> + uint32_t reg0_base :13;
>
Are uint32_t bitfields allowed? I thought bitfields were either signed
int or unsigned int, but never uint32_t.
> + uint32_t :3;
>
Are anonymous bitfields allowed?
> + uint32_t reg0_limit :13;
> + uint32_t :3;
> + };
> + /* FLREG1 BIOS */
> + struct {
> + uint32_t reg1_base :13;
> + uint32_t :3;
> + uint32_t reg1_limit :13;
> + uint32_t :3;
> + };
> + /* FLREG2 ME */
> + struct {
> + uint32_t reg2_base :13;
> + uint32_t :3;
> + uint32_t reg2_limit :13;
> + uint32_t :3;
> + };
> + /* FLREG3 GbE */
> + struct {
> + uint32_t reg3_base :13;
> + uint32_t :3;
> + uint32_t reg3_limit :13;
> + uint32_t :3;
> + };
> + /* FLREG4 Platform */
> + struct {
> + uint32_t reg4_base :13;
> + uint32_t :3;
> + uint32_t reg4_limit :13;
> + uint32_t :3;
> + };
> + };
> + };
> +};
> +
> +struct flash_master {
>
ich_flash_master
> + union {
> + uint32_t FLMSTR1;
> + struct {
> + uint32_t BIOS_req_ID :16;
> + uint32_t BIOS_descr_r :1;
> + uint32_t BIOS_BIOS_r :1;
> + uint32_t BIOS_ME_r :1;
> + uint32_t BIOS_GbE_r :1;
> + uint32_t BIOS_plat_r :1;
> + uint32_t :3;
> + uint32_t BIOS_descr_w :1;
> + uint32_t BIOS_BIOS_w :1;
> + uint32_t BIOS_ME_w :1;
> + uint32_t BIOS_GbE_w :1;
> + uint32_t BIOS_plat_w :1;
> + uint32_t :3;
> + };
> + };
> + union {
> + uint32_t FLMSTR2;
> + struct {
> + uint32_t ME_req_ID :16;
> + uint32_t ME_descr_r :1;
> + uint32_t ME_BIOS_r :1;
> + uint32_t ME_ME_r :1;
> + uint32_t ME_GbE_r :1;
> + uint32_t ME_plat_r :1;
> + uint32_t :3;
> + uint32_t ME_descr_w :1;
> + uint32_t ME_BIOS_w :1;
> + uint32_t ME_ME_w :1;
> + uint32_t ME_GbE_w :1;
> + uint32_t ME_plat_w :1;
> + uint32_t :3;
> + };
> + };
> + union {
> + uint32_t FLMSTR3;
> + struct {
> + uint32_t GbE_req_ID :16;
> + uint32_t GbE_descr_r :1;
> + uint32_t GbE_BIOS_r :1;
> + uint32_t GbE_ME_r :1;
> + uint32_t GbE_GbE_r :1;
> + uint32_t GbE_plat_r :1;
> + uint32_t :3;
> + uint32_t GbE_descr_w :1;
> + uint32_t GbE_BIOS_w :1;
> + uint32_t GbE_ME_w :1;
> + uint32_t GbE_GbE_w :1;
> + uint32_t GbE_plat_w :1;
> + uint32_t :3;
> + };
> + };
> +};
> +
> +struct flash_descriptors {
>
ich_flash_descriptors
> + struct flash_content content;
>
ich_flash_content
> + struct flash_component component;
>
ich_flash_component
> + struct flash_region region;
>
ich_flash_region
> + struct flash_master master;
>
ich_flash_master
> +};
> +
> +void prettyprint_ich_descriptors(enum chipset, const struct
> flash_descriptors *desc);
>
Missing parameter name for first parameter, and of course
ich_chipset ... ich_flash_descriptors.
> +
> +void prettyprint_ich_descriptor_content(const struct flash_content *content);
> +void prettyprint_ich_descriptor_component(const struct flash_descriptors
> *desc);
> +void prettyprint_ich_descriptor_region(const struct flash_region *region);
> +void prettyprint_ich_descriptor_master(const struct flash_master *master);
>
ich_ for all struct names (not for the variable names)
> +
> +void read_ich_descriptors_via_fdo(void *spibar, struct flash_descriptors
> *desc);
>
ich_flash_descriptors
> +
> +#endif // __ICH_DESCRIPTORS_H__
> +#endif // defined(__i386__) || defined(__x86_64__)
> diff --git a/ichspi.c b/ichspi.c
> index 2480fa5..b5ee23a 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -30,6 +30,7 @@
> #include "chipdrivers.h"
> #include "programmer.h"
> #include "spi.h"
> +#include "ich_descriptors.h"
>
> /* ICH9 controller register definition */
> #define ICH9_REG_HSFS 0x04 /* 16 Bits Hardware Sequencing
> Flash Status */
> @@ -120,6 +121,14 @@
> #define ICH9_REG_BBAR 0xA0 /* 32 Bits BIOS Base Address
> Configuration */
> #define BBAR_MASK 0x00ffff00 /* 8-23: Bottom of System Flash
> */
>
> +#define ICH9_REG_LVSCC 0xC4 /* 32 Bits Host Lower Vendor
> Specific Component Capabilities */
> +#define ICH9_REG_UVSCC 0xC8 /* 32 Bits Host Upper Vendor
> Specific Component Capabilities */
> +/* The individual fields of the VSCC registers are defined in the file
> + * ich_descriptors.h. The reason is that the same fields are also used in the
> + * flash descriptors to define the properties of the different flash chips
> + * supported by the BIOS image. These descriptors are also the source for the
> + * registers above. */
>
This comment is a bit unclear to me.
> +
> #define ICH9_REG_FPB 0xD0 /* 32 Bits Flash Partition Boundary */
> #define FPB_FPBA_OFF 0 /* 0-12: Block/Sector Erase Size */
> #define FPB_FPBA (0x1FFF << FPB_FPBA_OFF)
> @@ -302,8 +311,6 @@ static void prettyprint_opcodes(OPCODES *ops)
> }
> }
>
> -#define pprint_reg(reg, bit, val, sep) msg_pdbg("%s=%d" sep, #bit, (val &
> reg##_##bit)>>reg##_##bit##_OFF)
> -
> static void prettyprint_ich9_reg_hsfs(uint16_t reg_val)
> {
> msg_pdbg("HSFS: ");
> @@ -1130,9 +1137,6 @@ static int ich_spi_send_multicommand(struct spi_command
> *cmds)
> #define ICH_BRWA(x) ((x >> 8) & 0xff)
> #define ICH_BRRA(x) ((x >> 0) & 0xff)
>
> -#define ICH_FREG_BASE(x) ((x >> 0) & 0x1fff)
> -#define ICH_FREG_LIMIT(x) ((x >> 16) & 0x1fff)
> -
> static void do_ich9_spi_frap(uint32_t frap, int i)
> {
> static const char *const access_names[4] = {
> @@ -1159,9 +1163,8 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> return;
> }
>
> - msg_pdbg("0x%08x-0x%08x is %s\n",
> - (base << 12), (limit << 12) | 0x0fff,
> - access_names[rwperms]);
> + msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
> + access_names[rwperms]);
> }
>
> static const struct spi_programmer spi_programmer_ich7 = {
> @@ -1191,6 +1194,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base,
> void *rcrb,
> uint8_t old, new;
> uint16_t spibar_offset, tmp2;
> uint32_t tmp;
> + int ichspi_desc = 0;
>
> switch (ich_generation) {
> case 7:
> @@ -1262,6 +1266,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base,
> void *rcrb,
> msg_pinfo("WARNING: SPI Configuration Lockdown
> activated.\n");
> ichspi_lock = 1;
> }
> + if (tmp2 & HSFS_FDV)
> + ichspi_desc = 1;
>
> tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
> msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
> @@ -1312,9 +1318,26 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base,
> void *rcrb,
> ichspi_bbar = mmio_readl(ich_spibar + ICH9_REG_BBAR);
> msg_pdbg("0xA0: 0x%08x (BBAR)\n",
> ichspi_bbar);
> +
> + tmp = mmio_readl(ich_spibar + ICH9_REG_LVSCC);
> + msg_pdbg("0xC4: 0x%08x (LVSCC)\n", tmp);
> + msg_pdbg("LVSCC: ");
> + prettyprint_ich9_reg_vscc(tmp);
>
Does the block above work correctly for all ICH8 and later chipsets?
> +
> + tmp = mmio_readl(ich_spibar + ICH9_REG_UVSCC);
> + msg_pdbg("0xC8: 0x%08x (UVSCC)\n", tmp);
> + msg_pdbg("UVSCC: ");
> + prettyprint_ich9_reg_vscc(tmp);
>
Ditto.
> +
> tmp = mmio_readl(ich_spibar + ICH9_REG_FPB);
> msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp);
>
> + msg_pdbg("\n");
> + if (ichspi_desc) {
> + struct flash_descriptors desc = {{ 0 }};
> + read_ich_descriptors_via_fdo(ich_spibar, &desc);
>
Odd code structure, but I see what you're trying to do. Makes sense.
> + prettyprint_ich_descriptors(CHIPSET_UNKNOWN, &desc);
>
Hmmm... I really would like to see this fixed, but it's OK if you want
to do that in a followup patch.
> + }
> ich_init_opcodes();
> break;
> default:
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
