Re: [coreboot] Add support for SPI-Chips on ICH9 to flash rom / corrected

[Peter Stuge]

Hi Claus,

On Tue, May 13, 2008 at 09:56:06AM +0200, Claus Gindhart wrote:
> This patch adds support for the SPI Chips Atmel AT25DF32,
> ST M25P80, ST M25P16, M25P32, ST M25P64 attached to the ICH9.
> Because chipset plays an important role in communicating with these
> chips, it was not possible to integrate this support into the already 
> existing spi.c
> This module was tested with the devices as mentioned above, but
> it should be easy to expand it for other SPI chips attached to an ICH.
>
> Signed-off-by: Claus Gindhart

Thanks for the patch!

This is a big improvement over the last one!
> The previous patch was buggy; there was a merging conflict in my
> local code repository, which was incuded into the patch; sorry for
> that.

No problem as long as you can send a new one. :)
> I did not yet set the test status flag to any TEST_OK for my
> supporting function, because yet i dont know, who has the
> permission to do that; is it the initiator of this code, or any
> acknowledging person ?

It is quite simple; anyone who has tested the program with a chip can
send a notice to the list or [e-mail] (though I don't
know for sure that that address works yet) and that information will
be trusted and commited immediately. No need for Ack since that does
not make much sense for a test result.

So please do add TEST_OK_ flags for any chips that you have used with
flashrom.

If you prefer to just list the chips and what you have tested that is
fine, but a patch would be ideal.
> Index: sst_fwhub.c

This file should not be included in the commit, but it can be removed
manually. Please update your working copy too Claus so the same hunk
isn't included by mistake in future patches. Thanks!
> Index: ichspi.c

..
> +#undef DEVELOPPERS_DEBUG

There is a printf_debug() function that could be used instead of this
define. It prints output when -V is specified on the command line.
> +static OPCODES *curopcodes;
> +static inline uint8_t REGREAD8(int X)
> +static inline uint16_t REGREAD16(int X)
> +static inline uint32_t REGREAD32(int X)
> +#define REGWRITE32(X,Y) (*(uint32_t *)(curflash->virtual_registers+X)=Y)
> +#define REGWRITE16(X,Y) (*(uint16_t *)(curflash->virtual_registers+X)=Y)
> +#define REGWRITE8(X,Y) (*(uint8_t *)(curflash->virtual_registers+X)=Y)
>
> +/* Common SPI functions */
> +static int ProgramOpcodes(OPCODES * op);
> +static int RunOpcode(uint8_t nr, OPCODE op, uint32_t offset, uint8_t datalength,
>      uint8_t * data);
>
> +static int read_page_ichspi(struct flashchip *flash, uint8_t * buf, int Offset);
> +static int is_supported_chipset();

Maybe this will feel very petty, but I do not like this naming style.

Please use only lowercase letters, and use underscore to separate
words, for all types, functions and parameters.

Restrict uppercase names for #defines only.

I think this is important for readability of the program as a whole,
though since this is one isolated file that will eventually be
replaced anyway with better generic SPI support in flashrom, it might
be possible to convince me to commit this as is. :)
> Index: ichspiacc.h

..
> +typedef struct _SPIFLASH {
> const char *name;
> OPCODES opcodes;
> uint32_t pagesize;
> uint32_t size;
>
> +#if 0
> int (*erase) (struct _SPIFLASH * myflash, uint32 Offset, uint32 sectors)
> int (*write) (struct _SPIFLASH * myflash, uint32 Offset, uint32 number,
>       uint8_t * bytes)
> int (*read) (struct _SPIFLASH * myflash, uint32 Offset, uint32 number,
>      uint8_t * bytes)
>
> +#endif
> +} SPIFLASH;

This struct is never used - please remove it.

Claus, if you can address my comments above, I will be happy to Ack
and commit this patch!

//Peter