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

Tue Jun 11 19:49:45 CEST 2013

Hi Tom,

On Tue, Jun 11, 2013 at 9:46 PM, Tom Rini <trini at ti.com> wrote:
> On Mon, Jun 10, 2013 at 02:49:35PM -0700, Simon Glass 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.
>>
>> Tom may have further comments.
>
> I think it's important we keep both binary size and code complexity in
> mind when we say "no binary size increase".  It really has to mean "no
> unreasonable binary size increase, when compared with code complexity
> changes".  The question has to be "can we easily make things opt-in".

Understood.

>
> Perhaps the answer here is that we need to add
> CONFIG_SPI_FLASH_STMICRO_STATIC_TABLE and a corresponding set of
> CONFIG_SPI_FLASH_STMICRO_ID/PPS/NR_SECT/NAME (and Winbond variant) so
> that boards with size constrains can opt to define the single chip they
> support, and other boards can just get the full probe table.  This will
> allow boards to get a net size decrease here if desired.

Yes, we should think about this.

I will come to this again for better more idea, if any.

--
Thanks,
Jagan.

>
> Heck, for ARM we are, or have just, reclaimed a bunch of space at least
> on SPL using boards in the main binary now that we --gc-sections U-Boot.
>
> --
> Tom