[U-Boot] [PATCH] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

Simon Goldschmidt simon.k.r.goldschmidt at gmail.com
Fri Oct 5 17:54:13 UTC 2018


On 04.10.2018 17:16, Stefan Roese wrote:
> Some SPI NOR chips only support 4-byte mode addressing. Here the default
> 3-byte mode does not work and leads to incorrect accesses. This patch
> now reads the 4-byte mode status bit (in this case in the CR register
> of the Macronix SPI NOR) and configures the SPI transfers accordingly.
>
> This was noticed on the LinkIt Smart 7688 modul, which is equipped with
> an Macronix MX25L25635F device. But this device does *NOT* support
> switching to 3-byte mode via the EX4B command.
>
> This should also work when the bootrom configures the SPI flash to
> 4-byte mode and runs U-Boot after this. U-Boot should dectect this
S/dectect/detect/
> mode (if the 4-byte mode detection is available for this chip) and
> use the correct OPs in this case.
>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Jagan Teki <jagan at openedev.com>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

I've successfully tested this on my socfpga_cyclone5 board. But I had to 
implement the detection for 'stmicro' flash. See below.

> ---
> Please note that this patch replaces this one:
>
> [PATCH] sf: Add SPI_FLASH_4BYTE_MODE_ONLY option to support 4-byte mode
>
> which introduced a CONFIG option to enable this 4-byte mode. In discussion
> with Simon we agreed, that an auto detection would be much better. Hence
> this new version now.
>
>   drivers/mtd/spi/sf_internal.h |  3 +-
>   drivers/mtd/spi/spi_flash.c   | 94 +++++++++++++++++++++++++++--------
>   include/spi_flash.h           |  5 ++
>   3 files changed, 81 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> index 4f63cacc64..eb076401d1 100644
> --- a/drivers/mtd/spi/sf_internal.h
> +++ b/drivers/mtd/spi/sf_internal.h
> @@ -26,7 +26,8 @@ enum spi_nor_option_flags {
>   };
>   
>   #define SPI_FLASH_3B_ADDR_LEN		3
> -#define SPI_FLASH_CMD_LEN		(1 + SPI_FLASH_3B_ADDR_LEN)
> +#define SPI_FLASH_4B_ADDR_LEN		4
> +#define SPI_FLASH_CMD_MAX_LEN		(1 + SPI_FLASH_4B_ADDR_LEN)
>   #define SPI_FLASH_16MB_BOUN		0x1000000
>   
>   /* CFI Manufacture ID's */
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index c159124259..357b4f2422 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -20,12 +20,19 @@
>   
>   #include "sf_internal.h"
>   
> -static void spi_flash_addr(u32 addr, u8 *cmd)
> +static void spi_flash_addr(struct spi_flash *flash, u32 addr, u8 *cmd)
>   {
>   	/* cmd[0] is actual command */
> -	cmd[1] = addr >> 16;
> -	cmd[2] = addr >> 8;
> -	cmd[3] = addr >> 0;
> +	if (flash->in_4byte_mode) {
> +		cmd[1] = addr >> 24;
> +		cmd[2] = addr >> 16;
> +		cmd[3] = addr >> 8;
> +		cmd[4] = addr >> 0;
> +	} else {
> +		cmd[1] = addr >> 16;
> +		cmd[2] = addr >> 8;
> +		cmd[3] = addr >> 0;
> +	}
>   }
>   
>   static int read_sr(struct spi_flash *flash, u8 *rs)
> @@ -110,6 +117,33 @@ static int write_cr(struct spi_flash *flash, u8 wc)
>   }
>   #endif
>   
> +#if defined(CONFIG_SPI_FLASH_MACRONIX)
> +static bool flash_in_4byte_mode(struct spi_flash *flash)

I renamed this function 'flash_in_4byte_mode_macronix()' as the current 
generic function name gives me name clashes when compiling with MACRONIX 
and STMICRO enabled, see below.

> +{
> +	int ret;
> +	u8 cr;
> +	u8 cmd;
> +
> +	cmd = 0x15;	/* Macronix: read configuration register RDCR */
> +	ret = spi_flash_read_common(flash, &cmd, 1, &cr, 1);
> +	if (ret < 0) {
> +		debug("SF: fail to read config register\n");
> +		return false;
> +	}
> +
> +	/* Return true, if 4-byte mode is enabled */
> +	if (cr & BIT(5))
> +		return true;
> +
> +	return false;
> +}
> +#else

Here, I've added this function for STMICRO:
     #if defined(CONFIG_SPI_FLASH_STMICRO)
     static bool flash_in_4byte_mode_stmicro(struct spi_flash *flash)
     {
         int ret;
         u8 fsr;
         u8 cmd;

         cmd = 0x70;    /* STMicro/Micron: read flag status register */
         ret = spi_flash_read_common(flash, &cmd, 1, &fsr, 1);
         if (ret < 0) {
             debug("SF: fail to read config register\n");
             return false;
         }

         printf("flash_in_4byte_mode_stmicro(): fsr: 0x%02x\n", fsr);
         /* Return true, if 4-byte mode is enabled */
         if (fsr & BIT(0)) {
             return true;
         }

         return false;
     }
     #endif

> +static bool flash_in_4byte_mode(struct spi_flash *flash)
> +{
> +	return false;
> +}
> +#endif
> +
>   #ifdef CONFIG_SPI_FLASH_BAR
>   /*
>    * This "clean_bar" is necessary in a situation when one was accessing
> @@ -314,7 +348,7 @@ int spi_flash_write_common(struct spi_flash *flash, const u8 *cmd,
>   int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>   {
>   	u32 erase_size, erase_addr;
> -	u8 cmd[SPI_FLASH_CMD_LEN];
> +	u8 cmd[SPI_FLASH_CMD_MAX_LEN];
>   	int ret = -1;
>   
>   	erase_size = flash->erase_size;
> @@ -344,12 +378,13 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>   		if (ret < 0)
>   			return ret;
>   #endif
> -		spi_flash_addr(erase_addr, cmd);
> +		spi_flash_addr(flash, erase_addr, cmd);
>   
>   		debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
>   		      cmd[2], cmd[3], erase_addr);
>   
> -		ret = spi_flash_write_common(flash, cmd, sizeof(cmd), NULL, 0);
> +		ret = spi_flash_write_common(flash, cmd, flash->cmdlen,
> +					     NULL, 0);
>   		if (ret < 0) {
>   			debug("SF: erase failed\n");
>   			break;
> @@ -373,7 +408,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
>   	unsigned long byte_addr, page_size;
>   	u32 write_addr;
>   	size_t chunk_len, actual;
> -	u8 cmd[SPI_FLASH_CMD_LEN];
> +	u8 cmd[SPI_FLASH_CMD_MAX_LEN];
>   	int ret = -1;
>   
>   	page_size = flash->page_size;
> @@ -404,15 +439,15 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
>   
>   		if (spi->max_write_size)
>   			chunk_len = min(chunk_len,
> -					spi->max_write_size - sizeof(cmd));
> +					spi->max_write_size - flash->cmdlen);
>   
> -		spi_flash_addr(write_addr, cmd);
> +		spi_flash_addr(flash, write_addr, cmd);
>   
>   		debug("SF: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n",
>   		      buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
>   
> -		ret = spi_flash_write_common(flash, cmd, sizeof(cmd),
> -					buf + actual, chunk_len);
> +		ret = spi_flash_write_common(flash, cmd, flash->cmdlen,
> +					     buf + actual, chunk_len);
>   		if (ret < 0) {
>   			debug("SF: write failed\n");
>   			break;
> @@ -469,9 +504,11 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
>   {
>   	struct spi_slave *spi = flash->spi;
>   	u8 *cmd, cmdsz;
> -	u32 remain_len, read_len, read_addr;
> +	u64 remain_len;
> +	u32 read_len, read_addr;
>   	int bank_sel = 0;
>   	int ret = -1;
> +	int shift;
>   
>   	/* Handle memory-mapped SPI */
>   	if (flash->memory_map) {
> @@ -487,7 +524,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
>   		return 0;
>   	}
>   
> -	cmdsz = SPI_FLASH_CMD_LEN + flash->dummy_byte;
> +	cmdsz = flash->cmdlen + flash->dummy_byte;
>   	cmd = calloc(1, cmdsz);
>   	if (!cmd) {
>   		debug("SF: Failed to allocate cmd\n");
> @@ -508,8 +545,13 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
>   			return ret;
>   		bank_sel = flash->bank_curr;
>   #endif
> -		remain_len = ((SPI_FLASH_16MB_BOUN << flash->shift) *
> -				(bank_sel + 1)) - offset;
> +		shift = flash->shift;
> +		if (flash->in_4byte_mode)
> +			shift += 8;
> +
> +		remain_len = (((u64)SPI_FLASH_16MB_BOUN << shift) *
> +			      (bank_sel + 1)) - offset;
> +
>   		if (len < remain_len)
>   			read_len = len;
>   		else
> @@ -518,7 +560,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
>   		if (spi->max_read_size)
>   			read_len = min(read_len, spi->max_read_size);
>   
> -		spi_flash_addr(read_addr, cmd);
> +		spi_flash_addr(flash, read_addr, cmd);
>   
>   		ret = spi_flash_read_common(flash, cmd, cmdsz, data, read_len);
>   		if (ret < 0) {
> @@ -1160,6 +1202,15 @@ int spi_flash_scan(struct spi_flash *flash)
>   		write_sr(flash, sr);
>   	}
>   
> +	/* Set default value for cmd length */
> +	flash->cmdlen = 1 + SPI_FLASH_3B_ADDR_LEN;
> +	if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_MACRONIX) {
> +		if (flash_in_4byte_mode(flash)) {
> +			flash->in_4byte_mode = true;
> +			flash->cmdlen = 1 + SPI_FLASH_4B_ADDR_LEN;
> +		}
> +	}

And here, I inserted:
     #if defined(CONFIG_SPI_FLASH_STMICRO)
         if (JEDEC_MFR(info) == SPI_FLASH_CFI_MFR_STMICRO) {
             if (flash_in_4byte_mode_stmicro(flash)) {
                 flash->in_4byte_mode = true;
                 flash->cmdlen = 1 + SPI_FLASH_4B_ADDR_LEN;
             }
         }
     #endif

After that, it works fine. I'd probably always call 
'flash_in_4byte_mode()' and implement the vendor check in that function 
to make 'spi_flash_scan()' easier to read...

I'd be happy if you could integrate this into your patch as a v2!


Simon

> +
>   	flash->name = info->name;
>   	flash->memory_map = spi->memory_map;
>   
> @@ -1311,14 +1362,17 @@ int spi_flash_scan(struct spi_flash *flash)
>   	print_size(flash->size, "");
>   	if (flash->memory_map)
>   		printf(", mapped at %p", flash->memory_map);
> +	if (flash->in_4byte_mode)
> +		printf(" (4-byte mode)");
>   	puts("\n");
>   #endif
>   
>   #ifndef CONFIG_SPI_FLASH_BAR
> -	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
> -	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
> +	if ((((flash->dual_flash == SF_SINGLE_FLASH) &&
> +	      (flash->size > SPI_FLASH_16MB_BOUN)) ||
>   	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
> -	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
> +	      (flash->size > SPI_FLASH_16MB_BOUN << 1))) &&
> +	    !flash->in_4byte_mode) {
>   		puts("SF: Warning - Only lower 16MiB accessible,");
>   		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
>   	}
> diff --git a/include/spi_flash.h b/include/spi_flash.h
> index 0ec98fb55d..b5bc4a85f6 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -36,6 +36,8 @@ struct spi_slave;
>    * @dual_flash:		Indicates dual flash memories - dual stacked, parallel
>    * @shift:		Flash shift useful in dual parallel
>    * @flags:		Indication of spi flash flags
> + * @in_4byte_mode:	True if flash is detected to be in 4-byte mode
> + * @cmdlen:		CMD length (3-byte vs 4-byte mode)
>    * @size:		Total flash size
>    * @page_size:		Write (page) size
>    * @sector_size:	Sector size
> @@ -69,6 +71,9 @@ struct spi_flash {
>   	u8 shift;
>   	u16 flags;
>   
> +	bool in_4byte_mode;
> +	int cmdlen;
> +
>   	u32 size;
>   	u32 page_size;
>   	u32 sector_size;




More information about the U-Boot mailing list