[U-Boot] [PATCH 2/3] sf: add method to support memory size above 128Mib

Prabhakar Kushwaha prabhakar.kushwaha at nxp.com
Thu Dec 21 09:15:37 UTC 2017


Thanks Cyrille for taking time and reviewing this patch.

Please find reply in-lined. 

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen at wedev4u.fr]
> Sent: Wednesday, December 20, 2017 6:43 PM
> To: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>; u-
> boot at lists.denx.de
> Cc: jagannadh.teki at gmail.com; Poonam Aggrwal
> <poonam.aggrwal at nxp.com>; Suresh Gupta <suresh.gupta at nxp.com>;
> marek.vasut at gmail.com; Vignesh R <vigneshr at ti.com>
> Subject: Re: [PATCH 2/3] sf: add method to support memory size above 128Mib
> 
> Hi Prabhakar,
> 
> Le 20/12/2017 à 11:32, Prabhakar Kushwaha a écrit :
> > This patch add support of memories with size above 128Mib. It has
> > been ported from Linux commit "mtd: spi-nor: add a
> > stateless method to support memory size above 128Mib".
> >
> > It convert 3byte opcode into 4byte opcode based upon
> > OPCODES_4B or Spansion flash. Also the commands are malloc'ed
> > at run time based on 3byte or 4byte address opcode requirement.
> >
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
> > CC: Cyrille Pitchen <cyrille.pitchen at wedev4u.fr>
> > CC: Marek Vasut <marek.vasut at gmail.com>
> > CC: Vignesh R <vigneshr at ti.com>
> > ---
> > depends upon
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw
> ork.ozlabs.org%2Fpatch%2F826919%2F&data=02%7C01%7Cprabhakar.kushwah
> a%40nxp.com%7Cfd9d02dc9f994833f4b108d547ab5862%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C636493723634923096&sdata=EHEm2o4OnvY
> nyFJLmgbom%2F3YshxNupJ41xJtZZnz1ZM%3D&reserved=0
> >
> >  drivers/mtd/spi/sf_internal.h |  28 ++++++++-
> >  drivers/mtd/spi/spi_flash.c   | 136
> ++++++++++++++++++++++++++++++++++++------
> >  include/spi_flash.h           |   2 +
> >  3 files changed, 146 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> > index 06dee0a..164b0ea 100644
> > --- a/drivers/mtd/spi/sf_internal.h
> > +++ b/drivers/mtd/spi/sf_internal.h
> > @@ -27,7 +27,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_MAX_ADDR_WIDTH	4
> >  #define SPI_FLASH_16MB_BOUN		0x1000000
> >
> >  /* CFI Manufacture ID's */
> > @@ -57,13 +58,30 @@ enum spi_nor_option_flags {
> >  #define CMD_READ_DUAL_IO_FAST		0xbb
> >  #define CMD_READ_QUAD_OUTPUT_FAST	0x6b
> >  #define CMD_READ_QUAD_IO_FAST		0xeb
> > +
> > +/* 4B READ commands */
> > +#define CMD_READ_ARRAY_SLOW_4B		0x13
> > +#define CMD_READ_ARRAY_FAST_4B		0x0c
> > +#define CMD_READ_DUAL_OUTPUT_FAST_4B	0x3c
> > +#define CMD_READ_DUAL_IO_FAST_4B	0xbc
> > +#define CMD_READ_QUAD_OUTPUT_FAST_4B	0x6c
> > +#define CMD_READ_QUAD_IO_FAST_4B	0xec
> > +
> > +/* 4B Write commands */
> > +#define CMD_PAGE_PROGRAM_4B		0x12
> > +
> > +/* 4B Erase commands */
> > +#define CMD_ERASE_4K_4B			0x21
> > +#define CMD_ERASE_CHIP_4B		0x5c
> > +#define CMD_ERASE_64K_4B		0xdc
> > +
> >  #define CMD_READ_ID			0x9f
> >  #define CMD_READ_STATUS			0x05
> >  #define CMD_READ_STATUS1		0x35
> >  #define CMD_READ_CONFIG			0x35
> >  #define CMD_FLAG_STATUS			0x70
> > -#define CMD_EN4B				0xB7
> > -#define CMD_EX4B				0xE9
> > +#define CMD_EN4B			0xB7
> > +#define CMD_EX4B			0xE9
> >
> >  /* Bank addr access commands */
> >  #ifdef CONFIG_SPI_FLASH_BAR
> > @@ -133,6 +151,10 @@ struct spi_flash_info {
> >  #define RD_DUAL			BIT(5)	/* use Dual Read */
> >  #define RD_QUADIO		BIT(6)	/* use Quad IO Read */
> >  #define RD_DUALIO		BIT(7)	/* use Dual IO Read */
> > +#define OPCODES_4B		BIT(8)	/*
> > +					 * Use dedicated 4byte address op
> codes
> > +					 * to support memory size above
> 128Mib.
> > +					 */
> >  #define RD_FULL			(RD_QUAD | RD_DUAL | RD_QUADIO |
> RD_DUALIO)
> >  };
> >
> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> > index 7581622..4d2e58e 100644
> > --- a/drivers/mtd/spi/spi_flash.c
> > +++ b/drivers/mtd/spi/spi_flash.c
> > @@ -22,12 +22,28 @@
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > -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;
> > +
> > +	switch (flash->addr_width) {
> > +	case SPI_FLASH_3B_ADDR_LEN:
> > +		cmd[1] = addr >> 16;
> > +		cmd[2] = addr >> 8;
> > +		cmd[3] = addr >> 0;
> > +		break;
> > +
> > +	case SPI_FLASH_4B_ADDR_LEN:
> > +		cmd[1] = addr >> 24;
> > +		cmd[2] = addr >> 16;
> > +		cmd[3] = addr >> 8;
> > +		cmd[4] = addr >> 0;
> > +		break;
> > +
> > +	default:
> > +		debug("SF: Wrong opcode size\n");
> > +		break;
> > +	}
> 
> Not a big deal and mostly a question of personal taste but you can have a
> look at what's done by the m25p80 driver from linux (m25p_addr2cmd()
> function):
> 
> 	cmd[1] = addr >> (flash->addr_width * 8 -  8);
> 	cmd[2] = addr >> (flash->addr_width * 8 - 16);
> 	cmd[3] = addr >> (flash->addr_width * 8 - 24);
> 	cmd[4] = addr >> (flash->addr_width * 8 - 32);
> 

Let me migrate to this.


> >  }
> >
> >  static int read_sr(struct spi_flash *flash, u8 *rs)
> > @@ -74,6 +90,64 @@ static int write_sr(struct spi_flash *flash, u8 ws)
> >  	return 0;
> >  }
> >
> > +static u8 spi_flash_convert_opcode(u8 opcode, const u8 table[][2], size_t
> size)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < size; i++)
> > +		if (table[i][0] == opcode)
> > +			return table[i][1];
> > +
> > +	/* No conversion found, keep input op code. */
> > +	return opcode;
> > +}
> > +
> > +static inline u8 spi_flash_convert_3to4_read(u8 opcode)
> > +{
> > +	static const u8 spi_flash_3to4_read[][2] = {
> > +		{ CMD_READ_ARRAY_SLOW,
> CMD_READ_ARRAY_SLOW_4B },
> > +		{ CMD_READ_ARRAY_FAST,          CMD_READ_ARRAY_FAST_4B
> },
> > +		{ CMD_READ_DUAL_OUTPUT_FAST,
> CMD_READ_DUAL_OUTPUT_FAST_4B },
> > +		{ CMD_READ_DUAL_IO_FAST,
> CMD_READ_DUAL_IO_FAST_4B },
> > +		{ CMD_READ_QUAD_OUTPUT_FAST,
> CMD_READ_QUAD_OUTPUT_FAST_4B },
> > +		{ CMD_READ_QUAD_IO_FAST,
> CMD_READ_QUAD_IO_FAST_4B },
> > +
> > +	};
> > +
> > +	return spi_flash_convert_opcode(opcode, spi_flash_3to4_read,
> > +				      ARRAY_SIZE(spi_flash_3to4_read));
> > +}
> > +
> > +static inline u8 spi_flash_convert_3to4_program(u8 opcode)
> > +{
> > +	static const u8 spi_flash_3to4_program[][2] = {
> > +		{ CMD_PAGE_PROGRAM, CMD_PAGE_PROGRAM_4B },
> > +	};
> > +
> > +	return spi_flash_convert_opcode(opcode, spi_flash_3to4_program,
> > +				      ARRAY_SIZE(spi_flash_3to4_program));
> > +}
> > +
> > +static inline u8 spi_flash_convert_3to4_erase(u8 opcode)
> > +{
> > +	static const u8 spi_flash_3to4_erase[][2] = {
> > +		{ CMD_ERASE_4K,	   CMD_ERASE_4K_4B },
> > +		{ CMD_ERASE_CHIP,  CMD_ERASE_CHIP_4B },
> > +		{ CMD_ERASE_64K,   CMD_ERASE_64K_4B },
> > +	};
> > +
> > +	return spi_flash_convert_opcode(opcode, spi_flash_3to4_erase,
> > +				      ARRAY_SIZE(spi_flash_3to4_erase));
> > +}
> > +
> > +static void spi_flash_set_4byte_opcodes(struct spi_flash *flash,
> > +					const struct spi_flash_info *info)
> > +{
> > +	flash->read_cmd = spi_flash_convert_3to4_read(flash->read_cmd);
> > +	flash->write_cmd = spi_flash_convert_3to4_program(flash-
> >write_cmd);
> > +	flash->erase_cmd = spi_flash_convert_3to4_erase(flash->erase_cmd);
> > +}
> > +
> >  #if defined(CONFIG_SPI_FLASH_SPANSION) ||
> defined(CONFIG_SPI_FLASH_WINBOND)
> >  static int read_cr(struct spi_flash *flash, u8 *rc)
> >  {
> > @@ -364,7 +438,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, cmdsz;
> >  	int ret = -1;
> >
> >  	erase_size = flash->erase_size;
> > @@ -381,6 +455,13 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash,
> u32 offset, size_t len)
> >  		}
> >  	}
> >
> > +	cmdsz = 1 + flash->addr_width;
> > +	cmd = calloc(1, cmdsz);
> 
> calloc() but no free() ? Though we're not supposed to run u-boot forever,
> it's still a memory leak. 

I missed it. Thanks for pointing

> Why don't you simply keep u8
> cmd[SPI_FLASH_CMD_LEN] as earlier, only changing the definition of
> SPI_FLASH_CMD_LEN?
> 
> -#define SPI_FLASH_CMD_LEN		(1 + SPI_FLASH_3B_ADDR_LEN)
> +#define SPI_FLASH_CMD_LEN		(1 + SPI_FLASH_4B_ADDR_LEN)
> 
> Of course, you still need cmdsz instead of sizeof(cmd) as you do below,
> when calling spi_flash_write_common().
> 

I agree. I will modify both spi_flash_cmd_erase_ops and spi_flash_cmd_write_ops

--pk



More information about the U-Boot mailing list