| Message ID | 20180518093233.24241-1-tudor.ambarus@microchip.com |
|---|---|
| State | Superseded |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
| Series | [RFC] mtd: spi-nor: add support to non-uniform SPI NOR flash memories | expand |
On 05/18/2018 11:32 AM, Tudor Ambarus wrote: > From: Cyrille Pitchen <cyrille.pitchen@microchip.com> > > This patch is a first step in introducing the support of SPI memories > with non-uniform erase sizes like Spansion s25fs512s. > > It introduces the memory erase map which splits the memory array into one > or many erase regions. Each erase region supports up to 4 erase commands, > as defined by the JEDEC JESD216B (SFDP) specification. > In turn, an erase command is defined by an op code and a sector size. > > To be backward compatible, the erase map of uniform SPI NOR flash memories > is initialized so it contains only one erase region and this erase region > supports only one erase command. Hence a single size is used to erase any > sector/block of the memory. > > Besides, since the algorithm used to erase sectors on non-uniform SPI NOR > flash memories is quite expensive, when possible, the erase map is tuned > to come back to the uniform case. > > This is a transitional patch: non-uniform erase maps will be used later > when initialized based on the SFDP data. What about non-SFDP non-linear flashes ? > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> > > [tudor.ambarus@microchip.com: > - add improvements on how the erase map is handled. The map is an array > describing the boundaries of the erase regions. LSB bits of the region's > offset are used to describe the supported erase types, to indicate if > that specific region is the last region in the map and to mark if the > region is overlaid or not. When one sends an addr and len to erase a > chunk of memory, we identify in which region the address fits, we start > erasing with the best fitted erase commands and when the region ends, > continue to erase from the next region. The erase is optimal: identify > the start offset (once), then erase with the best erase command, > move forward and repeat. Is that like an R-tree ? > - order erase types by size, with the biggest erase type at BIT(0). With > this, we can iterate from the biggest supported erase type to the smallest, > and when find one that meets all the required conditions, break the loop. > This saves time in determining the best erase cmd. > > - minimize the amount of erase() calls by using the best sequence of erase > type commands depending on alignment. Nice, this was long overdue > - replace spi_nor_find_uniform_erase() with spi_nor_select_uniform_erase(). > Even for the SPI NOR memories with non-uniform erase types, we can determine > at init if there are erase types that can erase the entire memory. Fill at > init the uniform_erase_type bitmask, to encode the erase type commands that > can erase the entire memory. > > - clarify support for overlaid regions. Considering one of the erase maps > of the S25FS512S memory: > Bottom: 8x 4KB sectors at bottom (only 4KB erase supported), > 1x overlaid 224KB sector at bottom (only 256KB erase supported), > 255x 256KB sectors (only 256KB erase supported) > S25FS512S states that 'if a sector erase command is applied to a 256KB range > that is overlaid by 4KB secors, the overlaid 4kB sectors are not affected by > the erase'. When at init, the overlaid region size should be set to > region->size = erase_size - count; in order to not miss chunks of data > when traversing the regions. > > - backward compatibility test done on MX25L25673G. > > The 'erase with the best command, move forward and repeat' approach was > suggested by Cristian Birsan in a brainstorm session, so: > ] > Suggested-by: Cristian Birsan <cristian.birsan@microchip.com> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 281 +++++++++++++++++++++++++++++++++++++++--- > include/linux/mtd/spi-nor.h | 89 +++++++++++++ > 2 files changed, 356 insertions(+), 14 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 494b7a2..bb70664 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor, > nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode); > nor->program_opcode = spi_nor_convert_3to4_program(nor->program_opcode); > nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode); > + > + if (!spi_nor_has_uniform_erase(nor)) { > + struct spi_nor_erase_map *map = &nor->erase_map; > + struct spi_nor_erase_command *cmd; > + int i; > + > + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { > + cmd = &map->commands[i]; > + cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode); > + } > + } > } > > /* Enable/disable 4-byte addressing mode. */ > @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) > return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width); > } > > +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */ > +static inline u64 > +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd, > + u64 dividend, u32 *remainder) > +{ > + *remainder = (u32)dividend & cmd->size_mask; > + return dividend >> cmd->size_shift; > +} > + > +static const struct spi_nor_erase_command * > +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map, > + const struct spi_nor_erase_region *region, u64 addr, > + u32 len) > +{ > + const struct spi_nor_erase_command *cmd; > + u32 rem; > + int i; > + u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK; > + > + /* > + * Commands are ordered by size, with the biggest erase type at > + * index 0. > + */ > + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { > + /* Does the erase region support the tested erase command? */ > + if (!(cmd_mask & BIT(i))) > + continue; > + > + cmd = &map->commands[i]; > + > + /* Don't erase more than what the user has asked for. */ > + if (cmd->size > len) > + continue; Are you sure checking for the full erase block length first and then checking if you can sub-erase the block is OK ? > + if (!(region->offset & SNOR_OVERLAID_REGION)) { > + /* 'addr' must be aligned to the erase size. */ > + spi_nor_div_by_erase_size(cmd, addr, &rem); > + continue; > + } else { > + /* > + * 'cmd' will erase the remaining of the overlaid > + * region. > + */ > + return cmd; > + } > + } > + > + return NULL; > +} > + > +static const struct spi_nor_erase_region * > +spi_nor_region_next(const struct spi_nor_erase_region *region) > +{ > + if (spi_nor_region_is_last(region)) > + return NULL; > + region++; > + return region; > +} > + > +static const struct spi_nor_erase_region * > +spi_nor_find_erase_region(const struct spi_nor_erase_map *map, u64 addr, > + u32 len) > +{ > + const struct spi_nor_erase_region *region = map->regions; > + u64 region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK; > + u64 region_end = region_start + region->size; > + > + if (!len) > + return ERR_PTR(-EINVAL); > + > + while (addr < region_start || addr > region_end) { > + region = spi_nor_region_next(region); > + if (!region) > + return ERR_PTR(-EINVAL); > + > + region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK; > + region_end = region_start + region->size; > + } > + > + return region; > +} > + > +static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len) > +{ > + const struct spi_nor_erase_map *map = &nor->erase_map; > + const struct spi_nor_erase_command *cmd; > + const struct spi_nor_erase_region *region; > + u64 region_end; > + int ret; > + > + region = spi_nor_find_erase_region(map, addr, len); > + if (IS_ERR(region)) > + return PTR_ERR(region); > + > + region_end = spi_nor_region_end(region); > + > + while (len) { > + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); > + if (!cmd) > + return -EINVAL; What would happen if you realize mid-way that you cannot erase some sector , do you end up with partial erase ? [...]
Hi, Marek, On 05/21/2018 02:35 PM, Marek Vasut wrote: > On 05/18/2018 11:32 AM, Tudor Ambarus wrote: >> From: Cyrille Pitchen <cyrille.pitchen@microchip.com> >> >> This patch is a first step in introducing the support of SPI memories >> with non-uniform erase sizes like Spansion s25fs512s. >> >> It introduces the memory erase map which splits the memory array into one >> or many erase regions. Each erase region supports up to 4 erase commands, >> as defined by the JEDEC JESD216B (SFDP) specification. >> In turn, an erase command is defined by an op code and a sector size. >> >> To be backward compatible, the erase map of uniform SPI NOR flash memories >> is initialized so it contains only one erase region and this erase region >> supports only one erase command. Hence a single size is used to erase any >> sector/block of the memory. >> >> Besides, since the algorithm used to erase sectors on non-uniform SPI NOR >> flash memories is quite expensive, when possible, the erase map is tuned >> to come back to the uniform case. >> >> This is a transitional patch: non-uniform erase maps will be used later >> when initialized based on the SFDP data. > > What about non-SFDP non-linear flashes ? Non-SFDP non-uniform flashes support is not addressed with this proposal, I should have told this in the commit message, thanks. But we are backward compatible, if non-SFDP, the flashes are considered uniform. > >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> >> >> [tudor.ambarus@microchip.com: >> - add improvements on how the erase map is handled. The map is an array >> describing the boundaries of the erase regions. LSB bits of the region's >> offset are used to describe the supported erase types, to indicate if >> that specific region is the last region in the map and to mark if the >> region is overlaid or not. When one sends an addr and len to erase a >> chunk of memory, we identify in which region the address fits, we start >> erasing with the best fitted erase commands and when the region ends, >> continue to erase from the next region. The erase is optimal: identify >> the start offset (once), then erase with the best erase command, >> move forward and repeat. > > Is that like an R-tree ? Not really. I find this RFC proposal faster and neat, but I'm open for suggestions and guidance. One wants to erase a contiguous chunk of memory and sends us the starting address and the total length. The algorithm of finding the best sequence of erase commands can be summarized in four steps: 1. Find in which region the address fits. This step is done only once, at the beginning. For the non-uniform SFDP-defined flashes, usually there are two or three regions defined. Nevertheless, in the worst case, the maximum number of regions that can be defined is on eight bits, so 255. Linear search for just 255 elements in the worst case looks good for me, especially that we do this search once. 2. Find the *best* erase command that is defined in that region. Each region can define maximum 4 erase commands. *Best* is defined as the largest/biggest supported erase command with which the provided address is aligned and which does not erase more that what the user has asked for. In case of overlaid regions, alignment does not matter. The largest command will erase the remaining of the overlaid region without touching the region with which it overlaps (see S25FS512S). The supported erase commands are ordered by size with the biggest queried first. It is desirable to erase with large erase commands so that we erase as much as we can in one shoot, minimizing the erase() calls. 3. Erase sector with the *best* erase command and move forward in a linear fashion. addr += cmd->size; len -= cmd->size; If the new address exceeds the end of this region, move to the next. 4. While (len) goto step2. That's all. Linearity is an advantage. We find the starting region and then we traverse each region in order without other queries. > >> - order erase types by size, with the biggest erase type at BIT(0). With >> this, we can iterate from the biggest supported erase type to the smallest, >> and when find one that meets all the required conditions, break the loop. >> This saves time in determining the best erase cmd. >> >> - minimize the amount of erase() calls by using the best sequence of erase >> type commands depending on alignment. > > Nice, this was long overdue > >> - replace spi_nor_find_uniform_erase() with spi_nor_select_uniform_erase(). >> Even for the SPI NOR memories with non-uniform erase types, we can determine >> at init if there are erase types that can erase the entire memory. Fill at >> init the uniform_erase_type bitmask, to encode the erase type commands that >> can erase the entire memory. >> >> - clarify support for overlaid regions. Considering one of the erase maps >> of the S25FS512S memory: >> Bottom: 8x 4KB sectors at bottom (only 4KB erase supported), >> 1x overlaid 224KB sector at bottom (only 256KB erase supported), >> 255x 256KB sectors (only 256KB erase supported) >> S25FS512S states that 'if a sector erase command is applied to a 256KB range >> that is overlaid by 4KB secors, the overlaid 4kB sectors are not affected by >> the erase'. When at init, the overlaid region size should be set to >> region->size = erase_size - count; in order to not miss chunks of data >> when traversing the regions. >> >> - backward compatibility test done on MX25L25673G. >> >> The 'erase with the best command, move forward and repeat' approach was >> suggested by Cristian Birsan in a brainstorm session, so: >> ] >> Suggested-by: Cristian Birsan <cristian.birsan@microchip.com> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 281 +++++++++++++++++++++++++++++++++++++++--- >> include/linux/mtd/spi-nor.h | 89 +++++++++++++ >> 2 files changed, 356 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 494b7a2..bb70664 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor, >> nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode); >> nor->program_opcode = spi_nor_convert_3to4_program(nor->program_opcode); >> nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode); >> + >> + if (!spi_nor_has_uniform_erase(nor)) { >> + struct spi_nor_erase_map *map = &nor->erase_map; >> + struct spi_nor_erase_command *cmd; >> + int i; >> + >> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >> + cmd = &map->commands[i]; >> + cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode); >> + } >> + } >> } >> >> /* Enable/disable 4-byte addressing mode. */ >> @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) >> return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width); >> } >> >> +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */ >> +static inline u64 >> +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd, >> + u64 dividend, u32 *remainder) >> +{ >> + *remainder = (u32)dividend & cmd->size_mask; >> + return dividend >> cmd->size_shift; >> +} >> + >> +static const struct spi_nor_erase_command * >> +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map, >> + const struct spi_nor_erase_region *region, u64 addr, >> + u32 len) >> +{ >> + const struct spi_nor_erase_command *cmd; >> + u32 rem; >> + int i; >> + u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK; >> + >> + /* >> + * Commands are ordered by size, with the biggest erase type at >> + * index 0. >> + */ >> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >> + /* Does the erase region support the tested erase command? */ >> + if (!(cmd_mask & BIT(i))) >> + continue; >> + >> + cmd = &map->commands[i]; >> + >> + /* Don't erase more than what the user has asked for. */ >> + if (cmd->size > len) >> + continue; > > Are you sure checking for the full erase block length first and then > checking if you can sub-erase the block is OK ? will respond in the next comment. > >> + if (!(region->offset & SNOR_OVERLAID_REGION)) { >> + /* 'addr' must be aligned to the erase size. */ >> + spi_nor_div_by_erase_size(cmd, addr, &rem); oh, I missed the if here, this should have been confusing. if (rem) continue; else return cmd; The else case can be merged with the one from below. Returning to your previous question. I iterate from the biggest erase command to the smallest, because bigger is preferred, it will minimize the amount of erase() calls. The biggest erase command that doesn't erase more that what the user has asked for, will do. If the region is not-overlaid the address must also be aligned with the erase size. >> + continue; >> + } else { >> + /* >> + * 'cmd' will erase the remaining of the overlaid >> + * region. >> + */ >> + return cmd; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +static const struct spi_nor_erase_region * >> +spi_nor_region_next(const struct spi_nor_erase_region *region) >> +{ >> + if (spi_nor_region_is_last(region)) >> + return NULL; >> + region++; >> + return region; >> +} >> + >> +static const struct spi_nor_erase_region * >> +spi_nor_find_erase_region(const struct spi_nor_erase_map *map, u64 addr, >> + u32 len) >> +{ >> + const struct spi_nor_erase_region *region = map->regions; >> + u64 region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK; >> + u64 region_end = region_start + region->size; >> + >> + if (!len) >> + return ERR_PTR(-EINVAL); >> + >> + while (addr < region_start || addr > region_end) { >> + region = spi_nor_region_next(region); >> + if (!region) >> + return ERR_PTR(-EINVAL); >> + >> + region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK; >> + region_end = region_start + region->size; >> + } >> + >> + return region; >> +} >> + >> +static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len) >> +{ >> + const struct spi_nor_erase_map *map = &nor->erase_map; >> + const struct spi_nor_erase_command *cmd; >> + const struct spi_nor_erase_region *region; >> + u64 region_end; >> + int ret; >> + >> + region = spi_nor_find_erase_region(map, addr, len); >> + if (IS_ERR(region)) >> + return PTR_ERR(region); >> + >> + region_end = spi_nor_region_end(region); >> + >> + while (len) { >> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >> + if (!cmd) >> + return -EINVAL; > > What would happen if you realize mid-way that you cannot erase some > sector , do you end up with partial erase ? Is this possible? In non-overlaid regions, the address is aligned with at least one of the erase commands, else -EINVAL. For overlaid regions alignment doesn't matter. But yes, if this is possible, in this case, this proposal will do a partial erase. Best, ta
On 05/21/2018 06:42 PM, Tudor Ambarus wrote: > Hi, Marek, [...] >>> This is a transitional patch: non-uniform erase maps will be used later >>> when initialized based on the SFDP data. >> >> What about non-SFDP non-linear flashes ? > > Non-SFDP non-uniform flashes support is not addressed with this > proposal, I should have told this in the commit message, thanks. But we > are backward compatible, if non-SFDP, the flashes are considered > uniform. OK. btw wall-of-text description of patch isn't my fav thing. >>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> >>> >>> [tudor.ambarus@microchip.com: >>> - add improvements on how the erase map is handled. The map is an array >>> describing the boundaries of the erase regions. LSB bits of the region's >>> offset are used to describe the supported erase types, to indicate if >>> that specific region is the last region in the map and to mark if the >>> region is overlaid or not. When one sends an addr and len to erase a >>> chunk of memory, we identify in which region the address fits, we start >>> erasing with the best fitted erase commands and when the region ends, >>> continue to erase from the next region. The erase is optimal: identify >>> the start offset (once), then erase with the best erase command, >>> move forward and repeat. >> >> Is that like an R-tree ? > > Not really. I find this RFC proposal faster and neat, but I'm open for > suggestions and guidance. > > One wants to erase a contiguous chunk of memory and sends us the > starting address and the total length. The algorithm of finding the best > sequence of erase commands can be summarized in four steps: > > 1. Find in which region the address fits. > This step is done only once, at the beginning. For the non-uniform > SFDP-defined flashes, usually there are two or three regions defined. > Nevertheless, in the worst case, the maximum number of regions that can > be defined is on eight bits, so 255. Linear search for just 255 elements > in the worst case looks good for me, especially that we do this search > once. > > 2. Find the *best* erase command that is defined in that region. > Each region can define maximum 4 erase commands. *Best* is defined as > the largest/biggest supported erase command with which the provided > address is aligned and which does not erase more that what the user has > asked for. In case of overlaid regions, alignment does not matter. The > largest command will erase the remaining of the overlaid region without > touching the region with which it overlaps (see S25FS512S). The > supported erase commands are ordered by size with the biggest queried > first. It is desirable to erase with large erase commands so that we > erase as much as we can in one shoot, minimizing the erase() calls. > > 3. Erase sector with the *best* erase command and move forward in a > linear fashion. > addr += cmd->size; > len -= cmd->size; > If the new address exceeds the end of this region, move to the next. > > 4. While (len) goto step2. > > That's all. Linearity is an advantage. We find the starting region and > then we traverse each region in order without other queries. > >> >>> - order erase types by size, with the biggest erase type at BIT(0). With >>> this, we can iterate from the biggest supported erase type to the >>> smallest, >>> and when find one that meets all the required conditions, break the >>> loop. >>> This saves time in determining the best erase cmd. >>> >>> - minimize the amount of erase() calls by using the best sequence of >>> erase >>> type commands depending on alignment. >> >> Nice, this was long overdue >> >>> - replace spi_nor_find_uniform_erase() with >>> spi_nor_select_uniform_erase(). >>> Even for the SPI NOR memories with non-uniform erase types, we can >>> determine >>> at init if there are erase types that can erase the entire memory. >>> Fill at >>> init the uniform_erase_type bitmask, to encode the erase type >>> commands that >>> can erase the entire memory. >>> >>> - clarify support for overlaid regions. Considering one of the erase >>> maps >>> of the S25FS512S memory: >>> Bottom: 8x 4KB sectors at bottom (only 4KB erase supported), >>> 1x overlaid 224KB sector at bottom (only 256KB erase >>> supported), >>> 255x 256KB sectors (only 256KB erase supported) >>> S25FS512S states that 'if a sector erase command is applied to a >>> 256KB range >>> that is overlaid by 4KB secors, the overlaid 4kB sectors are not >>> affected by >>> the erase'. When at init, the overlaid region size should be set to >>> region->size = erase_size - count; in order to not miss chunks of data >>> when traversing the regions. >>> >>> - backward compatibility test done on MX25L25673G. >>> >>> The 'erase with the best command, move forward and repeat' approach was >>> suggested by Cristian Birsan in a brainstorm session, so: >>> ] >>> Suggested-by: Cristian Birsan <cristian.birsan@microchip.com> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 281 >>> +++++++++++++++++++++++++++++++++++++++--- >>> include/linux/mtd/spi-nor.h | 89 +++++++++++++ >>> 2 files changed, 356 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>> b/drivers/mtd/spi-nor/spi-nor.c >>> index 494b7a2..bb70664 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct >>> spi_nor *nor, >>> nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode); >>> nor->program_opcode = >>> spi_nor_convert_3to4_program(nor->program_opcode); >>> nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode); >>> + >>> + if (!spi_nor_has_uniform_erase(nor)) { >>> + struct spi_nor_erase_map *map = &nor->erase_map; >>> + struct spi_nor_erase_command *cmd; >>> + int i; >>> + >>> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >>> + cmd = &map->commands[i]; >>> + cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode); >>> + } >>> + } >>> } >>> /* Enable/disable 4-byte addressing mode. */ >>> @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor >>> *nor, u32 addr) >>> return nor->write_reg(nor, nor->erase_opcode, buf, >>> nor->addr_width); >>> } >>> +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */ >>> +static inline u64 >>> +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd, >>> + u64 dividend, u32 *remainder) >>> +{ >>> + *remainder = (u32)dividend & cmd->size_mask; >>> + return dividend >> cmd->size_shift; >>> +} >>> + >>> +static const struct spi_nor_erase_command * >>> +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map, >>> + const struct spi_nor_erase_region *region, u64 addr, >>> + u32 len) >>> +{ >>> + const struct spi_nor_erase_command *cmd; >>> + u32 rem; >>> + int i; >>> + u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK; >>> + >>> + /* >>> + * Commands are ordered by size, with the biggest erase type at >>> + * index 0. >>> + */ >>> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >>> + /* Does the erase region support the tested erase command? */ >>> + if (!(cmd_mask & BIT(i))) >>> + continue; >>> + >>> + cmd = &map->commands[i]; >>> + >>> + /* Don't erase more than what the user has asked for. */ >>> + if (cmd->size > len) >>> + continue; >> >> Are you sure checking for the full erase block length first and then >> checking if you can sub-erase the block is OK ? > > will respond in the next comment. > >> >>> + if (!(region->offset & SNOR_OVERLAID_REGION)) { >>> + /* 'addr' must be aligned to the erase size. */ >>> + spi_nor_div_by_erase_size(cmd, addr, &rem); > > oh, I missed the if here, this should have been confusing. > if (rem) > continue; > else > return cmd; > The else case can be merged with the one from below. > > Returning to your previous question. I iterate from the biggest erase > command to the smallest, because bigger is preferred, it will minimize > the amount of erase() calls. The biggest erase command that doesn't > erase more that what the user has asked for, will do. If the region is > not-overlaid the address must also be aligned with the erase size. You can have a flash with 4k sectors which also supports 64k erase and try to erase ie. 128k at offset +4k. That means you need to first erase small chunks, then big chunk, then small chunks again. So I don't think you can start with large chunk to see if you can erase it, since on such a setup the erase will degrade to massive amount of 4k erase ops. [...] >>> + while (len) { >>> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >>> + if (!cmd) >>> + return -EINVAL; >> >> What would happen if you realize mid-way that you cannot erase some >> sector , do you end up with partial erase ? > > Is this possible? In non-overlaid regions, the address is aligned with > at least one of the erase commands, else -EINVAL. For overlaid regions > alignment doesn't matter. But yes, if this is possible, in this case, > this proposal will do a partial erase. Shouldn't we fail up front instead ?
Hi, Marek, On 05/21/2018 07:59 PM, Marek Vasut wrote: > On 05/21/2018 06:42 PM, Tudor Ambarus wrote: >> Hi, Marek, > > [...] > >>>> This is a transitional patch: non-uniform erase maps will be used later >>>> when initialized based on the SFDP data. >>> >>> What about non-SFDP non-linear flashes ? >> >> Non-SFDP non-uniform flashes support is not addressed with this >> proposal, I should have told this in the commit message, thanks. But we >> are backward compatible, if non-SFDP, the flashes are considered >> uniform. > > OK. btw wall-of-text description of patch isn't my fav thing. > >>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> >>>> >>>> [tudor.ambarus@microchip.com: >>>> - add improvements on how the erase map is handled. The map is an array >>>> describing the boundaries of the erase regions. LSB bits of the region's >>>> offset are used to describe the supported erase types, to indicate if >>>> that specific region is the last region in the map and to mark if the >>>> region is overlaid or not. When one sends an addr and len to erase a >>>> chunk of memory, we identify in which region the address fits, we start >>>> erasing with the best fitted erase commands and when the region ends, >>>> continue to erase from the next region. The erase is optimal: identify >>>> the start offset (once), then erase with the best erase command, >>>> move forward and repeat. >>> >>> Is that like an R-tree ? >> >> Not really. I find this RFC proposal faster and neat, but I'm open for >> suggestions and guidance. >> >> One wants to erase a contiguous chunk of memory and sends us the >> starting address and the total length. The algorithm of finding the best >> sequence of erase commands can be summarized in four steps: >> >> 1. Find in which region the address fits. >> This step is done only once, at the beginning. For the non-uniform >> SFDP-defined flashes, usually there are two or three regions defined. >> Nevertheless, in the worst case, the maximum number of regions that can >> be defined is on eight bits, so 255. Linear search for just 255 elements >> in the worst case looks good for me, especially that we do this search >> once. >> >> 2. Find the *best* erase command that is defined in that region. >> Each region can define maximum 4 erase commands. *Best* is defined as >> the largest/biggest supported erase command with which the provided >> address is aligned and which does not erase more that what the user has >> asked for. In case of overlaid regions, alignment does not matter. The >> largest command will erase the remaining of the overlaid region without >> touching the region with which it overlaps (see S25FS512S). The >> supported erase commands are ordered by size with the biggest queried >> first. It is desirable to erase with large erase commands so that we >> erase as much as we can in one shoot, minimizing the erase() calls. >> >> 3. Erase sector with the *best* erase command and move forward in a >> linear fashion. >> addr += cmd->size; >> len -= cmd->size; >> If the new address exceeds the end of this region, move to the next. >> >> 4. While (len) goto step2. >> >> That's all. Linearity is an advantage. We find the starting region and >> then we traverse each region in order without other queries. >> >>> >>>> - order erase types by size, with the biggest erase type at BIT(0). With >>>> this, we can iterate from the biggest supported erase type to the >>>> smallest, >>>> and when find one that meets all the required conditions, break the >>>> loop. >>>> This saves time in determining the best erase cmd. >>>> >>>> - minimize the amount of erase() calls by using the best sequence of >>>> erase >>>> type commands depending on alignment. >>> >>> Nice, this was long overdue >>> >>>> - replace spi_nor_find_uniform_erase() with >>>> spi_nor_select_uniform_erase(). >>>> Even for the SPI NOR memories with non-uniform erase types, we can >>>> determine >>>> at init if there are erase types that can erase the entire memory. >>>> Fill at >>>> init the uniform_erase_type bitmask, to encode the erase type >>>> commands that >>>> can erase the entire memory. >>>> >>>> - clarify support for overlaid regions. Considering one of the erase >>>> maps >>>> of the S25FS512S memory: >>>> Bottom: 8x 4KB sectors at bottom (only 4KB erase supported), >>>> 1x overlaid 224KB sector at bottom (only 256KB erase >>>> supported), >>>> 255x 256KB sectors (only 256KB erase supported) >>>> S25FS512S states that 'if a sector erase command is applied to a >>>> 256KB range >>>> that is overlaid by 4KB secors, the overlaid 4kB sectors are not >>>> affected by >>>> the erase'. When at init, the overlaid region size should be set to >>>> region->size = erase_size - count; in order to not miss chunks of data >>>> when traversing the regions. >>>> >>>> - backward compatibility test done on MX25L25673G. >>>> >>>> The 'erase with the best command, move forward and repeat' approach was >>>> suggested by Cristian Birsan in a brainstorm session, so: >>>> ] >>>> Suggested-by: Cristian Birsan <cristian.birsan@microchip.com> >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>> --- >>>> drivers/mtd/spi-nor/spi-nor.c | 281 >>>> +++++++++++++++++++++++++++++++++++++++--- >>>> include/linux/mtd/spi-nor.h | 89 +++++++++++++ >>>> 2 files changed, 356 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>>> b/drivers/mtd/spi-nor/spi-nor.c >>>> index 494b7a2..bb70664 100644 >>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>> @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct >>>> spi_nor *nor, >>>> nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode); >>>> nor->program_opcode = >>>> spi_nor_convert_3to4_program(nor->program_opcode); >>>> nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode); >>>> + >>>> + if (!spi_nor_has_uniform_erase(nor)) { >>>> + struct spi_nor_erase_map *map = &nor->erase_map; >>>> + struct spi_nor_erase_command *cmd; >>>> + int i; >>>> + >>>> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >>>> + cmd = &map->commands[i]; >>>> + cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode); >>>> + } >>>> + } >>>> } >>>> /* Enable/disable 4-byte addressing mode. */ >>>> @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor >>>> *nor, u32 addr) >>>> return nor->write_reg(nor, nor->erase_opcode, buf, >>>> nor->addr_width); >>>> } >>>> +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */ >>>> +static inline u64 >>>> +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd, >>>> + u64 dividend, u32 *remainder) >>>> +{ >>>> + *remainder = (u32)dividend & cmd->size_mask; >>>> + return dividend >> cmd->size_shift; >>>> +} >>>> + >>>> +static const struct spi_nor_erase_command * >>>> +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map, >>>> + const struct spi_nor_erase_region *region, u64 addr, >>>> + u32 len) >>>> +{ >>>> + const struct spi_nor_erase_command *cmd; >>>> + u32 rem; >>>> + int i; >>>> + u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK; >>>> + >>>> + /* >>>> + * Commands are ordered by size, with the biggest erase type at >>>> + * index 0. >>>> + */ >>>> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >>>> + /* Does the erase region support the tested erase command? */ >>>> + if (!(cmd_mask & BIT(i))) >>>> + continue; >>>> + >>>> + cmd = &map->commands[i]; >>>> + >>>> + /* Don't erase more than what the user has asked for. */ >>>> + if (cmd->size > len) >>>> + continue; >>> >>> Are you sure checking for the full erase block length first and then >>> checking if you can sub-erase the block is OK ? >> >> will respond in the next comment. >> >>> >>>> + if (!(region->offset & SNOR_OVERLAID_REGION)) { >>>> + /* 'addr' must be aligned to the erase size. */ >>>> + spi_nor_div_by_erase_size(cmd, addr, &rem); >> >> oh, I missed the if here, this should have been confusing. >> if (rem) >> continue; >> else >> return cmd; >> The else case can be merged with the one from below. >> >> Returning to your previous question. I iterate from the biggest erase >> command to the smallest, because bigger is preferred, it will minimize >> the amount of erase() calls. The biggest erase command that doesn't >> erase more that what the user has asked for, will do. If the region is >> not-overlaid the address must also be aligned with the erase size. > > You can have a flash with 4k sectors which also supports 64k erase and > try to erase ie. 128k at offset +4k. That means you need to first erase > small chunks, then big chunk, then small chunks again. So I don't think > you can start with large chunk to see if you can erase it, since on such > a setup the erase will degrade to massive amount of 4k erase ops. > I'm looking for the biggest supported erase command with which the provided address is aligned and which does not erase more that what the user has asked for. In your example, 4k erase type will be used until reaching the 64k offset, then a 64k erase type, then a 4k type. > [...] > >>>> + while (len) { >>>> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >>>> + if (!cmd) >>>> + return -EINVAL; >>> >>> What would happen if you realize mid-way that you cannot erase some >>> sector , do you end up with partial erase ? >> >> Is this possible? In non-overlaid regions, the address is aligned with >> at least one of the erase commands, else -EINVAL. For overlaid regions >> alignment doesn't matter. But yes, if this is possible, in this case, >> this proposal will do a partial erase. > > Shouldn't we fail up front instead ? It will be great if we can do this without having performance penalties. Can we loose the conditions for the last erase command? If one wants to erase 80k chunk starting from offset 0 and only 32k and 64k erase type are supported, can we erase 96k? Thanks, ta
On 05/22/2018 07:01 AM, Tudor Ambarus wrote: > Hi, Marek, Hi! > On 05/21/2018 07:59 PM, Marek Vasut wrote: >> On 05/21/2018 06:42 PM, Tudor Ambarus wrote: >>> Hi, Marek, >> >> [...] >> >>>>> This is a transitional patch: non-uniform erase maps will be used >>>>> later >>>>> when initialized based on the SFDP data. >>>> >>>> What about non-SFDP non-linear flashes ? >>> >>> Non-SFDP non-uniform flashes support is not addressed with this >>> proposal, I should have told this in the commit message, thanks. But we >>> are backward compatible, if non-SFDP, the flashes are considered >>> uniform. >> >> OK. btw wall-of-text description of patch isn't my fav thing. >> >>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com> >>>>> >>>>> [tudor.ambarus@microchip.com: >>>>> - add improvements on how the erase map is handled. The map is an >>>>> array >>>>> describing the boundaries of the erase regions. LSB bits of the >>>>> region's >>>>> offset are used to describe the supported erase types, to indicate if >>>>> that specific region is the last region in the map and to mark if the >>>>> region is overlaid or not. When one sends an addr and len to erase a >>>>> chunk of memory, we identify in which region the address fits, we >>>>> start >>>>> erasing with the best fitted erase commands and when the region ends, >>>>> continue to erase from the next region. The erase is optimal: identify >>>>> the start offset (once), then erase with the best erase command, >>>>> move forward and repeat. >>>> >>>> Is that like an R-tree ? >>> >>> Not really. I find this RFC proposal faster and neat, but I'm open for >>> suggestions and guidance. >>> >>> One wants to erase a contiguous chunk of memory and sends us the >>> starting address and the total length. The algorithm of finding the best >>> sequence of erase commands can be summarized in four steps: >>> >>> 1. Find in which region the address fits. >>> This step is done only once, at the beginning. For the non-uniform >>> SFDP-defined flashes, usually there are two or three regions defined. >>> Nevertheless, in the worst case, the maximum number of regions that can >>> be defined is on eight bits, so 255. Linear search for just 255 elements >>> in the worst case looks good for me, especially that we do this search >>> once. >>> >>> 2. Find the *best* erase command that is defined in that region. >>> Each region can define maximum 4 erase commands. *Best* is defined as >>> the largest/biggest supported erase command with which the provided >>> address is aligned and which does not erase more that what the user has >>> asked for. In case of overlaid regions, alignment does not matter. The >>> largest command will erase the remaining of the overlaid region without >>> touching the region with which it overlaps (see S25FS512S). The >>> supported erase commands are ordered by size with the biggest queried >>> first. It is desirable to erase with large erase commands so that we >>> erase as much as we can in one shoot, minimizing the erase() calls. >>> >>> 3. Erase sector with the *best* erase command and move forward in a >>> linear fashion. >>> addr += cmd->size; >>> len -= cmd->size; >>> If the new address exceeds the end of this region, move to the next. >>> >>> 4. While (len) goto step2. >>> >>> That's all. Linearity is an advantage. We find the starting region and >>> then we traverse each region in order without other queries. >>> >>>> >>>>> - order erase types by size, with the biggest erase type at BIT(0). >>>>> With >>>>> this, we can iterate from the biggest supported erase type to the >>>>> smallest, >>>>> and when find one that meets all the required conditions, break the >>>>> loop. >>>>> This saves time in determining the best erase cmd. >>>>> >>>>> - minimize the amount of erase() calls by using the best sequence of >>>>> erase >>>>> type commands depending on alignment. >>>> >>>> Nice, this was long overdue >>>> >>>>> - replace spi_nor_find_uniform_erase() with >>>>> spi_nor_select_uniform_erase(). >>>>> Even for the SPI NOR memories with non-uniform erase types, we can >>>>> determine >>>>> at init if there are erase types that can erase the entire memory. >>>>> Fill at >>>>> init the uniform_erase_type bitmask, to encode the erase type >>>>> commands that >>>>> can erase the entire memory. >>>>> >>>>> - clarify support for overlaid regions. Considering one of the erase >>>>> maps >>>>> of the S25FS512S memory: >>>>> Bottom: 8x 4KB sectors at bottom (only 4KB erase supported), >>>>> 1x overlaid 224KB sector at bottom (only 256KB erase >>>>> supported), >>>>> 255x 256KB sectors (only 256KB erase supported) >>>>> S25FS512S states that 'if a sector erase command is applied to a >>>>> 256KB range >>>>> that is overlaid by 4KB secors, the overlaid 4kB sectors are not >>>>> affected by >>>>> the erase'. When at init, the overlaid region size should be set to >>>>> region->size = erase_size - count; in order to not miss chunks of data >>>>> when traversing the regions. >>>>> >>>>> - backward compatibility test done on MX25L25673G. >>>>> >>>>> The 'erase with the best command, move forward and repeat' approach >>>>> was >>>>> suggested by Cristian Birsan in a brainstorm session, so: >>>>> ] >>>>> Suggested-by: Cristian Birsan <cristian.birsan@microchip.com> >>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>>> --- >>>>> drivers/mtd/spi-nor/spi-nor.c | 281 >>>>> +++++++++++++++++++++++++++++++++++++++--- >>>>> include/linux/mtd/spi-nor.h | 89 +++++++++++++ >>>>> 2 files changed, 356 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>>>> b/drivers/mtd/spi-nor/spi-nor.c >>>>> index 494b7a2..bb70664 100644 >>>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>>> @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct >>>>> spi_nor *nor, >>>>> nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode); >>>>> nor->program_opcode = >>>>> spi_nor_convert_3to4_program(nor->program_opcode); >>>>> nor->erase_opcode = >>>>> spi_nor_convert_3to4_erase(nor->erase_opcode); >>>>> + >>>>> + if (!spi_nor_has_uniform_erase(nor)) { >>>>> + struct spi_nor_erase_map *map = &nor->erase_map; >>>>> + struct spi_nor_erase_command *cmd; >>>>> + int i; >>>>> + >>>>> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >>>>> + cmd = &map->commands[i]; >>>>> + cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode); >>>>> + } >>>>> + } >>>>> } >>>>> /* Enable/disable 4-byte addressing mode. */ >>>>> @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor >>>>> *nor, u32 addr) >>>>> return nor->write_reg(nor, nor->erase_opcode, buf, >>>>> nor->addr_width); >>>>> } >>>>> +/* JEDEC JESD216B Standard imposes erase sizes to be power of >>>>> 2. */ >>>>> +static inline u64 >>>>> +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd, >>>>> + u64 dividend, u32 *remainder) >>>>> +{ >>>>> + *remainder = (u32)dividend & cmd->size_mask; >>>>> + return dividend >> cmd->size_shift; >>>>> +} >>>>> + >>>>> +static const struct spi_nor_erase_command * >>>>> +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map, >>>>> + const struct spi_nor_erase_region *region, u64 addr, >>>>> + u32 len) >>>>> +{ >>>>> + const struct spi_nor_erase_command *cmd; >>>>> + u32 rem; >>>>> + int i; >>>>> + u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK; >>>>> + >>>>> + /* >>>>> + * Commands are ordered by size, with the biggest erase type at >>>>> + * index 0. >>>>> + */ >>>>> + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { >>>>> + /* Does the erase region support the tested erase command? */ >>>>> + if (!(cmd_mask & BIT(i))) >>>>> + continue; >>>>> + >>>>> + cmd = &map->commands[i]; >>>>> + >>>>> + /* Don't erase more than what the user has asked for. */ >>>>> + if (cmd->size > len) >>>>> + continue; >>>> >>>> Are you sure checking for the full erase block length first and then >>>> checking if you can sub-erase the block is OK ? >>> >>> will respond in the next comment. >>> >>>> >>>>> + if (!(region->offset & SNOR_OVERLAID_REGION)) { >>>>> + /* 'addr' must be aligned to the erase size. */ >>>>> + spi_nor_div_by_erase_size(cmd, addr, &rem); >>> >>> oh, I missed the if here, this should have been confusing. >>> if (rem) >>> continue; >>> else >>> return cmd; >>> The else case can be merged with the one from below. >>> >>> Returning to your previous question. I iterate from the biggest erase >>> command to the smallest, because bigger is preferred, it will minimize >>> the amount of erase() calls. The biggest erase command that doesn't >>> erase more that what the user has asked for, will do. If the region is >>> not-overlaid the address must also be aligned with the erase size. >> >> You can have a flash with 4k sectors which also supports 64k erase and >> try to erase ie. 128k at offset +4k. That means you need to first erase >> small chunks, then big chunk, then small chunks again. So I don't think >> you can start with large chunk to see if you can erase it, since on such >> a setup the erase will degrade to massive amount of 4k erase ops. >> > > I'm looking for the biggest supported erase command with which the > provided address is aligned and which does not erase more that what the > user has asked for. In your example, 4k erase type will be used until > reaching the 64k offset, then a 64k erase type, then a 4k type. That's good! >> [...] >> >>>>> + while (len) { >>>>> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >>>>> + if (!cmd) >>>>> + return -EINVAL; >>>> >>>> What would happen if you realize mid-way that you cannot erase some >>>> sector , do you end up with partial erase ? >>> >>> Is this possible? In non-overlaid regions, the address is aligned with >>> at least one of the erase commands, else -EINVAL. For overlaid regions >>> alignment doesn't matter. But yes, if this is possible, in this case, >>> this proposal will do a partial erase. >> >> Shouldn't we fail up front instead ? > > It will be great if we can do this without having performance penalties. > Can we loose the conditions for the last erase command? If one wants to > erase 80k chunk starting from offset 0 and only 32k and 64k erase type > are supported, can we erase 96k? No. But can you maybe build a list of erase commands to be executed once you validate that the erase can be performed for example ?
Hi, Marek, On 05/23/2018 12:56 PM, Marek Vasut wrote: [...] >>> [...] >>> >>>>>> + while (len) { >>>>>> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >>>>>> + if (!cmd) >>>>>> + return -EINVAL; >>>>> What would happen if you realize mid-way that you cannot erase some >>>>> sector , do you end up with partial erase ? >>>> Is this possible? In non-overlaid regions, the address is aligned with >>>> at least one of the erase commands, else -EINVAL. For overlaid regions >>>> alignment doesn't matter. But yes, if this is possible, in this case, >>>> this proposal will do a partial erase. >>> Shouldn't we fail up front instead ? >> It will be great if we can do this without having performance penalties. >> Can we loose the conditions for the last erase command? If one wants to >> erase 80k chunk starting from offset 0 and only 32k and 64k erase type >> are supported, can we erase 96k? > No. But can you maybe build a list of erase commands to be executed once > you validate that the erase can be performed for example ? My second choice was an array witch saves u8 opcode and u32 erasesize. There are flashes of 256MB, in the worst case scenario with 4k erase type, we will end up with 64K entries. How about enforcing the length to be multiple of mtd->erasesize, like we do in uniform_erase? With this, the problem disappears. Thanks, ta
On 05/23/2018 02:52 PM, Tudor Ambarus wrote: > Hi, Marek, Hi, > On 05/23/2018 12:56 PM, Marek Vasut wrote: > [...] >>>> [...] >>>> >>>>>>> + while (len) { >>>>>>> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >>>>>>> + if (!cmd) >>>>>>> + return -EINVAL; >>>>>> What would happen if you realize mid-way that you cannot erase some >>>>>> sector , do you end up with partial erase ? >>>>> Is this possible? In non-overlaid regions, the address is aligned with >>>>> at least one of the erase commands, else -EINVAL. For overlaid regions >>>>> alignment doesn't matter. But yes, if this is possible, in this case, >>>>> this proposal will do a partial erase. >>>> Shouldn't we fail up front instead ? >>> It will be great if we can do this without having performance penalties. >>> Can we loose the conditions for the last erase command? If one wants to >>> erase 80k chunk starting from offset 0 and only 32k and 64k erase type >>> are supported, can we erase 96k? >> No. But can you maybe build a list of erase commands to be executed once >> you validate that the erase can be performed for example ? > > My second choice was an array witch saves u8 opcode and u32 erasesize. > There are flashes of 256MB, in the worst case scenario with 4k erase > type, we will end up with 64K entries. Some RLE encoding might help here ? > How about enforcing the length to be multiple of mtd->erasesize, like we > do in uniform_erase? With this, the problem disappears. What is the erase size for the 4k-sector 256MiB flash ?
Hi, Marek, On 05/23/2018 03:54 PM, Marek Vasut wrote: > On 05/23/2018 02:52 PM, Tudor Ambarus wrote: >> Hi, Marek, > > Hi, > >> On 05/23/2018 12:56 PM, Marek Vasut wrote: >> [...] >>>>> [...] >>>>> >>>>>>>> + while (len) { >>>>>>>> + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); >>>>>>>> + if (!cmd) >>>>>>>> + return -EINVAL; >>>>>>> What would happen if you realize mid-way that you cannot erase some >>>>>>> sector , do you end up with partial erase ? >>>>>> Is this possible? In non-overlaid regions, the address is aligned with >>>>>> at least one of the erase commands, else -EINVAL. For overlaid regions >>>>>> alignment doesn't matter. But yes, if this is possible, in this case, >>>>>> this proposal will do a partial erase. >>>>> Shouldn't we fail up front instead ? >>>> It will be great if we can do this without having performance penalties. >>>> Can we loose the conditions for the last erase command? If one wants to >>>> erase 80k chunk starting from offset 0 and only 32k and 64k erase type >>>> are supported, can we erase 96k? >>> No. But can you maybe build a list of erase commands to be executed once >>> you validate that the erase can be performed for example ? >> >> My second choice was an array witch saves u8 opcode and u32 erasesize. >> There are flashes of 256MB, in the worst case scenario with 4k erase >> type, we will end up with 64K entries. > > Some RLE encoding might help here ? Nice. > >> How about enforcing the length to be multiple of mtd->erasesize, like we >> do in uniform_erase? With this, the problem disappears. > > What is the erase size for the 4k-sector 256MiB flash ? S70FS01GS[1] is a 128 MByte flash with non-uniform erase support. It supports 4k and 256k erase types. I would have to enforce the address and the length to be multiple of 256k in order to vanish the issue. But the whole point of non-uniform erase will vanish too, I guess. I don't have any other good :) idea, so I'll implement your suggestion with the list of erase commands and RLE encoding. Thanks, ta [1] http://www.cypress.com/file/215911/download
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 494b7a2..bb70664 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor, nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode); nor->program_opcode = spi_nor_convert_3to4_program(nor->program_opcode); nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode); + + if (!spi_nor_has_uniform_erase(nor)) { + struct spi_nor_erase_map *map = &nor->erase_map; + struct spi_nor_erase_command *cmd; + int i; + + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { + cmd = &map->commands[i]; + cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode); + } + } } /* Enable/disable 4-byte addressing mode. */ @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width); } +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */ +static inline u64 +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd, + u64 dividend, u32 *remainder) +{ + *remainder = (u32)dividend & cmd->size_mask; + return dividend >> cmd->size_shift; +} + +static const struct spi_nor_erase_command * +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map, + const struct spi_nor_erase_region *region, u64 addr, + u32 len) +{ + const struct spi_nor_erase_command *cmd; + u32 rem; + int i; + u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK; + + /* + * Commands are ordered by size, with the biggest erase type at + * index 0. + */ + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { + /* Does the erase region support the tested erase command? */ + if (!(cmd_mask & BIT(i))) + continue; + + cmd = &map->commands[i]; + + /* Don't erase more than what the user has asked for. */ + if (cmd->size > len) + continue; + + if (!(region->offset & SNOR_OVERLAID_REGION)) { + /* 'addr' must be aligned to the erase size. */ + spi_nor_div_by_erase_size(cmd, addr, &rem); + continue; + } else { + /* + * 'cmd' will erase the remaining of the overlaid + * region. + */ + return cmd; + } + } + + return NULL; +} + +static const struct spi_nor_erase_region * +spi_nor_region_next(const struct spi_nor_erase_region *region) +{ + if (spi_nor_region_is_last(region)) + return NULL; + region++; + return region; +} + +static const struct spi_nor_erase_region * +spi_nor_find_erase_region(const struct spi_nor_erase_map *map, u64 addr, + u32 len) +{ + const struct spi_nor_erase_region *region = map->regions; + u64 region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK; + u64 region_end = region_start + region->size; + + if (!len) + return ERR_PTR(-EINVAL); + + while (addr < region_start || addr > region_end) { + region = spi_nor_region_next(region); + if (!region) + return ERR_PTR(-EINVAL); + + region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK; + region_end = region_start + region->size; + } + + return region; +} + +static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len) +{ + const struct spi_nor_erase_map *map = &nor->erase_map; + const struct spi_nor_erase_command *cmd; + const struct spi_nor_erase_region *region; + u64 region_end; + int ret; + + region = spi_nor_find_erase_region(map, addr, len); + if (IS_ERR(region)) + return PTR_ERR(region); + + region_end = spi_nor_region_end(region); + + while (len) { + cmd = spi_nor_find_best_erase_cmd(map, region, addr, len); + if (!cmd) + return -EINVAL; + + nor->erase_opcode = cmd->opcode; + + ret = spi_nor_erase_sector(nor, addr); + if (ret) + return ret; + + addr += cmd->size; + len -= cmd->size; + + if (len && addr >= region_end) { + region = spi_nor_region_next(region); + if (!region) + return -EINVAL; + region_end = spi_nor_region_end(region); + } + + ret = spi_nor_wait_till_ready(nor); + if (ret) + return ret; + } + + return 0; +} + /* * Erase an address range on the nor chip. The address range may extend * one or more erase sectors. Return an error is there is a problem erasing. @@ -511,9 +647,11 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr, (long long)instr->len); - div_u64_rem(instr->len, mtd->erasesize, &rem); - if (rem) - return -EINVAL; + if (likely(spi_nor_has_uniform_erase(nor))) { + div_u64_rem(instr->len, mtd->erasesize, &rem); + if (rem) + return -EINVAL; + } addr = instr->addr; len = instr->len; @@ -552,7 +690,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) */ /* "sector"-at-a-time erase */ - } else { + } else if (likely(spi_nor_has_uniform_erase(nor))) { while (len) { write_enable(nor); @@ -567,6 +705,12 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) if (ret) goto erase_err; } + + /* erase multiple sectors */ + } else { + ret = spi_nor_erase_multi_sectors(nor, addr, len); + if (ret) + goto erase_err; } write_disable(nor); @@ -2329,6 +2473,27 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, return 0; } +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */ +static inline void +spi_nor_set_erase_command(struct spi_nor_erase_command *cmd, + u32 size, u8 opcode) +{ + cmd->size = size; + cmd->opcode = opcode; + cmd->size_shift = ffs(cmd->size) - 1; + cmd->size_mask = (1 << cmd->size_shift) - 1; +} + +static inline void +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map, + u8 cmd_mask, u64 flash_size) +{ + map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(cmd_mask, 1, 0, 0); + map->uniform_region.size = flash_size; + map->regions = &map->uniform_region; + map->uniform_erase_type = cmd_mask; +} + /** * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters. * @nor: pointer to a 'struct spi_nor' @@ -2443,6 +2608,9 @@ static int spi_nor_init_params(struct spi_nor *nor, const struct flash_info *info, struct spi_nor_flash_parameter *params) { + struct spi_nor_erase_map *map = &nor->erase_map; + u8 erase_mask = 0; + /* Set legacy flash parameters as default. */ memset(params, 0, sizeof(*params)); @@ -2482,6 +2650,24 @@ static int spi_nor_init_params(struct spi_nor *nor, spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], SPINOR_OP_PP, SNOR_PROTO_1_1_1); + /* + * Sector Erase settings. Order erase commands by size, with the biggest + * erase size at BIT(0) + */ + erase_mask |= BIT(0); + spi_nor_set_erase_command(&map->commands[0], + info->sector_size, SPINOR_OP_SE); + if (info->flags & SECT_4K_PMC) { + erase_mask |= BIT(1); + spi_nor_set_erase_command(&map->commands[1], + 4096u, SPINOR_OP_BE_4K_PMC); + } else if (info->flags & SECT_4K) { + erase_mask |= BIT(1); + spi_nor_set_erase_command(&map->commands[1], + 4096u, SPINOR_OP_BE_4K); + } + spi_nor_init_uniform_erase_map(map, erase_mask, params->size); + /* Select the procedure to set the Quad Enable bit. */ if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD | SNOR_HWCAPS_PP_QUAD)) { @@ -2631,29 +2817,96 @@ static int spi_nor_select_pp(struct spi_nor *nor, return 0; } +static const struct spi_nor_erase_command * +spi_nor_select_uniform_erase(struct spi_nor_erase_map *map, + const struct flash_info *info, u32 wanted_size) +{ + const struct spi_nor_erase_command *tested_cmd, *cmd = NULL; + int i; + u8 erase_type = map->uniform_erase_type; + + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) { + if (!(erase_type & BIT(i))) + continue; + + tested_cmd = &map->commands[i]; + + /* + * If the current erase size is the one, stop here: + * we have found the right uniform Sector Erase command. + */ + if (tested_cmd->size == wanted_size) { + cmd = tested_cmd; + break; + } + + /* + * Otherwise, the current erase size is still a valid canditate + * we select the biggest valid candidate unless we find the + * Sector Erase command for an erase size of + * 'info->sector_size'. + */ + if (!cmd || tested_cmd->size == info->sector_size) + cmd = tested_cmd; + } + + if (!cmd || !cmd->size) + return ERR_PTR(-EINVAL); + + /* Disable all other Sector Erase commands. */ + map->uniform_erase_type &= ~SNOR_CMD_ERASE_MASK; + map->uniform_erase_type |= BIT(cmd - map->commands); + return cmd; +} + static int spi_nor_select_erase(struct spi_nor *nor, const struct flash_info *info) { + struct spi_nor_erase_map *map = &nor->erase_map; + const struct spi_nor_erase_command *cmd = NULL; struct mtd_info *mtd = &nor->mtd; + u32 wanted_size = info->sector_size; + int i; /* Do nothing if already configured from SFDP. */ if (mtd->erasesize) return 0; + /* + * The previous implementation handling Sector Erase commands assumed + * that the SPI flash memory has an uniform layout then used only one + * of the supported erase sizes for all Sector Erase commands. + * So to be backward compatible, the new implementation also tries to + * manage the SPI flash memory as uniform with a single erase sector + * size, when possible. + */ #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS /* prefer "small sector" erase if possible */ - if (info->flags & SECT_4K) { - nor->erase_opcode = SPINOR_OP_BE_4K; - mtd->erasesize = 4096; - } else if (info->flags & SECT_4K_PMC) { - nor->erase_opcode = SPINOR_OP_BE_4K_PMC; - mtd->erasesize = 4096; - } else + wanted_size = 4096u; #endif - { - nor->erase_opcode = SPINOR_OP_SE; - mtd->erasesize = info->sector_size; + + if (map->uniform_erase_type) { + cmd = spi_nor_select_uniform_erase(map, info, wanted_size); + if (IS_ERR(cmd)) + return PTR_ERR(cmd); + nor->erase_opcode = cmd->opcode; + mtd->erasesize = cmd->size; + return 0; } + + /* + * For non-uniform SPI flash memory, set mtd->erasesize to the + * maximum erase sector size. No need to set nor->erase_opcode. + */ + for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) + if (!map->commands[i].size) { + cmd = &map->commands[i]; + break; + } + if (!cmd) + return -EINVAL; + + mtd->erasesize = cmd->size; return 0; } diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e60da0d..fcf14b7 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -238,6 +238,76 @@ enum spi_nor_option_flags { }; /** + * struct spi_nor_erase_command - Structure to describe a SPI NOR erase command + * @size: the size of the sector/block erased by the command. + * JEDEC JESD216B imposes erase sizes to be a power of 2. + * @size_shift: @size is a power of 2, the shift is stored in + * @size_shift. + * @size_mask: the size mask based on @size_shift. + * @opcode: the SPI command op code to erase the sector/block. + */ +struct spi_nor_erase_command { + u32 size; + u32 size_shift; + u32 size_mask; + u8 opcode; +}; + +/** + * struct spi_nor_erase_region - Structure to describe a SPI NOR erase region + * @offset: the offset in the data array of erase region start. + * LSB bits are used as a bitmask encoding flags to + * determine if this region is overlaid, if this region is + * the last in the SPI NOR flash memory and to indicate + * all the supported erase commands inside this region. + * The erase types are ordered by size with the biggest + * erase type at BIT(0). + * @size: the size of the region in bytes. + */ +struct spi_nor_erase_region { + u64 offset; + u64 size; +}; + +#define SNOR_CMD_ERASE_MAX 4 +#define SNOR_CMD_ERASE_MASK GENMASK_ULL(SNOR_CMD_ERASE_MAX - 1, 0) + +#define SNOR_LAST_REGION BIT(4) +#define SNOR_OVERLAID_REGION BIT(5) + +#define SNOR_ERASE_FLAGS_MAX 6 +#define SNOR_ERASE_FLAGS_MASK GENMASK_ULL(SNOR_ERASE_FLAGS_MAX - 1, 0) + +#define SNOR_ERASE_FLAGS_OFFSET(_cmd_mask, _last_region, _overlaid_region, \ + _offset) \ + ((((u64)(_offset)) & ~SNOR_ERASE_FLAGS_MASK) | \ + (((u64)(_cmd_mask)) & SNOR_CMD_ERASE_MASK) | \ + (((u64)(_last_region)) & SNOR_LAST_REGION) | \ + (((u64)(_overlaid_region)) & SNOR_OVERLAID_REGION)) + +/** + * struct spi_nor_erase_map - Structure to describe the SPI NOR erase map + * @regions: point to an array describing the boundaries of the erase + * regions. + * @uniform_region: a pre-allocated erase region for SPI NOR with a uniform + * sector size (legacy implementation). + * @commands: an array of erase commands shared by all the regions. + * The erase commands are oredered by size, with the + * biggest erase type command starting at index 0. + * @uniform_erase_type: bitmask encoding erase commands that can erase the + * entire memory. This member is also completed by + * non-uniform erase type SPI NOR flash memories if they + * support at least one erase type command that can erase + * the entire memory. + */ +struct spi_nor_erase_map { + struct spi_nor_erase_region *regions; + struct spi_nor_erase_region uniform_region; + struct spi_nor_erase_command commands[SNOR_CMD_ERASE_MAX]; + u8 uniform_erase_type; +}; + +/** * struct flash_info - Forward declaration of a structure used internally by * spi_nor_scan() */ @@ -261,6 +331,7 @@ struct flash_info; * @write_proto: the SPI protocol for write operations * @reg_proto the SPI protocol for read_reg/write_reg/erase operations * @cmd_buf: used by the write_reg + * @erase_map: the erase map of the SPI NOR * @prepare: [OPTIONAL] do some preparations for the * read/write/erase/lock/unlock operations * @unprepare: [OPTIONAL] do some post work after the @@ -296,6 +367,7 @@ struct spi_nor { bool sst_write_second; u32 flags; u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE]; + struct spi_nor_erase_map erase_map; int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops); void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops); @@ -316,6 +388,23 @@ struct spi_nor { void *priv; }; +#define spi_nor_region_is_last(region) (region->offset & SNOR_LAST_REGION) + +static inline u64 spi_nor_region_end(const struct spi_nor_erase_region *region) +{ + return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size; +} + +static inline void spi_nor_region_mark_end(struct spi_nor_erase_region *region) +{ + region->offset |= SNOR_LAST_REGION; +} + +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor) +{ + return (nor->erase_map.regions == &nor->erase_map.uniform_region); +} + static inline void spi_nor_set_flash_node(struct spi_nor *nor, struct device_node *np) {