[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes
Jagan Teki
jagannadh.teki at gmail.com
Mon Jun 10 18:05:18 CEST 2013
Hi Tom,
On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi,
>>
>> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>
>>> 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.
>>
>>
>> I am not sure now - sorry I did not keep a record. But the bundle I used is
>> here, and it doesn't apply cleanly.
>>
>> http://patchwork.ozlabs.org/bundle/sjg/jagan/
>>
>> git am ~/Downloads/bundle-4407-jagan.mbox
>> Applying: sf: Add bank address register writing support
>> Applying: sf: Add bank address register reading support
>> Applying: sf: Add extended addr write support for winbond|stmicro
>> Applying: sf: Add extended addr read support for winbond|stmicro
>> Applying: sf: read flash bank addr register at probe time
>> Applying: sf: Update sf to support all sizes of flashes
>> error: patch failed: drivers/mtd/spi/spi_flash.c:117
>> error: drivers/mtd/spi/spi_flash.c: patch does not apply
>> Patch failed at 0006 sf: Update sf to support all sizes of flashes
>> The copy of the patch that failed is found in:
>> /home/sjg/u/.git/rebase-apply/patch
>> When you have resolved this problem, run "git am --resolved".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort"
>>
>>>
>>>
>>> >
>>> > 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.
>>
>>
>> Yes this is fine.
>>
>>>
>>>
>>> + 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)
>>
>>
>> I suggest you make that function a nop (perhaps using an #ifdef
>> CONFIG_SPI_BANK_ADDR inside it or some other name) so that your patches
>> don't increase U-Boot code size for those boards that don't need support
>> larger devices (which I guess is almost all of them, right now). U-Boot is
>> quite concerned about code size.
>
> Little concern here, the point here I stated that these new changes is
> common for all sizes of flashes.(which are less or greater than
> 16Mbytes).
> and yes it increase the code size little bit but i don't think it will
> require the separate macro.
Any comments.
--
Thanks,
Jagan.
>
> Thanks,
> Jagan.
>
>>
>> Tom may chime in and decide it is fine, though.
>>
>>>
>>>
>>> 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,
>>
>>
>> OK I will take a look.
>>
>>>
>>>
>>> >
>>> >
>>> >
>>> >> ---
>>> >> 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?
>>
>>
>> OK, well we don't need a flag since you will never issue the bank address
>> command unless the chip is larger than 16MB.,
>>
>>>
>>>
>>> >
>>> >>
>>> >> +
>>> >> + 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.
>>
>>
>>
>> Regards,
>> Simon
>>
More information about the U-Boot
mailing list