[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes

Simon Glass sjg at chromium.org
Sat Jun 8 01:14:55 CEST 2013


Hi Jagan,

On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki <
jagannadha.sutradharudu-teki at xilinx.com> wrote:

> Updated the spi_flash framework to handle all sizes of flashes
> using bank/extd addr reg facility
>
> The current implementation in spi_flash supports 3-byte address mode
> due to this up to 16Mbytes amount of flash is able to access for those
> flashes which has an actual size of > 16MB.
>
> As most of the flashes introduces a bank/extd address registers
> for accessing the flashes in 16Mbytes of banks if the flash size
> is > 16Mbytes, this new scheme will add the bank selection feature
> for performing write/erase operations on all flashes.
>
> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna at xilinx.com>
>

I have a few comments on this series, but it mostly looks fine. I think the
new code is correct.

The patches did not apply cleanly for me. Perhaps I am missing something.
My tree looks like this after I did a bit of merging:

5864e87 (HEAD, try-spi) cfi_flash: Fix detection of 8-bit bus flash devices
via address shift
f700095 sf: Add Flag status register polling support
42f4b70 sf: Remove spi_flash_cmd_poll_bit()
fc31387 sf: Use spi_flash_read_common() in write status poll
923e40e sf: spansion: Add support for S25FL512S_256K
c72e52a sf: stmicro: Add support for N25Q1024A
4066f71 sf: stmicro: Add support for N25Q1024
0efaf86 sf: stmicro: Add support for N25Q512A
8fd962f sf: stmicro: Add support for N25Q512
f1a2080 sf: Use spi_flash_addr() in write call
31ed498 sf: Update sf read to support all sizes of flashes
0f77642 sf: Update sf to support all sizes of flashes
9e57220 sf: read flash bank addr register at probe time
e99a43d sf: Add extended addr read support for winbond|stmicro
2f06d56 sf: Add extended addr write support for winbond|stmicro
f02ecf1 sf: Add bank address register reading support
02ba27c sf: Add bank address register writing support

Also do you think spi_flash_cmd_bankaddr_write() and related stuff should
be behind a flag like CONFIG_SPI_BANK_ADDR or similar? How much code space
does this add?

In your change to spi_flash_cmd_poll_bit() the effect is not the same I
think. It is designed to hold CS active and read the status byte
continuously until it changes. But your implementation asserts CS, reads
the status byte, de-asserts CS, then repeats. Why do we want to change
this?



---
> Changes for v2:
>         - none
>
>  drivers/mtd/spi/spi_flash.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 4576a16..5386884 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -74,11 +74,9 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
> u32 offset,
>         unsigned long page_addr, byte_addr, page_size;
>         size_t chunk_len, actual;
>         int ret;
> -       u8 cmd[4];
> +       u8 cmd[4], bank_sel;
>
>         page_size = flash->page_size;
> -       page_addr = offset / page_size;
> -       byte_addr = offset % page_size;
>
>         ret = spi_claim_bus(flash->spi);
>         if (ret) {
> @@ -88,6 +86,16 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash,
> u32 offset,
>
>         cmd[0] = CMD_PAGE_PROGRAM;
>         for (actual = 0; actual < len; actual += chunk_len) {
> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> +
> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
> +               if (ret) {
> +                       debug("SF: fail to set bank%d\n", bank_sel);
> +                       return ret;
> +               }
>

So we are now doing this for all chips. But isn't it true that only some
chips (>16MB?) have a bank address. If so, then I think we should have a
flag somewhere to enable this feature


> +
> +               page_addr = offset / page_size;
> +               byte_addr = offset % page_size;
>

This is OK I think. We really don't care about the slower divide so it is
not worth optimising for I think.


>                 chunk_len = min(len - actual, page_size - byte_addr);
>
>                 if (flash->spi->max_write_size)
> @@ -117,11 +125,7 @@ int spi_flash_cmd_write_multi(struct spi_flash
> *flash, u32 offset,
>                 if (ret)
>                         break;
>
> -               byte_addr += chunk_len;
> -               if (byte_addr == page_size) {
> -                       page_addr++;
> -                       byte_addr = 0;
> -               }
> +               offset += chunk_len;
>         }
>
>         spi_release_bus(flash->spi);
> @@ -204,9 +208,9 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash,
> unsigned long timeout)
>
>  int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
>  {
> -       u32 end, erase_size;
> +       u32 erase_size;
>         int ret;
> -       u8 cmd[4];
> +       u8 cmd[4], bank_sel;
>
>         erase_size = flash->sector_size;
>         if (offset % erase_size || len % erase_size) {
> @@ -224,11 +228,17 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32
> offset, size_t len)
>                 cmd[0] = CMD_ERASE_4K;
>         else
>                 cmd[0] = CMD_ERASE_64K;
> -       end = offset + len;
>
> -       while (offset < end) {
> +       while (len) {
> +               bank_sel = offset / SPI_FLASH_16MB_BOUN;
> +
> +               ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
> +               if (ret) {
> +                       debug("SF: fail to set bank%d\n", bank_sel);
> +                       return ret;
> +               }
> +
>                 spi_flash_addr(offset, cmd);
> -               offset += erase_size;
>
>                 debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
>                       cmd[2], cmd[3], offset);
> @@ -244,6 +254,9 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32
> offset, size_t len)
>                 ret = spi_flash_cmd_wait_ready(flash,
> SPI_FLASH_PAGE_ERASE_TIMEOUT);
>                 if (ret)
>                         goto out;
> +
> +               offset += erase_size;
> +               len -= erase_size;
>         }
>
>   out:
> --
> 1.8.3
>
>
>
Regards,
Simon


More information about the U-Boot mailing list