[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