[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes
Simon Glass
sjg at chromium.org
Tue Jun 11 17:56:13 CEST 2013
Hi Jagan,
On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki <jagannadh.teki at gmail.com>wrote:
> 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?
>
What does 'comprise' mean in this context?
>
> >
> > 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.
>
OK, sorry, I didn't know about that tree...thanks for pointing me to it.
Regards,
Simon
>
> 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