[flashrom] [PATCH 1/2] ichspi: remove dependency on spi_programmer from generate_opcodes and program_opcodes

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Nov 2 02:44:17 CET 2011


Am 26.10.2011 15:35 schrieb Stefan Tauner:
> This allows them to be called before programmer registration.
> ---
> i think it might be a good idea to get rid of the whole
> switch(spi_programmer->type)
> pattern and create a file-scope static ich_generation variable instead
> of using the type member and the ich_generation parameters everywhere.

Absolutely agreed.
The only possible generic problem with that approach would be the
registration of multiple ICH-style SPI programmers, but unless we see
boards with two active ICH-style southbridges I think we can assume the
static variable handles it well. (Note that boards with multiple MCP55
southbridges exist, but only one southbridge has an attached flash chip.)

There might be an issue for those who want to use flashrom as a
standalone tool (e.g. on top of  libpayload) where heap allocations for
static variables are unwanted, but that's a huge can of worms and I'd
rather ignore that issue until either static variables work well in that
environment or until someone hacks a way around static variables being a
problem there.

> the type member is enough most of the time to derive the wanted
> information, but
> - not always (e.g. ich_set_bbar)
> - only available after registration, which we want to delay till the end
>   of init.
> - we really want to distinguish between chipset version-grained
>   attributes which are not reflected by the registered programmer.

Indeed. So if you could rework the patch to use your static variable
suggestion, it would reduce patch size and make the code more readable.


Regards,
Carl-Daniel


> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> ---
>  ichspi.c |   44 +++++++++++++++++++++++++++-----------------
>  1 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/ichspi.c b/ichspi.c
> index afa420b..bc03554 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -225,8 +225,8 @@ static uint16_t REGREAD8(int X)
>  /* Common SPI functions */
>  static int find_opcode(OPCODES *op, uint8_t opcode);
>  static int find_preop(OPCODES *op, uint8_t preop);
> -static int generate_opcodes(OPCODES * op);
> -static int program_opcodes(OPCODES *op, int enable_undo);
> +static int generate_opcodes(int ich_generation, OPCODES * op);
> +static int program_opcodes(int ich_generation, OPCODES *op, int enable_undo);
>  static int run_opcode(OPCODE op, uint32_t offset,
>  		      uint8_t datalength, uint8_t * data);
>  
> @@ -410,7 +410,18 @@ static int reprogram_opcode_on_the_fly(uint8_t opcode, unsigned int writecnt, un
>  		int oppos=2;	// use original JEDEC_BE_D8 offset
>  		curopcodes->opcode[oppos].opcode = opcode;
>  		curopcodes->opcode[oppos].spi_type = spi_type;
> -		program_opcodes(curopcodes, 0);
> +		switch (spi_programmer->type) {
> +			case SPI_CONTROLLER_ICH7:
> +			case SPI_CONTROLLER_VIA:
> +				program_opcodes(7, curopcodes, 0);
> +				break;
> +			case SPI_CONTROLLER_ICH9:
> +				program_opcodes(9, curopcodes, 0);
> +				break;
> +			default:
> +				msg_perr("%s: unsupported chipset\n", __func__);
> +				return -1;
> +		}
>  		oppos = find_opcode(curopcodes, opcode);
>  		msg_pdbg ("on-the-fly OPCODE (0x%02X) re-programmed, op-pos=%d\n", opcode, oppos);
>  		return oppos;
> @@ -443,7 +454,7 @@ static int find_preop(OPCODES *op, uint8_t preop)
>  }
>  
>  /* Create a struct OPCODES based on what we find in the locked down chipset. */
> -static int generate_opcodes(OPCODES * op)
> +static int generate_opcodes(int ich_generation, OPCODES * op)
>  {
>  	int a;
>  	uint16_t preop, optype;
> @@ -498,7 +509,7 @@ static int generate_opcodes(OPCODES * op)
>  	return 0;
>  }
>  
> -static int program_opcodes(OPCODES *op, int enable_undo)
> +static int program_opcodes(int ich_generation, OPCODES *op, int enable_undo)
>  {
>  	uint8_t a;
>  	uint16_t preop, optype;
> @@ -529,9 +540,8 @@ static int program_opcodes(OPCODES *op, int enable_undo)
>  	}
>  
>  	msg_pdbg("\n%s: preop=%04x optype=%04x opmenu=%08x%08x\n", __func__, preop, optype, opmenu[0], opmenu[1]);
> -	switch (spi_programmer->type) {
> -	case SPI_CONTROLLER_ICH7:
> -	case SPI_CONTROLLER_VIA:
> +	switch (ich_generation) {
> +	case 7:
>  		/* Register undo only for enable_undo=1, i.e. first call. */
>  		if (enable_undo) {
>  			rmmio_valw(ich_spibar + ICH7_REG_PREOP);
> @@ -544,7 +554,10 @@ static int program_opcodes(OPCODES *op, int enable_undo)
>  		mmio_writel(opmenu[0], ich_spibar + ICH7_REG_OPMENU);
>  		mmio_writel(opmenu[1], ich_spibar + ICH7_REG_OPMENU + 4);
>  		break;
> -	case SPI_CONTROLLER_ICH9:
> +	case 8:
> +	case 9:
> +	case 10:
> +	default:		/* Future version might behave the same */
>  		/* Register undo only for enable_undo=1, i.e. first call. */
>  		if (enable_undo) {
>  			rmmio_valw(ich_spibar + ICH9_REG_PREOP);
> @@ -557,9 +570,6 @@ static int program_opcodes(OPCODES *op, int enable_undo)
>  		mmio_writel(opmenu[0], ich_spibar + ICH9_REG_OPMENU);
>  		mmio_writel(opmenu[1], ich_spibar + ICH9_REG_OPMENU + 4);
>  		break;
> -	default:
> -		msg_perr("%s: unsupported chipset\n", __func__);
> -		return -1;
>  	}
>  
>  	return 0;
> @@ -652,7 +662,7 @@ static void ich_fill_data(const uint8_t *data, int len, int reg0_off)
>   *
>   * It should be called before ICH sends any spi command.
>   */
> -static int ich_init_opcodes(void)
> +static int ich_init_opcodes(int ich_generation)
>  {
>  	int rc = 0;
>  	OPCODES *curopcodes_done;
> @@ -663,11 +673,11 @@ static int ich_init_opcodes(void)
>  	if (ichspi_lock) {
>  		msg_pdbg("Reading OPCODES... ");
>  		curopcodes_done = &O_EXISTING;
> -		rc = generate_opcodes(curopcodes_done);
> +		rc = generate_opcodes(ich_generation, curopcodes_done);
>  	} else {
>  		msg_pdbg("Programming OPCODES... ");
>  		curopcodes_done = &O_ST_M25P;
> -		rc = program_opcodes(curopcodes_done, 1);
> +		rc = program_opcodes(ich_generation, curopcodes_done, 1);
>  	}
>  
>  	if (rc) {
> @@ -1343,6 +1353,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>  
>  	/* Assign Virtual Address */
>  	ich_spibar = rcrb + spibar_offset;
> +	ich_init_opcodes(ich_generation);
>  
>  	switch (ich_generation) {
>  	case 7:
> @@ -1383,7 +1394,6 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>  		}
>  		ich_set_bbar(ich_generation, 0);
>  		register_spi_programmer(&spi_programmer_ich7);
> -		ich_init_opcodes();
>  		break;
>  	case 8:
>  	case 9:
> @@ -1557,7 +1567,7 @@ int via_init_spi(struct pci_dev *dev)
>  	}
>  
>  	ich_set_bbar(7, 0);
> -	ich_init_opcodes();
> +	ich_init_opcodes(7);
>  
>  	return 0;
>  }


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





More information about the flashrom mailing list