* [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size @ 2011-02-08 14:13 Richard Retanubun 2011-02-08 14:40 ` Wolfgang Denk 0 siblings, 1 reply; 17+ messages in thread From: Richard Retanubun @ 2011-02-08 14:13 UTC (permalink / raw) To: u-boot Hello, I am working on hooks to build into my board to allow for SPI flash (STMicro M25P40 and M25P16) reflash. When calling spi_flash_erase, which eventually calls stmicro_erase, one must do so while knowing what the sector_size is of the flash. Is there a recommened API to getting this from struct stmicro_spi_flash_params? There are things like the "to_stmicro_spi_flash" #define in stmicro.c, but that is private. Will adding this API be okay? int spi_flash_get_sector_size(struct spi_flash *sf) that will simply return the sector_size number or an errno. thanks for everyone's time - Richard Retanubun ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size 2011-02-08 14:13 [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size Richard Retanubun @ 2011-02-08 14:40 ` Wolfgang Denk 2011-02-08 15:11 ` Richard Retanubun 0 siblings, 1 reply; 17+ messages in thread From: Wolfgang Denk @ 2011-02-08 14:40 UTC (permalink / raw) To: u-boot Dear Richard Retanubun, In message <4D514F6E.6040101@RuggedCom.com> you wrote: > > When calling spi_flash_erase, which eventually calls stmicro_erase, one must do so while knowing > what the sector_size is of the flash. Is there a recommened API to getting this from struct stmicro_spi_flash_params? Why would you need to know the sector size? To align your erase command accordingly? I think you are taking the wrong approach here. Look at how NOR flash is handled: we don't need to know the sector sizes there either. Instead we support the format "+N" (often used as "+${fileszize}") which will _internally_ round up N to match the next erase block boundary. Do the same for SPI flash. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The first thing we do is kill all the lawyers. (Shakespeare. II Henry VI, Act IV, scene ii) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size 2011-02-08 14:40 ` Wolfgang Denk @ 2011-02-08 15:11 ` Richard Retanubun 2011-02-08 15:19 ` Wolfgang Denk 0 siblings, 1 reply; 17+ messages in thread From: Richard Retanubun @ 2011-02-08 15:11 UTC (permalink / raw) To: u-boot On 02/08/11 09:40, Wolfgang Denk wrote: Thanks for the speedy response Wolfgang, > Dear Richard Retanubun, > > In message<4D514F6E.6040101@RuggedCom.com> you wrote: >> >> When calling spi_flash_erase, which eventually calls stmicro_erase, one must do so while knowing >> what the sector_size is of the flash. Is there a recommened API to getting this from struct stmicro_spi_flash_params? > > Why would you need to know the sector size? > > To align your erase command accordingly? Yep. I think you are taking the wrong approach here. Look at how NOR flash is handled: we don't need > to know the sector sizes there either. Instead we support the format > "+N" (often used as "+${fileszize}") which will _internally_ round up > N to match the next erase block boundary. > > Do the same for SPI flash. If by "how NOR flash in handled" you mean cmd_flash.c::flash_sect_roundb() then yes, I do want to do this but I need to know what number to round it to, no? In the context of cmd_flash, it has access to flash_info_t which contains the sector_size information. Also cfi_flash.c also provides a function called flash_sector_size() that is called by other components that needs to know. If I have misunderstood what you mean, sorry, please clarify. To have SPI flash do this, for example; stmicro.c::stmicro_erase() function will auto round up to the nearest sector_size. Which is indeed the simpler way. Right now, most drivers/mtd/spi/*.c just does this check: <quoted_code> sector_size = stm->params->page_size * stm->params->pages_per_sector; if (offset % sector_size || len % sector_size) { debug("SF: Erase offset/length not multiple of sector size\n"); return -1; } </quoted_code> I am worried about unintentionally erasing more than what the caller requested. Would it be better to round up manually and then calls the erase? Thanks for your time - Richard ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size 2011-02-08 15:11 ` Richard Retanubun @ 2011-02-08 15:19 ` Wolfgang Denk 2011-02-08 15:33 ` Richard Retanubun 2011-02-09 21:16 ` [U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler Richard Retanubun 0 siblings, 2 replies; 17+ messages in thread From: Wolfgang Denk @ 2011-02-08 15:19 UTC (permalink / raw) To: u-boot Dear Richard Retanubun, In message <4D515D06.7020209@RuggedCom.com> you wrote: > > If by "how NOR flash in handled" you mean cmd_flash.c::flash_sect_roundb() then yes, I do want to do this > but I need to know what number to round it to, no? The code needs to know that, you don't ;-) > In the context of cmd_flash, it has access to flash_info_t > which contains the sector_size information. Also cfi_flash.c also provides a function called flash_sector_size() > that is called by other components that needs to know. If I have misunderstood what you mean, sorry, please clarify. You are right. > To have SPI flash do this, for example; stmicro.c::stmicro_erase() function will auto round up to the nearest sector_size. > Which is indeed the simpler way. Right now, most drivers/mtd/spi/*.c just does this check: > > <quoted_code> > sector_size = stm->params->page_size * stm->params->pages_per_sector; > > if (offset % sector_size || len % sector_size) { > debug("SF: Erase offset/length not multiple of sector size\n"); > return -1; > } > </quoted_code> This is ok if an explicit size is given - you will se ethe same behaviour on NOR flash when trying something like erase 40000000 40002345 > I am worried about unintentionally erasing more than what the caller requested. > Would it be better to round up manually and then calls the erase? No. If the caller uses the "+<size>" notation he explictly requests to round up. He is supposed to know what he is doing. Being able to use "+${filesize}" allows for many powerful solutions to standard tasks that without this would result in cumbersome manipulation of the size - which would then probably break if you issue a new revision of your hardware with different flash chips fit. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "If you'll excuse me a minute, I'm going to have a cup of coffee." - broadcast from Apollo 11's LEM, "Eagle", to Johnson Space Center, Houston July 20, 1969, 7:27 P.M. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size 2011-02-08 15:19 ` Wolfgang Denk @ 2011-02-08 15:33 ` Richard Retanubun 2011-02-09 21:16 ` [U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler Richard Retanubun 1 sibling, 0 replies; 17+ messages in thread From: Richard Retanubun @ 2011-02-08 15:33 UTC (permalink / raw) To: u-boot On 02/08/11 10:19, Wolfgang Denk wrote: > Dear Richard Retanubun, > > In message<4D515D06.7020209@RuggedCom.com> you wrote: >> >> If by "how NOR flash in handled" you mean cmd_flash.c::flash_sect_roundb() then yes, I do want to do this >> but I need to know what number to round it to, no? > > The code needs to know that, you don't ;-) > >> In the context of cmd_flash, it has access to flash_info_t >> which contains the sector_size information. Also cfi_flash.c also provides a function called flash_sector_size() >> that is called by other components that needs to know. If I have misunderstood what you mean, sorry, please clarify. > > You are right. <sensing a wavering in your resolve> ;-) Thus we come to my original question, in the same way that cfi_flash.c provides flash_sector_size(); I propose we add spi_flash.c::spi_flash_sector_size() which returns the same information so that other code can validate (or round up if requested) the length of the operation to the nearest sector size. right now (unless I misunderstood), there is no easy way to get the spi flash sector_size, no? would this be acceptable? > >> To have SPI flash do this, for example; stmicro.c::stmicro_erase() function will auto round up to the nearest sector_size. >> Which is indeed the simpler way. Right now, most drivers/mtd/spi/*.c just does this check: >> >> <quoted_code> >> sector_size = stm->params->page_size * stm->params->pages_per_sector; >> >> if (offset % sector_size || len % sector_size) { >> debug("SF: Erase offset/length not multiple of sector size\n"); >> return -1; >> } >> </quoted_code> > > This is ok if an explicit size is given - you will see the same > behaviour on NOR flash when trying something like > > erase 40000000 40002345 Understood. So the spi flash driver erase function itself should only validate the args and not auto round up. This means that the decision of [warn or auto-round-up] should be handled by other code that parses the user's commands that is operating on spi flash. To do this, they need a way to ask the spi flash driver what the sector size to round up to is. > >> I am worried about unintentionally erasing more than what the caller requested. >> Would it be better to round up manually and then calls the erase? > > No. If the caller uses the "+<size>" notation he explictly requests > to round up. He is supposed to know what he is doing. > > Being able to use "+${filesize}" allows for many powerful solutions > to standard tasks that without this would result in cumbersome > manipulation of the size - which would then probably break if you > issue a new revision of your hardware with different flash chips fit. This is a cool convention. Should we add this to "sf erase command?" if it is not there already. if so, what command (or better yet, git commit) shows how to add handling of "+N" Thanks! - Richard ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler. 2011-02-08 15:19 ` Wolfgang Denk 2011-02-08 15:33 ` Richard Retanubun @ 2011-02-09 21:16 ` Richard Retanubun 2011-02-15 9:34 ` Mike Frysinger 1 sibling, 1 reply; 17+ messages in thread From: Richard Retanubun @ 2011-02-09 21:16 UTC (permalink / raw) To: u-boot ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler. 2011-02-09 21:16 ` [U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler Richard Retanubun @ 2011-02-15 9:34 ` Mike Frysinger 2011-02-16 20:27 ` [U-Boot] (no subject) Richard Retanubun ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Mike Frysinger @ 2011-02-15 9:34 UTC (permalink / raw) To: u-boot On Wednesday, February 09, 2011 16:16:12 Richard Retanubun wrote: > From hints by Wolfgang, this patch adds the ability to handle +len > argument for spi flash erase, which will round up the length to the > nearest [sector|page|block]_size. this should be split up into two patches. one that unifies the erase sizes and one that modifies cmd_sf.c to use the new field. ive already mostly unified the erase functions here: git://git.denx.de/u-boot-blackfin.git sf but the one piece missing is what you're proposing. so i'll want to merge the unification part you have here into that patch. if you could test out that sf branch now to see if it works for you, that'd be nice ;). > This is done by adding a new member to > struct spi_flash::u32 block_size > > The name 'block_size' is chosen to mean: > "the smallest unit erase commands will check against". let's use "sector_size" as this is what Linux already uses > +static int sf_parse_len_arg(char *arg, ulong *len) constify arg > + > + one new line only > + if (arg && *arg == '+'){ NULL check is useless as the caller already took care of it > + if (round_up_len) { > + /* Get sector size from flash and round up */ > + sector_size = flash->block_size; > + if (sector_size > 0) { > + *len = ((((len_arg -1)/sector_size) + 1) * sector_size); we have a DIV_ROUND_UP macro already > + if (*len > flash->size) { > + return -1; > + } > + } else { > + return -1; > + } > + } else { > + *len = len_arg; > + } pretty much all these braces can be punted > @@ -149,6 +204,7 @@ static int do_spi_flash_erase(int argc, char * const > argv[]) > > usage: > puts("Usage: sf erase offset len\n"); > + puts(" sf erase offset +len\n"); > return 1; > } hrm, a sep patch should be written and sent out before yours that drops this "usage" label and converts all "goto usage" to "return cmd_usage(cmdtp)". > --- a/drivers/mtd/spi/macronix.c > +++ b/drivers/mtd/spi/macronix.c i'll ignore all the spi flash changes per my earlier highlight of rewrites pending in this area. > --- a/include/spi_flash.h > +++ b/include/spi_flash.h > @@ -38,6 +38,8 @@ struct spi_flash { > > u32 size; > > + u32 block_size; not sure what it'll do to code size, but a u16 should be large enough to hold the base erase size. > @@ -67,5 +69,4 @@ static inline int spi_flash_erase(struct spi_flash > *flash, u32 offset, { > return flash->erase(flash, offset, len); > } > - > #endif /* _SPI_FLASH_H_ */ please avoid useless whitespace changes -mike -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. Url : http://lists.denx.de/pipermail/u-boot/attachments/20110215/a24def07/attachment.pgp ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] (no subject) 2011-02-15 9:34 ` Mike Frysinger @ 2011-02-16 20:27 ` Richard Retanubun 2011-02-17 5:46 ` Mike Frysinger 2011-02-16 20:27 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun 2011-02-16 20:27 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 2 siblings, 1 reply; 17+ messages in thread From: Richard Retanubun @ 2011-02-16 20:27 UTC (permalink / raw) To: u-boot Hi Mike, Thanks for the feedback, and for the cool unification. Here is the updated patch that implements your comments and is based on the head of the blackfin.git sf branch. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] (no subject) 2011-02-16 20:27 ` [U-Boot] (no subject) Richard Retanubun @ 2011-02-17 5:46 ` Mike Frysinger 0 siblings, 0 replies; 17+ messages in thread From: Mike Frysinger @ 2011-02-17 5:46 UTC (permalink / raw) To: u-boot On Wednesday, February 16, 2011 15:27:52 Richard Retanubun wrote: > Thanks for the feedback, and for the cool unification. > Here is the updated patch that implements your comments and is > based on the head of the blackfin.git sf branch. thanks ! i'll try to get to it this weekend, but no promises as i currently have non-u-boot stuff pressing for my time. -mike -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. Url : http://lists.denx.de/pipermail/u-boot/attachments/20110217/6b1f2553/attachment.pgp ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter 2011-02-15 9:34 ` Mike Frysinger 2011-02-16 20:27 ` [U-Boot] (no subject) Richard Retanubun @ 2011-02-16 20:27 ` Richard Retanubun 2011-02-16 21:00 ` Wolfgang Denk 2011-02-16 20:27 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 2 siblings, 1 reply; 17+ messages in thread From: Richard Retanubun @ 2011-02-16 20:27 UTC (permalink / raw) To: u-boot This patch adds a new member to struct spi_flash (u16 sector_size) and updates the spi flash drivers to start populating it. This parameter can be used by spi flash commands that need to round up units of operation to the flash's sector_size. --- drivers/mtd/spi/atmel.c | 1 + drivers/mtd/spi/macronix.c | 1 + drivers/mtd/spi/spansion.c | 4 ++-- drivers/mtd/spi/sst.c | 3 ++- drivers/mtd/spi/stmicro.c | 4 ++-- drivers/mtd/spi/winbond.c | 5 +++-- include/spi_flash.h | 2 ++ 7 files changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c index a9910b1..180a52b 100644 --- a/drivers/mtd/spi/atmel.c +++ b/drivers/mtd/spi/atmel.c @@ -498,6 +498,7 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) asf->flash.size = page_size * params->pages_per_block * params->blocks_per_sector * params->nr_sectors; + asf->flash.sector_size = page_size; printf("SF: Detected %s with page size %u, total ", params->name, page_size); diff --git a/drivers/mtd/spi/macronix.c b/drivers/mtd/spi/macronix.c index 4155d4d..4a8e17f 100644 --- a/drivers/mtd/spi/macronix.c +++ b/drivers/mtd/spi/macronix.c @@ -217,6 +217,7 @@ struct spi_flash *spi_flash_probe_macronix(struct spi_slave *spi, u8 *idcode) mcx->flash.read = spi_flash_cmd_read_fast; mcx->flash.size = params->page_size * params->pages_per_sector * params->sectors_per_block * params->nr_blocks; + mcx->flash.sector_size = mcx->flash.size/params->nr_blocks; printf("SF: Detected %s with page size %u, total ", params->name, params->page_size); diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c index d54a5fa..c88457b 100644 --- a/drivers/mtd/spi/spansion.c +++ b/drivers/mtd/spi/spansion.c @@ -199,8 +199,7 @@ static int spansion_write(struct spi_flash *flash, int spansion_erase(struct spi_flash *flash, u32 offset, size_t len) { struct spansion_spi_flash *spsn = to_spansion_spi_flash(flash); - return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE, - spsn->params->page_size * spsn->params->pages_per_sector, + return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE, flash->sector_size, offset, len); } @@ -242,6 +241,7 @@ struct spi_flash *spi_flash_probe_spansion(struct spi_slave *spi, u8 *idcode) spsn->flash.read = spi_flash_cmd_read_fast; spsn->flash.size = params->page_size * params->pages_per_sector * params->nr_sectors; + spsn->flash.sector_size = spsn->flash.size/params->nr_sectors; printf("SF: Detected %s with page size %u, total ", params->name, params->page_size); diff --git a/drivers/mtd/spi/sst.c b/drivers/mtd/spi/sst.c index 792d04d..15de12b 100644 --- a/drivers/mtd/spi/sst.c +++ b/drivers/mtd/spi/sst.c @@ -201,7 +201,7 @@ sst_write(struct spi_flash *flash, u32 offset, size_t len, const void *buf) int sst_erase(struct spi_flash *flash, u32 offset, size_t len) { - return spi_flash_cmd_erase(flash, CMD_SST_SE, SST_SECTOR_SIZE, + return spi_flash_cmd_erase(flash, CMD_SST_SE, flash->sector_size, offset, len); } @@ -257,6 +257,7 @@ spi_flash_probe_sst(struct spi_slave *spi, u8 *idcode) stm->flash.write = sst_write; stm->flash.erase = sst_erase; stm->flash.size = SST_SECTOR_SIZE * params->nr_sectors; + stm->flash.sector_size = SST_SECTOR_SIZE; printf("SF: Detected %s with page size %u, total ", params->name, SST_SECTOR_SIZE); diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c index 7ef690d..80d838e 100644 --- a/drivers/mtd/spi/stmicro.c +++ b/drivers/mtd/spi/stmicro.c @@ -200,8 +200,7 @@ static int stmicro_write(struct spi_flash *flash, int stmicro_erase(struct spi_flash *flash, u32 offset, size_t len) { struct stmicro_spi_flash *stm = to_stmicro_spi_flash(flash); - return spi_flash_cmd_erase(flash, CMD_M25PXX_SE, - stm->params->page_size * stm->params->pages_per_sector, + return spi_flash_cmd_erase(flash, CMD_M25PXX_SE, flash->sector_size, offset, len); } @@ -251,6 +250,7 @@ struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) stm->flash.read = spi_flash_cmd_read_fast; stm->flash.size = params->page_size * params->pages_per_sector * params->nr_sectors; + stm->flash.sector_size = stm->flash.size/params->nr_sectors; printf("SF: Detected %s with page size %u, total ", params->name, params->page_size); diff --git a/drivers/mtd/spi/winbond.c b/drivers/mtd/spi/winbond.c index e88802f..ec6fb79 100644 --- a/drivers/mtd/spi/winbond.c +++ b/drivers/mtd/spi/winbond.c @@ -173,8 +173,7 @@ out: int winbond_erase(struct spi_flash *flash, u32 offset, size_t len) { struct winbond_spi_flash *stm = to_winbond_spi_flash(flash); - return spi_flash_cmd_erase(flash, CMD_W25_SE, - (1 << stm->params->l2_page_size) * stm->params->pages_per_sector, + return spi_flash_cmd_erase(flash, CMD_W25_SE, flash->sector_size, offset, len); } @@ -216,6 +215,8 @@ struct spi_flash *spi_flash_probe_winbond(struct spi_slave *spi, u8 *idcode) stm->flash.size = page_size * params->pages_per_sector * params->sectors_per_block * params->nr_blocks; + stm->flash.sector_size = (1 << stm->params->l2_page_size) * + stm->params->pages_per_sector; printf("SF: Detected %s with page size %u, total ", params->name, page_size); diff --git a/include/spi_flash.h b/include/spi_flash.h index 1f8ba29..039b94e 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -38,6 +38,8 @@ struct spi_flash { u32 size; + u16 sector_size; + int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter 2011-02-16 20:27 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun @ 2011-02-16 21:00 ` Wolfgang Denk 0 siblings, 0 replies; 17+ messages in thread From: Wolfgang Denk @ 2011-02-16 21:00 UTC (permalink / raw) To: u-boot Dear Richard Retanubun, In message <1297888074-8344-2-git-send-email-RichardRetanubun@RuggedCom.com> you wrote: > This patch adds a new member to struct spi_flash (u16 sector_size) > and updates the spi flash drivers to start populating it. > > This parameter can be used by spi flash commands that need to round > up units of operation to the flash's sector_size. > --- > drivers/mtd/spi/atmel.c | 1 + > drivers/mtd/spi/macronix.c | 1 + > drivers/mtd/spi/spansion.c | 4 ++-- > drivers/mtd/spi/sst.c | 3 ++- > drivers/mtd/spi/stmicro.c | 4 ++-- > drivers/mtd/spi/winbond.c | 5 +++-- > include/spi_flash.h | 2 ++ > 7 files changed, 13 insertions(+), 7 deletions(-) Rejected - Signed-off-by: line missing. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de God made the integers; all else is the work of Man. - Kronecker ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command. 2011-02-15 9:34 ` Mike Frysinger 2011-02-16 20:27 ` [U-Boot] (no subject) Richard Retanubun 2011-02-16 20:27 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun @ 2011-02-16 20:27 ` Richard Retanubun 2011-02-16 21:06 ` Wolfgang Denk 2011-02-16 21:47 ` Scott Wood 2 siblings, 2 replies; 17+ messages in thread From: Richard Retanubun @ 2011-02-16 20:27 UTC (permalink / raw) To: u-boot This patch adds [+]len handler for the erase command that will automatically round up the requested erase length to the flash's sector_size. --- common/cmd_sf.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 49 insertions(+), 4 deletions(-) diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 6e7be81..bbd4842 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -19,6 +19,48 @@ static struct spi_flash *flash; + +/* + * This function computes the length argument for the erase command. + * The length on which the command is to operate can be given in two forms: + * 1. <cmd> offset len - operate on <'offset', 'len') + * 2. <cmd> offset +len - operate on <'offset', 'round_up(len)') + * If the second form is used and the length doesn't fall on the + * sector boundary, than it will be adjusted to the next sector boundary. + * If it isn't in the flash, the function will fail (return -1). + * Input: + * arg: length specification (i.e. both command arguments) + * Output: + * len: computed length for operation + * Return: + * 1: success + * -1: failure (bad format, bad address). +*/ +static int sf_parse_len_arg(char *arg, ulong *len) +{ + char *ep; + char round_up_len; /* indicates if the "+length" form used */ + ulong len_arg; + + + round_up_len = 0; + if (*arg == '+'){ + round_up_len = 1; + ++arg; + } + + len_arg = simple_strtoul(arg, &ep, 16); + if (ep == arg || *ep != '\0') + return -1; + + if (round_up_len && flash->sector_size > 0) + *len = DIV_ROUND_UP(len_arg, flash->sector_size); + else + *len = len_arg; + + return 1; +} + static int do_spi_flash_probe(int argc, char * const argv[]) { unsigned int bus = 0; @@ -135,9 +177,11 @@ static int do_spi_flash_erase(int argc, char * const argv[]) offset = simple_strtoul(argv[1], &endp, 16); if (*argv[1] == 0 || *endp != 0) goto usage; - len = simple_strtoul(argv[2], &endp, 16); - if (*argv[2] == 0 || *endp != 0) + + ret = sf_parse_len_arg(argv[2], &len); + if (ret != 1) { goto usage; + } ret = spi_flash_erase(flash, offset, len); if (ret) { @@ -148,7 +192,7 @@ static int do_spi_flash_erase(int argc, char * const argv[]) return 0; usage: - puts("Usage: sf erase offset len\n"); + puts("Usage: sf erase offset [+]len\n"); return 1; } @@ -189,5 +233,6 @@ U_BOOT_CMD( " `offset' to memory at `addr'\n" "sf write addr offset len - write `len' bytes from memory\n" " at `addr' to flash at `offset'\n" - "sf erase offset len - erase `len' bytes from `offset'" + "sf erase offset [+]len - erase `len' bytes from `offset'\n" + " `+len' round up `len' to block size" ); -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command. 2011-02-16 20:27 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun @ 2011-02-16 21:06 ` Wolfgang Denk 2011-02-16 21:37 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun 2011-02-16 21:39 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 2011-02-16 21:47 ` Scott Wood 1 sibling, 2 replies; 17+ messages in thread From: Wolfgang Denk @ 2011-02-16 21:06 UTC (permalink / raw) To: u-boot Dear Richard Retanubun, In message <1297888074-8344-3-git-send-email-RichardRetanubun@RuggedCom.com> you wrote: > This patch adds [+]len handler for the erase command that will > automatically round up the requested erase length to the flash's > sector_size. > --- > common/cmd_sf.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 49 insertions(+), 4 deletions(-) Please run your patches through checkpatch - this helps avoiding the frustration of seeing rejects due to simple issues like these: ERROR: Missing Signed-off-by: line(s) > + if (*arg == '+'){ ERROR: space required before the open brace '{' > + if (ret != 1) { > goto usage; > + } WARNING: braces {} are not necessary for single statement blocks Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de All these theories, diverse as they are, have two things in common: they explain the observed facts, and they are completeley and utterly wrong. - Terry Pratchett, _The Light Fantastic_ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter 2011-02-16 21:06 ` Wolfgang Denk @ 2011-02-16 21:37 ` Richard Retanubun 2011-04-12 6:34 ` Mike Frysinger 2011-02-16 21:39 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 1 sibling, 1 reply; 17+ messages in thread From: Richard Retanubun @ 2011-02-16 21:37 UTC (permalink / raw) To: u-boot This patch adds a new member to struct spi_flash (u16 sector_size) and updates the spi flash drivers to start populating it. This parameter can be used by spi flash commands that need to round up units of operation to the flash's sector_size. Signed-off-by: Richard Retanubun <RichardRetanubun@RuggedCom.com> --- v2: scrubbed via checkpatch, thanks WD! drivers/mtd/spi/atmel.c | 1 + drivers/mtd/spi/macronix.c | 1 + drivers/mtd/spi/spansion.c | 4 ++-- drivers/mtd/spi/sst.c | 3 ++- drivers/mtd/spi/stmicro.c | 4 ++-- drivers/mtd/spi/winbond.c | 5 +++-- include/spi_flash.h | 2 ++ 7 files changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c index a9910b1..180a52b 100644 --- a/drivers/mtd/spi/atmel.c +++ b/drivers/mtd/spi/atmel.c @@ -498,6 +498,7 @@ struct spi_flash *spi_flash_probe_atmel(struct spi_slave *spi, u8 *idcode) asf->flash.size = page_size * params->pages_per_block * params->blocks_per_sector * params->nr_sectors; + asf->flash.sector_size = page_size; printf("SF: Detected %s with page size %u, total ", params->name, page_size); diff --git a/drivers/mtd/spi/macronix.c b/drivers/mtd/spi/macronix.c index 4155d4d..4a8e17f 100644 --- a/drivers/mtd/spi/macronix.c +++ b/drivers/mtd/spi/macronix.c @@ -217,6 +217,7 @@ struct spi_flash *spi_flash_probe_macronix(struct spi_slave *spi, u8 *idcode) mcx->flash.read = spi_flash_cmd_read_fast; mcx->flash.size = params->page_size * params->pages_per_sector * params->sectors_per_block * params->nr_blocks; + mcx->flash.sector_size = mcx->flash.size/params->nr_blocks; printf("SF: Detected %s with page size %u, total ", params->name, params->page_size); diff --git a/drivers/mtd/spi/spansion.c b/drivers/mtd/spi/spansion.c index d54a5fa..c88457b 100644 --- a/drivers/mtd/spi/spansion.c +++ b/drivers/mtd/spi/spansion.c @@ -199,8 +199,7 @@ static int spansion_write(struct spi_flash *flash, int spansion_erase(struct spi_flash *flash, u32 offset, size_t len) { struct spansion_spi_flash *spsn = to_spansion_spi_flash(flash); - return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE, - spsn->params->page_size * spsn->params->pages_per_sector, + return spi_flash_cmd_erase(flash, CMD_S25FLXX_SE, flash->sector_size, offset, len); } @@ -242,6 +241,7 @@ struct spi_flash *spi_flash_probe_spansion(struct spi_slave *spi, u8 *idcode) spsn->flash.read = spi_flash_cmd_read_fast; spsn->flash.size = params->page_size * params->pages_per_sector * params->nr_sectors; + spsn->flash.sector_size = spsn->flash.size/params->nr_sectors; printf("SF: Detected %s with page size %u, total ", params->name, params->page_size); diff --git a/drivers/mtd/spi/sst.c b/drivers/mtd/spi/sst.c index 792d04d..15de12b 100644 --- a/drivers/mtd/spi/sst.c +++ b/drivers/mtd/spi/sst.c @@ -201,7 +201,7 @@ sst_write(struct spi_flash *flash, u32 offset, size_t len, const void *buf) int sst_erase(struct spi_flash *flash, u32 offset, size_t len) { - return spi_flash_cmd_erase(flash, CMD_SST_SE, SST_SECTOR_SIZE, + return spi_flash_cmd_erase(flash, CMD_SST_SE, flash->sector_size, offset, len); } @@ -257,6 +257,7 @@ spi_flash_probe_sst(struct spi_slave *spi, u8 *idcode) stm->flash.write = sst_write; stm->flash.erase = sst_erase; stm->flash.size = SST_SECTOR_SIZE * params->nr_sectors; + stm->flash.sector_size = SST_SECTOR_SIZE; printf("SF: Detected %s with page size %u, total ", params->name, SST_SECTOR_SIZE); diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c index 7ef690d..80d838e 100644 --- a/drivers/mtd/spi/stmicro.c +++ b/drivers/mtd/spi/stmicro.c @@ -200,8 +200,7 @@ static int stmicro_write(struct spi_flash *flash, int stmicro_erase(struct spi_flash *flash, u32 offset, size_t len) { struct stmicro_spi_flash *stm = to_stmicro_spi_flash(flash); - return spi_flash_cmd_erase(flash, CMD_M25PXX_SE, - stm->params->page_size * stm->params->pages_per_sector, + return spi_flash_cmd_erase(flash, CMD_M25PXX_SE, flash->sector_size, offset, len); } @@ -251,6 +250,7 @@ struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) stm->flash.read = spi_flash_cmd_read_fast; stm->flash.size = params->page_size * params->pages_per_sector * params->nr_sectors; + stm->flash.sector_size = stm->flash.size/params->nr_sectors; printf("SF: Detected %s with page size %u, total ", params->name, params->page_size); diff --git a/drivers/mtd/spi/winbond.c b/drivers/mtd/spi/winbond.c index e88802f..ec6fb79 100644 --- a/drivers/mtd/spi/winbond.c +++ b/drivers/mtd/spi/winbond.c @@ -173,8 +173,7 @@ out: int winbond_erase(struct spi_flash *flash, u32 offset, size_t len) { struct winbond_spi_flash *stm = to_winbond_spi_flash(flash); - return spi_flash_cmd_erase(flash, CMD_W25_SE, - (1 << stm->params->l2_page_size) * stm->params->pages_per_sector, + return spi_flash_cmd_erase(flash, CMD_W25_SE, flash->sector_size, offset, len); } @@ -216,6 +215,8 @@ struct spi_flash *spi_flash_probe_winbond(struct spi_slave *spi, u8 *idcode) stm->flash.size = page_size * params->pages_per_sector * params->sectors_per_block * params->nr_blocks; + stm->flash.sector_size = (1 << stm->params->l2_page_size) * + stm->params->pages_per_sector; printf("SF: Detected %s with page size %u, total ", params->name, page_size); diff --git a/include/spi_flash.h b/include/spi_flash.h index 1f8ba29..039b94e 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -38,6 +38,8 @@ struct spi_flash { u32 size; + u16 sector_size; + int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter 2011-02-16 21:37 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun @ 2011-04-12 6:34 ` Mike Frysinger 0 siblings, 0 replies; 17+ messages in thread From: Mike Frysinger @ 2011-04-12 6:34 UTC (permalink / raw) To: u-boot these patches have been integrated/superseded by my recent sf patchset -mike -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. Url : http://lists.denx.de/pipermail/u-boot/attachments/20110412/4005fbe8/attachment.pgp ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command. 2011-02-16 21:06 ` Wolfgang Denk 2011-02-16 21:37 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun @ 2011-02-16 21:39 ` Richard Retanubun 1 sibling, 0 replies; 17+ messages in thread From: Richard Retanubun @ 2011-02-16 21:39 UTC (permalink / raw) To: u-boot This patch adds [+]len handler for the erase command that will automatically round up the requested erase length to the flash's sector_size. Signed-off-by: Richard Retanubun <RichardRetanubun@RuggedCom.com> --- v2: scrubbed via checkpatch, thanks WD! common/cmd_sf.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 48 insertions(+), 4 deletions(-) diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 6e7be81..17a4dd8 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -19,6 +19,48 @@ static struct spi_flash *flash; + +/* + * This function computes the length argument for the erase command. + * The length on which the command is to operate can be given in two forms: + * 1. <cmd> offset len - operate on <'offset', 'len') + * 2. <cmd> offset +len - operate on <'offset', 'round_up(len)') + * If the second form is used and the length doesn't fall on the + * sector boundary, than it will be adjusted to the next sector boundary. + * If it isn't in the flash, the function will fail (return -1). + * Input: + * arg: length specification (i.e. both command arguments) + * Output: + * len: computed length for operation + * Return: + * 1: success + * -1: failure (bad format, bad address). +*/ +static int sf_parse_len_arg(char *arg, ulong *len) +{ + char *ep; + char round_up_len; /* indicates if the "+length" form used */ + ulong len_arg; + + + round_up_len = 0; + if (*arg == '+') { + round_up_len = 1; + ++arg; + } + + len_arg = simple_strtoul(arg, &ep, 16); + if (ep == arg || *ep != '\0') + return -1; + + if (round_up_len && flash->sector_size > 0) + *len = DIV_ROUND_UP(len_arg, flash->sector_size); + else + *len = len_arg; + + return 1; +} + static int do_spi_flash_probe(int argc, char * const argv[]) { unsigned int bus = 0; @@ -135,8 +177,9 @@ static int do_spi_flash_erase(int argc, char * const argv[]) offset = simple_strtoul(argv[1], &endp, 16); if (*argv[1] == 0 || *endp != 0) goto usage; - len = simple_strtoul(argv[2], &endp, 16); - if (*argv[2] == 0 || *endp != 0) + + ret = sf_parse_len_arg(argv[2], &len); + if (ret != 1) goto usage; ret = spi_flash_erase(flash, offset, len); @@ -148,7 +191,7 @@ static int do_spi_flash_erase(int argc, char * const argv[]) return 0; usage: - puts("Usage: sf erase offset len\n"); + puts("Usage: sf erase offset [+]len\n"); return 1; } @@ -189,5 +232,6 @@ U_BOOT_CMD( " `offset' to memory at `addr'\n" "sf write addr offset len - write `len' bytes from memory\n" " at `addr' to flash at `offset'\n" - "sf erase offset len - erase `len' bytes from `offset'" + "sf erase offset [+]len - erase `len' bytes from `offset'\n" + " `+len' round up `len' to block size" ); -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command. 2011-02-16 20:27 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 2011-02-16 21:06 ` Wolfgang Denk @ 2011-02-16 21:47 ` Scott Wood 1 sibling, 0 replies; 17+ messages in thread From: Scott Wood @ 2011-02-16 21:47 UTC (permalink / raw) To: u-boot On Wed, 16 Feb 2011 15:27:54 -0500 Richard Retanubun <RichardRetanubun@ruggedcom.com> wrote: > This patch adds [+]len handler for the erase command that will > automatically round up the requested erase length to the flash's > sector_size. > --- > common/cmd_sf.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/common/cmd_sf.c b/common/cmd_sf.c > index 6e7be81..bbd4842 100644 > --- a/common/cmd_sf.c > +++ b/common/cmd_sf.c > @@ -19,6 +19,48 @@ > > static struct spi_flash *flash; > > + > +/* > + * This function computes the length argument for the erase command. > + * The length on which the command is to operate can be given in two forms: > + * 1. <cmd> offset len - operate on <'offset', 'len') > + * 2. <cmd> offset +len - operate on <'offset', 'round_up(len)') > + * If the second form is used and the length doesn't fall on the > + * sector boundary, than it will be adjusted to the next sector boundary. > + * If it isn't in the flash, the function will fail (return -1). On NOR, + is used to indicate that the second argument is a length, as opposed to an ending address. Rounding seems like a side effect of length mode. On NAND we unconditionally round up erase lengths, as we don't support ending-address mode (looks like SPI doesn't either). -Scott ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-04-12 6:34 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-08 14:13 [U-Boot] [U-boot] sf: API for spi_flash_get_sector_size Richard Retanubun 2011-02-08 14:40 ` Wolfgang Denk 2011-02-08 15:11 ` Richard Retanubun 2011-02-08 15:19 ` Wolfgang Denk 2011-02-08 15:33 ` Richard Retanubun 2011-02-09 21:16 ` [U-Boot] [PATCH] [RFC] SF: Add "sf erase offset +len" command handler Richard Retanubun 2011-02-15 9:34 ` Mike Frysinger 2011-02-16 20:27 ` [U-Boot] (no subject) Richard Retanubun 2011-02-17 5:46 ` Mike Frysinger 2011-02-16 20:27 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun 2011-02-16 21:00 ` Wolfgang Denk 2011-02-16 20:27 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 2011-02-16 21:06 ` Wolfgang Denk 2011-02-16 21:37 ` [U-Boot] [PATCH 1/2] SPI: Add struct spi_flash.sector_size parameter Richard Retanubun 2011-04-12 6:34 ` Mike Frysinger 2011-02-16 21:39 ` [U-Boot] [PATCH 2/2] cmd_sf: Add handler for +len arg for erase command Richard Retanubun 2011-02-16 21:47 ` Scott Wood
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox