Re: [flashrom] [PATCH 1/3] Move Intel SPI initialisation to ichspi.c

[Carl-Daniel Hailfinger]

On 18.07.2010 16:19, Michael Karcher wrote:
> Signed-off-by: Michael Karcher
>
> chipset_enable.c |  202 +--------------------------------------------------
>  flash.h          |    6 +-
>  ichspi.c         |  214 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 218 insertions(+), 204 deletions(-)
>
> diff --git a/chipset_enable.c b/chipset_enable.c
> index dc0e55f..7434d65 100644
> --- a/chipset_enable.c
> +++ b/chipset_enable.c
> @@ -417,69 +417,15 @@ static int enable_flash_poulsbo(struct pci_dev *dev, const char *name)
>
> static int enable_flash_vt8237s_spi(struct pci_dev *dev, const char *name)
>  {
> uint32_t mmio_base;
> /* Do we really need no write enable? */
> mmio_base = (pci_read_long(dev, 0xbc)) << 8;
> msg_pdbg("MMIO base at = 0x%x\n", mmio_base);
> ich_spibar = physmap("VT8237S MMIO registers", mmio_base, 0x70);
> msg_pdbg("0x6c: 0x%04x     (CLOCK/DEBUG)\n",
>      mmio_readw(ich_spibar + 0x6c));
> /* Not sure if it speaks all these bus protocols. */
> buses_supported = CHIP_BUSTYPE_LPC | CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_VIA;
> ich_init_opcodes();
> return 0;
>
> -}
>
> -#define ICH_BMWAG(x) ((x >> 24) & 0xff)
> -#define ICH_BMRAG(x) ((x >> 16) & 0xff)
> -#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)
> -{
> const char *access_names[4] = {
> "locked", "read-only", "write-only", "read-write"
> };
> const char *region_names[5] = {
> "Flash Descriptor", "BIOS", "Management Engine",
> "Gigabit Ethernet", "Platform Data"
> };
> uint32_t base, limit;
> int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
>       (((ICH_BRRA(frap) >> i) & 1) << 0);
> int offset = 0x54 + i * 4;
> uint32_t freg = mmio_readl(ich_spibar + offset);
> msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
>      offset, freg, i, region_names[i]);
> base  = ICH_FREG_BASE(freg);
> limit = ICH_FREG_LIMIT(freg);
> if (base == 0x1fff && limit == 0) {
> /* this FREG is disabled */
> msg_pdbg("%s region is unused.\n", region_names[i]);
> return;
> }
> msg_pdbg("0x%08x-0x%08x is %s\n",
>     (base << 12), (limit << 12) | 0x0fff,
>     access_names[rwperms]);
> return via_init_spi(dev);
>
> }
>
> 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, tmp2;
> int ret;
> uint8_t bbs, buc;
> uint32_t tmp, gcs;
> void *rcrb;
> //TODO: These names are incorrect for EP80579. For that, the solution would look like the commented line
>
> @@ -518,147 +464,7 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name,
>    	return ret;
>    }
> switch (ich_generation) {
> case 7:
> buses_supported = CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_ICH7;
> spibar_offset = 0x3020;
> break;
> case 8:
> buses_supported = CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_ICH9;
> spibar_offset = 0x3020;
> break;
> case 9:
> case 10:
> default:		/* Future version might behave the same */
> buses_supported = CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_ICH9;
> spibar_offset = 0x3800;
> break;
> }
> /* SPIBAR is at RCRB+0x3020 for ICH[78] and RCRB+0x3800 for ICH9. */
> msg_pdbg("SPIBAR = 0x%x + 0x%04x\n", tmp, spibar_offset);
> /* Assign Virtual Address */
> ich_spibar = rcrb + spibar_offset;
> switch (spi_controller) {
> case SPI_CONTROLLER_ICH7:
> msg_pdbg("0x00: 0x%04x     (SPIS)\n",
> 	     mmio_readw(ich_spibar + 0));
> msg_pdbg("0x02: 0x%04x     (SPIC)\n",
> 	     mmio_readw(ich_spibar + 2));
> msg_pdbg("0x04: 0x%08x (SPIA)\n",
> 	     mmio_readl(ich_spibar + 4));
> for (i = 0; i < 8; i++) {
> 	int offs;
> 	offs = 8 + (i * 8);
> 	msg_pdbg("0x%02x: 0x%08x (SPID%d)\n", offs,
> 		     mmio_readl(ich_spibar + offs), i);
> 	msg_pdbg("0x%02x: 0x%08x (SPID%d+4)\n", offs + 4,
> 		     mmio_readl(ich_spibar + offs + 4), i);
> }
> ichspi_bbar = mmio_readl(ich_spibar + 0x50);
> msg_pdbg("0x50: 0x%08x (BBAR)\n",
> 	     ichspi_bbar);
> msg_pdbg("0x54: 0x%04x     (PREOP)\n",
> 	     mmio_readw(ich_spibar + 0x54));
> msg_pdbg("0x56: 0x%04x     (OPTYPE)\n",
> 	     mmio_readw(ich_spibar + 0x56));
> msg_pdbg("0x58: 0x%08x (OPMENU)\n",
> 	     mmio_readl(ich_spibar + 0x58));
> msg_pdbg("0x5c: 0x%08x (OPMENU+4)\n",
> 	     mmio_readl(ich_spibar + 0x5c));
> for (i = 0; i < 4; i++) {
> 	int offs;
> 	offs = 0x60 + (i * 4);
> 	msg_pdbg("0x%02x: 0x%08x (PBR%d)\n", offs,
> 		     mmio_readl(ich_spibar + offs), i);
> }
> msg_pdbg("\n");
> if (mmio_readw(ich_spibar) & (1 << 15)) {
> 	msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
> 	ichspi_lock = 1;
> }
> ich_init_opcodes();
> break;
> case SPI_CONTROLLER_ICH9:
> tmp2 = mmio_readw(ich_spibar + 4);
> msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
> msg_pdbg("FLOCKDN %i, ", (tmp2 >> 15 & 1));
> msg_pdbg("FDV %i, ", (tmp2 >> 14) & 1);
> msg_pdbg("FDOPSS %i, ", (tmp2 >> 13) & 1);
> msg_pdbg("SCIP %i, ", (tmp2 >> 5) & 1);
> msg_pdbg("BERASE %i, ", (tmp2 >> 3) & 3);
> msg_pdbg("AEL %i, ", (tmp2 >> 2) & 1);
> msg_pdbg("FCERR %i, ", (tmp2 >> 1) & 1);
> msg_pdbg("FDONE %i\n", (tmp2 >> 0) & 1);
> tmp = mmio_readl(ich_spibar + 0x50);
> msg_pdbg("0x50: 0x%08x (FRAP)\n", tmp);
> msg_pdbg("BMWAG 0x%02x, ", ICH_BMWAG(tmp));
> msg_pdbg("BMRAG 0x%02x, ", ICH_BMRAG(tmp));
> msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
> msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
> /* print out the FREGx registers along with FRAP access bits */
> for(i = 0; i < 5; i++)
> 	do_ich9_spi_frap(tmp, i);
> msg_pdbg("0x74: 0x%08x (PR0)\n",
> 	     mmio_readl(ich_spibar + 0x74));
> msg_pdbg("0x78: 0x%08x (PR1)\n",
> 	     mmio_readl(ich_spibar + 0x78));
> msg_pdbg("0x7C: 0x%08x (PR2)\n",
> 	     mmio_readl(ich_spibar + 0x7C));
> msg_pdbg("0x80: 0x%08x (PR3)\n",
> 	     mmio_readl(ich_spibar + 0x80));
> msg_pdbg("0x84: 0x%08x (PR4)\n",
> 	     mmio_readl(ich_spibar + 0x84));
> msg_pdbg("0x90: 0x%08x (SSFS, SSFC)\n",
> 	     mmio_readl(ich_spibar + 0x90));
> msg_pdbg("0x94: 0x%04x     (PREOP)\n",
> 	     mmio_readw(ich_spibar + 0x94));
> msg_pdbg("0x96: 0x%04x     (OPTYPE)\n",
> 	     mmio_readw(ich_spibar + 0x96));
> msg_pdbg("0x98: 0x%08x (OPMENU)\n",
> 	     mmio_readl(ich_spibar + 0x98));
> msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n",
> 	     mmio_readl(ich_spibar + 0x9C));
> ichspi_bbar = mmio_readl(ich_spibar + 0xA0);
> msg_pdbg("0xA0: 0x%08x (BBAR)\n",
> 	     ichspi_bbar);
> msg_pdbg("0xB0: 0x%08x (FDOC)\n",
> 	     mmio_readl(ich_spibar + 0xB0));
> if (tmp2 & (1 << 15)) {
> 	msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
> 	ichspi_lock = 1;
> }
> ich_init_opcodes();
> break;
> default:
> /* Nothing */
> break;
> }
> old = pci_read_byte(dev, 0xdc);
> msg_pdbg("SPI Read Configuration: ");
> new = (old >> 2) & 0x3;
> switch (new) {
> case 0:
> case 1:
> case 2:
> msg_pdbg("prefetching %sabled, caching %sabled, ",
> 	     (new & 0x2) ? "en" : "dis",
> 	     (new & 0x1) ? "dis" : "en");
> break;
> default:
> msg_pdbg("invalid prefetching/caching settings, ");
> break;
> }
> ich_init_spi(dev, tmp, rcrb, ich_generation);

check for the return value?
> return ret;
>  }
>
> diff --git a/flash.h b/flash.h
> index c3302c4..a8ca6d7 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -680,10 +680,10 @@ int default_spi_send_multicommand(struct spi_command *cmds);
>  uint32_t spi_get_valid_read_addr(void);
>
> /* ichspi.c */
> -extern int ichspi_lock;
>  extern uint32_t ichspi_bbar;
> -extern void *ich_spibar;
> -int ich_init_opcodes(void);
> +void ich_init_spi(struct pci_dev *dev, uint32_t base, void * rcrb,

whitespace after * and at the end of the line.
>              int ich_generation);
>
> +int via_init_spi(struct pci_dev * dev);

whitespace after *
> int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	    const unsigned char *writearr, unsigned char *readarr);
>  int ich_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len);
> diff --git a/ichspi.c b/ichspi.c
> index d6f9118..246f3ae 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -101,11 +101,11 @@
>  #define ICH7_REG_OPMENU                0x58	/* 64 Bits */
>
> /* ICH SPI configuration lock-down. May be set during chipset enabling. */
> -int ichspi_lock = 0;
> +static int ichspi_lock = 0;
>
> uint32_t ichspi_bbar = 0;
>
> -void *ich_spibar = NULL;
> +static void *ich_spibar = NULL;
>
> typedef struct _OPCODE {
>    uint8_t opcode;		//This commands spi opcode
> @@ -364,7 +364,7 @@ void ich_set_bbar(uint32_t minaddr)
> It should be called before ICH sends any spi command.
>
> */
> -int ich_init_opcodes(void)
> +static int ich_init_opcodes(void)
>  {
>    int rc = 0;
>    OPCODES *curopcodes_done;
> @@ -398,6 +398,214 @@ int ich_init_opcodes(void)
>    }
>  }
>
> +#define ICH_BMWAG(x) ((x >> 24) & 0xff)
> +#define ICH_BMRAG(x) ((x >> 16) & 0xff)
> +#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)
> +{
> const char *access_names[4] = {
> "locked", "read-only", "write-only", "read-write"
> };
> const char *region_names[5] = {
> "Flash Descriptor", "BIOS", "Management Engine",
> "Gigabit Ethernet", "Platform Data"
> };
> uint32_t base, limit;
> int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
>       (((ICH_BRRA(frap) >> i) & 1) << 0);
> int offset = 0x54 + i * 4;
> uint32_t freg = mmio_readl(ich_spibar + offset);
> msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
>      offset, freg, i, region_names[i]);
> base  = ICH_FREG_BASE(freg);
> limit = ICH_FREG_LIMIT(freg);
> if (base == 0x1fff && limit == 0) {
> /* this FREG is disabled */
> msg_pdbg("%s region is unused.\n", region_names[i]);
> return;
> }
> msg_pdbg("0x%08x-0x%08x is %s\n",
>     (base << 12), (limit << 12) | 0x0fff,
>     access_names[rwperms]);
>
> +}

Is the FRAP/FREG stuff really SPI specific? If not, it should stay in
chipset_enable.c AFAICS.
> +void ich_init_spi(struct pci_dev *dev, uint32_t base, void * rcrb,

whitespace after * and at end of line
Can you make this function return an error value?
>              int ich_generation)
>
> +{
>    int i;
>    uint8_t old, new;
> uint16_t spibar_offset, tmp2;
> uint32_t tmp;
> switch (ich_generation) {
> case 7:
> buses_supported = CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_ICH7;
> spibar_offset = 0x3020;
> break;
> case 8:
> buses_supported = CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI;

Is adding BUSTYPE_FWH to buses_supported a good idea here in ichspi.c?
> spi_controller = SPI_CONTROLLER_ICH9;
> spibar_offset = 0x3020;
> break;
> case 9:
> case 10:
> default:		/* Future version might behave the same */
> buses_supported = CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_ICH9;
> spibar_offset = 0x3800;
> break;
> }
> /* SPIBAR is at RCRB+0x3020 for ICH[78] and RCRB+0x3800 for ICH9. */
> msg_pdbg("SPIBAR = 0x%x + 0x%04x\n", base, spibar_offset);
> /* Assign Virtual Address */
> ich_spibar = rcrb + spibar_offset;
> switch (spi_controller) {
> case SPI_CONTROLLER_ICH7:
> msg_pdbg("0x00: 0x%04x     (SPIS)\n",
> 	     mmio_readw(ich_spibar + 0));
> msg_pdbg("0x02: 0x%04x     (SPIC)\n",
> 	     mmio_readw(ich_spibar + 2));
> msg_pdbg("0x04: 0x%08x (SPIA)\n",
> 	     mmio_readl(ich_spibar + 4));
> for (i = 0; i < 8; i++) {
> 	int offs;
> 	offs = 8 + (i * 8);
> 	msg_pdbg("0x%02x: 0x%08x (SPID%d)\n", offs,
> 		     mmio_readl(ich_spibar + offs), i);
> 	msg_pdbg("0x%02x: 0x%08x (SPID%d+4)\n", offs + 4,
> 		     mmio_readl(ich_spibar + offs + 4), i);
> }
> ichspi_bbar = mmio_readl(ich_spibar + 0x50);
> msg_pdbg("0x50: 0x%08x (BBAR)\n",
> 	     ichspi_bbar);
> msg_pdbg("0x54: 0x%04x     (PREOP)\n",
> 	     mmio_readw(ich_spibar + 0x54));
> msg_pdbg("0x56: 0x%04x     (OPTYPE)\n",
> 	     mmio_readw(ich_spibar + 0x56));
> msg_pdbg("0x58: 0x%08x (OPMENU)\n",
> 	     mmio_readl(ich_spibar + 0x58));
> msg_pdbg("0x5c: 0x%08x (OPMENU+4)\n",
> 	     mmio_readl(ich_spibar + 0x5c));
> for (i = 0; i < 4; i++) {
> 	int offs;
> 	offs = 0x60 + (i * 4);
> 	msg_pdbg("0x%02x: 0x%08x (PBR%d)\n", offs,
> 		     mmio_readl(ich_spibar + offs), i);
> }
> msg_pdbg("\n");
> if (mmio_readw(ich_spibar) & (1 << 15)) {
> 	msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
> 	ichspi_lock = 1;
> }
> ich_init_opcodes();
> break;
> case SPI_CONTROLLER_ICH9:
> tmp2 = mmio_readw(ich_spibar + 4);
> msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
> msg_pdbg("FLOCKDN %i, ", (tmp2 >> 15 & 1));
> msg_pdbg("FDV %i, ", (tmp2 >> 14) & 1);
> msg_pdbg("FDOPSS %i, ", (tmp2 >> 13) & 1);
> msg_pdbg("SCIP %i, ", (tmp2 >> 5) & 1);
> msg_pdbg("BERASE %i, ", (tmp2 >> 3) & 3);
> msg_pdbg("AEL %i, ", (tmp2 >> 2) & 1);
> msg_pdbg("FCERR %i, ", (tmp2 >> 1) & 1);
> msg_pdbg("FDONE %i\n", (tmp2 >> 0) & 1);
> tmp = mmio_readl(ich_spibar + 0x50);
> msg_pdbg("0x50: 0x%08x (FRAP)\n", tmp);
> msg_pdbg("BMWAG 0x%02x, ", ICH_BMWAG(tmp));
> msg_pdbg("BMRAG 0x%02x, ", ICH_BMRAG(tmp));
> msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
> msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
> /* print out the FREGx registers along with FRAP access bits */
> for(i = 0; i < 5; i++)
> 	do_ich9_spi_frap(tmp, i);
> msg_pdbg("0x74: 0x%08x (PR0)\n",
> 	     mmio_readl(ich_spibar + 0x74));
> msg_pdbg("0x78: 0x%08x (PR1)\n",
> 	     mmio_readl(ich_spibar + 0x78));
> msg_pdbg("0x7C: 0x%08x (PR2)\n",
> 	     mmio_readl(ich_spibar + 0x7C));
> msg_pdbg("0x80: 0x%08x (PR3)\n",
> 	     mmio_readl(ich_spibar + 0x80));
> msg_pdbg("0x84: 0x%08x (PR4)\n",
> 	     mmio_readl(ich_spibar + 0x84));
> msg_pdbg("0x90: 0x%08x (SSFS, SSFC)\n",
> 	     mmio_readl(ich_spibar + 0x90));
> msg_pdbg("0x94: 0x%04x     (PREOP)\n",
> 	     mmio_readw(ich_spibar + 0x94));
> msg_pdbg("0x96: 0x%04x     (OPTYPE)\n",
> 	     mmio_readw(ich_spibar + 0x96));
> msg_pdbg("0x98: 0x%08x (OPMENU)\n",
> 	     mmio_readl(ich_spibar + 0x98));
> msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n",
> 	     mmio_readl(ich_spibar + 0x9C));
> ichspi_bbar = mmio_readl(ich_spibar + 0xA0);
> msg_pdbg("0xA0: 0x%08x (BBAR)\n",
> 	     ichspi_bbar);
> msg_pdbg("0xB0: 0x%08x (FDOC)\n",
> 	     mmio_readl(ich_spibar + 0xB0));
> if (tmp2 & (1 << 15)) {
> 	msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
> 	ichspi_lock = 1;
> }
> ich_init_opcodes();
> break;
> default:
> /* Nothing */
> break;
> }
> old = pci_read_byte(dev, 0xdc);
> msg_pdbg("SPI Read Configuration: ");
> new = (old >> 2) & 0x3;
> switch (new) {
> case 0:
> case 1:
> case 2:
> msg_pdbg("prefetching %sabled, caching %sabled, ",
> 	     (new & 0x2) ? "en" : "dis",
> 	     (new & 0x1) ? "dis" : "en");
> break;
> default:
> msg_pdbg("invalid prefetching/caching settings, ");
> break;
> }
>
> +}
>
> +int via_init_spi(struct pci_dev * dev)

whitespace after *
> +{
> uint32_t mmio_base;
> mmio_base = (pci_read_long(dev, 0xbc)) << 8;
> msg_pdbg("MMIO base at = 0x%x\n", mmio_base);
> ich_spibar = physmap("VT8237S MMIO registers", mmio_base, 0x70);
> msg_pdbg("0x6c: 0x%04x     (CLOCK/DEBUG)\n",
>      mmio_readw(ich_spibar + 0x6c));
> /* Not sure if it speaks all these bus protocols. */
> buses_supported = CHIP_BUSTYPE_LPC | CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_VIA;
> ich_init_opcodes();
> return 0;
>
> +}
>
> static int ich7_run_opcode(OPCODE op, uint32_t offset,
>    		   uint8_t datalength, uint8_t * data, int maxdata)
>  {

Looks good otherwise.

Regards,
Carl-Daniel

[PATCH 1/3] Move Intel SPI initialisation to ichspi.c

[Michael Karcher]

Am Sonntag, den 18.07.2010, 17:24 +0200 schrieb Carl-Daniel Hailfinger:
> On 18.07.2010 16:19, Michael Karcher wrote:
> > ich_init_spi(dev, tmp, rcrb, ich_generation);
>
> check for the return value?

And do what in case of error? ich_init_spi should set CHIP_BUSTYPE_SPI
only if it worked, and ich_init_spi also knows better what failed if
initializing didn't work. Of course one can pass the error code to make
the chipset enable fail, but is it really fatal? (let me remind you:
this code path is also taken for FWH chips)
> > +#define ICH_BMWAG(x) ((x >> 24) & 0xff)
> > +#define ICH_BMRAG(x) ((x >> 16) & 0xff)
> > +#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)
> > +{
> > const char *access_names[4] = {
> > "locked", "read-only", "write-only", "read-write"
> > };
> > const char *region_names[5] = {
> > "Flash Descriptor", "BIOS", "Management Engine",
> > "Gigabit Ethernet", "Platform Data"
> > };
> > uint32_t base, limit;
> > int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
> >       (((ICH_BRRA(frap) >> i) & 1) << 0);
> > int offset = 0x54 + i * 4;
> > uint32_t freg = mmio_readl(ich_spibar + offset);
> > msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
> >      offset, freg, i, region_names[i]);
> > base  = ICH_FREG_BASE(freg);
> > limit = ICH_FREG_LIMIT(freg);
> > if (base == 0x1fff && limit == 0) {
> > /* this FREG is disabled */
> > msg_pdbg("%s region is unused.\n", region_names[i]);
> > return;
> > }
> > msg_pdbg("0x%08x-0x%08x is %s\n",
> >     (base << 12), (limit << 12) | 0x0fff,
> >     access_names[rwperms]);
> >
> > +}
>
> Is the FRAP/FREG stuff really SPI specific? If not, it should stay in
> chipset_enable.c AFAICS.

The whole region stuff (and flash descriptors at all) are SPI specific,
as far as I can tell.
> > +void ich_init_spi(struct pci_dev *dev, uint32_t base, void * rcrb,
>
> whitespace after * and at end of line
> Can you make this function return an error value?

Did so, but always returns success for now.
> Is adding BUSTYPE_FWH to buses_supported a good idea here in ichspi.c?

not really. Moved the FWH/no FWH handling back to chipset_enable.c

Thanks for the review, new version attached.

Regards,
  Michael Karcher

[PATCH 1/3] Move Intel SPI initialisation to ichspi.c

[Carl-Daniel Hailfinger]

On 18.07.2010 18:29, Michael Karcher wrote:
> Am Sonntag, den 18.07.2010, 17:24 +0200 schrieb Carl-Daniel Hailfinger:
> > On 18.07.2010 16:19, Michael Karcher wrote:
> > > ich_init_spi(dev, tmp, rcrb, ich_generation);
> >
> > check for the return value?
>
> And do what in case of error? ich_init_spi should set CHIP_BUSTYPE_SPI
> only if it worked, and ich_init_spi also knows better what failed if
> initializing didn't work. Of course one can pass the error code to make
> the chipset enable fail, but is it really fatal? (let me remind you:
> this code path is also taken for FWH chips)

In my latest Nvidia MCP SPI patch, I'm solving this with an additional
boolean parameter want_spi. If the chipset is strapped to SPI, I set
want_spi=1 and return an error code if SPI init fails. If the chipset is
not strapped to SPI, want_spi is set to 0 and the code tries SPI init
but will never return an error.
> > > +#define ICH_BMWAG(x) ((x >> 24) & 0xff)
> > > +#define ICH_BMRAG(x) ((x >> 16) & 0xff)
> > > +#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)
> > > +{
> > > const char *access_names[4] = {
> > > "locked", "read-only", "write-only", "read-write"
> > > };
> > > const char *region_names[5] = {
> > > "Flash Descriptor", "BIOS", "Management Engine",
> > > "Gigabit Ethernet", "Platform Data"
> > > };
> > > uint32_t base, limit;
> > > int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
> > >       (((ICH_BRRA(frap) >> i) & 1) << 0);
> > > int offset = 0x54 + i * 4;
> > > uint32_t freg = mmio_readl(ich_spibar + offset);
> > > msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
> > >      offset, freg, i, region_names[i]);
> > > base  = ICH_FREG_BASE(freg);
> > > limit = ICH_FREG_LIMIT(freg);
> > > if (base == 0x1fff && limit == 0) {
> > > /* this FREG is disabled */
> > > msg_pdbg("%s region is unused.\n", region_names[i]);
> > > return;
> > > }
> > > msg_pdbg("0x%08x-0x%08x is %s\n",
> > >     (base << 12), (limit << 12) | 0x0fff,
> > >     access_names[rwperms]);
> > >
> > > +}
> >
> > Is the FRAP/FREG stuff really SPI specific? If not, it should stay in
> > chipset_enable.c AFAICS.
>
> The whole region stuff (and flash descriptors at all) are SPI specific,
> as far as I can tell.

OK, thanks.
> > > +void ich_init_spi(struct pci_dev *dev, uint32_t base, void * rcrb,
> >
> > whitespace after * and at end of line
> > Can you make this function return an error value?
>
> Did so, but always returns success for now.
> > Is adding BUSTYPE_FWH to buses_supported a good idea here in ichspi.c?
>
> not really. Moved the FWH/no FWH handling back to chipset_enable.c
>
> Thanks for the review, new version attached.

Looks good. The want_spi stuff mentioned above may be desirable, but I
don't want to force this on you.

One additional comment: Could you move do_ich9_spi_frap down a bit so it
is in the same region as the init functions?
With that addressed, your patch is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[PATCH 1/3] Move Intel SPI initialisation to ichspi.c

[Michael Karcher]

Am Donnerstag, den 22.07.2010, 17:09 +0200 schrieb Carl-Daniel
Hailfinger:
> One additional comment: Could you move do_ich9_spi_frap down a bit so it
> is in the same region as the init functions?

Did already do so in my private tree. Committed as r1098. Thanks for
your review.

Regards,
  Michael Karcher