Hi, On Tue, Jun 11, 2013 at 9:19 AM, Jagan Teki <jagannadh.t...@gmail.com>wrote:
> Hi Simon, > > On Tue, Jun 11, 2013 at 9:26 PM, Simon Glass <s...@chromium.org> wrote: > > Hi Jagan, > > > > On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki <jagannadh.t...@gmail.com> > > wrote: > >> > >> Hi Simon, > >> > >> On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass <s...@chromium.org> wrote: > >> > On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.t...@gmail.com > > > >> > wrote: > >> >> > >> >> Hi Tom, > >> >> > >> >> On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.t...@gmail.com > > > >> >> wrote: > >> >> > Hi Simon, > >> >> > > >> >> > On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass <s...@chromium.org> > wrote: > >> >> >> Hi, > >> >> >> > >> >> >> On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki > >> >> >> <jagannadh.t...@gmail.com> > >> >> >> wrote: > >> >> >>> > >> >> >>> Hi Simon, > >> >> >>> > >> >> >>> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass <s...@chromium.org> > >> >> >>> wrote: > >> >> >>> > Hi Jagan, > >> >> >>> > > >> >> >>> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki > >> >> >>> > <jagannadha.sutradharudu-t...@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 < > jaga...@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? > > I am saying as uboot.bin size increases for these new updates I must > compromise use the macros for reducing the sizes. > Yes, I think it is OK to increase size by a few bytes, but if you are adding a new feature that will not be used by existing boards, suggest putting it behind a CONFIG_... flag. Also see Tom's comment. Regards, Simon > > Am i clear. > > Thanks, > Jagan. > > > > >> > >> > >> > > >> > 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 > >> >> >> > >> > > >> > > > >
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot