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

Jagan Teki jagannadh.teki at gmail.com
Tue Jun 11 17:31:49 CEST 2013


Hi Simon,

On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass <sjg at chromium.org> wrote:
> On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki at gmail.com>
> wrote:
>>
>> 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.
>
>
> In U-Boot it is generally not acceptable to increase code size for existing
> boards when adding a new feature that they don't use. So I suspect in this
> case you should add a new CONFIG to enable your feature. It seems to
> increase code by more than 200 bytes for snow, for example.

OK, I did coding on sf to have a common framework for all sizes of
flashes in mind.
but as you said it's increasing the size of u-boot.bin > 200 bytes.

Seems like no choice to comprise, I am going to create v3 series for
these changes.
Will that be OK?

>
> Tom may have further comments.
>
> Also my buildman run of your commit gave an error on this commit:
>
> 07: sf: Update sf to support all sizes of flashes

I am created these patches on top of u-boot-spi.git, there some
patches already available on sf.
may be you used master.

Thanks,
Jagan.

>
>
> Regards,
> Simon
>
>>
>>
>> --
>> 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