Hi,
On Tue, Jun 11, 2013 at 9:19 AM, Jagan Teki jagannadh.teki@gmail.comwrote:
Hi Simon,
On Tue, Jun 11, 2013 at 9:26 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Tue, Jun 11, 2013 at 3:19 AM, Simon Glass sjg@chromium.org wrote:
On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki <jagannadh.teki@gmail.com
wrote:
Hi Tom,
On Sat, Jun 8, 2013 at 8:24 PM, Jagan Teki <jagannadh.teki@gmail.com
wrote:
Hi Simon,
On Sat, Jun 8, 2013 at 8:11 PM, Simon Glass sjg@chromium.org
wrote:
> Hi, > > On Sat, Jun 8, 2013 at 1:22 AM, Jagan Teki > jagannadh.teki@gmail.com > wrote: >> >> Hi Simon, >> >> On Sat, Jun 8, 2013 at 4:44 AM, Simon Glass sjg@chromium.org >> wrote: >> > Hi Jagan, >> > >> > On Fri, May 31, 2013 at 5:52 AM, Jagannadha Sutradharudu Teki >> > jagannadha.sutradharudu-teki@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@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 >