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