[PATCH] flashrom: clean up SPI probing, bus handling

[Stefan Reinauer]

See patch. It's a first attempt, but I think we should get this in in
small enough step by step improvements.

[Carl-Daniel Hailfinger]

On 29.06.2008 01:04, Stefan Reinauer wrote:
> See patch. It's a first attempt, but I think we should get this in in
> small enough step by step improvements.
>
> First attempt to clean up SPI probing and create a common
> construct: the flash bus.
>
> At some point the flash bus will be part of struct flashchip.
>
> Signed-off-by: Stefan Reinauer

Nice, but I'd like a small design change:
* Keep a list of available buses: flashbuses[]
* Call all functions with an additional parameter: flashbus

Per-bus probing is a lot easier with that change.

The cleanups unrelated to flash bus support are
Acked-by: Carl-Daniel Hailfinger

For the rest, please consider my design change suggestions.
> Index: flash.h
>
> --- flash.h	(revision 3393)
> +++ flash.h	(working copy)
> @@ -370,10 +370,18 @@
>  /* chipset_enable.c */
>  int chipset_flash_enable(void);
>  void print_supported_chipsets(void);
> -extern int ich7_detected;
> -extern int ich9_detected;
> -extern void *ich_spibar;
>
> +typedef enum {

BUS_TYPE_FWH
BUS_TYPE_PARALLEL
> BUS_TYPE_LPC,
> BUS_TYPE_ICH7_SPI,
> BUS_TYPE_ICH9_SPI,
> BUS_TYPE_IT87XX_SPI,
> BUS_TYPE_VIA_SPI
>
> +} flashbus_t;
>
> +extern flashbus_t flashbus;

See my initial comment:

extern flashbus_t flashbuses[];
> +extern void *spibar;
>
> /* Physical memory mapping device */
>  #if defined (__sun) && (defined(__i386) || defined(__amd64))
>  #  define MEM_DEV "/dev/xsvc"
> Index: it87spi.c
> ===================================================================
> --- it87spi.c	(revision 3393)
> +++ it87spi.c	(working copy)
> @@ -3,6 +3,7 @@
> Copyright (C) 2007, 2008 Carl-Daniel Hailfinger
> Copyright (C) 2008 Ronald Hoogenboom
> Copyright (C) 2008 coresystems GmbH
> 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
>
> @@ -112,8 +113,13 @@
>  int it87xx_probe_spi_flash(const char *name)

int it87xx_probe_spi_flash(flashbus_t flashbus, const char *name)
> {
>    it8716f_flashport = find_ite_spi_flash_port(ITE_SUPERIO_PORT1);
> if (!it8716f_flashport)
>   it8716f_flashport = find_ite_spi_flash_port(ITE_SUPERIO_PORT2);
> if (it8716f_flashport)
> flashbus = BUS_TYPE_IT87XX_SPI;
> return (!it8716f_flashport);
>
> }
> Index: spi.c
>
> --- spi.c	(revision 3393)
> +++ spi.c	(working copy)
> @@ -34,13 +34,15 @@
>
> int spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr)

int spi_command(flashbus_t flashbus, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr)
> {
> if (it8716f_flashport)
> switch (flashbus) {
> case BUS_TYPE_IT87XX_SPI:
> return it8716f_spi_command(writecnt, readcnt, writearr, readarr);
>   else if (ich7_detected)
> case BUS_TYPE_ICH7_SPI:
> case BUS_TYPE_ICH9_SPI:
>            return ich_spi_command(writecnt, readcnt, writearr, readarr);
> else if (ich9_detected)
> return ich_spi_command(writecnt, readcnt, writearr, readarr);
> printf_debug("%s called, but no SPI chipset detected\n", __FUNCTION__);
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
> return 1;
>
> }
>
> @@ -299,11 +301,17 @@
>
> void spi_page_program(int block, uint8_t *buf, uint8_t *bios)

void spi_page_program(flashbus_t flashbus, int block, uint8_t *buf, uint8_t *bios)
> {
> if (it8716f_flashport) {
> switch (flashbus) {
> case BUS_TYPE_IT87XX_SPI:
> it8716f_spi_page_program(block, buf, bios);
> return;
> break;
> case BUS_TYPE_ICH7_SPI:
> case BUS_TYPE_ICH9_SPI:
> printf_debug("%s called, but not implemented for ICH\n", __FUNCTION__);
> break;
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
> printf_debug("%s called, but no SPI chipset detected\n", __FUNCTION__);
>
> }
>
> /*
> @@ -358,25 +366,32 @@
>
> int spi_chip_read(struct flashchip *flash, uint8_t *buf)

same here.
> {
> if (it8716f_flashport)
> switch (flashbus) {
> case BUS_TYPE_IT87XX_SPI:
> return it8716f_spi_chip_read(flash, buf);
>   else if (ich7_detected)
>           return ich_spi_read(flash, buf);
> else if (ich9_detected)
> case BUS_TYPE_ICH7_SPI:
> case BUS_TYPE_ICH9_SPI:
> return ich_spi_read(flash, buf);
> printf_debug("%s called, but no SPI chipset detected\n", __FUNCTION__);
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
> return 1;
>
> }
>
> int spi_chip_write(struct flashchip *flash, uint8_t *buf)

same here
> {
> if (it8716f_flashport)
> switch (flashbus) {
> case BUS_TYPE_IT87XX_SPI:
> return it8716f_spi_chip_write(flash, buf);
>   else if (ich7_detected)
>           return ich_spi_write(flash, buf);
> else if (ich9_detected)
> case BUS_TYPE_ICH7_SPI:
> case BUS_TYPE_ICH9_SPI:
> return ich_spi_write(flash, buf);
> printf_debug("%s called, but no SPI chipset detected\n", __FUNCTION__);
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
> return 1;
>
> }
> Index: chipset_enable.c
>
> --- chipset_enable.c	(revision 3393)
> +++ chipset_enable.c	(working copy)
> @@ -35,6 +35,17 @@
>  #include <unistd.h>
>  #include "flash.h"
>
> +/**
> flashrom defaults to LPC flash devices. If a known SPI controller is found
> and the SPI strappings are set, this will be overwritten by the probing code.
> Eventually, this will become an array when multiple flash support works.
> */
>
> +flashbus_t flashbus = BUS_TYPE_LPC;

flashbus_t flashbuses[MAX_BUSES] = {BUS_TYPE_LPC, };
int flashbus_count=1;

(and matching changes below)
> +void *spibar = NULL;
>
> static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name)
>  {
>    uint8_t tmp;
> @@ -124,7 +135,7 @@
>     * Note: Accesses to FFFF0000-FFFFFFFF are always forwarded to ISA.
>     * Set bit 2: BIOSCS# Write Enable (1=enable, 0=disable).
>     */
> new = old | 0x2c4;
>
> new = old | 0x02c4;
>
> if (new == old)
> return 0;
>
> @@ -185,68 +196,106 @@
>    return enable_flash_ich(dev, name, 0xdc);
>  }
>
> -void *ich_spibar = NULL;
> +#define ICH_STRAP_RSVD 0x00
> +#define ICH_STRAP_SPI  0x01
> +#define ICH_STRAP_PCI  0x02
> +#define ICH_STRAP_LPC  0x03
>
> -static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, unsigned long spibar)
> +static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, int ich_generation)
>  {
> int ret, i;
> uint8_t old, new, bbs, buc;
> uint16_t spibar_offset;
> uint32_t tmp, gcs;
> void *rcrb;
> static const char *straps_names[] = { "reserved", "SPI", "PCI", "LPC" };
> /* Enable Flash Writes */
> ret = enable_flash_ich_dc(dev, name);
> /* Read the Root Complex Base Address Register (RCBA) */
> tmp = pci_read_long(dev, 0xf0);
> /* Get physical address of Root Complex Register Block */
> tmp = pci_read_long(dev, 0xf0) & 0xffffc000;
> printf_debug("\nRoot Complex Register Block address = 0x%x\n", tmp);
> /* Calculate the Root Complex Register Block address */
> tmp &= 0xffffc000;
> printf_debug("\nRoot Complex Register Block address = 0x%x\n", tmp);
> /* Map RCBA to virtual memory */
> rcrb = mmap(0, 0x4000, PROT_READ | PROT_WRITE, MAP_SHARED, fd_mem, (off_t)tmp);
> if (rcrb == MAP_FAILED) {
> perror("Can't mmap memory using " MEM_DEV);
> exit(1);
> }
> printf_debug("GCS address = 0x%x\n", tmp + 0x3410);
> gcs = *(volatile uint32_t *)(rcrb + 0x3410);
>  printf_debug("GCS = 0x%x: ", gcs);
>  printf_debug("BIOS Interface Lock-Down: %sabled, ",
>        (gcs & 0x1) ? "en" : "dis");
>  bbs = (gcs >> 10) & 0x3;
> printf_debug("BOOT BIOS Straps: 0x%x (%s)\n",	bbs,
>      (bbs == 0x3) ? "LPC" : ((bbs == 0x2) ? "PCI" : "SPI"));
>
> printf_debug("BOOT BIOS Straps: 0x%x (%s)\n", bbs, straps_names[bbs]);
>
> buc = *(volatile uint8_t *)(rcrb + 0x3414);
> printf_debug("Top Swap : %s\n", (buc & 1)?"enabled (A16 inverted)":"not enabled");
>
> if (bbs == ICH_STRAP_LPC) {
> /* No further SPI initialization required */
> return ret;
>
> }
>
> switch (ich_generation) {
>
> case 7:
> flashbus = BUS_TYPE_ICH7_SPI;
> spibar_offset = 0x3020;
> break;
>
> case 8:
> flashbus = BUS_TYPE_ICH9_SPI;
> spibar_offset = 0x3020;
> break;
>
> case 9:
>
> default: /* Future version might behave the same */
> flashbus = BUS_TYPE_ICH9_SPI;
> spibar_offset = 0x3800;
> break;
>
> }
>
> /* SPIBAR is at RCRB+0x3020 for ICH[78] and RCRB+0x3800 for ICH9. */
> printf_debug("SPIBAR = 0x%x + 0x%04x\n", tmp, (uint16_t)spibar);
> printf_debug("SPIBAR = 0x%x + 0x%04x\n", tmp, spibar_offset);
> // Assign Virtual Address
> ich_spibar =  rcrb + spibar;
> /* Assign Virtual Address */
> spibar =  rcrb + spibar_offset;
> if (ich7_detected) {
> int i;
> printf_debug("0x00: 0x%04x     (SPIS)\n", *(uint16_t *)(ich_spibar + 0));
> printf_debug("0x02: 0x%04x     (SPIC)\n", *(uint16_t *)(ich_spibar + 2));
> printf_debug("0x04: 0x%08x (SPIA)\n", *(uint32_t *)(ich_spibar + 4));
> switch (flashbus) {
> case BUS_TYPE_ICH7_SPI:
> printf_debug("0x00: 0x%04x     (SPIS)\n", *(uint16_t *)(spibar + 0));
> printf_debug("0x02: 0x%04x     (SPIC)\n", *(uint16_t *)(spibar + 2));
> printf_debug("0x04: 0x%08x (SPIA)\n", *(uint32_t *)(spibar + 4));
> for (i=0; i < 8; i++) {
> 	int offs;
> 	offs = 8 + (i * 8);
> 	printf_debug("0x%02x: 0x%08x (SPID%d)\n", offs, *(uint32_t *)(ich_spibar + offs), i);
> 	printf_debug("0x%02x: 0x%08x (SPID%d+4)\n", offs+4, *(uint32_t *)(ich_spibar + offs +4), i);
> 	printf_debug("0x%02x: 0x%08x (SPID%d)\n", offs, *(uint32_t *)(spibar + offs), i);
> 	printf_debug("0x%02x: 0x%08x (SPID%d+4)\n", offs+4, *(uint32_t *)(spibar + offs +4), i);
> }
> printf_debug("0x50: 0x%08x (BBAR)\n", *(uint32_t *)(ich_spibar + 0x50));
> printf_debug("0x54: 0x%04x     (PREOP)\n", *(uint16_t *)(ich_spibar + 0x54));
> printf_debug("0x56: 0x%04x     (OPTYPE)\n", *(uint16_t *)(ich_spibar + 0x56));
> printf_debug("0x58: 0x%08x (OPMENU)\n", *(uint32_t *)(ich_spibar + 0x58));
> printf_debug("0x5c: 0x%08x (OPMENU+4)\n", *(uint32_t *)(ich_spibar + 0x5c));
> printf_debug("0x50: 0x%08x (BBAR)\n", *(uint32_t *)(spibar + 0x50));
> printf_debug("0x54: 0x%04x     (PREOP)\n", *(uint16_t *)(spibar + 0x54));
> printf_debug("0x56: 0x%04x     (OPTYPE)\n", *(uint16_t *)(spibar + 0x56));
> printf_debug("0x58: 0x%08x (OPMENU)\n", *(uint32_t *)(spibar + 0x58));
> printf_debug("0x5c: 0x%08x (OPMENU+4)\n", *(uint32_t *)(spibar + 0x5c));
> for (i=0; i < 4; i++) {
> 	int offs;
> 	offs = 0x60 + (i * 4);
> 	printf_debug("0x%02x: 0x%08x (PBR%d)\n", offs, *(uint32_t *)(ich_spibar + offs), i);
> 	printf_debug("0x%02x: 0x%08x (PBR%d)\n", offs, *(uint32_t *)(spibar + offs), i);
> }
> printf_debug("\n");
> if ( (*(uint16_t *)ich_spibar) & (1 << 15)) {
> if ( (*(uint16_t *)spibar) & (1 << 15)) {
> printf("WARNING: SPI Configuration Lockdown activated.\n");
>
> }
> break;
>
> case BUS_TYPE_ICH9_SPI:
> /* TODO: Add dumping function for ICH8/ICH9, or drop the 
>  * whole SPIBAR dumping from chipset_enable.c - There's 
>  * inteltool for this task already.
>  */
> break;
>
> default:
> /* Nothing */
> break;
>
> }
>
> old = pci_read_byte(dev, 0xdc);
>
> @@ -257,37 +306,30 @@
>    case 1:
>    case 2:
>    	printf_debug("prefetching %sabled, caching %sabled, ",
> 	(new & 0x2) ? "en" : "dis", (new & 0x1) ? "dis" : "en");
> 	(new & 0x2) ? "en" : "dis", 
> 	(new & 0x1) ? "dis" : "en");
> break;
> default:
> printf_debug("invalid prefetching/caching settings, ");
> break;
> }
> return enable_flash_ich_dc(dev, name);
> return ret;
>
> }
>
> -/* Flag for ICH7 SPI register block */
> -int ich7_detected = 0;
>
> static int enable_flash_ich7(struct pci_dev *dev, const char *name)
>  {
>   ich7_detected = 1;
> return enable_flash_ich_dc_spi(dev, name, 0x3020);
> return enable_flash_ich_dc_spi(dev, name, 7);
>
> }
>
> -/* Flag for ICH8/ICH9 SPI register block */
> -int ich9_detected = 0;
>
> static int enable_flash_ich8(struct pci_dev *dev, const char *name)
>  {
> ich9_detected = 1;
> return enable_flash_ich_dc_spi(dev, name, 0x3020);
> return enable_flash_ich_dc_spi(dev, name, 8);
>
> }
>
> static int enable_flash_ich9(struct pci_dev *dev, const char *name)
>  {
> ich9_detected = 1;
> return enable_flash_ich_dc_spi(dev, name, 0x3800);
> return enable_flash_ich_dc_spi(dev, name, 9);
>
> }
>
> static int enable_flash_vt823x(struct pci_dev *dev, const char *name)
> Index: ichspi.c
> ===================================================================
> --- ichspi.c	(revision 3393)
> +++ ichspi.c	(working copy)
> @@ -123,20 +123,20 @@
>  static inline uint32_t REGREAD32(int X)
>  {
>    volatile uint32_t regval;
> regval = *(volatile uint32_t *) ((uint8_t *) ich_spibar + X);
> regval = *(volatile uint32_t *) ((uint8_t *) spibar + X);
> return regval;
>
> }
>
> static inline uint16_t REGREAD16(int X)
>  {
>    volatile uint16_t regval;
> regval = *(volatile uint16_t *) ((uint8_t *) ich_spibar + X);
> regval = *(volatile uint16_t *) ((uint8_t *) spibar + X);
> return regval;
>
> }
>
> -#define REGWRITE32(X,Y) (*(uint32_t *)((uint8_t *)ich_spibar+X)=Y)
> -#define REGWRITE16(X,Y) (*(uint16_t *)((uint8_t *)ich_spibar+X)=Y)
> -#define REGWRITE8(X,Y)  (*(uint8_t *)((uint8_t *)ich_spibar+X)=Y)
> +#define REGWRITE32(X,Y) (*(uint32_t *)((uint8_t *)spibar+X)=Y)
> +#define REGWRITE16(X,Y) (*(uint16_t *)((uint8_t *)spibar+X)=Y)
> +#define REGWRITE8(X,Y)  (*(uint8_t *)((uint8_t *)spibar+X)=Y)
>
> /* Common SPI functions */
>  static int program_opcodes(OPCODES * op);
> @@ -167,58 +167,50 @@
>  int program_opcodes(OPCODES * op)
>  {
>    uint8_t a;
> uint16_t temp16;
> uint32_t temp32;
>
> uint16_t preop, optype;
>
> uint32_t opmenu[2];
>
> /* Program Prefix Opcodes */
> temp16 = 0;
> preop = 0;
> /* 0:7 Prefix Opcode 1 */
> temp16 = (op->preop[0]);
> preop = (op->preop[0]);
> /* 8:16 Prefix Opcode 2 */
> temp16 |= ((uint16_t) op->preop[1]) << 8;
> if (ich7_detected) {
> REGWRITE16(ICH7_REG_PREOP, temp16);
> } else if (ich9_detected) {
> REGWRITE16(ICH9_REG_PREOP, temp16);
> }
> preop |= ((uint16_t) op->preop[1]) << 8;
> /* Program Opcode Types 0 - 7 */
> temp16 = 0;
> optype = 0;
> for (a = 0; a < 8; a++) {
> temp16 |= ((uint16_t) op->opcode[a].spi_type) << (a * 2);
> optype |= ((uint16_t) op->opcode[a].spi_type) << (a * 2);
> }
> if (ich7_detected) {
> REGWRITE16(ICH7_REG_OPTYPE, temp16);
> } else if (ich9_detected) {
> REGWRITE16(ICH9_REG_OPTYPE, temp16);
> }
> /* Program Allowable Opcodes 0 - 3 */
> temp32 = 0;
> opmenu[0] = 0;
> for (a = 0; a < 4; a++) {
> temp32 |= ((uint32_t) op->opcode[a].opcode) << (a * 8);
> opmenu[0] |= ((uint32_t) op->opcode[a].opcode) << (a * 8);
> }
> if (ich7_detected) {
> REGWRITE32(ICH7_REG_OPMENU, temp32);
> } else if (ich9_detected) {
> REGWRITE32(ICH9_REG_OPMENU, temp32);
> }
> /*Program Allowable Opcodes 4 - 7 */
> temp32 = 0;
> opmenu[1] = 0;
> for (a = 4; a < 8; a++) {
> temp32 |=
>     ((uint32_t) op->opcode[a].opcode) << ((a - 4) * 8);
> opmenu[1] |= ((uint32_t) op->opcode[a].opcode) << ((a - 4) * 8);
> }
> if (ich7_detected) {
> REGWRITE32(ICH7_REG_OPMENU + 4, temp32);
> } else if (ich9_detected) {
> REGWRITE32(ICH9_REG_OPMENU + 4, temp32);
>
> switch (flashbus) {
>
> case BUS_TYPE_ICH7_SPI:
> REGWRITE16(ICH7_REG_PREOP, preop);
> REGWRITE16(ICH7_REG_OPTYPE, optype);
> REGWRITE32(ICH7_REG_OPMENU, opmenu[0]);
> REGWRITE32(ICH7_REG_OPMENU + 4, opmenu[1]);
> break;
>
> case BUS_TYPE_ICH9_SPI:
> REGWRITE16(ICH9_REG_PREOP, preop);
> REGWRITE16(ICH9_REG_OPTYPE, optype);
> REGWRITE32(ICH9_REG_OPMENU, opmenu[0]);
> REGWRITE32(ICH9_REG_OPMENU + 4, opmenu[1]);
> break;
>
> default:
> printf_debug("%s: unsupported chipset\n", __FUNCTION__);
> return -1;
>
> }
>
> return 0;
>
> @@ -332,6 +324,7 @@
>    		   uint8_t datalength, uint8_t * data)
>  {
>    int write_cmd = 0;
> int timeout;
> uint32_t temp32;
> uint32_t a;
>
> @@ -402,10 +395,13 @@
>    REGWRITE32(ICH9_REG_SSFS, temp32);
>
> /*wait for cycle complete */
> while ((REGREAD32(ICH9_REG_SSFS) & SSFS_CDS) == 0) {
> /*TODO; Do something that this can't lead into an endless loop. but some
>  * commands may cause this to be last more than 30 seconds */
>
> timeout = 1000 * 60;	// 60s is a looong timeout.
>
> while (((REGREAD32(ICH9_REG_SSFS) & SSFS_CDS) == 0) && --timeout) {
> myusec_delay(1000);
>
> }
>
> if (!timeout) {
> printf_debug("timeout\n");
>
> }
>
> if ((REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) != 0) {
> printf_debug("Transaction error!\n");
>
> @@ -430,10 +426,13 @@
>  static int run_opcode(uint8_t nr, OPCODE op, uint32_t offset,
>    	      uint8_t datalength, uint8_t * data)
>  {
> if (ich7_detected)
> switch (flashbus) {
> case BUS_TYPE_ICH7_SPI:
> return ich7_run_opcode(nr, op, offset, datalength, data);
> else if (ich9_detected) {
>
> case BUS_TYPE_ICH9_SPI:
> return ich9_run_opcode(nr, op, offset, datalength, data);
>
> default:
> printf_debug("%s: unsupported chipset\n", __FUNCTION__);
>
> }
>
> /* If we ever get here, something really weird happened */

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 29.06.2008 01:04, Stefan Reinauer wrote:
> > See patch. It's a first attempt, but I think we should get this in in
> > small enough step by step improvements.
> >
> > First attempt to clean up SPI probing and create a common
> > construct: the flash bus.
> >
> > At some point the flash bus will be part of struct flashchip.
> >
> > Signed-off-by: Stefan Reinauer
>
> Nice, but I'd like a small design change:
> Keep a list of available buses: flashbuses[]
> Call all functions with an additional parameter: flashbus

Yes, something along the line. Alongside with this I want to clean up 
use of struct flashchip
for flashes[] but I would really like to get the current change in the 
tree yet.
> Per-bus probing is a lot easier with that change.
>
> The cleanups unrelated to flash bus support are
> Acked-by: Carl-Daniel Hailfinger
>
> For the rest, please consider my design change suggestions.

Thanks.. please allow me to come up with something that integrates your 
suggestions after we get the current state in the tree.

I am thinking of splitting up struct flashchip into the description part 
and the instance part.

struct flashchip {
        const char *vendor;
        const char *name;
        /* With 32bit manufacture_id and model_id we can cover IDs up to
         * (including) the 4th bank of JEDEC JEP106W Standard Manufacturer's
         * Identification code.
         */
        uint32_t manufacture_id;
        uint32_t model_id;

int total_size;
        int page_size;

/* Indicate if flashrom has been tested with this flash chip and if
         * everything worked correctly.
         */
        uint32_t tested;

int (*probe) (struct flashchip *flash);
        int (*erase) (struct flashchip *flash);
        int (*write) (struct flashchip *flash, uint8_t *buf);
        int (*read) (struct flashchip *flash, uint8_t *buf);

uint32_t compatible_busses;
}

//  now this is what flashes is going to be:
struct flashchip_instance {
        struct flashchip *flash;

flashbus_t bus;

/* Some flash devices have an additional register space. */
        volatile uint8_t *virtual_memory;
        volatile uint8_t *virtual_registers;

unsigned long physical_memory;
};
> > Index: flash.h
> >
> > --- flash.h	(revision 3393)
> > +++ flash.h	(working copy)
> > @@ -370,10 +370,18 @@
> >  /* chipset_enable.c */
> >  int chipset_flash_enable(void);
> >  void print_supported_chipsets(void);
> > -extern int ich7_detected;
> > -extern int ich9_detected;
> > -extern void *ich_spibar;
> >
> > +typedef enum {
>
> BUS_TYPE_FWH
> BUS_TYPE_PARALLEL
> > BUS_TYPE_LPC,
> > BUS_TYPE_ICH7_SPI,
> > BUS_TYPE_ICH9_SPI,
> > BUS_TYPE_IT87XX_SPI,
> > BUS_TYPE_VIA_SPI
> >
> > +} flashbus_t;
> >
> > +extern flashbus_t flashbus;
>
> See my initial comment:
>
> extern flashbus_t flashbuses[];
> > +flashbus_t flashbus = BUS_TYPE_LPC;
>
> flashbus_t flashbuses[MAX_BUSES] = {BUS_TYPE_LPC, };
> int flashbus_count=1;
>
> (and matching changes below)

[Carl-Daniel Hailfinger]

On 29.06.2008 01:43, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > On 29.06.2008 01:04, Stefan Reinauer wrote:
> > > See patch. It's a first attempt, but I think we should get this in in
> > > small enough step by step improvements.
> > >
> > > First attempt to clean up SPI probing and create a common
> > > construct: the flash bus.
> > >
> > > At some point the flash bus will be part of struct flashchip.
> > >
> > > Signed-off-by: Stefan Reinauer
> >
> > Nice, but I'd like a small design change:
> > Keep a list of available buses: flashbuses[]
> > Call all functions with an additional parameter: flashbus
>
> Yes, something along the line. Alongside with this I want to clean up
> use of struct flashchip
> for flashes[] but I would really like to get the current change in the
> tree yet.

I'm not a fan of committing code which will be changed shortly
afterwards, but...
> > Per-bus probing is a lot easier with that change.
> >
> > The cleanups unrelated to flash bus support are
> > Acked-by: Carl-Daniel Hailfinger
> >
> > For the rest, please consider my design change suggestions.
>
> Thanks.. please allow me to come up with something that integrates
> your suggestions after we get the current state in the tree.
>
> I am thinking of splitting up struct flashchip into the description
> part and the instance part.
>
> struct flashchip {
>        const char *vendor;
>        const char *name;
>        /* With 32bit manufacture_id and model_id we can cover IDs up to
>         * (including) the 4th bank of JEDEC JEP106W Standard
> Manufacturer's
>         * Identification code.
>         */
>        uint32_t manufacture_id;
>        uint32_t model_id;
>    int total_size;
>    int page_size;
>
>    /* Indicate if flashrom has been tested with this flash chip
>
> and if
>         * everything worked correctly.
>         */
>        uint32_t tested;
>    int (*probe) (struct flashchip *flash);
>    int (*erase) (struct flashchip *flash);
>    int (*write) (struct flashchip *flash, uint8_t *buf);
>    int (*read) (struct flashchip *flash, uint8_t *buf);

Maybe change struct flashchip to struct flashchip_instance for the 4
functions above?
>    uint32_t compatible_busses;
>
> }
>
> //  now this is what flashes is going to be:
> struct flashchip_instance {
>        struct flashchip *flash;
>    flashbus_t bus;
>      /* Some flash devices have an additional register space. */
>    volatile uint8_t *virtual_memory;
>    volatile uint8_t *virtual_registers;
>
>   unsigned long physical_memory;

What does that struct member mean?
> };

Overall, I like your new design.

Since we don't know yet for sure what's the problem with Pietro's hang,
I'd like to keep the code churn in SPI low until he has given us more info.

Regards,
Carl-Daniel
> > > Index: flash.h
> > >
> > > --- flash.h    (revision 3393)
> > > +++ flash.h    (working copy)
> > > @@ -370,10 +370,18 @@
> > >  /* chipset_enable.c */
> > >  int chipset_flash_enable(void);
> > >  void print_supported_chipsets(void);
> > > -extern int ich7_detected;
> > > -extern int ich9_detected;
> > > -extern void *ich_spibar;
> > >
> > > +typedef enum {
> >
> > BUS_TYPE_FWH
> > BUS_TYPE_PARALLEL
> > > BUS_TYPE_LPC,
> > > BUS_TYPE_ICH7_SPI,
> > > BUS_TYPE_ICH9_SPI,
> > > BUS_TYPE_IT87XX_SPI,
> > > BUS_TYPE_VIA_SPI
> > >
> > > +} flashbus_t;
> > >
> > > +extern flashbus_t flashbus;
> >
> > See my initial comment:
> >
> > extern flashbus_t flashbuses[];
> > > +flashbus_t flashbus = BUS_TYPE_LPC;
> >
> > flashbus_t flashbuses[MAX_BUSES] = {BUS_TYPE_LPC, };
> > int flashbus_count=1;
> >
> > (and matching changes below)

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> > Yes, something along the line. Alongside with this I want to clean up
> > use of struct flashchip
> > for flashes[] but I would really like to get the current change in the
> > tree yet.
>
> I'm not a fan of committing code which will be changed shortly
> afterwards, but...

Release early, release often, yes? ;-) I can keep this in our internal
repository for a while until it's ripened, but I fear it will decay if
it's only floating around as a patch and not in the repo (see Rudolf's
SPI patch, though that can still easily be applied for VIA systems)
> > struct flashchip {
> >        const char *vendor;
> >        const char *name;
> >        /* With 32bit manufacture_id and model_id we can cover IDs up to
> >         * (including) the 4th bank of JEDEC JEP106W Standard
> > Manufacturer's
> >         * Identification code.
> >         */
> >        uint32_t manufacture_id;
> >        uint32_t model_id;
> >    int total_size;
> >    int page_size;
> >
> >    /* Indicate if flashrom has been tested with this flash chip
> >
> > and if
> >         * everything worked correctly.
> >         */
> >        uint32_t tested;
> >    int (*probe) (struct flashchip *flash);
> >    int (*erase) (struct flashchip *flash);
> >    int (*write) (struct flashchip *flash, uint8_t *buf);
> >    int (*read) (struct flashchip *flash, uint8_t *buf);
>
> Maybe change struct flashchip to struct flashchip_instance for the 4
> functions above?

The parameter? Yes, of course.
> >    uint32_t compatible_busses;
> >
> > }
> >
> > //  now this is what flashes is going to be:
> > struct flashchip_instance {
> >        struct flashchip *flash;
> >    flashbus_t bus;
> >      /* Some flash devices have an additional register space. */
> >    volatile uint8_t *virtual_memory;
> >    volatile uint8_t *virtual_registers;
> >
> >   unsigned long physical_memory;
>
> What does that struct member mean?

physical address of the flash chip. we'll need that to determine where
to look for flash chips in our memory maps. The virtual addresses are
not sufficient

[Carl-Daniel Hailfinger]

On 29.06.2008 12:35, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > > Yes, something along the line. Alongside with this I want to clean up
> > > use of struct flashchip
> > > for flashes[] but I would really like to get the current change in the
> > > tree yet.
> >
> > I'm not a fan of committing code which will be changed shortly
> > afterwards, but...
>
> Release early, release often, yes? ;-) I can keep this in our internal
> repository for a while until it's ripened, but I fear it will decay if
> it's only floating around as a patch and not in the repo (see Rudolf's
> SPI patch, though that can still easily be applied for VIA systems)

Actually, to give Rudolf the chance to adapt VIA chipset support in
flashrom to the new code I'd like to keep big commits away from ichspi.c
until VIA support is merged.
> > > struct flashchip {
> > >        const char *vendor;
> > >        const char *name;
> > >        /* With 32bit manufacture_id and model_id we can cover IDs up to
> > >         * (including) the 4th bank of JEDEC JEP106W Standard
> > > Manufacturer's
> > >         * Identification code.
> > >         */
> > >        uint32_t manufacture_id;
> > >        uint32_t model_id;
> > >    int total_size;
> > >    int page_size;
> > >
> > >    /* Indicate if flashrom has been tested with this flash chip
> > >
> > > and if
> > >         * everything worked correctly.
> > >         */
> > >        uint32_t tested;
> > >    int (*probe) (struct flashchip *flash);
> > >    int (*erase) (struct flashchip *flash);
> > >    int (*write) (struct flashchip *flash, uint8_t *buf);
> > >    int (*read) (struct flashchip *flash, uint8_t *buf);
> >
> > Maybe change struct flashchip to struct flashchip_instance for the 4
> > functions above?
>
> The parameter? Yes, of course.

Good.
> > >    uint32_t compatible_busses;
> > >
> > > }
> > >
> > > //  now this is what flashes is going to be:
> > > struct flashchip_instance {
> > >        struct flashchip *flash;
> > >    flashbus_t bus;
> > >      /* Some flash devices have an additional register space. */
> > >    volatile uint8_t *virtual_memory;
> > >    volatile uint8_t *virtual_registers;
> > >
> > >   unsigned long physical_memory;
> >
> > What does that struct member mean?
>
> physical address of the flash chip. we'll need that to determine where
> to look for flash chips in our memory maps. The virtual addresses are
> not sufficient

I always thought that virtual_memory is exactly the location of the
flash chip in the memory map. If not, what's the point of virtual_memory?

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 29.06.2008 12:35, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger wrote:
> > > > Yes, something along the line. Alongside with this I want to clean up
> > > > use of struct flashchip
> > > > for flashes[] but I would really like to get the current change in the
> > > > tree yet.
> > >
> > > I'm not a fan of committing code which will be changed shortly
> > > afterwards, but...
> >
> > Release early, release often, yes? ;-) I can keep this in our internal
> > repository for a while until it's ripened, but I fear it will decay if
> > it's only floating around as a patch and not in the repo (see Rudolf's
> > SPI patch, though that can still easily be applied for VIA systems)
>
> Actually, to give Rudolf the chance to adapt VIA chipset support in
> flashrom to the new code I'd like to keep big commits away from ichspi.c
> until VIA support is merged.

I have his changes in my tree. If this is going in, I can commit his
stuff right afterwards.
> I always thought that virtual_memory is exactly the location of the
> flash chip in the memory map. If not, what's the point of virtual_memory?

* virtual_memory is the flash chip mapped into the flashrom process'
memory map. It's an arbitrary address valid for that one process.

* physical_memory is the address on the "outside" memory bus of the cpu.
that's where all the PCI bars live etc.

Those are completely distinct address spaces, and usually nothing
(except some memory chunks) from the physical address space is visible
in the virtual address space. flashrom can not access the flashchip,
unless it maps a piece of physical memory (0xfff00000-0xffffffff for
example) to its virtual address space. That's why we need to mmap to
actually see it. But the address we get from mmap does not have any
descriptive character, it's a mere handle for us to access that physical
memory.

Stefan

[Carl-Daniel Hailfinger]

On 29.06.2008 13:26, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > On 29.06.2008 12:35, Stefan Reinauer wrote:
> > > Carl-Daniel Hailfinger wrote:
> > > > > Yes, something along the line. Alongside with this I want to clean up
> > > > > use of struct flashchip
> > > > > for flashes[] but I would really like to get the current change in the
> > > > > tree yet.
> > > >
> > > > I'm not a fan of committing code which will be changed shortly
> > > > afterwards, but...
> > >
> > > Release early, release often, yes? ;-) I can keep this in our internal
> > > repository for a while until it's ripened, but I fear it will decay if
> > > it's only floating around as a patch and not in the repo (see Rudolf's
> > > SPI patch, though that can still easily be applied for VIA systems)
> >
> > Actually, to give Rudolf the chance to adapt VIA chipset support in
> > flashrom to the new code I'd like to keep big commits away from ichspi.c
> > until VIA support is merged.
>
> I have his changes in my tree. If this is going in, I can commit his
> stuff right afterwards.

Do you have them integrated into ichspi.c? IIRC Rudolf wanted to merge
his ICH7/VT8237S code into ichspi.c, but the patch he sent had it as a
separate file due to time constraints.
> > I always thought that virtual_memory is exactly the location of the
> > flash chip in the memory map. If not, what's the point of virtual_memory?
> virtual_memory is the flash chip mapped into the flashrom process'
>
> memory map. It's an arbitrary address valid for that one process.
> physical_memory is the address on the "outside" memory bus of the cpu.
>
> that's where all the PCI bars live etc.
>
> Those are completely distinct address spaces, and usually nothing
> (except some memory chunks) from the physical address space is visible
> in the virtual address space. flashrom can not access the flashchip,
> unless it maps a piece of physical memory (0xfff00000-0xffffffff for
> example) to its virtual address space. That's why we need to mmap to
> actually see it. But the address we get from mmap does not have any
> descriptive character, it's a mere handle for us to access that physical
> memory.

I thought we use mmap(,,,MAP_FIXED,,) which would have ensured
virtual_memory and physical_memory being identical. Looking at the
source it seems that I misremembered the actual mmap call, so your
statement makes a lot of sense and I agree with it.

Thanks for the explanation.

Regards,
Carl-Daniel