Thanks Cyrille for taking time and reviewing this patch. Please find reply in-lined.
> -----Original Message----- > From: Cyrille Pitchen [mailto:cyrille.pitc...@wedev4u.fr] > Sent: Wednesday, December 20, 2017 6:43 PM > To: Prabhakar Kushwaha <prabhakar.kushw...@nxp.com>; u- > b...@lists.denx.de > Cc: jagannadh.t...@gmail.com; Poonam Aggrwal > <poonam.aggr...@nxp.com>; Suresh Gupta <suresh.gu...@nxp.com>; > marek.va...@gmail.com; Vignesh R <vigne...@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.kushw...@nxp.com> > > CC: Cyrille Pitchen <cyrille.pitc...@wedev4u.fr> > > CC: Marek Vasut <marek.va...@gmail.com> > > CC: Vignesh R <vigne...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
