Re: [LinuxBIOS] [PATCH] improved SPI flash support (restructured)

[Uwe Hermann]

On Mon, Oct 01, 2007 at 02:53:35PM +0200, Carl-Daniel Hailfinger wrote:
> Index: util/flashrom/flash.h
>
> --- util/flashrom/flash.h	(Revision 2814)
> +++ util/flashrom/flash.h	(Arbeitskopie)
> @@ -55,6 +55,8 @@
>  /* Please keep this list sorted alphabetically by manufacturer. The first
> entry of each section should be the manufacturer ID, followed by the
> list of devices from that manufacturer (sorted by device IDs).
> All LPC/FWH parts (parallel flash) have 8-bit device IDs.
> All SPI parts have 16-bit device IDs.
> */
>
> #define AMD_ID			0x01	/* AMD */
> @@ -68,8 +70,31 @@
>  #define AT_29C040A		0xA4
>  #define AT_29C020		0xDA
>
> +#define EON_ID			0x1C
> +/* EN25 chips are SPI, first byte of device id is memory type,
> second byte of device id is log(bitsize)-9 */

id -> ID
> +#define EN_25B05		0x2010	/* 2^19 kbit or 2^16 kByte */
> +#define EN_25B10		0x2011
> +#define EN_25B20		0x2012
> +#define EN_25B40		0x2013
> +#define EN_25B80		0x2014
> +#define EN_25B16		0x2015
> +#define EN_25B32		0x2016
>
> #define MX_ID			0xC2	/* Macronix (MX) */
>  #define MX_29F002		0xB0
> +/* MX25L chips are SPI, first byte of device id is memory type,
> second byte of device id is log(bitsize)-9 */

id -> ID
> +#define MX_25L512		0x2010	/* 2^19 kbit or 2^16 kByte */
> +#define MX_25L1005		0x2011
> +#define MX_25L2005		0x2012
> +#define MX_25L4005		0x2013	/* MX25L4005{,A} */
> +#define MX_25L8005		0x2014
> +#define MX_25L1605		0x2015	/* MX25L1605{,A,D} */
> +#define MX_25L3205		0x2016	/* MX25L3205{,A} */
> +#define MX_25L6405		0x2017	/* MX25L3205{,D} */
> +#define MX_25L1635D		0x2415
> +#define MX_25L3235D		0x2416
>
> #define SHARP_ID		0xB0	/* Sharp */
>  #define SHARP_LHF00L04		0xCF
> @@ -182,6 +207,8 @@
>  int linuxbios_init(void);
>  extern char *lb_part, *lb_vendor;
>
> +int probe_spi(struct flashchip *flash);
>
> /* 82802ab.c */
>  int probe_82802ab(struct flashchip *flash);
>  int erase_82802ab(struct flashchip *flash);
> Index: util/flashrom/board_enable.c
> ===================================================================
> --- util/flashrom/board_enable.c	(Revision 2814)
> +++ util/flashrom/board_enable.c	(Arbeitskopie)
> @@ -30,6 +30,15 @@
>  #include <string.h>
>  #include "flash.h"
>
> +#define ITE_SUPERIO_PORT1	0x2e
> +#define ITE_SUPERIO_PORT2	0x4e
>
> +#define JEDEC_RDID	{0x9f}
> +#define JEDEC_RDID_OUTSIZE	0x01
> +#define JEDEC_RDID_INSIZE	0x03
>
> +static uint16_t it8716f_flashport = 0;
>
> /* Generic Super I/O helper functions */
>  uint8_t regval(uint16_t port, uint8_t reg)
>  {
> @@ -51,7 +60,7 @@
>    outb(0x87, port);
>    outb(0x01, port);
>    outb(0x55, port);
> if (port == 0x2e)
> if (port == ITE_SUPERIO_PORT1)
> outb(0x55, port);
> else
> outb(0xaa, port);
>
> @@ -96,35 +105,97 @@
>    return flashport;
>  }
>
> -static void it8716_serial_rdid(uint16_t port)
> +/* The IT8716F only supports commands with length 1,2,4,5 bytes including
> command byte and can not read more than 3 bytes from the device.
> This function expects writearr[0] to be the first byte sent to the device,
> whereas the IT8716F splits commands internally into address and non-address
> commands with the address in inverse wire order. That's why the register
> ordering in case 4 and 5 may seem strange. */

Please make all multi-line comments look the same as in the rest of the
code:

/* Foo
 * Bar
 */

Or, if we want to use the exact Linux-style:

/*
 * Foo
 * Bar
 */
> +static int it8716f_spi_command(uint16_t port, unsigned char writecnt, unsigned char readcnt, const unsigned char *writearr, unsigned char *readarr)

As per another mail, writecnt/readcnt/writearr/readarr seems to have a
dont-care size, so I think we can make them all 'int' (or 'unsigned int'
if they can't or shouldn't get negative).

I think using the "native" int type is the best solution in general, it
gives the compiler maximum freedom to optimize (not that it would matter
much here).
> Index: util/flashrom/flashchips.c
>
> --- util/flashrom/flashchips.c	(Revision 2814)
> +++ util/flashrom/flashchips.c	(Arbeitskopie)
> @@ -38,6 +38,8 @@
>     probe_jedec,	erase_chip_jedec, write_jedec},
>    {"Mx29f002",	MX_ID,		MX_29F002,	256, 64 * 1024,
>     probe_29f002,	erase_29f002, 	write_29f002},
> {"MX25L4005",	MX_ID,		MX_25L4005,	512, 4 * 1024,
> probe_spi,	NULL,		NULL},

All the other chips from above are not added?
> {"SST29EE020A", SST_ID,		SST_29EE020A,	256, 128,
>     probe_jedec,	erase_chip_jedec, write_jedec},
>    {"SST28SF040A", SST_ID,		SST_28SF040,	512, 256,

With the above fixes:
Acked-by: Uwe Hermann

Uwe.