Re: [LinuxBIOS] [PATCH] flashrom: SPI serial flash: The sector/block erase pitfalls

[Uwe Hermann]

On Thu, Oct 18, 2007 at 10:32:01PM +0200, Carl-Daniel Hailfinger wrote:
> Index: util/flashrom/spi.c
>
> --- util/flashrom/spi.c	(Revision 2876)
> +++ util/flashrom/spi.c	(Arbeitskopie)
> @@ -45,17 +45,31 @@
>  #define JEDEC_WRDI_OUTSIZE	0x01
>  #define JEDEC_WRDI_INSIZE	0x00
>
> -/* Both Chip Erase commands below should work */
> -/* Chip Erase 0x60 */
> +/* Chip Erase 0x60 is supported by Macronix/SST chips */

Missing full stop at end of sentence. Also in other lines below.
> #define JEDEC_CE_1	{0x60};
>  #define JEDEC_CE_1_OUTSIZE	0x01
>  #define JEDEC_CE_1_INSIZE	0x00
>
> -/* Chip Erase 0xc7 */
> +/* Chip Erase 0xc7 is supported by EON/Macronix chips */
>  #define JEDEC_CE_2	{0xc7};
>  #define JEDEC_CE_2_OUTSIZE	0x01
>  #define JEDEC_CE_2_INSIZE	0x00
>
> +/* Block Erase 0x52 is supported by SST chips */
> +#define JEDEC_BE_1	{0x52};
> +#define JEDEC_BE_1_OUTSIZE	0x04
> +#define JEDEC_BE_1_INSIZE	0x00
>
> +/* Block Erase 0xd8 is supported by EON/Macronix chips */
> +#define JEDEC_BE_2	{0xd8};
> +#define JEDEC_BE_2_OUTSIZE	0x04
> +#define JEDEC_BE_2_INSIZE	0x00
>
> +/* Sector Erase 0x20 is supported by Macronix/SST chips*/

Missing space and missing full stop.
> +#define JEDEC_SE	{0x20};
> +#define JEDEC_SE_OUTSIZE	0x04
> +#define JEDEC_SE_INSIZE	0x00
>
> /* Read Status Register */
>  #define JEDEC_RDSR	{0x05};
>  #define JEDEC_RDSR_OUTSIZE	0x01
> @@ -269,13 +283,52 @@
>
> generic_spi_write_enable();
>    /* Send CE (Chip Erase) */
> generic_spi_command(1, 0, cmd, NULL);
> generic_spi_command(JEDEC_CE_2_OUTSIZE, JEDEC_CE_2_INSIZE, cmd, NULL);

Yep, nice.
> /* Wait until the Write-In-Progress bit is cleared */
>    while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>    	sleep(1);
>    return 0;
>  }
>
> +/* Block size is usually
> 64k for Macronix
> 32k for SST
> 4-32k non-uniform for EON
> */

Please make it a Doxygen comment and describe what the function does.
> +int generic_spi_block_erase(struct flashchip *flash, unsigned long addr)

"flash" can be const?
> +{
> unsigned char cmd[JEDEC_BE_2_OUTSIZE] = JEDEC_BE_2;

uint8_t
> cmd[1] = (addr & 0x00ff0000) >> 16;
> cmd[2] = (addr & 0x0000ff00) >> 8;
> cmd[3] = (addr & 0x000000ff);
> generic_spi_write_enable();
> /* Send BE (Block Erase) */
> generic_spi_command(JEDEC_BE_2_OUTSIZE, JEDEC_BE_2_INSIZE, cmd, NULL);
> /* Wait until the Write-In-Progress bit is cleared */

Missing full stop.
> while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> usleep(100 * 1000);

Is the length of this delay documented in the datasheet or is it just an
estimate we hope will work?
> return 0;
>
> +}
>
> +/* Sector size is usually 4k, though Macronix eliteflash has 64k */
> +int generic_spi_sector_erase(struct flashchip *flash, unsigned long addr)

constify "flash"?
> +{
> unsigned char cmd[JEDEC_SE_OUTSIZE] = JEDEC_SE;

uint8_t
> cmd[1] = (addr & 0x00ff0000) >> 16;
> cmd[2] = (addr & 0x0000ff00) >> 8;
> cmd[3] = (addr & 0x000000ff);
> generic_spi_write_enable();
> /* Send SE (Sector Erase) */
> generic_spi_command(JEDEC_SE_OUTSIZE, JEDEC_SE_INSIZE, cmd, NULL);
> /* Wait until the Write-In-Progress bit is cleared */

Missing full stop.
> while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> usleep(10 * 1000);

Lower delay than above. By design? What does the datasheet say?
> return 0;
>
> +}
>
> +/* Page size is usually 256 bytes */
>  void it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) {
>    int i;

Looks good IMO. With the above fixes:

Acked-by: Uwe Hermann

Uwe.