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

Tue Jun 11 18:16:52 CEST 2013

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

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130611/3399dec4/attachment.pgp>