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

[Jagan Teki]

Hi Simon,

On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <sjg at chromium.org> wrote:
> 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:

Which patches you had an issues while applying,we have few patches on
u-boot-spi.git i created these on top of it.

>
> 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?

Initially i thought of the same, but I just updated sf which is
generic to all sizes of flashes.
due to this i haven't included the bank read/write on macros, and the
flash ops will call these
bank write call irrespective of flash sizes.

As flashes which has below 16Mbytes will have a bank_curr value 0
so-that even bank write will exit for
bank 0 operations.

+	if (flash->bank_curr == bank_sel) {
+		debug("SF: not require to enable bank%d\n", bank_sel);
+		return 0;
+	}
+

And due to these framework changes bank+flash ops addition, bin size
increases appr' ~600bytes
by enabling stmicro, winbond and spansion flash drivers.(please check
the size from your end also if required)

Please see the commit body on below thread for more info
http://patchwork.ozlabs.org/patch/247954/

>
> 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?

I commented on the actual patch thread, please refer,

>
>
>
>> ---
>> 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

APAMK, currently stmicro, winbond, spansion and macronix have a
flashes which has > 16Mbytes flashes.

And macronix is also have same bank setup like stmicro, extended addr
read(RDEAR - 0xC8) and extended addr write(WREAR - 0xC5)
We need to add this in near future.

I have added Prafulla Wadaskar on this thread (initial contributor for
macronix.c), may be he will give some more information
for accessing > 16Mbytes flashes in 3-byte addr mode.

I think we can go ahead for now, may be we will tune sf some more in
future based on the availability of different flash which crosses
16Mbytes
with different apparoch (other than banking/extended), what do you say?

>
>>
>> +
>> +               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.

Yes, I just used the existing spi_flash_addr with offset as an
argument, anyway sf write have a logic
to use writing in terms of page sizes and even offset is also varies
page sizes or requested sizes.

Thanks,
Jagan.