[U-Boot] [PATCH v2 06/16] sf: Update sf to support all sizes of flashes
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 --- 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; + } + + page_addr = offset / page_size; + byte_addr = offset % page_size; chunk_len = min(len - actual, page_size - byte_addr);
if (flash->spi->max_write_size) @@ -117,11 +125,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, if (ret) break;
- byte_addr += chunk_len; - if (byte_addr == page_size) { - page_addr++; - byte_addr = 0; - } + offset += chunk_len; }
spi_release_bus(flash->spi); @@ -204,9 +208,9 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) { - u32 end, erase_size; + u32 erase_size; int ret; - u8 cmd[4]; + u8 cmd[4], bank_sel;
erase_size = flash->sector_size; if (offset % erase_size || len % erase_size) { @@ -224,11 +228,17 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) cmd[0] = CMD_ERASE_4K; else cmd[0] = CMD_ERASE_64K; - end = offset + len;
- while (offset < end) { + while (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; + } + spi_flash_addr(offset, cmd); - offset += erase_size;
debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1], cmd[2], cmd[3], offset); @@ -244,6 +254,9 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT); if (ret) goto out; + + offset += erase_size; + len -= erase_size; }
out:
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:
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?
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?
---
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
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.
chunk_len = min(len - actual, page_size - byte_addr); if (flash->spi->max_write_size)@@ -117,11 +125,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, if (ret) break;
byte_addr += chunk_len;if (byte_addr == page_size) {page_addr++;byte_addr = 0;}
offset += chunk_len; } spi_release_bus(flash->spi);@@ -204,9 +208,9 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) {
u32 end, erase_size;
u32 erase_size; int ret;
u8 cmd[4];
u8 cmd[4], bank_sel; erase_size = flash->sector_size; if (offset % erase_size || len % erase_size) {@@ -224,11 +228,17 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) cmd[0] = CMD_ERASE_4K; else cmd[0] = CMD_ERASE_64K;
end = offset + len;while (offset < end) {
while (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;}spi_flash_addr(offset, cmd);
offset += erase_size; debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1], cmd[2], cmd[3], offset);@@ -244,6 +254,9 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT); if (ret) goto out;
offset += erase_size;len -= erase_size; }out:
-- 1.8.3
Regards, Simon
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.
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.
+ 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)
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,
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?
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.
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.
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
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 is this 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.
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
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.
-- 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
On Mon, Jun 10, 2013 at 9:05 AM, Jagan Teki jagannadh.teki@gmail.comwrote:
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.
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
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
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?
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.
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
Hi Jagan,
On Tue, Jun 11, 2013 at 8:31 AM, Jagan Teki jagannadh.teki@gmail.comwrote:
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?
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
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.
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
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 >
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@gmail.comwrote:
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.
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.
Hi Tom,
On Tue, Jun 11, 2013 at 9:46 PM, Tom Rini trini@ti.com wrote:
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@gmail.comwrote:
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.
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".
Understood.
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.
Yes, we should think about this.
I will come to this again for better more idea, if any.
-- Thanks, Jagan.
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
participants (4)
-
Jagan Teki -
Jagannadha Sutradharudu Teki -
Simon Glass -
Tom Rini