| Message ID | 1418215892-17617-2-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
Hi Bin,
I think you have a different interpretation of sector size here-
/* The size listed here is what works with SPINOR_OP_SE, which isn't
* necessarily called a "sector" by the vendor.
*/
Say for example SST25VF040B has 8 sectors of which each sector size is
64 * 1024 out of this we can use 4K sector erase or 32K sector erase or
64K sector erase through flags.
Linux does follow the same-
/* SST -- large erase sizes are "overlays", "sectors" are 4K */
{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K |
SST_WRITE) },
{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K |
SST_WRITE) },
{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K |
SST_WRITE) },
{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K |
SST_WRITE) },
Please check it.
On 10 December 2014 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> Update SST25VF064C read command to RD_EXTN per datasheet.
> Also change flash sector size to 4KiB to match SECT_4K.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> drivers/mtd/spi/sf_params.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_params.c b/drivers/mtd/spi/sf_params.c
> index 30875b3..5482700 100644
> --- a/drivers/mtd/spi/sf_params.c
> +++ b/drivers/mtd/spi/sf_params.c
> @@ -89,16 +89,16 @@ const struct spi_flash_params spi_flash_params_table[] = {
> {"N25Q1024A", 0x20bb21, 0x0, 64 * 1024, 2048, RD_FULL, WR_QPP | E_FSR | SECT_4K},
> #endif
> #ifdef CONFIG_SPI_FLASH_SST /* SST */
> - {"SST25VF040B", 0xbf258d, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR},
> - {"SST25VF080B", 0xbf258e, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR},
> - {"SST25VF016B", 0xbf2541, 0x0, 64 * 1024, 32, RD_NORM, SECT_4K | SST_WR},
> - {"SST25VF032B", 0xbf254a, 0x0, 64 * 1024, 64, RD_NORM, SECT_4K | SST_WR},
> - {"SST25VF064C", 0xbf254b, 0x0, 64 * 1024, 128, RD_NORM, SECT_4K},
> - {"SST25WF512", 0xbf2501, 0x0, 64 * 1024, 1, RD_NORM, SECT_4K | SST_WR},
> - {"SST25WF010", 0xbf2502, 0x0, 64 * 1024, 2, RD_NORM, SECT_4K | SST_WR},
> - {"SST25WF020", 0xbf2503, 0x0, 64 * 1024, 4, RD_NORM, SECT_4K | SST_WR},
> - {"SST25WF040", 0xbf2504, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR},
> - {"SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR},
> + {"SST25VF040B", 0xbf258d, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR},
> + {"SST25VF080B", 0xbf258e, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR},
> + {"SST25VF016B", 0xbf2541, 0x0, 4 * 1024, 512, RD_NORM, SECT_4K | SST_WR},
> + {"SST25VF032B", 0xbf254a, 0x0, 4 * 1024, 1024, RD_NORM, SECT_4K | SST_WR},
> + {"SST25VF064C", 0xbf254b, 0x0, 4 * 1024, 2048, RD_EXTN, SECT_4K},
> + {"SST25WF512", 0xbf2501, 0x0, 4 * 1024, 16, RD_NORM, SECT_4K | SST_WR},
> + {"SST25WF010", 0xbf2502, 0x0, 4 * 1024, 32, RD_NORM, SECT_4K | SST_WR},
> + {"SST25WF020", 0xbf2503, 0x0, 4 * 1024, 64, RD_NORM, SECT_4K | SST_WR},
> + {"SST25WF040", 0xbf2504, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR},
> + {"SST25WF080", 0xbf2505, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR},
> #endif
> #ifdef CONFIG_SPI_FLASH_WINBOND /* WINBOND */
> {"W25P80", 0xef2014, 0x0, 64 * 1024, 16, RD_NORM, 0},
> --
> 1.8.2.1
>
thanks!
Hi Jagan, On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Bin, > > I think you have a different interpretation of sector size here- > > /* The size listed here is what works with SPINOR_OP_SE, which isn't > * necessarily called a "sector" by the vendor. > */ > Say for example SST25VF040B has 8 sectors of which each sector size is > 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or > 64K sector erase through flags. > > Linux does follow the same- > /* SST -- large erase sizes are "overlays", "sectors" are 4K */ > { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | > SST_WRITE) }, > { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | > SST_WRITE) }, > { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | > SST_WRITE) }, > { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | > SST_WRITE) }, > > Please check it. Yes, I know this pretty well. And I want to change this behavior, as my cover letter says. Currently the 'sf erase' command operates on a 64KB granularity, while the actual erase command is 4KB granularity, which is inconsistent and causes confusion. > On 10 December 2014 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote: >> Update SST25VF064C read command to RD_EXTN per datasheet. >> Also change flash sector size to 4KiB to match SECT_4K. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> drivers/mtd/spi/sf_params.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mtd/spi/sf_params.c b/drivers/mtd/spi/sf_params.c >> index 30875b3..5482700 100644 >> --- a/drivers/mtd/spi/sf_params.c >> +++ b/drivers/mtd/spi/sf_params.c >> @@ -89,16 +89,16 @@ const struct spi_flash_params spi_flash_params_table[] = { >> {"N25Q1024A", 0x20bb21, 0x0, 64 * 1024, 2048, RD_FULL, WR_QPP | E_FSR | SECT_4K}, >> #endif >> #ifdef CONFIG_SPI_FLASH_SST /* SST */ >> - {"SST25VF040B", 0xbf258d, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25VF080B", 0xbf258e, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25VF016B", 0xbf2541, 0x0, 64 * 1024, 32, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25VF032B", 0xbf254a, 0x0, 64 * 1024, 64, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25VF064C", 0xbf254b, 0x0, 64 * 1024, 128, RD_NORM, SECT_4K}, >> - {"SST25WF512", 0xbf2501, 0x0, 64 * 1024, 1, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25WF010", 0xbf2502, 0x0, 64 * 1024, 2, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25WF020", 0xbf2503, 0x0, 64 * 1024, 4, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25WF040", 0xbf2504, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR}, >> - {"SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25VF040B", 0xbf258d, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25VF080B", 0xbf258e, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25VF016B", 0xbf2541, 0x0, 4 * 1024, 512, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25VF032B", 0xbf254a, 0x0, 4 * 1024, 1024, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25VF064C", 0xbf254b, 0x0, 4 * 1024, 2048, RD_EXTN, SECT_4K}, >> + {"SST25WF512", 0xbf2501, 0x0, 4 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25WF010", 0xbf2502, 0x0, 4 * 1024, 32, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25WF020", 0xbf2503, 0x0, 4 * 1024, 64, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25WF040", 0xbf2504, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR}, >> + {"SST25WF080", 0xbf2505, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR}, >> #endif >> #ifdef CONFIG_SPI_FLASH_WINBOND /* WINBOND */ >> {"W25P80", 0xef2014, 0x0, 64 * 1024, 16, RD_NORM, 0}, >> -- Regards, Bin
Hi Bin, On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >> Hi Bin, >> >> I think you have a different interpretation of sector size here- >> >> /* The size listed here is what works with SPINOR_OP_SE, which isn't >> * necessarily called a "sector" by the vendor. >> */ >> Say for example SST25VF040B has 8 sectors of which each sector size is >> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >> 64K sector erase through flags. >> >> Linux does follow the same- >> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >> SST_WRITE) }, >> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >> SST_WRITE) }, >> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >> SST_WRITE) }, >> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >> SST_WRITE) }, >> >> Please check it. > > > Yes, I know this pretty well. And I want to change this behavior, as > my cover letter says. > > Currently the 'sf erase' command operates on a 64KB granularity, while > the actual erase command is 4KB granularity, which is inconsistent and > causes confusion. It never related to 'sf erase' instead based on the 'params->flags' sf_probe will decide which erase_cmd with erase_size will use. /* Compute erase sector and command */ if (params->flags & SECT_4K) { flash->erase_cmd = CMD_ERASE_4K; flash->erase_size = 4096 << flash->shift; } else if (params->flags & SECT_32K) { flash->erase_cmd = CMD_ERASE_32K; flash->erase_size = 32768 << flash->shift; } else { flash->erase_cmd = CMD_ERASE_64K; flash->erase_size = flash->sector_size; } And to be honest, these flashes were tested with lowest ie 4KB so even if they do support 64KB, we mentioned on 4KB on 'params->flags' as we tested well with that and it works consistently. > >> On 10 December 2014 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Update SST25VF064C read command to RD_EXTN per datasheet. >>> Also change flash sector size to 4KiB to match SECT_4K. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> drivers/mtd/spi/sf_params.c | 20 ++++++++++---------- >>> 1 file changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/mtd/spi/sf_params.c b/drivers/mtd/spi/sf_params.c >>> index 30875b3..5482700 100644 >>> --- a/drivers/mtd/spi/sf_params.c >>> +++ b/drivers/mtd/spi/sf_params.c >>> @@ -89,16 +89,16 @@ const struct spi_flash_params spi_flash_params_table[] = { >>> {"N25Q1024A", 0x20bb21, 0x0, 64 * 1024, 2048, RD_FULL, WR_QPP | E_FSR | SECT_4K}, >>> #endif >>> #ifdef CONFIG_SPI_FLASH_SST /* SST */ >>> - {"SST25VF040B", 0xbf258d, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25VF080B", 0xbf258e, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25VF016B", 0xbf2541, 0x0, 64 * 1024, 32, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25VF032B", 0xbf254a, 0x0, 64 * 1024, 64, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25VF064C", 0xbf254b, 0x0, 64 * 1024, 128, RD_NORM, SECT_4K}, >>> - {"SST25WF512", 0xbf2501, 0x0, 64 * 1024, 1, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25WF010", 0xbf2502, 0x0, 64 * 1024, 2, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25WF020", 0xbf2503, 0x0, 64 * 1024, 4, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25WF040", 0xbf2504, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR}, >>> - {"SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25VF040B", 0xbf258d, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25VF080B", 0xbf258e, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25VF016B", 0xbf2541, 0x0, 4 * 1024, 512, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25VF032B", 0xbf254a, 0x0, 4 * 1024, 1024, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25VF064C", 0xbf254b, 0x0, 4 * 1024, 2048, RD_EXTN, SECT_4K}, >>> + {"SST25WF512", 0xbf2501, 0x0, 4 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25WF010", 0xbf2502, 0x0, 4 * 1024, 32, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25WF020", 0xbf2503, 0x0, 4 * 1024, 64, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25WF040", 0xbf2504, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR}, >>> + {"SST25WF080", 0xbf2505, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR}, >>> #endif >>> #ifdef CONFIG_SPI_FLASH_WINBOND /* WINBOND */ >>> {"W25P80", 0xef2014, 0x0, 64 * 1024, 16, RD_NORM, 0}, >>> -- thanks!
Hi Jagan, On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Bin, > > On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>> Hi Bin, >>> >>> I think you have a different interpretation of sector size here- >>> >>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>> * necessarily called a "sector" by the vendor. >>> */ >>> Say for example SST25VF040B has 8 sectors of which each sector size is >>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>> 64K sector erase through flags. >>> >>> Linux does follow the same- >>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>> SST_WRITE) }, >>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>> SST_WRITE) }, >>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>> SST_WRITE) }, >>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>> SST_WRITE) }, >>> >>> Please check it. >> >> >> Yes, I know this pretty well. And I want to change this behavior, as >> my cover letter says. >> >> Currently the 'sf erase' command operates on a 64KB granularity, while >> the actual erase command is 4KB granularity, which is inconsistent and >> causes confusion. > > It never related to 'sf erase' instead based on the 'params->flags' > sf_probe will decide which erase_cmd with erase_size will use. No, it is related. See cmd_sf.c: 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 = ROUND(len_arg, flash->sector_size); else *len = len_arg; return 1; } So even you are passing 4KB in the flash params, the 'sf erase' command WILL erase 64KB. > /* Compute erase sector and command */ > if (params->flags & SECT_4K) { > flash->erase_cmd = CMD_ERASE_4K; > flash->erase_size = 4096 << flash->shift; > } else if (params->flags & SECT_32K) { > flash->erase_cmd = CMD_ERASE_32K; > flash->erase_size = 32768 << flash->shift; > } else { > flash->erase_cmd = CMD_ERASE_64K; > flash->erase_size = flash->sector_size; > } > > And to be honest, these flashes were tested with lowest ie 4KB so even if they > do support 64KB, we mentioned on 4KB on 'params->flags' as we tested > well with that > and it works consistently. > >> Regards, Bin
Hi Bin, On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >> Hi Bin, >> >> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> I think you have a different interpretation of sector size here- >>>> >>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>> * necessarily called a "sector" by the vendor. >>>> */ >>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>> 64K sector erase through flags. >>>> >>>> Linux does follow the same- >>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>> SST_WRITE) }, >>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>> SST_WRITE) }, >>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>> SST_WRITE) }, >>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>> SST_WRITE) }, >>>> >>>> Please check it. >>> >>> >>> Yes, I know this pretty well. And I want to change this behavior, as >>> my cover letter says. >>> >>> Currently the 'sf erase' command operates on a 64KB granularity, while >>> the actual erase command is 4KB granularity, which is inconsistent and >>> causes confusion. >> >> It never related to 'sf erase' instead based on the 'params->flags' >> sf_probe will decide which erase_cmd with erase_size will use. > > No, it is related. See cmd_sf.c: I'm not getting your point- how could it erase use 64K sector size instead of 4K. Suppose the sector size is 4K flash->sector_size = 0x1000 1. erase 4K len flash (it's total erase length) # sf erase 0x0 0x1000 len_arg = simple_strtoul(arg, &ep, 16); gives - 0x1000 *len becomes 0x1000 2. erase 4K+1 len flash # sf erase 0x0 +0x1001 len_arg = simple_strtoul(arg, &ep, 16); gives - 0x1001 *len becomes 0x2000 All the way when it goes to sf_ops.c erase will take by means of erase_size which is assigned in sf_probe.c based on flags like 4K 32K or 64K. > > 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 = ROUND(len_arg, flash->sector_size); > else > *len = len_arg; > > return 1; > } > > So even you are passing 4KB in the flash params, the 'sf erase' > command WILL erase 64KB. > >> /* Compute erase sector and command */ >> if (params->flags & SECT_4K) { >> flash->erase_cmd = CMD_ERASE_4K; >> flash->erase_size = 4096 << flash->shift; >> } else if (params->flags & SECT_32K) { >> flash->erase_cmd = CMD_ERASE_32K; >> flash->erase_size = 32768 << flash->shift; >> } else { >> flash->erase_cmd = CMD_ERASE_64K; >> flash->erase_size = flash->sector_size; >> } >> >> And to be honest, these flashes were tested with lowest ie 4KB so even if they >> do support 64KB, we mentioned on 4KB on 'params->flags' as we tested >> well with that >> and it works consistently. thanks!
Hi Jagan, On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Bin, > > On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>> Hi Bin, >>> >>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>> Hi Bin, >>>>> >>>>> I think you have a different interpretation of sector size here- >>>>> >>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>> * necessarily called a "sector" by the vendor. >>>>> */ >>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>> 64K sector erase through flags. >>>>> >>>>> Linux does follow the same- >>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>> SST_WRITE) }, >>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>> SST_WRITE) }, >>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>> SST_WRITE) }, >>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>> SST_WRITE) }, >>>>> >>>>> Please check it. >>>> >>>> >>>> Yes, I know this pretty well. And I want to change this behavior, as >>>> my cover letter says. >>>> >>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>> the actual erase command is 4KB granularity, which is inconsistent and >>>> causes confusion. >>> >>> It never related to 'sf erase' instead based on the 'params->flags' >>> sf_probe will decide which erase_cmd with erase_size will use. >> >> No, it is related. See cmd_sf.c: > > I'm not getting your point- how could it erase use 64K sector size > instead of 4K. It indeed erases 64K sector size. You need check the logic in spi_flash_validate_params(). > Suppose the sector size is 4K > > flash->sector_size = 0x1000 > 1. erase 4K len flash (it's total erase length) > > # sf erase 0x0 0x1000 > > len_arg = simple_strtoul(arg, &ep, 16); > gives - 0x1000 > > *len becomes 0x1000 > > 2. erase 4K+1 len flash > > # sf erase 0x0 +0x1001 > > len_arg = simple_strtoul(arg, &ep, 16); > gives - 0x1001 > > *len becomes 0x2000 > > All the way when it goes to sf_ops.c erase will take by means of > erase_size which is assigned in sf_probe.c based on flags like 4K > 32K or 64K. Regards, Bin
Hi Bin, On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >> Hi Bin, >> >> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, >>>>> >>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> I think you have a different interpretation of sector size here- >>>>>> >>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>> * necessarily called a "sector" by the vendor. >>>>>> */ >>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>> 64K sector erase through flags. >>>>>> >>>>>> Linux does follow the same- >>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>> SST_WRITE) }, >>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>> SST_WRITE) }, >>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>> SST_WRITE) }, >>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>> SST_WRITE) }, >>>>>> >>>>>> Please check it. >>>>> >>>>> >>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>> my cover letter says. >>>>> >>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>> causes confusion. >>>> >>>> It never related to 'sf erase' instead based on the 'params->flags' >>>> sf_probe will decide which erase_cmd with erase_size will use. >>> >>> No, it is related. See cmd_sf.c: >> >> I'm not getting your point- how could it erase use 64K sector size >> instead of 4K. > > It indeed erases 64K sector size. You need check the logic in > spi_flash_validate_params(). We're assigning erase_size to sector_size only when SECT_4K and SECT_32K and for these erase_size becomes direct values, please check this. /* Compute erase sector and command */ if (params->flags & SECT_4K) { flash->erase_cmd = CMD_ERASE_4K; flash->erase_size = 4096; } else if (params->flags & SECT_32K) { flash->erase_cmd = CMD_ERASE_32K; flash->erase_size = 32768; } else { flash->erase_cmd = CMD_ERASE_64K; flash->erase_size = flash->sector_size; } > >> Suppose the sector size is 4K >> >> flash->sector_size = 0x1000 >> 1. erase 4K len flash (it's total erase length) >> >> # sf erase 0x0 0x1000 >> >> len_arg = simple_strtoul(arg, &ep, 16); >> gives - 0x1000 >> >> *len becomes 0x1000 >> >> 2. erase 4K+1 len flash >> >> # sf erase 0x0 +0x1001 >> >> len_arg = simple_strtoul(arg, &ep, 16); >> gives - 0x1001 >> >> *len becomes 0x2000 >> >> All the way when it goes to sf_ops.c erase will take by means of >> erase_size which is assigned in sf_probe.c based on flags like 4K >> 32K or 64K. thanks!
Hi Jagan, On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Bin, > > On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>> Hi Bin, >>> >>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>> Hi Bin, >>>>> >>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>> Hi Bin, >>>>>>> >>>>>>> I think you have a different interpretation of sector size here- >>>>>>> >>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>>> * necessarily called a "sector" by the vendor. >>>>>>> */ >>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>>> 64K sector erase through flags. >>>>>>> >>>>>>> Linux does follow the same- >>>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>>> SST_WRITE) }, >>>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>>> SST_WRITE) }, >>>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>>> SST_WRITE) }, >>>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>>> SST_WRITE) }, >>>>>>> >>>>>>> Please check it. >>>>>> >>>>>> >>>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>>> my cover letter says. >>>>>> >>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>>> causes confusion. >>>>> >>>>> It never related to 'sf erase' instead based on the 'params->flags' >>>>> sf_probe will decide which erase_cmd with erase_size will use. >>>> >>>> No, it is related. See cmd_sf.c: >>> >>> I'm not getting your point- how could it erase use 64K sector size >>> instead of 4K. >> >> It indeed erases 64K sector size. You need check the logic in >> spi_flash_validate_params(). > > We're assigning erase_size to sector_size only when SECT_4K and SECT_32K > and for these erase_size becomes direct values, please check this. You previous email already said: the 'sf erase' command depends on *flash->sector_size* > /* Compute erase sector and command */ > if (params->flags & SECT_4K) { > flash->erase_cmd = CMD_ERASE_4K; > flash->erase_size = 4096; > } else if (params->flags & SECT_32K) { > flash->erase_cmd = CMD_ERASE_32K; > flash->erase_size = 32768; > } else { > flash->erase_cmd = CMD_ERASE_64K; > flash->erase_size = flash->sector_size; > } Here the codes says: *flash->erase_size* So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K. >> >>> Suppose the sector size is 4K >>> >>> flash->sector_size = 0x1000 >>> 1. erase 4K len flash (it's total erase length) >>> >>> # sf erase 0x0 0x1000 >>> >>> len_arg = simple_strtoul(arg, &ep, 16); >>> gives - 0x1000 >>> >>> *len becomes 0x1000 >>> >>> 2. erase 4K+1 len flash >>> >>> # sf erase 0x0 +0x1001 >>> >>> len_arg = simple_strtoul(arg, &ep, 16); >>> gives - 0x1001 >>> >>> *len becomes 0x2000 >>> >>> All the way when it goes to sf_ops.c erase will take by means of >>> erase_size which is assigned in sf_probe.c based on flags like 4K >>> 32K or 64K. > > thanks! > -- Regards, Bin
Hi Bin, On 22 April 2015 at 12:44, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >> Hi Bin, >> >> On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, >>>>> >>>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>> Hi Bin, >>>>>>>> >>>>>>>> I think you have a different interpretation of sector size here- >>>>>>>> >>>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>>>> * necessarily called a "sector" by the vendor. >>>>>>>> */ >>>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>>>> 64K sector erase through flags. >>>>>>>> >>>>>>>> Linux does follow the same- >>>>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>>>> SST_WRITE) }, >>>>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>>>> SST_WRITE) }, >>>>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>>>> SST_WRITE) }, >>>>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>>>> SST_WRITE) }, >>>>>>>> >>>>>>>> Please check it. >>>>>>> >>>>>>> >>>>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>>>> my cover letter says. >>>>>>> >>>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>>>> causes confusion. >>>>>> >>>>>> It never related to 'sf erase' instead based on the 'params->flags' >>>>>> sf_probe will decide which erase_cmd with erase_size will use. >>>>> >>>>> No, it is related. See cmd_sf.c: >>>> >>>> I'm not getting your point- how could it erase use 64K sector size >>>> instead of 4K. >>> >>> It indeed erases 64K sector size. You need check the logic in >>> spi_flash_validate_params(). >> >> We're assigning erase_size to sector_size only when SECT_4K and SECT_32K >> and for these erase_size becomes direct values, please check this. > > You previous email already said: the 'sf erase' command depends on > *flash->sector_size* > >> /* Compute erase sector and command */ >> if (params->flags & SECT_4K) { >> flash->erase_cmd = CMD_ERASE_4K; >> flash->erase_size = 4096; >> } else if (params->flags & SECT_32K) { >> flash->erase_cmd = CMD_ERASE_32K; >> flash->erase_size = 32768; >> } else { >> flash->erase_cmd = CMD_ERASE_64K; >> flash->erase_size = flash->sector_size; >> } > > Here the codes says: *flash->erase_size* > > So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K. Example: "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, sf probe gives sector_size = 64 * 1024 and erase_size = 4096 sf erase 0 100 sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns "SF: Erase offset/length not multiple of erase size" Example: "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, SST_WR}, sf probe gives sector_size = 64 * 1024 and erase_size = 64 * 1024 sf erase 0 100 sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns "SF: Erase offset/length not multiple of erase size" Still have any concerns, please come to IRC for more discussion > >>> >>>> Suppose the sector size is 4K >>>> >>>> flash->sector_size = 0x1000 >>>> 1. erase 4K len flash (it's total erase length) >>>> >>>> # sf erase 0x0 0x1000 >>>> >>>> len_arg = simple_strtoul(arg, &ep, 16); >>>> gives - 0x1000 >>>> >>>> *len becomes 0x1000 >>>> >>>> 2. erase 4K+1 len flash >>>> >>>> # sf erase 0x0 +0x1001 >>>> >>>> len_arg = simple_strtoul(arg, &ep, 16); >>>> gives - 0x1001 >>>> >>>> *len becomes 0x2000 >>>> >>>> All the way when it goes to sf_ops.c erase will take by means of >>>> erase_size which is assigned in sf_probe.c based on flags like 4K >>>> 32K or 64K. thanks!
Hi Jagan, On Wed, Apr 22, 2015 at 4:06 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Bin, > > On 22 April 2015 at 12:44, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>> Hi Bin, >>> >>> On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>> Hi Bin, >>>>> >>>>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> I think you have a different interpretation of sector size here- >>>>>>>>> >>>>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>>>>> * necessarily called a "sector" by the vendor. >>>>>>>>> */ >>>>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>>>>> 64K sector erase through flags. >>>>>>>>> >>>>>>>>> Linux does follow the same- >>>>>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>>>>> SST_WRITE) }, >>>>>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>>>>> SST_WRITE) }, >>>>>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>>>>> SST_WRITE) }, >>>>>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>>>>> SST_WRITE) }, >>>>>>>>> >>>>>>>>> Please check it. >>>>>>>> >>>>>>>> >>>>>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>>>>> my cover letter says. >>>>>>>> >>>>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>>>>> causes confusion. >>>>>>> >>>>>>> It never related to 'sf erase' instead based on the 'params->flags' >>>>>>> sf_probe will decide which erase_cmd with erase_size will use. >>>>>> >>>>>> No, it is related. See cmd_sf.c: >>>>> >>>>> I'm not getting your point- how could it erase use 64K sector size >>>>> instead of 4K. >>>> >>>> It indeed erases 64K sector size. You need check the logic in >>>> spi_flash_validate_params(). >>> >>> We're assigning erase_size to sector_size only when SECT_4K and SECT_32K >>> and for these erase_size becomes direct values, please check this. >> >> You previous email already said: the 'sf erase' command depends on >> *flash->sector_size* >> >>> /* Compute erase sector and command */ >>> if (params->flags & SECT_4K) { >>> flash->erase_cmd = CMD_ERASE_4K; >>> flash->erase_size = 4096; >>> } else if (params->flags & SECT_32K) { >>> flash->erase_cmd = CMD_ERASE_32K; >>> flash->erase_size = 32768; >>> } else { >>> flash->erase_cmd = CMD_ERASE_64K; >>> flash->erase_size = flash->sector_size; >>> } >> >> Here the codes says: *flash->erase_size* >> >> So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K. > > Example: > "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, > SECT_4K | SST_WR}, > > sf probe gives > sector_size = 64 * 1024 and erase_size = 4096 > > sf erase 0 100 > sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns > "SF: Erase offset/length not multiple of erase size" sf erase 0 +100. Sorry for the typo. But looks like you are not really reading the codes. => sf probe SF: Detected SST25VF016B with page size 256 Bytes, erase size 4 KiB, total 2 MiB, mapped at ffe00000 => sf erase 0 +100 SF: 65536 bytes @ 0x0 Erased: OK Tested on two boards, and both shows 64K was erased. > Example: > "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, > SST_WR}, > > sf probe gives > sector_size = 64 * 1024 and erase_size = 64 * 1024 > > sf erase 0 100 > sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns > "SF: Erase offset/length not multiple of erase size" > > Still have any concerns, please come to IRC for more discussion > Regards, Bin
Hi Bin, On 22 April 2015 at 14:13, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Wed, Apr 22, 2015 at 4:06 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >> Hi Bin, >> >> On 22 April 2015 at 12:44, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, >>>>> >>>>> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>> Hi Bin, >>>>>>>> >>>>>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>>>> Hi Bin, >>>>>>>>>> >>>>>>>>>> I think you have a different interpretation of sector size here- >>>>>>>>>> >>>>>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>>>>>> * necessarily called a "sector" by the vendor. >>>>>>>>>> */ >>>>>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>>>>>> 64K sector erase through flags. >>>>>>>>>> >>>>>>>>>> Linux does follow the same- >>>>>>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>>>>>> SST_WRITE) }, >>>>>>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>>>>>> SST_WRITE) }, >>>>>>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>>>>>> SST_WRITE) }, >>>>>>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>>>>>> SST_WRITE) }, >>>>>>>>>> >>>>>>>>>> Please check it. >>>>>>>>> >>>>>>>>> >>>>>>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>>>>>> my cover letter says. >>>>>>>>> >>>>>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>>>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>>>>>> causes confusion. >>>>>>>> >>>>>>>> It never related to 'sf erase' instead based on the 'params->flags' >>>>>>>> sf_probe will decide which erase_cmd with erase_size will use. >>>>>>> >>>>>>> No, it is related. See cmd_sf.c: >>>>>> >>>>>> I'm not getting your point- how could it erase use 64K sector size >>>>>> instead of 4K. >>>>> >>>>> It indeed erases 64K sector size. You need check the logic in >>>>> spi_flash_validate_params(). >>>> >>>> We're assigning erase_size to sector_size only when SECT_4K and SECT_32K >>>> and for these erase_size becomes direct values, please check this. >>> >>> You previous email already said: the 'sf erase' command depends on >>> *flash->sector_size* >>> >>>> /* Compute erase sector and command */ >>>> if (params->flags & SECT_4K) { >>>> flash->erase_cmd = CMD_ERASE_4K; >>>> flash->erase_size = 4096; >>>> } else if (params->flags & SECT_32K) { >>>> flash->erase_cmd = CMD_ERASE_32K; >>>> flash->erase_size = 32768; >>>> } else { >>>> flash->erase_cmd = CMD_ERASE_64K; >>>> flash->erase_size = flash->sector_size; >>>> } >>> >>> Here the codes says: *flash->erase_size* >>> >>> So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K. >> >> Example: >> "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, >> SECT_4K | SST_WR}, >> >> sf probe gives >> sector_size = 64 * 1024 and erase_size = 4096 >> >> sf erase 0 100 >> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns >> "SF: Erase offset/length not multiple of erase size" > > sf erase 0 +100. Sorry for the typo. But looks like you are not really > reading the codes. > Worked on too-many overclocked issue, sorry for that. So, something fixed in sf_probe.c will fix this I guess. > => sf probe > SF: Detected SST25VF016B with page size 256 Bytes, erase size 4 KiB, > total 2 MiB, mapped at ffe00000 > > => sf erase 0 +100 > SF: 65536 bytes @ 0x0 Erased: OK > > Tested on two boards, and both shows 64K was erased. > >> Example: >> "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, >> SST_WR}, >> >> sf probe gives >> sector_size = 64 * 1024 and erase_size = 64 * 1024 >> >> sf erase 0 100 >> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns >> "SF: Erase offset/length not multiple of erase size" >> >> Still have any concerns, please come to IRC for more discussion thanks!
Hi Jagan, On Wed, Apr 22, 2015 at 5:15 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Bin, > > On 22 April 2015 at 14:13, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Wed, Apr 22, 2015 at 4:06 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>> Hi Bin, >>> >>> On 22 April 2015 at 12:44, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>> Hi Bin, >>>>> >>>>> On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>> Hi Jagan, >>>>>>>>>> >>>>>>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>>>>> Hi Bin, >>>>>>>>>>> >>>>>>>>>>> I think you have a different interpretation of sector size here- >>>>>>>>>>> >>>>>>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>>>>>>> * necessarily called a "sector" by the vendor. >>>>>>>>>>> */ >>>>>>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>>>>>>> 64K sector erase through flags. >>>>>>>>>>> >>>>>>>>>>> Linux does follow the same- >>>>>>>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>>>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>> >>>>>>>>>>> Please check it. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>>>>>>> my cover letter says. >>>>>>>>>> >>>>>>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>>>>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>>>>>>> causes confusion. >>>>>>>>> >>>>>>>>> It never related to 'sf erase' instead based on the 'params->flags' >>>>>>>>> sf_probe will decide which erase_cmd with erase_size will use. >>>>>>>> >>>>>>>> No, it is related. See cmd_sf.c: >>>>>>> >>>>>>> I'm not getting your point- how could it erase use 64K sector size >>>>>>> instead of 4K. >>>>>> >>>>>> It indeed erases 64K sector size. You need check the logic in >>>>>> spi_flash_validate_params(). >>>>> >>>>> We're assigning erase_size to sector_size only when SECT_4K and SECT_32K >>>>> and for these erase_size becomes direct values, please check this. >>>> >>>> You previous email already said: the 'sf erase' command depends on >>>> *flash->sector_size* >>>> >>>>> /* Compute erase sector and command */ >>>>> if (params->flags & SECT_4K) { >>>>> flash->erase_cmd = CMD_ERASE_4K; >>>>> flash->erase_size = 4096; >>>>> } else if (params->flags & SECT_32K) { >>>>> flash->erase_cmd = CMD_ERASE_32K; >>>>> flash->erase_size = 32768; >>>>> } else { >>>>> flash->erase_cmd = CMD_ERASE_64K; >>>>> flash->erase_size = flash->sector_size; >>>>> } >>>> >>>> Here the codes says: *flash->erase_size* >>>> >>>> So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K. >>> >>> Example: >>> "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, >>> SECT_4K | SST_WR}, >>> >>> sf probe gives >>> sector_size = 64 * 1024 and erase_size = 4096 >>> >>> sf erase 0 100 >>> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns >>> "SF: Erase offset/length not multiple of erase size" >> >> sf erase 0 +100. Sorry for the typo. But looks like you are not really >> reading the codes. >> > > Worked on too-many overclocked issue, sorry for that. > > So, something fixed in sf_probe.c will fix this I guess. Good, you finally got it! So you prefer fixing this inconsistency in sf_probe.c? I guess by overriding flash->sector_size and flash->nr_sectors if SECT_4K? >> => sf probe >> SF: Detected SST25VF016B with page size 256 Bytes, erase size 4 KiB, >> total 2 MiB, mapped at ffe00000 >> >> => sf erase 0 +100 >> SF: 65536 bytes @ 0x0 Erased: OK >> >> Tested on two boards, and both shows 64K was erased. >> >>> Example: >>> "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, >>> SST_WR}, >>> >>> sf probe gives >>> sector_size = 64 * 1024 and erase_size = 64 * 1024 >>> >>> sf erase 0 100 >>> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns >>> "SF: Erase offset/length not multiple of erase size" >>> >>> Still have any concerns, please come to IRC for more discussion > As you see I have rebased this patch once for v2/v3 and lots of effort were spent on this series. I remember you said this patch series needs some testing on your side, but this comment shows that you may want to do it in another way. I really hope such comments could be sent months ago. Today I can't even remember all of the details in this series. Luckily I don't lose interest to get this upstream so I kept asking for an update. Regards, Bin
Hi Bin, On 22 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Wed, Apr 22, 2015 at 5:15 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >> Hi Bin, >> >> On 22 April 2015 at 14:13, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Wed, Apr 22, 2015 at 4:06 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> On 22 April 2015 at 12:44, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, >>>>> >>>>> On Wed, Apr 22, 2015 at 3:03 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 22 April 2015 at 12:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Tue, Apr 21, 2015 at 8:47 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>> Hi Bin, >>>>>>>> >>>>>>>> On 20 April 2015 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On Fri, Apr 17, 2015 at 4:48 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>>>> Hi Bin, >>>>>>>>>> >>>>>>>>>> On 17 April 2015 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>>> Hi Jagan, >>>>>>>>>>> >>>>>>>>>>> On Fri, Apr 17, 2015 at 2:09 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote: >>>>>>>>>>>> Hi Bin, >>>>>>>>>>>> >>>>>>>>>>>> I think you have a different interpretation of sector size here- >>>>>>>>>>>> >>>>>>>>>>>> /* The size listed here is what works with SPINOR_OP_SE, which isn't >>>>>>>>>>>> * necessarily called a "sector" by the vendor. >>>>>>>>>>>> */ >>>>>>>>>>>> Say for example SST25VF040B has 8 sectors of which each sector size is >>>>>>>>>>>> 64 * 1024 out of this we can use 4K sector erase or 32K sector erase or >>>>>>>>>>>> 64K sector erase through flags. >>>>>>>>>>>> >>>>>>>>>>>> Linux does follow the same- >>>>>>>>>>>> /* SST -- large erase sizes are "overlays", "sectors" are 4K */ >>>>>>>>>>>> { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, SECT_4K | >>>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>>> { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | >>>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>>> { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | >>>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>>> { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | >>>>>>>>>>>> SST_WRITE) }, >>>>>>>>>>>> >>>>>>>>>>>> Please check it. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yes, I know this pretty well. And I want to change this behavior, as >>>>>>>>>>> my cover letter says. >>>>>>>>>>> >>>>>>>>>>> Currently the 'sf erase' command operates on a 64KB granularity, while >>>>>>>>>>> the actual erase command is 4KB granularity, which is inconsistent and >>>>>>>>>>> causes confusion. >>>>>>>>>> >>>>>>>>>> It never related to 'sf erase' instead based on the 'params->flags' >>>>>>>>>> sf_probe will decide which erase_cmd with erase_size will use. >>>>>>>>> >>>>>>>>> No, it is related. See cmd_sf.c: >>>>>>>> >>>>>>>> I'm not getting your point- how could it erase use 64K sector size >>>>>>>> instead of 4K. >>>>>>> >>>>>>> It indeed erases 64K sector size. You need check the logic in >>>>>>> spi_flash_validate_params(). >>>>>> >>>>>> We're assigning erase_size to sector_size only when SECT_4K and SECT_32K >>>>>> and for these erase_size becomes direct values, please check this. >>>>> >>>>> You previous email already said: the 'sf erase' command depends on >>>>> *flash->sector_size* >>>>> >>>>>> /* Compute erase sector and command */ >>>>>> if (params->flags & SECT_4K) { >>>>>> flash->erase_cmd = CMD_ERASE_4K; >>>>>> flash->erase_size = 4096; >>>>>> } else if (params->flags & SECT_32K) { >>>>>> flash->erase_cmd = CMD_ERASE_32K; >>>>>> flash->erase_size = 32768; >>>>>> } else { >>>>>> flash->erase_cmd = CMD_ERASE_64K; >>>>>> flash->erase_size = flash->sector_size; >>>>>> } >>>>> >>>>> Here the codes says: *flash->erase_size* >>>>> >>>>> So when I give a 'sf erase 0 100' it erase 64KB even if you have SECT_4K. >>>> >>>> Example: >>>> "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, >>>> SECT_4K | SST_WR}, >>>> >>>> sf probe gives >>>> sector_size = 64 * 1024 and erase_size = 4096 >>>> >>>> sf erase 0 100 >>>> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns >>>> "SF: Erase offset/length not multiple of erase size" >>> >>> sf erase 0 +100. Sorry for the typo. But looks like you are not really >>> reading the codes. >>> >> >> Worked on too-many overclocked issue, sorry for that. >> >> So, something fixed in sf_probe.c will fix this I guess. > > Good, you finally got it! So you prefer fixing this inconsistency in > sf_probe.c? I guess by overriding flash->sector_size and > flash->nr_sectors if SECT_4K? I'm posting patch. > >>> => sf probe >>> SF: Detected SST25VF016B with page size 256 Bytes, erase size 4 KiB, >>> total 2 MiB, mapped at ffe00000 >>> >>> => sf erase 0 +100 >>> SF: 65536 bytes @ 0x0 Erased: OK >>> >>> Tested on two boards, and both shows 64K was erased. >>> >>>> Example: >>>> "SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, >>>> SST_WR}, >>>> >>>> sf probe gives >>>> sector_size = 64 * 1024 and erase_size = 64 * 1024 >>>> >>>> sf erase 0 100 >>>> sf_parse_len_arg len returns 100 and spi_flash_cmd_erase_ops returns >>>> "SF: Erase offset/length not multiple of erase size" >>>> >>>> Still have any concerns, please come to IRC for more discussion >> > > As you see I have rebased this patch once for v2/v3 and lots of effort > were spent on this series. I remember you said this patch series needs > some testing on your side, but this comment shows that you may want to > do it in another way. I really hope such comments could be sent months > ago. Today I can't even remember all of the details in this series. > Luckily I don't lose interest to get this upstream so I kept asking > for an update. Yes - I did some testing for Micron patch at-least and even I post the comment for the same. I have a plan to test the remaining patches as well and in-fact these questions I got it from sst patch(this patch). I agree some delay in my side but as these are core changes and I need to see each and test them respectively, that is my main Job. I'm very much thankful to you for keeping me updates. thanks!
diff --git a/drivers/mtd/spi/sf_params.c b/drivers/mtd/spi/sf_params.c index 30875b3..5482700 100644 --- a/drivers/mtd/spi/sf_params.c +++ b/drivers/mtd/spi/sf_params.c @@ -89,16 +89,16 @@ const struct spi_flash_params spi_flash_params_table[] = { {"N25Q1024A", 0x20bb21, 0x0, 64 * 1024, 2048, RD_FULL, WR_QPP | E_FSR | SECT_4K}, #endif #ifdef CONFIG_SPI_FLASH_SST /* SST */ - {"SST25VF040B", 0xbf258d, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR}, - {"SST25VF080B", 0xbf258e, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, - {"SST25VF016B", 0xbf2541, 0x0, 64 * 1024, 32, RD_NORM, SECT_4K | SST_WR}, - {"SST25VF032B", 0xbf254a, 0x0, 64 * 1024, 64, RD_NORM, SECT_4K | SST_WR}, - {"SST25VF064C", 0xbf254b, 0x0, 64 * 1024, 128, RD_NORM, SECT_4K}, - {"SST25WF512", 0xbf2501, 0x0, 64 * 1024, 1, RD_NORM, SECT_4K | SST_WR}, - {"SST25WF010", 0xbf2502, 0x0, 64 * 1024, 2, RD_NORM, SECT_4K | SST_WR}, - {"SST25WF020", 0xbf2503, 0x0, 64 * 1024, 4, RD_NORM, SECT_4K | SST_WR}, - {"SST25WF040", 0xbf2504, 0x0, 64 * 1024, 8, RD_NORM, SECT_4K | SST_WR}, - {"SST25WF080", 0xbf2505, 0x0, 64 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, + {"SST25VF040B", 0xbf258d, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR}, + {"SST25VF080B", 0xbf258e, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR}, + {"SST25VF016B", 0xbf2541, 0x0, 4 * 1024, 512, RD_NORM, SECT_4K | SST_WR}, + {"SST25VF032B", 0xbf254a, 0x0, 4 * 1024, 1024, RD_NORM, SECT_4K | SST_WR}, + {"SST25VF064C", 0xbf254b, 0x0, 4 * 1024, 2048, RD_EXTN, SECT_4K}, + {"SST25WF512", 0xbf2501, 0x0, 4 * 1024, 16, RD_NORM, SECT_4K | SST_WR}, + {"SST25WF010", 0xbf2502, 0x0, 4 * 1024, 32, RD_NORM, SECT_4K | SST_WR}, + {"SST25WF020", 0xbf2503, 0x0, 4 * 1024, 64, RD_NORM, SECT_4K | SST_WR}, + {"SST25WF040", 0xbf2504, 0x0, 4 * 1024, 128, RD_NORM, SECT_4K | SST_WR}, + {"SST25WF080", 0xbf2505, 0x0, 4 * 1024, 256, RD_NORM, SECT_4K | SST_WR}, #endif #ifdef CONFIG_SPI_FLASH_WINBOND /* WINBOND */ {"W25P80", 0xef2014, 0x0, 64 * 1024, 16, RD_NORM, 0},
Update SST25VF064C read command to RD_EXTN per datasheet. Also change flash sector size to 4KiB to match SECT_4K. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- drivers/mtd/spi/sf_params.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)